From a7fec642fccc8cf6cc3aed789c20d9292c937dc2 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 d8b6fe726..fd6bb8282 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 b8ab0caf4..dc413ed4d 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 6ba560308..f076d626c 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 64765ca8a..f280387a1 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 49c487700..1ee66ce83 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 4554add0e..30a517b61 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 e465d3a10..a79fe7c8a 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 }