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

Remove "get" verb from "events" resource of RBAC for provisioner sidecar #245

Conversation

mpatlasov
Copy link
Contributor

Let's keep assets/rbac/provisioner_role.yaml in sync with upstream (https://github.com/openshift/csi-external-provisioner/blob/master/deploy/kubernetes/rbac.yaml)

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 23, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mpatlasov
Once this PR has been reviewed and has the lgtm label, please assign tsmetana 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

@gnufied
Copy link
Member

gnufied commented Jun 23, 2023

I think it is unnecessary to remove get permissions when - list, watch etc is included. I would think that, there might be code that assumes it has get permissions because it has list permissions etc. It would surprise such users.

It also doesn't buys much in terms of reducing permission vector of the sidecar/operators.

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jun 23, 2023

I think it is unnecessary to remove get permissions when - list, watch etc is included. I would think that, there might be code that assumes it has get permissions because it has list permissions etc. It would surprise such users.

It also doesn't buys much in terms of reducing permission vector of the sidecar/operators.

Yes, I fully agree! I opened this PR as a follow-up for Jan's comment that it would be great to keep our openshift perm list in-sync with upstream.

Upstream doesn't have get, while openshift does!

IMO, we have 3 options:

  1. Convince upstream that lack of get might surprise some users (and other arguments you mentioned above)
  2. Remove get from openshift (if CI-tests are happy, and nothing gets broken)
  3. Keep everything as is (because the issue is too minor to bother) :)

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2023
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

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.

@mpatlasov
Copy link
Contributor Author

@jsafrane , can you please read my comment above (#245 (comment)) and reply something. (I tend to close this PR because it's rather minor topic and nobody is interested in it, but if you think it's worthy to follow-up, I can rebase and discuss it further)

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2023

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

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ovn-upgrade 51875d7 link true /test e2e-aws-ovn-upgrade

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.

@jsafrane
Copy link
Contributor

I think we should use upstream RBAC definitions where possible. If we're going to use custom permissions, then we need to review all code an updated driver to see if we need to modify them. If we use the upstream ones, then it's task of the upstream to keep them up to date and we can blindly copy them.

@mpatlasov
Copy link
Contributor Author

The file assets/rbac/provisioner_role.yaml was moved to another repo recently. See openshift/cluster-storage-operator#390 as a follow-up.

@mpatlasov mpatlasov closed this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants