From f179603fef81c5ff595c7e378fb197fc56fb7197 Mon Sep 17 00:00:00 2001 From: Utku Ozdemir Date: Thu, 5 Dec 2024 00:54:05 +0100 Subject: [PATCH] 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 --- .../generic/qtransform/qtransform.go | 24 +++++++++++-------- .../generic/qtransform/qtransform_test.go | 12 +++++++--- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/pkg/controller/generic/qtransform/qtransform.go b/pkg/controller/generic/qtransform/qtransform.go index d795cfc..2746993 100644 --- 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 index 25a59c0..47b840d 100644 --- 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"))))