✨ Deprecate ClusterExtension spec.serviceAccount#2770
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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 66.76% 67.41% +0.64%
==========================================
Files 149 138 -11
Lines 11386 10164 -1222
==========================================
- Hits 7602 6852 -750
+ Misses 3223 2823 -400
+ Partials 561 489 -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:
|
| However, it should be noted that the purpose of these primitives is not to enable multi-tenancy. It is to enable administrators to provide configuration for the installation of an extension. The fact that operators can be packaged as separate bundles and parameterized in a way that permits multiple controller installations is incidental, and not something that OLM v1 will encourage or promote. | ||
|
|
||
| ### Make OLM secure by default | ||
| ### Make OLM secure by safeguarding ClusterExtension permissions |
There was a problem hiding this comment.
Wanted to specifically draw attention to the rewrite here, if anybody can think of a better sub-title and wording in the below paragraph.
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
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: operator-controller-manager-role | ||
| name: cluster-admin | ||
| subjects: |
There was a problem hiding this comment.
Fixed in 4fdb3a4 — added kubectl delete clusterrolebinding operator-controller-manager-rolebinding --ignore-not-found before kubectl apply in scripts/install.tpl.sh. Verified with make test-upgrade-st2st-e2e.
| @@ -0,0 +1,96 @@ | |||
| # OCPSTRAT-3040: Adopt cluster-admin scope and deprecate ServiceAccount | |||
There was a problem hiding this comment.
note to reviewers: this file is going to removed from the PR before the merge. It was used to track the work done, given that the work items were split between myself and @dtfranz .
| @@ -0,0 +1 @@ | |||
| - [Update OCPSTRAT-3040 TODO](update-ocpstrat-3040-todo.md) — Check off items in `.claude/TODO-OCPSTRAT-3040.md` whenever related work is completed | |||
There was a problem hiding this comment.
note to reviewers: this file is going to removed from the PR before the merge. It was used to track the work done, given that the work items were split between myself and @dtfranz .
| @@ -0,0 +1,12 @@ | |||
| --- | |||
There was a problem hiding this comment.
note to reviewers: this file is going to removed from the PR before the merge. It was used to track the work done, given that the work items were split between myself and @dtfranz .
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
| # Delete the old ClusterRoleBinding if it exists — Kubernetes does not allow | ||
| # changing roleRef via kubectl apply, so we must delete before re-creating. | ||
| kubectl delete clusterrolebinding operator-controller-manager-rolebinding --ignore-not-found |
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 |
…rning Stream 1 (OPRUN-4630) - kubectl deprecation warning: - Add ValidatingAdmissionPolicy + Binding that emits a warning when the deprecated spec.serviceAccount field is set on ClusterExtension - Users now see a deprecation warning in kubectl output on create/update - Adapt TestClusterExtensionAdmissionServiceAccount to verify warnings using a warning-capturing client with rest.WarningHandler - Remove Test_SyntheticUserRestConfigMapper_UsesDefaultConfigMapper (tested SA-based branching that was removed in prior commit) - Regenerate manifests Signed-off-by: Predrag Knezevic <pknezevi@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
…rning scenario Stream 4 (OPRUN-4630, OPRUN-4631, OPRUN-4632) - e2e test cleanup: - Remove spec.serviceAccount blocks from all ClusterExtension YAML in feature files (install, update, recover, proxy, status, uninstall, user-managed-fields) - Remove ServiceAccount setup steps (olm-sa, olm-admin) from all feature files - Remove SA annotations from ClusterObjectSet resources in revision.feature - Remove uninstall scenario that tested SA-scoped cleanup resilience - Remove PreflightPermissions and SyntheticPermissions from e2e feature gate defaults - Remove ServiceAccountWithoutCreatePermissions step definition and isFeatureGateEnabled helper (dead code) - Modify k8scliWithInput to capture stderr separately for warning assertion - Add ResourceApplyEmitsWarning step and deprecation warning scenario Signed-off-by: Predrag Knezevic <pknezevi@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
- Remove serviceAccount blocks and SA setup steps from all feature files - Replace SA setup with explicit namespace step in Background sections (install, update, proxy, status, uninstall, user-managed-fields) or per-scenario (recover - recovery scenario delays namespace creation) - Add NamespaceIsAvailable step using kubectl apply with namespace template - Add ResourceApplyEmitsWarning step for deprecation warning assertion - Add deprecation warning e2e scenario to install.feature - Restore 4 scenarios incorrectly removed in prior commit: watchNamespace own namespace, webhook install, DNS validation, reject unsupported - Remove SA annotations from COS resources in revision.feature - Remove SA-specific uninstall and recover scenarios - Remove dead PreflightPermissions/SyntheticPermissions e2e code - Remove unused olm-sa RBAC templates - Capture kubectl stderr in k8scliWithInput for warning assertions Signed-off-by: Predrag Knezevic <pknezevi@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Predrag Knezevic <pknezevi@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
…smaintainer Remove the RBAC pre-authorization package which was wired but never activated (PreAuthorizer fields declared in applier structs were never called). This eliminates ~1,600 lines of dead code and the heavy k8s.io/kubernetes transitive dependency. - Delete internal/operator-controller/authorization/ (rbac.go, rbac_test.go) - Remove PreAuthorizer fields from Helm and Boxcutter applier structs - Remove authorization wiring and PreflightPermissions gate checks from main.go - Remove k8s.io/kubernetes from go.mod require and 21 replace directives - Delete hack/tools/k8smaintainer/ (version pinning tool for k8s staging modules) - Remove k8s-pin Makefile target, update verify to use tidy instead OPRUN-4631 Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
… comments The PreflightPermissions feature gate is now completely orphaned after the authorization package removal in the previous commit. Remove the gate constant and spec definition, and clean up stale pre-auth comments in the Boxcutter applier. RBAC watches in main.go are retained as they serve Boxcutter's phase ordering for bundle RBAC resources. OPRUN-4631 Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…ions feature gates and residuals. Signed-off-by: Daniel Franz <dfranz@redhat.com>
Replace ClusterAdminRestConfigMapper with an anonymous closure in main.go, eliminating the now-unnecessary action/restconfig.go file. Delete obsolete synthetic-permissions howto documentation. OPRUN-4630, OPRUN-4631, OPRUN-4632 Co-Authored-By: Claude <noreply@anthropic.com>
Replace the per-ClusterExtension contentmanager cache with a single shared TrackingCache from boxcutter/managedcache. The contentmanager created per-CE dynamic clients and informer factories for drift detection; now that all operations use the same controller identity, a single global cache is simpler and more efficient. - Create TrackingCache at shared scope before controller setup - Wire TrackingCache.Source to CE controller via WithWatchesRawSource - Replace Manager+Watcher fields in Helm applier with trackingCache interface (Watch + Free) - Suppress Create events on the source predicate to match old behavior - Update finalizer to call TrackingCache.Free instead of cm.Delete - Remove ClientRestConfigMapper from Helm action config getter - Update helm_test.go mocks for new interface - Delete contentmanager/ package (~1,240 lines) OPRUN-4633 Co-Authored-By: Claude <noreply@anthropic.com>
- Remove unused saClient parameter from ServiceAccountDeprecationWarning - Relax serviceAccount.name immutability rule to allow clearing the deprecated field (self == oldSelf || size(self) == 0) - Delete old ClusterRoleBinding before kubectl apply during upgrade — Kubernetes does not allow changing roleRef on existing CRBs Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Daniel Franz <dfranz@redhat.com>
Signed-off-by: Daniel Franz <dfranz@redhat.com>
Return an error if TrackingCache is nil in Apply() instead of panicking on nil dereference. The reconcileExistingRelease path already had a nil guard with graceful degradation; Apply() should fail explicitly since drift detection is required on the primary install/upgrade path. Co-Authored-By: Claude <noreply@anthropic.com>
Restructure serviceAccount and Name field godoc to start with the JSON field name (lowercase) per OpenShift API conventions, with DEPRECATED marker in the body. Suppress optionalfields lint for deprecated fields where making them pointers would be a breaking API change. Co-Authored-By: Claude <noreply@anthropic.com>
Only register the TrackingCache WatchesRawSource with
EnqueueRequestForOwner(&ClusterExtension{}) when Boxcutter runtime
is disabled (Helm path). In Boxcutter mode, managed objects are
owned by ClusterObjectSets, and the COS controller registers its
own Source separately.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 103 out of 107 changed files in this pull request and generated 12 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
| // serviceAccount was previously used to specify the service account for | ||
| // extension management. | ||
| // DEPRECATED: serviceAccount is no longer used and will be removed in a future release. | ||
| // OLMv1 is a single-tenant system where users with ClusterExtension write access are | ||
| // effectively delegated cluster-admin trust. The operator-controller runs with | ||
| // cluster-admin privileges and uses its own service account for all cluster interactions. |
| // ServiceAccountReference was previously used to reference a service account for | ||
| // extension management. | ||
| // DEPRECATED: ServiceAccountReference is no longer used and will be removed in a future release. |
| // name was previously a required, immutable reference to the name of the | ||
| // ServiceAccount used for installation and management of the content for | ||
| // the package specified in the packageName field. | ||
| // DEPRECATED: name is no longer used and will be removed in a future release. |
Update generated files after godoc restructuring to follow OpenShift deprecated field conventions. Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Daniel Franz <dfranz@redhat.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 103 out of 107 changed files in this pull request and generated 8 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
| // serviceAccount was previously used to specify the service account for | ||
| // extension management. | ||
| // DEPRECATED: serviceAccount is no longer used and will be removed in a future release. | ||
| // OLMv1 is a single-tenant system where users with ClusterExtension write access are | ||
| // effectively delegated cluster-admin trust. The operator-controller runs with |
| // ServiceAccountReference was previously used to reference a service account for | ||
| // extension management. | ||
| // DEPRECATED: ServiceAccountReference is no longer used and will be removed in a future release. | ||
| type ServiceAccountReference struct { |
| // name was previously a required, immutable reference to the name of the | ||
| // ServiceAccount used for installation and management of the content for | ||
| // the package specified in the packageName field. | ||
| // DEPRECATED: name is no longer used and will be removed in a future release. | ||
| // |
| if _, _, err := k8scliWithInput(yaml, "apply", "-f", "-"); err != nil { | ||
| return fmt.Errorf("failed to apply namespace %s: %w", ns, err) | ||
| } |
Signed-off-by: Daniel Franz <dfranz@redhat.com>
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