Handle delete before adding finalizer

In Reconcile() method, move the object deletion above add finalizer. Finalizers can't be set when an object is being deleted. Introduce a cacheless client in suite_test to use for testing this change. It ensures that the Reconcile() call always operates on the latest version of the object which has the deletion timestamp and existing finalizer. Signed-off-by: Sunny <[email protected]>
fluxcd · Jul 26, 2023 · 347afa3 · 347afa3
1 parent 386d678
commit 347afa3
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 11 deletions.
diff --git a/internal/controller/imageupdateautomation_controller.go b/internal/controller/imageupdateautomation_controller.go
@@ -110,7 +110,18 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
+	// If the object is under deletion, record the readiness, and remove our finalizer.
+	if !auto.ObjectMeta.DeletionTimestamp.IsZero() {
+		controllerutil.RemoveFinalizer(&auto, imagev1.ImageUpdateAutomationFinalizer)
+		if err := r.Update(ctx, &auto); err != nil {
+			return ctrl.Result{}, err
+		}
+		return ctrl.Result{}, nil
+	}
+
 	// Add our finalizer if it does not exist.
+	// Note: Finalizers in general can only be added when the deletionTimestamp
+	// is not set.
 	if !controllerutil.ContainsFinalizer(&auto, imagev1.ImageUpdateAutomationFinalizer) {
 		patch := client.MergeFrom(auto.DeepCopy())
 		controllerutil.AddFinalizer(&auto, imagev1.ImageUpdateAutomationFinalizer)
@@ -120,15 +131,6 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 		}
 	}
 
-	// If the object is under deletion, record the readiness, and remove our finalizer.
-	if !auto.ObjectMeta.DeletionTimestamp.IsZero() {
-		controllerutil.RemoveFinalizer(&auto, imagev1.ImageUpdateAutomationFinalizer)
-		if err := r.Update(ctx, &auto); err != nil {
-			return ctrl.Result{}, err
-		}
-		return ctrl.Result{}, nil
-	}
-
 	// record suspension metrics
 	r.RecordSuspend(ctx, &auto, auto.Spec.Suspend)
 

diff --git a/internal/controller/imageupdateautomation_controller_test.go b/internal/controller/imageupdateautomation_controller_test.go
@@ -18,8 +18,17 @@ package controller
 
 import (
 	"testing"
+	"time"
 
 	fuzz "github.com/AdaLogics/go-fuzz-headers"
+	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/tools/record"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	imagev1 "github.com/fluxcd/image-automation-controller/api/v1beta1"
 )
 
 func Fuzz_templateMsg(f *testing.F) {
@@ -33,3 +42,40 @@ func Fuzz_templateMsg(f *testing.F) {
 		_, _ = templateMsg(template, &values)
 	})
 }
+
+func TestImageUpdateAutomationReconciler_deleteBeforeFinalizer(t *testing.T) {
+	g := NewWithT(t)
+
+	namespaceName := "ocirepo-" + 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())
+	})
+
+	imageUpdate := &imagev1.ImageUpdateAutomation{}
+	imageUpdate.Name = "test-imageupdate"
+	imageUpdate.Namespace = namespaceName
+	imageUpdate.Spec = imagev1.ImageUpdateAutomationSpec{
+		Interval: metav1.Duration{Duration: time.Second},
+		SourceRef: imagev1.CrossNamespaceSourceReference{
+			Kind: "GitRepository",
+			Name: "foo",
+		},
+	}
+	// Add a test finalizer to prevent the object from getting deleted.
+	imageUpdate.SetFinalizers([]string{"test-finalizer"})
+	g.Expect(k8sClient.Create(ctx, imageUpdate)).NotTo(HaveOccurred())
+	// Add deletion timestamp by deleting the object.
+	g.Expect(k8sClient.Delete(ctx, imageUpdate)).NotTo(HaveOccurred())
+
+	r := &ImageUpdateAutomationReconciler{
+		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(imageUpdate)})
+	g.Expect(err).NotTo(HaveOccurred())
+}
diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go
@@ -27,6 +27,7 @@ import (
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/kubernetes/scheme"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	imagev1_reflect "github.com/fluxcd/image-reflector-controller/api/v1beta2"
 	"github.com/fluxcd/pkg/runtime/controller"
@@ -44,8 +45,9 @@ import (
 // Gomega.
 
 var (
-	testEnv *testenv.Environment
-	ctx     = ctrl.SetupSignalHandler()
+	k8sClient client.Client
+	testEnv   *testenv.Environment
+	ctx       = ctrl.SetupSignalHandler()
 )
 
 func init() {
@@ -74,6 +76,13 @@ func runTestsWithFeatures(m *testing.M, feats map[string]bool) int {
 		testenv.WithMaxConcurrentReconciles(2),
 	)
 
+	var err error
+	// Initialize a cacheless client for tests that need the latest objects.
+	k8sClient, err = client.New(testEnv.Config, client.Options{Scheme: scheme.Scheme})
+	if err != nil {
+		panic(fmt.Sprintf("failed to create k8s client: %v", err))
+	}
+
 	controllerName := "image-automation-controller"
 	if err := (&ImageUpdateAutomationReconciler{
 		Client:        testEnv,