Add exit code 1 on not allowed to kubectl auth can-i #59579

fbac · 2018-02-08T15:48:46Z

What this PR does / why we need it:
kubectl auth can-i verb resource always returns 0 status, even if the user can't

With this commit, kubectl will return exit code 1 when a verb is not allowed. It doesn't affect quiet option.

Release note:

Fix kubectl auth can-i exit code. It will return 1 if the user is not allowed and 0 if it's allowed.

k8s-ci-robot · 2018-02-08T15:48:47Z

@fbac: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

ericchiang · 2018-02-08T18:43:53Z

I think "can-i" answering "no" is still a successful result. For example:

$ kubectl can-i --as=system:anonymous get pods
no

If you get a non-zero exit code, can-i could also have failed. I don't think that'd be the correct way to detect if a user didn't have access.

fbac · 2018-02-08T19:08:56Z

When can-i fails it raises an error with CheckErr. This exit 1 only happens if err == nil

In my opinion, current behavior is a bit inconsistent. With the same non-allowed query it returns different exit codes
kubectl auth can-i verb resource --quiet returns exit code 1
kubectl auth can-i verb resource returns exit code 0

With this little fix, it will return 1 always when not allowed, 0 when allowed. It doesn affect --quiet nor the CheckErr if something went bad.

ericchiang · 2018-02-08T19:36:38Z

In my opinion, current behavior is a bit inconsistent. With the same non-allowed query it returns different exit codes
kubectl auth can-i verb resource --quiet returns exit code 1
kubectl auth can-i verb resource returns exit code 0

This seems intentional: #43900

This is minor, but the current behavior makes sense to me.

Interactive -> you view the output
Non-interactive -> have to go off of return code

/ok-to-test
cc @kubernetes/sig-auth-pr-reviews in case others have opinions here.

fbac · 2018-02-09T08:22:42Z

/test pull-kubernetes-bazel-test

fbac · 2018-04-08T12:00:59Z

@dims @ghodss hi, just wanted to know if I'm missing something in this PR to get it reviewed. Thanks!

fbac · 2018-04-26T13:49:35Z

Hi, can anybody in @kubernetes/sig-auth-pr-reviews review this?

k8s-ci-robot · 2018-04-26T13:49:43Z

@fbac: Reiterating the mentions to trigger a notification:
@kubernetes/sig-auth-pr-reviews

In response to this:

Hi, can anybody in @kubernetes/sig-auth-pr-reviews review this?

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.

dims · 2018-04-30T13:06:16Z

/release-note-none

dims · 2018-05-03T00:38:22Z

/retest

fbac · 2018-05-04T16:46:37Z

PTAL @soltysh can you review this?

liggitt · 2018-05-10T14:34:48Z

this change makes sense to me. needs a test and a release note

fbac · 2018-05-11T11:59:22Z

/retest

liggitt · 2018-05-11T13:31:57Z

pkg/kubectl/cmd/auth/cani_test.go

@@ -166,6 +174,11 @@ func TestRunAccessCheck(t *testing.T) {
 			}

 			actualAllowed, err := test.o.RunAccessCheck()
+			actualReturnCode := 0
+			if !actualAllowed {
+				actualReturnCode = 1


this isn't really testing your change... this test would pass even without your change. I expected something in test-cmd-util.sh that actually checked exit code

You're completely right. I've written an small test in test-cmd-util.sh, but becase the apiserver is always in AlwaysAllow during tests, does it makes sense? It will never return exit 1.

hrmm... true. I guess we'll skip that for now and go with manual testing.

fbac · 2018-05-11T15:44:36Z

/retest

soltysh · 2018-05-11T19:48:38Z

hack/make-rules/test-cmd-util.sh

@@ -5219,6 +5219,12 @@ runTests() {

    output_message=$(kubectl auth can-i list jobs.batch/bar -n foo --quiet 2>&1 "${kube_flags[@]}")
    kube::test::if_empty_string "${output_message}"
+
+    output_message=$(kubectl auth can-i get pods --subresource=log 2>&1 "${kube_flags[@]}"; echo $?)


This is still not testing your particular change. Can you add a test for your case, iow. one that will return non-zero when not allowed?

Actually there is no way to test kubectl returning exit code 1, since the apiserver is in AlwaysAllow, how can we test this?

Yeah, we'll have to live with a manual test. See #59579 (comment)

@soltysh so what changes should I make? Since there is no way to test error exit 1 I think the commit is good as it is now.

soltysh

/lgtm
/approve

k8s-ci-robot · 2018-06-05T08:40:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fbac, soltysh

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:

~~hack/OWNERS~~ [soltysh]
~~pkg/kubectl/OWNERS~~ [soltysh]

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

soltysh · 2018-06-05T08:41:26Z

/sig cli
/status approved-for-milestone
/kind bug
/priority important-longterm

k8s-ci-robot · 2018-06-05T08:41:27Z

You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to add status labels.

k8s-github-robot · 2018-06-05T08:42:14Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@fbac @soltysh

Pull Request Labels

sig/auth sig/cli: Pull Request will be escalated to these SIGs if needed.
priority/important-longterm: Escalate to the pull request owners; move out of the milestone after 1 attempt.
kind/bug: Fixes a bug discovered during the current release.

Help

fbac · 2018-06-05T11:11:14Z

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-bazel-test

k8s-github-robot · 2018-06-05T13:06:11Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-06-05T14:01:06Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from dims and ghodss February 8, 2018 15:48

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 Feb 8, 2018

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 8, 2018

fbac mentioned this pull request Feb 27, 2018

"oc auth can-i" command should not exit 0 if the user can't execute a command openshift/origin#17812

Closed

dims approved these changes Apr 30, 2018

View reviewed changes

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 30, 2018

fbac force-pushed the kubectl-cani-exitcode branch from 3990295 to 67e2d30 Compare May 11, 2018 09:11

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 11, 2018

fbac force-pushed the kubectl-cani-exitcode branch from 67e2d30 to 28dfbdd Compare May 11, 2018 09:39

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 11, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 11, 2018

liggitt reviewed May 11, 2018

View reviewed changes

fbac force-pushed the kubectl-cani-exitcode branch from c6fb9c1 to d2ce05d Compare May 11, 2018 14:40

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 11, 2018

Fix kubectl auth can-i exit errcode

9fa269a

fbac force-pushed the kubectl-cani-exitcode branch from d2ce05d to 9fa269a Compare May 11, 2018 14:41

soltysh requested changes May 11, 2018

View reviewed changes

soltysh added this to the v1.11 milestone Jun 5, 2018

soltysh approved these changes Jun 5, 2018

View reviewed changes

k8s-ci-robot assigned soltysh Jun 5, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2018

k8s-github-robot added the milestone/incomplete-labels label Jun 5, 2018

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Jun 5, 2018

soltysh added the status/approved-for-milestone label Jun 5, 2018

k8s-github-robot removed the milestone/incomplete-labels label Jun 5, 2018

k8s-github-robot merged commit fa20f87 into kubernetes:master Jun 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exit code 1 on not allowed to kubectl auth can-i #59579

Add exit code 1 on not allowed to kubectl auth can-i #59579

fbac commented Feb 8, 2018 •

edited

k8s-ci-robot commented Feb 8, 2018

ericchiang commented Feb 8, 2018

fbac commented Feb 8, 2018

ericchiang commented Feb 8, 2018

fbac commented Feb 9, 2018

fbac commented Apr 8, 2018

fbac commented Apr 26, 2018

k8s-ci-robot commented Apr 26, 2018

dims commented Apr 30, 2018

dims commented May 3, 2018

fbac commented May 4, 2018

liggitt commented May 10, 2018

fbac commented May 11, 2018

liggitt May 11, 2018

fbac May 11, 2018

liggitt May 11, 2018

fbac commented May 11, 2018

soltysh May 11, 2018

fbac May 11, 2018

liggitt May 11, 2018

fbac Jun 4, 2018

soltysh left a comment

k8s-ci-robot commented Jun 5, 2018

soltysh commented Jun 5, 2018 •

edited

k8s-ci-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

fbac commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

Add exit code 1 on not allowed to kubectl auth can-i #59579

Add exit code 1 on not allowed to kubectl auth can-i #59579

Conversation

fbac commented Feb 8, 2018 • edited

k8s-ci-robot commented Feb 8, 2018

ericchiang commented Feb 8, 2018

fbac commented Feb 8, 2018

ericchiang commented Feb 8, 2018

fbac commented Feb 9, 2018

fbac commented Apr 8, 2018

fbac commented Apr 26, 2018

k8s-ci-robot commented Apr 26, 2018

dims commented Apr 30, 2018

dims commented May 3, 2018

fbac commented May 4, 2018

liggitt commented May 10, 2018

fbac commented May 11, 2018

liggitt May 11, 2018

Choose a reason for hiding this comment

fbac May 11, 2018

Choose a reason for hiding this comment

liggitt May 11, 2018

Choose a reason for hiding this comment

fbac commented May 11, 2018

soltysh May 11, 2018

Choose a reason for hiding this comment

fbac May 11, 2018

Choose a reason for hiding this comment

liggitt May 11, 2018

Choose a reason for hiding this comment

fbac Jun 4, 2018

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 5, 2018

soltysh commented Jun 5, 2018 • edited

k8s-ci-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

fbac commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

fbac commented Feb 8, 2018 •

edited

soltysh commented Jun 5, 2018 •

edited