fix: fix the bug in IgnoreTeardownUntil logic

The recently added `IgnoreTeardownWhile` function to the QTransform controllers introduced a bug in the existing `IgnoreTeardownUntil` logic: we were wrongly short-circuiting in the finalizer check loop, causing the controller to run teardown prematurely. Fix the logic and modify the `IgnoreTeardownUntil` test to catch the issue before the fix. Signed-off-by: Utku Ozdemir <[email protected]>
cosi-project · Dec 4, 2024 · f179603 · f179603
1 parent de18545
commit f179603
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 13 deletions.
diff --git a/pkg/controller/generic/qtransform/qtransform.go b/pkg/controller/generic/qtransform/qtransform.go
@@ -198,19 +198,23 @@ func (ctrl *QController[Input, Output]) Reconcile(ctx context.Context, logger *z
 	case resource.PhaseTearingDown:
 		ignoreTearingDown := false
 
-		// if there's an option to ignore finalizers, check if we should ignore tearing down
-		// and perform "normal" reconcile instead
-		if ctrl.options.ignoreTeardownUntilFinalizers != nil || ctrl.options.ignoreTeardownWhileFinalizers != nil {
-			for _, fin := range *in.Metadata().Finalizers() {
-				if fin == ctrl.ControllerName {
-					continue
-				}
+		for _, fin := range *in.Metadata().Finalizers() {
+			if fin == ctrl.ControllerName {
+				continue
+			}
+
+			if ctrl.options.ignoreTeardownUntilFinalizers != nil {
+				if _, allowed := ctrl.options.ignoreTeardownUntilFinalizers[fin]; !allowed {
+					ignoreTearingDown = true // found an unexpected finalizer, skip teardown
 
-				if _, present = ctrl.options.ignoreTeardownUntilFinalizers[fin]; present {
-					continue
+					break
 				}
+			}
 
-				if _, present = ctrl.options.ignoreTeardownWhileFinalizers[fin]; present {
+			// Check ignoreTeardownWhileFinalizers
+			if ctrl.options.ignoreTeardownWhileFinalizers != nil {
+				if _, present := ctrl.options.ignoreTeardownWhileFinalizers[fin]; present {
+					// Found a matching "while" finalizer, skip teardown
 					ignoreTearingDown = true
 
 					break

diff --git a/pkg/controller/generic/qtransform/qtransform_test.go b/pkg/controller/generic/qtransform/qtransform_test.go
@@ -417,7 +417,6 @@ func TestDestroy(t *testing.T) {
 	})
 }
 
-//nolint:dupl
 func TestDestroyWithIgnoreTeardownUntil(t *testing.T) {
 	setup(t, func(ctx context.Context, st state.State, runtime *runtime.Runtime) {
 		require.NoError(t, runtime.RegisterQController(NewABCController(qtransform.WithIgnoreTeardownUntil("extra-finalizer"))))
@@ -437,7 +436,7 @@ func TestDestroyWithIgnoreTeardownUntil(t *testing.T) {
 		rtestutils.AssertNoResource[*B](ctx, t, st, "transformed-1")
 
 		// add two finalizers to '2'
-		require.NoError(t, st.AddFinalizer(ctx, NewA("2", ASpec{}).Metadata(), "extra-finalizer", "other-finalizer"))
+		require.NoError(t, st.AddFinalizer(ctx, NewA("2", ASpec{}).Metadata(), "extra-finalizer", "other-finalizer", "yet-another-finalizer"))
 
 		// teardown input '2'
 		_, err := st.Teardown(ctx, NewA("2", ASpec{}).Metadata())
@@ -451,6 +450,14 @@ func TestDestroyWithIgnoreTeardownUntil(t *testing.T) {
 		// remove other-finalizer
 		require.NoError(t, st.RemoveFinalizer(ctx, NewA("2", ASpec{}).Metadata(), "other-finalizer"))
 
+		// the output 'transformed-2' should still not be torn down yet
+		rtestutils.AssertResources(ctx, t, st, []resource.ID{"transformed-2"}, func(r *B, asrt *assert.Assertions) {
+			asrt.Equal(resource.PhaseRunning, r.Metadata().Phase())
+		})
+
+		// remove yet-another-finalizer
+		require.NoError(t, st.RemoveFinalizer(ctx, NewA("2", ASpec{}).Metadata(), "yet-another-finalizer"))
+
 		// the output 'transformed-2' should be destroyed now
 		rtestutils.AssertNoResource[*B](ctx, t, st, "transformed-2")
 
@@ -467,7 +474,6 @@ func TestDestroyWithIgnoreTeardownUntil(t *testing.T) {
 	})
 }
 
-//nolint:dupl
 func TestDestroyWithIgnoreTeardownWhile(t *testing.T) {
 	setup(t, func(ctx context.Context, st state.State, runtime *runtime.Runtime) {
 		require.NoError(t, runtime.RegisterQController(NewABCController(qtransform.WithIgnoreTeardownWhile("extra-finalizer"))))