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

support NonResourceURL for kubectl create clusterrole #45809

Merged
merged 1 commit into from
May 26, 2017
Merged

support NonResourceURL for kubectl create clusterrole #45809

merged 1 commit into from
May 26, 2017

Conversation

CaoShuFeng
Copy link
Contributor

@CaoShuFeng CaoShuFeng commented May 15, 2017 •
edited

Release note:

add --non-resource-url to kubectl create clusterrole

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 15, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels May 15, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @CaoShuFeng. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

@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 May 15, 2017
@eparis
Copy link
Contributor

eparis commented May 16, 2017

@k8s-bot 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 16, 2017
@eparis eparis added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 16, 2017
liggitt
liggitt reviewed May 16, 2017
View reviewed changes
pkg/kubectl/cmd/create_clusterrole.go Outdated
@@ -72,16 +77,46 @@ func NewCmdCreateClusterRole(f cmdutil.Factory, cmdOut io.Writer) *cobra.Command
cmdutil.AddPrinterFlags(cmd)
cmdutil.AddDryRunFlag(cmd)
cmd.Flags().StringSliceVar(&c.Verbs, "verb", []string{}, "verb that applies to the resources contained in the rule")
cmd.Flags().StringSliceVar(&c.NonResourceURLs, "prefix", []string{}, "a partial url that user should have access to.")
Copy link
Member

@liggitt liggitt May 16, 2017 •
edited

Choose a reason for hiding this comment

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

--prefix is misleading... /foo does not give access to /foo/bar. I would call this --non-resource-url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@CaoShuFeng CaoShuFeng force-pushed the non-resource-url-create-rolebinding branch from 6a094ac to ffc31a8 Compare May 17, 2017 08:13
@CaoShuFeng
Copy link
Contributor Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this

2 similar comments
@CaoShuFeng
Copy link
Contributor Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@CaoShuFeng
Copy link
Contributor Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this

liggitt
liggitt reviewed May 23, 2017
View reviewed changes
pkg/kubectl/cmd/create_role.go Outdated
@@ -336,6 +338,12 @@ func generateResourcePolicyRules(mapper meta.RESTMapper, verbs []string, resourc
rule.ResourceNames = resourceNames
rules = append(rules, rule)
}
for _, u := range nonResourceURLs {
rule := rbac.PolicyRule{}
rule.Verbs = verbs
Copy link
Member

Choose a reason for hiding this comment

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

verbs are going to be tricky if you are trying to create a role containing both resources and non-resource urls (the verbs that apply to those are different... see https://kubernetes.io/docs/admin/authorization/#review-your-request-attributes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

liggitt
liggitt reviewed May 23, 2017
View reviewed changes
hack/make-rules/test-cmd-util.sh
@@ -3018,6 +3018,9 @@ runTests() {
kube::test::get_object_assert clusterrole/resourcename-reader "{{range.rules}}{{range.resources}}{{.}}:{{end}}{{end}}" 'pods:'
kube::test::get_object_assert clusterrole/resourcename-reader "{{range.rules}}{{range.apiGroups}}{{.}}:{{end}}{{end}}" ':'
kube::test::get_object_assert clusterrole/resourcename-reader "{{range.rules}}{{range.resourceNames}}{{.}}:{{end}}{{end}}" 'foo:'
kubectl create "${kube_flags[@]}" clusterrole url-reader --verb=get --non-resource-url=/logs/* --non-resource-url=/healthz/*
Copy link
Member

Choose a reason for hiding this comment

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

surprised the * didn't need quoting

@liggitt
Copy link
Member

liggitt commented May 23, 2017

@kubernetes/sig-cli-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label May 23, 2017
@CaoShuFeng CaoShuFeng force-pushed the non-resource-url-create-rolebinding branch from ffc31a8 to 301cec6 Compare May 23, 2017 12:44
liggitt
liggitt reviewed May 25, 2017
View reviewed changes
pkg/kubectl/cmd/create_clusterrole.go Outdated
kubectl create clusterrole "foo" --verb=get --non-resource-url=/logs/*`))

// Valid nonResource verb list for validation.
validNonResourceVerbs = []string{"*", "get", "post", "put", "delete"}
Copy link
Member

Choose a reason for hiding this comment

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

include patch, head, and options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

liggitt
liggitt reviewed May 25, 2017
View reviewed changes
pkg/kubectl/cmd/create_role.go Outdated
@@ -336,6 +338,12 @@ func generateResourcePolicyRules(mapper meta.RESTMapper, verbs []string, resourc
rule.ResourceNames = resourceNames
rules = append(rules, rule)
}
for _, u := range nonResourceURLs {
Copy link
Member

Choose a reason for hiding this comment

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

all the nonResourceURLs can go in a single rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@CaoShuFeng CaoShuFeng force-pushed the non-resource-url-create-rolebinding branch from 301cec6 to a65052d Compare May 25, 2017 09:27
liggitt
liggitt reviewed May 25, 2017
View reviewed changes
pkg/kubectl/cmd/create_clusterrole.go Outdated

func (c *CreateClusterRoleOptions) Validate() error {
err := c.CreateRoleOptions.Validate()
if err == nonResourceErr {
Copy link
Member

Choose a reason for hiding this comment

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

depending on an error condition this way is a little odd... I'd factor out a validateResources() helper and use it from both Validate() methods, rather than delegate to CreateRoleOptions.Validate() and ignore specific returned errors. You'll end up duplicating the empty name and verbs check but that's not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@CaoShuFeng CaoShuFeng force-pushed the non-resource-url-create-rolebinding branch from a65052d to 93e50b1 Compare May 26, 2017 04:22
@liggitt
Copy link
Member

liggitt commented May 26, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017
@deads2k
Copy link
Contributor

deads2k commented May 26, 2017

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, deads2k, liggitt

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 899b6c0 into kubernetes:master May 26, 2017
@CaoShuFeng CaoShuFeng mentioned this pull request May 31, 2017
Merged
dims pushed a commit to dims/kubernetes that referenced this pull request Jun 2, 2017
Merge pull request kubernetes#46701 from CaoShuFeng/unit_clusterrole_…
b7a424f 
…binding

Automatic merge from submit-queue (batch tested with PRs 46432, 46701, 46326, 40848, 46396)

add some unit tests for "kubectl create clusterrole"

kubernetes#45809 adds support for non-resource-url to "kubectl create clusterrole"
This pr add some unit test for kubernetes#45809 

**Release note**:

```
NONE
```
@CaoShuFeng CaoShuFeng deleted the non-resource-url-create-rolebinding branch November 6, 2017 05:45
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

Assignees

@liggitt liggitt

@smarterclayton smarterclayton

@eparis eparis

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. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@CaoShuFeng @k8s-ci-robot @eparis @liggitt @deads2k @k8s-github-robot @smarterclayton