Merge pull request #1112 from fluxcd/fix-provider-state-machine

Reintroduce default state machine for Provider controller

Author: matheuscscp

internal/controller/finalizer_predicate.go

@@ -28,7 +28,6 @@ import (
 // that have the flux finalizer.
 type finalizerPredicate struct {
 	predicate.Funcs
-	observeDeletion bool
 }
 
 // Create allows events for objects with flux finalizer that have beed created.
@@ -46,6 +45,6 @@ func (finalizerPredicate) Update(e event.UpdateEvent) bool {
 
 // Delete allows events for objects with flux finalizer that have been marked
 // for deletion.
-func (f finalizerPredicate) Delete(e event.DeleteEvent) bool {
-	return f.observeDeletion || controllerutil.ContainsFinalizer(e.Object, apiv1.NotificationFinalizer)
+func (finalizerPredicate) Delete(e event.DeleteEvent) bool {
+	return controllerutil.ContainsFinalizer(e.Object, apiv1.NotificationFinalizer)
 }

internal/controller/provider_controller.go

@@ -19,7 +19,6 @@ package controller
 import (
 	"context"
 
-	corev1 "k8s.io/api/core/v1"
 	kuberecorder "k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
@@ -45,42 +44,21 @@ type ProviderReconciler struct {
 	client.Client
 	kuberecorder.EventRecorder
 
-	ControllerName string
-	TokenCache     *cache.TokenCache
+	TokenCache *cache.TokenCache
 }
 
 func (r *ProviderReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
-		For(&apiv1beta3.Provider{}, builder.WithPredicates(finalizerPredicate{observeDeletion: true})).
+		For(&apiv1beta3.Provider{}, builder.WithPredicates(providerPredicate{})).
 		Complete(r)
 }
 
 func (r *ProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, retErr error) {
-	log := ctrl.LoggerFrom(ctx)
-
 	obj := &apiv1beta3.Provider{}
 	if err := r.Get(ctx, req.NamespacedName, obj); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	// Examine if the object is under deletion.
-	var delete bool
-	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-		delete = true
-		r.TokenCache.DeleteEventsForObject(apiv1beta3.ProviderKind, obj.GetName(), obj.GetNamespace(), notifier.OperationPost)
-	}
-
-	// Early return if no migration is needed.
-	if !controllerutil.ContainsFinalizer(obj, apiv1.NotificationFinalizer) {
-		return ctrl.Result{}, nil
-	}
-
-	// Skip if it's suspend and not being deleted.
-	if obj.Spec.Suspend && !delete {
-		log.Info("reconciliation is suspended for this object")
-		return ctrl.Result{}, nil
-	}
-
 	patcher, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
 		return ctrl.Result{}, err
@@ -92,11 +70,29 @@ func (r *ProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (r
 		}
 	}()
 
-	// Remove the notification-controller finalizer.
-	controllerutil.RemoveFinalizer(obj, apiv1.NotificationFinalizer)
+	// Examine if the object is under deletion.
+	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
+		return r.reconcileDelete(obj)
+	}
 
-	log.Info("removed finalizer from Provider to migrate to static Provider")
-	r.Event(obj, corev1.EventTypeNormal, "Migration", "removed finalizer from Provider to migrate to static Provider")
+	// Add finalizer if it doesn't exist.
+	if !controllerutil.ContainsFinalizer(obj, apiv1.NotificationFinalizer) {
+		controllerutil.AddFinalizer(obj, apiv1.NotificationFinalizer)
+	}
 
 	return
 }
+
+// reconcileDelete handles the deletion of the object.
+// It cleans up the caches and removes the finalizer.
+func (r *ProviderReconciler) reconcileDelete(obj *apiv1beta3.Provider) (ctrl.Result, error) {
+	// Remove our finalizer from the list
+	controllerutil.RemoveFinalizer(obj, apiv1.NotificationFinalizer)
+
+	// Cleanup caches.
+	r.TokenCache.DeleteEventsForObject(apiv1beta3.ProviderKind,
+		obj.GetName(), obj.GetNamespace(), notifier.OperationPost)
+
+	// Stop reconciliation as the object is being deleted
+	return ctrl.Result{}, nil
+}

internal/controller/provider_controller_test.go

@@ -52,61 +52,29 @@ func TestProviderReconciler(t *testing.T) {
 	}
 	providerKey := client.ObjectKeyFromObject(provider)
 
-	// Remove finalizer at create.
-
-	provider.ObjectMeta.Finalizers = append(provider.ObjectMeta.Finalizers, "foo.bar", apiv1.NotificationFinalizer)
+	// Create without finalizer.
 	provider.Spec = apiv1beta3.ProviderSpec{
-		Type: "slack",
+		Type: "generic",
 	}
 	g.Expect(testEnv.Create(ctx, provider)).ToNot(HaveOccurred())
 
+	// Should eventually have finalizer.
 	g.Eventually(func() bool {
 		_ = testEnv.Get(ctx, providerKey, provider)
-		return !controllerutil.ContainsFinalizer(provider, apiv1.NotificationFinalizer)
+		return controllerutil.ContainsFinalizer(provider, apiv1.NotificationFinalizer)
 	}, timeout, time.Second).Should(BeTrue())
 
-	// Remove finalizer at update.
-
+	// Remove finalizer.
 	patchHelper, err := patch.NewHelper(provider, testEnv.Client)
 	g.Expect(err).ToNot(HaveOccurred())
-	provider.ObjectMeta.Finalizers = append(provider.ObjectMeta.Finalizers, apiv1.NotificationFinalizer)
+	controllerutil.RemoveFinalizer(provider, apiv1.NotificationFinalizer)
 	g.Expect(patchHelper.Patch(ctx, provider)).ToNot(HaveOccurred())
 
+	// Should eventually have finalizer again.
 	g.Eventually(func() bool {
 		_ = testEnv.Get(ctx, providerKey, provider)
-		return !controllerutil.ContainsFinalizer(provider, apiv1.NotificationFinalizer)
-	}, timeout, time.Second).Should(BeTrue())
-
-	// Remove finalizer at delete.
-
-	patchHelper, err = patch.NewHelper(provider, testEnv.Client)
-	g.Expect(err).ToNot(HaveOccurred())
-
-	// Suspend the provider to prevent finalizer from getting removed.
-	// Ensure only flux finalizer is set to allow the object to be garbage
-	// collected at the end.
-	// NOTE: Suspending and updating finalizers are done separately here as
-	// doing them in a single patch results in flaky test where the finalizer
-	// update doesn't gets registered with the kube-apiserver, resulting in
-	// timeout waiting for finalizer to appear on the object below.
-	provider.Spec.Suspend = true
-	g.Expect(patchHelper.Patch(ctx, provider)).ToNot(HaveOccurred())
-	g.Eventually(func() bool {
-		_ = k8sClient.Get(ctx, providerKey, provider)
-		return provider.Spec.Suspend == true
-	}, timeout).Should(BeTrue())
-
-	patchHelper, err = patch.NewHelper(provider, testEnv.Client)
-	g.Expect(err).ToNot(HaveOccurred())
-
-	// Add finalizer and verify that finalizer exists on the object using a live
-	// client.
-	provider.ObjectMeta.Finalizers = []string{apiv1.NotificationFinalizer}
-	g.Expect(patchHelper.Patch(ctx, provider)).ToNot(HaveOccurred())
-	g.Eventually(func() bool {
-		_ = k8sClient.Get(ctx, providerKey, provider)
 		return controllerutil.ContainsFinalizer(provider, apiv1.NotificationFinalizer)
-	}, timeout).Should(BeTrue())
+	}, timeout, time.Second).Should(BeTrue())
 
 	// Delete the object and verify.
 	g.Expect(testEnv.Delete(ctx, provider)).ToNot(HaveOccurred())

internal/controller/provider_predicate.go

@@ -0,0 +1,48 @@
+/*
+Copyright 2025 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controller
+
+import (
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+
+	apiv1 "github.com/fluxcd/notification-controller/api/v1"
+	apiv1beta3 "github.com/fluxcd/notification-controller/api/v1beta3"
+)
+
+// providerPredicate implements predicate functions for the Provider API.
+type providerPredicate struct{}
+
+func (providerPredicate) Create(e event.CreateEvent) bool {
+	return !controllerutil.ContainsFinalizer(e.Object, apiv1.NotificationFinalizer)
+}
+
+func (providerPredicate) Update(e event.UpdateEvent) bool {
+	if e.ObjectNew == nil {
+		return false
+	}
+	return !controllerutil.ContainsFinalizer(e.ObjectNew, apiv1.NotificationFinalizer) ||
+		!e.ObjectNew.(*apiv1beta3.Provider).ObjectMeta.DeletionTimestamp.IsZero()
+}
+
+func (providerPredicate) Delete(e event.DeleteEvent) bool {
+	return false
+}
+
+func (providerPredicate) Generic(e event.GenericEvent) bool {
+	return !controllerutil.ContainsFinalizer(e.Object, apiv1.NotificationFinalizer)
+}

internal/controller/suite_test.go

@@ -82,9 +82,8 @@ func TestMain(m *testing.M) {
 	}
 
 	if err := (&ProviderReconciler{
-		Client:         testEnv,
-		ControllerName: controllerName,
-		EventRecorder:  testEnv.GetEventRecorderFor(controllerName),
+		Client:        testEnv,
+		EventRecorder: testEnv.GetEventRecorderFor(controllerName),
 	}).SetupWithManager(testEnv); err != nil {
 		panic(fmt.Sprintf("Failed to start ProviderReconciler: %v", err))
 	}

main.go

@@ -196,10 +196,9 @@ func main() {
 	}
 
 	if err = (&controller.ProviderReconciler{
-		Client:         mgr.GetClient(),
-		ControllerName: controllerName,
-		EventRecorder:  mgr.GetEventRecorderFor(controllerName),
-		TokenCache:     tokenCache,
+		Client:        mgr.GetClient(),
+		EventRecorder: mgr.GetEventRecorderFor(controllerName),
+		TokenCache:    tokenCache,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "Provider")
 		os.Exit(1)