Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: ensure unneeded Vault clients are pruned from the cache #806

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/v1beta1/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package v1beta1

import (
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// Destination provides the configuration that will be applied to the
Expand Down Expand Up @@ -123,4 +124,6 @@ type VaultClientMeta struct {
// ID is the Vault ID of the authenticated client. The ID should never contain
// any sensitive information.
ID string `json:"id,omitempty"`
// CreationTimestamp is the time the client was created.
CreationTimestamp metav1.Time `json:"creationTimestamp,omitempty"`
}
3 changes: 3 additions & 0 deletions api/v1beta1/vaultpkisecret_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,9 @@ type VaultPKISecretStatus struct {
SecretMAC string `json:"secretMAC,omitempty"`
Valid bool `json:"valid"`
Error string `json:"error"`
// VaultClientMeta contains the status of the Vault client and is used during
// resource reconciliation.
VaultClientMeta VaultClientMeta `json:"vaultClientMeta,omitempty"`
}

//+kubebuilder:object:root=true
Expand Down
3 changes: 3 additions & 0 deletions api/v1beta1/vaultstaticsecret_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ type VaultStaticSecretStatus struct {
// The SecretMac is also used to detect drift in the Destination Secret's Data.
// If drift is detected the data will be synced to the Destination.
SecretMAC string `json:"secretMAC,omitempty"`
// VaultClientMeta contains the status of the Vault client and is used during
// resource reconciliation.
VaultClientMeta VaultClientMeta `json:"vaultClientMeta,omitempty"`
}

//+kubebuilder:object:root=true
Expand Down
11 changes: 7 additions & 4 deletions api/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions chart/crds/secrets.hashicorp.com_vaultdynamicsecrets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,10 @@ spec:
description: CacheKey is the unique key used to identify the client
cache.
type: string
creationTimestamp:
description: CreationTimestamp is the time the client was created.
format: date-time
type: string
id:
description: |-
ID is the Vault ID of the authenticated client. The ID should never contain
Expand Down
19 changes: 19 additions & 0 deletions chart/crds/secrets.hashicorp.com_vaultpkisecrets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,25 @@ spec:
type: string
valid:
type: boolean
vaultClientMeta:
description: |-
VaultClientMeta contains the status of the Vault client and is used during
resource reconciliation.
properties:
cacheKey:
description: CacheKey is the unique key used to identify the client
cache.
type: string
creationTimestamp:
description: CreationTimestamp is the time the client was created.
format: date-time
type: string
id:
description: |-
ID is the Vault ID of the authenticated client. The ID should never contain
any sensitive information.
type: string
type: object
required:
- error
- lastGeneration
Expand Down
19 changes: 19 additions & 0 deletions chart/crds/secrets.hashicorp.com_vaultstaticsecrets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,25 @@ spec:
The SecretMac is also used to detect drift in the Destination Secret's Data.
If drift is detected the data will be synced to the Destination.
type: string
vaultClientMeta:
description: |-
VaultClientMeta contains the status of the Vault client and is used during
resource reconciliation.
properties:
cacheKey:
description: CacheKey is the unique key used to identify the client
cache.
type: string
creationTimestamp:
description: CreationTimestamp is the time the client was created.
format: date-time
type: string
id:
description: |-
ID is the Vault ID of the authenticated client. The ID should never contain
any sensitive information.
type: string
type: object
required:
- lastGeneration
type: object
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,10 @@ spec:
description: CacheKey is the unique key used to identify the client
cache.
type: string
creationTimestamp:
description: CreationTimestamp is the time the client was created.
format: date-time
type: string
id:
description: |-
ID is the Vault ID of the authenticated client. The ID should never contain
Expand Down
19 changes: 19 additions & 0 deletions config/crd/bases/secrets.hashicorp.com_vaultpkisecrets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,25 @@ spec:
type: string
valid:
type: boolean
vaultClientMeta:
description: |-
VaultClientMeta contains the status of the Vault client and is used during
resource reconciliation.
properties:
cacheKey:
description: CacheKey is the unique key used to identify the client
cache.
type: string
creationTimestamp:
description: CreationTimestamp is the time the client was created.
format: date-time
type: string
id:
description: |-
ID is the Vault ID of the authenticated client. The ID should never contain
any sensitive information.
type: string
type: object
required:
- error
- lastGeneration
Expand Down
19 changes: 19 additions & 0 deletions config/crd/bases/secrets.hashicorp.com_vaultstaticsecrets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,25 @@ spec:
The SecretMac is also used to detect drift in the Destination Secret's Data.
If drift is detected the data will be synced to the Destination.
type: string
vaultClientMeta:
description: |-
VaultClientMeta contains the status of the Vault client and is used during
resource reconciliation.
properties:
cacheKey:
description: CacheKey is the unique key used to identify the client
cache.
type: string
creationTimestamp:
description: CreationTimestamp is the time the client was created.
format: date-time
type: string
id:
description: |-
ID is the Vault ID of the authenticated client. The ID should never contain
any sensitive information.
type: string
type: object
required:
- lastGeneration
type: object
Expand Down
67 changes: 36 additions & 31 deletions controllers/vaultdynamicsecret_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R
return ctrl.Result{}, nil
}
logger.Error(err, "error getting resource from k8s", "obj", o)
return ctrl.Result{}, err
return ctrl.Result{RequeueAfter: requeueDurationOnError}, nil
}

if o.GetDeletionTimestamp() != nil {
Expand All @@ -132,21 +132,29 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R
return ctrl.Result{RequeueAfter: requeueDurationOnError}, nil
}

vClient, err := r.ClientFactory.Get(ctx, r.Client, o)
defer func() {
if err := r.updateStatus(ctx, o); err != nil {
logger.Error(err, "Failed to update resource status")
}
}()

c, err := r.ClientFactory.Get(ctx, r.Client, o)
if err != nil {
o.Status.VaultClientMeta = secretsv1beta1.VaultClientMeta{}
r.Recorder.Eventf(o, corev1.EventTypeWarning, consts.ReasonVaultClientConfigError,
"Failed to get Vault client: %s", err)
return ctrl.Result{RequeueAfter: computeHorizonWithJitter(requeueDurationOnError)}, nil
}

// we can ignore the error here, since it was handled above in the Get() call.
clientCacheKey, _ := vClient.GetCacheKey()
clientCacheKey, _ := c.GetCacheKey()
lastClientCacheKey := o.Status.VaultClientMeta.CacheKey
lastClientID := o.Status.VaultClientMeta.ID

// update the VaultClientMeta in the resource's status.
o.Status.VaultClientMeta.CacheKey = clientCacheKey.String()
o.Status.VaultClientMeta.ID = vClient.ID()
o.Status.VaultClientMeta.ID = c.ID()
o.Status.VaultClientMeta.CreationTimestamp = metav1.NewTime(c.Stat().CreationTimestamp())

var syncReason string
// doSync indicates that the controller should perform the secret sync,
Expand Down Expand Up @@ -192,9 +200,6 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R
r.Recorder.Eventf(o, corev1.EventTypeNormal, consts.ReasonSecretLeaseRenewal,
"Not in renewal window after transitioning to a new leader/pod, lease_id=%s, horizon=%s",
leaseID, horizon)
if err := r.updateStatus(ctx, o); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{RequeueAfter: horizon}, nil
}
} else if inWindow {
Expand All @@ -203,16 +208,13 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R
r.Recorder.Eventf(o, corev1.EventTypeNormal, consts.ReasonSecretLeaseRenewal,
"In rotation period after transitioning to a new leader/pod, lease_id=%s, horizon=%s",
leaseID, horizon)
if err := r.updateStatus(ctx, o); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{RequeueAfter: horizon}, nil
}
}

if !doSync && r.isRenewableLease(&o.Status.SecretLease, o, true) && !o.Spec.AllowStaticCreds && leaseID != "" {
// Renew the lease and return from Reconcile if the lease is successfully renewed.
if secretLease, err := r.renewLease(ctx, vClient, o); err == nil {
if secretLease, err := r.renewLease(ctx, c, o); err == nil {
if !r.isRenewableLease(secretLease, o, false) {
return ctrl.Result{}, nil
}
Expand All @@ -227,9 +229,6 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R
o.Status.StaticCredsMetaData = secretsv1beta1.VaultStaticCredsMetaData{}
o.Status.SecretLease = *secretLease
o.Status.LastRenewalTime = nowFunc().Unix()
if err := r.updateStatus(ctx, o); err != nil {
return ctrl.Result{}, err
}

leaseDuration := time.Duration(secretLease.LeaseDuration) * time.Second
if leaseDuration < 1 {
Expand All @@ -252,7 +251,7 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R
"Could not renew lease, lease_id=%s, err=%s", leaseID, err)
} else if vault.IsForbiddenError(err) {
logger.V(consts.LogLevelWarning).Info("Tainting client", "err", err)
vClient.Taint()
c.Taint()
}
syncReason = consts.ReasonSecretLeaseRenewalError
}
Expand All @@ -271,12 +270,12 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R
}

// sync the secret
secretLease, staticCredsUpdated, err := r.syncSecret(ctx, vClient, o, transOption)
secretLease, staticCredsUpdated, err := r.syncSecret(ctx, c, o, transOption)
if err != nil {
r.SyncRegistry.Add(req.NamespacedName)
if vault.IsForbiddenError(err) {
logger.V(consts.LogLevelWarning).Info("Tainting client", "err", err)
vClient.Taint()
c.Taint()
}
entry, _ := r.BackOffRegistry.Get(req.NamespacedName)
horizon := entry.NextBackOff()
Expand All @@ -292,10 +291,6 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R
doRolloutRestart := (doSync && o.Status.LastGeneration > 1) || staticCredsUpdated
o.Status.SecretLease = *secretLease
o.Status.LastRenewalTime = nowFunc().Unix()
if err := r.updateStatus(ctx, o); err != nil {
return ctrl.Result{}, err
}

horizon := r.computePostSyncHorizon(ctx, o)
r.Recorder.Eventf(o, corev1.EventTypeNormal, reason,
"Secret synced, lease_id=%q, horizon=%s, sync_reason=%q",
Expand Down Expand Up @@ -641,16 +636,19 @@ func (r *VaultDynamicSecretReconciler) SetupWithManager(mgr ctrl.Manager, opts c
// * revoking any associated outstanding leases
// * removing our finalizer
func (r *VaultDynamicSecretReconciler) handleDeletion(ctx context.Context, o *secretsv1beta1.VaultDynamicSecret) error {
logger := log.FromContext(ctx)
// We are ignoring errors inside `revokeLease`, otherwise we may fail to remove the finalizer.
logger := log.FromContext(ctx).WithName("handleDeletion")
// We are ignoring errors inside `handleLeaseRevocation`, otherwise we may fail to remove the finalizer.
// Worst case at this point we will leave a dangling lease instead of a secret which
// cannot be deleted. Events are emitted in these cases.
r.revokeLease(ctx, o, "")
r.handleLeaseRevocation(ctx, o)

objKey := client.ObjectKeyFromObject(o)
r.SyncRegistry.Delete(objKey)
r.BackOffRegistry.Delete(objKey)
r.referenceCache.Remove(SecretTransformation, objKey)
if err := r.ClientFactory.UnregisterObjectRef(ctx, o); err != nil {
logger.Error(err, "Warning: failed to unregister object reference")
}
if controllerutil.ContainsFinalizer(o, vaultDynamicSecretFinalizer) {
logger.Info("Removing finalizer")
if controllerutil.RemoveFinalizer(o, vaultDynamicSecretFinalizer) {
Expand All @@ -664,18 +662,25 @@ func (r *VaultDynamicSecretReconciler) handleDeletion(ctx context.Context, o *se
return nil
}

// revokeLease revokes the VDS secret's lease.
// handleLeaseRevocation revokes the VDS secret's lease.
// NOTE: Enabling revocation requires the VaultAuthMethod referenced by `o.Spec.VaultAuthRef` to have a policy
// that includes `path "sys/leases/revoke" { capabilities = ["update"] }`, otherwise this will fail with permission
// errors.
func (r *VaultDynamicSecretReconciler) revokeLease(ctx context.Context, o *secretsv1beta1.VaultDynamicSecret, id string) {
logger := log.FromContext(ctx)
func (r *VaultDynamicSecretReconciler) handleLeaseRevocation(ctx context.Context, o *secretsv1beta1.VaultDynamicSecret) {
logger := log.FromContext(ctx).WithName("handleLeaseRevocation")
// Allow us to override the SecretLease in the event that we want to revoke an old lease.
leaseID := id
if !o.Spec.Revoke {
logger.V(consts.LogLevelDebug).Info("Lease revocation not enabled")
return
}

leaseID := o.Status.SecretLease.ID
if leaseID == "" {
leaseID = o.Status.SecretLease.ID
logger.Info("No lease to revoke")
return
}
logger.Info("Revoking lease for credential ", "id", leaseID)

logger.V(consts.LogLevelDebug).Info("Revoking lease for credential ", "id", leaseID)
c, err := r.ClientFactory.Get(ctx, r.Client, o)
if err != nil {
logger.Error(err, "Failed to get client when revoking lease for ", "id", leaseID)
Expand All @@ -690,7 +695,7 @@ func (r *VaultDynamicSecretReconciler) revokeLease(ctx context.Context, o *secre
} else {
msg := "Lease revoked"
r.Recorder.Eventf(o, corev1.EventTypeNormal, consts.ReasonSecretLeaseRevoke, msg+": %s", leaseID)
logger.Info("Lease revoked ", "id", leaseID)
logger.V(consts.LogLevelDebug).Info("Lease revoked ", "id", leaseID)
}
}

Expand Down
Loading