support NonResourceURL for kubectl create clusterrole #45809

CaoShuFeng · 2017-05-15T06:54:43Z

Release note:

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

k8s-ci-robot · 2017-05-15T06:54:50Z

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.

eparis · 2017-05-16T14:19:48Z

@k8s-bot ok to test

liggitt · 2017-05-16T20:45:14Z

pkg/kubectl/cmd/create_clusterrole.go

@@ -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.")


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

CaoShuFeng · 2017-05-17T09:49:15Z

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

CaoShuFeng · 2017-05-17T11:07:34Z

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

CaoShuFeng · 2017-05-22T03:08:39Z

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

liggitt · 2017-05-23T05:29:50Z

pkg/kubectl/cmd/create_role.go

@@ -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


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)

liggitt · 2017-05-23T05:30:07Z

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/*


surprised the * didn't need quoting

liggitt · 2017-05-23T05:31:00Z

@kubernetes/sig-cli-pr-reviews

liggitt · 2017-05-25T06:09:49Z

pkg/kubectl/cmd/create_clusterrole.go

+		kubectl create clusterrole "foo" --verb=get --non-resource-url=/logs/*`))
+
+	// Valid nonResource verb list for validation.
+	validNonResourceVerbs = []string{"*", "get", "post", "put", "delete"}


include patch, head, and options

liggitt · 2017-05-25T06:11:12Z

pkg/kubectl/cmd/create_role.go

@@ -336,6 +338,12 @@ func generateResourcePolicyRules(mapper meta.RESTMapper, verbs []string, resourc
 		rule.ResourceNames = resourceNames
 		rules = append(rules, rule)
 	}
+	for _, u := range nonResourceURLs {


all the nonResourceURLs can go in a single rule

liggitt · 2017-05-25T14:21:29Z

pkg/kubectl/cmd/create_clusterrole.go

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


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.

liggitt · 2017-05-26T16:30:29Z

/lgtm

deads2k · 2017-05-26T16:39:32Z

/approve

k8s-github-robot · 2017-05-26T16:39:40Z

[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:

~~hack/OWNERS~~ [deads2k]
~~pkg/kubectl/OWNERS~~ [deads2k,liggitt]

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 · 2017-05-26T23:47:06Z

Automatic merge from submit-queue

…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 ```

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 assigned smarterclayton and eparis May 15, 2017

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 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 assigned liggitt May 16, 2017

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 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 reviewed May 16, 2017

View reviewed changes

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

liggitt reviewed May 23, 2017

View reviewed changes

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label May 23, 2017

CaoShuFeng force-pushed the non-resource-url-create-rolebinding branch from ffc31a8 to 301cec6 Compare May 23, 2017 12:44

liggitt reviewed May 25, 2017

View reviewed changes

CaoShuFeng force-pushed the non-resource-url-create-rolebinding branch from 301cec6 to a65052d Compare May 25, 2017 09:27

liggitt reviewed May 25, 2017

View reviewed changes

support NonResourceURL for kubectl create clusterrole

93e50b1

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

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

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 merged commit 899b6c0 into kubernetes:master May 26, 2017

CaoShuFeng mentioned this pull request May 31, 2017

add some unit tests for "kubectl create clusterrole" #46701

Merged

CaoShuFeng deleted the non-resource-url-create-rolebinding branch November 6, 2017 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support NonResourceURL for kubectl create clusterrole #45809

support NonResourceURL for kubectl create clusterrole #45809

CaoShuFeng commented May 15, 2017 •

edited

Loading

k8s-ci-robot commented May 15, 2017

eparis commented May 16, 2017

liggitt May 16, 2017 •

edited

Loading

CaoShuFeng May 17, 2017

CaoShuFeng commented May 17, 2017

CaoShuFeng commented May 17, 2017

CaoShuFeng commented May 22, 2017

liggitt May 23, 2017

CaoShuFeng May 25, 2017

liggitt May 23, 2017

liggitt commented May 23, 2017

liggitt May 25, 2017

CaoShuFeng May 25, 2017

CaoShuFeng May 25, 2017

liggitt May 25, 2017

CaoShuFeng May 25, 2017

CaoShuFeng May 25, 2017

liggitt May 25, 2017

CaoShuFeng May 26, 2017

liggitt commented May 26, 2017

deads2k commented May 26, 2017

k8s-github-robot commented May 26, 2017

k8s-github-robot commented May 26, 2017

support NonResourceURL for kubectl create clusterrole #45809

support NonResourceURL for kubectl create clusterrole #45809

Conversation

CaoShuFeng commented May 15, 2017 • edited Loading

k8s-ci-robot commented May 15, 2017

eparis commented May 16, 2017

liggitt May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaoShuFeng commented May 17, 2017

CaoShuFeng commented May 17, 2017

CaoShuFeng commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented May 26, 2017

deads2k commented May 26, 2017

k8s-github-robot commented May 26, 2017

k8s-github-robot commented May 26, 2017

CaoShuFeng commented May 15, 2017 •

edited

Loading

liggitt May 16, 2017 •

edited

Loading