OLMv1: permissions validation preflight check #1768
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
grokspawn
force-pushed
the
permissions-preflight
branch
from
ef70d79 to
1902247
Compare
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
grokspawn
force-pushed
the
permissions-preflight
branch
from
1902247 to
f8fe50e
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
|
|
||
|
|
||
| ### Non-Goals | ||
|
|
There was a problem hiding this comment.
I would also call out here that it is a non-goal to replace or supersede the cluster's authorization chain.
| Introduce a preflight check that evaluates the permissions granted to the provided ServiceAccount against the permissions required by the ClusterExtension bundle. If any permissions are missing, the system will: | ||
| - Provide a detailed, user-friendly error message listing missing permissions. | ||
| - Prevent installation or upgrades from proceeding until the issue is resolved. | ||
| - The pre-flight check implemented as part of this work should include more than just surfacing the RBAC in the bundle. Generally, it should surface any permissions that are needed to stamp out all resources in the bundle on the cluster. |
There was a problem hiding this comment.
It should also surface permissions that OLM itself needs to manage the lifecycle of the bundle. For example:
- list/watch for all bundle objects so that OLM can run informers and trigger reconciliation when they change
- update for clusterextensions/finalizers for the parent clusterextension, so that OLM can include blockOwnerDeletion owner references
We don't need to list all of the examples out. Just point out that OLM itself might exert requirements on the rules that a service account needs.
| ### Non-Goals | ||
|
|
||
| - Only what is necessary to install the ClusterExtension based on what is provided in the bundle should be verified. OLM will only have knowledge of Kubernetes and applying Kubernetes resources so we won't be able to verify something like access to an external secret store as part of an extension's installation. | ||
| - Optionality of the preflight check. |
There was a problem hiding this comment.
I think this should be more of an open question for cluster auth-aligned reviewers. Is it reasonable to have a required check that cannot be disabled that assumes RBAC authorization for ClusterExtension service accounts?
There was a problem hiding this comment.
Added an explicit note to ensure we have the convo
f3a38b2
|
|
||
| The preflight check will extend the existing preflight interface in the operator controller. Key components: | ||
| 1. Permission Analysis | ||
| - Use the ServiceAccount token to verify access against required permissions declared in the ClusterExtension’s bundle. |
There was a problem hiding this comment.
I don't think we're actually planning to use the SA token in this case. Rather, we are building a local cache of all RBAC and doing a local best-effort evaluation.
| Our approach can be broken down into two parts. | ||
|
|
||
| Part 1: Validating permissions required for installing the Cluster Extension: | ||
| - An initial fail-fast check that ensures the provided ServiceAccount has GET permissions required to perform the Helm dry-run. GET permissions are needed because the Helm dry-run API simulates the installation by rendering manifests and checking the Kubernetes API server for resource existence and state. Without these permissions, Helm cannot validate or resolve dependencies, leading to authorization errors or incomplete simulation. |
There was a problem hiding this comment.
I think this is out-of-date now. The latest plan is to do a client-only template rendering (which requires no cluster permissions at all), and then do the preauthorization checks based on the local-only-generated manifest.
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten enhancement proposals close after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Reopen the proposal by commenting /close |
|
@openshift-bot: Closed this PR. In 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 kubernetes-sigs/prow repository. |
|
/reopen |
|
@grokspawn: Reopened this PR. In 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 kubernetes-sigs/prow 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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
|
@grokspawn: 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. |
No description provided.