From a1934657c2b48a4065a4518c14bd488ad7c85324 Mon Sep 17 00:00:00 2001 From: Simon Ferquel Date: Wed, 17 Jun 2020 10:40:44 +0200 Subject: [PATCH 1/2] Defer error from referencing a non-exisiting context from the config file to 1st API call When a user removes a context from the store without using the CLI (by removing the context directory directly in the store), it can trigger an issue where the CLI does not work at all (even for changing the current context) anymore without editing the config file by hand. With Docker Desktop and WSL 2, we now link the wsl 2 distros context store to the windows one, so that a context created from Windows can be used in WSL 2 and vice-versa. However we keep the CLI config files independant (at least for now), so this issue can be triggered quite often. This commit fixes that by changing the way we handle context missing error: if the resolved context name comes from the config file and refers a missing context, the error is reported when the API is actually called instead of at the CLI initialization. Thus, local-only commands continue to work (shuch as `context use`, `context ls`,...) Signed-off-by: Simon Ferquel --- cli/command/cli.go | 50 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index a9e7e30763d3..6c8d7e01e206 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -4,6 +4,7 @@ import ( "context" "io" "io/ioutil" + "net" "os" "path/filepath" "runtime" @@ -38,6 +39,16 @@ import ( "github.com/theupdateframework/notary/passphrase" ) +type contextNameSource int + +const ( + contextFallbackToDockerHost contextNameSource = iota + contextFromFlag + contextFromEnv + contextFromConfigFile + contextDefault +) + // Streams is an interface which exposes the standard input and output streams type Streams interface { In() *streams.In @@ -251,12 +262,25 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize return ResolveDefaultContext(opts.Common, cli.ConfigFile(), cli.contextStoreConfig, cli.Err()) }, } - cli.currentContext, err = resolveContextName(opts.Common, cli.configFile, cli.contextStore) + var contextSource contextNameSource + cli.currentContext, contextSource, err = resolveContextName(opts.Common, cli.configFile) if err != nil { return err } cli.dockerEndpoint, err = resolveDockerEndpoint(cli.contextStore, cli.currentContext) - if err != nil { + if store.IsErrContextDoesNotExist(err) && contextSource == contextFromConfigFile { + if cli.client == nil { + // populate a client that always fails with "context not found" + // this is to defer the context not found error until API is really used to allow + // operations like `docker context use` or `docker context ls` + cli.client, err = client.NewClientWithOpts(client.WithDialContext(func(ctx context.Context, network, addr string) (net.Conn, error) { + return nil, errors.Errorf("Current context %q is not found on the file system, please check your config file at %s", cli.configFile.CurrentContext, cli.configFile.Filename) + })) + if err != nil { + return err + } + } + } else if err != nil { return errors.Wrap(err, "unable to resolve docker endpoint") } @@ -292,7 +316,7 @@ func NewAPIClientFromFlags(opts *cliflags.CommonOptions, configFile *configfile. return ResolveDefaultContext(opts, configFile, storeConfig, ioutil.Discard) }, } - contextName, err := resolveContextName(opts, configFile, store) + contextName, _, err := resolveContextName(opts, configFile) if err != nil { return nil, err } @@ -530,30 +554,26 @@ func UserAgent() string { // - if DOCKER_CONTEXT is set, use this value // - if Config file has a globally set "CurrentContext", use this value // - fallbacks to default HOST, uses TLS config from flags/env vars -func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigFile, contextstore store.Reader) (string, error) { +func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigFile) (string, contextNameSource, error) { if opts.Context != "" && len(opts.Hosts) > 0 { - return "", errors.New("Conflicting options: either specify --host or --context, not both") + return "", contextDefault, errors.New("Conflicting options: either specify --host or --context, not both") } if opts.Context != "" { - return opts.Context, nil + return opts.Context, contextFromFlag, nil } if len(opts.Hosts) > 0 { - return DefaultContextName, nil + return DefaultContextName, contextFallbackToDockerHost, nil } if _, present := os.LookupEnv("DOCKER_HOST"); present { - return DefaultContextName, nil + return DefaultContextName, contextFallbackToDockerHost, nil } if ctxName, ok := os.LookupEnv("DOCKER_CONTEXT"); ok { - return ctxName, nil + return ctxName, contextFromEnv, nil } if config != nil && config.CurrentContext != "" { - _, err := contextstore.GetMetadata(config.CurrentContext) - if store.IsErrContextDoesNotExist(err) { - return "", errors.Errorf("Current context %q is not found on the file system, please check your config file at %s", config.CurrentContext, config.Filename) - } - return config.CurrentContext, err + return config.CurrentContext, contextFromConfigFile, nil } - return DefaultContextName, nil + return DefaultContextName, contextDefault, nil } var defaultStoreEndpoints = []store.NamedTypeGetter{ From 22ebf23c04eca28a08633792e088c0d42ceb8b7c Mon Sep 17 00:00:00 2001 From: Simon Ferquel Date: Wed, 17 Jun 2020 11:36:17 +0200 Subject: [PATCH 2/2] Added validating test Signed-off-by: Simon Ferquel --- cli/command/cli_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/cli/command/cli_test.go b/cli/command/cli_test.go index 32c999df753a..08053fb06e62 100644 --- a/cli/command/cli_test.go +++ b/cli/command/cli_test.go @@ -11,6 +11,7 @@ import ( "runtime" "testing" + "github.com/docker/cli/cli/config" cliconfig "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/flags" @@ -322,3 +323,20 @@ func TestInitializeShouldAlwaysCreateTheContextStore(t *testing.T) { }))) assert.Check(t, cli.ContextStore() != nil) } + +func TestMissingContextFromConfigFileShouldDeferError(t *testing.T) { + tmpDir, err := ioutil.TempDir("", t.Name()) + assert.NilError(t, err) + defer os.RemoveAll(tmpDir) + + config.SetDir(tmpDir) + configFile := config.LoadDefaultConfigFile(ioutil.Discard) + configFile.CurrentContext = "missing" + assert.NilError(t, configFile.Save()) + + cli, err := NewDockerCli() + assert.NilError(t, err) + assert.NilError(t, cli.Initialize(flags.NewClientOptions())) + _, err = cli.client.Info(context.Background()) + assert.ErrorContains(t, err, `Current context "missing" is not found on the file system`) +}