Sa optional #2355
Sa optional #2355
Conversation
Changes the ClusterExtension API field spec.ServiceAccount to be optional. Operator-controller will use its own service account by default unless the spec.ServiceAccount field is set. RBAC PreAuthorization only happens if the optional SA field is set, as well. Give operator-controller's SA cluster-admin by default.
…ard/experimental .spec.serviceaccount handling Signed-off-by: grokspawn <[email protected]>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR makes the serviceAccount field optional in the experimental channel while keeping it required in the standard channel. The changes enable operator-controller to use its own ServiceAccount (with synthetic user permissions) when no ServiceAccount is specified, supporting a new installation mode. Key implementation aspects include:
- Modified API types to use channel-specific validation tags
- Enhanced the CRD generator tool to handle optional/required field scoping
- Updated controller logic to conditionally skip authorization checks when ServiceAccount is not provided
- Elevated operator-controller permissions to cluster-admin to support the new mode
- Added comprehensive E2E and unit tests
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1/clusterextension_types.go | Added channel-specific descriptions and validations for ServiceAccount field, changed to omitzero JSON tag, fixed typo |
| api/v1/clusterextension_types_test.go | Added unit tests for ServiceAccount JSON marshaling behavior with omitzero |
| hack/tools/crd-generator/main.go | Enhanced to handle Optional/Required validation tags and propagate required field changes to parent schemas |
| helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml | Generated CRD with updated ServiceAccount description for standard channel |
| helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml | Generated CRD with optional ServiceAccount for experimental channel |
| manifests/standard.yaml | Updated CRD and changed operator-controller to cluster-admin role binding |
| manifests/standard-e2e.yaml | Updated CRD and changed operator-controller to cluster-admin role binding |
| manifests/experimental.yaml | Updated CRD for optional ServiceAccount, changed role binding name |
| manifests/experimental-e2e.yaml | Updated CRD for optional ServiceAccount, changed role binding name |
| helm/olmv1/templates/rbac/clusterrolebinding-operator-controller-manager-rolebinding.yml | Simplified to always use cluster-admin role, removed conditional logic |
| internal/operator-controller/applier/helm.go | Added check to skip pre-authorization when ServiceAccount name is empty |
| internal/operator-controller/action/restconfig.go | Added logic to use operator-controller's ServiceAccount when none is specified |
| internal/operator-controller/controllers/clusterextension_admission_test.go | Updated test to reflect that empty ServiceAccount name is now valid |
| test/e2e/cluster_extension_install_test.go | Added E2E test for installation without ServiceAccount using synthetic user |
| docs/api-reference/olmv1-api-reference.md | Updated documentation with raw opcon tags instead of processed descriptions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | --- | --- | --- | --- | | ||
| | `namespace` _string_ | namespace is a reference to a Kubernetes namespace.<br />This is the namespace in which the provided ServiceAccount must exist.<br />It also designates the default namespace where namespace-scoped resources<br />for the extension are applied to the cluster.<br />Some extensions may contain namespace-scoped resources to be applied in other namespaces.<br />This namespace must exist.<br />namespace is required, immutable, and follows the DNS label standard<br />as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-),<br />start and end with an alphanumeric character, and be no longer than 63 characters<br />[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 63 <br />Required: \{\} <br /> | | ||
| | `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions<br />with the cluster that are required to manage the extension.<br />The ServiceAccount must be configured with the necessary permissions to perform these interactions.<br />The ServiceAccount must exist in the namespace referenced in the spec.<br />serviceAccount is required. | | Required: \{\} <br /> | | ||
| | `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | <opcon:standard:description><br />serviceAccount is a required field that references a ServiceAccount used to<br />perform all interactions with the cluster that are required to manage the extension.<br /></opcon:standard:description><br /><opcon:standard:validation:Required><br /><opcon:experimental:description><br />serviceAccount is an optional field that references a ServiceAccount used to<br />perform all interactions with the cluster that are required to manage the extension.<br />If not set, operator-controller will use its own ServiceAccount for extension management.<br />The ServiceAccount must be configured with the necessary permissions to perform these interactions.<br />The ServiceAccount must exist in the namespace referenced in the spec.<br /></opcon:experimental:description><br /><opcon:experimental:validation:Optional> | | | |
There was a problem hiding this comment.
The documentation contains raw <opcon:*> tags that should have been processed by the CRD generator. These internal markup tags should not appear in end-user documentation. The formatDescription function should have removed or processed these tags to show only the appropriate channel-specific content.
| | `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | <opcon:standard:description><br />serviceAccount is a required field that references a ServiceAccount used to<br />perform all interactions with the cluster that are required to manage the extension.<br /></opcon:standard:description><br /><opcon:standard:validation:Required><br /><opcon:experimental:description><br />serviceAccount is an optional field that references a ServiceAccount used to<br />perform all interactions with the cluster that are required to manage the extension.<br />If not set, operator-controller will use its own ServiceAccount for extension management.<br />The ServiceAccount must be configured with the necessary permissions to perform these interactions.<br />The ServiceAccount must exist in the namespace referenced in the spec.<br /></opcon:experimental:description><br /><opcon:experimental:validation:Optional> | | | | |
| | `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a required field that references a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension.<br />**Experimental:** serviceAccount is an optional field that references a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. If not set, operator-controller will use its own ServiceAccount for extension management. The ServiceAccount must be configured with the necessary permissions to perform these interactions. The ServiceAccount must exist in the namespace referenced in the spec. | | Required (standard), Optional (experimental) | |
| serviceAccount is a required field that references a ServiceAccount used to | ||
| perform all interactions with the cluster that are required to manage the extension. |
There was a problem hiding this comment.
The description states that serviceAccount is 'a required field' but the required list at line 173 has been removed, making this field actually optional in the CRD schema. This creates an inconsistency between the description and the actual schema enforcement. For the standard channel, either the description should be corrected to say it's optional, or the field should remain in the required list.
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| {{- if has "BoxcutterRuntime" .Values.options.operatorController.features.enabled }} | ||
| name: cluster-admin |
There was a problem hiding this comment.
Binding operator-controller to cluster-admin grants unrestricted access to all cluster resources. This is overly permissive and violates the principle of least privilege. The operator-controller should only have the specific permissions it needs to manage ClusterExtensions. Consider creating a custom ClusterRole with only the necessary permissions (e.g., managing CRDs, deployments, services, RBAC resources in specific contexts) instead of using cluster-admin.
| name: cluster-admin | |
| name: operator-controller-manager-role |
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: operator-controller-manager-role | ||
| name: cluster-admin |
There was a problem hiding this comment.
Binding operator-controller to cluster-admin grants unrestricted access to all cluster resources. This is overly permissive and violates the principle of least privilege. The operator-controller should only have the specific permissions it needs to manage ClusterExtensions. Consider creating a custom ClusterRole with only the necessary permissions instead of using cluster-admin.
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: operator-controller-manager-role | ||
| name: cluster-admin |
There was a problem hiding this comment.
Binding operator-controller to cluster-admin grants unrestricted access to all cluster resources. This is overly permissive and violates the principle of least privilege. The operator-controller should only have the specific permissions it needs to manage ClusterExtensions. Consider creating a custom ClusterRole with only the necessary permissions instead of using cluster-admin.
2 participants
Description
Addresses review comments (including new unit, e2e tests) and adds capabilities to the generate-crd tool to handle new optional/required scoped validations which required a change in the API to inform the schema parent to adjust its required list entries based on the desired scoped validation.
(Yes, there appear to be some additional files in here which are unrelated. For a throwaway draft PR, I'm not going to worry about them. )
Reviewer Checklist