Skip to content

Fix VDS bug where 2 leases are being generated on initial deployment #1054

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.

Sign up for GitHub

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Fix VDS bug where 2 leases are being generated on initial deployment #1054

Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5fe2ebb
Fix VDS controller where 2 leases are being generated on initial depl…
Zlaticanin Apr 22, 2025
b720864
update
Zlaticanin Apr 22, 2025
38b7dfe
fix
Zlaticanin Apr 22, 2025
b3c5b88
revert
Zlaticanin Apr 22, 2025
74c88dd
Merge branch 'main' into VAULT-35251/fix-vds-crd-bug
Zlaticanin Apr 22, 2025
f65c2e6
update to check if lease exists right before we try to sync
Zlaticanin Apr 23, 2025
094c61b
fix tests
Zlaticanin Apr 23, 2025
6b6d05e
Add a check to see if the lease is renewable
Zlaticanin Apr 23, 2025
bcbc556
fix tests
Zlaticanin Apr 23, 2025
eafe9a7
check if in window
Zlaticanin Apr 23, 2025
6b8a499
fix tests
Zlaticanin Apr 23, 2025
bd8a3d2
fix
Zlaticanin Apr 23, 2025
4a4dc56
add check for static creds
Zlaticanin Apr 23, 2025
e024c75
move maybeAddFinalizer out of updateStatus func into the Reconcile func
Zlaticanin May 5, 2025
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
10 changes: 8 additions & 2 deletions controllers/vaultdynamicsecret_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,13 @@ func (r *VaultDynamicSecretReconciler) Reconcile(ctx context.Context, req ctrl.R
return ctrl.Result{}, r.handleDeletion(ctx, o)
}

if addedFinalizer, err := maybeAddFinalizer(ctx, r.Client, o, vaultDynamicSecretFinalizer); err != nil {
return ctrl.Result{}, err
} else if addedFinalizer {
// the finalizer was added, requeue the request.
return ctrl.Result{Requeue: true}, nil
}
Comment on lines +126 to +131
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a comment in maybeAddFinalizer():

vault-secrets-operator/controllers/common.go

Lines 248 to 249 in e024c75

// always call maybeAddFinalizer() after client.Client.Status.Update() to avoid
// API validation errors due to changes to the status schema.

Any concern with API validation errors now that maybeAddFinalizer() is being called before updateStatus()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reviewing! My understanding for the comment is that an API validation error would happen if both metadata (finalizers) and status of a resource are updated in the same reconcile pass. But in our case, we are calling maybeAddFinalizer() first, and if a finalizer is added, we exit early and requeue. So we shouldn't be touching the status in the same loop, which would avoid the validation issue because the next reconcile handles the status update separately. Please let me know if I am misunderstanding!


r.referenceCache.Set(SecretTransformation, req.NamespacedName,
helpers.GetTransformationRefObjKeys(
o.Spec.Destination.Transformation, o.Namespace)...)
Expand Down Expand Up @@ -558,8 +565,7 @@ func (r *VaultDynamicSecretReconciler) updateStatus(ctx context.Context, o *secr
"Failed to update the resource's status, err=%s", err)
}

_, err := maybeAddFinalizer(ctx, r.Client, o, vaultDynamicSecretFinalizer)
return err
return nil
}

func (r *VaultDynamicSecretReconciler) getVaultSecretLease(resp *api.Secret) *secretsv1beta1.VaultSecretLease {
Expand Down