make oc login kubeconfig permission error clearer #9968

juanvallejo · 2016-07-20T22:17:28Z

Related BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1332131

When logging in with a $KUBECONFIG variable, or --config flag pointing to a location that denies write access, oc login prints a non-specific error at the bottom of its output:

export KUBECONFIG=/src
oc login --token=<my_token> --insecure-skip-tls-verify

Logged into "https://localhost:8443" as "test" using the token provided.

You have one project on this server: "test-project"

Using project "test-project".
error: open /src.lock: permission denied

This updates the error to be more clear as to what has happened:

oc login -u test -p test --insecure-skip-tls-verify

Login successful.

You have one project on this server: "myproject"

Using project "myproject".
error: KUBECONFIG is set to a file that cannot be created or modified: /src
Solution:

You can unset the KUBECONFIG variable to use the default location for it:
   unset KUBECONFIG

Or you can set its value to a file that can be written to:
   export KUBECONFIG=/path/to/file

Caused By:
  open /src.lock: permission denied

Same output, but using a --config flag:

oc login -u test -p test --insecure-skip-tls-verify --config=/src

Login successful.

You have one project on this server: "myproject"

Using project "myproject".
error: KUBECONFIG is set to a file that cannot be created or modified: /src
Solution:

Make sure that the value of the --config flag passed contains a valid path:
        --config=/path/to/valid/file

Caused By:
  open /src.lock: permission denied

juanvallejo · 2016-07-21T15:14:57Z

[test]

juanvallejo · 2016-07-21T15:15:52Z

@fabianofranz Could you please take a look? Thanks!

liggitt · 2016-07-21T15:18:34Z

pkg/cmd/cli/cmd/loginoptions.go

@@ -394,7 +394,11 @@ func (o *LoginOptions) SaveConfig() (bool, error) {
 	}

 	if err := kclientcmd.ModifyConfig(o.PathOptions, *configToWrite, true); err != nil {


Is the only error that could come back from this a file permission error

@liggitt hm, I could parse the error string to make sure it is a "permission denied" error before returning this message, and then otherwise return just the original error message. But I am not sure that the only error that comes back is file permission error.

Parsing error strings is a code smell

Hm, what if I type the errors in loader.go#WriteToFile and then return a message accordingly in loginoptions.go?

EDIT: turns out os.IsPermission works :p 0a85f23#diff-f260fb37cbe8b4d20167cc6bc6d4e51aR397

juanvallejo · 2016-07-21T17:41:01Z

[test]

stevekuznetsov · 2016-07-21T20:02:00Z

Check with @csrwng, he implemented a similar feature in oc cluster up recently.

csrwng · 2016-07-21T20:12:15Z

I added a check and error message in case your config is not writeable:
https://github.com/openshift/origin/blob/master/pkg/bootstrap/docker/up.go#L356

juanvallejo · 2016-07-21T20:34:01Z

@csrwng @fabianofranz I think @csrwng's error message for kubeconfig not being writable definitely would work for oc login, do you think it'd be better to just import github.com/openshift/origin/pkg/bootstrap/docker/errors to use it here, or simply copy it over?

fabianofranz · 2016-07-22T01:15:44Z

@juanvallejo importing works for me.

liggitt · 2016-07-22T01:26:51Z

I would move it if we intend to reuse it. I don't want login depending on docker bootstrap packages.

csrwng · 2016-07-22T18:10:49Z

@juanvallejo so in the cluster up code I check for the interfaces implemented by the error so I can print out additional details, solution, etc. In this case, it seems that would all make sense to be part of the error message, since that's what would get printed out, no?

stevekuznetsov · 2016-07-22T18:11:41Z

test/cmd/basicresources.sh

@@ -275,6 +275,11 @@ os::cmd::expect_success_and_not_text 'oc get is' 'is/test1'
 echo "resource printer: ok"
 os::test::junit::declare_suite_end

+os::test::junit::declare_suite_start "cmd/basicresources/login"
+os::cmd::expect_failure_and_text "oc login ${KUBERNETES_MASTER} -u test -p test --config=/src --insecure-skip-tls-verify" 'Failed to create kubeconfig under'


Quote your vars

os::cmd::expect_failure_and_text "oc login '${KUBERNETES_MASTER}' -u test ... ^ ^

juanvallejo · 2016-07-22T20:12:51Z

@csrwng

@juanvallejo so in the cluster up code I check for the interfaces implemented by the error so I can print out additional details, solution, etc

Thanks for pointing this out! I went ahead and added that in the printer.go file, as well as a FormatError function for cases where you're returning the final error with its message, rather than passing a writer interface.

csrwng · 2016-07-22T20:17:37Z

@juanvallejo you can implement FormatError by calling PrintError on a buffer and returning the buffer string.

One concern is that we don't usually display errors like this for other CLI commands.

juanvallejo · 2016-07-22T20:50:16Z

@csrwng

you can implement FormatError by calling PrintError on a buffer and returning the buffer string.

Sounds good

One concern is that we don't usually display errors like this for other CLI commands.

I went ahead and changed the format so that it resembles more a standard error msg:

Login successful.

You have one project on this server: "myproject"

Using project "myproject".
error: KUBECONFIG is set to a file that cannot be created or modified: /src
open /src.lock: permission denied

You can unset the KUBECONFIG variable to use the default location for it:
   unset KUBECONFIG

Or you can set its value to a file that can be written to:
   export KUBECONFIG=/path/to/file

juanvallejo · 2016-07-25T13:48:08Z

re[test]

fabianofranz · 2016-07-26T16:08:07Z

Looks like parts of this new errors structure (the interface, printer, internalError, etc) are generic enough to be used not only by oc but also other commands (oadm, openshift, etc). Should then be in a parent package like pkg/cmd/errors?

juanvallejo · 2016-07-26T16:44:29Z

@fabianofranz

Should then be in a parent package like pkg/cmd/errors?

I agree with this, I'll go ahead and update the PR

fabianofranz · 2016-07-26T21:10:48Z

test/cmd/authentication.sh

 os::test::junit::declare_suite_start "cmd/authentication"

 os::test::junit::declare_suite_start "cmd/authentication/scopedtokens"
 os::cmd::expect_success 'oadm policy add-role-to-user admin scoped-user'

+# make sure we handle invalid config file destination
+os::cmd::expect_failure_and_text "oc login '${KUBERNETES_MASTER}' -u test -p test --config=/src --insecure-skip-tls-verify" 'KUBECONFIG is set to a file that cannot be created or modified'


I believe this belongs to the suite you started in line 13 (which is doing nothing).

The rest of our tests for oc login are in the main body of hack/test-cmd.sh.

Although I have a PR in flight to move that location, I think it makes the most sense to colocate this test and those others. Whichever of us merges first will rebase.

Sounds good, I will update the PR with these changes

juanvallejo · 2016-07-27T14:40:09Z

@deads2k @fabianofranz

If you're going to use this type to collapse the pkg/bootstrap/docker usage, then it can live here. If its only call site is pkg/cmd/cli/cmd, then it should live there.

Sounds good, since errors/login.go is only used by pkg/cmd/cli/cmd/loginoptions.go, I have moved it to pkg/cmd/cli/cmd/errors

fabianofranz · 2016-07-27T16:10:15Z

@juanvallejo good to go?

juanvallejo · 2016-07-27T16:11:06Z

@fabianofranz Yeah, everything looks good to me

openshift-bot · 2016-07-27T16:20:27Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7026/) (Image: devenv-rhel7_4690)

deads2k · 2016-07-27T17:37:29Z

@juanvallejo could use a squash

juanvallejo · 2016-07-27T17:59:37Z

@deads2k

could use a squash

Done!

fabianofranz · 2016-07-27T18:00:17Z

[merge]

openshift-bot · 2016-07-27T18:05:40Z

Evaluated for origin test up to ce8595f

openshift-bot · 2016-07-27T19:21:34Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7001/)

openshift-bot · 2016-07-28T00:10:26Z

Evaluated for origin merge up to ce8595f

juanvallejo force-pushed the jvallejo_update-kubeconfig-permission-error branch from ec5b7eb to 4510817 Compare July 21, 2016 15:14

liggitt reviewed Jul 21, 2016
View reviewed changes

juanvallejo force-pushed the jvallejo_update-kubeconfig-permission-error branch from 4510817 to 0a85f23 Compare July 21, 2016 17:14

stevekuznetsov reviewed Jul 22, 2016
View reviewed changes

juanvallejo force-pushed the jvallejo_update-kubeconfig-permission-error branch from 2e003dd to ac74302 Compare July 22, 2016 20:06

juanvallejo force-pushed the jvallejo_update-kubeconfig-permission-error branch from ac74302 to ea204df Compare July 22, 2016 20:46

juanvallejo force-pushed the jvallejo_update-kubeconfig-permission-error branch from ea204df to c9f443d Compare July 22, 2016 20:51

juanvallejo force-pushed the jvallejo_update-kubeconfig-permission-error branch 3 times, most recently from aac0667 to 2e709a5 Compare July 26, 2016 19:13

fabianofranz reviewed Jul 26, 2016
View reviewed changes

make oc login kubeconfig permission error clearer

ce8595f

juanvallejo force-pushed the jvallejo_update-kubeconfig-permission-error branch from f3354e7 to ce8595f Compare July 27, 2016 17:59

openshift-bot merged commit 75fce88 into openshift:master Jul 28, 2016

juanvallejo deleted the jvallejo_update-kubeconfig-permission-error branch July 28, 2016 22:07

juanvallejo mentioned this pull request Apr 11, 2017

Misleading error message creating project when $KUBECONFIG is read-only #3918

Closed

		@@ -394,7 +394,11 @@ func (o *LoginOptions) SaveConfig() (bool, error) {
		}

		if err := kclientcmd.ModifyConfig(o.PathOptions, *configToWrite, true); err != nil {

make oc login kubeconfig permission error clearer #9968

make oc login kubeconfig permission error clearer #9968

Uh oh!

Conversation

juanvallejo commented Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juanvallejo commented Jul 21, 2016

Uh oh!

juanvallejo commented Jul 21, 2016

Uh oh!

liggitt Jul 21, 2016

Choose a reason for hiding this comment

Uh oh!

juanvallejo Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Jul 21, 2016

Choose a reason for hiding this comment

Uh oh!

juanvallejo Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanvallejo commented Jul 21, 2016

Uh oh!

stevekuznetsov commented Jul 21, 2016

Uh oh!

csrwng commented Jul 21, 2016

Uh oh!

juanvallejo commented Jul 21, 2016

Uh oh!

fabianofranz commented Jul 22, 2016

Uh oh!

liggitt commented Jul 22, 2016

Uh oh!

csrwng commented Jul 22, 2016

Uh oh!

stevekuznetsov Jul 22, 2016

Choose a reason for hiding this comment

Uh oh!

juanvallejo commented Jul 22, 2016

Uh oh!

csrwng commented Jul 22, 2016

Uh oh!

juanvallejo commented Jul 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juanvallejo commented Jul 25, 2016

Uh oh!

fabianofranz commented Jul 26, 2016

Uh oh!

juanvallejo commented Jul 26, 2016

Uh oh!

fabianofranz Jul 26, 2016

Choose a reason for hiding this comment

Uh oh!

stevekuznetsov Jul 26, 2016

Choose a reason for hiding this comment

Uh oh!

juanvallejo Jul 26, 2016

Choose a reason for hiding this comment

Uh oh!

juanvallejo commented Jul 27, 2016

Uh oh!

fabianofranz commented Jul 27, 2016

Uh oh!

juanvallejo commented Jul 27, 2016

Uh oh!

openshift-bot commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k commented Jul 27, 2016

Uh oh!

juanvallejo commented Jul 27, 2016

Uh oh!

fabianofranz commented Jul 27, 2016

Uh oh!

openshift-bot commented Jul 27, 2016

juanvallejo commented Jul 20, 2016 •

edited

Loading

juanvallejo Jul 21, 2016 •

edited

Loading

juanvallejo Jul 21, 2016 •

edited

Loading

juanvallejo commented Jul 22, 2016 •

edited

Loading

openshift-bot commented Jul 27, 2016 •

edited

Loading