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

@@ -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}/... \
+              $pkgs
-              ./agent/consul/{authmethod,fsm,state,stream}/... \
-              ./agent/{grpc,rpc,rpcclient,submatview}/... \
-              ./snapshot
 
       - store_test_results:
           path: *TEST_RESULTS_DIR

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
+	testAC.stop()
-	// There is no deterministic way to know once its been written so we wrap this in a retry.
+	<-testAC.ac.done
-	testretry.Run(t, func(r *testretry.R) {
-		resp, err := testAC.ac.readPersistedAutoConfig()
-		require.NoError(r, err)
 
-		// ensure the roots got persisted to disk
+	resp, err := testAC.ac.readPersistedAutoConfig()
-		require.Equal(r, thirdCert.CertPEM, resp.Certificate.GetCertPEM())
+	require.NoError(t, err)
-		require.Equal(r, secondRoots.ActiveRootID, resp.CARoots.GetActiveRootID())
+
-	})
+	// 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) {

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()
 

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)

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"

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 {

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
 }