fix(operator): avoid encoded postgres secret collision

[flemzord]

Summary

• avoid overwriting a user-provided Postgres credentials Secret when its name collides with the derived encoded credentials Secret name
• keep the existing derived Secret name for non-conflicting deployments, and use a collision-safe name only when needed
• only delete encoded credentials Secrets controlled by the Database reconciler, so cleanup cannot remove an uncontrolled source Secret
• add regression tests for env rendering, Secret reconciliation idempotence, and cleanup behavior

Root cause

v3.12.0 introduced a derived Secret named <database>-postgres-uri-credentials for URL-encoded Postgres credentials. A real deployment can already use that exact name as the source Secret referenced by ?secret=... (for example formance-ledger-postgres-uri-credentials). In that case, the Database reconciler writes encoded credentials back into the source Secret it just read, turning its input into its output and causing repeated reconciliations/rollouts around Ledger deployments.

Helm / release context

formance-1.15.0 resolves regions to 3.12.0, which resolves the Operator chart to 3.12.0; that is how the Operator change from #484 reached users. No Helm template change is needed for the fix itself, but a follow-up Operator release and Helm dependency refresh will be needed to ship it through the umbrella chart.

Validation

• CGO_ENABLED=0 go test ./internal/resources/databases -run 'Test(GetPostgresEnvVarsAvoidsEncodedSecretNameCollision|ReconcileEncodedPostgresCredentialsSecretAvoidsSourceSecretNameCollision|DeleteEncodedPostgresCredentialsSecretKeepsUncontrolledSourceSecret)' -count=1 -v
• KUBEBUILDER_ASSETS="$(pwd)/bin/k8s/1.32.0-darwin-arm64" CGO_ENABLED=0 go test ./...
• nix develop -c just pre-commit

Operational checks

• prod-eu-west-1-hosting: Operator is still v3.11.2; Ledger databases use ?secret=..., so it is exposed if upgraded to 3.12.0 before this fix.
• staging-eu-west-1-hosting: Operator is on the post-v3.12.0 image, but visible Ledger databases use inline credentials, so it does not exercise the affected ?secret=... path.
• No matching object has been modified / POSTGRES_URL_ENCODED / postgres-uri-credentials loop was found in the last 24h of our prod/staging hosting operator logs.

[coderabbitai]

Warning

Review limit reached

@flemzord, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 15221269-6eeb-442b-8b43-f32be46c8a0e

📥 Commits

Reviewing files that changed from the base of the PR and between e55f810 and ef34297.

⛔ Files ignored due to path filters (1)

• docs/09-Configuration reference/settings.catalog.json is excluded by !**/*.json

📒 Files selected for processing (5)

• internal/resources/databases/env.go
• internal/resources/databases/env_test.go
• internal/resources/databases/init.go
• internal/resources/databases/secret.go
• internal/resources/databases/secret_test.go

Walkthrough

This PR changes Postgres encoded-credentials secret naming to avoid collisions with the source secret, updates env var generation and reconcile/delete flows to use collision-safe names and ownership checks, and adds tests for collision handling and uncontrolled-secret preservation.

Changes

Encoded secret name collision fix

Layer / File(s)	Summary
Collision-safe naming helpers internal/resources/databases/secret.go	Adds hashing-based encoded-secret naming helpers and candidate-name generation for collision-safe reconciliation and deletion.
Env var wiring uses collision-safe name internal/resources/databases/env.go, internal/resources/databases/env_test.go	GetPostgresEnvVars now uses the source secret when deriving encoded username/password env secrets, and a new test covers the collision case.
Reconcile preserves previous source secret reference internal/resources/databases/init.go	The reconcile path now looks up the prior Postgres credentials secret reference before deleting encoded credentials secrets when the URI no longer includes secret.
Secret reconciliation and stale cleanup internal/resources/databases/secret.go, internal/resources/databases/secret_test.go	Reconciliation removes controller references from the source secret, writes the encoded secret under the collision-safe name, and deletes stale candidate secrets; tests cover collision avoidance and source-data preservation.
Delete remediation and controlled deletion internal/resources/databases/init.go, internal/resources/databases/secret.go, internal/resources/databases/secret_test.go	Deletion now remediates the source secret reference before database removal, and encoded-secret deletion only proceeds for controlled secrets; tests confirm uncontrolled source secrets remain unchanged.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• formancehq/operator#484: Both PRs modify the Postgres ?secret=... env-var and encoded-credentials secret flow in internal/resources/databases/*.

Suggested reviewers: sofiasimdianova

Poem

I hopped by the secrets, neat and bright,
and named them so they wouldn’t fight.
One secret, two paths, no tangled nest —
the encoded ones now pass the test 🐰

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely matches the main change: preventing encoded Postgres secret name collisions.
Description check	✅ Passed	The description is directly related to the fix and accurately summarizes the collision issue, remediation, and tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch agent/fix-postgres-secret-collision

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[NumaryBot]

✅ Approve — automated review

All significant findings raised during the review cycle have been addressed by the author across multiple follow-up commits. The stale controller ownerReference remediation, the delete/finalizer path remediation, the hashed fallback naming to prevent secondary collisions, the existence-check guard before CreateOrUpdate, the stale hashed fallback cleanup, the operation ordering fix (encoded Secret deleted before ResourceReference), and the annotation-based detection of operator-created Secrets have all been implemented with corresponding regression tests. The single remaining finding from one reviewer (codex-1) describes a migration scenario where a previously-colliding source Secret could be overwritten, but this scenario is directly addressed by the annotation-based operator-created Secret detection (fbf8125) and the ensureEncodedPostgresCredentialsSecretCanBeWritten guard, which rejects writing to any uncontrolled existing Secret. No new unresolved blockers or major issues are identified.

No findings.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #487 (comment)

[coderabbitai]

🧹 Nitpick comments (1)

internal/resources/databases/secret.go (1)

85-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding a test for stale encoded secret cleanup during reconciliation.

The deleteStaleEncodedPostgresCredentialsSecrets function deletes candidate encoded secrets that are neither the source nor the desired name, but only if controlled by the database. No test currently verifies this cleanup path — e.g., a scenario where a previous deployment used the collision-safe name and the source secret later changes to a non-colliding name, leaving a stale controlled secret to be cleaned up.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/resources/databases/secret.go` around lines 85 - 95, The cleanup
path in deleteStaleEncodedPostgresCredentialsSecrets is not covered by tests, so
add a reconciliation test that exercises stale encoded secret removal when a
previous collision-safe secret becomes obsolete after the source secret changes.
Use deleteStaleEncodedPostgresCredentialsSecrets and
deleteEncodedPostgresCredentialsSecretIfControlled in the test setup to verify
that candidate names other than sourceSecretName and desiredSecretName are
deleted only when controlled by the Database, while current source/desired
secrets are preserved.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/resources/databases/secret.go`:
- Around line 85-95: The cleanup path in
deleteStaleEncodedPostgresCredentialsSecrets is not covered by tests, so add a
reconciliation test that exercises stale encoded secret removal when a previous
collision-safe secret becomes obsolete after the source secret changes. Use
deleteStaleEncodedPostgresCredentialsSecrets and
deleteEncodedPostgresCredentialsSecretIfControlled in the test setup to verify
that candidate names other than sourceSecretName and desiredSecretName are
deleted only when controlled by the Database, while current source/desired
secrets are preserved.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d6139b1c-26de-4efd-885d-77b4bd4aa55d

📥 Commits

Reviewing files that changed from the base of the PR and between 519015f and b98f6d4.

⛔ Files ignored due to path filters (1)

• docs/09-Configuration reference/settings.catalog.json is excluded by !**/*.json

📒 Files selected for processing (4)

• internal/resources/databases/env.go
• internal/resources/databases/env_test.go
• internal/resources/databases/secret.go
• internal/resources/databases/secret_test.go

[flemzord]

Addressed in the latest push (643a686):

• remediates already-affected clusters by removing the stale Database controller ownerReference from the source Postgres credentials Secret during reconciliation;
• keeps the user-provided source Secret data intact while creating the collision-safe encoded Secret;
• adds regression coverage for that upgrade/remediation path;
• adds coverage for stale controlled encoded Secret cleanup when the source Secret later changes back to a non-colliding name.

Validation run locally:

• CGO_ENABLED=0 go test ./internal/resources/databases -count=1
• KUBEBUILDER_ASSETS="$(pwd)/bin/k8s/1.32.0-darwin-arm64" CGO_ENABLED=0 go test ./...
• nix develop -c just pre-commit

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #487 (comment)

[flemzord]

Follow-up for the latest NumaryBot finding addressed in 0ae381c:

• the delete path now reads the previous Postgres ResourceReference before deleting it, so it can identify the former source Secret when the URI changes away from ?secret=...;
• if that former source Secret is one of the encoded-secret candidate names, the operator removes the stale Database controller ownerReference and preserves the Secret instead of deleting it;
• if the ResourceReference is already missing, the default collision-prone candidate is treated conservatively as a possible user source Secret and remediated instead of deleted;
• controlled operator-created encoded Secrets are still deleted when the previous source Secret is known and non-colliding.

Additional validation run locally:

• targeted delete/reconcile regression tests for source Secret remediation and stale cleanup
• CGO_ENABLED=0 go test ./internal/resources/databases -count=1
• KUBEBUILDER_ASSETS="$(pwd)/bin/k8s/1.32.0-darwin-arm64" CGO_ENABLED=0 go test ./...
• nix develop -c just pre-commit

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #487 (comment)

[flemzord]

Finalizer follow-up addressed in 7ad375c:

• Delete now remediates stale Database controller ownerReferences on the Postgres credentials source Secret before any early return, including the clear-database=false path;
• the remediation uses the existing Postgres ResourceReference when available, falls back to database.status.uri?secret=..., and finally treats the default collision-prone candidate conservatively;
• added regression coverage for the Database finalizer path and the ResourceReference-based remediation path.

Validation run locally after this change:

• targeted finalizer/delete remediation tests
• CGO_ENABLED=0 go test ./internal/resources/databases -count=1
• KUBEBUILDER_ASSETS="$(pwd)/bin/k8s/1.32.0-darwin-arm64" CGO_ENABLED=0 go test ./...
• nix develop -c just pre-commit

[NumaryBot]

NumaryBot posted 2 new inline findings.

Summary: #487 (comment)

[flemzord]

Hashed fallback follow-up addressed in 01e1fd3:

• the collision fallback Secret name now includes a deterministic hash of the source Secret name instead of using only the fixed <database>-encoded-postgres-uri-credentials suffix;
• the previous fixed fallback name remains in the candidate list only for stale controlled Secret cleanup;
• reconciliation now refuses to write to an existing encoded Secret target unless that Secret is already controlled by the same Database, preventing overwrites of unrelated user Secrets;
• added regression coverage for preserving an uncontrolled legacy fallback Secret and for rejecting an uncontrolled existing hashed target.

Validation run locally after this change:

• targeted fallback collision/remediation tests
• CGO_ENABLED=0 go test ./internal/resources/databases -count=1
• KUBEBUILDER_ASSETS="$(pwd)/bin/k8s/1.32.0-darwin-arm64" CGO_ENABLED=0 go test ./...
• nix develop -c just pre-commit

[NumaryBot]

NumaryBot posted 2 new inline findings.

Summary: #487 (comment)

[flemzord]

Hashed fallback stale cleanup addressed in e55f810:

• the stale encoded Secret candidate list now always includes the hashed fallback for the historical colliding source name, even when the current source Secret is non-colliding;
• this cleans up controlled hashed fallback Secrets when a Database moves from <database>-postgres-uri-credentials back to another ?secret=... source;
• extended the stale cleanup regression test to cover both the legacy fixed fallback Secret and the new hashed fallback Secret.

Validation run locally:

• targeted stale hashed fallback cleanup test
• CGO_ENABLED=0 go test ./internal/resources/databases -count=1
• KUBEBUILDER_ASSETS="$(pwd)/bin/k8s/1.32.0-darwin-arm64" CGO_ENABLED=0 go test ./...
• nix develop -c just pre-commit

[NumaryBot]

NumaryBot posted 2 new inline findings.

Summary: #487 (comment)

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/resources/databases/init.go`:
- Around line 64-72: The cleanup order in the postgres credentials path is
wrong: in the branch that uses getPostgresCredentialsSecretReference,
deleteEncodedPostgresCredentialsSecret should run before
resourcereferences.Delete so a failed reconcile can still recover the stored
reference. Update the deletion flow in init.go so the encoded secret is removed
first, then delete the "postgres" ResourceReference afterward; keep the existing
error handling intact so retries remain safe and do not leak the encoded secret.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ab1b9ec0-c29d-4904-9c81-c5ade76df632

📥 Commits

Reviewing files that changed from the base of the PR and between b98f6d4 and e55f810.

⛔ Files ignored due to path filters (1)

• docs/09-Configuration reference/settings.catalog.json is excluded by !**/*.json

📒 Files selected for processing (5)

• internal/resources/databases/env.go
• internal/resources/databases/env_test.go
• internal/resources/databases/init.go
• internal/resources/databases/secret.go
• internal/resources/databases/secret_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/resources/databases/env.go
• internal/resources/databases/secret_test.go

[NumaryBot]

NumaryBot posted 2 new inline findings.

Summary: #487 (comment)

[NumaryBot]

NumaryBot posted 2 new inline findings.

Summary: #487 (comment)

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #487 (comment)

[NumaryBot]

NumaryBot review complete: no remaining inline findings.

Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).

Summary: #487 (comment)

[flemzord]

Final follow-up after CI review comments:

• fixed the retry leak by keeping the postgres ResourceReference until encoded secret cleanup succeeds;
• made finalizer remediation cover both the postgres ResourceReference source secret and the database.status.uri?secret=... source secret when they disagree;
• annotated operator-owned encoded postgres secrets and constrained finalizer fallback cleanup so ambiguous unannotated default-name secrets are not mutated without source evidence;
• reconstituted the collision-safe encoded secret before clear-database drop jobs.

Validation:

• CGO_ENABLED=0 go test ./internal/resources/databases -count=1
• KUBEBUILDER_ASSETS="$(pwd)/bin/k8s/1.32.0-darwin-arm64" CGO_ENABLED=0 go test ./...
• nix develop -c zsh -lc "just pre-commit"
• GitHub Actions Default run 28931949680 is green across tests, Dirty/pre-commit, and E2E Kubernetes 1.31-1.35.
• CodeRabbit: pass; NumaryBot review: pass / no findings; unresolved review threads: 0.