[WIP] Raise error if the user has multiple contexts but no current set #94905

oz123 · 2020-09-18T21:51:04Z

The current behavior is to ignore this and try to connect to localhost:8080.
This is quite confusing, as reported in this issue.

This commit improves the UX by telling the users they need to explicitly choose a context, if there are multiple contexts:

$ kubectl get pods
error: Multiple contexts found. However, no current-context is set in the configuration

What type of PR is this?
/kind bug

What this PR does / why we need it:
This is an attept to improve a confusing error message. Please see the issue linked.

Which issue(s) this PR fixes:
Fixes kubernetes/kubectl#936

Special notes for your reviewer:

I am aware that this PR is still missing test cases. However, I'm publishing this to solicit feedback, and I do not expect this to be merged at this state for the reasons explained below.
With the current patch, if you have not set a context, you see a clear error message. However, you can now only use an editor to fix the issue. That is because this patch disables the ability to use kubeconfig config set-context.

$ ./kubectl config use-context foo
error: Multiple contexts found. However, no current-context is set in the configuration

Users who are a bit familiar with the shell can circumvent the problem:

$ echo "current-context: foo" >> ~/.kube/config

IMHO, this would be a better interface to exclude the config sub-command from producing this error. There are two possible solutions I could think of:

Pass the sub-command to the config validator.
Check for the number of contexts in each individual command, e.g. modify all entry points in kubernetes/vendor/k8s.io/kubectl/pkg/cmd.

I haven't implemented any yet. It's quite obvious that both approaches make the PR significantly larger. The latter approach makes even larger than the former approach.

Maybe there is another way I have not thought of or not aware of?
I would be happy to get some feedback.

In any case, once I know which approach is preferred I will add the required changes and include tests.

Does this PR introduce a user-facing change?:

kubectl will now raise an error if the user has multiple contexts but no current set.
Previously, it simply tried to connect to localhost:8080.

k8s-ci-robot · 2020-09-18T21:52:08Z

Hi @oz123. 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 /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

dougsland · 2020-09-22T16:28:20Z

looks sane to me. Just wondering why the title is WIP? Still more progress to made related to this RFE? @oz123

oz123 · 2020-09-22T16:42:19Z

@dougsland well, it's WIP, because I think there is still some work to do.
For the following reasons:

I didn't add any tests
I was not sure about the implementation, hence I was thinking out loud in my comment.

I am OK if it's merged like this, but I'm not sure it will be accepted.

dougsland · 2020-09-22T16:45:50Z

@dougsland well, it's WIP, because I think there is still some work to do.
For the following reasons:

I didn't add any tests

I was not sure about the implementation, hence I was thinking out loud in my comment.

I agree we might useful to add a test here.

I am OK if it's merged like this, but I'm not sure it will be accepted.

I can't merge patches but let me try to dig here and help moving this one forward.

fedebongio · 2020-09-22T20:08:07Z

/remove-sig api-machinery
/sig cli

dougsland

just double checked, this is the correct repo to send the PR, my bad. The client-go is a mirror.

staging/src/k8s.io/client-go/tools/clientcmd/loader.go

dougsland · 2020-09-23T00:12:21Z

@ahmetb @corneliusweig @soltysh might help here.

oz123 · 2020-09-23T00:22:13Z

@douglasland thanks 😊 you beat me to respond. I was trying to comment on this.

corneliusweig · 2020-09-23T06:49:48Z

It's not an option to break kubectl config set-context. Maybe you can inspect the error list that is returned from the loader in the config subcommand and filter out the error you added here?

The current behavior is to ignore this and try to connect to localhost:8080. [This is quite confusing, as reported in this issue][1]. This commit improves the UX by telling the users they need to explicitly choose a context in the that multiple context exist: ``` $ kubectl get pods error: Multiple contexts found. However, no current-context is set in the configuration ``` [1]: kubernetes/kubectl#936

To enable this sub command, it was necessary to propagate the error further up the calling chain for inspection. All the subcommands of config, can now look at the error and decide whether the can continue or not. As the only command that should still work is "use-context" they all stop with error if a context is not set. But this was not enough, since the config is also read while doing the actual write, it was needed to check for the actually command which was running. Hence, the signature of `ModifyConfig` and `writeCurrentContext` was updated. If the use-context subcommand is called the last argument is set to `true`, this signals that it's OK to ignore the error `NoConextSet` and continue the execution. This set of changes displays the error: ``` error: Multiple contexts found. However, no current-context is set in the configuration ``` If running, for example, `kubectl get pods`, but it still allows one to `get-contexts` and `use-context` quickly from the command line to fix the probelm. Demo: ``` $ ./kubectl get pods error: Multiple contexts found. However, no current-context is set in the configuration $ ./kubectl config get-contexts CURRENT NAME CLUSTER AUTHINFO NAMESPACE coldsweet microk8s-cluster admin-microk8s coldsweet devops devops-calico admin devops-cilium devops-cilium admin-cilium microk8s-context microk8s-cluster admin-microk8s $ ./kubectl config use-context devops Switched to context "devops". ./kubectl get pods NAME READY STATUS RESTARTS AGE nginx-blue-9cf945578-6gsmz 1/1 Running 0 20d nginx-green-df5fcc8d5-xk7ww 1/1 Running 0 20d ```

corneliusweig

Your change has led to some fundamental changes. I think it's better to iterate on it via an issue than a PR to avoid wasting time. The go-to person for these questions are @.soltysh or @.seans3.

corneliusweig · 2020-09-23T06:39:44Z

staging/src/k8s.io/client-go/tools/clientcmd/validation.go

-	ErrNoContext   = errors.New("no context chosen")
-	ErrEmptyConfig = NewEmptyConfigError("no configuration has been provided, try setting KUBERNETES_MASTER environment variable")
+	ErrNoContext    = errors.New("no context chosen")
+	ErrNoContextSet = errors.New("Multiple contexts found. However, no current-context is set in the configuration")


Suggested change

ErrNoContextSet = errors.New("Multiple contexts found. However, no current-context is set in the configuration")

ErrNoContextSet = errors.New("multiple contexts found, but no current-context is set")

Go errors should not be sentences, because it reads better if they are chained. Therefore also start in lower case.

corneliusweig · 2020-09-23T20:43:16Z

staging/src/k8s.io/client-go/tools/clientcmd/config.go

 	if err != nil {
+		if errors.Is(err, ErrNoContextSet) {
+			return &rawConfig, err
+		}
 		return nil, err
 	}


Suggested change

if err != nil {

if errors.Is(err, ErrNoContextSet) {

return &rawConfig, err

}

return nil, err

}

if err != nil && !errors.Is(err, ErrNoContextSet) {

return nil, err

}

corneliusweig · 2020-09-23T20:45:23Z

staging/src/k8s.io/client-go/tools/clientcmd/loader.go

@@ -244,6 +244,9 @@ func (rules *ClientConfigLoadingRules) Load() (*clientcmdapi.Config, error) {
 	mergo.MergeWithOverwrite(config, mapConfig)
 	mergo.MergeWithOverwrite(config, nonMapConfig)

+	if (len(config.Contexts) > 1 && config.CurrentContext == "") {


Suggested change

if (len(config.Contexts) > 1 && config.CurrentContext == "") {

if len(config.Contexts) > 0 && config.CurrentContext == "" {

This seems to be the wrong place to add this logic though. The loaders responsibility is to load stuff, not validate that the loaded file satisfies some invariants. This forced you into making a lot more error assertions/exception to compensate for this premature check.

I think the better place is to add it in ClientConfigLoadingRules

kubernetes/staging/src/k8s.io/client-go/tools/clientcmd/loader.go

Line 247 in 323f348

if rules.ResolvePaths() {

corneliusweig · 2020-09-23T20:47:11Z

staging/src/k8s.io/client-go/tools/clientcmd/config.go

@@ -164,7 +167,7 @@ func NewDefaultPathOptions() *PathOptions {
 // (no nil strings), we're forced have separate handling for them.  In the kubeconfig cases, newConfig should have at most one difference,
 // that means that this code will only write into a single file.  If you want to relativizePaths, you must provide a fully qualified path in any
 // modified element.
-func ModifyConfig(configAccess ConfigAccess, newConfig clientcmdapi.Config, relativizePaths bool) error {
+func ModifyConfig(configAccess ConfigAccess, newConfig clientcmdapi.Config, relativizePaths bool, isSetContext bool) error {


I'm not sure why the isSetContext parameter was introduced. I'd assume you can remove it and the logic works just fine.

This is changing external API in a library we provide, you can't do that b/c you're breaking all the existing users.

corneliusweig · 2020-09-23T21:25:15Z

staging/src/k8s.io/client-go/tools/clientcmd/config.go

+
+		err != nil {
+			if isSetContext && errors.Is(err, ErrNoContextSet) {
+


empty if blocks are unidiomatic in Go. Try to refactor.

corneliusweig · 2020-09-23T21:26:57Z

staging/src/k8s.io/client-go/tools/clientcmd/config.go

 	if err != nil {
-		return err
+		if isSetContext && errors.Is(err, ErrNoContextSet) {


You need to be careful with errors.Is(). It returns true if at least one of the reported errors is the respective instance. However, please think about what should happen, if multiple errors are returned in the aggregate.

My intent here was to keep the old behavior. Other errors should be reported as before. I added isSetContext, so one can still use set-context. I tried explaining this in the commit message.
Without this, the check I added caused an error on all the commands.
I will need some time to think of an alternative without isSetContext.

Maybe I missed some detail why this is necessary. However, my main point is to discuss this change in an issue before investing more time into the implementation. It's possible that there is a reason for the current behavior that I don't know.

I understand that the change should be discussed in an issue first. This is why I created one and initially sent a very minimal patch.
Can the discussion for this issue happen on the original issue I opened in the kubectl repository or should I open a new issue in this repository?

soltysh

The rest of the comments from Cornelius still hold.

soltysh · 2020-10-28T16:43:27Z

staging/src/k8s.io/client-go/tools/clientcmd/config.go

@@ -164,7 +167,7 @@ func NewDefaultPathOptions() *PathOptions {
 // (no nil strings), we're forced have separate handling for them.  In the kubeconfig cases, newConfig should have at most one difference,
 // that means that this code will only write into a single file.  If you want to relativizePaths, you must provide a fully qualified path in any
 // modified element.
-func ModifyConfig(configAccess ConfigAccess, newConfig clientcmdapi.Config, relativizePaths bool) error {
+func ModifyConfig(configAccess ConfigAccess, newConfig clientcmdapi.Config, relativizePaths bool, isSetContext bool) error {


This is changing external API in a library we provide, you can't do that b/c you're breaking all the existing users.

k8s-ci-robot · 2020-10-28T16:46:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: oz123
To complete the pull request process, please assign smarterclayton after the PR has been reviewed.
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

The full list of commands accepted by this bot can be found 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

fejta-bot · 2021-01-26T17:06:42Z

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

fejta-bot · 2021-02-25T17:52:26Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

fejta-bot · 2021-03-27T18:38:32Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

k8s-ci-robot · 2021-03-27T18:38:39Z

@oz123: PR needs rebase.

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.

k8s-ci-robot · 2021-03-27T18:38:41Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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.

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 Sep 18, 2020

k8s-ci-robot requested review from sttts and yliaog September 18, 2020 21:52

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 18, 2020

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Sep 22, 2020

dougsland reviewed Sep 23, 2020

View reviewed changes

staging/src/k8s.io/client-go/tools/clientcmd/loader.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 23, 2020

oz123 force-pushed the raise-error-when-no-context-is-set branch from b8ed01e to aa74aaa Compare September 23, 2020 15:56

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 23, 2020

oz123 added 2 commits September 23, 2020 17:59

oz123 force-pushed the raise-error-when-no-context-is-set branch from aa74aaa to 3c6fe02 Compare September 23, 2020 16:00

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2020

corneliusweig reviewed Sep 23, 2020

View reviewed changes

soltysh requested changes Oct 28, 2020

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 26, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 25, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2021

k8s-ci-robot closed this Mar 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Raise error if the user has multiple contexts but no current set #94905

[WIP] Raise error if the user has multiple contexts but no current set #94905

oz123 commented Sep 18, 2020

k8s-ci-robot commented Sep 18, 2020

dougsland commented Sep 22, 2020

oz123 commented Sep 22, 2020

dougsland commented Sep 22, 2020

fedebongio commented Sep 22, 2020

dougsland left a comment

dougsland commented Sep 23, 2020 •

edited

oz123 commented Sep 23, 2020

corneliusweig commented Sep 23, 2020

corneliusweig left a comment

corneliusweig Sep 23, 2020

corneliusweig Sep 23, 2020

corneliusweig Sep 23, 2020

corneliusweig Sep 23, 2020

soltysh Oct 28, 2020

corneliusweig Sep 23, 2020

corneliusweig Sep 23, 2020

oz123 Sep 24, 2020

corneliusweig Sep 27, 2020

oz123 Sep 27, 2020

soltysh left a comment

soltysh Oct 28, 2020

k8s-ci-robot commented Oct 28, 2020

fejta-bot commented Jan 26, 2021

fejta-bot commented Feb 25, 2021

fejta-bot commented Mar 27, 2021

k8s-ci-robot commented Mar 27, 2021

k8s-ci-robot commented Mar 27, 2021

	ErrNoContextSet = errors.New("Multiple contexts found. However, no current-context is set in the configuration")
	ErrNoContextSet = errors.New("multiple contexts found, but no current-context is set")

	if (len(config.Contexts) > 1 && config.CurrentContext == "") {
	if len(config.Contexts) > 0 && config.CurrentContext == "" {


		err != nil {
		if isSetContext && errors.Is(err, ErrNoContextSet) {

[WIP] Raise error if the user has multiple contexts but no current set #94905

[WIP] Raise error if the user has multiple contexts but no current set #94905

Conversation

oz123 commented Sep 18, 2020

k8s-ci-robot commented Sep 18, 2020

dougsland commented Sep 22, 2020

oz123 commented Sep 22, 2020

dougsland commented Sep 22, 2020

fedebongio commented Sep 22, 2020

dougsland left a comment

Choose a reason for hiding this comment

dougsland commented Sep 23, 2020 • edited

oz123 commented Sep 23, 2020

corneliusweig commented Sep 23, 2020

corneliusweig left a 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

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

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 28, 2020

fejta-bot commented Jan 26, 2021

fejta-bot commented Feb 25, 2021

fejta-bot commented Mar 27, 2021

k8s-ci-robot commented Mar 27, 2021

k8s-ci-robot commented Mar 27, 2021

dougsland commented Sep 23, 2020 •

edited