K8SPG-1010 Fix secrets cleanup#1685
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a deletion-ordering race in the PerconaPGCluster controller where Percona finalizers delete secrets before the upstream Crunchy PostgresCluster is marked for deletion, allowing the Crunchy reconciler to recreate those secrets.
Changes:
- Split Percona finalizers into “pre-PostgresCluster deletion” and “post-PostgresCluster deletion” groups to ensure secret deletion happens after the upstream cluster is gone.
- Update the controller deletion flow to delete the upstream
PostgresClusterfirst and only then run post-deletion finalizers. - Extend the finalizers E2E scenario to include the
delete-backupsfinalizer and assert secrets are not recreated after deletion.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| percona/controller/pgcluster/finalizer.go | Refactors finalizer execution to support pre/post upstream deletion finalizer groups. |
| percona/controller/pgcluster/controller.go | Updates deletion reconcile flow to wait for PostgresCluster deletion before running secret/PVC finalizers. |
| e2e-tests/tests/finalizers/01-create-cluster.yaml | Adds percona.com/delete-backups to the test cluster finalizers. |
| e2e-tests/tests/finalizers/01-assert.yaml | Updates expected finalizers to include delete-backups. |
| e2e-tests/tests/finalizers/02-delete-cluster.yaml | Adds waits and assertions to detect recreated secrets after cluster deletion. |
| e2e-tests/tests/finalizers/03-create-cluster.yaml | Adds percona.com/delete-backups to the second test cluster finalizers. |
| e2e-tests/tests/finalizers/03-assert.yaml | Updates expected finalizers to include delete-backups. |
| type finalizerEntry struct { | ||
| name string | ||
| fn controller.FinalizerFunc[*v2.PerconaPGCluster] | ||
| func (r *PGClusterReconciler) runFinalizers(ctx context.Context, cr *v2.PerconaPGCluster, finalizers []finalizerEntry) error { |
There was a problem hiding this comment.
after k8sClient.Delete(cr) + one Reconcile pass, but before the PostgresCluster is gone, maybe we can assert on a test that on the fetched CR that FinalizerStopWatchers and FinalizerDeleteBackups are already removed while FinalizerDeletePVC / FinalizerDeleteSSL are still present
There was a problem hiding this comment.
good point, thanks for the suggestion, added the "pre-deletion finalizers removed before PostgresCluster is gone" unit test
| fn controller.FinalizerFunc[*v2.PerconaPGCluster] | ||
| } | ||
|
|
||
| func (r *PGClusterReconciler) prePostgresClusterDeletionFinalizers() []finalizerEntry { |
There was a problem hiding this comment.
please add a comment describing what needs to go into pre finalizers list, i think it'll be confusing in the future
There was a problem hiding this comment.
good point, done, thanks
add1675
Fix secrets cleanup
Problem:
K8SPG-1010
When a PerconaPGCluster with delete-pvc, delete-ssl, and delete-backups finalizers is deleted, the secrets removed by those finalizers get recreated and left behind after the cluster is fully gone.
Cause:
The root cause is an ordering issue in the deletion flow. The Percona reconciler runs its finalizers (which delete user and TLS secrets) before it issues a delete on the underlying PostgresCluster
object. At that point, the crunchy PostgresCluster is still fully alive and its reconciler is operational. Since the crunchy reconciler watches owned Secrets, each secret deletion triggers a reconcile
event. Because no DeletionTimestamp is set on the PostgresCluster yet, the crunchy reconciler treats this as normal operation and recreates the missing secrets.
The delete-backups finalizer makes this consistently reproducible because it triggers finishBackup, which performs multiple writes to the crunchy PostgresCluster (clearing ManualBackup status, modifying Jobs), repeatedly re-enqueuing the crunchy reconciler and widening the race window.
Solution:
Split finalizers into to two parts (Pre- and Post- deletion) so that the secret deletion finalizers would be processed after the upstream cluster if deleted.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability