From c88fae0aac2af321001170c65169be5c07ca36a4 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 14 May 2020 17:02:52 -0400 Subject: [PATCH] ci: Add staticcheck and fix most errors Three of the checks are temporarily disabled to limit the size of the diff, and allow us to enable all the other checks in CI. In a follow up we can fix the issues reported by the other checks one at a time, and enable them. --- .golangci.yml | 15 +++++++++++++++ agent/acl_endpoint_legacy_test.go | 3 +-- agent/agent.go | 2 +- agent/config/builder.go | 1 + agent/consul/client_test.go | 2 -- agent/consul/fsm/commands_oss_test.go | 2 -- agent/consul/stats_fetcher_test.go | 6 ++---- agent/consul/util_test.go | 8 +++++--- agent/proxycfg/state.go | 1 + agent/router/manager_test.go | 2 -- agent/session_endpoint_test.go | 6 ++++-- agent/structs/intention.go | 1 + api/connect_ca_test.go | 2 +- command/acl/token/list/token_list_test.go | 8 +++++++- command/acl/token/update/token_update_test.go | 2 +- command/connect/proxy/proxy.go | 11 +++-------- connect/tls.go | 6 ++---- snapshot/snapshot_test.go | 2 -- tlsutil/generate.go | 2 +- 19 files changed, 46 insertions(+), 36 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 882771a5ce..9479540d61 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -4,12 +4,27 @@ linters: - gofmt - govet - unconvert + - staticcheck issues: # Disable the default exclude list so that all excludes are explicitly # defined in this file. exclude-use-default: false + exclude-rules: + # Temp Ignore SA9004: only the first constant in this group has an explicit type + # https://staticcheck.io/docs/checks#SA9004 + - linters: [staticcheck] + text: "SA9004:" + + # Temp ignore SA4006: this value of `x` is never used + - linters: [staticcheck] + text: "SA4006:" + + # Temp ignore SA2002: the goroutine calls T.Fatalf, which must be called in the same goroutine as the test + - linters: [staticcheck] + text: "SA2002:" + linters-settings: gofmt: simplify: false diff --git a/agent/acl_endpoint_legacy_test.go b/agent/acl_endpoint_legacy_test.go index cade0078a8..95a63e558b 100644 --- a/agent/acl_endpoint_legacy_test.go +++ b/agent/acl_endpoint_legacy_test.go @@ -255,9 +255,8 @@ func TestACL_Legacy_List(t *testing.T) { defer a.Shutdown() testrpc.WaitForLeader(t, a.RPC, "dc1") - var ids []string for i := 0; i < 10; i++ { - ids = append(ids, makeTestACL(t, a.srv)) + makeTestACL(t, a.srv) } req, _ := http.NewRequest("GET", "/v1/acl/list?token=root", nil) diff --git a/agent/agent.go b/agent/agent.go index 8e419e3b93..cbe5cc2c05 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -832,7 +832,7 @@ func (a *Agent) listenAndServeDNS() error { merr = multierror.Append(merr, err) case <-timeout: merr = multierror.Append(merr, fmt.Errorf("agent: timeout starting DNS servers")) - break + return merr.ErrorOrNil() } } return merr.ErrorOrNil() diff --git a/agent/config/builder.go b/agent/config/builder.go index 42c277974c..6ab53e1bcb 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -627,6 +627,7 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { return RuntimeConfig{}, fmt.Errorf("'connect.enable_mesh_gateway_wan_federation=true' requires 'connect.enabled=true'") } if connectCAConfig != nil { + // nolint: staticcheck // CA config should be changed to use HookTranslateKeys lib.TranslateKeys(connectCAConfig, map[string]string{ // Consul CA config "private_key": "PrivateKey", diff --git a/agent/consul/client_test.go b/agent/consul/client_test.go index 8b037b18b4..3d9267a330 100644 --- a/agent/consul/client_test.go +++ b/agent/consul/client_test.go @@ -380,7 +380,6 @@ func TestClient_RPC_Pool(t *testing.T) { func TestClient_RPC_ConsulServerPing(t *testing.T) { t.Parallel() var servers []*Server - var serverDirs []string const numServers = 5 for n := 0; n < numServers; n++ { @@ -390,7 +389,6 @@ func TestClient_RPC_ConsulServerPing(t *testing.T) { defer s.Shutdown() servers = append(servers, s) - serverDirs = append(serverDirs, dir) } const numClients = 1 diff --git a/agent/consul/fsm/commands_oss_test.go b/agent/consul/fsm/commands_oss_test.go index 62db910a36..68c4103d7d 100644 --- a/agent/consul/fsm/commands_oss_test.go +++ b/agent/consul/fsm/commands_oss_test.go @@ -1487,7 +1487,6 @@ func TestFSM_Chunking_Lifecycle(t *testing.T) { require.NoError(err) var logOfLogs [][]*raft.Log - var bufs [][]byte for i := 0; i < 10; i++ { req := structs.RegisterRequest{ Datacenter: "dc1", @@ -1527,7 +1526,6 @@ func TestFSM_Chunking_Lifecycle(t *testing.T) { Extensions: chunkBytes, }) } - bufs = append(bufs, buf) logOfLogs = append(logOfLogs, logs) } diff --git a/agent/consul/stats_fetcher_test.go b/agent/consul/stats_fetcher_test.go index 0148e66577..beba30424b 100644 --- a/agent/consul/stats_fetcher_test.go +++ b/agent/consul/stats_fetcher_test.go @@ -36,13 +36,11 @@ func TestStatsFetcher(t *testing.T) { t.Fatalf("bad len: %d", len(members)) } - var servers []*metadata.Server for _, member := range members { - ok, server := metadata.IsConsulServer(member) + ok, _ := metadata.IsConsulServer(member) if !ok { - t.Fatalf("bad: %#v", member) + t.Fatalf("expected member to be a server: %#v", member) } - servers = append(servers, server) } // Do a normal fetch and make sure we get three responses. diff --git a/agent/consul/util_test.go b/agent/consul/util_test.go index 84919c5bcf..3307ada8ba 100644 --- a/agent/consul/util_test.go +++ b/agent/consul/util_test.go @@ -229,15 +229,17 @@ func TestByteConversion(t *testing.T) { func TestGenerateUUID(t *testing.T) { t.Parallel() prev := generateUUID() + re, err := regexp.Compile("[\\da-f]{8}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{12}") + require.NoError(t, err) + for i := 0; i < 100; i++ { id := generateUUID() if prev == id { t.Fatalf("Should get a new ID!") } - matched, err := regexp.MatchString( - "[\\da-f]{8}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{12}", id) - if !matched || err != nil { + matched := re.MatchString(id) + if !matched { t.Fatalf("expected match %s %v %s", id, matched, err) } } diff --git a/agent/proxycfg/state.go b/agent/proxycfg/state.go index 740024e5d3..1cd703eb52 100644 --- a/agent/proxycfg/state.go +++ b/agent/proxycfg/state.go @@ -1110,6 +1110,7 @@ func (s *state) handleUpdateTerminatingGateway(u cache.UpdateEvent, snap *Config } } + // nolint: staticcheck // github.com/dominikh/go-tools/issues/580 case strings.HasPrefix(u.CorrelationID, serviceIntentionsIDPrefix): // no-op: Intentions don't get stored in the snapshot, calls to ConnectAuthorize will fetch them from the cache diff --git a/agent/router/manager_test.go b/agent/router/manager_test.go index 3b99bfe654..b91a3ab79f 100644 --- a/agent/router/manager_test.go +++ b/agent/router/manager_test.go @@ -354,12 +354,10 @@ func TestManager_RemoveServer(t *testing.T) { m.AddServer(s2) const maxServers = 19 - servers := make([]*metadata.Server, maxServers) // Already added two servers above for i := maxServers; i > 2; i-- { nodeName := fmt.Sprintf(nodeNameFmt, i) server := &metadata.Server{Name: nodeName} - servers = append(servers, server) m.AddServer(server) } if m.NumServers() != maxServers { diff --git a/agent/session_endpoint_test.go b/agent/session_endpoint_test.go index faf86f895e..8dea91af39 100644 --- a/agent/session_endpoint_test.go +++ b/agent/session_endpoint_test.go @@ -668,9 +668,11 @@ func TestSessionList(t *testing.T) { if !ok { t.Fatalf("should work") } - if len(respObj) != 10 { - t.Fatalf("bad: %v", respObj) + respIDs := make([]string, 0, len(respObj)) + for _, obj := range respObj { + respIDs = append(respIDs, obj.ID) } + require.ElementsMatch(t, respIDs, ids) }) } diff --git a/agent/structs/intention.go b/agent/structs/intention.go index 4e5ef42dd3..3dae4eda38 100644 --- a/agent/structs/intention.go +++ b/agent/structs/intention.go @@ -103,6 +103,7 @@ func (t *Intention) UnmarshalJSON(data []byte) (err error) { return nil } +// nolint: staticcheck // should be fixed in https://github.com/hashicorp/consul/pull/7900 func (x *Intention) SetHash(force bool) []byte { if force || x.Hash == nil { hash, err := blake2b.New256(nil) diff --git a/api/connect_ca_test.go b/api/connect_ca_test.go index 8ae494b60f..82bbcedfb5 100644 --- a/api/connect_ca_test.go +++ b/api/connect_ca_test.go @@ -40,7 +40,7 @@ func TestAPI_ConnectCARoots_list(t *testing.T) { connect := c.Connect() list, meta, err := connect.CARoots(nil) r.Check(err) - if meta.LastIndex <= 0 { + if meta.LastIndex == 0 { r.Fatalf("expected roots raft index to be > 0") } if v := len(list.Roots); v != 1 { diff --git a/command/acl/token/list/token_list_test.go b/command/acl/token/list/token_list_test.go index 3cb93da052..fe3d9da43f 100644 --- a/command/acl/token/list/token_list_test.go +++ b/command/acl/token/list/token_list_test.go @@ -126,7 +126,13 @@ func TestTokenListCommand_JSON(t *testing.T) { assert.Equal(code, 0) assert.Empty(ui.ErrorWriter.String()) - var jsonOutput json.RawMessage + var jsonOutput []api.ACLTokenListEntry err := json.Unmarshal([]byte(ui.OutputWriter.String()), &jsonOutput) require.NoError(t, err, "token unmarshalling error") + + respIDs := make([]string, 0, len(jsonOutput)) + for _, obj := range jsonOutput { + respIDs = append(respIDs, obj.AccessorID) + } + require.Subset(t, respIDs, tokenIds) } diff --git a/command/acl/token/update/token_update_test.go b/command/acl/token/update/token_update_test.go index 329bba271e..c4881af572 100644 --- a/command/acl/token/update/token_update_test.go +++ b/command/acl/token/update/token_update_test.go @@ -64,7 +64,7 @@ func TestTokenUpdateCommand(t *testing.T) { ) req.NoError(err) - // create a legacy token + // nolint: staticcheck // we want the deprecated legacy token legacyTokenSecretID, _, err := client.ACL().Create(&api.ACLEntry{ Name: "Legacy token", Type: "client", diff --git a/command/connect/proxy/proxy.go b/command/connect/proxy/proxy.go index 0d74a3f9a2..5d90410f83 100644 --- a/command/connect/proxy/proxy.go +++ b/command/connect/proxy/proxy.go @@ -244,20 +244,15 @@ func LookupProxyIDForSidecar(client *api.Client, sidecarFor string) (string, err return proxyIDs[0], nil } -// LookupGatewayProxyID finds the gateway service registered with the local -// agent if any and returns its service ID. It will return an ID if and only if -// there is exactly one gateway of this kind registered to the agent. +// LookupGatewayProxy finds the gateway service registered with the local +// agent. If exactly one gateway exists it will be returned, otherwise an error +// is returned. func LookupGatewayProxy(client *api.Client, kind api.ServiceKind) (*api.AgentService, error) { svcs, err := client.Agent().ServicesWithFilter(fmt.Sprintf("Kind == `%s`", kind)) if err != nil { return nil, fmt.Errorf("Failed looking up %s instances: %v", kind, err) } - var proxyIDs []string - for _, svc := range svcs { - proxyIDs = append(proxyIDs, svc.ID) - } - switch len(svcs) { case 0: return nil, fmt.Errorf("No %s services registered with this agent", kind) diff --git a/connect/tls.go b/connect/tls.go index 07a5e32054..e465d3a10a 100644 --- a/connect/tls.go +++ b/connect/tls.go @@ -177,8 +177,7 @@ func extractCertURI(certs []*x509.Certificate) (*url.URL, error) { // verifyServerCertMatchesURI is used on tls connections dialed to a connect // server to ensure that the certificate it presented has the correct identity. -func verifyServerCertMatchesURI(certs []*x509.Certificate, - expected connect.CertURI) error { +func verifyServerCertMatchesURI(certs []*x509.Certificate, expected connect.CertURI) error { expectedStr := expected.URI().String() gotURI, err := extractCertURI(certs) @@ -192,8 +191,7 @@ func verifyServerCertMatchesURI(certs []*x509.Certificate, // domains. expectURI := expected.URI() expectURI.Host = gotURI.Host - if strings.ToLower(gotURI.String()) == strings.ToLower(expectURI.String()) { - // OK! + if strings.EqualFold(gotURI.String(), expectURI.String()) { return nil } diff --git a/snapshot/snapshot_test.go b/snapshot/snapshot_test.go index 6d2d75c0f7..86e8662589 100644 --- a/snapshot/snapshot_test.go +++ b/snapshot/snapshot_test.go @@ -239,7 +239,6 @@ func TestSnapshot_TruncatedVerify(t *testing.T) { // Make a Raft and populate it with some data. We tee everything we // apply off to a buffer for checking post-snapshot. - var expected []bytes.Buffer entries := 64 * 1024 before, _ := makeRaft(t, filepath.Join(dir, "before")) defer before.Shutdown() @@ -253,7 +252,6 @@ func TestSnapshot_TruncatedVerify(t *testing.T) { future := before.Apply(log.Bytes(), time.Second) require.NoError(t, future.Error()) - expected = append(expected, copy) } // Take a snapshot. diff --git a/tlsutil/generate.go b/tlsutil/generate.go index bd4b727d4e..2157386890 100644 --- a/tlsutil/generate.go +++ b/tlsutil/generate.go @@ -191,7 +191,7 @@ func Verify(caString, certString, dns string) error { } opts := x509.VerifyOptions{ - DNSName: fmt.Sprintf(dns), + DNSName: fmt.Sprint(dns), Roots: roots, }