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

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a96d3b
Raise error if the user has multiple contexts but no current set
oz123 Sep 18, 2020
3c6fe02
Allow setting a context when the config does not have one
oz123 Sep 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 25 additions & 8 deletions staging/src/k8s.io/client-go/tools/clientcmd/config.go
View file
Open in desktop
Expand Up @@ -101,6 +101,9 @@ func (o *PathOptions) GetStartingConfig() (*clientcmdapi.Config, error) {
return clientcmdapi.NewConfig(), nil
}
if err != nil {
if errors.Is(err, ErrNoContextSet) {
return &rawConfig, err
}
return nil, err
}
Comment on lines 103 to 108
Copy link
Contributor

Choose a reason for hiding this comment

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

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
}


Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if UseModifyConfigLock {
possibleSources := configAccess.GetLoadingPrecedence()
// sort the possible kubeconfig files so we always "lock" in the same order
Expand All @@ -179,8 +182,13 @@ func ModifyConfig(configAccess ConfigAccess, newConfig clientcmdapi.Config, rela
}

startingConfig, err := configAccess.GetStartingConfig()

if err != nil {
return err
if isSetContext && errors.Is(err, ErrNoContextSet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


} else {
return err
}
}

// We need to find all differences, locate their original files, read a partial config to modify only that stanza and write out the file.
Expand All @@ -191,8 +199,14 @@ func ModifyConfig(configAccess ConfigAccess, newConfig clientcmdapi.Config, rela
}

if startingConfig.CurrentContext != newConfig.CurrentContext {
if err := writeCurrentContext(configAccess, newConfig.CurrentContext); err != nil {
return err
if err := writeCurrentContext(configAccess, newConfig.CurrentContext, isSetContext);

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

} else {
return err
}
}
}

Expand Down Expand Up @@ -371,7 +385,7 @@ func (p *persister) Persist(config map[string]string) error {
authInfo, ok := newConfig.AuthInfos[p.user]
if ok && authInfo.AuthProvider != nil {
authInfo.AuthProvider.Config = config
ModifyConfig(p.configAccess, *newConfig, false)
ModifyConfig(p.configAccess, *newConfig, false, false)
}
return nil
}
Expand All @@ -380,9 +394,12 @@ func (p *persister) Persist(config map[string]string) error {
// If newCurrentContext is the same as the startingConfig's current context, then we exit.
// If newCurrentContext has a value, then that value is written into the default destination file.
// If newCurrentContext is empty, then we find the config file that is setting the CurrentContext and clear the value from that file
func writeCurrentContext(configAccess ConfigAccess, newCurrentContext string) error {
if startingConfig, err := configAccess.GetStartingConfig(); err != nil {
return err
func writeCurrentContext(configAccess ConfigAccess, newCurrentContext string, isSetContext bool) error {
startingConfig, err := configAccess.GetStartingConfig()
if err != nil {
if !(isSetContext == true && errors.Is(err, ErrNoContextSet)){
return err
}
} else if startingConfig.CurrentContext == newCurrentContext {
return nil
}
Expand Down
3 changes: 3 additions & 0 deletions staging/src/k8s.io/client-go/tools/clientcmd/loader.go
View file
Open in desktop
Expand Up @@ -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 == "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

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() {

errlist = append(errlist, ErrNoContextSet)
}
if rules.ResolvePaths() {
if err := ResolveLocalPaths(config); err != nil {
errlist = append(errlist, err)
Expand Down
17 changes: 13 additions & 4 deletions staging/src/k8s.io/client-go/tools/clientcmd/merged_client_builder.go
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package clientcmd

import (
"errors"
"io"
"sync"

Expand Down Expand Up @@ -67,7 +68,7 @@ func (config *DeferredLoadingClientConfig) createClientConfig() (ClientConfig, e
return config.clientConfig, nil
}
mergedConfig, err := config.loader.Load()
if err != nil {
if err != nil && !errors.Is(err, ErrNoContextSet){
return nil, err
}

Expand All @@ -80,15 +81,23 @@ func (config *DeferredLoadingClientConfig) createClientConfig() (ClientConfig, e
} else {
config.clientConfig = NewNonInteractiveClientConfig(*mergedConfig, currentContext, config.overrides, config.loader)
}
return config.clientConfig, nil
if err != nil {
return config.clientConfig, err
} else {
return config.clientConfig, nil
}
}

func (config *DeferredLoadingClientConfig) RawConfig() (clientcmdapi.Config, error) {
mergedConfig, err := config.createClientConfig()
if err != nil {
return clientcmdapi.Config{}, err
if errors.Is(err, ErrNoContextSet) {
config, _ := mergedConfig.RawConfig()
return config, err
} else {
return clientcmdapi.Config{}, err
}
}

return mergedConfig.RawConfig()
}

Expand Down
5 changes: 3 additions & 2 deletions staging/src/k8s.io/client-go/tools/clientcmd/validation.go
View file
Open in desktop
Expand Up @@ -29,8 +29,9 @@ import (
)

var (
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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

ErrEmptyConfig = NewEmptyConfigError("no configuration has been provided, try setting KUBERNETES_MASTER environment variable")
// message is for consistency with old behavior
ErrEmptyCluster = errors.New("cluster has no server defined")
)
Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/config/create_authinfo.go
View file
Open in desktop
Expand Up @@ -200,7 +200,7 @@ func (o createAuthInfoOptions) run() error {
authInfo := o.modifyAuthInfo(*startingStanza)
config.AuthInfos[o.name] = &authInfo

if err := clientcmd.ModifyConfig(o.configAccess, *config, true); err != nil {
if err := clientcmd.ModifyConfig(o.configAccess, *config, true, false); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/config/create_cluster.go
View file
Open in desktop
Expand Up @@ -112,7 +112,7 @@ func (o createClusterOptions) run() error {
cluster := o.modifyCluster(*startingStanza)
config.Clusters[o.name] = &cluster

if err := clientcmd.ModifyConfig(o.configAccess, *config, true); err != nil {
if err := clientcmd.ModifyConfig(o.configAccess, *config, true, false); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/config/create_context.go
View file
Open in desktop
Expand Up @@ -107,7 +107,7 @@ func (o createContextOptions) run() (string, bool, error) {
context := o.modifyContext(*startingStanza)
config.Contexts[name] = &context

if err := clientcmd.ModifyConfig(o.configAccess, *config, true); err != nil {
if err := clientcmd.ModifyConfig(o.configAccess, *config, true, false); err != nil {
return name, exists, err
}

Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/config/delete_cluster.go
View file
Open in desktop
Expand Up @@ -74,7 +74,7 @@ func runDeleteCluster(out io.Writer, configAccess clientcmd.ConfigAccess, cmd *c

delete(config.Clusters, name)

if err := clientcmd.ModifyConfig(configAccess, *config, true); err != nil {
if err := clientcmd.ModifyConfig(configAccess, *config, true, false); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/config/delete_context.go
View file
Open in desktop
Expand Up @@ -78,7 +78,7 @@ func runDeleteContext(out, errOut io.Writer, configAccess clientcmd.ConfigAccess

delete(config.Contexts, name)

if err := clientcmd.ModifyConfig(configAccess, *config, true); err != nil {
if err := clientcmd.ModifyConfig(configAccess, *config, true, false); err != nil {
return err
}

Expand Down
3 changes: 2 additions & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/config/get_contexts.go
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package config

import (
"errors"
"fmt"
"io"
"sort"
Expand Down Expand Up @@ -111,7 +112,7 @@ func (o *GetContextsOptions) Complete(cmd *cobra.Command, args []string) error {
// RunGetContexts implements all the necessary functionality for context retrieval.
func (o GetContextsOptions) RunGetContexts() error {
config, err := o.configAccess.GetStartingConfig()
if err != nil {
if err != nil && !errors.Is(err, clientcmd.ErrNoContextSet){
return err
}

Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/config/rename_context.go
View file
Open in desktop
Expand Up @@ -123,7 +123,7 @@ func (o RenameContextOptions) RunRenameContext(out io.Writer) error {
config.CurrentContext = o.newName
}

if err := clientcmd.ModifyConfig(o.configAccess, *config, true); err != nil {
if err := clientcmd.ModifyConfig(o.configAccess, *config, true, false); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/config/set.go
View file
Open in desktop
Expand Up @@ -111,7 +111,7 @@ func (o setOptions) run() error {
return err
}

if err := clientcmd.ModifyConfig(o.configAccess, *config, false); err != nil {
if err := clientcmd.ModifyConfig(o.configAccess, *config, false, false); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/config/unset.go
View file
Open in desktop
Expand Up @@ -89,7 +89,7 @@ func (o unsetOptions) run(out io.Writer) error {
return err
}

if err := clientcmd.ModifyConfig(o.configAccess, *config, false); err != nil {
if err := clientcmd.ModifyConfig(o.configAccess, *config, false, false); err != nil {
return err
}
if _, err := fmt.Fprintf(out, "Property %q unset.\n", o.propertyName); err != nil {
Expand Down
4 changes: 2 additions & 2 deletions staging/src/k8s.io/kubectl/pkg/cmd/config/use_context.go
View file
Open in desktop
Expand Up @@ -64,7 +64,7 @@ func NewCmdConfigUseContext(out io.Writer, configAccess clientcmd.ConfigAccess)

func (o useContextOptions) run() error {
config, err := o.configAccess.GetStartingConfig()
if err != nil {
if err != nil && !errors.Is(err, clientcmd.ErrNoContextSet){
return err
}

Expand All @@ -75,7 +75,7 @@ func (o useContextOptions) run() error {

config.CurrentContext = o.contextName

return clientcmd.ModifyConfig(o.configAccess, *config, true)
return clientcmd.ModifyConfig(o.configAccess, *config, true, true)
}

func (o *useContextOptions) complete(cmd *cobra.Command) error {
Expand Down