Merge pull request #11339 from hashicorp/dnephin/ca-manager-isolate-secondary-2

ca: reduce use of state in the secondary

agent/consul/leader_connect_ca.go

@@ -70,10 +70,9 @@ type CAManager struct {
 	providerRoot *structs.CARoot
 
 	// stateLock protects the internal state used for administrative CA tasks.
-	stateLock         sync.Mutex
+	stateLock    sync.Mutex
-	state             caState
+	state        caState
-	primaryRoots      structs.IndexedCARoots // The most recently seen state of the root CAs from the primary datacenter.
+	primaryRoots structs.IndexedCARoots // The most recently seen state of the root CAs from the primary datacenter.
-	actingSecondaryCA bool                   // True if this datacenter has been initialized as a secondary CA.
 
 	leaderRoutineManager *routine.Manager
 	// providerShim is used to test CAManager with a fake provider.
@@ -191,19 +190,15 @@ func (e *caStateError) Error() string {
 }
 
 // secondarySetPrimaryRoots updates the most recently seen roots from the primary.
-func (c *CAManager) secondarySetPrimaryRoots(newRoots structs.IndexedCARoots) error {
+func (c *CAManager) secondarySetPrimaryRoots(newRoots structs.IndexedCARoots) {
+	// TODO: this could be a different lock, as long as its the same lock in secondaryGetPrimaryRoots
 	c.stateLock.Lock()
 	defer c.stateLock.Unlock()
-
+	c.primaryRoots = newRoots
-	if c.state == caStateInitializing || c.state == caStateReconfig {
-		c.primaryRoots = newRoots
-	} else {
-		return fmt.Errorf("Cannot update primary roots in state %q", c.state)
-	}
-	return nil
 }
 
 func (c *CAManager) secondaryGetPrimaryRoots() structs.IndexedCARoots {
+	// TODO: this could be a different lock, as long as its the same lock in secondarySetPrimaryRoots
 	c.stateLock.Lock()
 	defer c.stateLock.Unlock()
 	return c.primaryRoots
@@ -346,7 +341,6 @@ func (c *CAManager) Stop() {
 
 	c.setState(caStateUninitialized, false)
 	c.primaryRoots = structs.IndexedCARoots{}
-	c.actingSecondaryCA = false
 	c.setCAProvider(nil, nil)
 }
 
@@ -439,9 +433,7 @@ func (c *CAManager) secondaryInitialize(provider ca.Provider, conf *structs.CACo
 	if err := c.delegate.forwardDC("ConnectCA.Roots", c.serverConf.PrimaryDatacenter, &args, &roots); err != nil {
 		return err
 	}
-	if err := c.secondarySetPrimaryRoots(roots); err != nil {
+	c.secondarySetPrimaryRoots(roots)
-		return err
-	}
 
 	// Configure the CA provider and initialize the intermediate certificate if necessary.
 	if err := c.secondaryInitializeProvider(provider, roots); err != nil {
@@ -1144,7 +1136,7 @@ func (c *CAManager) RenewIntermediate(ctx context.Context, isPrimary bool) error
 		return nil
 	}
 	// If this isn't the primary, make sure the CA has been initialized.
-	if !isPrimary && !c.secondaryIsCAConfigured() {
+	if !isPrimary && !c.secondaryHasProviderRoots() {
 		return fmt.Errorf("secondary CA is not yet configured.")
 	}
 
@@ -1263,34 +1255,35 @@ func (c *CAManager) secondaryUpdateRoots(roots structs.IndexedCARoots) error {
 	defer c.setState(caStateInitialized, false)
 
 	// Update the cached primary roots now that the lock is held.
-	if err := c.secondarySetPrimaryRoots(roots); err != nil {
+	c.secondarySetPrimaryRoots(roots)
-		return err
-	}
 
-	// Check to see if the primary has been upgraded in case we're waiting to switch to
-	// secondary mode.
 	provider, _ := c.getCAProvider()
 	if provider == nil {
 		// this happens when leadership is being revoked and this go routine will be stopped
 		return nil
 	}
-	if !c.secondaryIsCAConfigured() {
-		if err := c.delegate.ServersSupportMultiDCConnectCA(); err != nil {
-			return fmt.Errorf("failed to initialize while updating primary roots: %w", err)
-		}
-		if err := c.secondaryInitializeProvider(provider, roots); err != nil {
-			return fmt.Errorf("Failed to initialize secondary CA provider: %v", err)
-		}
-	}
 
 	// Run the secondary CA init routine to see if we need to request a new
 	// intermediate.
-	if c.secondaryIsCAConfigured() {
+	if c.secondaryHasProviderRoots() {
 		if err := c.secondaryInitializeIntermediateCA(provider, nil); err != nil {
 			return fmt.Errorf("Failed to initialize the secondary CA: %v", err)
 		}
+		return nil
 	}
 
+	// Attempt to initialize now that we have updated roots. This is an optimization
+	// so that we don't have to wait for the InitializeCA retry backoff if we were
+	// waiting on roots from the primary to be able to complete initialization.
+	if err := c.delegate.ServersSupportMultiDCConnectCA(); err != nil {
+		return fmt.Errorf("failed to initialize while updating primary roots: %w", err)
+	}
+	if err := c.secondaryInitializeProvider(provider, roots); err != nil {
+		return fmt.Errorf("Failed to initialize secondary CA provider: %v", err)
+	}
+	if err := c.secondaryInitializeIntermediateCA(provider, nil); err != nil {
+		return fmt.Errorf("Failed to initialize the secondary CA: %v", err)
+	}
 	return nil
 }
 
@@ -1316,29 +1309,17 @@ func (c *CAManager) secondaryInitializeProvider(provider ca.Provider, roots stru
 	if err := provider.Configure(pCfg); err != nil {
 		return fmt.Errorf("error configuring provider: %v", err)
 	}
-
-	return c.secondarySetCAConfigured()
-}
-
-// secondarySetCAConfigured sets the flag for acting as a secondary CA to true.
-func (c *CAManager) secondarySetCAConfigured() error {
-	c.stateLock.Lock()
-	defer c.stateLock.Unlock()
-
-	if c.state == caStateInitializing || c.state == caStateReconfig {
-		c.actingSecondaryCA = true
-	} else {
-		return fmt.Errorf("Cannot update secondary CA flag in state %q", c.state)
-	}
-
 	return nil
 }
 
-// secondaryIsCAConfigured returns true if we have been initialized as a secondary datacenter's CA.
+// secondaryHasProviderRoots returns true after providerRoot has been set. This
-func (c *CAManager) secondaryIsCAConfigured() bool {
+// method is used to detect when the secondary has received the roots from the
-	c.stateLock.Lock()
+// primary DC.
-	defer c.stateLock.Unlock()
+func (c *CAManager) secondaryHasProviderRoots() bool {
-	return c.actingSecondaryCA
+	// TODO: this could potentially also use primaryRoots instead of providerRoot
+	c.providerLock.Lock()
+	defer c.providerLock.Unlock()
+	return c.providerRoot != nil
 }
 
 type connectSignRateLimiter struct {