✨ Deprecate ClusterExtension spec.serviceAccount#2770
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
✅ 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 pull request deprecates ClusterExtension.spec.serviceAccount and removes the per-service-account authorization/impersonation infrastructure, shifting operator-controller to consistently use its own cluster-admin service account for cluster interactions. It also simplifies drift-detection/watch plumbing by moving from per-ClusterExtension cache management to a shared managedcache.TrackingCache, and updates manifests + test suites accordingly.
Changes:
- Deprecates
spec.serviceAccountin the API (godoc + schema changes) and adds aValidatingAdmissionPolicyto emit kubectl deprecation warnings when it’s set. - Removes per-SA authentication/authorization code paths, feature gates, and related tooling/dependencies.
- Updates Helm charts/manifests and E2E/unit tests to reflect the new model and add warning assertions.
Reviewed changes
Copilot reviewed 74 out of 77 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/extension-developer-e2e/extension_developer_test.go | Removes spec.serviceAccount usage from extension-developer E2E test. |
| test/e2e/steps/testdata/olm-sa-helm-rbac-template.yaml | Removes SA-scoped RBAC template used by older E2E flows. |
| test/e2e/steps/testdata/olm-sa-boxcutter-rbac-template.yaml | Removes SA-scoped Boxcutter RBAC template. |
| test/e2e/steps/testdata/olm-sa-boxcutter-no-create-rbac-template.yaml | Removes “no create verb” RBAC template previously used for preflight-permissions scenarios. |
| test/e2e/steps/testdata/namespace-template.yaml | Adds a reusable namespace YAML template for E2E setup. |
| test/e2e/steps/steps.go | Adds namespace/warning steps; captures kubectl stderr for warning assertions; removes preflight-permissions SA step. |
| test/e2e/steps/hooks.go | Tracks last kubectl apply stderr; removes removed feature gates from test feature map. |
| test/e2e/features/user-managed-fields.feature | Drops SA setup/field; ensures namespace exists explicitly. |
| test/e2e/features/update.feature | Drops SA setup/field; ensures namespace exists explicitly. |
| test/e2e/features/uninstall.feature | Drops SA setup/field and removes SA-deletion resilience scenario tied to old model. |
| test/e2e/features/status.feature | Drops SA setup/field; ensures namespace exists explicitly. |
| test/e2e/features/revision.feature | Drops SA annotations and SA setup; adds namespace setup steps. |
| test/e2e/features/recover.feature | Drops SA setup/field and removes preflight-permissions recovery scenario. |
| test/e2e/features/proxy.feature | Drops SA setup/field; ensures namespace exists explicitly. |
| manifests/standard.yaml | Regenerates CRD schema (serviceAccount optional/deprecated), removes manager ClusterRole, binds operator-controller to cluster-admin, adds VAP + binding. |
| manifests/standard-e2e.yaml | Same as standard.yaml but for e2e manifest variant. |
| manifests/experimental.yaml | Same changes for experimental feature set manifest. |
| manifests/experimental-e2e.yaml | Same changes for experimental e2e manifest variant. |
| Makefile | Removes k8s-pin target and updates verify to use tidy. |
| internal/operator-controller/resolve/catalog_test.go | Updates tests to stop setting spec.serviceAccount. |
| internal/operator-controller/labels/labels.go | Removes SA annotation keys previously used to record service account identity. |
| internal/operator-controller/features/features.go | Removes PreflightPermissions and SyntheticPermissions feature gates. |
| internal/operator-controller/controllers/suite_test.go | Adds envtest setup for VAP + warning capture utilities. |
| internal/operator-controller/controllers/revision_engine_factory.go | Removes SA-scoped client creation; uses base config client for revision engines. |
| internal/operator-controller/controllers/clusterobjectset_controller_test.go | Updates controller tests to remove SA annotations/fields and SA-scoped error scenarios. |
| internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go | Updates internal controller tests to remove SA fields. |
| internal/operator-controller/controllers/clusterextension_reconcile_steps.go | Replaces SA existence validation with a deprecation log warning validator. |
| internal/operator-controller/controllers/clusterextension_controller.go | Adds a WithWatchesRawSource controller builder option. |
| internal/operator-controller/controllers/clusterextension_controller_test.go | Removes SA-related setup and validator tests. |
| internal/operator-controller/controllers/clusterextension_admission_test.go | Adds admission tests for deprecated spec.serviceAccount validation + VAP warning behavior. |
| internal/operator-controller/contentmanager/sourcerer.go | Removes old per-CE dynamic sourcerer implementation. |
| internal/operator-controller/contentmanager/source/internal/eventhandler.go | Removes copied controller-runtime event handler implementation. |
| internal/operator-controller/contentmanager/source/dynamicsource.go | Removes dynamic informer source implementation. |
| internal/operator-controller/contentmanager/source/dynamicsource_test.go | Removes tests for deleted dynamic source. |
| internal/operator-controller/contentmanager/contentmanager.go | Removes per-CE content manager and cache management layer. |
| internal/operator-controller/contentmanager/cache/cache.go | Removes managed-content cache implementation (watch lifecycle per CE). |
| internal/operator-controller/contentmanager/cache/cache_test.go | Removes tests for deleted cache implementation. |
| internal/operator-controller/authentication/tripper.go | Removes token-injecting round-tripper. |
| internal/operator-controller/authentication/tokengetter.go | Removes service-account token getter/cache. |
| internal/operator-controller/authentication/tokengetter_test.go | Removes tests for deleted token getter. |
| internal/operator-controller/authentication/synthetic.go | Removes synthetic impersonation utilities. |
| internal/operator-controller/authentication/synthetic_test.go | Removes tests for deleted synthetic impersonation utilities. |
| internal/operator-controller/applier/helm.go | Removes pre-auth; switches drift detection watch setup to shared tracking cache keyed by GVKs. |
| internal/operator-controller/applier/helm_test.go | Updates Helm applier tests to use mock tracking cache and drop pre-auth cases. |
| internal/operator-controller/applier/boxcutter.go | Removes SA annotations and pre-auth logic from Boxcutter path. |
| internal/operator-controller/applier/boxcutter_test.go | Updates Boxcutter tests to remove SA fields/annotations and pre-auth integration coverage. |
| internal/operator-controller/action/restconfig.go | Removes SA/synthetic rest config mappers. |
| internal/operator-controller/action/restconfig_test.go | Removes tests for deleted restconfig mappers. |
| helm/tilt.yaml | Removes PreflightPermissions from Tilt-enabled feature list. |
| helm/olmv1/values.yaml | Removes deprecated/removed feature gates from default values. |
| helm/olmv1/templates/validatingadmissionpolicybinding-clusterextension-serviceaccount-deprecated.yml | Adds chart template for VAP binding (Warn action). |
| helm/olmv1/templates/validatingadmissionpolicy-clusterextension-serviceaccount-deprecated.yml | Adds chart template for VAP that warns on deprecated serviceAccount usage. |
| helm/olmv1/templates/rbac/clusterrolebinding-operator-controller-manager-rolebinding.yml | Updates binding to reference cluster-admin. |
| helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml | Removes operator-controller manager ClusterRole template. |
| helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml | Regenerates standard CRD with deprecation + optional serviceAccount semantics. |
| helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml | Regenerates experimental CRD with deprecation + optional serviceAccount semantics. |
| helm/experimental.yaml | Removes removed feature gates from experimental values. |
| hack/tools/k8smaintainer/README.md | Removes docs for deleted k8s pinning tool. |
| hack/demo/synthetic-user-cluster-admin-demo-script.sh | Removes synthetic-user demo script tied to deleted features. |
| hack/demo/resources/synthetic-user-perms/cegroup-admin-binding.yaml | Removes synthetic-user demo RBAC. |
| hack/demo/resources/synthetic-user-perms/argocd-clusterextension.yaml | Removes synthetic-user demo ClusterExtension example. |
| go.sum | Removes checksums tied to dropped dependencies (notably k8s.io/kubernetes). |
| go.mod | Drops k8s.io/kubernetes and replace directives; adjusts dependency direct/indirect classification. |
| docs/draft/howto/use-synthetic-permissions.md | Removes draft docs for synthetic permissions feature. |
| docs/api-reference/olmv1-api-reference.md | Updates generated API reference to reflect deprecation + optional serviceAccount. |
| cmd/operator-controller/main.go | Removes SA/synthetic/preflight wiring; introduces global tracking cache and raw watch source; binds to cluster-admin via charts/manifests. |
| applyconfigurations/api/v1/serviceaccountreference.go | Updates generated apply config docs to mark deprecated type/field. |
| applyconfigurations/api/v1/clusterextensionspec.go | Updates generated apply config docs to mark deprecated field and remove SA semantics. |
| api/v1/validation_test.go | Updates validation test defaults to no longer require serviceAccount. |
| api/v1/clusterextension_types.go | Deprecates spec.serviceAccount, switches it to optional omitzero, and relaxes name validation to allow empty. |
| .claude/TODO-OCPSTRAT-3040.md | Adds internal tracking checklist for the epic work items. |
| .claude/memory/update-ocpstrat-3040-todo.md | Adds a memory instruction to keep the TODO checklist updated. |
| .claude/memory/MEMORY.md | Adds index entry for the new memory note. |
Files not reviewed (2)
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2770 +/- ##
==========================================
+ Coverage 70.42% 71.60% +1.17%
==========================================
Files 143 132 -11
Lines 10625 9403 -1222
==========================================
- Hits 7483 6733 -750
+ Misses 2580 2180 -400
+ Partials 562 490 -72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 102 out of 106 changed files in this pull request and generated 2 comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 103 out of 107 changed files in this pull request and generated 2 comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 103 out of 107 changed files in this pull request and generated 2 comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
This might cause problems in systems that perform upgrades without using our upstream install scripts. Perhaps a safer alternative would be to continue using our existing role, but changing its rules to match what |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 98 out of 102 changed files in this pull request and generated 2 comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
| if h.TrackingCache == nil { | ||
| return true, "", fmt.Errorf("TrackingCache not initialized, cannot set up drift detection watches") | ||
| } | ||
|
|
||
| if err := cache.Watch(ctx, h.Watcher, relObjects...); err != nil { | ||
| klog.FromContext(ctx).Info("watching managed objects") | ||
| gvks := gvksForObjects(relObjects) |
| k8s.io/client-go v1.5.2 | ||
| k8s.io/component-base v0.36.2 | ||
| k8s.io/klog/v2 v2.140.0 | ||
| k8s.io/kubernetes v1.36.1 | ||
| k8s.io/utils v0.0.0-20260319190234-28399d86e0b5 | ||
| pkg.package-operator.run/boxcutter v0.14.0 |
| ### Secure access to ClusterExtensions through API access RBAC and ValidatingAdmissionPolicy | ||
|
|
||
| OLMv0 runs as cluster-admin, which is a security concern. OLMv0 has optional security controls for operator installations via the OperatorGroup, which allows a user with permission to create or update them to also set a ServiceAccount that will be used for authorization purposes on operator installations and upgrades in that namespace. If a ServiceAccount is not explicitly specified, OLM’s cluster-admin credentials are used. Another avenue that cluster administrators have is to lock down permissions and usage of the CatalogSource API, disable default catalogs, and provide tenants with custom vetted catalogs. However if a cluster admin is not aware of these options, the default configuration of a cluster results in users with permission to create a Subscription in namespaces that contain an OperatorGroup effectively have cluster-admin, because OLMv0 has unlimited permissions to install any bundle available in the default catalogs and the default community catalog is not vetted for limited RBAC. Because OLMv0 is used to install more RBAC and run arbitrary workloads, there are numerous potential vectors that attackers could exploit. While there are no known exploits and there has not been any specific concern reported from customers, we believe CNCF’s reputation rest on secure cloud-native software and that this is a non-negotiable area to improve. | ||
| Initially, OLMv1 was designed as secure-by-default and required cluster administrators to create ServiceAccounts and associated RBAC to enable the installation of ClusterExtensions. However, any users granted permission to create ClusterExtension resources are effectively being delegated cluster-admin trust. Requiring administrators to create least-privileged RBAC for ClusterExtension installation does not remove this avenue of privilege escalation. Thus, OLMv1 adopts cluster-admin scope directly to eliminate the complexity and UX burden of requiring users to manage ServiceAccounts with difficult-to-determine permissions, and enables simpler internal operations for namespace management and content lifecycle. |
There was a problem hiding this comment.
Maybe this is okay? But I also wonder if we should just use present tense here and just explain what the current/new security model is without referencing the past.
Maybe @michaelryanpeter would have some guidance for us here.
There was a problem hiding this comment.
Happy to rewrite this in present tense — waiting for @michaelryanpeter's guidance on the preferred framing before making the change.
There was a problem hiding this comment.
I agree with emphasizing the present.
I like going a bit more aggressive with the reframing. WDYT of the following:
- Describe the present state of OLM v1 and the reasons behind the decisions without getting into v0 and the previous iterations of v1
- Add an admonition (maybe an INFO block?) that briefly describes the previous v1 stance.
- (optional) add an additional resources section that could include the following links:
- GH issues that explains the change
- OLM v0 documentation that describes how it works
- upstream Slack if people have questions
If questions come up in terms of the change, we could always add more documentation or add links to any ADR-like issues/docs.
If this sounds like a good path forward, I would be happy to help shape the text.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 103 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 103 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
| Initially, OLMv1 was designed as secure-by-default and required cluster administrators to create ServiceAccounts and associated RBAC to enable the installation of ClusterExtensions. However, any users granted permission to create ClusterExtension resources are effectively being delegated cluster-admin trust. Requiring administrators to create least-privileged RBAC for ClusterExtension installation does not remove this avenue of privilege escalation. Thus, OLMv1 adopts cluster-admin scope directly to eliminate the complexity and UX burden of requiring users to manage ServiceAccounts with difficult-to-determine permissions, and enables simpler internal operations for namespace management and content lifecycle. | ||
|
|
||
| To make OLM secure by default: | ||
| Users who previously set `spec.serviceAccount` in their `ClusterExtension` manifests should remove that field. It is retained in the API for backwards compatibility — the API server will still accept it — but its value is silently ignored and will be removed in a future release. Because the operator-controller now always uses its own cluster-admin service account for all operations, cluster administrators should treat the ability to create or modify `ClusterExtension` resources as equivalent to granting cluster-admin privileges, and guard that access accordingly. To restrict which users may trigger installations, use Kubernetes RBAC on the `ClusterExtension` API; for finer-grained constraints such as limiting which packages, catalogs, namespaces, or upgrade policies a user may specify, pair RBAC with a `ValidatingAdmissionPolicy`. See [Protecting OLMv1 API Access](../howto/how-to-protect-olmv1-api-access.md) for concrete examples of both approaches. |
There was a problem hiding this comment.
| Initially, OLMv1 was designed as secure-by-default and required cluster administrators to create ServiceAccounts and associated RBAC to enable the installation of ClusterExtensions. However, any users granted permission to create ClusterExtension resources are effectively being delegated cluster-admin trust. Requiring administrators to create least-privileged RBAC for ClusterExtension installation does not remove this avenue of privilege escalation. Thus, OLMv1 adopts cluster-admin scope directly to eliminate the complexity and UX burden of requiring users to manage ServiceAccounts with difficult-to-determine permissions, and enables simpler internal operations for namespace management and content lifecycle. | |
| To make OLM secure by default: | |
| Users who previously set `spec.serviceAccount` in their `ClusterExtension` manifests should remove that field. It is retained in the API for backwards compatibility — the API server will still accept it — but its value is silently ignored and will be removed in a future release. Because the operator-controller now always uses its own cluster-admin service account for all operations, cluster administrators should treat the ability to create or modify `ClusterExtension` resources as equivalent to granting cluster-admin privileges, and guard that access accordingly. To restrict which users may trigger installations, use Kubernetes RBAC on the `ClusterExtension` API; for finer-grained constraints such as limiting which packages, catalogs, namespaces, or upgrade policies a user may specify, pair RBAC with a `ValidatingAdmissionPolicy`. See [Protecting OLMv1 API Access](../howto/how-to-protect-olmv1-api-access.md) for concrete examples of both approaches. | |
| OLMv1 adopts cluster-admin scope directly. Previously, OLM v1 required cluster administrators to create ServiceAccounts and associated RBAC to enable the installation of ClusterExtensions. The current approach provides the following benefits: | |
| - Eliminates the complexity of difficult-to-determine permissions | |
| - Removes the UX burden of requiring users to manage ServiceAccounts | |
| - Enables simpler internal operations for namespace management and content lifecycle | |
| NOTE: | |
| The `spec.serviceAccount` field is depricated and will be removed in a future release. The field is retained in the API for backwards compatibility . The API server still accepts the field, but its value is silently ignored. | |
| For more information, see [Protecting OLMv1 API Access](../howto/how-to-protect-olmv1-api-access.md). |
That might be a little too aggressive, but feel free to take what is useful and disregard what is not.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 103 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
| if h.TrackingCache == nil { | ||
| return true, "", fmt.Errorf("TrackingCache not initialized, cannot set up drift detection watches") | ||
| } | ||
|
|
||
| if err := cache.Watch(ctx, h.Watcher, relObjects...); err != nil { | ||
| klog.FromContext(ctx).Info("watching managed objects") | ||
| gvks := gvksForObjects(relObjects) | ||
| if err := h.TrackingCache.Watch(ctx, ext, gvks); err != nil { | ||
| return true, "", err | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 104 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
| // +kubebuilder:validation:XValidation:rule="self == oldSelf || size(self) == 0",message="name is immutable once set but may be cleared" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="name must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters" | ||
| // +required | ||
| Name string `json:"name"` | ||
| // +optional |
There was a problem hiding this comment.
The enhancement proposal covers why we're not allowing "" - see here. The only modification that can be made to this field is completely removing spec.serviceAccount.
Mark spec.serviceAccount as deprecated in the ClusterExtension API. Remove PreAuthorizer integration, SA-scoped RBAC templates, and restconfig action. Simplify the controller to use cluster-admin scope directly. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
…rning Create VAP and VAPB resources that emit a warning when spec.serviceAccount is set on a ClusterExtension. Add integration test infrastructure for warning collection. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
Remove SA-specific e2e test scenarios, SA RBAC templates, and the extension developer test suite. Simplify test steps to use cluster-admin scope. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
Delete authentication and authorization packages, feature gates, k8smaintainer tool, SA demo scripts, and documentation. Remove k8s.io/kubernetes dependency and unnecessary k8s staging replace directives. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
Delete the contentmanager package and replace it with boxcutter's shared TrackingCache. Simplify revision engine factory to use the base config directly. Remove SA label constants. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
Remove SA-related documentation, tutorials, and demo scripts. Clean up remaining ServiceAccount references from unit tests. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
…tect OLMv1 API access. Signed-off-by: Daniel Franz <dfranz@redhat.com> Co-Authored-By: Predrag Knezevic <pknezevi@redhat.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 104 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
| for _, kind := range []string{"ValidatingAdmissionPolicy", "ValidatingAdmissionPolicyBinding"} { | ||
| objs, err := test.LoadManifests(kind) | ||
| utilruntime.Must(err) | ||
| for _, obj := range objs { | ||
| utilruntime.Must(cl.Create(ctx, obj)) |
| // +kubebuilder:validation:MinLength:=1 | ||
| // +kubebuilder:validation:MaxLength:=253 | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable" | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable once set but may be cleared" |
There was a problem hiding this comment.
I'm noticing that once I clear the field, I'm able to set a serviceAccount.name on it. I'm guessing self == oldSelf is not catching that case because it only fires when there is an oldSelf to compare against.
There was a problem hiding this comment.
I realized the absent -> set case may be impossible to block since we need to keep allowing the initial create to set a service account. CEL cannot distinguish between these:
- initial create with
.spec.serviceAccount.nameset - update from absent
.spec.serviceAccountto.spec.serviceAccount.nameset`
There was a problem hiding this comment.
Couldn't we do this with another VAP? We could remove UPDATE from the current VAP's operations and add another VAP for it which gives errors instead of warnings.
| ### Secure access to ClusterExtensions through API access RBAC and ValidatingAdmissionPolicy | ||
|
|
||
| OLMv0 runs as cluster-admin, which is a security concern. OLMv0 has optional security controls for operator installations via the OperatorGroup, which allows a user with permission to create or update them to also set a ServiceAccount that will be used for authorization purposes on operator installations and upgrades in that namespace. If a ServiceAccount is not explicitly specified, OLM’s cluster-admin credentials are used. Another avenue that cluster administrators have is to lock down permissions and usage of the CatalogSource API, disable default catalogs, and provide tenants with custom vetted catalogs. However if a cluster admin is not aware of these options, the default configuration of a cluster results in users with permission to create a Subscription in namespaces that contain an OperatorGroup effectively have cluster-admin, because OLMv0 has unlimited permissions to install any bundle available in the default catalogs and the default community catalog is not vetted for limited RBAC. Because OLMv0 is used to install more RBAC and run arbitrary workloads, there are numerous potential vectors that attackers could exploit. While there are no known exploits and there has not been any specific concern reported from customers, we believe CNCF’s reputation rest on secure cloud-native software and that this is a non-negotiable area to improve. | ||
| OLMv1 adopts cluster-admin scope directly. Previously, OLMv1 required cluster administrators to create ServiceAccounts and associated RBAC to enable the installation of ClusterExtensions. The current approach provides the following benefits: |
There was a problem hiding this comment.
I would suggest moving the Previously... sentence into the field deprecation note.
And then I think the next sentence and bullet points are still sort of comparing/contrasting with the previous logic, which again I think is only relevant in the deprecation note (if at all).
I'll try to wordsmith more and use only present tense descriptions and motivations.
Also updates go.mod/go.sum to fix k8s.io/client-go version.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 104 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
|
|
||
| replace k8s.io/api => k8s.io/api v0.36.1 | ||
|
|
||
| replace k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.36.1 | ||
|
|
||
| replace k8s.io/apimachinery => k8s.io/apimachinery v0.36.1 | ||
|
|
||
| replace k8s.io/apiserver => k8s.io/apiserver v0.36.1 | ||
|
|
||
| replace k8s.io/cli-runtime => k8s.io/cli-runtime v0.36.1 | ||
|
|
||
| replace k8s.io/client-go => k8s.io/client-go v0.36.1 | ||
|
|
||
| replace k8s.io/component-base => k8s.io/component-base v0.36.1 | ||
|
|
||
| replace k8s.io/component-helpers => k8s.io/component-helpers v0.36.1 | ||
|
|
||
| replace k8s.io/controller-manager => k8s.io/controller-manager v0.36.1 | ||
|
|
||
| replace k8s.io/kubectl => k8s.io/kubectl v0.36.1 | ||
|
|
||
| replace k8s.io/streaming => k8s.io/streaming v0.36.1 |
There was a problem hiding this comment.
Are these replaces no longer needed?
There was a problem hiding this comment.
Yes - this copilot comment highlighted it as well.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
PR needs rebase. DetailsInstructions 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. |
perdasilva
left a comment
perdasilva
left a comment
There was a problem hiding this comment.
Finding — semantic mismatch on finalizer constant (not inline-commentable; line 60 is outside the diff hunks):
internal/operator-controller/controllers/clusterextension_controller.go:60
The constant ClusterExtensionCleanupContentManagerCacheFinalizer still holds the string "olm.operatorframework.io/cleanup-contentmanager-cache", but the finalizer handler now calls trackingCache.Free (Helm path) or is a no-op (Boxcutter path). Neither path involves a content manager.
This string is baked into etcd on all existing ClusterExtension objects. Operators debugging stuck finalizers and future maintainers will search for content-manager cleanup logic that no longer exists. Consider renaming the constant and updating the string (with a migration step for existing objects), or adding a comment explaining the historical name.
| klog.FromContext(ctx).Info("watching managed objects") | ||
| gvks := gvksForObjects(relObjects) | ||
| if err := h.TrackingCache.Watch(ctx, ext, gvks); err != nil { | ||
| return true, "", err |
There was a problem hiding this comment.
Bug: When TrackingCache.Watch() fails, Apply() returns (true, "", err). The caller in clusterextension_reconcile_steps.go interprets rolloutSucceeded=true → sets InstalledCondition=true, then interprets err != nil → sets ProgressingCondition=error and requeues. The extension ends up in a permanently contradictory state (Installed=true + Progressing=error) with continuous retries.
If a Watch failure should block the install, return (false, "", err) so the reconciler retries cleanly without setting the Installed condition. If drift detection is non-fatal, log the error and return (true, "", nil) instead.
| func gvksForObjects(objs []client.Object) sets.Set[schema.GroupVersionKind] { | ||
| gvks := sets.New[schema.GroupVersionKind]() | ||
| for _, obj := range objs { | ||
| gvks.Insert(obj.GetObjectKind().GroupVersionKind()) |
There was a problem hiding this comment.
Potential bug: GetObjectKind().GroupVersionKind() returns a zero-value GroupVersionKind{} for Unstructured objects whose apiVersion/kind fields were not set (e.g. a malformed Helm template that renders a YAML document without those fields). There is no guard here against inserting an empty GVK into the set.
A zero-value GVK passed to TrackingCache.Watch causes ensureCacheSyncForGVK to fail REST mapping and return an error, which propagates through Apply() as (true, "", err) — triggering the contradictory Installed+Error state described in the line 159 comment.
Fix: skip zero-value GVKs before inserting, or return an error so callers can surface malformed manifests explicitly:
if gvk := obj.GetObjectKind().GroupVersionKind(); !gvk.Empty() {
gvks.Insert(gvk)
}| func ServiceAccountValidator(saClient corev1client.ServiceAccountsGetter) ClusterExtensionValidator { | ||
| // ServiceAccountDeprecationWarning returns a validator that checks if the deprecated | ||
| // serviceAccount field is populated and logs a warning if found. | ||
| func ServiceAccountDeprecationWarning() ClusterExtensionValidator { |
There was a problem hiding this comment.
Bug: The old ServiceAccountValidator returned an error when the referenced service account did not exist, surfacing a status condition to users. ServiceAccountDeprecationWarning() always returns nil — it only emits an l.Info() log entry.
A user who sets spec.serviceAccount.name=nonexistent-sa will see no error and no status condition; the extension installs with full cluster-admin privileges, silently ignoring the field.
At minimum the validator should emit a Kubernetes Warning response header (displayed by kubectl automatically) or set a status condition, rather than logging only at Info level.
| } | ||
|
|
||
| scopedClient, err := f.createScopedClient(saNamespace, saName) | ||
| c, err := client.New(f.BaseConfig, client.Options{ |
There was a problem hiding this comment.
Performance: client.New(f.BaseConfig, ...) is called inside CreateRevisionEngine, which is invoked once per ClusterObjectSet reconcile. This allocates a fresh non-caching REST client with its own connection pool and TLS stack on every reconcile cycle.
The old per-SA design justified this cost for identity isolation. Now that all engines share the same BaseConfig, the client can be built once.
Fix: call client.New in NewDefaultRevisionEngineFactory, store the result in the factory struct, and reference the pre-built client in CreateRevisionEngine.
| return fmt.Errorf("failed to apply resource as %s/%s: %v; stderr: %s", user, group, out, stdErr) | ||
| } | ||
| sc.lastApplyStderr = stdErr | ||
| if res.GetKind() == "ClusterExtension" { |
There was a problem hiding this comment.
Bug: ResourceIsAppliedAsUser only tracks ClusterExtension resources in sc.addedResources. Compare with ResourceIsApplied (~line 617), which also handles ClusterObjectSet (setting sc.clusterObjectSetName) and a generic else branch for all other resource kinds with their namespace.
Any non-ClusterExtension resource applied via impersonation (e.g. a Namespace, ClusterRole, or other type used in the VAP access-control tests) will not be registered for cleanup, leaking it into subsequent test scenarios.
| } | ||
| return strings.Contains(stdErr, errMsg) | ||
| }) | ||
| _, stdErr, err := k8scliWithInput(ctx, yamlContent, "apply", "-f", "-", "--as="+user, "--as-group="+group) |
There was a problem hiding this comment.
Bug: After the dry-run polling confirms the request is denied, this line performs a real kubectl apply (no --dry-run). If the VAP is momentarily inactive at write time (timing race between the dry-run poll succeeding and the real write), the apply succeeds and creates the resource. The function then returns an error ("expected apply to fail"), but the created resource is never added to sc.addedResources, so end-of-scenario cleanup does not delete it.
This can corrupt subsequent test scenarios if the leaked resource conflicts with setup. Consider tracking the resource before returning the error, or adding a deferred cleanup.
| func ValidatingAdmissionPolicyIsActive(ctx context.Context, name string) error { | ||
| sc := scenarioCtx(ctx) | ||
| name = substituteScenarioVars(name, sc) | ||
| waitFor(ctx, func() bool { |
There was a problem hiding this comment.
Bug: The return value of waitFor() is discarded. If the named ValidatingAdmissionPolicy never reaches observedGeneration == generation (e.g. wrong name, missing binding, or an old API server), waitFor polls until timeout — but the timeout/error result is silently dropped and the function returns nil.
All downstream steps that assume the VAP is enforcing access control then pass even though no admission is taking place, producing a false-green result.
Fix: capture and propagate the error:
if err := waitFor(ctx, func() bool { ... }); err != nil {
return fmt.Errorf("ValidatingAdmissionPolicy %q did not become active: %w", name, err)
}
Description
Deprecate
spec.serviceAccounton ClusterExtension. OLMv1 is a single-tenant system where users with ClusterExtension write access are effectively delegated cluster-admin trust. With the service account field deprecated, operator-controller uses its own cluster-admin service account for all cluster interactions, eliminating the per-SA permission model and ~5,700 lines of supporting infrastructure.Changes
API deprecation:
spec.serviceAccountas deprecated withDeprecated:godoc convention andomitzerotagserviceAccountis setserviceAccount.nameimmutability rule to allow clearing the deprecated fieldRemove per-SA infrastructure:
authorization/package,authentication/package, SA-scoped config mappersk8s.io/kubernetesdependency and transitive replace directives from go.modhack/tools/k8smaintainer/andk8s-pinMakefile targetPreflightPermissionsandSyntheticPermissionsfeature gatesSimplify cache architecture:
contentmanager/package with sharedTrackingCachefrom boxcutter/managedcacheRestConfigMapperpatternE2E test cleanup:
serviceAccountfrom all feature files, add explicit namespace setup stepUpgrade safety:
Removed ~5,700 lines of unneeded code.
Reviewer Checklist