Skip to content

Commit

Permalink
Serialize censusManager teardown
Browse files Browse the repository at this point in the history
This commit enforces complete censusManager teardown prior to stopping
the expiration manager, allowing us to simplify locking elsewhere in
censusManager code.

We're now guaranteed to not have state change underneath us, since
censusManager goroutine lifecycles are postUnseal -> preSeal.
  • Loading branch information
mpalmi committed Dec 4, 2024
1 parent 73bf3eb commit 73e4e4d
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 22 deletions.
11 changes: 7 additions & 4 deletions vault/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -2898,14 +2898,17 @@ func (c *Core) preSeal() error {
if err := c.teardownAudits(); err != nil {
result = multierror.Append(result, fmt.Errorf("error tearing down audits: %w", err))
}
if err := c.stopExpiration(); err != nil {
result = multierror.Append(result, fmt.Errorf("error stopping expiration: %w", err))
}
// Ensure that the ActivityLog and CensusManager are both completely torn
// down before stopping the ExpirationManager. This ordering is critical,
// due to a tight coupling between the ActivityLog, CensusManager, and
// ExpirationManager for product usage reporting.
c.stopActivityLog()
// Clean up census on seal
if err := c.teardownCensusManager(); err != nil {
result = multierror.Append(result, fmt.Errorf("error tearing down reporting agent: %w", err))
}
if err := c.stopExpiration(); err != nil {
result = multierror.Append(result, fmt.Errorf("error stopping expiration: %w", err))
}
if err := c.teardownCredentials(context.Background()); err != nil {
result = multierror.Append(result, fmt.Errorf("error tearing down credentials: %w", err))
}
Expand Down
5 changes: 3 additions & 2 deletions vault/expiration.go
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,9 @@ func (c *Core) setupExpiration(e ExpireLeaseStrategy) error {
return nil
}

// stopExpiration is used to stop the expiration manager before
// sealing the Vault.
// stopExpiration is used to stop the expiration manager before sealing the
// Vault. This *must* be called before shutting down the CensusManager, since
// the CensusManager is tightly coupled with Core for product usage reporting.
func (c *Core) stopExpiration() error {
if c.expiration != nil {
if err := c.expiration.Stop(); err != nil {
Expand Down
32 changes: 20 additions & 12 deletions vault/expiration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -855,10 +855,7 @@ func TestExpiration_Restore(t *testing.T) {
}

// Stop everything
err = c.stopExpiration()
if err != nil {
t.Fatalf("err: %v", err)
}
stopExpiration(t, c)

if exp.leaseCount != 0 {
t.Fatalf("expected %v leases, got %v", 0, exp.leaseCount)
Expand Down Expand Up @@ -3008,6 +3005,23 @@ func registerOneLease(t *testing.T, ctx context.Context, exp *ExpirationManager)
return leaseID
}

// stopExpiration is a test helper which allows us to safely teardown the
// expiration manager. This preserves the shutdown order of Core for these few
// outlier tests that directly stop the ExpirationManager.
func stopExpiration(t *testing.T, core *Core) {
t.Helper()
core.stopActivityLog()
err := core.teardownCensusManager()
if err != nil {
t.Fatalf("error stopping census manager: %v", err)
}

err = core.stopExpiration()
if err != nil {
t.Fatalf("error stopping expiration manager: %v", err)
}
}

func TestExpiration_MarkIrrevocable(t *testing.T) {
c, _, _ := TestCoreUnsealed(t)
exp := c.expiration
Expand Down Expand Up @@ -3060,10 +3074,7 @@ func TestExpiration_MarkIrrevocable(t *testing.T) {
}

// stop and restore to verify that irrevocable leases are properly loaded from storage
err = c.stopExpiration()
if err != nil {
t.Fatalf("error stopping expiration manager: %v", err)
}
stopExpiration(t, c)

err = exp.Restore(nil)
if err != nil {
Expand Down Expand Up @@ -3153,10 +3164,7 @@ func TestExpiration_StopClearsIrrevocableCache(t *testing.T) {
exp.markLeaseIrrevocable(ctx, le, fmt.Errorf("test irrevocable error"))
exp.pendingLock.Unlock()

err = c.stopExpiration()
if err != nil {
t.Fatalf("error stopping expiration manager: %v", err)
}
stopExpiration(t, c)

if _, ok := exp.irrevocable.Load(leaseID); ok {
t.Error("expiration manager irrevocable cache should be cleared on stop")
Expand Down
5 changes: 1 addition & 4 deletions vault/token_store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1170,10 +1170,7 @@ func TestTokenStore_CreateLookup_ExpirationInRestoreMode(t *testing.T) {
t.Fatalf("err: %v", err)
}

err = c.stopExpiration()
if err != nil {
t.Fatal(err)
}
stopExpiration(t, c)

// Reset expiration manager to restore mode
ts.expiration.restoreModeLock.Lock()
Expand Down

0 comments on commit 73e4e4d

Please sign in to comment.