chore: audit controller and management controller permissions (#2230)

Signed-off-by: Kent Rancourt <[email protected]>

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 {

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) {

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: