From b27457dac88a630f0a398bb4fa758b02806599f1 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 11 Nov 2020 13:15:33 -0500 Subject: [PATCH] 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 }