🌱 Use SA from spec when installing bundles #907

skattoju · 2024-06-04T14:52:12Z

Description

Adds a ServiceAccountName field to the spec and sets up the action client getter to provide an action client that uses a token based on the service account provided in the spec. Related to #737

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-06-04T14:52:29Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`9b1e2e5`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6678cffae477b4000824e2e6
😎 Deploy Preview	https://deploy-preview-907--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

varshaprasad96 · 2024-06-10T18:48:25Z

/ok-to-test

(not sure if this label is accepted)

joelanford · 2024-06-10T21:08:44Z

internal/controllers/clusterextension_admission_test.go

-				InstallNamespace: tc.installNamespace,
+				PackageName:        "package",
+				InstallNamespace:   tc.installNamespace,
+				ServiceAccountName: tc.serviceAccountName,


This test is just for the spec.installNamespace validation. Should we hardcode ServiceAccountName to something we know will be valid?

That would avoid maintainer/contributor confusion. Folks might see that the service account name is varied as part of this test and conclude that the service account name somehow plays into the installNamespace validatation.

We also want to test side-effects from varying service account names.

joelanford · 2024-06-10T21:09:19Z

internal/controllers/clusterextension_admission_test.go

 			}))
 			if tc.errMsg == "" {
 				require.NoError(t, err, "unexpected error for installNamespace %q: %w", tc.installNamespace, err)
+				require.NoError(t, err, "unexpected error for serviceAccountName %q: %w", tc.serviceAccountName, err)


I think we should put the serviceAccount validations into an isolated test.

makes sense 👍

joelanford · 2024-06-10T21:14:34Z

testdata/rbac/test-rbac.yaml

+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  namespace: default
+  name: test-clusterrole
+rules:
+- apiGroups: ["*"]
+  resources: ["*"]
+  verbs: ["*"]


This is exactly where I would start (and I know this is still WIP), but before we merge, we should update these rules and potentially create separate sets of rules/serviceaccounts for each test bundle.

That way, we can actually prove to ourselves the what a minimum rule set is for a particular bundle. And we'll want to set a good example and not give service accounts cluster admin.

Not related to this work at all: but thinking about it, to make a better UX it would be nice to have some tooling around generating rbac for bundle contents (could be complicated!)

Yeah seems like it will be a lot of trial and error to create the RBAC. Extracting the RBAC from the csv could be a starting point.

joelanford · 2024-06-10T21:16:59Z

cmd/manager/main.go

+	cfgGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(),
+		helmclient.ClientNamespaceMapper(nsMapper),
+		helmclient.StorageNamespaceMapper(nsMapper),
+		helmclient.RestConfigMapper(rcm),


With this setup, we should also remove the */*/* kubebuilder rbac marker in the clusterextension_controller.go file.

we would still need list and watch on all objects right ?
//+kubebuilder:rbac:groups=*,resources=*,verbs=list;watch

The initial reason for using such a broad RBAC was the uncertainty regarding the objects the bundle would contain. Consequently, the controller was granted admin permissions to monitor and perform CRUD operations on all GVKs.

However, in this scenario, the user (specifically the cluster admin) provides a SA that the controller uses to install contents. It's not necessary for the SA to have admin permissions. Instead, the SA can be granted restricted permissions, limited to only the GVKs present in the bundle. It's expected that the entity providing this SA is aware of the specific permissions needed to install and manage the bundle.

This way we minimise the scope of the SA's access, ensuring it only has the necessary permissions to interact with the relevant GRs.

True for installing the bundles the SA provided in the spec will be used but the controller also establishes watches on the bundle content and this is done with the controller service account so it needs to be able to watch arbitrary objects.

everettraven · 2024-06-11T19:24:28Z

api/v1alpha1/clusterextension_types.go

+	//+kubebuilder:validation:MaxLength:=253
+	// ServiceAccountName is the name of the ServiceAccount to use to manage the resources in the bundle.
+	// The service account is expected to exist in the InstallNamespace.
+	ServiceAccountName string `json:"serviceAccountName"`


Optional Nit: Using https://pkg.go.dev/k8s.io/api/core/v1#LocalObjectReference could be nice here as it is meant for local object references like we are doing here.

not sure what is meant here, is there an example you can point me to ?

varshaprasad96 · 2024-06-13T15:38:55Z

internal/authentication/tokengetter.go

+
+type keyLock[K comparable] struct {
+	locks map[K]*sync.Mutex
+	mu    sync.Mutex


Probably an implementation nit: Can we also use a sync.RWMutex here so that we allow multiple threads to do concurrent reading. Looks like this is being used to fetch the token so forcing a single lock is unnecessary? (This is based on a quick look at the code, please feel free to correct me if its otherwise)

varshaprasad96 · 2024-06-13T16:17:13Z

testdata/rbac/test-rbac.yaml

+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  namespace: default
+  name: test-clusterrole
+rules:
+- apiGroups: ["*"]
+  resources: ["*"]
+  verbs: ["*"]


Not related to this work at all: but thinking about it, to make a better UX it would be nice to have some tooling around generating rbac for bundle contents (could be complicated!)

openshift-merge-robot · 2024-06-24T01:48:04Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

everettraven · 2024-07-01T16:55:07Z

@skattoju I'm going to close this draft PR since we are going to split the implementation into individual PRs.

Thank you a ton for all the effort you put into this PoC that helped drive the discussion and design process!

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2024

skattoju force-pushed the sa_from_spec_poc branch 4 times, most recently from cbc5ef7 to c7e5542 Compare June 10, 2024 18:27

openshift-ci bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 10, 2024

joelanford reviewed Jun 10, 2024

View reviewed changes

everettraven reviewed Jun 11, 2024

View reviewed changes

varshaprasad96 reviewed Jun 13, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2024

skattoju force-pushed the sa_from_spec_poc branch 2 times, most recently from ced5993 to b373ac5 Compare June 15, 2024 00:15

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2024

skattoju force-pushed the sa_from_spec_poc branch 3 times, most recently from e4c62fe to a03522c Compare June 15, 2024 14:26

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2024

skattoju force-pushed the sa_from_spec_poc branch from a03522c to 2e7d963 Compare June 17, 2024 23:18

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2024

skattoju force-pushed the sa_from_spec_poc branch 2 times, most recently from 857a904 to 8933ba6 Compare June 20, 2024 20:18

Use SA from spec

9b1e2e5

skattoju force-pushed the sa_from_spec_poc branch from 8933ba6 to 9b1e2e5 Compare June 24, 2024 01:46

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2024

everettraven closed this Jul 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Use SA from spec when installing bundles #907

🌱 Use SA from spec when installing bundles #907

skattoju commented Jun 4, 2024 •

edited

Loading

netlify bot commented Jun 4, 2024 •

edited

Loading

varshaprasad96 commented Jun 10, 2024

joelanford Jun 10, 2024

skattoju Jun 15, 2024

joelanford Jun 10, 2024

skattoju Jun 15, 2024

joelanford Jun 10, 2024

varshaprasad96 Jun 13, 2024

skattoju Jun 15, 2024

joelanford Jun 10, 2024

skattoju Jun 15, 2024

varshaprasad96 Jun 15, 2024

skattoju Jun 15, 2024

everettraven Jun 11, 2024

skattoju Jun 15, 2024

varshaprasad96 Jun 13, 2024 •

edited

Loading

varshaprasad96 Jun 13, 2024

openshift-merge-robot commented Jun 24, 2024

everettraven commented Jul 1, 2024

🌱 Use SA from spec when installing bundles #907

🌱 Use SA from spec when installing bundles #907

Conversation

skattoju commented Jun 4, 2024 • edited Loading

Description

Reviewer Checklist

netlify bot commented Jun 4, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

varshaprasad96 commented Jun 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Jun 24, 2024

everettraven commented Jul 1, 2024

skattoju commented Jun 4, 2024 •

edited

Loading

netlify bot commented Jun 4, 2024 •

edited

Loading

varshaprasad96 Jun 13, 2024 •

edited

Loading