From eee3a2e785010e2e5b503b6a82642c6f2b68d072 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Sat, 21 Aug 2021 10:15:54 -0400 Subject: [PATCH] Fix code path that could cause panic --- http/service.go | 33 +++++++++++++++++++++++++++------ http/service_test.go | 22 +++++++++++++++++++++- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/http/service.go b/http/service.go index 75696315..2110f694 100644 --- a/http/service.go +++ b/http/service.go @@ -26,6 +26,11 @@ import ( "github.com/rqlite/rqlite/store" ) +var ( + // ErrLeaderNotFound is returned when a node cannot locate a leader + ErrLeaderNotFound = errors.New("leader not found") +) + // Database is the interface any queryable system must implement type Database interface { // Execute executes a slice of queries, each of which is not expected @@ -126,6 +131,7 @@ type Response struct { var stats *expvar.Map const ( + numLeaderNotFound = "leader_not_found" numExecutions = "executions" numQueries = "queries" numRemoteExecutions = "remote_executions" @@ -167,6 +173,7 @@ const ( func init() { stats = expvar.NewMap("http") + stats.Add(numLeaderNotFound, 0) stats.Add(numExecutions, 0) stats.Add(numQueries, 0) stats.Add(numRemoteExecutions, 0) @@ -375,7 +382,8 @@ func (s *Service) handleJoin(w http.ResponseWriter, r *http.Request) { if err == store.ErrNotLeader { leaderAPIAddr := s.LeaderAPIAddr() if leaderAPIAddr == "" { - http.Error(w, err.Error(), http.StatusServiceUnavailable) + stats.Add(numLeaderNotFound, 1) + http.Error(w, ErrLeaderNotFound.Error(), http.StatusServiceUnavailable) return } @@ -427,7 +435,8 @@ func (s *Service) handleRemove(w http.ResponseWriter, r *http.Request) { if err == store.ErrNotLeader { leaderAPIAddr := s.LeaderAPIAddr() if leaderAPIAddr == "" { - http.Error(w, err.Error(), http.StatusServiceUnavailable) + stats.Add(numLeaderNotFound, 1) + http.Error(w, ErrLeaderNotFound.Error(), http.StatusServiceUnavailable) return } @@ -470,7 +479,8 @@ func (s *Service) handleBackup(w http.ResponseWriter, r *http.Request) { if err == store.ErrNotLeader { leaderAPIAddr := s.LeaderAPIAddr() if leaderAPIAddr == "" { - http.Error(w, err.Error(), http.StatusServiceUnavailable) + stats.Add(numLeaderNotFound, 1) + http.Error(w, ErrLeaderNotFound.Error(), http.StatusServiceUnavailable) return } @@ -522,7 +532,8 @@ func (s *Service) handleLoad(w http.ResponseWriter, r *http.Request) { if err == store.ErrNotLeader { leaderAPIAddr := s.LeaderAPIAddr() if leaderAPIAddr == "" { - http.Error(w, err.Error(), http.StatusServiceUnavailable) + stats.Add(numLeaderNotFound, 1) + http.Error(w, ErrLeaderNotFound.Error(), http.StatusServiceUnavailable) return } @@ -767,7 +778,8 @@ func (s *Service) handleExecute(w http.ResponseWriter, r *http.Request) { if redirect { leaderAPIAddr := s.LeaderAPIAddr() if leaderAPIAddr == "" { - http.Error(w, err.Error(), http.StatusServiceUnavailable) + stats.Add(numLeaderNotFound, 1) + http.Error(w, ErrLeaderNotFound.Error(), http.StatusServiceUnavailable) return } loc := s.FormRedirect(r, leaderAPIAddr) @@ -779,6 +791,10 @@ func (s *Service) handleExecute(w http.ResponseWriter, r *http.Request) { if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) } + if addr == "" { + stats.Add(numLeaderNotFound, 1) + http.Error(w, ErrLeaderNotFound.Error(), http.StatusServiceUnavailable) + } results, resultsErr = s.cluster.Execute(er, addr, timeout) stats.Add(numRemoteExecutions, 1) w.Header().Add(ServedByHTTPHeader, addr) @@ -849,7 +865,8 @@ func (s *Service) handleQuery(w http.ResponseWriter, r *http.Request) { if redirect { leaderAPIAddr := s.LeaderAPIAddr() if leaderAPIAddr == "" { - http.Error(w, err.Error(), http.StatusServiceUnavailable) + stats.Add(numLeaderNotFound, 1) + http.Error(w, ErrLeaderNotFound.Error(), http.StatusServiceUnavailable) return } loc := s.FormRedirect(r, leaderAPIAddr) @@ -861,6 +878,10 @@ func (s *Service) handleQuery(w http.ResponseWriter, r *http.Request) { if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) } + if addr == "" { + stats.Add(numLeaderNotFound, 1) + http.Error(w, ErrLeaderNotFound.Error(), http.StatusServiceUnavailable) + } results, resultsErr = s.cluster.Query(qr, addr, timeout) stats.Add(numRemoteQueries, 1) w.Header().Add(ServedByHTTPHeader, addr) diff --git a/http/service_test.go b/http/service_test.go index da47c484..6915976d 100644 --- a/http/service_test.go +++ b/http/service_test.go @@ -677,7 +677,7 @@ func Test_Nodes(t *testing.T) { } } -func Test_ForwardingRedirectQueries(t *testing.T) { +func Test_ForwardingRedirectQuery(t *testing.T) { m := &MockStore{ leaderAddr: "foo:1234", } @@ -725,6 +725,16 @@ func Test_ForwardingRedirectQueries(t *testing.T) { if resp.StatusCode != http.StatusMovedPermanently { t.Fatalf("failed to get expected StatusMovedPermanently for query, got %d", resp.StatusCode) } + + // Check leader failure case. + m.leaderAddr = "" + resp, err = client.Get(host + "/db/query?pretty&timings&q=SELECT%20%2A%20FROM%20foo") + if err != nil { + t.Fatalf("failed to make query forwarded request") + } + if resp.StatusCode != http.StatusServiceUnavailable { + t.Fatalf("failed to get expected StatusServiceUnavailable for node with no leader, got %d", resp.StatusCode) + } } func Test_ForwardingRedirectExecute(t *testing.T) { @@ -775,6 +785,16 @@ func Test_ForwardingRedirectExecute(t *testing.T) { if resp.StatusCode != http.StatusMovedPermanently { t.Fatalf("failed to get expected StatusMovedPermanently for execute, got %d", resp.StatusCode) } + + // Check leader failure case. + m.leaderAddr = "" + resp, err = client.Post(host+"/db/execute", "application/json", strings.NewReader(`["Some SQL"]`)) + if err != nil { + t.Fatalf("failed to make execute request") + } + if resp.StatusCode != http.StatusServiceUnavailable { + t.Fatalf("failed to get expected StatusServiceUnavailable for node with no leader, got %d", resp.StatusCode) + } } func Test_TLSServce(t *testing.T) {