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

kubectl create {clusterrole,role}'s --resources flag support asterisk to specify all resources #62945

Merged
merged 1 commit into from
Aug 9, 2018
Merged

kubectl create {clusterrole,role}'s --resources flag support asterisk to specify all resources #62945

merged 1 commit into from
Aug 9, 2018

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Apr 22, 2018 •
edited
Loading

What this PR does / why we need it:

Currently kubectl create (cluster)role's --resources flag does not support asterisk to specify all resources.

# kubectl create clusterrole superrole --verb=get  --resource=*
the server doesn't have a resource type "*"

As an user, we create a role with --resources=* sometimes, so this PR supports it.

Fixes #62989

Special notes for your reviewer:

  • This patch does not support --resource=* for SpecialVerbs - e.g kubectl create role foo --verb=impersonate --resource=*, because current code also does not support kubectl create role foo --verb=impersonate --resource=users,pods

Release note:

`kubectl create {clusterrole,role}`'s `--resources` flag supports asterisk to specify all resources.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 22, 2018
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 22, 2018
@nak3 nak3 mentioned this pull request Apr 23, 2018
Closed
liggitt
liggitt reviewed Apr 24, 2018
View reviewed changes
pkg/kubectl/cmd/create/create_role.go Outdated
@@ -288,6 +293,9 @@ func (o *CreateRoleOptions) validateResource() error {
}

if err != nil {
if r.Resource == "*" && meta.IsNoMatchError(err) {
Copy link
Member

Choose a reason for hiding this comment

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

Check this first, rather than in response to an error

liggitt
liggitt reviewed Apr 24, 2018
View reviewed changes
pkg/kubectl/cmd/create/create_role.go
@@ -192,6 +192,11 @@ func (o *CreateRoleOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args
}
resource.Resource = parts[0]

if resource.Resource == "*" && len(parts) == 1 && len(sections) == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Check this before splitting into sections

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, actually, we probably want to allow * within a specific group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that this line is better to put here to support specific groups like *.extensions.

Copy link
Member

Choose a reason for hiding this comment

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

did you want to add a test for *.extensions?

Copy link
Contributor Author

@nak3 nak3 Aug 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I added it now.

@nak3
Copy link
Contributor Author

nak3 commented Apr 26, 2018

Updated (leave Line#195 as it is). I hope that I didn't misunderstand your second comment regarding specific group.

@dims
Copy link
Member

dims commented May 7, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 7, 2018
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 5, 2018
liggitt
liggitt reviewed Aug 8, 2018
View reviewed changes
pkg/kubectl/cmd/create/create_role.go Outdated
@@ -287,6 +292,9 @@ func (o *CreateRoleOptions) validateResource() error {
}
}

if r.Resource == "*" && meta.IsNoMatchError(err) {
Copy link
Member

Choose a reason for hiding this comment

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

skip the lookup above if resource is "*"?

@nak3
Copy link
Contributor Author

nak3 commented Aug 8, 2018

Thank you @liggitt Updated.

@liggitt
Copy link
Member

liggitt commented Aug 8, 2018

please squash, then lgtm

@nak3
Copy link
Contributor Author

nak3 commented Aug 9, 2018

Sure, thank you. Squashed.

@liggitt
Copy link
Member

liggitt commented Aug 9, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, nak3

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 67026, 62945, 66917). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit bd0de22 into kubernetes:master Aug 9, 2018
@nak3 nak3 deleted the all-resource-create-role branch August 9, 2018 04:17
@neolit123
Copy link
Member

/sig cli

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Aug 24, 2018
@suonto
Copy link
Contributor

suonto commented Sep 7, 2018

@liggitt could you please tell which release this feature will be in?

@liggitt
Copy link
Member

liggitt commented Sep 7, 2018

1.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@shiywang shiywang Awaiting requested review from shiywang

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. 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
@nak3 @dims @fejta-bot @liggitt @k8s-ci-robot @k8s-github-robot @neolit123 @suonto