Merge pull request hashicorp#8034 from hashicorp/dnephin/add-linter-s…

…taticcheck-4

ci: enable SA4006 staticcheck check and add ineffassign

.golangci.yml

@@ -5,6 +5,7 @@ linters:
     - govet
     - unconvert
     - staticcheck
+    - ineffassign
 
 issues:
   # Disable the default exclude list so that all excludes are explicitly
@@ -17,10 +18,6 @@ issues:
     - linters: [staticcheck]
       text: "SA9004:"
 
-    # Temp ignore SA4006: this value of `x` is never used
-    - linters: [staticcheck]
-      text: "SA4006:"
-
 linters-settings:
   gofmt:
     simplify: true

agent/agent.go

@@ -706,18 +706,17 @@ func (a *Agent) setupClientAutoEncryptWatching(rootsReq *structs.DCSpecificReque
 	// in time. The agent would be stuck in that case because the watches
 	// never use the AutoEncrypt.Sign endpoint.
 	go func() {
+		// Check 10sec after cert expires. The agent cache
+		// should be handling the expiration and renew before
+		// it.
+		// If there is no cert, AutoEncryptCertNotAfter returns
+		// a value in the past which immediately triggers the
+		// renew, but this case shouldn't happen because at
+		// this point, auto_encrypt was just being setup
+		// successfully.
+		interval := a.tlsConfigurator.AutoEncryptCertNotAfter().Sub(time.Now().Add(10 * time.Second))
+		autoLogger := a.logger.Named(logging.AutoEncrypt)
 		for {
-
-			// Check 10sec after cert expires. The agent cache
-			// should be handling the expiration and renew before
-			// it.
-			// If there is no cert, AutoEncryptCertNotAfter returns
-			// a value in the past which immediately triggers the
-			// renew, but this case shouldn't happen because at
-			// this point, auto_encrypt was just being setup
-			// successfully.
-			autoLogger := a.logger.Named(logging.AutoEncrypt)
-			interval := a.tlsConfigurator.AutoEncryptCertNotAfter().Sub(time.Now().Add(10 * time.Second))
 			a.logger.Debug("setting up client certificate expiration check on interval", "interval", interval)
 			select {
 			case <-a.shutdownCh:

agent/cache-types/catalog_datacenters_test.go

@@ -54,33 +54,28 @@ func TestCatalogDatacenters(t *testing.T) {
 
 	// Fetch first time
 	result, err := typ.Fetch(cache.FetchOptions{}, &structs.DatacentersRequest{})
-	result2, err := typ.Fetch(cache.FetchOptions{LastResult: &result}, &structs.DatacentersRequest{QueryOptions: structs.QueryOptions{MustRevalidate: true}})
-	result3, err := typ.Fetch(cache.FetchOptions{LastResult: &result2}, &structs.DatacentersRequest{QueryOptions: structs.QueryOptions{MustRevalidate: true}})
-
-	// make sure it was called the right number of times
-	rpc.AssertExpectations(t)
-
-	// make sure the first result was correct
 	require.NoError(t, err)
 	require.Equal(t, result, cache.FetchResult{
 		Value: resp,
 		Index: 1,
 	})
 
-	// validate the second result
+	result2, err := typ.Fetch(cache.FetchOptions{LastResult: &result}, &structs.DatacentersRequest{QueryOptions: structs.QueryOptions{MustRevalidate: true}})
 	require.NoError(t, err)
 	require.Equal(t, result2, cache.FetchResult{
 		Value: resp2,
 		Index: 2,
 	})
 
-	// validate the third result
+	result3, err := typ.Fetch(cache.FetchOptions{LastResult: &result2}, &structs.DatacentersRequest{QueryOptions: structs.QueryOptions{MustRevalidate: true}})
 	require.NoError(t, err)
 	require.Equal(t, result3, cache.FetchResult{
 		Value: resp3,
 		Index: 3,
 	})
 
+	// make sure it was called the right number of times
+	rpc.AssertExpectations(t)
 }
 
 func TestDatacenters_badReqType(t *testing.T) {

agent/cache-types/service_checks.go

@@ -76,6 +76,9 @@ func (c *ServiceHTTPChecks) Fetch(opts cache.FetchOptions, req cache.Request) (c
 			return hash, reply, nil
 		},
 	)
+	if err != nil {
+		return result, err
+	}
 
 	result.Value = resp
 

agent/consul/acl_endpoint_test.go

@@ -85,6 +85,7 @@ func TestACLEndpoint_BootstrapTokens(t *testing.T) {
 	require.True(t, strings.HasPrefix(err.Error(), structs.ACLBootstrapNotAllowedErr.Error()))
 
 	_, resetIdx, err := srv.fsm.State().CanBootstrapACLToken()
+	require.NoError(t, err)
 
 	resetPath := filepath.Join(dir, "acl-bootstrap-reset")
 	require.NoError(t, ioutil.WriteFile(resetPath, []byte(fmt.Sprintf("%d", resetIdx)), 0600))
@@ -1692,6 +1693,7 @@ func TestACLEndpoint_TokenSet_anon(t *testing.T) {
 	require.NotEmpty(t, token.SecretID)
 
 	tokenResp, err := retrieveTestToken(codec, TestDefaultMasterToken, "dc1", structs.ACLTokenAnonymousID)
+	require.NoError(t, err)
 	require.Equal(t, len(tokenResp.Token.Policies), 1)
 	require.Equal(t, tokenResp.Token.Policies[0].ID, policy.ID)
 
@@ -1900,6 +1902,7 @@ func TestACLEndpoint_TokenDelete_anon(t *testing.T) {
 
 	// Make sure the token is still there
 	tokenResp, err := retrieveTestToken(codec, TestDefaultMasterToken, "dc1", structs.ACLTokenAnonymousID)
+	require.NoError(t, err)
 	require.NotNil(t, tokenResp.Token)
 }
 
@@ -2291,6 +2294,7 @@ func TestACLEndpoint_PolicyDelete(t *testing.T) {
 
 	// Make sure the policy is gone
 	tokenResp, err := retrieveTestPolicy(codec, TestDefaultMasterToken, "dc1", existingPolicy.ID)
+	require.NoError(t, err)
 	require.Nil(t, tokenResp.Policy)
 }
 
@@ -2903,6 +2907,7 @@ func TestACLEndpoint_RoleDelete(t *testing.T) {
 
 	// Make sure the role is gone
 	roleResp, err := retrieveTestRole(codec, TestDefaultMasterToken, "dc1", existingRole.ID)
+	require.NoError(t, err)
 	require.Nil(t, roleResp.Role)
 }
 
@@ -4395,6 +4400,7 @@ func TestACLEndpoint_Login(t *testing.T) {
 		structs.BindingRuleBindTypeNode,
 		"${serviceaccount.name}",
 	)
+	require.NoError(t, err)
 
 	t.Run("do not provide a token", func(t *testing.T) {
 		req := structs.ACLLoginRequest{

agent/consul/acl_test.go

@@ -3041,7 +3041,7 @@ func TestACL_filterDatacenterCheckServiceNodes(t *testing.T) {
 	node_prefix "" { policy = "read" }
 	`, acl.SyntaxCurrent, nil, nil)
 	require.NoError(t, err)
-	perms, err = acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
+	_, err = acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
 	require.NoError(t, err)
 
 	// Now it should go through.

agent/consul/fsm/snapshot_oss_test.go

@@ -533,6 +533,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 
 	// Verify the acl-token-bootstrap index was restored
 	canBootstrap, index, err := fsm2.state.CanBootstrapACLToken()
+	require.NoError(t, err)
 	require.False(t, canBootstrap)
 	require.True(t, index > 0)
 

agent/consul/server_lookup_test.go

@@ -1,11 +1,11 @@
 package consul
 
 import (
-	"fmt"
 	"testing"
 
 	"github.com/hashicorp/consul/agent/metadata"
 	"github.com/hashicorp/raft"
+	"github.com/stretchr/testify/require"
 )
 
 type testAddr struct {
@@ -46,11 +46,8 @@ func TestServerLookup(t *testing.T) {
 
 	lookup.RemoveServer(svr)
 
-	got, err = lookup.ServerAddr("1")
-	expectedErr := fmt.Errorf("Could not find address for server id 1")
-	if expectedErr.Error() != err.Error() {
-		t.Fatalf("Unexpected error, got %v wanted %v", err, expectedErr)
-	}
+	_, err = lookup.ServerAddr("1")
+	require.EqualError(t, err, "Could not find address for server id 1")
 
 	svr2 := &metadata.Server{ID: "2", Addr: &testAddr{"123.4.5.6"}}
 	lookup.RemoveServer(svr2)

agent/consul/state/catalog_test.go

@@ -795,7 +795,7 @@ func TestStateStore_EnsureNode(t *testing.T) {
 	if err := s.EnsureNode(3, in2); err != nil {
 		t.Fatalf("err: %s", err)
 	}
-	idx, out, err = s.GetNode("node1")
+	_, out, err = s.GetNode("node1")
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -839,7 +839,8 @@ func TestStateStore_EnsureNode(t *testing.T) {
 	}
 
 	// Retrieve the node
-	idx, out, err = s.GetNode("node1")
+	_, out, err = s.GetNode("node1")
+	require.NoError(t, err)
 	if out != nil {
 		t.Fatalf("Node should not exist anymore: %q", out)
 	}
@@ -903,7 +904,8 @@ func TestStateStore_EnsureNode(t *testing.T) {
 	}
 
 	// Retrieve the node
-	idx, out, err = s.GetNode("Node1bis")
+	_, out, err = s.GetNode("Node1bis")
+	require.NoError(t, err)
 	if out == nil {
 		t.Fatalf("Node should exist, but was null")
 	}
@@ -991,7 +993,7 @@ func TestStateStore_EnsureNode(t *testing.T) {
 	if err := s.EnsureNode(15, in); err != nil {
 		t.Fatalf("[DEPRECATED] it should work, err:= %q", err)
 	}
-	idx, out, err = s.GetNode("Node1-Renamed2")
+	_, out, err = s.GetNode("Node1-Renamed2")
 	if err != nil {
 		t.Fatalf("[DEPRECATED] err: %s", err)
 	}
@@ -2022,6 +2024,7 @@ func TestStateStore_DeleteService(t *testing.T) {
 	// Delete the service.
 	ws := memdb.NewWatchSet()
 	_, _, err := s.NodeServices(ws, "node1", nil)
+	require.NoError(t, err)
 	if err := s.DeleteService(4, "node1", "service1", nil); err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -3373,7 +3376,7 @@ func TestStateStore_ConnectQueryBlocking(t *testing.T) {
 
 			// Run the query
 			ws := memdb.NewWatchSet()
-			idx, res, err := s.CheckConnectServiceNodes(ws, tt.svc, nil)
+			_, res, err := s.CheckConnectServiceNodes(ws, tt.svc, nil)
 			require.NoError(err)
 			require.Len(res, tt.wantBeforeResLen)
 			require.Len(ws, tt.wantBeforeWatchSetSize)
@@ -3392,7 +3395,7 @@ func TestStateStore_ConnectQueryBlocking(t *testing.T) {
 
 			// Re-query the same result. Should return the desired index and len
 			ws = memdb.NewWatchSet()
-			idx, res, err = s.CheckConnectServiceNodes(ws, tt.svc, nil)
+			idx, res, err := s.CheckConnectServiceNodes(ws, tt.svc, nil)
 			require.NoError(err)
 			require.Len(res, tt.wantAfterResLen)
 			require.Equal(tt.wantAfterIndex, idx)
@@ -3465,7 +3468,7 @@ func TestStateStore_CheckServiceNodes(t *testing.T) {
 		t.Fatalf("bad")
 	}
 	ws = memdb.NewWatchSet()
-	idx, results, err = s.CheckServiceNodes(ws, "service1", nil)
+	idx, _, err = s.CheckServiceNodes(ws, "service1", nil)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -3481,7 +3484,7 @@ func TestStateStore_CheckServiceNodes(t *testing.T) {
 		t.Fatalf("bad")
 	}
 	ws = memdb.NewWatchSet()
-	idx, results, err = s.CheckServiceNodes(ws, "service1", nil)
+	idx, _, err = s.CheckServiceNodes(ws, "service1", nil)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -3495,7 +3498,7 @@ func TestStateStore_CheckServiceNodes(t *testing.T) {
 		t.Fatalf("bad")
 	}
 	ws = memdb.NewWatchSet()
-	idx, results, err = s.CheckServiceNodes(ws, "service1", nil)
+	idx, _, err = s.CheckServiceNodes(ws, "service1", nil)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}

agent/consul/state/coordinate_test.go

@@ -81,7 +81,7 @@ func TestStateStore_Coordinate_Updates(t *testing.T) {
 	require.Nil(t, all)
 
 	coordinateWs = memdb.NewWatchSet()
-	idx, coords, err = s.Coordinate("node1", coordinateWs)
+	idx, _, err = s.Coordinate("node1", coordinateWs)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}

agent/consul/state/kvs_test.go

@@ -429,7 +429,7 @@ func TestStateStore_KVSList(t *testing.T) {
 
 	// Delete a key and make sure the index comes from the tombstone.
 	ws = memdb.NewWatchSet()
-	idx, _, err = s.KVSList(ws, "foo/bar/baz", nil)
+	_, _, err = s.KVSList(ws, "foo/bar/baz", nil)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}