fix: Add ClusterRoleBinding for instancetype:view ClusterRole #11025

0xFelix · 2024-01-16T17:33:59Z

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.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note:

Allow unprivileged users read-only access to VirtualMachineCluster{Instancetypes,Preferences} by default.

0xFelix · 2024-01-16T17:34:23Z

/cc @tiraboschi
/cc @xpivarc

0xFelix · 2024-01-17T07:16:34Z

/retest

fabiand · 2024-01-18T12:45:03Z

@0xFelix please complete the checklist.

Why is this prefixed with "feat:" isn't it rather a fix to allow cluster ITs to be viewed?

0xFelix · 2024-01-18T12:51:42Z

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?

fabiand · 2024-01-18T13:43:23Z

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.

0xFelix · 2024-01-18T14:46:34Z

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>

0xFelix · 2024-01-19T10:47:25Z

/retest

acardace · 2024-01-19T14:32:19Z

pkg/virt-operator/resource/generate/rbac/cluster.go

@@ -715,3 +720,33 @@ func newInstancetypeViewClusterRole() *rbacv1.ClusterRole {
 		},
 	}
 }
+
+func newInstancetypeViewClusterRoleBinding() *rbacv1.ClusterRoleBinding {


Why don't you aggregate to the kubernetes view role like kubevirt.io:view does? This saves you from creating an additional binding.

Because the aggregated kubevirt.io:view role is bound with a RoleBinding, which does not grant access to cluster wide resources.

See #10437 for reference

acardace · 2024-01-19T14:53:31Z

/approve

kubevirt-bot · 2024-01-19T14:53:40Z

[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:

~~OWNERS~~ [acardace]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

0xFelix · 2024-01-22T10:05:02Z

/cherry-pick release-1.1
/cherry-pick release-1.0

kubevirt-bot · 2024-01-22T10:05:04Z

@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:

/cherry-pick release-1.1
/cherry-pick release-1.0

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 · 2024-01-22T10:05:14Z

/cherry-pick release-1.0

kubevirt-bot · 2024-01-22T10:05:17Z

@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:

/cherry-pick release-1.0

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.

jcanocan · 2024-01-22T11:30:16Z

Nice job @0xFelix! Thanks a lot!
/lgtm

kubevirt-commenter-bot · 2024-01-22T11:30:22Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-k8s-1.27-ipv6-sig-network

0xFelix · 2024-01-22T11:30:56Z

/retest

0xFelix · 2024-01-22T12:30:03Z

/retest

kubevirt-bot · 2024-01-22T23:54:51Z

@0xFelix: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-k8s-1.29-sig-compute	`2d215db`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.29-sig-compute`
pull-kubevirt-e2e-k8s-1.27-sig-compute	`2d215db`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.27-sig-compute`

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.

kubevirt-commenter-bot · 2024-01-23T02:54:00Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot · 2024-01-23T03:56:46Z

@0xFelix: #11025 failed to apply on top of branch "release-1.1":

Applying: fix: Use correct constant in ClusterPreference access test
Applying: fix: Add ClusterRoleBinding for instancetype:view ClusterRole
Using index info to reconstruct a base tree...
M	pkg/virt-operator/kubevirt_test.go
M	pkg/virt-operator/resource/generate/rbac/cluster.go
M	pkg/virt-operator/resource/generate/rbac/cluster_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-operator/resource/generate/rbac/cluster_test.go
Auto-merging pkg/virt-operator/resource/generate/rbac/cluster.go
CONFLICT (content): Merge conflict in pkg/virt-operator/resource/generate/rbac/cluster.go
Auto-merging pkg/virt-operator/kubevirt_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 fix: Add ClusterRoleBinding for instancetype:view ClusterRole
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.1
/cherry-pick release-1.0

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.

kubevirt-bot · 2024-01-23T03:57:24Z

@0xFelix: #11025 failed to apply on top of branch "release-1.0":

Applying: fix: Use correct constant in ClusterPreference access test
Applying: fix: Add ClusterRoleBinding for instancetype:view ClusterRole
Using index info to reconstruct a base tree...
M	pkg/virt-operator/kubevirt_test.go
M	pkg/virt-operator/resource/generate/rbac/cluster.go
A	pkg/virt-operator/resource/generate/rbac/cluster_test.go
M	tests/access_test.go
Falling back to patching base and 3-way merge...
Auto-merging tests/access_test.go
CONFLICT (modify/delete): pkg/virt-operator/resource/generate/rbac/cluster_test.go deleted in HEAD and modified in fix: Add ClusterRoleBinding for instancetype:view ClusterRole. Version fix: Add ClusterRoleBinding for instancetype:view ClusterRole of pkg/virt-operator/resource/generate/rbac/cluster_test.go left in tree.
Auto-merging pkg/virt-operator/resource/generate/rbac/cluster.go
CONFLICT (content): Merge conflict in pkg/virt-operator/resource/generate/rbac/cluster.go
Auto-merging pkg/virt-operator/kubevirt_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 fix: Add ClusterRoleBinding for instancetype:view ClusterRole
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.0

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.

lyarwood · 2024-01-24T10:30:45Z

/area instancetype

0xFelix · 2024-01-24T13:55:18Z

/cherry-pick release-1.1

kubevirt-bot · 2024-01-24T13:56:57Z

@0xFelix: new pull request created: #11077

In response to this:

/cherry-pick release-1.1

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.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Jan 16, 2024

kubevirt-bot requested review from kbidarkar and xpivarc January 16, 2024 17:34

kubevirt-bot requested a review from tiraboschi January 16, 2024 17:34

0xFelix force-pushed the instancetype-view-cluster-role-binding branch from 03f79e6 to 0c096af Compare January 18, 2024 14:46

0xFelix changed the title ~~feat: Add ClusterRoleBinding for instancetype:view ClusterRole~~ fix: Add ClusterRoleBinding for instancetype:view ClusterRole Jan 18, 2024

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2024

0xFelix added 2 commits January 19, 2024 09:50

fix: Use correct constant in ClusterPreference access test

6c3f71c

Previously the wrong constant for ClusterInstancetypes was used instead. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

0xFelix force-pushed the instancetype-view-cluster-role-binding branch from 0c096af to 2d215db Compare January 19, 2024 08:52

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2024

acardace reviewed Jan 19, 2024

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2024

kubevirt-bot assigned jcanocan Jan 22, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2024

kubevirt-bot merged commit 2ad6146 into kubevirt:main Jan 23, 2024
35 of 37 checks passed

kubevirt-bot added the area/instancetype label Jan 24, 2024

kubevirt-bot mentioned this pull request Jan 24, 2024

[release-1.1] fix: Add ClusterRoleBinding for instancetype:view ClusterRole #11077

Merged

lyarwood mentioned this pull request Feb 26, 2024

instancetype: Add view ClusterRole docs kubevirt/user-guide#721

Closed

chenrui333 mentioned this pull request Mar 6, 2024

virtctl 1.2.0 Homebrew/homebrew-core#165266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add ClusterRoleBinding for instancetype:view ClusterRole #11025

fix: Add ClusterRoleBinding for instancetype:view ClusterRole #11025

0xFelix commented Jan 16, 2024 •

edited

0xFelix commented Jan 16, 2024

0xFelix commented Jan 17, 2024

fabiand commented Jan 18, 2024

0xFelix commented Jan 18, 2024

fabiand commented Jan 18, 2024

0xFelix commented Jan 18, 2024

0xFelix commented Jan 19, 2024

acardace Jan 19, 2024

0xFelix Jan 19, 2024

0xFelix Jan 19, 2024

acardace commented Jan 19, 2024

kubevirt-bot commented Jan 19, 2024

0xFelix commented Jan 22, 2024

kubevirt-bot commented Jan 22, 2024

0xFelix commented Jan 22, 2024

kubevirt-bot commented Jan 22, 2024

jcanocan commented Jan 22, 2024

kubevirt-commenter-bot commented Jan 22, 2024

0xFelix commented Jan 22, 2024

0xFelix commented Jan 22, 2024

kubevirt-bot commented Jan 22, 2024

kubevirt-commenter-bot commented Jan 23, 2024

kubevirt-bot commented Jan 23, 2024

kubevirt-bot commented Jan 23, 2024

lyarwood commented Jan 24, 2024

0xFelix commented Jan 24, 2024

kubevirt-bot commented Jan 24, 2024

fix: Add ClusterRoleBinding for instancetype:view ClusterRole #11025

fix: Add ClusterRoleBinding for instancetype:view ClusterRole #11025

Conversation

0xFelix commented Jan 16, 2024 • edited

0xFelix commented Jan 16, 2024

0xFelix commented Jan 17, 2024

fabiand commented Jan 18, 2024

0xFelix commented Jan 18, 2024

fabiand commented Jan 18, 2024

0xFelix commented Jan 18, 2024

0xFelix commented Jan 19, 2024

acardace Jan 19, 2024

Choose a reason for hiding this comment

0xFelix Jan 19, 2024

Choose a reason for hiding this comment

0xFelix Jan 19, 2024

Choose a reason for hiding this comment

acardace commented Jan 19, 2024

kubevirt-bot commented Jan 19, 2024

0xFelix commented Jan 22, 2024

kubevirt-bot commented Jan 22, 2024

0xFelix commented Jan 22, 2024

kubevirt-bot commented Jan 22, 2024

jcanocan commented Jan 22, 2024

kubevirt-commenter-bot commented Jan 22, 2024

0xFelix commented Jan 22, 2024

0xFelix commented Jan 22, 2024

kubevirt-bot commented Jan 22, 2024

kubevirt-commenter-bot commented Jan 23, 2024

kubevirt-bot commented Jan 23, 2024

kubevirt-bot commented Jan 23, 2024

lyarwood commented Jan 24, 2024

0xFelix commented Jan 24, 2024

kubevirt-bot commented Jan 24, 2024

0xFelix commented Jan 16, 2024 •

edited