Repository: akuity/kargo
Pull request #2230 (2024-07-02 02:07:54)

chore: audit controller and management controller permissions

netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *docs-kargo-akuity-io* processing.


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | e1e6aca81df91797f30a5fda8a2357cce6e012d7 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/668360fd479cc1000841e12c |
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/akuity/kargo/pull/2230?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) Report
Attention: Patch coverage is `78.46154%` with `14 lines` in your changes missing coverage. Please review.
> Project coverage is 46.35%. Comparing base [(`9ae2aba`)](https://app.codecov.io/gh/akuity/kargo/commit/9ae2aba78bc713583a4a99ab1f8e3d3876e0b6b7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) to head [(`e1e6aca`)](https://app.codecov.io/gh/akuity/kargo/commit/e1e6aca81df91797f30a5fda8a2357cce6e012d7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).
> Report is 2 commits behind head on main.

| [Files](https://app.codecov.io/gh/akuity/kargo/pull/2230?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) | Patch % | Lines |
|---|---|---|
| [api/v1alpha1/helpers.go](https://app.codecov.io/gh/akuity/kargo/pull/2230?src=pr&el=tree&filepath=api%2Fv1alpha1%2Fhelpers.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity#diff-YXBpL3YxYWxwaGExL2hlbHBlcnMuZ28=) | 79.54% | [5 Missing and 4 partials :warning: ](https://app.codecov.io/gh/akuity/kargo/pull/2230?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) |
| [internal/controller/stages/stages.go](https://app.codecov.io/gh/akuity/kargo/pull/2230?src=pr&el=tree&filepath=internal%2Fcontroller%2Fstages%2Fstages.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity#diff-aW50ZXJuYWwvY29udHJvbGxlci9zdGFnZXMvc3RhZ2VzLmdv) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/akuity/kargo/pull/2230?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) |
| [...nternal/controller/management/projects/projects.go](https://app.codecov.io/gh/akuity/kargo/pull/2230?src=pr&el=tree&filepath=internal%2Fcontroller%2Fmanagement%2Fprojects%2Fprojects.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity#diff-aW50ZXJuYWwvY29udHJvbGxlci9tYW5hZ2VtZW50L3Byb2plY3RzL3Byb2plY3RzLmdv) | 86.66% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/akuity/kargo/pull/2230?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #2230      +/-   ##
==========================================
+ Coverage   46.30%   46.35%   +0.05%     
==========================================
  Files         242      242              
  Lines       16775    16808      +33     
==========================================
+ Hits         7767     7792      +25     
- Misses       8636     8640       +4     
- Partials      372      376       +4     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/akuity/kargo/pull/2230?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).


Patch:
diff --git a/api/v1alpha1/helpers.go b/api/v1alpha1/helpers.go
index f65f022b..5a98693d 100644
--- a/api/v1alpha1/helpers.go
+++ b/api/v1alpha1/helpers.go
@@ -5,40 +5,75 @@ import (
 	"encoding/json"
 	"fmt"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
 	"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 {
+func EnsureFinalizer(ctx context.Context, c client.Client, obj client.Object) (bool, 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 true, patchFinalizers(ctx, c, obj)
+	}
+	return false, nil
+}
+
+func RemoveFinalizer(ctx context.Context, c client.Client, obj client.Object) error {
+	if controllerutil.RemoveFinalizer(obj, FinalizerName) {
+		return patchFinalizers(ctx, c, obj)
 	}
 	return nil
 }
 
-func ClearAnnotations(ctx context.Context, c client.Client, obj client.Object, keys ...string) error {
-	kvs := make(map[string]*string, len(keys))
-	for _, k := range keys {
-		kvs[k] = nil
+func patchFinalizers(ctx context.Context, c client.Client, obj client.Object) error {
+	type objectMeta struct {
+		Finalizers []string `json:"finalizers"`
+	}
+	type patch struct {
+		ObjectMeta objectMeta `json:"metadata"`
+	}
+	data, err := json.Marshal(patch{
+		ObjectMeta: objectMeta{
+			Finalizers: obj.GetFinalizers(),
+		},
+	})
+	if err != nil {
+		return fmt.Errorf("marshal patch data: %w", err)
 	}
-	return patchAnnotations(ctx, c, obj, kvs)
+	if err := c.Patch(
+		ctx,
+		obj,
+		client.RawPatch(types.MergePatchType, data),
+	); err != nil {
+		return fmt.Errorf("patch finalizers: %w", err)
+	}
+	return nil
+}
+
+func PatchOwnerReferences(ctx context.Context, c client.Client, obj client.Object) error {
+	type objectMeta struct {
+		OwnerReferences []metav1.OwnerReference `json:"ownerReferences"`
+	}
+	type patch struct {
+		ObjectMeta objectMeta `json:"metadata"`
+	}
+	data, err := json.Marshal(patch{
+		ObjectMeta: objectMeta{
+			OwnerReferences: obj.GetOwnerReferences(),
+		},
+	})
+	if err != nil {
+		return fmt.Errorf("marshal patch data: %w", err)
+	}
+	if err := c.Patch(
+		ctx,
+		obj,
+		client.RawPatch(types.MergePatchType, data),
+	); err != nil {
+		return fmt.Errorf("patch owner references: %w", err)
+	}
+	return nil
 }
 
 func patchAnnotation(ctx context.Context, c client.Client, obj client.Object, key, value string) error {
diff --git a/charts/kargo/templates/controller/cluster-roles.yaml b/charts/kargo/templates/controller/cluster-roles.yaml
index 997ab512..46cd68b5 100644
--- a/charts/kargo/templates/controller/cluster-roles.yaml
+++ b/charts/kargo/templates/controller/cluster-roles.yaml
@@ -40,7 +40,6 @@ rules:
   - get
   - list
   - watch
-  - patch
 - apiGroups:
   - kargo.akuity.io
   resources:
@@ -50,7 +49,6 @@ rules:
   - list
   - patch
   - promote
-  - update
   - watch
 - apiGroups:
   - kargo.akuity.io
@@ -59,7 +57,6 @@ rules:
   verbs:
   - get
   - list
-  - patch
   - watch
 - apiGroups:
   - kargo.akuity.io
@@ -70,16 +67,7 @@ rules:
   - warehouses/finalizers
   - warehouses/status
   verbs:
-  - update
   - patch
-- apiGroups:
-  - argoproj.io
-  resources:
-  - analysistemplates
-  verbs:
-  - get
-  - list
-  - watch
 {{- if and .Values.controller.argocd.integrationEnabled (not .Values.controller.argocd.watchArgocdNamespaceOnly) }}
 ---
 apiVersion: rbac.authorization.k8s.io/v1
@@ -110,6 +98,14 @@ metadata:
     {{- include "kargo.labels" . | nindent 4 }}
     {{- include "kargo.controller.labels" . | nindent 4 }}
 rules:
+- apiGroups:
+  - argoproj.io
+  resources:
+  - analysistemplates
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - argoproj.io
   resources:
diff --git a/charts/kargo/templates/management-controller/cluster-roles.yaml b/charts/kargo/templates/management-controller/cluster-roles.yaml
index 8ae7bd7d..d7852193 100644
--- a/charts/kargo/templates/management-controller/cluster-roles.yaml
+++ b/charts/kargo/templates/management-controller/cluster-roles.yaml
@@ -16,7 +16,6 @@ rules:
   - get
   - list
   - patch
-  - update
   - watch
 - apiGroups:
   - ""
@@ -47,5 +46,4 @@ rules:
   - projects/status
   verbs:
   - patch
-  - update
 {{- end }}  
diff --git a/internal/controller/management/namespaces/namespaces.go b/internal/controller/management/namespaces/namespaces.go
index 13e0535d..34464890 100644
--- a/internal/controller/management/namespaces/namespaces.go
+++ b/internal/controller/management/namespaces/namespaces.go
@@ -38,10 +38,10 @@ type reconciler struct {
 		...client.DeleteOption,
 	) error
 
-	updateNamespaceFn func(
+	removeFinalizerFn func(
 		context.Context,
+		client.Client,
 		client.Object,
-		...client.UpdateOption,
 	) error
 }
 
@@ -70,7 +70,7 @@ func newReconciler(kubeClient client.Client) *reconciler {
 	}
 	r.getNamespaceFn = r.client.Get
 	r.deleteProjectFn = r.client.Delete
-	r.updateNamespaceFn = r.client.Update
+	r.removeFinalizerFn = kargoapi.RemoveFinalizer
 	return r
 }
 
@@ -118,10 +118,8 @@ func (r *reconciler) Reconcile(
 	); err != nil {
 		return ctrl.Result{}, fmt.Errorf("error deleting Project %q: %w", ns.Name, err)
 	}
-	if controllerutil.RemoveFinalizer(ns, kargoapi.FinalizerName) {
-		if err := r.updateNamespaceFn(ctx, ns); err != nil {
-			return ctrl.Result{}, fmt.Errorf("error removing finalizer: %w", err)
-		}
+	if err := r.removeFinalizerFn(ctx, r.client, ns); err != nil {
+		return ctrl.Result{}, fmt.Errorf("error removing finalizer: %w", err)
 	}
 	logger.Debug("done reconciling Namespace")
 	return ctrl.Result{}, nil
diff --git a/internal/controller/management/projects/projects.go b/internal/controller/management/projects/projects.go
index 8e1040b1..31c189d5 100644
--- a/internal/controller/management/projects/projects.go
+++ b/internal/controller/management/projects/projects.go
@@ -78,12 +78,18 @@ type reconciler struct {
 		...client.CreateOption,
 	) error
 
-	updateNamespaceFn func(
+	patchOwnerReferencesFn func(
 		context.Context,
+		client.Client,
 		client.Object,
-		...client.UpdateOption,
 	) error
 
+	ensureFinalizerFn func(
+		context.Context,
+		client.Client,
+		client.Object,
+	) (bool, error)
+
 	ensureAPIAdminPermissionsFn func(context.Context, *kargoapi.Project) error
 
 	ensureDefaultProjectRolesFn func(context.Context, *kargoapi.Project) error
@@ -138,7 +144,8 @@ func newReconciler(kubeClient client.Client, cfg ReconcilerConfig) *reconciler {
 	r.patchProjectStatusFn = r.patchProjectStatus
 	r.getNamespaceFn = r.client.Get
 	r.createNamespaceFn = r.client.Create
-	r.updateNamespaceFn = r.client.Update
+	r.patchOwnerReferencesFn = kargoapi.PatchOwnerReferences
+	r.ensureFinalizerFn = kargoapi.EnsureFinalizer
 	r.ensureAPIAdminPermissionsFn = r.ensureAPIAdminPermissions
 	r.ensureDefaultProjectRolesFn = r.ensureDefaultProjectRoles
 	r.createServiceAccountFn = r.client.Create
@@ -283,12 +290,24 @@ func (r *reconciler) ensureNamespace(
 		// uncommon scenario where an organization has its own controller that
 		// creates and initializes namespaces to ensure compliance with
 		// internal policies. Such a controller might already own the namespace.
+		updated, err := r.ensureFinalizerFn(ctx, r.client, ns)
+		if err != nil {
+			return status, fmt.Errorf("error ensuring finalizer on namespace %q: %w", project.Name, err)
+		}
+		if updated {
+			logger.Debug("added finalizer to namespace")
+		}
 		ns.OwnerReferences = append(ns.OwnerReferences, *ownerRef)
-		controllerutil.AddFinalizer(ns, kargoapi.FinalizerName)
-		if err = r.updateNamespaceFn(ctx, ns); err != nil {
-			return status, fmt.Errorf("error updating namespace %q: %w", project.Name, err)
+		if err = r.patchOwnerReferencesFn(ctx, r.client, ns); err != nil {
+			return status, fmt.Errorf(
+				"error patching namespace %q with project %q as owner: %w",
+				project.Name,
+				project.Name,
+				err,
+			)
 		}
-		logger.Debug("updated namespace with Project as owner")
+		logger.Debug("patched namespace with Project as owner")
+
 		return status, nil
 	}
 
diff --git a/internal/controller/stages/stages.go b/internal/controller/stages/stages.go
index eb69e272..0cf96988 100644
--- a/internal/controller/stages/stages.go
+++ b/internal/controller/stages/stages.go
@@ -528,14 +528,13 @@ func (r *reconciler) Reconcile(
 	var newStatus kargoapi.StageStatus
 	if stage.DeletionTimestamp != nil {
 		newStatus, err = r.syncStageDelete(ctx, stage)
-		if err == nil && controllerutil.RemoveFinalizer(stage, kargoapi.FinalizerName) {
-			if err = r.kargoClient.Update(ctx, stage); err != nil {
+		if err == nil {
+			if err = kargoapi.RemoveFinalizer(ctx, r.kargoClient, stage); err != nil {
 				err = fmt.Errorf("error removing finalizer: %w", err)
 			}
 		}
 	} else {
-		err = kargoapi.AddFinalizer(ctx, r.kargoClient, stage)
-		if err != nil {
+		if _, err = kargoapi.EnsureFinalizer(ctx, r.kargoClient, stage); err != nil {
 			newStatus = stage.Status
 		} else {
 			if stage.Spec.PromotionMechanisms == nil {


Test patch:
diff --git a/api/v1alpha1/helpers_test.go b/api/v1alpha1/helpers_test.go
index 916a71de..da25ac52 100644
--- a/api/v1alpha1/helpers_test.go
+++ b/api/v1alpha1/helpers_test.go
@@ -5,15 +5,17 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/ptr"
 	"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) {
+func TestEnsureFinalizer(t *testing.T) {
 	const testNamespace = "fake-namespace"
 	const testStageName = "fake-stage"
 
@@ -31,8 +33,9 @@ func TestAddFinalizer(t *testing.T) {
 	require.NoError(t, err)
 	c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(stage).Build()
 
-	err = AddFinalizer(ctx, c, stage)
+	updated, err := EnsureFinalizer(ctx, c, stage)
 	require.NoError(t, err)
+	require.True(t, updated)
 
 	patchedStage := &Stage{}
 	err = c.Get(
@@ -48,126 +51,96 @@ func TestAddFinalizer(t *testing.T) {
 	require.True(t, controllerutil.ContainsFinalizer(patchedStage, FinalizerName))
 }
 
-func TestClearAnnotations(t *testing.T) {
-	scheme := k8sruntime.NewScheme()
-	require.NoError(t, SchemeBuilder.AddToScheme(scheme))
+func TestRemoveFinalizer(t *testing.T) {
+	const testNamespace = "fake-namespace"
+	const testStageName = "fake-stage"
 
-	newFakeClient := func(obj ...client.Object) client.Client {
-		return fake.NewClientBuilder().
-			WithScheme(scheme).
-			WithObjects(obj...).
-			Build()
-	}
+	ctx := context.Background()
 
-	testCases := []struct {
-		name       string
-		client     client.Client
-		obj        client.Object
-		keys       []string
-		assertions func(*testing.T, client.Object, error)
-	}{
-		{
-			name:   "no keys",
-			client: newFakeClient(),
-			obj:    nil,
-			keys:   nil,
-			assertions: func(t *testing.T, _ client.Object, err error) {
-				require.NoError(t, err)
-			},
-		},
-		{
-			name: "no annotations",
-			client: newFakeClient(&Stage{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "stage",
-				},
-			}),
-			obj: &Stage{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "stage",
-				},
-			},
-			keys: []string{"key"},
-			assertions: func(t *testing.T, _ client.Object, err error) {
-				require.NoError(t, err)
-			},
+	stage := &Stage{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace:  testNamespace,
+			Name:       testStageName,
+			Finalizers: []string{FinalizerName},
 		},
-		{
-			name:   "not found",
-			client: newFakeClient(),
-			obj: &Stage{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "stage",
-				},
-			},
-			keys: []string{"key"},
-			assertions: func(t *testing.T, _ client.Object, err error) {
-				require.ErrorContains(t, err, "patch annotation")
-			},
+	}
+
+	scheme := k8sruntime.NewScheme()
+	err := AddToScheme(scheme)
+	require.NoError(t, err)
+	c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(stage).Build()
+
+	err = RemoveFinalizer(ctx, c, stage)
+	require.NoError(t, err)
+
+	patchedStage := &Stage{}
+	err = c.Get(
+		ctx,
+		types.NamespacedName{
+			Namespace: testNamespace,
+			Name:      testStageName,
 		},
-		{
-			name: "clear one",
-			client: newFakeClient(&Stage{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "stage",
-					Annotations: map[string]string{
-						"key1": "value1",
-						"key2": "value2",
-					},
-				},
-			}),
-			obj: &Stage{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "stage",
-				},
-			},
-			keys: []string{"key1"},
-			assertions: func(t *testing.T, obj client.Object, err error) {
-				require.NoError(t, err)
-				require.Contains(t, obj.GetAnnotations(), "key2")
-				require.NotContains(t, obj.GetAnnotations(), "key1")
-			},
+		patchedStage,
+	)
+	require.NoError(t, err)
+
+	require.False(t, controllerutil.ContainsFinalizer(patchedStage, FinalizerName))
+}
+
+func TestPatchOwnerReferences(t *testing.T) {
+	const testNamespace = "fake-namespace"
+	const testProjectName = "fake-project"
+
+	ctx := context.Background()
+
+	initialNS := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: testNamespace,
 		},
-		{
-			name: "clear two",
-			client: newFakeClient(&Stage{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "stage",
-					Annotations: map[string]string{
-						"key1": "value1",
-						"key2": "value2",
-					},
-				},
-			}),
-			obj: &Stage{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "stage",
-				},
-			},
-			keys: []string{"key1", "key2"},
-			assertions: func(t *testing.T, obj client.Object, err error) {
-				require.NoError(t, err)
-				require.NotContains(t, obj.GetAnnotations(), "key2")
-				require.NotContains(t, obj.GetAnnotations(), "key1")
-			},
+	}
+
+	testProject := &Project{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: testProjectName,
 		},
 	}
 
-	for _, tc := range testCases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-			err := ClearAnnotations(context.TODO(), tc.client, tc.obj, tc.keys...)
-			tc.assertions(t, tc.obj, err)
-		})
+	scheme := k8sruntime.NewScheme()
+	err := corev1.AddToScheme(scheme)
+	require.NoError(t, err)
+	err = AddToScheme(scheme)
+	require.NoError(t, err)
+	c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(
+		initialNS,
+		testProject,
+	).Build()
+
+	newNS := initialNS.DeepCopy()
+
+	ownerRef := metav1.NewControllerRef(
+		testProject,
+		GroupVersion.WithKind("Project"),
+	)
+	ownerRef.BlockOwnerDeletion = ptr.To(false)
+
+	newNS.OwnerReferences = []metav1.OwnerReference{
+		*ownerRef,
 	}
+
+	err = PatchOwnerReferences(ctx, c, newNS)
+	require.NoError(t, err)
+
+	patchedNS := &corev1.Namespace{}
+	err = c.Get(
+		ctx,
+		types.NamespacedName{
+			Name: testNamespace,
+		},
+		patchedNS,
+	)
+	require.NoError(t, err)
+
+	require.Equal(t, newNS.OwnerReferences, patchedNS.OwnerReferences)
 }
 
 func Test_patchAnnotation(t *testing.T) {
diff --git a/internal/controller/management/namespaces/namespaces_test.go b/internal/controller/management/namespaces/namespaces_test.go
index 42a4a4d6..a815ac68 100644
--- a/internal/controller/management/namespaces/namespaces_test.go
+++ b/internal/controller/management/namespaces/namespaces_test.go
@@ -23,7 +23,7 @@ func TestNewReconciler(t *testing.T) {
 	require.NotNil(t, r.client)
 	require.NotNil(t, r.getNamespaceFn)
 	require.NotNil(t, r.deleteProjectFn)
-	require.NotNil(t, r.updateNamespaceFn)
+	require.NotNil(t, r.removeFinalizerFn)
 }
 
 func TestReconcile(t *testing.T) {
@@ -142,10 +142,10 @@ func TestReconcile(t *testing.T) {
 				) error {
 					return apierrors.NewNotFound(schema.GroupResource{}, "")
 				},
-				updateNamespaceFn: func(
+				removeFinalizerFn: func(
 					context.Context,
+					client.Client,
 					client.Object,
-					...client.UpdateOption,
 				) error {
 					return nil
 				},
@@ -209,10 +209,10 @@ func TestReconcile(t *testing.T) {
 				) error {
 					return nil
 				},
-				updateNamespaceFn: func(
+				removeFinalizerFn: func(
 					context.Context,
+					client.Client,
 					client.Object,
-					...client.UpdateOption,
 				) error {
 					return errors.New("something went wrong")
 				},
@@ -243,10 +243,10 @@ func TestReconcile(t *testing.T) {
 				) error {
 					return nil
 				},
-				updateNamespaceFn: func(
+				removeFinalizerFn: func(
 					context.Context,
+					client.Client,
 					client.Object,
-					...client.UpdateOption,
 				) error {
 					return nil
 				},
diff --git a/internal/controller/management/projects/projects_test.go b/internal/controller/management/projects/projects_test.go
index ee355c52..787564a5 100644
--- a/internal/controller/management/projects/projects_test.go
+++ b/internal/controller/management/projects/projects_test.go
@@ -29,7 +29,8 @@ func TestNewReconciler(t *testing.T) {
 	require.NotNil(t, r.patchProjectStatusFn)
 	require.NotNil(t, r.getNamespaceFn)
 	require.NotNil(t, r.createNamespaceFn)
-	require.NotNil(t, r.updateNamespaceFn)
+	require.NotNil(t, r.patchOwnerReferencesFn)
+	require.NotNil(t, r.ensureFinalizerFn)
 	require.NotNil(t, r.ensureAPIAdminPermissionsFn)
 	require.NotNil(t, r.ensureDefaultProjectRolesFn)
 	require.NotNil(t, r.createServiceAccountFn)
@@ -402,7 +403,7 @@ func TestEnsureNamespace(t *testing.T) {
 		},
 		{
 			name: "namespace exists, is labeled as a project namespace, and is " +
-				"NOT already owned by the project; error updating it",
+				"NOT already owned by the project; error ensuring finalizer",
 			project: &kargoapi.Project{
 				Status: kargoapi.ProjectStatus{
 					Phase: kargoapi.ProjectPhaseInitializing,
@@ -422,16 +423,60 @@ func TestEnsureNamespace(t *testing.T) {
 					}
 					return nil
 				},
-				updateNamespaceFn: func(
+				ensureFinalizerFn: func(
 					context.Context,
+					client.Client,
+					client.Object,
+				) (bool, error) {
+					return false, errors.New("something went wrong")
+				},
+			},
+			assertions: func(t *testing.T, status kargoapi.ProjectStatus, err error) {
+				require.ErrorContains(t, err, "error ensuring finalizer on namespace")
+				require.ErrorContains(t, err, "something went wrong")
+				// Phase wasn't changed
+				require.Equal(t, kargoapi.ProjectPhaseInitializing, status.Phase)
+			},
+		},
+		{
+			name: "namespace exists, is labeled as a project namespace, and is " +
+				"NOT already owned by the project; error patching it",
+			project: &kargoapi.Project{
+				Status: kargoapi.ProjectStatus{
+					Phase: kargoapi.ProjectPhaseInitializing,
+				},
+			},
+			reconciler: &reconciler{
+				getNamespaceFn: func(
+					_ context.Context,
+					_ types.NamespacedName,
+					obj client.Object,
+					_ ...client.GetOption,
+				) error {
+					ns, ok := obj.(*corev1.Namespace)
+					require.True(t, ok)
+					ns.Labels = map[string]string{
+						kargoapi.ProjectLabelKey: kargoapi.LabelTrueValue,
+					}
+					return nil
+				},
+				ensureFinalizerFn: func(
+					context.Context,
+					client.Client,
+					client.Object,
+				) (bool, error) {
+					return false, nil
+				},
+				patchOwnerReferencesFn: func(
+					context.Context,
+					client.Client,
 					client.Object,
-					...client.UpdateOption,
 				) error {
 					return errors.New("something went wrong")
 				},
 			},
 			assertions: func(t *testing.T, status kargoapi.ProjectStatus, err error) {
-				require.ErrorContains(t, err, "error updating namespace")
+				require.ErrorContains(t, err, "error patching namespace")
 				require.ErrorContains(t, err, "something went wrong")
 				// Phase wasn't changed
 				require.Equal(t, kargoapi.ProjectPhaseInitializing, status.Phase)
@@ -439,7 +484,7 @@ func TestEnsureNamespace(t *testing.T) {
 		},
 		{
 			name: "namespace exists, is labeled as a project namespace, and is " +
-				"NOT already owned by the project; success updating it",
+				"NOT already owned by the project; success",
 			project: &kargoapi.Project{
 				Status: kargoapi.ProjectStatus{
 					Phase: kargoapi.ProjectPhaseInitializing,
@@ -459,10 +504,17 @@ func TestEnsureNamespace(t *testing.T) {
 					}
 					return nil
 				},
-				updateNamespaceFn: func(
+				ensureFinalizerFn: func(
 					context.Context,
+					client.Client,
+					client.Object,
+				) (bool, error) {
+					return false, nil
+				},
+				patchOwnerReferencesFn: func(
+					context.Context,
+					client.Client,
 					client.Object,
-					...client.UpdateOption,
 				) error {
 					return nil
 				},