Bug 2052923:[release-4.13] remove all wildcard permissions in rbacs #1967

parth-gr · 2023-03-21T14:45:06Z

remove all wildcard permissions in rbacs

openshift-ci · 2023-03-21T14:45:24Z

@parth-gr: This pull request references Bugzilla bug 2052923, which is valid.

No validations were run on this bug

No GitHub users were found matching the public email listed for the QA contact in Bugzilla ([email protected]), skipping review request.

In response to this:

Bug 2052923:[release-4.13] remove all wildcard permissions in rbac de…

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.

parth-gr · 2023-03-21T14:47:44Z

enum : - '*', still need to look upon

travisn

Two more verbs that may need to be considered:

watch: Likely needed on many of the resources
deletecollection: Not likely needed, unless the ocs operator is deleting multiple resources based on a label, or all resources in a namespace, etc.

parth-gr · 2023-03-29T15:29:13Z

Two more verbs that may need to be considered:

watch: Likely needed on many of the resources

deletecollection: Not likely needed, unless the ocs operator is deleting multiple resources based on a label, or all resources in a namespace, etc.

Sure

travisn · 2023-03-29T17:08:46Z

deploy/csv-templates/ics-operator.csv.yaml.in

@@ -382,6 +432,8 @@ spec:
          - patch
          - update
          - watch
+          - deletecollection


No need to add the permissions where wildcards were not being used before. In this case, there was no wilcard for the verb.

travisn · 2023-03-29T17:10:49Z

deploy/csv-templates/ics-operator.csv.yaml.in

          - create
          - delete
          - get
          - list
          - patch
          - update
          - watch
+          - deletecollection
+          - create


No need to duplicate some of these permissions that are already specified

these are still duplicated

travisn · 2023-03-29T17:12:03Z

deploy/csv-templates/ics-operator.csv.yaml.in

@@ -394,6 +446,8 @@ spec:
          - patch
          - update
          - watch
+          - deletecollection


@malayparida2000 Do you know of any places the OCS operator needs to delete multiple resources in a single API call? If not, the deletecollection would not be needed to add when removing the wildcard.

@iamniting ^^

I dont beleive if we are deleting collections anywhere in the ocs-operator

iamniting

Could you please check the controller when removing the asterisk permissions? We should evaluate whether we actually require all of them, and if not, we can add only the necessary ones.

iamniting · 2023-03-30T16:23:14Z

deploy/csv-templates/ics-operator.csv.yaml.in

@@ -382,6 +432,8 @@ spec:
          - patch
          - update
          - watch
+          - deletecollection


iamniting · 2023-03-30T16:23:57Z

deploy/csv-templates/ics-operator.csv.yaml.in

@@ -394,6 +446,8 @@ spec:
          - patch
          - update
          - watch
+          - deletecollection


I dont beleive if we are deleting collections anywhere in the ocs-operator

openshift-ci · 2023-03-31T14:23:02Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: parth-gr
Once this PR has been reviewed and has the lgtm label, please ask for approval from iamniting. 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:

OWNERS

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

parth-gr · 2023-03-31T14:24:25Z

Could you please check the controller when removing the asterisk permissions? We should evaluate whether we actually require all of them, and if not, we can add only the necessary ones.

With this change, we are planning for changing from * to all required permission and would think of restricting to specifics in the future.

travisn · 2023-03-31T14:26:22Z

deploy/csv-templates/ics-operator.csv.yaml.in

@@ -382,6 +427,7 @@ spec:
          - patch
          - update
          - watch
+          - watch


watch is duplicated in several places here

Thanks for catching it,

Tried to update all the places, please have a look once again

travisn · 2023-03-31T14:26:40Z

deploy/csv-templates/ics-operator.csv.yaml.in

          - create
          - delete
          - get
          - list
          - patch
          - update
          - watch
+          - deletecollection
+          - create


these are still duplicated

iamniting · 2023-04-03T04:35:10Z

Could you please check the controller when removing the asterisk permissions? We should evaluate whether we actually require all of them, and if not, we can add only the necessary ones.

With this change, we are planning for changing from * to all required permission and would think of restricting to specifics in the future.

I think as of now it would be easy to do that as we will have to look for fewer permissions (only the ones where * is present) for now. If we decide to do it later we will have to look for all the permissions.

iamniting

Can you please fix the commit-msg as well? It looks like it is in 2 lines.

iamniting · 2023-04-03T04:27:58Z

config/rbac/role.yaml

 - apiGroups:
  - storage.k8s.io
  resources:
  - storageclasses
  verbs:
-  - '*'
+   - create


These are duplicates too, All of these are already present here.

iamniting · 2023-04-03T04:30:46Z

config/rbac/role.yaml

@@ -23,7 +23,13 @@ rules:
  - replicasets
  - statefulsets
  verbs:
-  - '*'
+   - create


Can you pls fix the indentation? There is one extra space.

iamniting · 2023-04-03T04:31:58Z

rbac/rook-monitor-role.yaml

  verbs:
-  - '*'
+   - create


Can you fix the indentation here as well?

iamniting · 2023-04-03T04:32:29Z

metrics/deploy/rbac.yaml

  verbs:
-  - '*'
+   - create


Can you fix the indentation here as well?

openshift-ci · 2023-04-03T15:32:01Z

@parth-gr: This pull request references Bugzilla bug 2052923, which is valid.

No validations were run on this bug

No GitHub users were found matching the public email listed for the QA contact in Bugzilla ([email protected]), skipping review request.

In response to this:

Bug 2052923:[release-4.13] remove all wildcard permissions in rbacs

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.

travisn · 2023-04-03T16:24:36Z

config/rbac/role.yaml

+  - list
+  - patch
+  - update
+   - watch


- watch looks indented an extra space in many places

iamniting · 2023-04-04T10:08:57Z

config/rbac/role.yaml

+  - list
+  - patch
+  - update
+   - watch


iamniting · 2023-04-04T10:12:52Z

@parth-gr It looks like you are changing the files under deploy manually. That's why CI is failing. You can auto-generate the changes using make gen-latest-csv and FUSION=true make gen-latest-csv.

parth-gr · 2023-04-04T11:56:02Z

FUSION=true make gen-latest-csv

Running this make gen-latest-csv and FUSION=true make gen-latest-csv makes the changes back to use wildcard

parth-gr · 2023-04-04T12:06:32Z

So basically these look automated files,
So from where these files are generated so I can apply the changes there
@iamniting

iamniting · 2023-04-04T12:16:08Z

So basically these look automated files, So from where these files are generated so I can apply the changes there @iamniting

These files are generated from the config dir and the rook CSV. If the rook CSV has wildcards we need to solve there themselves. For now, let's not update the files under deploy manually. Updating them manually will not help us as they will be reverted back while generating a build. Also, CI won't allow us to merge such PR.

parth-gr · 2023-04-04T12:46:37Z

So basically these look automated files, So from where these files are generated so I can apply the changes there @iamniting

These files are generated from the config dir and the rook CSV. If the rook CSV has wildcards we need to solve there themselves. For now, let's not update the files under deploy manually. Updating them manually will not help us as they will be reverted back while generating a build. Also, CI won't allow us to merge such PR.

Thanks

iamniting · 2023-04-04T13:27:25Z

rbac/rook-monitor-role.yaml

@@ -6,6 +6,27 @@ rules:
 - apiGroups:
  - monitoring.coreos.com
  resources:
-  - '*'
+  - cephclients


I am wondering if these resources really belongs to the monitoring.coreos.com. If I am not mistaken they belong to the ceph.rook.io. @umangachapagain Can you pls guide @parth-gr What resources should come under the monitoring.coreos.com

We only care about servicemonitors and prometheusrules AFAIR for monitoring.
Link to CRD def: https://github.com/prometheus-operator/kube-prometheus/tree/main/manifests/setup

Okay thanks

Signed-off-by: parth-gr <[email protected]>

travisn

The only wildcards were related to the prometheus resources? Previously your PR had many more wildcards updated.

parth-gr · 2023-04-05T15:30:16Z

The only wildcards were related to the prometheus resources? Previously your PR had many more wildcards updated.

If I am running make gen-latest-csv and FUSION=true make gen-latest-csv they all get reverted,
It because the deploy files are auto created

iamniting · 2023-04-06T09:00:07Z

Hi @parth-gr We will require few more changes

We need to remove permissions from the CSV merger tool here
We need to add the same permissions here

iamniting

Hi @parth-gr We will require few more changes

We need to remove permissions from the CSV merger tool here
We need to add the same permissions here

Let me know if you need help we can meet and get it sorted.

parth-gr · 2023-04-06T14:53:51Z

Hi @parth-gr We will require few more changes
We need to remove permissions from the CSV merger tool here
We need to add the same permissions here

Let me know if you need help we can meet and get it sorted.

Sure will meet you tmr, thanks

openshift-merge-robot · 2023-07-01T00:15:42Z

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.

openshift-ci · 2023-07-17T07:16:44Z

@parth-gr: 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
ci/prow/ci-bundle-ocs-operator-bundle	`fd9fd0e`	link	true	`/test ci-bundle-ocs-operator-bundle`
ci/prow/ocs-operator-bundle-e2e-aws	`fd9fd0e`	link	true	`/test ocs-operator-bundle-e2e-aws`

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.

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Mar 21, 2023

travisn requested changes Mar 27, 2023

View reviewed changes

openshift-ci bot assigned travisn Mar 27, 2023

parth-gr force-pushed the wildcard-ocs branch from ad024c8 to bf8080d Compare March 29, 2023 15:29

parth-gr force-pushed the wildcard-ocs branch from bf8080d to 2971b6a Compare March 29, 2023 15:30

travisn requested changes Mar 29, 2023

View reviewed changes

iamniting requested changes Mar 30, 2023

View reviewed changes

openshift-ci bot assigned iamniting Mar 30, 2023

parth-gr force-pushed the wildcard-ocs branch from 2971b6a to f860a7a Compare March 31, 2023 14:22

parth-gr requested a review from iamniting March 31, 2023 14:24

parth-gr force-pushed the wildcard-ocs branch from f860a7a to e294357 Compare March 31, 2023 14:26

travisn reviewed Mar 31, 2023

View reviewed changes

parth-gr force-pushed the wildcard-ocs branch 3 times, most recently from 36e34a9 to 6f35d74 Compare March 31, 2023 14:39

parth-gr requested a review from travisn March 31, 2023 14:40

iamniting requested changes Apr 3, 2023

View reviewed changes

parth-gr force-pushed the wildcard-ocs branch from 6f35d74 to f04c812 Compare April 3, 2023 15:31

parth-gr changed the title ~~Bug 2052923:[release-4.13] remove all wildcard permissions in rbac de…~~ Bug 2052923:[release-4.13] remove all wildcard permissions in rbacs Apr 3, 2023

travisn reviewed Apr 3, 2023

View reviewed changes

iamniting requested changes Apr 4, 2023

View reviewed changes

config/rbac/role.yaml Outdated

- list

- patch

- update

- watch

Copy link

Member

iamniting Apr 4, 2023

Choose a reason for hiding this comment

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

+1

parth-gr force-pushed the wildcard-ocs branch from f04c812 to 4d275b3 Compare April 4, 2023 12:45

iamniting requested changes Apr 4, 2023

View reviewed changes

parth-gr force-pushed the wildcard-ocs branch from 4d275b3 to 496eb14 Compare April 5, 2023 13:05

Bug 2052923:[release-4.13] remove all wildcard permissions in rbacs

fd9fd0e

Signed-off-by: parth-gr <[email protected]>

parth-gr force-pushed the wildcard-ocs branch from 496eb14 to fd9fd0e Compare April 5, 2023 13:10

parth-gr requested a review from iamniting April 5, 2023 14:57

travisn reviewed Apr 5, 2023

View reviewed changes

iamniting requested changes Apr 6, 2023

View reviewed changes

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

@@ @@ -382,6 +427,7 @@ spec: @@
                         - patch
                         - update
                         - watch
+                        - watch

Bug 2052923:[release-4.13] remove all wildcard permissions in rbacs #1967

Are you sure you want to change the base?

Bug 2052923:[release-4.13] remove all wildcard permissions in rbacs #1967

Conversation

parth-gr commented Mar 21, 2023 • edited Loading

openshift-ci bot commented Mar 21, 2023

parth-gr commented Mar 21, 2023

travisn left a comment

Choose a reason for hiding this comment

parth-gr commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamniting left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 31, 2023

parth-gr commented Mar 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamniting commented Apr 3, 2023

iamniting left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamniting commented Apr 4, 2023

parth-gr commented Apr 4, 2023

parth-gr commented Apr 4, 2023

iamniting commented Apr 4, 2023

parth-gr commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisn left a comment

Choose a reason for hiding this comment

parth-gr commented Apr 5, 2023

iamniting commented Apr 6, 2023

iamniting left a comment

Choose a reason for hiding this comment

parth-gr commented Apr 6, 2023

openshift-merge-robot commented Jul 1, 2023

openshift-ci bot commented Jul 17, 2023

parth-gr commented Mar 21, 2023 •

edited

Loading