fix TestLeader_SecondaryCA_IntermediateRenew (#8702)

* fix lessThanHalfTime
* get lock for CAProvider()
* make a var to relate both vars
* rename to getCAProviderWithLock
* move CertificateTimeDriftBuffer to agent/connect/ca

agent/connect/ca/provider_consul.go

@@ -21,6 +21,13 @@ import (
 	"github.com/hashicorp/go-hclog"
 )
 
+const (
+
+	// NotBefore will be CertificateTimeDriftBuffer in the past to account for
+	// time drift between different servers.
+	CertificateTimeDriftBuffer = time.Minute
+)
+
 var ErrNotInitialized = errors.New("provider not initialized")
 
 type ConsulProvider struct {
@@ -474,7 +481,7 @@ func (c *ConsulProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
 	// Sign the certificate valid from 1 minute in the past, this helps it be
 	// accepted right away even when nodes are not in close time sync across the
 	// cluster. A minute is more than enough for typical DC clock drift.
-	effectiveNow := time.Now().Add(-1 * time.Minute)
+	effectiveNow := time.Now().Add(-1 * CertificateTimeDriftBuffer)
 	template := x509.Certificate{
 		SerialNumber:          sn,
 		Subject:               csr.Subject,

agent/consul/leader_connect.go

@@ -696,7 +696,7 @@ func (s *Server) secondaryIntermediateCertRenewalWatch(ctx context.Context) erro
 					return fmt.Errorf("error parsing active intermediate cert: %v", err)
 				}
 
-				if lessThanHalfTimePassed(time.Now(), intermediateCert.NotBefore,
+				if lessThanHalfTimePassed(time.Now(), intermediateCert.NotBefore.Add(ca.CertificateTimeDriftBuffer),
 					intermediateCert.NotAfter) {
 					return nil
 				}

agent/consul/leader_connect_test.go

@@ -100,8 +100,8 @@ func TestLeader_SecondaryCA_Initialize(t *testing.T) {
 				err               error
 			)
 			retry.Run(t, func(r *retry.R) {
-				_, caRoot = s1.getCAProvider()
+				_, caRoot = getCAProviderWithLock(s1)
-				secondaryProvider, _ = s2.getCAProvider()
+				secondaryProvider, _ = getCAProviderWithLock(s2)
 				intermediatePEM, err = secondaryProvider.ActiveIntermediate()
 				require.NoError(r, err)
 
@@ -165,7 +165,7 @@ func TestLeader_SecondaryCA_Initialize(t *testing.T) {
 
 func waitForActiveCARoot(t *testing.T, srv *Server, expect *structs.CARoot) {
 	retry.Run(t, func(r *retry.R) {
-		_, root := srv.getCAProvider()
+		_, root := getCAProviderWithLock(srv)
 		if root == nil {
 			r.Fatal("no root")
 		}
@@ -175,6 +175,12 @@ func waitForActiveCARoot(t *testing.T, srv *Server, expect *structs.CARoot) {
 	})
 }
 
+func getCAProviderWithLock(s *Server) (ca.Provider, *structs.CARoot) {
+	s.caProviderReconfigurationLock.Lock()
+	defer s.caProviderReconfigurationLock.Unlock()
+	return s.getCAProvider()
+}
+
 func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 	// no parallel execution because we change globals
 	origInterval := structs.IntermediateCertRenewInterval
@@ -227,7 +233,8 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 	testrpc.WaitForLeader(t, s2.RPC, "dc2")
 
 	// Get the original intermediate
-	secondaryProvider, _ := s2.getCAProvider()
+	// TODO: Wait for intermediate instead of wait for leader
+	secondaryProvider, _ := getCAProviderWithLock(s2)
 	intermediatePEM, err := secondaryProvider.ActiveIntermediate()
 	require.NoError(err)
 	cert, err := connect.ParseCert(intermediatePEM)
@@ -253,7 +260,7 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 	// however, defaultQueryTime will be configurable and we con lower it
 	// so that it returns for sure.
 	retry.Run(t, func(r *retry.R) {
-		secondaryProvider, _ := s2.getCAProvider()
+		secondaryProvider, _ = getCAProviderWithLock(s2)
 		intermediatePEM, err = secondaryProvider.ActiveIntermediate()
 		r.Check(err)
 		cert, err := connect.ParseCert(intermediatePEM)
@@ -266,9 +273,9 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 	})
 	require.NoError(err)
 
-	// Get the new root from dc1 and validate a chain of:
+	// Get the root from dc1 and validate a chain of:
 	// dc2 leaf -> dc2 intermediate -> dc1 root
-	_, caRoot := s1.getCAProvider()
+	_, caRoot := getCAProviderWithLock(s1)
 
 	// Have dc2 sign a leaf cert and make sure the chain is correct.
 	spiffeService := &connect.SpiffeIDService{
@@ -329,7 +336,7 @@ func TestLeader_SecondaryCA_IntermediateRefresh(t *testing.T) {
 	testrpc.WaitForLeader(t, s2.RPC, "dc2")
 
 	// Get the original intermediate
-	secondaryProvider, _ := s2.getCAProvider()
+	secondaryProvider, _ := getCAProviderWithLock(s2)
 	oldIntermediatePEM, err := secondaryProvider.ActiveIntermediate()
 	require.NoError(err)
 	require.NotEmpty(oldIntermediatePEM)
@@ -415,7 +422,7 @@ func TestLeader_SecondaryCA_IntermediateRefresh(t *testing.T) {
 
 	// Get the new root from dc1 and validate a chain of:
 	// dc2 leaf -> dc2 intermediate -> dc1 root
-	_, caRoot := s1.getCAProvider()
+	_, caRoot := getCAProviderWithLock(s1)
 
 	// Have dc2 sign a leaf cert and make sure the chain is correct.
 	spiffeService := &connect.SpiffeIDService{
@@ -524,7 +531,7 @@ func TestLeader_SecondaryCA_FixSigningKeyID_via_IntermediateRefresh(t *testing.T
 	// the CA provider anyway.
 	retry.Run(t, func(r *retry.R) {
 		// verify that the root is now corrected
-		provider, activeRoot := s2.getCAProvider()
+		provider, activeRoot := getCAProviderWithLock(s2)
 		require.NotNil(r, provider)
 		require.NotNil(r, activeRoot)
 
@@ -709,7 +716,7 @@ func TestLeader_SecondaryCA_UpgradeBeforePrimary(t *testing.T) {
 
 	// Wait for the secondary transition to happen and then verify the secondary DC
 	// has both roots present.
-	secondaryProvider, _ := s2.getCAProvider()
+	secondaryProvider, _ := getCAProviderWithLock(s2)
 	retry.Run(t, func(r *retry.R) {
 		state1 := s1.fsm.State()
 		_, roots1, err := state1.CARoots(nil)
@@ -730,7 +737,7 @@ func TestLeader_SecondaryCA_UpgradeBeforePrimary(t *testing.T) {
 		require.NotEmpty(r, inter, "should have valid intermediate")
 	})
 
-	_, caRoot := s1.getCAProvider()
+	_, caRoot := getCAProviderWithLock(s1)
 	intermediatePEM, err := secondaryProvider.ActiveIntermediate()
 	require.NoError(t, err)
 
@@ -1325,7 +1332,7 @@ func TestLeader_PersistIntermediateCAs(t *testing.T) {
 	}
 
 	// Get the active root before leader change.
-	_, root := s1.getCAProvider()
+	_, root := getCAProviderWithLock(s1)
 	require.Len(root.IntermediateCerts, 1)
 
 	// Force a leader change and make sure the root CA values are preserved.
@@ -1344,7 +1351,7 @@ func TestLeader_PersistIntermediateCAs(t *testing.T) {
 			r.Fatal("no leader")
 		}
 
-		_, newLeaderRoot := leader.getCAProvider()
+		_, newLeaderRoot := getCAProviderWithLock(leader)
 		if !reflect.DeepEqual(newLeaderRoot, root) {
 			r.Fatalf("got %v, want %v", newLeaderRoot, root)
 		}