From 5688d246afd358a99b757970fcf0534bf71dd35c Mon Sep 17 00:00:00 2001 From: Michael Blaum <96261585+hashiblaum@users.noreply.github.com> Date: Thu, 22 May 2025 16:19:14 -0400 Subject: [PATCH] refactor changes for irrevocable lease removal (#30703) * refactor changes for irrevocable lease removal * changelog addition * add logs * update changelog; always sleep during irrevocable lease revoke party * comment * typo * pass Core pntr to removeIrrevocableLeasesEnabled * remove log --- changelog/30703.txt | 3 + vault/expiration.go | 105 +++++++++--------- vault/expiration_ce.go | 17 --- ...piration_util.go => expiration_util_ce.go} | 4 + vault/expiration_util_ce_test.go | 18 +++ 5 files changed, 79 insertions(+), 68 deletions(-) create mode 100644 changelog/30703.txt delete mode 100644 vault/expiration_ce.go rename vault/{expiration_util.go => expiration_util_ce.go} (90%) create mode 100644 vault/expiration_util_ce_test.go diff --git a/changelog/30703.txt b/changelog/30703.txt new file mode 100644 index 0000000000..d719d7a8c0 --- /dev/null +++ b/changelog/30703.txt @@ -0,0 +1,3 @@ +```release-note:feature +**Auto Irrevocable Lease Removal (enterprise)**: Add the Vault Enterprise configuration param, `remove_irrevocable_lease_after`. When set to a non-zero value, this will automatically delete irrevocable leases after the configured duration exceeds the lease's expire time. The minimum duration allowed for this field is two days. +``` diff --git a/vault/expiration.go b/vault/expiration.go index 527ee6c2fa..ab5b38521f 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -128,6 +128,8 @@ type ExpirationManager struct { // This value is protected by pendingLock irrevocableLeaseCount int + irrevocableLeaseRemovalEnabled bool + // The uniquePolicies map holds policy sets, so they can // be deduplicated. It is periodically emptied to prevent // unbounded growth. @@ -419,6 +421,10 @@ func (c *Core) setupExpiration(e ExpireLeaseStrategy) error { } go c.expiration.Restore(errorFunc) + if mgr.removeIrrevocableLeasesEnabled(c) { + mgr.irrevocableLeaseRemovalEnabled = true + } + quit := c.expiration.quitCh go func() { t := time.NewTimer(24 * time.Hour) @@ -964,9 +970,6 @@ func (m *ExpirationManager) lazyRevokeInternal(ctx context.Context, leaseID stri // should be run on a schedule. something like once a day, maybe once a week func (m *ExpirationManager) attemptIrrevocableLeasesRevoke(removeIrrevocableLeaseAfter time.Duration) { - // determine if irrevocable leases should be evaluated for removal after revocation attempt - removeIrrevocableLeases := m.removeIrrevocableLeasesEnabled(removeIrrevocableLeaseAfter) - m.irrevocable.Range(func(k, v interface{}) bool { leaseID := k.(string) le := v.(*leaseEntry) @@ -988,17 +991,26 @@ func (m *ExpirationManager) attemptIrrevocableLeasesRevoke(removeIrrevocableLeas ctxWithNS := namespace.ContextWithNamespace(m.core.activeContext, leaseNS) ctxWithNSAndTimeout, _ := context.WithTimeout(ctxWithNS, time.Minute) - // attempt to remove irrevocable lease if feature enabled - if removeIrrevocableLeases && le.ExpireTime.Add(removeIrrevocableLeaseAfter).Before(time.Now()) { - defer m.deleteIrrevocableLease(ctxWithNSAndTimeout, le) + // remove irrevocable lease if 'remove irrevocable leases' feature is enabled + var forceRevoke bool + if m.irrevocableLeaseRemovalEnabled && le.ExpireTime.Add(removeIrrevocableLeaseAfter).Before(time.Now()) { + forceRevoke = true } - if err := m.revokeCommon(ctxWithNSAndTimeout, leaseID, false, false); err != nil { - // on failure, force some delay to mitigate resource spike while - // this is running. if revocations succeed, we are okay with - // the higher resource consumption. - time.Sleep(10 * time.Millisecond) + err = m.revokeCommon(ctxWithNSAndTimeout, leaseID, forceRevoke, false) + + // Log the appropriate message on errors or force revocation + if forceRevoke { + if err != nil { + m.logger.Debug("failed to delete irrevocable lease", "lease_id", le.LeaseID, "err", err) + } else { + m.logger.Debug("deleted irrevocable lease", "lease_id", le.LeaseID, "lease_expire_time", le.ExpireTime) + } } + + // Force some delay to mitigate resource spike while + // this is running. + time.Sleep(10 * time.Millisecond) } return true @@ -1007,9 +1019,8 @@ func (m *ExpirationManager) attemptIrrevocableLeasesRevoke(removeIrrevocableLeas // revokeCommon does the heavy lifting. If force is true, we ignore a problem // during revocation and still remove entries/index/lease timers -func (m *ExpirationManager) revokeCommon(ctx context.Context, leaseID string, force, skipToken bool) error { +func (m *ExpirationManager) revokeCommon(ctx context.Context, leaseID string, force, skipToken bool) (err error) { defer metrics.MeasureSince([]string{"expire", "revoke-common"}, time.Now()) - if !skipToken { // Acquire lock for this lease // If skipToken is true, then we're either being (1) called via RevokeByToken, so @@ -1039,51 +1050,18 @@ func (m *ExpirationManager) revokeCommon(ctx context.Context, leaseID string, fo } if m.logger.IsWarn() { - m.logger.Warn("revocation from the backend failed, but in force mode so ignoring", "error", err) + m.logger.Warn("revocation from the backend failed, but in force mode so ignoring. external resources may be orphaned.", "lease_id", leaseID, "error", err) } } } - err = m.deleteLeaseCommon(ctx, le) - if err != nil { - return err - } - - if !skipToken { - err = m.core.observations.RecordObservationToLedger(ctx, observations.ObservationTypeLeaseRevocation, le.namespace, map[string]interface{}{ - "lease_id": leaseID, - "path": le.Path, - "issue_time": le.IssueTime, - "expire_time": le.ExpireTime, - }) - if err != nil { - if !force { - return err - } - } - } - - if m.logger.IsInfo() && !skipToken && m.logLeaseExpirations { - m.logger.Info("revoked lease", "lease_id", leaseID) - } - if m.logger.IsWarn() && !skipToken && le.isIncorrectlyNonExpiring() { - var accessor string - if le.Auth != nil { - accessor = le.Auth.Accessor - } - m.logger.Warn("finished revoking incorrectly non-expiring lease", "leaseID", le.LeaseID, "accessor", accessor) - } - return nil -} - -func (m *ExpirationManager) deleteLeaseCommon(ctx context.Context, le *leaseEntry) error { // Delete the entry if err := m.deleteEntry(ctx, le); err != nil { return err } // Lease has been removed, also remove the in-memory lock. - m.deleteLockForLease(le.LeaseID) + m.deleteLockForLease(leaseID) // Delete the secondary index, but only if it's a leased secret (not auth) if le.Secret != nil { @@ -1114,14 +1092,39 @@ func (m *ExpirationManager) deleteLeaseCommon(ctx context.Context, le *leaseEntr // Clear the expiration handler m.pendingLock.Lock() - m.removeFromPending(ctx, le.LeaseID, true) - m.nonexpiring.Delete(le.LeaseID) + m.removeFromPending(ctx, leaseID, true) + m.nonexpiring.Delete(leaseID) if _, ok := m.irrevocable.Load(le.LeaseID); ok { - m.irrevocable.Delete(le.LeaseID) + m.irrevocable.Delete(leaseID) m.irrevocableLeaseCount-- } m.pendingLock.Unlock() + + if !skipToken { + err = m.core.observations.RecordObservationToLedger(ctx, observations.ObservationTypeLeaseRevocation, le.namespace, map[string]interface{}{ + "lease_id": leaseID, + "path": le.Path, + "issue_time": le.IssueTime, + "expire_time": le.ExpireTime, + }) + if err != nil { + if !force { + return err + } + } + } + + if m.logger.IsInfo() && !skipToken && m.logLeaseExpirations { + m.logger.Info("revoked lease", "lease_id", leaseID) + } + if m.logger.IsWarn() && !skipToken && le.isIncorrectlyNonExpiring() { + var accessor string + if le.Auth != nil { + accessor = le.Auth.Accessor + } + m.logger.Warn("finished revoking incorrectly non-expiring lease", "leaseID", le.LeaseID, "accessor", accessor) + } return nil } diff --git a/vault/expiration_ce.go b/vault/expiration_ce.go deleted file mode 100644 index d7f2b37b9a..0000000000 --- a/vault/expiration_ce.go +++ /dev/null @@ -1,17 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -//go:build !enterprise - -package vault - -import ( - "context" - "time" -) - -func (m *ExpirationManager) deleteIrrevocableLease(ctx context.Context, le *leaseEntry) {} - -func (m *ExpirationManager) removeIrrevocableLeasesEnabled(removeIrrevocableLeaseAfter time.Duration) bool { - return false -} diff --git a/vault/expiration_util.go b/vault/expiration_util_ce.go similarity index 90% rename from vault/expiration_util.go rename to vault/expiration_util_ce.go index f59f36af3a..2be5e375d2 100644 --- a/vault/expiration_util.go +++ b/vault/expiration_util_ce.go @@ -31,3 +31,7 @@ func (m *ExpirationManager) collectLeases() (map[*namespace.Namespace][]string, leaseCount += len(keys) return existing, leaseCount, nil } + +func (m *ExpirationManager) removeIrrevocableLeasesEnabled(c *Core) bool { + return false +} diff --git a/vault/expiration_util_ce_test.go b/vault/expiration_util_ce_test.go new file mode 100644 index 0000000000..cbc0d4766d --- /dev/null +++ b/vault/expiration_util_ce_test.go @@ -0,0 +1,18 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +//go:build !enterprise + +package vault + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestExpiration_IrrevocableLeaseRemovalDisabled verifies that the irrevocable lease removal is disabled on Vault CE +func TestExpiration_IrrevocableLeaseRemovalDisabled(t *testing.T) { + exp := mockExpiration(t) + require.Equal(t, false, exp.irrevocableLeaseRemovalEnabled) +}