Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: audit controller and management controller permissions #2230

Merged
merged 9 commits into from
Jul 2, 2024
Merged

chore: audit controller and management controller permissions #2230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ed119c5
remove controller permissions to update stages
krancour Jul 1, 2024
a35a1f8
remove controller permissions to patch freight and promotions
krancour Jul 2, 2024
0eab185
remove controller permissions to patch warehouses
krancour Jul 2, 2024
818c25b
remove controller permissions to update various status subresources (…
krancour Jul 2, 2024
89e12a7
make controller get/list/watch on analysistemplates conditional
krancour Jul 2, 2024
05b06ba
remove management controller permissions to update namespaces
krancour Jul 1, 2024
824a808
remove management controller permissions to update project status (pa…
krancour Jul 2, 2024
34ff2f3
remove unused ClearAnnotations function
krancour Jul 1, 2024
415edab
corresponding code changes
krancour Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 57 additions & 22 deletions api/v1alpha1/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,40 +5,75 @@
"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

Check warning on line 19 in api/v1alpha1/helpers.go

View check run for this annotation

Codecov / codecov/patch

api/v1alpha1/helpers.go#L19 

Added line #L19 was not covered by tests
}

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)

Check warning on line 42 in api/v1alpha1/helpers.go

View check run for this annotation

Codecov / codecov/patch

api/v1alpha1/helpers.go#L42 

Added line #L42 was not covered by tests
}
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)

Check warning on line 49 in api/v1alpha1/helpers.go

View check run for this annotation

Codecov / codecov/patch

api/v1alpha1/helpers.go#L49 

Added line #L49 was not covered by tests
}
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)

Check warning on line 67 in api/v1alpha1/helpers.go

View check run for this annotation

Codecov / codecov/patch

api/v1alpha1/helpers.go#L67 

Added line #L67 was not covered by tests
}
if err := c.Patch(
ctx,
obj,
client.RawPatch(types.MergePatchType, data),
); err != nil {
return fmt.Errorf("patch owner references: %w", err)

Check warning on line 74 in api/v1alpha1/helpers.go

View check run for this annotation

Codecov / codecov/patch

api/v1alpha1/helpers.go#L74 

Added line #L74 was not covered by tests
}
return nil
}

func patchAnnotation(ctx context.Context, c client.Client, obj client.Object, key, value string) error {
Expand Down
199 changes: 86 additions & 113 deletions api/v1alpha1/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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(
Expand All @@ -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) {
Expand Down
20 changes: 8 additions & 12 deletions charts/kargo/templates/controller/cluster-roles.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ rules:
- get
- list
- watch
- patch
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The controller doesn't need to patch Freight or Promotions at all.

- apiGroups:
- kargo.akuity.io
resources:
Expand All @@ -50,7 +49,6 @@ rules:
- list
- patch
- promote
- update
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only update to Stages involves removing a finalizer and that can quite easily be done with a patch.

- watch
- apiGroups:
- kargo.akuity.io
Expand All @@ -59,7 +57,6 @@ rules:
verbs:
- get
- list
- patch
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The controller does not need to patch Warehouses at all.

- watch
- apiGroups:
- kargo.akuity.io
Expand All @@ -70,16 +67,7 @@ rules:
- warehouses/finalizers
- warehouses/status
verbs:
- update
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The controller only ever patches status subresources.

- 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
Expand Down Expand Up @@ -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
Comment on lines +101 to +108
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These permissions are better off down here where they will not be granted in the event that Rollouts integration is completely disabled.

- apiGroups:
- argoproj.io
resources:
Expand Down
Loading
Loading