Skip to content
This repository has been archived by the owner on Jul 30, 2021. It is now read-only.

add expanded cluster-reader role rules #112

Closed
wants to merge 1 commit into from
Closed

add expanded cluster-reader role rules #112

wants to merge 1 commit into from

Conversation

richm
Copy link

@richm richm commented Feb 26, 2019

the cluster-logging-operator needs the cluster-reader clusterrole
rights - we cannot add a roleRef: cluster-reader so add the
full list of rules from oc get clusterrole cluster-reader
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1680504

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 26, 2019
@richm
Copy link
Author

richm commented Feb 26, 2019

@jcantrill @ewolinetz

SamiSousa
SamiSousa reviewed Feb 27, 2019
View reviewed changes
Copy link
Contributor

@SamiSousa SamiSousa left a comment

Choose a reason for hiding this comment

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

Is it possible to consolidate some of these roles? I was following the messages on the forum, and was wondering if this could be reduced in any way.
Lines of code isn't being enforced, but minimizing rbac for each operator is preferable.

community-operators/cluster-logging/clusterlogging.v0.0.1.clusterserviceversion.yaml
- ""
- route.openshift.io
resources:
- routes/status
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this role be consolidated with the role above? They have the same api group and verbs

Copy link
Author

Choose a reason for hiding this comment

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

/me wishes the output of oc get clusterrole cluster-reader -o yaml would be already condensed . . .

you mean, something like this?

        - apiGroups:
          - ""
          - route.openshift.io
          resources:
          - routes
          - routes/status
          verbs:
          - get
          - list
          - watch

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah just like that. That's effectively the same role as far as I can tell.

@richm richm force-pushed the logging-add-expanded-cluster-reader branch from 139ce43 to 13e38a5 Compare February 27, 2019 18:28
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 27, 2019
@richm richm mentioned this pull request Feb 27, 2019
Closed
@richm
add expanded cluster-reader role rules
252f4f2 
This is what I did to get the list of `cluster-reader` rules
to add to the `clusterPermissions`:

- on a current cluster, `oc get clusterrole cluster-reader -o yaml`
- edit the file so that it contained just the list of rules
- run through a python script to "de-dup" the rules

For example, change this:

```yaml
        - apiGroups:
          - ""
          - route.openshift.io
          resources:
          - routes/status
          verbs:
          - get
          - list
          - watch
        - apiGroups:
          - ""
          - route.openshift.io
          resources:
          - routes
          verbs:
          - get
          - list
          - watch
```

to this

```
        - apiGroups:
          - ""
          - route.openshift.io
          resources:
          - routes
          - routes/status
          verbs:
          - get
          - list
          - watch
```

There were several such cases in the cluster-reader role definition.
@richm richm force-pushed the logging-add-expanded-cluster-reader branch from 13e38a5 to 252f4f2 Compare February 27, 2019 21:23
@richm
Copy link
Author

richm commented Feb 27, 2019

note - not the same as cluster-reader - I had to remove this:

        - apiGroups:
          - build.openshift.io
          resources:
          - jenkins
          verbs:
          - view

because it failed verb validation - view is not a valid verb

@enj
Copy link

enj commented Feb 27, 2019

/hold

cluster reader changes every release, and is an aggregated cluster role that can be expanded based on other installed components. Any attempt to copy it is brittle.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2019
@richm
Copy link
Author

richm commented Feb 27, 2019

ok - so what are our alternatives?

Not sure how long either of these will take, and in the meantime, logging is blocked due to https://bugzilla.redhat.com/show_bug.cgi?id=1680504 . . .

@enj
Copy link

enj commented Feb 27, 2019

We can investigate, and with some trial&error, pare down the list of rules needed by fluentd and therefore cluster-logging-operator, in order to have the minimum necessary permissions

That seems like the best approach.

@richm
Copy link
Author

richm commented Feb 28, 2019

closing in favor of openshift/cluster-logging-operator#106

@richm richm closed this Feb 28, 2019
@richm richm deleted the logging-add-expanded-cluster-reader branch February 28, 2019 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SamiSousa SamiSousa SamiSousa left review comments

@awgreene awgreene Awaiting requested review from awgreene

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@richm @enj @SamiSousa @openshift-ci-robot