FIX: Return error on Vault failure paths to prevent VSS from dropping off work queue - #1323
Merged
Conversation
siyer-corp
reviewed
Aug 18, 2026
Comment 1 (siyer-corp): Drop the new ReasonSecretUpToDate constant and the
newUpToDateCondition helper. Instead, reuse Reason=Synced with
ConditionTrue in the doSync=false branch. This avoids a breaking change
for scripts, alerting rules, and Argo CD health checks that match on the
existing reason value.
Comment 2 (siyer-corp): Refactor vaultstaticsecret_controller_test.go to
follow the conventions used by vaultdynamicsecret_controller_test.go:
- Add an err field to the shared reconcileTestClientFactory (defined in
vaultdynamicsecret_controller_test.go) instead of introducing a new
errClientFactory type
- Use reconcileTestVaultClient with MockRecordingVaultClient{CheckPaths:true}
instead of a separate errReadVaultClient/okReadVaultClient type
- Construct the reconciler inline in each subtest rather than via helper
functions, so setup is immediately visible
- Collapse the four separate test functions into a single table-driven
TestVaultStaticSecretReconciler_Reconcile
Collaborator
|
Can we update the title of the PR with the actual fix being made? |
tejashwiniingalagi
changed the title
tejashwiniingalagi
changed the title
sanvikam79
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Problem
After a Vault HA event (leader re-election, pod restart), VSO emits transient errors like:
"local node not active but active cluster node not found"
This is expected and self-resolving — once the new leader is established, Vault recovers. However, VSO permanently stopped reconciling the affected VaultStaticSecret resources. Days later, secrets were stale with zero controller log entries since the failure. Restarting the VSO pod alone did not recover them; the only workaround was a manual spec-touch to force a re-enqueue.
Root Cause
Two error return paths in VaultStaticSecretReconciler.Reconcile returned nil error alongside RequeueAfter:
// Before — both error paths
return ctrl.Result{RequeueAfter: horizon}, nil
controller-runtime treats a nil error + RequeueAfter as a one-shot delayed retry. Once that timer fires and Vault has not yet recovered, no further retries are scheduled — the resource is silently dropped from the work queue.
A secondary issue: when hmacSecretData=true and the Vault read succeeds but the secret data is unchanged (doSync=false), the SecretSynced condition was never updated — leaving it permanently False even after full recovery.
Fix
// After
return ctrl.Result{RequeueAfter: horizon}, err
Returning a non-nil error alongside RequeueAfter tells controller-runtime to place the resource back into the rate-limiter queue. It will keep retrying with exponential backoff via BackOffRegistry (5s → 60s plateau) indefinitely until Vault recovers — no spec-touch required.
When HMAC comparison shows the secret data is unchanged, the reconciler previously wrote no condition at all — leaving a stale SecretSynced=False from the prior failure. Now both the sync and no-sync paths always write the condition, accurately reflecting the healthy post-recovery state.
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.