From b27457dac88a630f0a398bb4fa758b02806599f1 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 11 Nov 2020 13:15:33 -0500 Subject: [PATCH 1/2] ci: go-test-race switch to exclude list Most packages should pass the race detector. An exclude list ensures that new packages are automatically tested with -race. Also fix a couple small test races to allow more packages to be tested. Returning readyCh requires a lock because it can be set to nil, and setting it to nil will race without the lock. Move the TestServer.Listening calls around so that they properly guard setting TestServer.l. Otherwise it races. Remove t.Parallel in a small package. The entire package tests run in a few seconds, so t.Parallel does very little. In auto-config, wait for the AutoConfig.run goroutine to stop before calling readPersistedAutoConfig. Without this change there was a data race on reading ac.config. --- .circleci/config.yml | 9 ++++----- agent/auto-config/auto_config_test.go | 19 ++++++++++--------- connect/proxy/config_test.go | 4 +--- connect/proxy/proxy_test.go | 6 ++---- connect/service_test.go | 4 ++-- connect/testing.go | 7 ++++--- connect/tls.go | 5 ++++- 7 files changed, 27 insertions(+), 27 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index d8b6fe7269..fd6bb8282f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -236,16 +236,15 @@ jobs: name: go test -race command: | mkdir -p $TEST_RESULTS_DIR /tmp/jsonfile + pkgs="$(go list ./... | \ + grep -E -v '^github.com/hashicorp/consul/agent(/consul|/local|/xds|/routine-leak-checker)?$' | \ + grep -E -v '^github.com/hashicorp/consul/command/')" gotestsum \ - --format=short-verbose \ --jsonfile /tmp/jsonfile/go-test-race.log \ --junitfile $TEST_RESULTS_DIR/gotestsum-report.xml -- \ -tags="$GOTAGS" -p 2 \ -race -gcflags=all=-d=checkptr=0 \ - ./agent/{ae,cache,cache-types,checks,config,pool,proxycfg,router}/... \ - ./agent/consul/{authmethod,fsm,state,stream}/... \ - ./agent/{grpc,rpc,rpcclient,submatview}/... \ - ./snapshot + $pkgs - store_test_results: path: *TEST_RESULTS_DIR diff --git a/agent/auto-config/auto_config_test.go b/agent/auto-config/auto_config_test.go index b8ab0caf45..dc413ed4d0 100644 --- a/agent/auto-config/auto_config_test.go +++ b/agent/auto-config/auto_config_test.go @@ -634,6 +634,7 @@ type testAutoConfig struct { ac *AutoConfig tokenUpdates chan struct{} originalToken string + stop func() initialRoots *structs.IndexedCARoots initialCert *structs.IssuedCert @@ -835,6 +836,7 @@ func startedAutoConfig(t *testing.T, autoEncrypt bool) testAutoConfig { initialRoots: indexedRoots, initialCert: cert, extraCerts: extraCerts, + stop: cancel, } } @@ -1098,16 +1100,15 @@ func TestFallback(t *testing.T) { // now wait for the fallback routine to be invoked require.True(t, waitForChans(100*time.Millisecond, fallbackCtx.Done()), "fallback routines did not get invoked within the alotted time") - // persisting these to disk happens after the RPC we waited on above will have fired - // There is no deterministic way to know once its been written so we wrap this in a retry. - testretry.Run(t, func(r *testretry.R) { - resp, err := testAC.ac.readPersistedAutoConfig() - require.NoError(r, err) + testAC.stop() + <-testAC.ac.done - // ensure the roots got persisted to disk - require.Equal(r, thirdCert.CertPEM, resp.Certificate.GetCertPEM()) - require.Equal(r, secondRoots.ActiveRootID, resp.CARoots.GetActiveRootID()) - }) + resp, err := testAC.ac.readPersistedAutoConfig() + require.NoError(t, err) + + // ensure the roots got persisted to disk + require.Equal(t, thirdCert.CertPEM, resp.Certificate.GetCertPEM()) + require.Equal(t, secondRoots.ActiveRootID, resp.CARoots.GetActiveRootID()) } func TestIntroToken(t *testing.T) { diff --git a/connect/proxy/config_test.go b/connect/proxy/config_test.go index 6ba5603089..f076d626cc 100644 --- a/connect/proxy/config_test.go +++ b/connect/proxy/config_test.go @@ -5,12 +5,12 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/connect" "github.com/hashicorp/consul/sdk/testutil" - "github.com/stretchr/testify/require" ) func TestUpstreamResolverFuncFromClient(t *testing.T) { @@ -79,8 +79,6 @@ func TestUpstreamResolverFuncFromClient(t *testing.T) { } func TestAgentConfigWatcherSidecarProxy(t *testing.T) { - t.Parallel() - a := agent.StartTestAgent(t, agent.TestAgent{Name: "agent_smith"}) defer a.Shutdown() diff --git a/connect/proxy/proxy_test.go b/connect/proxy/proxy_test.go index 64765ca8a2..f280387a1d 100644 --- a/connect/proxy/proxy_test.go +++ b/connect/proxy/proxy_test.go @@ -5,7 +5,7 @@ import ( "net" "testing" - "github.com/hashicorp/consul/testrpc" + "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent" agConnect "github.com/hashicorp/consul/agent/connect" @@ -14,12 +14,10 @@ import ( "github.com/hashicorp/consul/sdk/freeport" "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" - "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/testrpc" ) func TestProxy_public(t *testing.T) { - t.Parallel() - require := require.New(t) ports := freeport.MustTake(1) diff --git a/connect/service_test.go b/connect/service_test.go index 49c4877007..1ee66ce83d 100644 --- a/connect/service_test.go +++ b/connect/service_test.go @@ -14,13 +14,13 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" - "github.com/stretchr/testify/require" ) // Assert io.Closer implementation @@ -89,8 +89,8 @@ func TestService_Dial(t *testing.T) { err := testSvr.Serve() require.NoError(err) }() - defer testSvr.Close() <-testSvr.Listening + defer testSvr.Close() } // Always expect to be connecting to a "DB" diff --git a/connect/testing.go b/connect/testing.go index 4554add0e8..30a517b61f 100644 --- a/connect/testing.go +++ b/connect/testing.go @@ -10,11 +10,12 @@ import ( "net/http" "sync/atomic" + "github.com/hashicorp/go-hclog" + testing "github.com/mitchellh/go-testing-interface" + "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/sdk/freeport" - "github.com/hashicorp/go-hclog" - testing "github.com/mitchellh/go-testing-interface" ) // TestService returns a Service instance based on a static TLS Config. @@ -124,8 +125,8 @@ func (s *TestServer) Serve() error { if err != nil { return err } - close(s.Listening) s.l = l + close(s.Listening) log.Printf("test connect service listening on %s", s.Addr) for { diff --git a/connect/tls.go b/connect/tls.go index e465d3a10a..a79fe7c8a3 100644 --- a/connect/tls.go +++ b/connect/tls.go @@ -13,9 +13,10 @@ import ( "strings" "sync" + "github.com/hashicorp/go-hclog" + "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/api" - "github.com/hashicorp/go-hclog" ) // parseLeafX509Cert will parse an X509 certificate @@ -460,5 +461,7 @@ func (cfg *dynamicTLSConfig) Ready() bool { // method will not stop returning a nil chan in that case. It is only useful // for initial startup. For ongoing health Ready() should be used. func (cfg *dynamicTLSConfig) ReadyWait() <-chan struct{} { + cfg.RLock() + defer cfg.RUnlock() return cfg.readyCh } From a10283a3132b3f17dd79923970c2cf2358216979 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 17 Nov 2020 12:37:02 -0500 Subject: [PATCH 2/2] acl: remove t.Parallel These tests run faster without it, and it was causing races in enterprise tests. --- acl/authorizer_test.go | 2 -- acl/chained_authorizer_test.go | 10 ---------- acl/policy_authorizer_test.go | 9 --------- acl/static_authorizer_test.go | 6 ------ 4 files changed, 27 deletions(-) diff --git a/acl/authorizer_test.go b/acl/authorizer_test.go index b5534b00cd..b1833efd14 100644 --- a/acl/authorizer_test.go +++ b/acl/authorizer_test.go @@ -195,8 +195,6 @@ func (m *mockAuthorizer) Snapshot(ctx *AuthorizerContext) EnforcementDecision { } func TestACL_Enforce(t *testing.T) { - t.Parallel() - type testCase struct { method string resource Resource diff --git a/acl/chained_authorizer_test.go b/acl/chained_authorizer_test.go index 4ff7b2e4f1..870a00f9c3 100644 --- a/acl/chained_authorizer_test.go +++ b/acl/chained_authorizer_test.go @@ -94,11 +94,7 @@ func (authz testAuthorizer) Snapshot(*AuthorizerContext) EnforcementDecision { } func TestChainedAuthorizer(t *testing.T) { - t.Parallel() - t.Run("No Authorizers", func(t *testing.T) { - t.Parallel() - authz := NewChainedAuthorizer([]Authorizer{}) checkDenyACLRead(t, authz, "foo", nil) checkDenyACLWrite(t, authz, "foo", nil) @@ -129,8 +125,6 @@ func TestChainedAuthorizer(t *testing.T) { }) t.Run("Authorizer Defaults", func(t *testing.T) { - t.Parallel() - authz := NewChainedAuthorizer([]Authorizer{testAuthorizer(Default)}) checkDenyACLRead(t, authz, "foo", nil) checkDenyACLWrite(t, authz, "foo", nil) @@ -161,8 +155,6 @@ func TestChainedAuthorizer(t *testing.T) { }) t.Run("Authorizer No Defaults", func(t *testing.T) { - t.Parallel() - authz := NewChainedAuthorizer([]Authorizer{testAuthorizer(Allow)}) checkAllowACLRead(t, authz, "foo", nil) checkAllowACLWrite(t, authz, "foo", nil) @@ -193,8 +185,6 @@ func TestChainedAuthorizer(t *testing.T) { }) t.Run("First Found", func(t *testing.T) { - t.Parallel() - authz := NewChainedAuthorizer([]Authorizer{testAuthorizer(Deny), testAuthorizer(Allow)}) checkDenyACLRead(t, authz, "foo", nil) checkDenyACLWrite(t, authz, "foo", nil) diff --git a/acl/policy_authorizer_test.go b/acl/policy_authorizer_test.go index 0f074e49aa..d303eea924 100644 --- a/acl/policy_authorizer_test.go +++ b/acl/policy_authorizer_test.go @@ -13,8 +13,6 @@ import ( // ensure compatibility from version to version those tests have been only minimally altered. The tests in this // file are specific to the newer functionality. func TestPolicyAuthorizer(t *testing.T) { - t.Parallel() - type aclCheck struct { name string prefix string @@ -446,8 +444,6 @@ func TestPolicyAuthorizer(t *testing.T) { name := name tcase := tcase t.Run(name, func(t *testing.T) { - t.Parallel() - authz, err := NewPolicyAuthorizer([]*Policy{tcase.policy}, nil) require.NoError(t, err) @@ -458,7 +454,6 @@ func TestPolicyAuthorizer(t *testing.T) { } t.Run(checkName, func(t *testing.T) { check := check - t.Parallel() check.check(t, authz, check.prefix, nil) }) @@ -468,8 +463,6 @@ func TestPolicyAuthorizer(t *testing.T) { } func TestAnyAllowed(t *testing.T) { - t.Parallel() - type radixInsertion struct { segment string value *policyAuthorizerRadixLeaf @@ -719,8 +712,6 @@ func TestAnyAllowed(t *testing.T) { } func TestAllAllowed(t *testing.T) { - t.Parallel() - type radixInsertion struct { segment string value *policyAuthorizerRadixLeaf diff --git a/acl/static_authorizer_test.go b/acl/static_authorizer_test.go index a2865754ee..b9ed590933 100644 --- a/acl/static_authorizer_test.go +++ b/acl/static_authorizer_test.go @@ -5,11 +5,7 @@ import ( ) func TestStaticAuthorizer(t *testing.T) { - t.Parallel() - t.Run("AllowAll", func(t *testing.T) { - t.Parallel() - authz := AllowAll() checkDenyACLRead(t, authz, "foo", nil) checkDenyACLWrite(t, authz, "foo", nil) @@ -40,7 +36,6 @@ func TestStaticAuthorizer(t *testing.T) { }) t.Run("DenyAll", func(t *testing.T) { - t.Parallel() authz := DenyAll() checkDenyACLRead(t, authz, "foo", nil) checkDenyACLWrite(t, authz, "foo", nil) @@ -71,7 +66,6 @@ func TestStaticAuthorizer(t *testing.T) { }) t.Run("ManageAll", func(t *testing.T) { - t.Parallel() authz := ManageAll() checkAllowACLRead(t, authz, "foo", nil) checkAllowACLWrite(t, authz, "foo", nil)