diff --git a/internal/controller/alert_controller.go b/internal/controller/alert_controller.go index dbc792947..d22e52e6d 100644 --- a/internal/controller/alert_controller.go +++ b/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") diff --git a/internal/controller/alert_controller_test.go b/internal/controller/alert_controller_test.go index 4c8332c64..ea97151d4 100644 --- a/internal/controller/alert_controller_test.go +++ b/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 diff --git a/internal/controller/provider_controller.go b/internal/controller/provider_controller.go index 8c86a2534..8b07b052d 100644 --- a/internal/controller/provider_controller.go +++ b/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") diff --git a/internal/controller/provider_controller_test.go b/internal/controller/provider_controller_test.go index 43ff11921..585e52d07 100644 --- a/internal/controller/provider_controller_test.go +++ b/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 diff --git a/internal/controller/receiver_controller.go b/internal/controller/receiver_controller.go index 9153a8cde..990cfd068 100644 --- a/internal/controller/receiver_controller.go +++ b/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") diff --git a/internal/controller/receiver_controller_test.go b/internal/controller/receiver_controller_test.go index 3de694813..3b4da9cbd 100644 --- a/internal/controller/receiver_controller_test.go +++ b/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)