Skip to content

Conversation

@Zlaticanin
Copy link
Collaborator

@Zlaticanin Zlaticanin commented Apr 22, 2025

This PR fixes a race condition that could cause two Vault leases to be created during the initial reconcile of VDS. This happened because the finalizer was added after the status update (LastGeneration = generation), which caused the generation to be bumped again, triggering another reconcile that passed the predicate filters and re-ran sync logic.

I moved the maybeAddFinalizer logic to run before setting status or performing sync logic. If a finalizer is added, we requeue and exit early. By doing this the generation bump caused by adding the finalizer is processed cleanly, and the reconcile won't proceed to status updates or sync logic prematurely.

@Zlaticanin Zlaticanin requested a review from a team as a code owner April 22, 2025 20:46
@Zlaticanin Zlaticanin marked this pull request as draft April 22, 2025 21:37
@Zlaticanin Zlaticanin marked this pull request as ready for review April 23, 2025 23:51
@Zlaticanin Zlaticanin requested a review from benashz April 24, 2025 17:12
Copy link
Collaborator

@benashz benashz left a comment

Choose a reason for hiding this comment

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

Could we add tests for this case.

@Zlaticanin Zlaticanin requested a review from benashz April 29, 2025 19:42
@Zlaticanin Zlaticanin requested review from benashz and tvoran and removed request for benashz May 5, 2025 20:59
Comment on lines +126 to +131
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
}
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():

// 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants