[OPRUN-4144] OLMv1: Make ServiceAccount Field Optional in the ClusterExtension API #1897
Conversation
Signed-off-by: Rashmi Gottipati <[email protected]>
|
[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 |
|
@rashmigottipati: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Spoken with Jordan sync on this, when reviewing this I don't necessarily have the most up to date "why" and so some comments may be based on an older understanding. Please point me in the right direction if I've missed the "why" here
|
|
||
| One of the core design principles of OLMv1 is Secure By Default. This enhancement aims at upholding and also improving upon that principle while addressing a usability issue. | ||
|
|
||
| The proposal is to make the `spec.serviceAccount` field in the ClusterExtension API optional in Tech Preview. For extensions without a ServiceAccount, OLMv1 uses a synthetic identity with zero permissions and relies on Kubernetes impersonation, allowing administrators to explicitly grant the necessary privileges. For extensions with a ServiceAccount, OLMv1 continues to use token based authentication, preserving backward compatibility. |
There was a problem hiding this comment.
For extensions with a ServiceAccount, OLMv1 continues to use token based authentication, preserving backward compatibility.
Why can you not switch the existing to impersonation? From a permissions perspective OLM would need to be able to impersonate service accounts. Does it not have that permission today?
|
|
||
| Currently, the `spec.serviceAccount` field is required on every `ClusterExtension`, which creates usability and security challenges. | ||
|
|
||
| From a usability standpoint, users must understand the exact permissions their extension requires, create a corresponding ServiceAccount, and configure ClusterRoleBindings appropriately. Ensuring that the service account has the correct permissions is a manual and often complex process, leading to failed installations and a frustrating experience, particularly for new users. User feedback indicates a strong preference to avoid configuring RBAC for each ClusterExtension, with some users resorting to granting cluster admin privileges just to satisfy the requirement. |
There was a problem hiding this comment.
How could you make this process less frustrating for users so that the least-privilege approach is not frustrating?
I've reviewed the docs that explain how this is done. It involves many steps including extracting manifests and merging different rules to create the appropriate Roles and ClusterRoles required.
It looks like it could be scripted. Why is a CLI utility (Extension of existing OLM CLI) not on the cards to automate this process and spit out the required least-privilege RBAC for the user?
|
|
||
| Before arriving at this design, we evaluated simpler alternatives such as removing the field entirely and implicitly using a cluster-admin service account, or making the field optional with cluster-admin as the default. While these approaches improve ease of use, they conflict with the principle of least privilege. | ||
|
|
||
| By making the spec.serviceAccount field optional and introducing synthetic identities with zero permissions by default, this enhancement provides a safer and simpler installation experience. It preserves backward compatibility and still allows the use of custom ServiceAccounts when fine-grained control is needed, aligning usability with the principle of least privilege. |
There was a problem hiding this comment.
Up to this point I'm not seeing how this changes the complexity for an end user.
In the old world:
- They create a service account
- They have to create Roles/ClusterRoles and the appropriate bindings to add permissions to the service account
- They provide the service account to the ClusterExtension to use
The second bullet is the complex part
In the new world:
- They have to create Roles/ClusterRoles and the appropriate bindings to add permissions to the synthetic service account
The complex part still exists?
|
|
||
| ### User Stories | ||
|
|
||
| - As a cluster admin, I want extensions to run with zero privileges by default, so that the cluster remains secure unless I explicitly grant permissions |
There was a problem hiding this comment.
In the current world, I create a service account and bind nothing to it to achieve this, right?
|
|
||
| - As a cluster admin, I want extensions to run with zero privileges by default, so that the cluster remains secure unless I explicitly grant permissions | ||
| - As an extension author, I want to retain the ability to use custom ServiceAccounts for fine grained RBAC, allowing my extensions to operate with the privileges they need | ||
| - As a cluster admin, I want to grant permissions to multiple extensions using group bindings to apply the same permissions to all extensions without a ServiceAccount |
There was a problem hiding this comment.
This user story I'm not quite following. The Why part of the user story is missing, can you expand?
As a <persona>, I would like <something>, so that I can <achieve some goal>
(Nit, none of your user stories follow this format currently)
| #### Deletion Behavior | ||
|
|
||
| - **Extensions with a ServiceAccount:** | ||
| - Existing behavior applies; all resources owned by the CE are deleted, including associated RBAC |
There was a problem hiding this comment.
Why is the service account and RBAC associated to the service account deleted? They are not owned by the ClusterExtension, they are owned by the cluster admin
| - **Extensions with a ServiceAccount:** | ||
| - Existing behavior applies; all resources owned by the CE are deleted, including associated RBAC | ||
| - **Extensions without a ServiceAccount (synthetic identity):** | ||
| - No actual ServiceAccount exists, so OLM does not delete any RBAC bindings created by the admin, so admins are responsible for removing any ClusterRoleBindings or group bindings associated with synthetic identities if desired. |
There was a problem hiding this comment.
This is how it should be for service accounts too, I'm surprised it isn't
| - Document when to use group vs per extension bindings | ||
|
|
||
| ### Benefits | ||
| This approach satisfies both security and usability: zero-permission default when unset, explicit privilege delegation, simplified installation for new users (one manual step of RBAC binding instead of ServiceAccount + RBAC), and preserves backward compatibility. |
There was a problem hiding this comment.
- Service accounts have zero permissions by default
- They require explicit privilege delegation
- The act of creating a service account and binding to that is not particularly cumbersome when put in scope of the RBAC creation - we appear to solving the easy part not the complex part?
| - Installation requires extra manual step of creating RBAC | ||
| - Learning curve for synthetic identities | ||
|
|
||
| Choosing this approach despite these drawbacks as there are more advantages: secure by default, better UX than ServiceAccount+RBAC, simplified implementation. |
There was a problem hiding this comment.
I don't think the first two are real benefits given earlier comments.
On simplified implementation, I think we can improve the implementation of the service account token/impersonation story without having to make any breaking changes.
| 3. Keep SA field required, only add impersonation | ||
| - This doesn't solve usability problem | ||
| 4. Always use impersonation (even when SA is set) | ||
| - Breaks backward compatibility, requires more extensive migration and changes existing behavior |
There was a problem hiding this comment.
These are basically the same right?
Does OLM have the ability (RBAC) to impersonate already? Or is that something that would need to be added?
If it needs to be added, is that something that would require cluster admin intervention, or does CVO handle OLMs RBAC?
Do you have data on how many people are using OLMv1 in the field? If we required a one time RBAC change (create a binding allowing impersonation of a certain service account), how many people would actually be affected?
2 participants
No description provided.