From f1944458e4bb53fdc8bc907cef8f43dd0d6c4ecf Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 18 Oct 2021 11:44:47 -0400 Subject: [PATCH 1/2] ca: remove actingSecondaryCA This commit removes the actingSecondaryCA field, and removes the stateLock around it. This field was acting as a proxy for providerRoot != nil, so replace it with that check instead. The two methods which called secondarySetCAConfigured already set the state, so checking the state again at this point will not catch runtime errors (only programming errors, which we can catch with tests). In general, handling state transitions should be done on the "entrypoint" methods where execution starts, not in every internal method. This is being done to remove some unnecessary references to c.state, in preparations for extracting types for primary/secondary. --- agent/consul/leader_connect_ca.go | 62 +++++++++++++------------------ 1 file changed, 25 insertions(+), 37 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 22a4ec766e..30ce677c37 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -68,10 +68,9 @@ type CAManager struct { providerRoot *structs.CARoot // stateLock protects the internal state used for administrative CA tasks. - stateLock sync.Mutex - state caState - 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. + stateLock sync.Mutex + state caState + primaryRoots structs.IndexedCARoots // The most recently seen state of the root CAs from the primary datacenter. leaderRoutineManager *routine.Manager // providerShim is used to test CAManager with a fake provider. @@ -339,7 +338,6 @@ func (c *CAManager) Stop() { c.setState(caStateUninitialized, false) c.primaryRoots = structs.IndexedCARoots{} - c.actingSecondaryCA = false c.setCAProvider(nil, nil) } @@ -1137,7 +1135,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.") } @@ -1260,30 +1258,33 @@ func (c *CAManager) secondaryUpdateRoots(roots structs.IndexedCARoots) error { 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 } @@ -1309,29 +1310,16 @@ 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. -func (c *CAManager) secondaryIsCAConfigured() bool { - c.stateLock.Lock() - defer c.stateLock.Unlock() - return c.actingSecondaryCA +// secondaryHasProviderRoots returns true after providerRoot has been set. This +// method is used to detect when the secondary has received the roots from the +// primary DC. +func (c *CAManager) secondaryHasProviderRoots() bool { + c.providerLock.Lock() + defer c.providerLock.Unlock() + return c.providerRoot != nil } type connectSignRateLimiter struct { From 91a0c25932166ec15ff53e5a1299792c8c0af73b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sun, 10 Oct 2021 21:15:13 -0400 Subject: [PATCH 2/2] ca: remove state check in secondarySetPrimaryRoots This function is only ever called from operations that have already acquired the state lock, so checking the value of state can never fail. This change is being made in preparation for splitting out a separate type for the secondary logic. The state can't easily be shared, so really only the expored top-level functions should acquire the 'state lock'. --- agent/consul/leader_connect_ca.go | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 30ce677c37..8955d7e789 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -183,19 +183,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() - - 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 + c.primaryRoots = newRoots } 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 @@ -430,9 +426,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 { - return err - } + c.secondarySetPrimaryRoots(roots) // Configure the CA provider and initialize the intermediate certificate if necessary. if err := c.secondaryInitializeProvider(provider, roots); err != nil { @@ -1254,9 +1248,7 @@ 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 { - return err - } + c.secondarySetPrimaryRoots(roots) provider, _ := c.getCAProvider() if provider == nil { @@ -1317,6 +1309,7 @@ func (c *CAManager) secondaryInitializeProvider(provider ca.Provider, roots stru // method is used to detect when the secondary has received the roots from the // primary DC. func (c *CAManager) secondaryHasProviderRoots() bool { + // TODO: this could potentially also use primaryRoots instead of providerRoot c.providerLock.Lock() defer c.providerLock.Unlock() return c.providerRoot != nil