From cb050b280ceb4186de765118611a7a92d8158c3f Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 5 Jun 2020 15:28:03 -0400 Subject: [PATCH 1/2] ci: enable SA4006 staticcheck check And fix the 'value not used' issues. Many of these are not bugs, but a few are tests not checking errors, and one appears to be a missed error in non-test code. --- .golangci.yml | 4 --- agent/cache-types/catalog_datacenters_test.go | 13 +++----- agent/cache-types/service_checks.go | 3 ++ agent/consul/acl_endpoint_test.go | 5 +++ agent/consul/acl_test.go | 2 +- agent/consul/fsm/snapshot_oss_test.go | 1 + agent/consul/server_lookup_test.go | 9 ++---- agent/consul/state/catalog_test.go | 21 +++++++------ agent/consul/state/coordinate_test.go | 2 +- agent/consul/state/kvs_test.go | 2 +- agent/consul/state/session_test.go | 31 ++++++++++--------- agent/metadata/server_test.go | 13 +++----- agent/proxycfg/state.go | 12 +++---- agent/session_endpoint_test.go | 5 ++- agent/util_test.go | 2 +- api/agent_test.go | 2 +- api/config_entry_gateways_test.go | 4 +-- api/config_entry_test.go | 4 +-- api/lock.go | 3 ++ api/semaphore_test.go | 2 +- api/session_test.go | 13 ++++---- internal/go-sso/oidcauth/jwt.go | 2 +- tlsutil/config_test.go | 4 +++ 23 files changed, 80 insertions(+), 79 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 5573610570..8c4bdbedac 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -17,10 +17,6 @@ issues: - linters: [staticcheck] text: "SA9004:" - # Temp ignore SA4006: this value of `x` is never used - - linters: [staticcheck] - text: "SA4006:" - linters-settings: gofmt: simplify: false diff --git a/agent/cache-types/catalog_datacenters_test.go b/agent/cache-types/catalog_datacenters_test.go index fdae222902..dd78b0ac90 100644 --- a/agent/cache-types/catalog_datacenters_test.go +++ b/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) { diff --git a/agent/cache-types/service_checks.go b/agent/cache-types/service_checks.go index bb346390c3..9bc5919f43 100644 --- a/agent/cache-types/service_checks.go +++ b/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 diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index 04834f7761..1ae7a9090f 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/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) } diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index 248faa2c9a..91f8888d39 100644 --- a/agent/consul/acl_test.go +++ b/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. diff --git a/agent/consul/fsm/snapshot_oss_test.go b/agent/consul/fsm/snapshot_oss_test.go index 70fdae164b..acfd90cb76 100644 --- a/agent/consul/fsm/snapshot_oss_test.go +++ b/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) diff --git a/agent/consul/server_lookup_test.go b/agent/consul/server_lookup_test.go index 6e5a93d683..7c0b131224 100644 --- a/agent/consul/server_lookup_test.go +++ b/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) diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index bc0c052947..f9b0aafeb3 100644 --- a/agent/consul/state/catalog_test.go +++ b/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) } @@ -3371,7 +3374,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) @@ -3390,7 +3393,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) @@ -3463,7 +3466,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) } @@ -3479,7 +3482,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) } @@ -3493,7 +3496,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) } diff --git a/agent/consul/state/coordinate_test.go b/agent/consul/state/coordinate_test.go index 98ec794a8f..c640773754 100644 --- a/agent/consul/state/coordinate_test.go +++ b/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) } diff --git a/agent/consul/state/kvs_test.go b/agent/consul/state/kvs_test.go index 5de0f72576..8d3022bbca 100644 --- a/agent/consul/state/kvs_test.go +++ b/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) } diff --git a/agent/consul/state/session_test.go b/agent/consul/state/session_test.go index d4f9e85db7..fb5768d8e8 100644 --- a/agent/consul/state/session_test.go +++ b/agent/consul/state/session_test.go @@ -2,12 +2,13 @@ package state import ( "fmt" - "github.com/stretchr/testify/assert" "reflect" "strings" "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/types" @@ -543,7 +544,7 @@ func TestStateStore_Session_Invalidate_DeleteNode(t *testing.T) { // Delete the node and make sure the watch fires. ws := memdb.NewWatchSet() - idx, s2, err := s.SessionGet(ws, session.ID, nil) + _, _, err := s.SessionGet(ws, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -555,7 +556,7 @@ func TestStateStore_Session_Invalidate_DeleteNode(t *testing.T) { } // Lookup by ID, should be nil. - idx, s2, err = s.SessionGet(nil, session.ID, nil) + idx, s2, err := s.SessionGet(nil, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -598,7 +599,7 @@ func TestStateStore_Session_Invalidate_DeleteService(t *testing.T) { // Delete the service and make sure the watch fires. ws := memdb.NewWatchSet() - idx, s2, err := s.SessionGet(ws, session.ID, nil) + _, _, err := s.SessionGet(ws, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -610,7 +611,7 @@ func TestStateStore_Session_Invalidate_DeleteService(t *testing.T) { } // Lookup by ID, should be nil. - idx, s2, err = s.SessionGet(nil, session.ID, nil) + idx, s2, err := s.SessionGet(nil, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -648,7 +649,7 @@ func TestStateStore_Session_Invalidate_Critical_Check(t *testing.T) { // Invalidate the check and make sure the watches fire. ws := memdb.NewWatchSet() - idx, s2, err := s.SessionGet(ws, session.ID, nil) + _, _, err := s.SessionGet(ws, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -661,7 +662,7 @@ func TestStateStore_Session_Invalidate_Critical_Check(t *testing.T) { } // Lookup by ID, should be nil. - idx, s2, err = s.SessionGet(nil, session.ID, nil) + idx, s2, err := s.SessionGet(nil, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -699,7 +700,7 @@ func TestStateStore_Session_Invalidate_DeleteCheck(t *testing.T) { // Delete the check and make sure the watches fire. ws := memdb.NewWatchSet() - idx, s2, err := s.SessionGet(ws, session.ID, nil) + _, _, err := s.SessionGet(ws, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -711,7 +712,7 @@ func TestStateStore_Session_Invalidate_DeleteCheck(t *testing.T) { } // Lookup by ID, should be nil. - idx, s2, err = s.SessionGet(nil, session.ID, nil) + idx, s2, err := s.SessionGet(nil, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -767,7 +768,7 @@ func TestStateStore_Session_Invalidate_Key_Unlock_Behavior(t *testing.T) { // Delete the node and make sure the watches fire. ws := memdb.NewWatchSet() - idx, s2, err := s.SessionGet(ws, session.ID, nil) + _, _, err = s.SessionGet(ws, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -779,7 +780,7 @@ func TestStateStore_Session_Invalidate_Key_Unlock_Behavior(t *testing.T) { } // Lookup by ID, should be nil. - idx, s2, err = s.SessionGet(nil, session.ID, nil) + idx, s2, err := s.SessionGet(nil, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -849,7 +850,7 @@ func TestStateStore_Session_Invalidate_Key_Delete_Behavior(t *testing.T) { // Delete the node and make sure the watches fire. ws := memdb.NewWatchSet() - idx, s2, err := s.SessionGet(ws, session.ID, nil) + _, _, err = s.SessionGet(ws, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -861,7 +862,7 @@ func TestStateStore_Session_Invalidate_Key_Delete_Behavior(t *testing.T) { } // Lookup by ID, should be nil. - idx, s2, err = s.SessionGet(nil, session.ID, nil) + idx, s2, err := s.SessionGet(nil, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -917,7 +918,7 @@ func TestStateStore_Session_Invalidate_PreparedQuery_Delete(t *testing.T) { // Invalidate the session and make sure the watches fire. ws := memdb.NewWatchSet() - idx, s2, err := s.SessionGet(ws, session.ID, nil) + _, _, err := s.SessionGet(ws, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -929,7 +930,7 @@ func TestStateStore_Session_Invalidate_PreparedQuery_Delete(t *testing.T) { } // Make sure the session is gone. - idx, s2, err = s.SessionGet(nil, session.ID, nil) + idx, s2, err := s.SessionGet(nil, session.ID, nil) if err != nil { t.Fatalf("err: %v", err) } diff --git a/agent/metadata/server_test.go b/agent/metadata/server_test.go index 1b3f777630..6b96785ccf 100644 --- a/agent/metadata/server_test.go +++ b/agent/metadata/server_test.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/consul/agent/metadata" "github.com/hashicorp/serf/serf" + "github.com/stretchr/testify/require" ) func TestServer_Key_params(t *testing.T) { @@ -136,10 +137,8 @@ func TestIsConsulServer(t *testing.T) { } delete(m.Tags, "role") - ok, parts = metadata.IsConsulServer(m) - if ok { - t.Fatalf("unexpected ok server") - } + ok, _ = metadata.IsConsulServer(m) + require.False(t, ok, "expected to not be a consul server") } func TestIsConsulServer_Optional(t *testing.T) { @@ -209,8 +208,6 @@ func TestIsConsulServer_Optional(t *testing.T) { } delete(m.Tags, "role") - ok, parts = metadata.IsConsulServer(m) - if ok { - t.Fatalf("unexpected ok server") - } + ok, _ = metadata.IsConsulServer(m) + require.False(t, ok, "expected to not be a consul server") } diff --git a/agent/proxycfg/state.go b/agent/proxycfg/state.go index db62075ffc..e8323432fe 100644 --- a/agent/proxycfg/state.go +++ b/agent/proxycfg/state.go @@ -750,10 +750,8 @@ func (s *state) handleUpdateUpstreams(u cache.UpdateEvent, snap *ConfigSnapshotU return fmt.Errorf("invalid correlation id %q", u.CorrelationID) } - m, ok := snap.WatchedUpstreamEndpoints[svc] - if !ok { - m = make(map[string]structs.CheckServiceNodes) - snap.WatchedUpstreamEndpoints[svc] = m + if _, ok := snap.WatchedUpstreamEndpoints[svc]; !ok { + snap.WatchedUpstreamEndpoints[svc] = make(map[string]structs.CheckServiceNodes) } snap.WatchedUpstreamEndpoints[svc][targetID] = resp.Nodes @@ -767,10 +765,8 @@ func (s *state) handleUpdateUpstreams(u cache.UpdateEvent, snap *ConfigSnapshotU if !ok { return fmt.Errorf("invalid correlation id %q", u.CorrelationID) } - m, ok := snap.WatchedGatewayEndpoints[svc] - if !ok { - m = make(map[string]structs.CheckServiceNodes) - snap.WatchedGatewayEndpoints[svc] = m + if _, ok = snap.WatchedGatewayEndpoints[svc]; !ok { + snap.WatchedGatewayEndpoints[svc] = make(map[string]structs.CheckServiceNodes) } snap.WatchedGatewayEndpoints[svc][dc] = resp.Nodes default: diff --git a/agent/session_endpoint_test.go b/agent/session_endpoint_test.go index 8dea91af39..144aa26a3f 100644 --- a/agent/session_endpoint_test.go +++ b/agent/session_endpoint_test.go @@ -487,9 +487,8 @@ func TestSessionCustomTTL(t *testing.T) { r.Fatalf("err: %v", err) } respObj, ok = obj.(structs.Sessions) - if len(respObj) != 0 { - r.Fatalf("session '%s' should have been destroyed", id) - } + require.True(r, ok, "unexpected type: %T", obj) + require.Len(r, respObj, 0) }) } diff --git a/agent/util_test.go b/agent/util_test.go index d7af22031f..65df2cadc0 100644 --- a/agent/util_test.go +++ b/agent/util_test.go @@ -165,7 +165,7 @@ func TestHelperProcess(t *testing.T) { defer os.Exit(0) args = args[1:] // strip sentinel value - cmd, args := args[0], args[1:] + cmd := args[0] switch cmd { case "parent-signal": diff --git a/api/agent_test.go b/api/agent_test.go index 352d0ad9a2..bef1f5e41e 100644 --- a/api/agent_test.go +++ b/api/agent_test.go @@ -741,7 +741,7 @@ func TestAPI_AgentService(t *testing.T) { WaitTime: 100 * time.Millisecond, // Just long enough to be reliably measurable } start := time.Now() - got, qm, err = agent.Service("foo", &opts) + _, _, err = agent.Service("foo", &opts) elapsed := time.Since(start) require.NoError(err) require.True(elapsed >= opts.WaitTime) diff --git a/api/config_entry_gateways_test.go b/api/config_entry_gateways_test.go index bf7157d5ee..205a0e546a 100644 --- a/api/config_entry_gateways_test.go +++ b/api/config_entry_gateways_test.go @@ -150,7 +150,7 @@ func TestAPI_ConfigEntries_IngressGateway(t *testing.T) { require.NotEqual(t, 0, wm.RequestTime) // verify deletion - entry, qm, err = config_entries.Get(IngressGateway, "foo", nil) + _, _, err = config_entries.Get(IngressGateway, "foo", nil) require.Error(t, err) } @@ -279,6 +279,6 @@ func TestAPI_ConfigEntries_TerminatingGateway(t *testing.T) { require.NotEqual(t, 0, wm.RequestTime) // verify deletion - entry, qm, err = configEntries.Get(TerminatingGateway, "foo", nil) + _, _, err = configEntries.Get(TerminatingGateway, "foo", nil) require.Error(t, err) } diff --git a/api/config_entry_test.go b/api/config_entry_test.go index deedd81959..6d7acefb3b 100644 --- a/api/config_entry_test.go +++ b/api/config_entry_test.go @@ -83,7 +83,7 @@ func TestAPI_ConfigEntries(t *testing.T) { require.NotNil(t, wm) require.NotEqual(t, 0, wm.RequestTime) - entry, qm, err = config_entries.Get(ProxyDefaults, ProxyConfigGlobal, nil) + _, _, err = config_entries.Get(ProxyDefaults, ProxyConfigGlobal, nil) require.Error(t, err) }) @@ -181,7 +181,7 @@ func TestAPI_ConfigEntries(t *testing.T) { require.NotEqual(t, 0, wm.RequestTime) // verify deletion - entry, qm, err = config_entries.Get(ServiceDefaults, "foo", nil) + _, _, err = config_entries.Get(ServiceDefaults, "foo", nil) require.Error(t, err) }) } diff --git a/api/lock.go b/api/lock.go index a532fbf397..5cacee8f7e 100644 --- a/api/lock.go +++ b/api/lock.go @@ -227,6 +227,9 @@ WAIT: // Determine why the lock failed qOpts.WaitIndex = 0 pair, meta, err = kv.Get(l.opts.Key, &qOpts) + if err != nil { + return nil, err + } if pair != nil && pair.Session != "" { //If the session is not null, this means that a wait can safely happen //using a long poll diff --git a/api/semaphore_test.go b/api/semaphore_test.go index dfa3333a49..37294c411b 100644 --- a/api/semaphore_test.go +++ b/api/semaphore_test.go @@ -227,7 +227,7 @@ func TestAPI_SemaphoreBadLimit(t *testing.T) { s.WaitForSerfCheck(t) - sema, err := c.SemaphorePrefix("test/semaphore", 0) + _, err := c.SemaphorePrefix("test/semaphore", 0) if err == nil { t.Fatalf("should error, limit must be positive") } diff --git a/api/session_test.go b/api/session_test.go index 401841560f..687feb58cf 100644 --- a/api/session_test.go +++ b/api/session_test.go @@ -2,10 +2,11 @@ package api import ( "context" - "github.com/stretchr/testify/assert" "strings" "testing" "time" + + "github.com/stretchr/testify/assert" ) func TestAPI_SessionCreateDestroy(t *testing.T) { @@ -411,7 +412,7 @@ func TestAPI_SessionNode(t *testing.T) { } defer session.Destroy(id, nil) - info, qm, err := session.Info(id, nil) + info, _, err := session.Info(id, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -478,14 +479,14 @@ func TestAPI_SessionNodeChecks(t *testing.T) { } session := c.Session() - id, _, err := session.Create(&se, nil) + _, _, err := session.Create(&se, nil) if err == nil { t.Fatalf("should have failed") } // Empty node check should lead to serf check se.NodeChecks = []string{} - id, _, err = session.Create(&se, nil) + id, _, err := session.Create(&se, nil) if err != nil { t.Fatalf("err: %v", err) } @@ -588,7 +589,7 @@ func TestAPI_SessionServiceChecks(t *testing.T) { } session := c.Session() - id, _, err := session.Create(&se, nil) + _, _, err := session.Create(&se, nil) if err == nil { t.Fatalf("should have failed") } @@ -624,7 +625,7 @@ func TestAPI_SessionServiceChecks(t *testing.T) { {"redis:alive", ""}, } - id, _, err = session.Create(&se, nil) + id, _, err := session.Create(&se, nil) if err != nil { t.Fatalf("err: %v", err) } diff --git a/internal/go-sso/oidcauth/jwt.go b/internal/go-sso/oidcauth/jwt.go index 2ca80ffba2..c0488f8b3a 100644 --- a/internal/go-sso/oidcauth/jwt.go +++ b/internal/go-sso/oidcauth/jwt.go @@ -180,7 +180,7 @@ func (a *Authenticator) verifyVanillaJWT(ctx context.Context, loginToken string) // // go-sso added support for ed25519 (EdDSA) func parsePublicKeyPEM(data []byte) (interface{}, error) { - block, data := pem.Decode(data) + block, _ := pem.Decode(data) if block != nil { var rawKey interface{} var err error diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 91bd99de82..eb1fc1c4e8 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -964,6 +964,7 @@ func TestConfigurator_OutgoingRPCWrapper(t *testing.T) { require.NotNil(t, wrapper) conn := &net.TCPConn{} cWrap, err := wrapper("", conn) + require.NoError(t, err) require.Equal(t, conn, cWrap) c, err = NewConfigurator(Config{ @@ -975,6 +976,7 @@ func TestConfigurator_OutgoingRPCWrapper(t *testing.T) { wrapper = c.OutgoingRPCWrapper() require.NotNil(t, wrapper) cWrap, err = wrapper("", conn) + require.EqualError(t, err, "invalid argument") require.NotEqual(t, conn, cWrap) } @@ -984,6 +986,7 @@ func TestConfigurator_OutgoingALPNRPCWrapper(t *testing.T) { require.NotNil(t, wrapper) conn := &net.TCPConn{} cWrap, err := wrapper("", conn) + require.NoError(t, err) require.Equal(t, conn, cWrap) c, err = NewConfigurator(Config{ @@ -995,6 +998,7 @@ func TestConfigurator_OutgoingALPNRPCWrapper(t *testing.T) { wrapper = c.OutgoingRPCWrapper() require.NotNil(t, wrapper) cWrap, err = wrapper("", conn) + require.EqualError(t, err, "invalid argument") require.NotEqual(t, conn, cWrap) } From d345cd8d30dcd4ba74d9cb2fb9b1dac7530f8663 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 5 Jun 2020 18:08:30 -0400 Subject: [PATCH 2/2] ci: Add ineffsign linter And fix an additional ineffective assignment that was not caught by staticcheck --- .golangci.yml | 1 + agent/agent.go | 21 ++++++++++----------- agent/consul/acl_endpoint_test.go | 1 + lib/decode/decode_test.go | 4 ++++ 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 8c4bdbedac..306fdc4c4b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -5,6 +5,7 @@ linters: - govet - unconvert - staticcheck + - ineffassign issues: # Disable the default exclude list so that all excludes are explicitly diff --git a/agent/agent.go b/agent/agent.go index ce69d8da5c..233e0285ac 100644 --- a/agent/agent.go +++ b/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: diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index 1ae7a9090f..f2176f4b5c 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/agent/consul/acl_endpoint_test.go @@ -4400,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{ diff --git a/lib/decode/decode_test.go b/lib/decode/decode_test.go index 1f2b5d3529..8c1e6da5c5 100644 --- a/lib/decode/decode_test.go +++ b/lib/decode/decode_test.go @@ -282,6 +282,9 @@ func decodeHCLToMapStructure(source string, target interface{}) error { Metadata: md, Result: target, }) + if err != nil { + return err + } return decoder.Decode(&raw) } @@ -305,6 +308,7 @@ func TestHookWeakDecodeFromSlice_DoesNotModifySliceTargetsFromSliceInterface(t * }) require.NoError(t, err) err = decoder.Decode(&raw) + require.NoError(t, err) expected := &nested{ Slice: []Item{{Name: "first"}},