try to constrain rbac based on e2e usage #598
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awgreene The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
awgreene
force-pushed
the
constrain-olm-rbac
branch
2 times, most recently
from
4e82c3e to
c4db2c1
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
awgreene
force-pushed
the
constrain-olm-rbac
branch
from
c4db2c1 to
add2e28
Compare
| - apiGroups: | ||
| - example.com | ||
| resources: | ||
| - tests | ||
| verbs: | ||
| - "*" | ||
| - apiGroups: | ||
| - kiali.io | ||
| resources: | ||
| - kialis | ||
| verbs: | ||
| - "*" | ||
| - apiGroups: | ||
| - monitoring.kiali.io | ||
| resources: | ||
| - monitoringdashboards | ||
| verbs: | ||
| - "*" |
There was a problem hiding this comment.
The RBAC proposed in this PR stemmed from a run of audit2RBAC against an e2e test's audit logs, I'm not sure how we use these RBACs.
- apiGroups:
- example.com
resources:
- tests
verbs:
- "*"
- apiGroups:
- kiali.io
resources:
- kialis
verbs:
- "*"
- apiGroups:
- monitoring.kiali.io
resources:
- monitoringdashboards
verbs:
- "*"
There was a problem hiding this comment.
How confident can we be here that this audit will have captured all of the permissions we would need? I'm seeing kiali operator stuff in there and wondering if that means we would need to add every resource type from every operator that OLM manages in order to maintain functionality, which would be impractical.
There was a problem hiding this comment.
I’m not sure what other operators APIs has to do with this PR. This PR is to restrict the RBAC that is currently used by olm-operator to avoid using wildcards which means it can access a lot of other APIs that it doesn’t need. This only restricts it to a set of APIs that it needs.
There was a problem hiding this comment.
I don’t know why those Kiali APIs were added in the original version of this PR but they are not needed as far as I know unless all the sudden OLM is using Kiali operator now.
There was a problem hiding this comment.
Also this PR should be on upstream as well. We can certainly refine/restrict RBAC on manifests that are used on upstream.
There was a problem hiding this comment.
Hey! So I saw a similar issue upstream and it's because of the CR/CRD validation logic that OLM has. As @dinhxuanvu pointed out, we had an upstream PR that was a bit less strict on the RBAC we requested, I primarily was using this PR to test something on an OpenShift cluster.
The upstream PR linked earlier is being downstreamed in #600, so I'm going to close this PR.
| verbs: | ||
| - "*" |
There was a problem hiding this comment.
There were options to constrain this further, but I was happy enough with wildcard permissions on what we actively use for now.
Signed-off-by: Alexander Greene <[email protected]>
awgreene
force-pushed
the
constrain-olm-rbac
branch
from
add2e28 to
da37912
Compare
|
@awgreene: The following tests 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/test-infra repository. I understand the commands that are listed here. |
|
I was using this PR to test some changes to OLM's RBAC, which lead to this upstream PR which is being downstreamed in #600, so I'm going to close this PR. |
No description provided.