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

Remove RBAC UserAll #38981

Merged
merged 1 commit into from
Jan 5, 2017
Merged

Remove RBAC UserAll #38981

merged 1 commit into from
Jan 5, 2017

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Dec 19, 2016 •
edited
Loading

  • Removes special handling of User * subjects in rolebinding matching evaluation
  • Converts v1alpha1 rolebindings to User * subjects to Group system:authenticated subjects for backwards compatibility
RBAC's special handling of the user "*" in RoleBinding and ClusterRoleBinding objects is deprecated and will be removed in v1beta1. To match all users, explicitly bind to the group "system:authenticated" and/or "system:unauthenticated". Existing v1alpha1 bindings to the user "*" will be automatically converted to the group "system:authenticated".

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 19, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Dec 19, 2016
@liggitt liggitt force-pushed the remove-rbac-user-all branch from 41622ef to 0d31f63 Compare December 19, 2016 18:53
@liggitt
Copy link
Member Author

liggitt commented Dec 19, 2016

cc @kubernetes/sig-auth-misc

@liggitt liggitt mentioned this pull request Dec 19, 2016
Merged
@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed release-note-label-needed labels Dec 19, 2016
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 19, 2016
@bgrant0607
Copy link
Member

A non-backward-compatible change? Was it previously deprecated?

@liggitt
Copy link
Member Author

liggitt commented Dec 19, 2016 •
edited
Loading

A non-backward-compatible change? Was it previously deprecated?

rbac is still in alpha, would like to cut a beta version in 1.6.

If there's significant concern about the compatibility aspect from v1alpha1, we can convert a binding to User * to a binding to Group system:authenticated (c.f. #38968)

@ericchiang
Copy link
Contributor

If there's significant concern about the compatibility aspect from v1alpha1, we can convert a binding to User * to a binding to Group system:authenticated (c.f. #38968)

I feel like users have to be relatively advanced to understand the exactly when to use system:authenticated vs system:unauthenticated. Would there be value in making "*" just "do the right thing" beyond backward compatibility? E.g. for users switching over from ABAC?

@liggitt
Copy link
Member Author

liggitt commented Dec 19, 2016

I don't think there's a clear "just do the right thing". Allowing both authenticated and unauthenticated users to read discovery docs seems ok. Allowing all authenticated to read certain API objects in some clusters could be ok. But I would want people to grant to the group they mean, not just hope the platform turns "*" into what they mean

@ericchiang
Copy link
Contributor

But I would want people to grant to the group they mean, not just hope the platform turns "*" into what they mean

sgtm

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 21, 2016
@liggitt
Copy link
Member Author

liggitt commented Dec 21, 2016

modified this to convert User * v1alpha1 role bindings to Group system:authenticated bindings and documented deprecation targeted for the v1beta1 release

@liggitt liggitt force-pushed the remove-rbac-user-all branch 2 times, most recently from a052180 to 7637741 Compare December 21, 2016 15:59
deads2k
deads2k reviewed Jan 4, 2017
View reviewed changes
pkg/apis/rbac/v1alpha1/conversion.go
// User * in v1alpha1 will only match all authenticated users
// This is only for compatibility with old RBAC bindings
// Special treatment for * should not be included in v1beta1
if out.Kind == UserKind && out.Name == "*" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll bet this does good things with kubectl apply

Copy link
Member Author

Choose a reason for hiding this comment

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

happens in conversion, so no matter how many times it's submitted as User *, it'll always persist as Group system:authenticated. A little odd, but I think it works

@deads2k
Copy link
Contributor

deads2k commented Jan 4, 2017

lgtm

@deads2k
Copy link
Contributor

deads2k commented Jan 4, 2017

After this we're ready for #38706, correct?

@liggitt
Copy link
Member Author

liggitt commented Jan 4, 2017 •
edited
Loading

After this we're ready for #38706, correct?

yes

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2017
@liggitt
Copy link
Member Author

liggitt commented Jan 4, 2017

@k8s-bot test this

@liggitt liggitt force-pushed the remove-rbac-user-all branch from 7637741 to cc07cb6 Compare January 4, 2017 21:56
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2017
@liggitt liggitt force-pushed the remove-rbac-user-all branch from cc07cb6 to b8c2ad6 Compare January 4, 2017 22:11
@liggitt
Copy link
Member Author

liggitt commented Jan 4, 2017

rebased

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2017
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 39408, 38981)

@k8s-github-robot k8s-github-robot merged commit a104229 into kubernetes:master Jan 5, 2017
@liggitt liggitt added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 5, 2017
@liggitt liggitt deleted the remove-rbac-user-all branch January 10, 2017 05:20
@liggitt liggitt mentioned this pull request Jan 17, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

Assignees

@ericchiang ericchiang

@bgrant0607 bgrant0607

@erictune erictune

@deads2k deads2k

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/auth Categorizes an issue or PR as relevant to SIG Auth. 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.
8 participants
@liggitt @k8s-reviewable @bgrant0607 @ericchiang @deads2k @k8s-github-robot @erictune @k8s-ci-robot