Merge pull request #425 from fluxcd/delete-before-finalize

Handle delete before adding finalizer

Author: darkowlzz

internal/controller/imagepolicy_controller.go

@@ -179,18 +179,20 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		r.Metrics.RecordDuration(ctx, obj, start)
 	}()
 
+	// Examine if the object is under deletion.
+	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
+		return r.reconcileDelete(ctx, obj)
+	}
+
 	// Add finalizer first if it doesn't 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, imagev1.ImagePolicyFinalizer) {
 		controllerutil.AddFinalizer(obj, imagev1.ImagePolicyFinalizer)
 		return ctrl.Result{Requeue: true}, nil
 	}
 
-	// Examine if the object is under deletion.
-	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-		return r.reconcileDelete(ctx, obj)
-	}
-
 	// Call subreconciler.
 	result, retErr = r.reconcile(ctx, serialPatcher, obj)
 	return

internal/controller/imagepolicy_controller_test.go

@@ -28,12 +28,50 @@ import (
 	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"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	imagev1 "github.com/fluxcd/image-reflector-controller/api/v1beta2"
 	"github.com/fluxcd/image-reflector-controller/internal/policy"
 )
 
+func TestImagePolicyReconciler_deleteBeforeFinalizer(t *testing.T) {
+	g := NewWithT(t)
+
+	namespaceName := "imagepolicy-" + 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())
+	})
+
+	imagePolicy := &imagev1.ImagePolicy{}
+	imagePolicy.Name = "test-imagepolicy"
+	imagePolicy.Namespace = namespaceName
+	imagePolicy.Spec = imagev1.ImagePolicySpec{
+		ImageRepositoryRef: meta.NamespacedObjectReference{
+			Name: "foo",
+		},
+		Policy: imagev1.ImagePolicyChoice{},
+	}
+	// Add a test finalizer to prevent the object from getting deleted.
+	imagePolicy.SetFinalizers([]string{"test-finalizer"})
+	g.Expect(k8sClient.Create(ctx, imagePolicy)).NotTo(HaveOccurred())
+	// Add deletion timestamp by deleting the object.
+	g.Expect(k8sClient.Delete(ctx, imagePolicy)).NotTo(HaveOccurred())
+
+	r := &ImagePolicyReconciler{
+		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(imagePolicy)})
+	g.Expect(err).NotTo(HaveOccurred())
+}
+
 func TestImagePolicyReconciler_getImageRepository(t *testing.T) {
 	testImageRepoName := "test-repo"
 	testNamespace1 := "test-ns1" // Default namespace of ImagePolicy.

internal/controller/imagerepository_controller.go

@@ -166,18 +166,20 @@ func (r *ImageRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		r.Metrics.RecordDuration(ctx, obj, start)
 	}()
 
+	// Examine if the object is under deletion.
+	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
+		return r.reconcileDelete(ctx, obj)
+	}
+
 	// Add finalizer first if it doesn't 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, imagev1.ImageRepositoryFinalizer) {
 		controllerutil.AddFinalizer(obj, imagev1.ImageRepositoryFinalizer)
 		return ctrl.Result{Requeue: true}, nil
 	}
 
-	// Examine if the object is under deletion.
-	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-		return r.reconcileDelete(ctx, obj)
-	}
-
 	// Return if the object is suspended.
 	if obj.Spec.Suspend {
 		log.Info("reconciliation is suspended for this object")

internal/controller/imagerepository_controller_test.go

@@ -28,6 +28,7 @@ import (
 	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"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -63,6 +64,40 @@ func (db mockDatabase) Tags(repo string) ([]string, error) {
 	return db.TagData, nil
 }
 
+func TestImageRepositoryReconciler_deleteBeforeFinalizer(t *testing.T) {
+	g := NewWithT(t)
+
+	namespaceName := "imagerepo-" + 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())
+	})
+
+	imagerepo := &imagev1.ImageRepository{}
+	imagerepo.Name = "test-gitrepo"
+	imagerepo.Namespace = namespaceName
+	imagerepo.Spec = imagev1.ImageRepositorySpec{
+		Interval: metav1.Duration{Duration: interval},
+		Image:    "test-image",
+	}
+	// Add a test finalizer to prevent the object from getting deleted.
+	imagerepo.SetFinalizers([]string{"test-finalizer"})
+	g.Expect(k8sClient.Create(ctx, imagerepo)).NotTo(HaveOccurred())
+	// Add deletion timestamp by deleting the object.
+	g.Expect(k8sClient.Delete(ctx, imagerepo)).NotTo(HaveOccurred())
+
+	r := &ImageRepositoryReconciler{
+		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(imagerepo)})
+	g.Expect(err).NotTo(HaveOccurred())
+}
+
 func TestImageRepositoryReconciler_setAuthOptions(t *testing.T) {
 	testImg := "example.com/foo/bar"
 	testSecretName := "test-secret"

internal/controller/suite_test.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/fluxcd/pkg/runtime/controller"
 	"github.com/fluxcd/pkg/runtime/testenv"
@@ -53,6 +54,7 @@ const (
 )
 
 var (
+	k8sClient    client.Client
 	testEnv      *testenv.Environment
 	testBadgerDB *badger.DB
 	ctx          = ctrl.SetupSignalHandler()
@@ -69,6 +71,12 @@ func TestMain(m *testing.M) {
 	testEnv = testenv.New(testenv.WithCRDPath(filepath.Join("..", "..", "config", "crd", "bases")))
 
 	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))
+	}
+
 	var badgerDir string
 	badgerDir, err = os.MkdirTemp("", "badger")
 	if err != nil {