fix: handle finalizers on cluster deletion

Signed-off-by: Marco Nenciarini <[email protected]>

config/crd/bases/postgresql.cnpg.io_publications.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.4
+    controller-gen.kubebuilder.io/version: v0.16.5
   name: publications.postgresql.cnpg.io
 spec:
   group: postgresql.cnpg.io

config/crd/bases/postgresql.cnpg.io_subscriptions.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.4
+    controller-gen.kubebuilder.io/version: v0.16.5
   name: subscriptions.postgresql.cnpg.io
 spec:
   group: postgresql.cnpg.io

internal/controller/cluster_controller.go

@@ -160,10 +160,10 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 				"namespace", req.Namespace,
 			)
 		}
-		if err := r.deleteDatabaseFinalizers(ctx, req.NamespacedName); err != nil {
+		if err := r.deleteFinalizers(ctx, req.NamespacedName); err != nil {
 			contextLogger.Error(
 				err,
-				"error while deleting finalizers of Databases on the cluster",
+				"error while deleting finalizers of objects on the cluster",
 				"clusterName", req.Name,
 				"namespace", req.Namespace,
 			)

internal/controller/finalizers_delete.go

@@ -28,8 +28,26 @@ import (
 	"github.com/cloudnative-pg/cloudnative-pg/pkg/utils"
 )
 
+// deleteFinalizers deletes object finalizers when the cluster they were in has been deleted
+func (r *ClusterReconciler) deleteFinalizers(ctx context.Context, namespacedName types.NamespacedName) error {
+	if err := r.deleteDatabaseFinalizers(ctx, namespacedName); err != nil {
+		return err
+	}
+	if err := r.deletePublicationFinalizers(ctx, namespacedName); err != nil {
+		return err
+	}
+	if err := r.deleteSubscriptionFinalizers(ctx, namespacedName); err != nil {
+		return err
+	}
+	return nil
+}
+
 // deleteDatabaseFinalizers deletes Database object finalizers when the cluster they were in has been deleted
-func (r *ClusterReconciler) deleteDatabaseFinalizers(ctx context.Context, namespacedName types.NamespacedName) error {
+// nolint: dupl
+func (r *ClusterReconciler) deleteDatabaseFinalizers(
+	ctx context.Context,
+	namespacedName types.NamespacedName,
+) error {
 	contextLogger := log.FromContext(ctx)
 
 	databases := apiv1.DatabaseList{}
@@ -66,3 +84,89 @@ func (r *ClusterReconciler) deleteDatabaseFinalizers(ctx context.Context, namesp
 
 	return nil
 }
+
+// deletePublicationFinalizers deletes Publication object finalizers when the cluster they were in has been deleted
+// nolint: dupl
+func (r *ClusterReconciler) deletePublicationFinalizers(
+	ctx context.Context,
+	namespacedName types.NamespacedName,
+) error {
+	contextLogger := log.FromContext(ctx)
+
+	publications := apiv1.PublicationList{}
+	if err := r.List(ctx,
+		&publications,
+		client.InNamespace(namespacedName.Namespace),
+	); err != nil {
+		return err
+	}
+
+	for idx := range publications.Items {
+		publication := &publications.Items[idx]
+
+		if publication.Spec.ClusterRef.Name != namespacedName.Name {
+			continue
+		}
+
+		origPublication := publication.DeepCopy()
+		if controllerutil.RemoveFinalizer(publication, utils.PublicationFinalizerName) {
+			contextLogger.Debug("Removing finalizer from publication",
+				"finalizer", utils.PublicationFinalizerName, "publication", publication.Name)
+			if err := r.Patch(ctx, publication, client.MergeFrom(origPublication)); err != nil {
+				contextLogger.Error(
+					err,
+					"error while removing finalizer from publication",
+					"publication", publication.Name,
+					"oldFinalizerList", origPublication.ObjectMeta.Finalizers,
+					"newFinalizerList", publication.ObjectMeta.Finalizers,
+				)
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
+// deleteSubscriptionFinalizers deletes Subscription object finalizers when the cluster they were in has been deleted
+// nolint: dupl
+func (r *ClusterReconciler) deleteSubscriptionFinalizers(
+	ctx context.Context,
+	namespacedName types.NamespacedName,
+) error {
+	contextLogger := log.FromContext(ctx)
+
+	subscriptions := apiv1.SubscriptionList{}
+	if err := r.List(ctx,
+		&subscriptions,
+		client.InNamespace(namespacedName.Namespace),
+	); err != nil {
+		return err
+	}
+
+	for idx := range subscriptions.Items {
+		subscription := &subscriptions.Items[idx]
+
+		if subscription.Spec.ClusterRef.Name != namespacedName.Name {
+			continue
+		}
+
+		origSubscription := subscription.DeepCopy()
+		if controllerutil.RemoveFinalizer(subscription, utils.SubscriptionFinalizerName) {
+			contextLogger.Debug("Removing finalizer from subscription",
+				"finalizer", utils.SubscriptionFinalizerName, "subscription", subscription.Name)
+			if err := r.Patch(ctx, subscription, client.MergeFrom(origSubscription)); err != nil {
+				contextLogger.Error(
+					err,
+					"error while removing finalizer from subscription",
+					"subscription", subscription.Name,
+					"oldFinalizerList", origSubscription.ObjectMeta.Finalizers,
+					"newFinalizerList", subscription.ObjectMeta.Finalizers,
+				)
+				return err
+			}
+		}
+	}
+
+	return nil
+}

internal/controller/finalizers_delete_test.go

@@ -32,7 +32,8 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-var _ = Describe("Database CRD finalizers", func() {
+// nolint: dupl
+var _ = Describe("CRD finalizers", func() {
 	var (
 		r              ClusterReconciler
 		scheme         *runtime.Scheme
@@ -88,7 +89,7 @@ var _ = Describe("Database CRD finalizers", func() {
 
 		cli := fake.NewClientBuilder().WithScheme(scheme).WithLists(databaseList).Build()
 		r.Client = cli
-		err := r.deleteDatabaseFinalizers(ctx, namespacedName)
+		err := r.deleteFinalizers(ctx, namespacedName)
 		Expect(err).ToNot(HaveOccurred())
 
 		for _, db := range databaseList.Items {
@@ -123,12 +124,174 @@ var _ = Describe("Database CRD finalizers", func() {
 
 			cli := fake.NewClientBuilder().WithScheme(scheme).WithLists(databaseList).Build()
 			r.Client = cli
-			err := r.deleteDatabaseFinalizers(ctx, namespacedName)
+			err := r.deleteFinalizers(ctx, namespacedName)
 			Expect(err).ToNot(HaveOccurred())
 
 			database := &apiv1.Database{}
 			err = cli.Get(ctx, client.ObjectKeyFromObject(&databaseList.Items[0]), database)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(database.Finalizers).To(BeEquivalentTo([]string{utils.DatabaseFinalizerName}))
 		})
+
+	It("should delete publication finalizers for publications on the cluster", func(ctx SpecContext) {
+		publicationList := &apiv1.PublicationList{
+			Items: []apiv1.Publication{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Finalizers: []string{
+							utils.PublicationFinalizerName,
+						},
+						Name:      "pub-1",
+						Namespace: "test",
+					},
+					Spec: apiv1.PublicationSpec{
+						Name: "pub-test",
+						ClusterRef: corev1.LocalObjectReference{
+							Name: "cluster",
+						},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Finalizers: []string{
+							utils.PublicationFinalizerName,
+						},
+						Name:      "pub-2",
+						Namespace: "test",
+					},
+					Spec: apiv1.PublicationSpec{
+						Name: "pub-test-2",
+						ClusterRef: corev1.LocalObjectReference{
+							Name: "cluster",
+						},
+					},
+				},
+			},
+		}
+
+		cli := fake.NewClientBuilder().WithScheme(scheme).WithLists(publicationList).Build()
+		r.Client = cli
+		err := r.deleteFinalizers(ctx, namespacedName)
+		Expect(err).ToNot(HaveOccurred())
+
+		for _, pub := range publicationList.Items {
+			publication := &apiv1.Publication{}
+			err = cli.Get(ctx, client.ObjectKeyFromObject(&pub), publication)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(publication.Finalizers).To(BeZero())
+		}
+	})
+
+	It("should not delete publication finalizers for publications in another cluster", func(ctx SpecContext) {
+		publicationList := &apiv1.PublicationList{
+			Items: []apiv1.Publication{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Finalizers: []string{
+							utils.PublicationFinalizerName,
+						},
+						Name:      "pub-1",
+						Namespace: "test",
+					},
+					Spec: apiv1.PublicationSpec{
+						Name: "pub-test",
+						ClusterRef: corev1.LocalObjectReference{
+							Name: "another-cluster",
+						},
+					},
+				},
+			},
+		}
+
+		cli := fake.NewClientBuilder().WithScheme(scheme).WithLists(publicationList).Build()
+		r.Client = cli
+		err := r.deleteFinalizers(ctx, namespacedName)
+		Expect(err).ToNot(HaveOccurred())
+
+		publication := &apiv1.Publication{}
+		err = cli.Get(ctx, client.ObjectKeyFromObject(&publicationList.Items[0]), publication)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(publication.Finalizers).To(BeEquivalentTo([]string{utils.PublicationFinalizerName}))
+	})
+
+	It("should delete subscription finalizers for subscriptions on the cluster", func(ctx SpecContext) {
+		subscriptionList := &apiv1.SubscriptionList{
+			Items: []apiv1.Subscription{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Finalizers: []string{
+							utils.SubscriptionFinalizerName,
+						},
+						Name:      "sub-1",
+						Namespace: "test",
+					},
+					Spec: apiv1.SubscriptionSpec{
+						Name: "sub-test",
+						ClusterRef: corev1.LocalObjectReference{
+							Name: "cluster",
+						},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Finalizers: []string{
+							utils.SubscriptionFinalizerName,
+						},
+						Name:      "sub-2",
+						Namespace: "test",
+					},
+					Spec: apiv1.SubscriptionSpec{
+						Name: "sub-test-2",
+						ClusterRef: corev1.LocalObjectReference{
+							Name: "cluster",
+						},
+					},
+				},
+			},
+		}
+
+		cli := fake.NewClientBuilder().WithScheme(scheme).WithLists(subscriptionList).Build()
+		r.Client = cli
+		err := r.deleteFinalizers(ctx, namespacedName)
+		Expect(err).ToNot(HaveOccurred())
+
+		for _, sub := range subscriptionList.Items {
+			subscription := &apiv1.Subscription{}
+			err = cli.Get(ctx, client.ObjectKeyFromObject(&sub), subscription)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(subscription.Finalizers).To(BeZero())
+		}
+	})
+
+	It("should not delete subscription finalizers for subscriptions in another cluster", func(ctx SpecContext) {
+		subscriptionList := &apiv1.SubscriptionList{
+			Items: []apiv1.Subscription{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Finalizers: []string{
+							utils.SubscriptionFinalizerName,
+						},
+						Name:      "sub-1",
+						Namespace: "test",
+					},
+					Spec: apiv1.SubscriptionSpec{
+						Name: "sub-test",
+						ClusterRef: corev1.LocalObjectReference{
+							Name: "another-cluster",
+						},
+					},
+				},
+			},
+		}
+
+		cli := fake.NewClientBuilder().WithScheme(scheme).WithLists(subscriptionList).Build()
+		r.Client = cli
+		err := r.deleteFinalizers(ctx, namespacedName)
+		Expect(err).ToNot(HaveOccurred())
+
+		subscription := &apiv1.Subscription{}
+		err = cli.Get(ctx, client.ObjectKeyFromObject(&subscriptionList.Items[0]), subscription)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(subscription.Finalizers).To(BeEquivalentTo([]string{utils.SubscriptionFinalizerName}))
+	})
 })

tests/e2e/publication_subscription_test.go

@@ -20,15 +20,10 @@ import (
 	"fmt"
 	"time"
 
-	apierrs "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/types"
-	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
-
 	apiv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1"
-	"github.com/cloudnative-pg/cloudnative-pg/pkg/utils"
 	"github.com/cloudnative-pg/cloudnative-pg/tests"
 	testUtils "github.com/cloudnative-pg/cloudnative-pg/tests/utils"
+	"k8s.io/apimachinery/pkg/types"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -235,30 +230,6 @@ var _ = Describe("Publication and Subscription", Label(tests.LabelDeclarativePub
 				}
 				AssertDataExpectedCount(env, tableLocator, 2)
 			})
-
-			// TODO: remove once finalizers cleanup is handled by the operator
-			deleteObjectWithFinalizer := func(object client.Object, finalizerName string) error {
-				if err := testUtils.DeleteObject(env, object); err != nil {
-					return err
-				}
-
-				updatedObj := object.DeepCopyObject().(client.Object)
-				controllerutil.RemoveFinalizer(updatedObj, finalizerName)
-				if err := env.Client.Patch(env.Ctx, updatedObj, client.MergeFrom(object)); err != nil {
-					if apierrs.IsNotFound(err) {
-						return nil
-					}
-					return err
-				}
-
-				return nil
-			}
-
-			err = deleteObjectWithFinalizer(pub, utils.PublicationFinalizerName)
-			Expect(err).ToNot(HaveOccurred())
-
-			err = deleteObjectWithFinalizer(sub, utils.SubscriptionFinalizerName)
-			Expect(err).ToNot(HaveOccurred())
 		})
 	})
 })