From c02d4e1390cfdabb90d4a459bab2a30b90d393fb Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 19 May 2020 18:44:06 -0400 Subject: [PATCH] Merge pull request #7894 from hashicorp/dnephin/add-linter-staticcheck-1 Fix some bugs/issues found by staticcheck --- agent/agent_endpoint.go | 2 +- agent/agent_endpoint_test.go | 2 +- agent/cache-types/service_checks_test.go | 4 +- agent/consul/autopilot/autopilot.go | 6 -- agent/consul/leader_routine_manager.go | 2 +- agent/consul/prepared_query/template.go | 6 +- agent/consul/state/txn.go | 73 ++++++++++++------------ agent/coordinate_endpoint_test.go | 8 +-- agent/dns_test.go | 6 +- agent/http.go | 27 ++++----- agent/http_test.go | 8 +-- agent/kvs_endpoint_test.go | 2 +- agent/router/manager.go | 19 +++--- agent/session_endpoint_test.go | 7 ++- agent/ui_endpoint_test.go | 2 +- agent/xds/endpoints.go | 2 +- command/connect/envoy/envoy.go | 2 +- 17 files changed, 87 insertions(+), 91 deletions(-) diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 769d755ed2..6500aa1bd5 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -914,7 +914,7 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re // the catalog endpoint so it helps ensure the sync will work properly. if err := ns.Validate(); err != nil { resp.WriteHeader(http.StatusBadRequest) - fmt.Fprintf(resp, err.Error()) + fmt.Fprint(resp, err.Error()) return nil, nil } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index bfa0ce0df1..e8d995a928 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -3683,7 +3683,7 @@ func testAgent_RegisterServiceDeregisterService_Sidecar(t *testing.T, extraHCL s require.Nil(obj) svcs := a.State.Services(nil) - svc, ok = svcs[structs.NewServiceID(tt.wantNS.ID, nil)] + _, ok = svcs[structs.NewServiceID(tt.wantNS.ID, nil)] if tt.wantSidecarIDLeftAfterDereg { require.True(ok, "removed non-sidecar service at "+tt.wantNS.ID) } else { diff --git a/agent/cache-types/service_checks_test.go b/agent/cache-types/service_checks_test.go index 198c0735ea..f6b5a594de 100644 --- a/agent/cache-types/service_checks_test.go +++ b/agent/cache-types/service_checks_test.go @@ -188,8 +188,8 @@ func (m *mockAgent) LocalState() *local.State { return m.state } -func (m *mockAgent) LocalBlockingQuery(alwaysBlock bool, hash string, wait time.Duration, - fn func(ws memdb.WatchSet) (string, interface{}, error)) (string, interface{}, error) { +func (m *mockAgent) LocalBlockingQuery(_ bool, _ string, _ time.Duration, + _ func(ws memdb.WatchSet) (string, interface{}, error)) (string, interface{}, error) { hash, err := hashChecks(m.checks) if err != nil { diff --git a/agent/consul/autopilot/autopilot.go b/agent/consul/autopilot/autopilot.go index be0937608e..7e0bbc9ce4 100644 --- a/agent/consul/autopilot/autopilot.go +++ b/agent/consul/autopilot/autopilot.go @@ -420,12 +420,6 @@ func (a *Autopilot) updateClusterHealth() error { // consistent of a sample as possible. We capture the leader's index // here as well so it roughly lines up with the same point in time. targetLastIndex := raftNode.LastIndex() - var fetchList []*ServerInfo - for _, server := range servers { - if parts, ok := serverMap[string(server.ID)]; ok { - fetchList = append(fetchList, parts) - } - } d := time.Now().Add(a.healthInterval / 2) ctx, cancel := context.WithDeadline(context.Background(), d) defer cancel() diff --git a/agent/consul/leader_routine_manager.go b/agent/consul/leader_routine_manager.go index c2b056da14..11c23f2f68 100644 --- a/agent/consul/leader_routine_manager.go +++ b/agent/consul/leader_routine_manager.go @@ -48,7 +48,7 @@ func (m *LeaderRoutineManager) IsRunning(name string) bool { } func (m *LeaderRoutineManager) Start(name string, routine LeaderRoutine) error { - return m.StartWithContext(nil, name, routine) + return m.StartWithContext(context.TODO(), name, routine) } func (m *LeaderRoutineManager) StartWithContext(parentCtx context.Context, name string, routine LeaderRoutine) error { diff --git a/agent/consul/prepared_query/template.go b/agent/consul/prepared_query/template.go index c26551c260..466fc143f8 100644 --- a/agent/consul/prepared_query/template.go +++ b/agent/consul/prepared_query/template.go @@ -116,13 +116,9 @@ func (ct *CompiledTemplate) Render(name string, source structs.QuerySource) (*st return nil, fmt.Errorf("Failed to copy query") } - // Run the regular expression, if provided. We execute on a copy here - // to avoid internal lock contention because we expect this to be called - // from multiple goroutines. var matches []string if ct.re != nil { - re := ct.re.Copy() - matches = re.FindStringSubmatch(name) + matches = ct.re.FindStringSubmatch(name) } // Create a safe match function that can't fail at run time. It will diff --git a/agent/consul/state/txn.go b/agent/consul/state/txn.go index c67d6708b1..06f20681e9 100644 --- a/agent/consul/state/txn.go +++ b/agent/consul/state/txn.go @@ -210,61 +210,62 @@ func (s *Store) txnNode(tx *memdb.Txn, idx uint64, op *structs.TxnNodeOp) (struc // txnService handles all Service-related operations. func (s *Store) txnService(tx *memdb.Txn, idx uint64, op *structs.TxnServiceOp) (structs.TxnResults, error) { - var entry *structs.NodeService - var err error - switch op.Verb { case api.ServiceGet: - entry, err = s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) - if entry == nil && err == nil { - err = fmt.Errorf("service %q on node %q doesn't exist", op.Service.ID, op.Node) + entry, err := s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) + switch { + case err != nil: + return nil, err + case entry == nil: + return nil, fmt.Errorf("service %q on node %q doesn't exist", op.Service.ID, op.Node) + default: + return structs.TxnResults{&structs.TxnResult{Service: entry}}, nil } case api.ServiceSet: - err = s.ensureServiceTxn(tx, idx, op.Node, &op.Service) - entry, err = s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) + if err := s.ensureServiceTxn(tx, idx, op.Node, &op.Service); err != nil { + return nil, err + } + entry, err := s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) + return newTxnResultFromNodeServiceEntry(entry), err case api.ServiceCAS: - var ok bool - ok, err = s.ensureServiceCASTxn(tx, idx, op.Node, &op.Service) + ok, err := s.ensureServiceCASTxn(tx, idx, op.Node, &op.Service) + // TODO: err != nil case is ignored if !ok && err == nil { - err = fmt.Errorf("failed to set service %q on node %q, index is stale", op.Service.ID, op.Node) - break + err := fmt.Errorf("failed to set service %q on node %q, index is stale", op.Service.ID, op.Node) + return nil, err } - entry, err = s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) + + entry, err := s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) + return newTxnResultFromNodeServiceEntry(entry), err case api.ServiceDelete: - err = s.deleteServiceTxn(tx, idx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) + err := s.deleteServiceTxn(tx, idx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) + return nil, err case api.ServiceDeleteCAS: - var ok bool - ok, err = s.deleteServiceCASTxn(tx, idx, op.Service.ModifyIndex, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) + ok, err := s.deleteServiceCASTxn(tx, idx, op.Service.ModifyIndex, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) if !ok && err == nil { - err = fmt.Errorf("failed to delete service %q on node %q, index is stale", op.Service.ID, op.Node) + return nil, fmt.Errorf("failed to delete service %q on node %q, index is stale", op.Service.ID, op.Node) } + return nil, err default: - err = fmt.Errorf("unknown Service verb %q", op.Verb) + return nil, fmt.Errorf("unknown Service verb %q", op.Verb) } - if err != nil { - return nil, err +} + +// newTxnResultFromNodeServiceEntry returns a TxnResults with a single result, +// a copy of entry. The entry is copied to prevent modification of the state +// store. +func newTxnResultFromNodeServiceEntry(entry *structs.NodeService) structs.TxnResults { + if entry == nil { + return nil } - - // For a GET we keep the value, otherwise we clone and blank out the - // value (we have to clone so we don't modify the entry being used by - // the state store). - if entry != nil { - if op.Verb == api.ServiceGet { - result := structs.TxnResult{Service: entry} - return structs.TxnResults{&result}, nil - } - - clone := *entry - result := structs.TxnResult{Service: &clone} - return structs.TxnResults{&result}, nil - } - - return nil, nil + clone := *entry + result := structs.TxnResult{Service: &clone} + return structs.TxnResults{&result} } // txnCheck handles all Check-related operations. diff --git a/agent/coordinate_endpoint_test.go b/agent/coordinate_endpoint_test.go index c6263c98b1..19fff8f19f 100644 --- a/agent/coordinate_endpoint_test.go +++ b/agent/coordinate_endpoint_test.go @@ -236,7 +236,7 @@ func TestCoordinate_Node(t *testing.T) { // Make sure we get a 404 with no coordinates. req, _ := http.NewRequest("GET", "/v1/coordinate/node/foo?dc=dc1", nil) resp := httptest.NewRecorder() - obj, err := a.srv.CoordinateNode(resp, req) + _, err := a.srv.CoordinateNode(resp, req) if err != nil { t.Fatalf("err: %v", err) } @@ -285,7 +285,7 @@ func TestCoordinate_Node(t *testing.T) { // Query back and check the nodes are present. req, _ = http.NewRequest("GET", "/v1/coordinate/node/foo?dc=dc1", nil) resp = httptest.NewRecorder() - obj, err = a.srv.CoordinateNode(resp, req) + obj, err := a.srv.CoordinateNode(resp, req) if err != nil { t.Fatalf("err: %v", err) } @@ -303,7 +303,7 @@ func TestCoordinate_Node(t *testing.T) { // Filter on a nonexistent node segment req, _ = http.NewRequest("GET", "/v1/coordinate/node/foo?segment=nope", nil) resp = httptest.NewRecorder() - obj, err = a.srv.CoordinateNode(resp, req) + _, err = a.srv.CoordinateNode(resp, req) if err != nil { t.Fatalf("err: %v", err) } @@ -331,7 +331,7 @@ func TestCoordinate_Node(t *testing.T) { // Make sure the empty filter works req, _ = http.NewRequest("GET", "/v1/coordinate/node/foo?segment=", nil) resp = httptest.NewRecorder() - obj, err = a.srv.CoordinateNode(resp, req) + _, err = a.srv.CoordinateNode(resp, req) if err != nil { t.Fatalf("err: %v", err) } diff --git a/agent/dns_test.go b/agent/dns_test.go index a9372da8eb..9d20ab3133 100644 --- a/agent/dns_test.go +++ b/agent/dns_test.go @@ -116,11 +116,11 @@ func TestRecursorAddr(t *testing.T) { if addr != "[2001:4860:4860::8888]:53" { t.Fatalf("bad: %v", addr) } - addr, err = recursorAddr("1.2.3.4::53") + _, err = recursorAddr("1.2.3.4::53") if err == nil || !strings.Contains(err.Error(), "too many colons in address") { t.Fatalf("err: %v", err) } - addr, err = recursorAddr("2001:4860:4860::8888:::53") + _, err = recursorAddr("2001:4860:4860::8888:::53") if err == nil || !strings.Contains(err.Error(), "too many colons in address") { t.Fatalf("err: %v", err) } @@ -282,7 +282,7 @@ func TestDNS_NodeLookup(t *testing.T) { require.Equal(t, "127.0.0.1", aRec.A.String()) require.Equal(t, uint32(0), aRec.Hdr.Ttl) - txt, ok = in.Answer[1].(*dns.TXT) + _, ok = in.Answer[1].(*dns.TXT) require.True(t, ok, "Second answer is not a TXT record") // lookup a non-existing node, we should receive a SOA diff --git a/agent/http.go b/agent/http.go index c52c232f1c..099e38a563 100644 --- a/agent/http.go +++ b/agent/http.go @@ -169,20 +169,21 @@ type templatedIndexFS struct { func (fs *templatedIndexFS) Open(name string) (http.File, error) { file, err := fs.fs.Open(name) - if err == nil && name == "/index.html" { - content, _ := ioutil.ReadAll(file) - file.Seek(0, 0) - t, err := template.New("fmtedindex").Parse(string(content)) - if err != nil { - return nil, err - } - var out bytes.Buffer - err = t.Execute(&out, fs.templateVars()) - - file = newTemplatedFile(&out, file) + if err != nil || name != "/index.html" { + return file, err } - return file, err + content, _ := ioutil.ReadAll(file) + file.Seek(0, 0) + t, err := template.New("fmtedindex").Parse(string(content)) + if err != nil { + return nil, err + } + var out bytes.Buffer + if err := t.Execute(&out, fs.templateVars()); err != nil { + return nil, err + } + return newTemplatedFile(&out, file), nil } // endpoint is a Consul-specific HTTP handler that takes the usual arguments in @@ -496,7 +497,7 @@ func (s *HTTPServer) wrap(handler endpoint, methods []string) http.HandlerFunc { fmt.Fprint(resp, err.Error()) case isNotFound(err): resp.WriteHeader(http.StatusNotFound) - fmt.Fprintf(resp, err.Error()) + fmt.Fprint(resp, err.Error()) case isTooManyRequests(err): resp.WriteHeader(http.StatusTooManyRequests) fmt.Fprint(resp, err.Error()) diff --git a/agent/http_test.go b/agent/http_test.go index 27c84ba426..759e7fd237 100644 --- a/agent/http_test.go +++ b/agent/http_test.go @@ -323,11 +323,11 @@ func TestHTTPAPI_Ban_Nonprintable_Characters(t *testing.T) { a := NewTestAgent(t, "") defer a.Shutdown() - req, err := http.NewRequest("GET", "/v1/kv/bad\x00ness", nil) + _, err := http.NewRequest("GET", "/v1/kv/bad\x00ness", nil) if err == nil { t.Fatal("expected error") } - req, err = http.NewRequest("GET", "/v1/kv/bad%00ness", nil) + req, err := http.NewRequest("GET", "/v1/kv/bad%00ness", nil) if err != nil { t.Fatal(err) } @@ -342,11 +342,11 @@ func TestHTTPAPI_Allow_Nonprintable_Characters_With_Flag(t *testing.T) { a := NewTestAgent(t, "disable_http_unprintable_char_filter = true") defer a.Shutdown() - req, err := http.NewRequest("GET", "/v1/kv/bad\x00ness", nil) + _, err := http.NewRequest("GET", "/v1/kv/bad\x00ness", nil) if err == nil { t.Fatal("expected error") } - req, err = http.NewRequest("GET", "/v1/kv/bad%00ness", nil) + req, err := http.NewRequest("GET", "/v1/kv/bad%00ness", nil) if err != nil { t.Fatal(err) } diff --git a/agent/kvs_endpoint_test.go b/agent/kvs_endpoint_test.go index bfaeda202e..ceb6d907f1 100644 --- a/agent/kvs_endpoint_test.go +++ b/agent/kvs_endpoint_test.go @@ -416,7 +416,7 @@ func TestKVSEndpoint_GET_Raw(t *testing.T) { req, _ = http.NewRequest("GET", "/v1/kv/test?raw", nil) resp = httptest.NewRecorder() - obj, err = a.srv.KVSEndpoint(resp, req) + _, err = a.srv.KVSEndpoint(resp, req) if err != nil { t.Fatalf("err: %v", err) } diff --git a/agent/router/manager.go b/agent/router/manager.go index 2a7af2ebf1..7944e48d26 100644 --- a/agent/router/manager.go +++ b/agent/router/manager.go @@ -378,16 +378,17 @@ func (m *Manager) RebalanceServers() { "number_of_servers", len(l.servers), "active_server", l.servers[0].String(), ) - } else { - // reconcileServerList failed because Serf removed the server - // that was at the front of the list that had successfully - // been Ping'ed. Between the Ping and reconcile, a Serf - // event had shown up removing the node. - // - // Instead of doing any heroics, "freeze in place" and - // continue to use the existing connection until the next - // rebalance occurs. } + // else { + // reconcileServerList failed because Serf removed the server + // that was at the front of the list that had successfully + // been Ping'ed. Between the Ping and reconcile, a Serf + // event had shown up removing the node. + // + // Instead of doing any heroics, "freeze in place" and + // continue to use the existing connection until the next + // rebalance occurs. + // } } // reconcileServerList returns true when the first server in serverList diff --git a/agent/session_endpoint_test.go b/agent/session_endpoint_test.go index 22f23d92b3..faf86f895e 100644 --- a/agent/session_endpoint_test.go +++ b/agent/session_endpoint_test.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/consul/types" + "github.com/stretchr/testify/require" ) func verifySession(t *testing.T, r *retry.R, a *TestAgent, want structs.Session) { @@ -715,9 +716,11 @@ func TestSessionsForNode(t *testing.T) { if !ok { t.Fatalf("should work") } - if len(respObj) != 10 { - t.Fatalf("bad: %v", respObj) + respIDs := make([]string, 0, len(ids)) + for _, session := range respObj { + respIDs = append(respIDs, session.ID) } + require.ElementsMatch(t, ids, respIDs) }) } diff --git a/agent/ui_endpoint_test.go b/agent/ui_endpoint_test.go index 153e86fd21..3e9c5540c8 100644 --- a/agent/ui_endpoint_test.go +++ b/agent/ui_endpoint_test.go @@ -36,7 +36,7 @@ func TestUiIndex(t *testing.T) { // Create file path := filepath.Join(a.Config.UIDir, "my-file") - if err := ioutil.WriteFile(path, []byte("test"), 777); err != nil { + if err := ioutil.WriteFile(path, []byte("test"), 0777); err != nil { t.Fatalf("err: %v", err) } diff --git a/agent/xds/endpoints.go b/agent/xds/endpoints.go index 6af502b218..b96934d5bb 100644 --- a/agent/xds/endpoints.go +++ b/agent/xds/endpoints.go @@ -3,6 +3,7 @@ package xds import ( "errors" "fmt" + envoy "github.com/envoyproxy/go-control-plane/envoy/api/v2" envoycore "github.com/envoyproxy/go-control-plane/envoy/api/v2/core" envoyendpoint "github.com/envoyproxy/go-control-plane/envoy/api/v2/endpoint" @@ -324,7 +325,6 @@ func (s *Server) endpointsFromDiscoveryChain( ) if actualTargetID != targetID { targetID = actualTargetID - target = chain.Targets[actualTargetID] } failover = nil diff --git a/command/connect/envoy/envoy.go b/command/connect/envoy/envoy.go index 48b93ee2bf..2f0ad30b48 100644 --- a/command/connect/envoy/envoy.go +++ b/command/connect/envoy/envoy.go @@ -552,7 +552,7 @@ func (c *cmd) grpcAddress(httpCfg *api.Config) (GRPC, error) { } else { // Parse as host:port with option http prefix grpcAddr = strings.TrimPrefix(addr, "http://") - grpcAddr = strings.TrimPrefix(addr, "https://") + grpcAddr = strings.TrimPrefix(grpcAddr, "https://") var err error var host string