-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Add ClusterRoleBinding for instancetype:view ClusterRole #11025
fix: Add ClusterRoleBinding for instancetype:view ClusterRole #11025
Conversation
/cc @tiraboschi |
/retest |
@0xFelix please complete the checklist. Why is this prefixed with "feat:" isn't it rather a fix to allow cluster ITs to be viewed? |
IMO you could consider it both. Isn't it a feature to provide access to something which unprivileged users didn't have access to before by adding a new ClusterRoleBinding? |
I'd say that one of the inheritn goals of having ITs on the cluster level is to make them available to all usres (privileged or not) in order to align on this set of ITs. IOW ITs which can not be accessed by unprivileged users seem to be useless. |
03f79e6
to
0c096af
Compare
Let's consider it a fix then. |
Previously the wrong constant for ClusterInstancetypes was used instead. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>
This adds a ClusterRolebinding which grants every authorized user read access to VirtualMachineCluster{Instancetype,Preference} CRs. This allows unprivileged users to make use of VirtualMachineCluster{Instancetypes,Preferences} by default. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>
0c096af
to
2d215db
Compare
/retest |
@@ -715,3 +720,33 @@ func newInstancetypeViewClusterRole() *rbacv1.ClusterRole { | |||
}, | |||
} | |||
} | |||
|
|||
func newInstancetypeViewClusterRoleBinding() *rbacv1.ClusterRoleBinding { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you aggregate to the kubernetes view role like kubevirt.io:view
does? This saves you from creating an additional binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the aggregated kubevirt.io:view
role is bound with a RoleBinding
, which does not grant access to cluster wide resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #10437 for reference
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: acardace 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 |
/cherry-pick release-1.1 |
@0xFelix: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you. 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/test-infra repository. |
/cherry-pick release-1.0 |
@0xFelix: once the present PR merges, I will cherry-pick it on top of release-1.0 in a new PR and assign it to you. 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/test-infra repository. |
Nice job @0xFelix! Thanks a lot! |
Required labels detected, running phase 2 presubmits: |
/retest |
1 similar comment
/retest |
@0xFelix: The following tests failed, say
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. |
/retest-required |
@0xFelix: #11025 failed to apply on top of branch "release-1.1":
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/test-infra repository. |
@0xFelix: #11025 failed to apply on top of branch "release-1.0":
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/test-infra repository. |
/area instancetype |
/cherry-pick release-1.1 |
@0xFelix: new pull request created: #11077 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/test-infra repository. |
What this PR does / why we need it:
This adds a ClusterRolebinding which grants every authorized user read
access to VirtualMachineCluster{Instancetype,Preference} CRs.
This allows unprivileged users to make use of
VirtualMachineCluster{Instancetypes,Preferences} by default.
Also previously the wrong constant for ClusterInstancetypes was used instead, so fix that.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes https://issues.redhat.com/browse/CNV-36300
Special notes for your reviewer:
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: