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.

Sign up for GitHub

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

Jump to bottom

Aggregate cluster-reader role #20279

Merged
merged 3 commits into from
Jul 29, 2018
Merged

Aggregate cluster-reader role #20279

merged 3 commits into from
Jul 29, 2018

Conversation

mrogers950
Copy link
Contributor

Turn cluster-reader into an aggregate-able role. Aggregate the view role
into cluster-reader and remove duplicates from the stock cluster-reader
role.
@openshift/sig-security @smarterclayton @deads2k

@openshift-ci-robot openshift-ci-robot added sig/security size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 10, 2018
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 10, 2018
@mrogers950
Copy link
Contributor Author

/retest

deads2k
deads2k reviewed Jul 11, 2018
View reviewed changes
pkg/cmd/server/bootstrappolicy/policy.go Outdated
},
},
{
ObjectMeta: metav1.ObjectMeta{Name: AggregatedClusterReaderRoleName, Labels: map[string]string{"rbac.authorization.k8s.io/aggregate-to-cluster-reader": "true"}},
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: formatting

deads2k
deads2k reviewed Jul 11, 2018
View reviewed changes
pkg/cmd/server/bootstrappolicy/policy.go Outdated
@@ -173,30 +179,23 @@ func GetOpenshiftBootstrapClusterRoles() []rbacv1.ClusterRole {

rbacv1helpers.NewRule(read...).Groups(authzGroup, legacyAuthzGroup).Resources("clusterroles", "clusterrolebindings", "roles", "rolebindings", "rolebindingrestrictions").RuleOrDie(),

rbacv1helpers.NewRule(read...).Groups(buildGroup, legacyBuildGroup).Resources("builds", "builds/details", "buildconfigs", "buildconfigs/webhooks", "builds/log").RuleOrDie(),
rbacv1helpers.NewRule(read...).Groups(buildGroup, legacyBuildGroup).Resources("builds/details").RuleOrDie(),
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these changing?

deads2k
deads2k reviewed Jul 11, 2018
View reviewed changes
pkg/cmd/server/bootstrappolicy/policy.go Outdated
ObjectMeta: metav1.ObjectMeta{Name: AggregatedViewRoleName, Labels: map[string]string{"rbac.authorization.k8s.io/aggregate-to-view": "true"}},
ObjectMeta: metav1.ObjectMeta{Name: AggregatedViewRoleName, Labels: map[string]string{
"rbac.authorization.k8s.io/aggregate-to-view": "true",
"rbac.authorization.k8s.io/aggregate-to-cluster-reader": "true",
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't going to end well for you. No one "normal" will add to this.

deads2k
deads2k reviewed Jul 11, 2018
View reviewed changes
pkg/cmd/server/bootstrappolicy/policy.go Outdated
},
},
{
ObjectMeta: metav1.ObjectMeta{Name: AggregatedClusterReaderRoleName, Labels: map[string]string{"rbac.authorization.k8s.io/aggregate-to-cluster-reader": "true"}},
Copy link
Contributor

Choose a reason for hiding this comment

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

you want "rbac.authorization.k8s.io/aggregate-to-view" too right? Otherwise nothing from upstream or the community will aggregate the way you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the current rules to initially aggregate to cluster-reader, why aggregate them to view also? My intent was to assemble cluster-reader by aggregating the current rules plus view into cluster-reader.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry commented on the wrong line. the selector needs that. I may have been in the wrong spot in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, I thought that may have been the case, so I tried updating the cluster-reader role's MatchLabel with both aggregate-to-cluster-reader and aggregate-to-view labels, but nothing gets aggregated. I think that matching on two labels means a to-aggregate role must have both labels.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem, I thought that may have been the case, so I tried updating the cluster-reader role's MatchLabel with both aggregate-to-cluster-reader and aggregate-to-view labels, but nothing gets aggregated. I think that matching on two labels means a to-aggregate role must have both labels.

I wrote it to OR them. Try adding a second label selector instead of adding to the first selector

@deads2k
Copy link
Contributor

deads2k commented Jul 11, 2018

The idea is fine. I think you need to aggregate more than you currently are and I would suggest doing a separate pull to prune the old role.

Matt Rogers added 2 commits July 11, 2018 17:11
Aggregate cluster-reader role
d3105b2 
Turn cluster-reader into an aggregate-able role and aggregate view into it.
The aggregation of view results in adding the following rules to
cluster-reader:

- imagestreammappings: get list watch
- imagestreammappings.image.openshift.io: get list watch
- jenkins.build.openshift.io: view
Remove duplicate view roles from aggregated cluster-reader
726079c
@mrogers950 mrogers950 force-pushed the aggregate-cr branch 2 times, most recently from 8d0c4df to 2195841 Compare July 13, 2018 19:12
@mrogers950
Copy link
Contributor Author

/retest

mrogers950
mrogers950 commented Jul 13, 2018
View reviewed changes
pkg/cmd/server/bootstrappolicy/web_console_role_test.go
@@ -65,6 +65,7 @@ var rolesToHide = sets.NewString(
"system:openshift:aggregate-to-admin",
"system:openshift:aggregate-to-edit",
"system:openshift:aggregate-to-view",
"system:openshift:aggregate-to-cluster-reader",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@openshift/origin-web-console-committers I've added a new role here that I think should be hidden to the end user, (heads up per the test's comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrogers950 @enj aha, the test lives! But we can eliminate that test, might be a good time to do so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benjaminapetersen I can do a follow-up to get rid of this test if it's no longer useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrogers950 sounds good. Happy to chat about it if that would be helpful.

@mrogers950
Copy link
Contributor Author

@deads2k should be all set. If there's no other changes needed can we merge this in 3.11 or should we defer it?

@mrogers950
Copy link
Contributor Author

@deads2k PTAL

@deads2k
Copy link
Contributor

deads2k commented Jul 26, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, mrogers950

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2018
@mrogers950
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 91bbfec into openshift:master Jul 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjaminapetersen benjaminapetersen benjaminapetersen left review comments

@deads2k deads2k deads2k left review comments

@knobunc knobunc Awaiting requested review from knobunc

@squeed squeed Awaiting requested review from squeed

Assignees

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. sig/security 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.

None yet
6 participants
@mrogers950 @deads2k @openshift-ci-robot @openshift-bot @benjaminapetersen @openshift-merge-robot