Repository: akuity/kargo
Pull request #1647 (2024-03-18 18:07:12)

refactor: add finalizer to stages in controller instead of webhook
@gdsoumya had a discussion offline and decided this was preferable because it means the finalizer only ever gets added to Stages if the controller is actually running...

This makes it easier to delete Stages that have never been reconciled.


Patch:
diff --git a/api/v1alpha1/helpers.go b/api/v1alpha1/helpers.go
index e4c03c81..4da7384d 100644
--- a/api/v1alpha1/helpers.go
+++ b/api/v1alpha1/helpers.go
@@ -6,8 +6,31 @@ import (
 
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
+func AddFinalizer(ctx context.Context, c client.Client, obj client.Object) error {
+	if controllerutil.AddFinalizer(obj, FinalizerName) {
+		patchBytes := []byte(`{"metadata":{"finalizers":[`)
+		for i, finalizer := range obj.GetFinalizers() {
+			if i > 0 {
+				patchBytes = append(patchBytes, ',')
+			}
+			patchBytes = append(patchBytes, fmt.Sprintf("%q", finalizer)...)
+		}
+		patchBytes = append(patchBytes, "]}}"...)
+		if err := c.Patch(
+			ctx,
+			obj,
+			client.RawPatch(types.MergePatchType, patchBytes),
+		); err != nil {
+			return fmt.Errorf("patch annotation: %w", err)
+		}
+		return nil
+	}
+	return nil
+}
+
 func ClearAnnotations(ctx context.Context, c client.Client, obj client.Object, keys ...string) error {
 	if len(keys) == 0 {
 		return nil
diff --git a/internal/controller/stages/stages.go b/internal/controller/stages/stages.go
index f171b060..54c96111 100644
--- a/internal/controller/stages/stages.go
+++ b/internal/controller/stages/stages.go
@@ -533,10 +533,17 @@ func (r *reconciler) Reconcile(
 				err = fmt.Errorf("error removing finalizer: %w", err)
 			}
 		}
-	} else if stage.Spec.PromotionMechanisms == nil {
-		newStatus, err = r.syncControlFlowStage(ctx, stage)
 	} else {
-		newStatus, err = r.syncNormalStage(ctx, stage)
+		err = kargoapi.AddFinalizer(ctx, r.kargoClient, stage)
+		if err != nil {
+			newStatus = stage.Status
+		} else {
+			if stage.Spec.PromotionMechanisms == nil {
+				newStatus, err = r.syncControlFlowStage(ctx, stage)
+			} else {
+				newStatus, err = r.syncNormalStage(ctx, stage)
+			}
+		}
 	}
 	if err != nil {
 		newStatus.Message = err.Error()
diff --git a/internal/webhook/stage/webhook.go b/internal/webhook/stage/webhook.go
index e51edd16..20e0d3f9 100644
--- a/internal/webhook/stage/webhook.go
+++ b/internal/webhook/stage/webhook.go
@@ -4,14 +4,12 @@ import (
 	"context"
 	"fmt"
 
-	admissionv1 "k8s.io/api/admission/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	admission "sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
@@ -61,7 +59,7 @@ func newWebhook(kubeClient client.Client) *webhook {
 	return w
 }
 
-func (w *webhook) Default(ctx context.Context, obj runtime.Object) error {
+func (w *webhook) Default(_ context.Context, obj runtime.Object) error {
 	stage := obj.(*kargoapi.Stage) // nolint: forcetypeassert
 
 	// Sync the convenience shard field with the shard label
@@ -74,19 +72,6 @@ func (w *webhook) Default(ctx context.Context, obj runtime.Object) error {
 		stage.Spec.Shard = stage.Labels[kargoapi.ShardLabelKey]
 	}
 
-	req, err := admission.RequestFromContext(ctx)
-	if err != nil {
-		return apierrors.NewInternalError(
-			fmt.Errorf("error getting admission request from context: %w", err),
-		)
-	}
-
-	// Determine if this is a create or update. We don't want to add a finalizer
-	// during update, because that would make it impossible to remove!
-	if req.Operation == admissionv1.Create {
-		controllerutil.AddFinalizer(stage, kargoapi.FinalizerName)
-	}
-
 	return nil
 }
 


Test patch:
diff --git a/api/v1alpha1/helpers_test.go b/api/v1alpha1/helpers_test.go
index 0472b797..17176b65 100644
--- a/api/v1alpha1/helpers_test.go
+++ b/api/v1alpha1/helpers_test.go
@@ -7,10 +7,47 @@ import (
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sruntime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
+func TestAddFinalizer(t *testing.T) {
+	const testNamespace = "fake-namespace"
+	const testStageName = "fake-stage"
+
+	ctx := context.Background()
+
+	stage := &Stage{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: testNamespace,
+			Name:      testStageName,
+		},
+	}
+
+	scheme := k8sruntime.NewScheme()
+	err := AddToScheme(scheme)
+	require.NoError(t, err)
+	c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(stage).Build()
+
+	err = AddFinalizer(ctx, c, stage)
+	require.NoError(t, err)
+
+	patchedStage := &Stage{}
+	err = c.Get(
+		ctx,
+		types.NamespacedName{
+			Namespace: testNamespace,
+			Name:      testStageName,
+		},
+		patchedStage,
+	)
+	require.NoError(t, err)
+
+	require.True(t, controllerutil.ContainsFinalizer(patchedStage, FinalizerName))
+}
+
 func TestClearAnnotations(t *testing.T) {
 	scheme := k8sruntime.NewScheme()
 	require.NoError(t, SchemeBuilder.AddToScheme(scheme))
diff --git a/internal/webhook/stage/webhook_test.go b/internal/webhook/stage/webhook_test.go
index 36a41bcc..0fcfcf19 100644
--- a/internal/webhook/stage/webhook_test.go
+++ b/internal/webhook/stage/webhook_test.go
@@ -12,7 +12,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
@@ -78,34 +77,6 @@ func TestDefault(t *testing.T) {
 				require.Equal(t, testShardName, stage.Spec.Shard)
 			},
 		},
-		{
-			name:      "finalizer gets added on create",
-			operation: admissionv1.Create,
-			stage: &kargoapi.Stage{
-				Spec: &kargoapi.StageSpec{},
-			},
-			assertions: func(t *testing.T, stage *kargoapi.Stage, err error) {
-				require.NoError(t, err)
-				require.True(
-					t,
-					controllerutil.ContainsFinalizer(stage, kargoapi.FinalizerName),
-				)
-			},
-		},
-		{
-			name:      "finalizer does not get added on update",
-			operation: admissionv1.Update,
-			stage: &kargoapi.Stage{
-				Spec: &kargoapi.StageSpec{},
-			},
-			assertions: func(t *testing.T, stage *kargoapi.Stage, err error) {
-				require.NoError(t, err)
-				require.False(
-					t,
-					controllerutil.ContainsFinalizer(stage, kargoapi.FinalizerName),
-				)
-			},
-		},
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {