K8SPG-1010 Fix secrets cleanup

e2e-tests/tests/finalizers/01-assert.yaml

@@ -124,6 +124,7 @@ metadata:
   finalizers:
   - percona.com/delete-pvc
   - percona.com/delete-ssl
+  - percona.com/delete-backups
   - internal.percona.com/stop-watchers
 status:
   pgbouncer:

e2e-tests/tests/finalizers/01-create-cluster.yaml

@@ -9,5 +9,5 @@ commands:
       source ../../functions
 
       get_cr \
-        | yq eval '.metadata.finalizers += ["percona.com/delete-pvc", "percona.com/delete-ssl"]' \
+        | yq eval '.metadata.finalizers += ["percona.com/delete-pvc", "percona.com/delete-ssl", "percona.com/delete-backups"]' \
         | kubectl -n "${NAMESPACE}" apply -f -

e2e-tests/tests/finalizers/02-delete-cluster.yaml

@@ -4,3 +4,28 @@ delete:
 - apiVersion: pgv2.percona.com/v2
   kind: PerconaPGCluster
   name: finalizers
+commands:
+  - timeout: 180
+    script: |-
+      set -o errexit
+      set -o xtrace
+
+      source ../../functions
+
+      wait_for_delete "postgrescluster.upstream.pgv2.percona.com/finalizers"
+      wait_for_delete "pg/finalizers"
+
+      assert_no_cluster_secrets() {
+        local count
+        count=$(kubectl -n "${NAMESPACE}" get secret -l postgres-operator.crunchydata.com/cluster=finalizers --no-headers 2>/dev/null | wc -l | tr -d ' ')
+        if [[ "${count}" != "0" ]]; then
+          echo "Found recreated secrets for deleted cluster finalizers"
+          kubectl -n "${NAMESPACE}" get secret -l postgres-operator.crunchydata.com/cluster=finalizers
+          exit 1
+        fi
+      }
+
+      # Check twice with a short delay to catch late secret recreation.
+      assert_no_cluster_secrets
+      sleep 15
+      assert_no_cluster_secrets

e2e-tests/tests/finalizers/03-assert.yaml

@@ -124,6 +124,7 @@ metadata:
   finalizers:
   - percona.com/delete-pvc
   - percona.com/delete-ssl
+  - percona.com/delete-backups
   - internal.percona.com/stop-watchers
 status:
   pgbouncer:

e2e-tests/tests/finalizers/03-create-cluster.yaml

@@ -9,5 +9,5 @@ commands:
       source ../../functions
 
       get_cr "finalizers-2" \
-        | yq eval '.metadata.finalizers += ["percona.com/delete-pvc", "percona.com/delete-ssl"]' \
+        | yq eval '.metadata.finalizers += ["percona.com/delete-pvc", "percona.com/delete-ssl", "percona.com/delete-backups"]' \
         | kubectl -n "${NAMESPACE}" apply -f -

e2e-tests/tests/upgrade-minor/06-upgrade-cluster.yaml

@@ -31,7 +31,8 @@ commands:
 
       kubectl wait -n $NAMESPACE --timeout 180s --for=jsonpath='{.spec.template.spec.containers[0].image}'=$target_image_backrest sts/upgrade-minor-repo-host
       kubectl wait -n $NAMESPACE --timeout 180s --for=jsonpath='{.spec.template.spec.containers[0].image}'=$target_image_pgbouncer deployment/upgrade-minor-pgbouncer
-      for s in $(kubectl get sts --no-headers -l postgres-operator.crunchydata.com/instance-set=instance1 --output=custom-columns='NAME:.metadata.name'); do
+      for s in $(kubectl get sts -n $NAMESPACE --no-headers -l postgres-operator.crunchydata.com/instance-set=instance1 --output=custom-columns='NAME:.metadata.name'); do
           kubectl wait -n $NAMESPACE --timeout 180s --for=jsonpath='{.spec.template.spec.containers[0].image}'=$target_image_postgresql sts/${s}
       done
     timeout: 180
+

percona/controller/pgcluster/controller.go

@@ -284,8 +284,10 @@ func (r *PGClusterReconciler) Reconcile(ctx context.Context, request reconcile.R
 	if cr.DeletionTimestamp != nil {
 		log.Info("Deleting PerconaPGCluster", "deletionTimestamp", cr.DeletionTimestamp)
 
-		if err := r.runFinalizers(ctx, cr); err != nil {
-			return reconcile.Result{RequeueAfter: 5 * time.Second}, errors.Wrap(err, "run finalizers")
+		// Run pre-deletion finalizers before deleting PostgresCluster,
+		// because they may need to exec into pods (e.g. deleteBackups).
+		if err := r.runPrePostgresClusterDeletionFinalizers(ctx, cr); err != nil {
+			return reconcile.Result{RequeueAfter: 5 * time.Second}, errors.Wrap(err, "run pre-postgrescluster finalizers")
 		}
 
 		// We're deleting PostgresCluster explicitly to let Crunchy controller run its finalizers and not mess with us.
@@ -294,8 +296,15 @@ func (r *PGClusterReconciler) Reconcile(ctx context.Context, request reconcile.R
 		}
 
 		if err := r.Client.Get(ctx, client.ObjectKeyFromObject(postgresCluster), postgresCluster); err == nil {
+
 			log.Info("Waiting for PostgresCluster to be deleted")
 			return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+		} else if client.IgnoreNotFound(err) != nil {
+			return ctrl.Result{RequeueAfter: 5 * time.Second}, errors.Wrap(err, "get postgres cluster")
+		}
+
+		if err := r.runPostPostgresClusterDeletionFinalizers(ctx, cr); err != nil {
+			return reconcile.Result{RequeueAfter: 5 * time.Second}, errors.Wrap(err, "run post-postgrescluster finalizers")
 		}
 
 		return reconcile.Result{}, nil

percona/controller/pgcluster/finalizer.go

@@ -20,8 +20,6 @@ import (
 	v2 "github.com/percona/percona-postgresql-operator/v2/pkg/apis/pgv2.percona.com/v2"
 )
 
-type finalizerFunc func(context.Context, *v2.PerconaPGCluster) error
-
 func (r *PGClusterReconciler) deletePVC(ctx context.Context, cr *v2.PerconaPGCluster) error {
 	log := logging.FromContext(ctx)
 
@@ -212,24 +210,42 @@ func (r *PGClusterReconciler) deleteBackups(ctx context.Context, cr *v2.PerconaP
 	return nil
 }
 
-func (r *PGClusterReconciler) runFinalizers(ctx context.Context, cr *v2.PerconaPGCluster) error {
-	type finalizerEntry struct {
-		name string
-		fn   controller.FinalizerFunc[*v2.PerconaPGCluster]
+func (r *PGClusterReconciler) runFinalizers(ctx context.Context, cr *v2.PerconaPGCluster, finalizers []finalizerEntry) error {
+	for _, entry := range finalizers {
+		if _, err := controller.RunFinalizer(ctx, r.Client, cr, entry.name, entry.fn); err != nil {
+			return errors.Wrapf(err, "run finalizer %s", entry.name)
+		}
 	}
 
-	finalizers := []finalizerEntry{
-		{pNaming.FinalizerDeletePVC, r.deletePVCAndSecrets},
-		{pNaming.FinalizerDeleteSSL, r.deleteTLSSecrets},
+	return nil
+}
+
+type finalizerEntry struct {
+	name string
+	fn   controller.FinalizerFunc[*v2.PerconaPGCluster]
+}
+
+// prePostgresClusterDeletionFinalizers returns finalizers that must run while the
+// PostgresCluster still exists (e.g. operations that require running pods or
+// cluster resources such as stopping watchers and deleting backups from repos).
+func (r *PGClusterReconciler) prePostgresClusterDeletionFinalizers() []finalizerEntry {
+	return []finalizerEntry{
 		{pNaming.FinalizerStopWatchers, r.stopExternalWatchers},
 		{pNaming.FinalizerDeleteBackups, r.deleteBackups},
 	}
+}
 
-	for _, entry := range finalizers {
-		if _, err := controller.RunFinalizer(ctx, r.Client, cr, entry.name, entry.fn); err != nil {
-			return errors.Wrapf(err, "run finalizer %s", entry.name)
-		}
+func (r *PGClusterReconciler) postPostgresClusterDeletionFinalizers() []finalizerEntry {
+	return []finalizerEntry{
+		{pNaming.FinalizerDeletePVC, r.deletePVCAndSecrets},
+		{pNaming.FinalizerDeleteSSL, r.deleteTLSSecrets},
 	}
+}
 
-	return nil
+func (r *PGClusterReconciler) runPrePostgresClusterDeletionFinalizers(ctx context.Context, cr *v2.PerconaPGCluster) error {
+	return r.runFinalizers(ctx, cr, r.prePostgresClusterDeletionFinalizers())
+}
+
+func (r *PGClusterReconciler) runPostPostgresClusterDeletionFinalizers(ctx context.Context, cr *v2.PerconaPGCluster) error {
+	return r.runFinalizers(ctx, cr, r.postPostgresClusterDeletionFinalizers())
 }

percona/controller/pgcluster/finalizer_test.go

@@ -5,6 +5,7 @@ package pgcluster
 import (
 	"context"
 	"fmt"
+	"io"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -353,6 +354,81 @@ var _ = Describe("Finalizers", Ordered, func() {
 		})
 	})
 
+	Context("pre-deletion finalizers removed before PostgresCluster is gone", Ordered, func() {
+		crName := ns + "-pre-finalizer-order"
+		crNamespacedName := types.NamespacedName{Name: crName, Namespace: ns}
+
+		When("CR has all four finalizers", func() {
+			cr, err := readDefaultCR(crName, ns)
+			It("should read default cr.yaml", func() {
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			controllerutil.AddFinalizer(cr, pNaming.FinalizerStopWatchers)
+			controllerutil.AddFinalizer(cr, pNaming.FinalizerDeleteBackups)
+			controllerutil.AddFinalizer(cr, pNaming.FinalizerDeletePVC)
+			controllerutil.AddFinalizer(cr, pNaming.FinalizerDeleteSSL)
+
+			It("should create PerconaPGCluster and a labeled pod for deleteBackups", func() {
+				status := cr.Status
+				Expect(k8sClient.Create(ctx, cr)).Should(Succeed())
+				cr.Status = status
+				Expect(k8sClient.Status().Update(ctx, cr)).Should(Succeed())
+
+				pod := &corev1.Pod{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      crName + "-pod",
+						Namespace: ns,
+						Labels: map[string]string{
+							"app.kubernetes.io/instance":  crName,
+							"app.kubernetes.io/component": "pg",
+						},
+					},
+					Spec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{Name: "database", Image: "busybox"},
+						},
+					},
+				}
+				Expect(k8sClient.Create(ctx, pod)).Should(Succeed())
+			})
+
+			It("should reconcile to create PostgresCluster", func() {
+				rec := reconciler(cr)
+				rec.PodExec = func(_ context.Context, _, _, _ string, _ io.Reader, _ io.Writer, _ io.Writer, _ ...string) error {
+					return nil
+				}
+				_, err = rec.Reconcile(ctx, ctrl.Request{NamespacedName: crNamespacedName})
+				Expect(err).NotTo(HaveOccurred())
+				_, err = crunchyReconciler().Reconcile(ctx, ctrl.Request{NamespacedName: crNamespacedName})
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("should delete PerconaPGCluster", func() {
+				Expect(k8sClient.Delete(ctx, cr)).Should(Succeed())
+			})
+
+			It("should run one Reconcile pass (pre-finalizers run, PostgresCluster still exists)", func() {
+				rec := reconciler(cr)
+				rec.PodExec = func(_ context.Context, _, _, _ string, _ io.Reader, _ io.Writer, _ io.Writer, _ ...string) error {
+					return nil
+				}
+				_, err = rec.Reconcile(ctx, ctrl.Request{NamespacedName: crNamespacedName})
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("should have pre-deletion finalizers removed and post-deletion finalizers still present", func() {
+				fetched := &v2.PerconaPGCluster{}
+				Expect(k8sClient.Get(ctx, crNamespacedName, fetched)).Should(Succeed())
+
+				Expect(fetched.Finalizers).ShouldNot(ContainElement(pNaming.FinalizerStopWatchers))
+				Expect(fetched.Finalizers).ShouldNot(ContainElement(pNaming.FinalizerDeleteBackups))
+				Expect(fetched.Finalizers).Should(ContainElement(pNaming.FinalizerDeletePVC))
+				Expect(fetched.Finalizers).Should(ContainElement(pNaming.FinalizerDeleteSSL))
+			})
+		})
+	})
+
 	Context(pNaming.FinalizerStopWatchers, Ordered, func() {
 		When("without finalizer", func() {
 			crName := ns + "-without-stop-watchers"