Merge pull request #7947 from hashicorp/dnephin/add-linter-staticcheck-3

ci: Enable staticcheck and fix most errors

.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

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)

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()

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",

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

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)
 	}
 

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.

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(
+		matched := re.MatchString(id)
-			"[\\da-f]{8}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{4}-[\\da-f]{12}", id)
+		if !matched {
-		if !matched || err != nil {
 			t.Fatalf("expected match %s %v %s", id, matched, err)
 		}
 	}

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
 

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 {

agent/session_endpoint_test.go

@@ -668,9 +668,11 @@ func TestSessionList(t *testing.T) {
 		if !ok {
 			t.Fatalf("should work")
 		}
-		if len(respObj) != 10 {
+		respIDs := make([]string, 0, len(respObj))
-			t.Fatalf("bad: %v", respObj)
+		for _, obj := range respObj {
+			respIDs = append(respIDs, obj.ID)
 		}
+		require.ElementsMatch(t, respIDs, ids)
 	})
 }
 

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)

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 {

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)
 }

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",

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
+// LookupGatewayProxy 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
+// agent. If exactly one gateway exists it will be returned, otherwise an error
-// there is exactly one gateway of this kind registered to the agent.
+// 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)

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,
+func verifyServerCertMatchesURI(certs []*x509.Certificate, expected connect.CertURI) error {
-	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()) {
+	if strings.EqualFold(gotURI.String(), expectURI.String()) {
-		// OK!
 		return nil
 	}
 

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.

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,
 	}