Handle delete before adding finalizer

In Reconcile() methods, move the object deletion above add finalizer.
Finalizers can't be set when an object is being deleted.

Signed-off-by: Sunny <darkowlzz@protonmail.com>

internal/controller/alert_controller.go

@@ -133,18 +133,22 @@ func (r *AlertReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resu
 		}
 	}()
 
-	if !controllerutil.ContainsFinalizer(obj, apiv1.NotificationFinalizer) {
-		controllerutil.AddFinalizer(obj, apiv1.NotificationFinalizer)
-		result = ctrl.Result{Requeue: true}
-		return
-	}
-
 	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
 		controllerutil.RemoveFinalizer(obj, apiv1.NotificationFinalizer)
 		result = ctrl.Result{}
 		return
 	}
 
+	// Add finalizer first if not exist to avoid the race condition between init
+	// and delete.
+	// Note: Finalizers in general can only be added when the deletionTimestamp
+	// is not set.
+	if !controllerutil.ContainsFinalizer(obj, apiv1.NotificationFinalizer) {
+		controllerutil.AddFinalizer(obj, apiv1.NotificationFinalizer)
+		result = ctrl.Result{Requeue: true}
+		return
+	}
+
 	// Return early if the object is suspended.
 	if obj.Spec.Suspend {
 		log.Info("Reconciliation is suspended for this object")

internal/controller/alert_controller_test.go

@@ -35,6 +35,8 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/tools/record"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -48,6 +50,39 @@ import (
 	"github.com/fluxcd/notification-controller/internal/server"
 )
 
+func TestAlertReconciler_deleteBeforeFinalizer(t *testing.T) {
+	g := NewWithT(t)
+
+	namespaceName := "alert-" + randStringRunes(5)
+	namespace := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{Name: namespaceName},
+	}
+	g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred())
+	t.Cleanup(func() {
+		g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred())
+	})
+
+	alert := &apiv1beta2.Alert{}
+	alert.Name = "test-alert"
+	alert.Namespace = namespaceName
+	alert.Spec.EventSources = []apiv1.CrossNamespaceObjectReference{
+		{Kind: "Bucket", Name: "Foo"},
+	}
+	// Add a test finalizer to prevent the object from getting deleted.
+	alert.SetFinalizers([]string{"test-finalizer"})
+	g.Expect(k8sClient.Create(ctx, alert)).NotTo(HaveOccurred())
+	// Add deletion timestamp by deleting the object.
+	g.Expect(k8sClient.Delete(ctx, alert)).NotTo(HaveOccurred())
+
+	r := &AlertReconciler{
+		Client:        k8sClient,
+		EventRecorder: record.NewFakeRecorder(32),
+	}
+	// NOTE: Only a real API server responds with an error in this scenario.
+	_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(alert)})
+	g.Expect(err).NotTo(HaveOccurred())
+}
+
 func TestAlertReconciler_Reconcile(t *testing.T) {
 	g := NewWithT(t)
 	timeout := 5 * time.Second

internal/controller/provider_controller.go

@@ -126,18 +126,22 @@ func (r *ProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (r
 		}
 	}()
 
-	if !controllerutil.ContainsFinalizer(obj, apiv1.NotificationFinalizer) {
-		controllerutil.AddFinalizer(obj, apiv1.NotificationFinalizer)
-		result = ctrl.Result{Requeue: true}
-		return
-	}
-
 	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
 		controllerutil.RemoveFinalizer(obj, apiv1.NotificationFinalizer)
 		result = ctrl.Result{}
 		return
 	}
 
+	// Add finalizer first if not exist to avoid the race condition
+	// between init and delete.
+	// Note: Finalizers in general can only be added when the deletionTimestamp
+	// is not set.
+	if !controllerutil.ContainsFinalizer(obj, apiv1.NotificationFinalizer) {
+		controllerutil.AddFinalizer(obj, apiv1.NotificationFinalizer)
+		result = ctrl.Result{Requeue: true}
+		return
+	}
+
 	// Return early if the object is suspended.
 	if obj.Spec.Suspend {
 		log.Info("Reconciliation is suspended for this object")

internal/controller/provider_controller_test.go

@@ -27,6 +27,8 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/tools/record"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
@@ -38,6 +40,37 @@ import (
 	apiv1beta2 "github.com/fluxcd/notification-controller/api/v1beta2"
 )
 
+func TestProviderReconciler_deleteBeforeFinalizer(t *testing.T) {
+	g := NewWithT(t)
+
+	namespaceName := "provider-" + randStringRunes(5)
+	namespace := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{Name: namespaceName},
+	}
+	g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred())
+	t.Cleanup(func() {
+		g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred())
+	})
+
+	provider := &apiv1beta2.Provider{}
+	provider.Name = "test-provider"
+	provider.Namespace = namespaceName
+	provider.Spec.Type = "slack"
+	// Add a test finalizer to prevent the object from getting deleted.
+	provider.SetFinalizers([]string{"test-finalizer"})
+	g.Expect(k8sClient.Create(ctx, provider)).NotTo(HaveOccurred())
+	// Add deletion timestamp by deleting the object.
+	g.Expect(k8sClient.Delete(ctx, provider)).NotTo(HaveOccurred())
+
+	r := &ProviderReconciler{
+		Client:        k8sClient,
+		EventRecorder: record.NewFakeRecorder(32),
+	}
+	// NOTE: Only a real API server responds with an error in this scenario.
+	_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)})
+	g.Expect(err).NotTo(HaveOccurred())
+}
+
 func TestProviderReconciler_Reconcile(t *testing.T) {
 	g := NewWithT(t)
 	timeout := 5 * time.Second

internal/controller/receiver_controller.go

@@ -129,18 +129,22 @@ func (r *ReceiverReconciler) Reconcile(ctx context.Context, req ctrl.Request) (r
 		}
 	}()
 
-	if !controllerutil.ContainsFinalizer(obj, apiv1.NotificationFinalizer) {
-		controllerutil.AddFinalizer(obj, apiv1.NotificationFinalizer)
-		result = ctrl.Result{Requeue: true}
-		return
-	}
-
 	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
 		controllerutil.RemoveFinalizer(obj, apiv1.NotificationFinalizer)
 		result = ctrl.Result{}
 		return
 	}
 
+	// Add finalizer first if not exist to avoid the race condition
+	// between init and delete.
+	// Note: Finalizers in general can only be added when the deletionTimestamp
+	// is not set.
+	if !controllerutil.ContainsFinalizer(obj, apiv1.NotificationFinalizer) {
+		controllerutil.AddFinalizer(obj, apiv1.NotificationFinalizer)
+		result = ctrl.Result{Requeue: true}
+		return
+	}
+
 	// Return early if the object is suspended.
 	if obj.Spec.Suspend {
 		log.Info("Reconciliation is suspended for this object")

internal/controller/receiver_controller_test.go

@@ -31,6 +31,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/tools/record"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -43,6 +45,43 @@ import (
 	"github.com/fluxcd/notification-controller/internal/server"
 )
 
+func TestReceiverReconciler_deleteBeforeFinalizer(t *testing.T) {
+	g := NewWithT(t)
+
+	namespaceName := "receiver-" + randStringRunes(5)
+	namespace := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{Name: namespaceName},
+	}
+	g.Expect(k8sClient.Create(ctx, namespace)).ToNot(HaveOccurred())
+	t.Cleanup(func() {
+		g.Expect(k8sClient.Delete(ctx, namespace)).NotTo(HaveOccurred())
+	})
+
+	receiver := &apiv1.Receiver{}
+	receiver.Name = "test-receiver"
+	receiver.Namespace = namespaceName
+	receiver.Spec = apiv1.ReceiverSpec{
+		Type: "github",
+		Resources: []apiv1.CrossNamespaceObjectReference{
+			{Kind: "Bucket", Name: "Foo"},
+		},
+		SecretRef: meta.LocalObjectReference{Name: "foo-secret"},
+	}
+	// Add a test finalizer to prevent the object from getting deleted.
+	receiver.SetFinalizers([]string{"test-finalizer"})
+	g.Expect(k8sClient.Create(ctx, receiver)).NotTo(HaveOccurred())
+	// Add deletion timestamp by deleting the object.
+	g.Expect(k8sClient.Delete(ctx, receiver)).NotTo(HaveOccurred())
+
+	r := &ReceiverReconciler{
+		Client:        k8sClient,
+		EventRecorder: record.NewFakeRecorder(32),
+	}
+	// NOTE: Only a real API server responds with an error in this scenario.
+	_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(receiver)})
+	g.Expect(err).NotTo(HaveOccurred())
+}
+
 func TestReceiverReconciler_Reconcile(t *testing.T) {
 	g := NewWithT(t)