Skip to content
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

forklift: add missing rbac #3320

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bennyz
Copy link
Contributor

@bennyz bennyz commented Jun 13, 2024

What this PR does / why we need it:

Add missing RBAC for ovirt and openstack volume populator CRDs

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 #

Special notes for your reviewer:

Release note:

Add missing RBAC for ovirt and openstack volume populator CRDs

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 13, 2024
@kubevirt-bot
Copy link
Contributor

Hi @bennyz. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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.

@kubevirt-bot kubevirt-bot requested review from akalenyu and awels June 13, 2024 17:17
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign akalenyu for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added size/M release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 13, 2024
@bennyz bennyz force-pushed the forklift-missing-rbac branch from 1ccdb77 to 8497de8 Compare June 13, 2024 19:20
@bennyz bennyz marked this pull request as ready for review June 13, 2024 19:21
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2024
"get",
"list",
"watch",
"*",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bennyz hi. I remember having global perms was not recommended. Refer - https://issues.redhat.com/browse/CNV-22907 and https://issues.redhat.com/browse/CNV-32812

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have to do this to because of how pkg/operator/resources/cluster/rbac.go is set

There is also:

		{
			APIGroups: []string{
				"cdi.kubevirt.io",
			},
			Resources: []string{
				"*",
			},
			Verbs: []string{
				"*",
			},
},

in L#169

But I'm open to suggestions

Copy link
Collaborator

@akalenyu akalenyu Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind unifying those two with a comment that explains the motivation?
Something along the lines of, "*" permissions are bad, but logically it's clear that CDI can do anything on CDI resources

@bennyz bennyz force-pushed the forklift-missing-rbac branch from 8497de8 to 4584d36 Compare June 14, 2024 06:14
@akalenyu
Copy link
Collaborator

akalenyu commented Jun 14, 2024

/test all

I remember having global perms was not recommended

Yup, there should be a unit test for this
EDIT:
We have exceptions for some global permissions. For example, it would make sense that "CDI can do anything on CDI resources", maybe the same applies to these resources as well, have to check

{
APIGroups: []string{
"forklift.cdi.kubevirt.io",
},
Resources: []string{
"ovirtvolumepopulators",
"openstackvolumepopulators",
},
Verbs: []string{
"get",
"list",
"watch",
},
},
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this?

@bennyz bennyz force-pushed the forklift-missing-rbac branch from 50d7e4d to e0b0dc2 Compare June 25, 2024 07:54
@awels
Copy link
Member

awels commented Jul 12, 2024

/test all

@awels
Copy link
Member

awels commented Aug 16, 2024

/retest

@kubevirt-bot
Copy link
Contributor

@bennyz: 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-cdi-goveralls e0b0dc2 link false /test pull-cdi-goveralls
pull-cdi-unit-test e0b0dc2 link false /test pull-cdi-unit-test

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.

@aglitke
Copy link
Member

aglitke commented Oct 8, 2024

@awels This PR seems stalled. Do we still want to have it merged?

@awels
Copy link
Member

awels commented Oct 9, 2024

We do, but we need to address the * permissions. We have a unit test that disallows this. We should be explicit in the permissions we want to give otherwise security will complain. Alex already pointed this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants