OPRUN-4675: Enhancement proposal for serviceAccount field deprecation#2054
OPRUN-4675: Enhancement proposal for serviceAccount field deprecation#2054pedjak wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pedjak: This pull request references OPRUN-4675 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[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 |
|
/jira refresh |
|
@pedjak: This pull request references OPRUN-4675 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
Deprecate the spec.serviceAccount field from the ClusterExtension API and grant the operator-controller cluster-admin privileges. The field was introduced for per-extension least-privilege but provides a false security boundary in OLMv1's single-tenant model while imposing significant UX complexity. Includes telemetry-backed adoption analysis (59 clusters / 80 CEs out of ~199k reporting, ~6 confirmed production), deprecation strategy via ValidatingAdmissionPolicy warnings, downstream tracking through ClusterOperator conditions, and replacement access control model using RBAC and ValidatingAdmissionPolicy for delegation. Co-Authored-By: Claude <noreply@anthropic.com>
pedjak
force-pushed
the
olm-sa-deprecation
branch
from
1efad96 to
0b52ae5
Compare
|
/cc @JoelSpeed |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Had a brief chat with @dmesser about this EP while reviewing. What I still see as missing is how does a cluster admin restrict not only who can create a CE, but what that CE can target.
What if we consider the following user story?
- As a cluster admin, I want specific cluster users to be able to install only certain operator at certain versions, so that I can vet all CEs being installed in the cluster and approve them before installation
The EP is giving OLM cluster admin. That means that today anyone with CE create permissions can install anything. With the previous method, they could only install an extension if the extension needed a subset of permissions from service accounts available to them.
If we could formulate some documentation that would allow a cluster admin to create a VAP with an allowlist of CE/version combinations (perhaps even with specific users or groups being able to install these) then that can give cluster admins fine grained control over what's being installed into their cluster, without the need to manage fine grained RBAC. They can vet the CE RBAC at a fine grained level should they be concerned about what the CEs are doing, and apply the relevant restrictions as necessary.
I think that covers us in terms of relaxing the security posture here, while giving security concious admins a way to still be fairly restrictive about what's going into their clusters
| - **Extension content is inherently cluster-scoped.** Operators install CRDs, ClusterRoles, | ||
| webhooks, and other cluster-scoped resources. True per-extension least-privilege is impossible | ||
| without restricting what extensions can install, which would break most operators. |
There was a problem hiding this comment.
Can you expand on this? What kind of restrictions are you implying?
| - **Internal implementation details leak into required permissions.** The controller needs | ||
| informers for managed resources, which requires list/watch permissions that users must include | ||
| in the ServiceAccount's RBAC — but these are implementation details that change when internals | ||
| change, creating fragility. |
There was a problem hiding this comment.
Do we have examples of when/how often this type of change occurs?
There was a problem hiding this comment.
The required SA permissions depend on the controller's internal cache architecture, not just on the bundle content. A concrete example is PR #2539: the Helm applier's ContentManager requires list/watch on cluster-scoped resources for its informer caches, while the boxcutter applier does not. The e2e test suite maintains two separate RBAC templates (rbac-template.yaml vs boxcutter-rbac-template.yaml) because the required SA permissions differ depending on which runtime is active.
This means users would need to update their ServiceAccount RBAC whenever the controller's cache architecture changes, even when the bundle content they're installing is identical. Updated the EP text with this example.
| - Preserve backward compatibility — existing ClusterExtensions with `spec.serviceAccount` set | ||
| continue to function after upgrade without any user action. |
There was a problem hiding this comment.
Even though the field is deprecated, should it continue to use the derived service account so that those who have set this up continue to have the same experience. Make it their choice when to remove the serviceAccount field and switch to cluster admin?
| 59 clusters have any `ClusterExtension` resources, with a total of 80 ClusterExtensions | ||
| across the fleet. Of those 59 clusters, 17 are registered as production in OpenShift | ||
| Cluster Manager, and only 6 can be confirmed as genuine customer production environments | ||
| (across 5 organizations, totaling 11 ClusterExtensions). |
There was a problem hiding this comment.
This seems like a small enough number that we could talk to them about their usage and see if they would be for or against a change like this, and possibly also help them move so that we could accelerate a deprecation/removal cycle
There was a problem hiding this comment.
Yes, we've already asked PM to reach out to these customers to get their feedback.
| - Console/UI changes to remove the ServiceAccount field from ClusterExtension workflows | ||
| (tracked separately). |
There was a problem hiding this comment.
This feels like a slightly odd distinction to track separately to me, why is this considered separate?
There was a problem hiding this comment.
+1 on including these changes in the scope of this EP. I'm pretty sure the OLMv1 Console UI workflows are still fairly minimal, and I know they are all still behind a TPNU feature gate.
So changes to the UI for this feature should hopefully be straightforward and free of baggage from the past.
There was a problem hiding this comment.
The current OLMv1 Console UI is a generic CRD YAML editor with a schema-driven documentation sidebar — there is no custom ClusterExtension form. The sidebar automatically reflects CRD schema changes, so making serviceAccount optional requires no Console code changes.
Moved Console/UI out of Non-Goals and added a brief note in the API Extensions section stating no Console changes are needed.
| Options 2 and 3 still require the full SA-impersonation infrastructure plus the | ||
| derive-service-account tooling. The privilege escalation issue in the SA reference model | ||
| remains — any ClusterExtension writer can reference any ServiceAccount in any namespace | ||
| they have access to, regardless of which tier is chosen. Building better UX around a | ||
| fundamentally flawed security model does not solve the core problem. |
There was a problem hiding this comment.
This doesn't accurately represent my suggestion by the way. The issues you're documenting here were also considered and a viable path that mitigated these concerns was suggested.
To repeat, we can prevent users from leveraging service accounts for cluster extensions unless they have been given explicit permission to do so. This would involve a VAP checking if a user is authorized to perform a certain verb (we can make up what that verb is) on a service account when the CE is created.
The SA impersonation IIRC was overly complicated in the way OLM implemented it (generating tokens?) and could be massively simplified by using the built-in impersonation within the Kube API
There was a problem hiding this comment.
I recall my concern there being the fact that there are no current VAP/RBAC mechanisms to limit SA use, which would naively mean that we'd break existing client workflows.
An existing client that doesn't have RBAC would start getting admission failures after upgrade and would no longer be able to make any changes until the new RBAC needs were covered.
Perhaps a way to overcome that would be:
- A VAP that checks for either of:
- Desired path: User has certain verb associated somehow associated with the referenced SA.
- OR, Grandfathered-path: SA has a label like "legacy-allowed-to-referenced-by-cluster-extension"
- A job that runs on upgrade that sets that grandfathered label on all existing CE-referenced SAs
- A prometheus metric and alert, that warns cluster admins about the presence of SAs with "legacy-allowed-to-referenced-by-cluster-extension" labels.
There was a problem hiding this comment.
I recall my concern there being the fact that there are no current VAP/RBAC mechanisms to limit SA use, which would naively mean that we'd break existing client workflows.
This does resonate, i added this comment only because I don't think the alternative was represented correctly. Capturing this as a reason for not taking that path would be a stronger argument here and mean that anyone else who comes up with a similar alternative might have a better understanding of why this option was dismissed
An existing client that doesn't have RBAC would start getting admission failures after upgrade and would no longer be able to make any changes until the new RBAC needs were covered.
I don't think this was an issue. I think the suggestion when we last looked at this was to only validate whan the SA reference changed, so on create and then certain updates. Which would mean existing resources would continue to work as expected until someone attempted to change the serviceAccountReference.name to a new value
There was a problem hiding this comment.
Which would mean existing resources would continue to work as expected until someone attempted to change the serviceAccountReference.name to a new value
Ah yes, that would solve that problem well enough I think.
Regardless, +1 on updating this alternative to capture those details. @pedjak let me know if you need any other background on this.
| Auto-generating RBAC requires the tool to have broad read permissions to inspect bundle content | ||
| and map it to policy rules — effectively cluster-admin with extra steps. The |
There was a problem hiding this comment.
Again I don't think this is what @everettraven was talking about. The CLI tool doesn't need cluster admin, it's a CLI tool running on a cluster admins laptop. They generate the rules using the CLI, then use kubectl to apply the restrictive set of rules
| permission set changes on every upgrade. Additionally, client tooling would need to know the | ||
| controller's internal permission requirements (e.g., list/watch for informers), which are | ||
| implementation details that change from release to release. There is also a chicken-and-egg |
There was a problem hiding this comment.
Each bundle has a valid set of RBAC manifests that show what these are. The tool doesn't need to know anything about the implementation, just that it can extract the RBAC and merge it together (which is what is documented in the existing user docs btw)
There was a problem hiding this comment.
In theory, yes. But since the current OLM implementation uses the CE-provided SA for essentially all interactions with the cluster for that CE, the OLM implementation does play a part.
We could change that as well. We could have an OLM SA for the OLM-specific controller needs, but that would ramp up complexity. For example, we'd need up to two apiserver interactions for each managed object:
- Use CE's SA to create/update the object
- Use OLM's SA to attach an ownerRef to the managed object pointing back to parent OLM object (need
<parentObjectResource>/finalizersupdatepermission)
| problem: the ClusterExtension simultaneously performs resolution and applies the bundle, but | ||
| the user cannot know which bundle will be resolved without pinning the version, meaning they | ||
| cannot correctly configure least-privilege permissions. This forces a two-step process where | ||
| the ClusterExtension enters a failure state between steps — a poor UX. |
There was a problem hiding this comment.
Is it common practice not to pin the bundle to a specific version? That definitely feels like something we should advise (just like we recommend pinning workload images?
There was a problem hiding this comment.
OLMv0 does not have a way pin or direct the upgrade process. It is just "Manual" vs "Automatic" upgrade approval with the only version knob being "which version do you want for the initial install?"
So the idea of pinning or specifying an acceptable version range is unique to OLMv1, and so there's not much in the way of recommendation given OLMv0's behavior.
I'd expect lots of users to pin now that the option is available, but I'd also expect a decent amount of minor version pinning as well for users that trust our layered product patch releases. There are docs that describe how to do it, but I don't see any recommendations.
| This addresses the escalation vector but does not resolve the other problems: the UX | ||
| complexity of deriving and maintaining per-extension RBAC remains, the controller's internal |
There was a problem hiding this comment.
Yes, because you've split one solution into two 😅
This suggestion was part of ### Three paths: no SA, namespace admin, full least-privilege. They only work together, and your reasons for discarding them are basically "they don't solve half the probelm", when combined, they do solve the whole problem
|
In addition to @JoelSpeed's points (which I think can be addressed), I wanted to call out the nature of this change. Ignoring the However, given the early-stage adoption OLM v1 and the nature of this change being non-disruptive, I am of the opinion that the downsides outweigh the upside, which is a massive simplification of the install and update UX. These points have been the strategic drivers for the OLM rewrite that resulted in the |
|
I echo @dmesser's sentiment. While I think there likely are ways we could be more careful with this deprecation and change, I think the question is whether the cost of that care is warranted in this case. Ultimately if we can't find any existing users that care about this feature, or if we do but we are able to convince them of the merits of this change and the alternatives they can use, then the more careful alternative would be wasted effort and higher ongoing maintenance costs. Perhaps what would be helpful would be to outline just a single alternative:
And then a cost-benefit analysis of each option (i.e. what is the expected revenue/reputational impact of each option?) |
- Expand Motivation bullets with concrete reasoning: CRD create permissions are all-or-nothing, PR #2539 RBAC template divergence - Add ServiceAccountReference struct to API section - Add concrete VAP example for package allowlisting in delegation model - Merge "Three paths" and "Lock down SA references" into single "Fix the SA model" alternative with accurate representation - Drop x-kubernetes-deprecated issue reference per reviewer guidance - Add Console/UI impact section (generic CRD editor, no changes needed) - Clarify namespace isolation drawback (VAP-based, not watch scoping) - Remove unused reference link Co-Authored-By: Claude <noreply@anthropic.com>
|
@pedjak: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
5 participants
Summary
Enhancement proposal to deprecate the
spec.serviceAccountfield from theClusterExtensionAPI and grant the operator-controller cluster-admin privileges.The field was introduced for per-extension least-privilege but provides a false
security boundary in OLMv1's single-tenant model while imposing significant UX
complexity. Access control shifts to standard Kubernetes RBAC and
ValidatingAdmissionPolicy.
Key points