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

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