fix(pushsecret): honor updatePolicy: None for operator-managed secrets#81
Open
matheusvellone wants to merge 1 commit into
Open
Conversation
The final "Check if any of the existing secrets values have changed" loop in ReconcileInfisicalPushSecret updated secrets in Infisical whenever the secret was originally created by the same resource, regardless of updatePolicy. The earlier branches already gate on updatePolicy correctly; this one short-circuited with `managedByOperator || ...`. The most visible symptom is a Password ClusterGenerator pushed via InfisicalPushSecret with updatePolicy: None: every reconcile (including the implicit one fired by CreateFunc on operator pod restart or leader election flip) regenerates the value and pushes it, even though the user explicitly opted out of updates. Drop the managedByOperator branch so updatePolicy is the single gate, matching the documented "defaults to no replacement" behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR changes the reconciliation logic for updating existing Infisical secrets by removing the “managed by operator” condition from the update decision.
Changes:
- Removes lookup of
managedByOperatorfromStatus.ManagedSecretsfor existing secrets. - Restricts secret updates to only when the
PUSH_SECRET_REPLACE_POLICY_ENABLEDupdate policy is set.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if secretValue != existingSecret.SecretValue { | ||
|
|
||
| if managedByOperator || updatePolicy == string(constants.PUSH_SECRET_REPLACE_POLICY_ENABLED) { | ||
| if updatePolicy == string(constants.PUSH_SECRET_REPLACE_POLICY_ENABLED) { |
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
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.
Summary
Closes #79
InfisicalPushSecretwithupdatePolicy: Nonecurrently overwrites secrets in Infisical anyway whenever the secret was originally created by the same resource (i.e. its ID is inStatus.ManagedSecrets). TheNonepolicy is only honored for pre-existing, unmanaged secrets — which makes it effectively a no-op for the most common use case: push a generated value once, never touch it again.The most visible symptom is a
PasswordClusterGeneratorpushed viaInfisicalPushSecretwithupdatePolicy: None: every reconcile (including the implicit one fired byCreateFuncon operator pod restart or leader-election flip) regenerates the value and pushes it, breaking any consumer still holding the previous value (Redis server, env-var-injected app pods, etc.).This is a one-line behavioral fix; no API changes.
Root cause
In
internal/services/infisicalpushsecret/reconciler.go, insideReconcileInfisicalPushSecret, the final loop (// Check if any of the existing secrets values have changed) had:The
managedByOperator ||clause short-circuits the policy check. Any secret whose ID is instatus.managedSecretswas updated on every reconcile where the source value differed — and for generator-backed pushes,processGeneratorsproduces a fresh value each call, so the values always differ.The two earlier branches in the same function (first-reconcile-create and "secret added later") already gate on
updatePolicycorrectly; this final loop was the outlier.A
push.secret-styleInfisicalPushSecretwithupdatePolicy: Noneappears to behave correctly because the sourcecorev1.Secretis stable, sosecretValue == existingSecret.SecretValueand the branch is skipped. But it is the same bug — just dormant until the source value changes.The fix
Drop
managedByOperator ||soupdatePolicyis the single gate, matching the documented behavior ("[updatePolicy] defaults to no replacement") on the InfisicalPushSecret CRD page.(With this gate, the unused
managedByOperatordeclaration is also removed.)Verification
Reproduced and validated against my staging cluster running
infisical/kubernetes-operator:v0.10.26:InfisicalPushSecret(updatePolicy: None) backed by aPasswordClusterGenerator.kubectl delete pod -n infisical-operator -l control-plane=controller-managerrotates the value in Infisical on every restart.push.secret-styleInfisicalPushSecretwithupdatePolicy: Replacestill updates Infisical when the source kubeSecretchanges. That path is unaffected by this change.go build ./...andgo vet ./...both clean.The existing
infisicalpushsecret_controller_test.gois a scaffolded stub with no real assertions for this branch; meaningful coverage would require mocking the Infisical Go SDK client. Happy to follow up with that in a separate PR if you'd like — wanted to keep this one minimal and focused on the behavioral fix.Backwards compatibility
For anyone currently relying on the bug (i.e.
updatePolicy: None+ apush.secret-style resource whose sourceSecretmutates and where they expected Infisical to get the updates), this is a behavior change — they should switch toupdatePolicy: Replace, which is the documented way to opt into updates. I'd argue the current behavior was already a documentation/implementation mismatch rather than a contract, so afix(pushsecret):commit is appropriate.Disclosure: I investigated this with Claude Code (Anthropic) — it traced the reconcile logic with me and helped draft this PR. I reproduced the bug against my own cluster (
v0.10.26), confirmed the fix locally, and reviewed every line of the diff and this PR body before opening.