From 2f5698511a26e51eaa8a2c51bd91de56f348b699 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 4 Nov 2022 10:22:40 +0100 Subject: [PATCH 1/4] cli/command: resolveContextName() don't validate if context exists resolveContextName() is used to find which context to use, based on the available configuration options. Once resolved, the context name is used to load the actual context, which will fail if the context doesn't exist, so there's no need to produce an error at this stage; only check priority of the configuration options to pick the context with the highest priority. Signed-off-by: Sebastiaan van Stijn --- cli/command/cli.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index b5c1ccd996..ed9cb6cc3c 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -28,7 +28,6 @@ import ( "github.com/docker/docker/api/types/registry" "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/client" - "github.com/docker/docker/errdefs" "github.com/docker/go-connections/tlsconfig" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -222,7 +221,7 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize return ResolveDefaultContext(opts, cli.contextStoreConfig) }, } - cli.currentContext, err = resolveContextName(opts, cli.configFile, cli.contextStore) + cli.currentContext, err = resolveContextName(opts, cli.configFile) if err != nil { return err } @@ -250,7 +249,7 @@ func NewAPIClientFromFlags(opts *cliflags.ClientOptions, configFile *configfile. return ResolveDefaultContext(opts, storeConfig) }, } - contextName, err := resolveContextName(opts, configFile, contextStore) + contextName, err := resolveContextName(opts, configFile) if err != nil { return nil, err } @@ -445,7 +444,10 @@ 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.ClientOptions, config *configfile.ConfigFile, contextstore store.Reader) (string, error) { +// +// resolveContextName does not validate if the given context exists; errors may +// occur when trying to use it. +func resolveContextName(opts *cliflags.ClientOptions, config *configfile.ConfigFile) (string, error) { if opts.Context != "" && len(opts.Hosts) > 0 { return "", errors.New("Conflicting options: either specify --host or --context, not both") } @@ -462,11 +464,8 @@ func resolveContextName(opts *cliflags.ClientOptions, config *configfile.ConfigF return ctxName, nil } if config != nil && config.CurrentContext != "" { - _, err := contextstore.GetMetadata(config.CurrentContext) - if errdefs.IsNotFound(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 + // We don't validate if this context exists: errors may occur when trying to use it. + return config.CurrentContext, nil } return DefaultContextName, nil } From 793f09705d77324b65c494bc1361813514593991 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 4 Nov 2022 10:33:22 +0100 Subject: [PATCH 2/4] cli/command: resolveContextName() move conflicting options check There's no strict need to perform this validation inside this function; validating flags should happen earlier, to allow faster detecting of configuration issues (we may want to have a central config "validate" function though). Signed-off-by: Sebastiaan van Stijn --- cli/command/cli.go | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index ed9cb6cc3c..7777dbc75d 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -211,6 +211,9 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize if opts.Debug { debug.Enable() } + if opts.Context != "" && len(opts.Hosts) > 0 { + return errors.New("conflicting options: either specify --host or --context, not both") + } cli.loadConfigFile() @@ -221,10 +224,7 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize return ResolveDefaultContext(opts, cli.contextStoreConfig) }, } - cli.currentContext, err = resolveContextName(opts, cli.configFile) - if err != nil { - return err - } + cli.currentContext = resolveContextName(opts, cli.configFile) cli.dockerEndpoint, err = resolveDockerEndpoint(cli.contextStore, cli.currentContext) if err != nil { return errors.Wrap(err, "unable to resolve docker endpoint") @@ -242,6 +242,10 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize // NewAPIClientFromFlags creates a new APIClient from command line flags func NewAPIClientFromFlags(opts *cliflags.ClientOptions, configFile *configfile.ConfigFile) (client.APIClient, error) { + if opts.Context != "" && len(opts.Hosts) > 0 { + return nil, errors.New("conflicting options: either specify --host or --context, not both") + } + storeConfig := DefaultContextStoreConfig() contextStore := &ContextStoreWithDefault{ Store: store.New(config.ContextStoreDir(), storeConfig), @@ -249,11 +253,7 @@ func NewAPIClientFromFlags(opts *cliflags.ClientOptions, configFile *configfile. return ResolveDefaultContext(opts, storeConfig) }, } - contextName, err := resolveContextName(opts, configFile) - if err != nil { - return nil, err - } - endpoint, err := resolveDockerEndpoint(contextStore, contextName) + endpoint, err := resolveDockerEndpoint(contextStore, resolveContextName(opts, configFile)) if err != nil { return nil, errors.Wrap(err, "unable to resolve docker endpoint") } @@ -447,27 +447,24 @@ func UserAgent() string { // // resolveContextName does not validate if the given context exists; errors may // occur when trying to use it. -func resolveContextName(opts *cliflags.ClientOptions, config *configfile.ConfigFile) (string, error) { - if opts.Context != "" && len(opts.Hosts) > 0 { - return "", errors.New("Conflicting options: either specify --host or --context, not both") +func resolveContextName(opts *cliflags.ClientOptions, config *configfile.ConfigFile) string { + if opts != nil && opts.Context != "" { + return opts.Context } - if opts.Context != "" { - return opts.Context, nil - } - if len(opts.Hosts) > 0 { - return DefaultContextName, nil + if opts != nil && len(opts.Hosts) > 0 { + return DefaultContextName } if os.Getenv(client.EnvOverrideHost) != "" { - return DefaultContextName, nil + return DefaultContextName } if ctxName := os.Getenv("DOCKER_CONTEXT"); ctxName != "" { - return ctxName, nil + return ctxName } if config != nil && config.CurrentContext != "" { // We don't validate if this context exists: errors may occur when trying to use it. - return config.CurrentContext, nil + return config.CurrentContext } - return DefaultContextName, nil + return DefaultContextName } var defaultStoreEndpoints = []store.NamedTypeGetter{ From e36d5a0929a90906e6e10cca39d327695f56a184 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 18 Nov 2022 00:22:25 +0100 Subject: [PATCH 3/4] cli/command: DockerCli.CurrentContext: improve GoDoc Also move the resolveContextName() function together with the method for easier cross-referencing. Signed-off-by: Sebastiaan van Stijn --- cli/command/cli.go | 85 +++++++++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 32 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index 7777dbc75d..f27d43a5f7 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -362,11 +362,63 @@ func (cli *DockerCli) ContextStore() store.Store { return cli.contextStore } -// CurrentContext returns the current context name +// CurrentContext returns the current context name, based on flags, +// environment variables and the cli configuration file, in the following +// order of preference: +// +// 1. The "--context" command-line option. +// 2. The "DOCKER_CONTEXT" environment variable. +// 3. The current context as configured through the in "currentContext" +// field in the CLI configuration file ("~/.docker/config.json"). +// 4. If no context is configured, use the "default" context. +// +// # Fallbacks for backward-compatibility +// +// To preserve backward-compatibility with the "pre-contexts" behavior, +// the "default" context is used if: +// +// - The "--host" option is set +// - The "DOCKER_HOST" ([DefaultContextName]) environment variable is set +// to a non-empty value. +// +// In these cases, the default context is used, which uses the host as +// specified in "DOCKER_HOST", and TLS config from flags/env vars. +// +// Setting both the "--context" and "--host" flags is ambiguous and results +// in an error when the cli is started. +// +// CurrentContext does not validate if the given context exists or if it's +// valid; errors may occur when trying to use it. func (cli *DockerCli) CurrentContext() string { return cli.currentContext } +// CurrentContext returns the current context name, based on flags, +// environment variables and the cli configuration file. It does not +// validate if the given context exists or if it's valid; errors may +// occur when trying to use it. +// +// Refer to [DockerCli.CurrentContext] above for further details. +func resolveContextName(opts *cliflags.ClientOptions, config *configfile.ConfigFile) string { + if opts != nil && opts.Context != "" { + return opts.Context + } + if opts != nil && len(opts.Hosts) > 0 { + return DefaultContextName + } + if os.Getenv(client.EnvOverrideHost) != "" { + return DefaultContextName + } + if ctxName := os.Getenv("DOCKER_CONTEXT"); ctxName != "" { + return ctxName + } + if config != nil && config.CurrentContext != "" { + // We don't validate if this context exists: errors may occur when trying to use it. + return config.CurrentContext + } + return DefaultContextName +} + // DockerEndpoint returns the current docker endpoint func (cli *DockerCli) DockerEndpoint() docker.Endpoint { return cli.dockerEndpoint @@ -436,37 +488,6 @@ func UserAgent() string { return "Docker-Client/" + version.Version + " (" + runtime.GOOS + ")" } -// resolveContextName resolves the current context name with the following rules: -// - setting both --context and --host flags is ambiguous -// - if --context is set, use this value -// - if --host flag or DOCKER_HOST (client.EnvOverrideHost) is set, fallbacks to use the same logic as before context-store was added -// for backward compatibility with existing scripts -// - 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 -// -// resolveContextName does not validate if the given context exists; errors may -// occur when trying to use it. -func resolveContextName(opts *cliflags.ClientOptions, config *configfile.ConfigFile) string { - if opts != nil && opts.Context != "" { - return opts.Context - } - if opts != nil && len(opts.Hosts) > 0 { - return DefaultContextName - } - if os.Getenv(client.EnvOverrideHost) != "" { - return DefaultContextName - } - if ctxName := os.Getenv("DOCKER_CONTEXT"); ctxName != "" { - return ctxName - } - if config != nil && config.CurrentContext != "" { - // We don't validate if this context exists: errors may occur when trying to use it. - return config.CurrentContext - } - return DefaultContextName -} - var defaultStoreEndpoints = []store.NamedTypeGetter{ store.EndpointTypeGetter(docker.DockerEndpoint, func() interface{} { return &docker.EndpointMeta{} }), } From 9c42cd9a3ec5a80c67be7f28900713907e548a24 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 22 Nov 2022 15:11:08 +0100 Subject: [PATCH 4/4] cli/command: TestInitializeFromClientHangs fix unhandled error Signed-off-by: Sebastiaan van Stijn --- cli/command/cli_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/command/cli_test.go b/cli/command/cli_test.go index 9f4cc99ae3..dcd9103fb6 100644 --- a/cli/command/cli_test.go +++ b/cli/command/cli_test.go @@ -200,7 +200,8 @@ func TestInitializeFromClientHangs(t *testing.T) { go func() { cli := &DockerCli{client: apiClient, initTimeout: time.Millisecond} - cli.Initialize(flags.NewClientOptions()) + err := cli.Initialize(flags.NewClientOptions()) + assert.Check(t, err) close(initializedCh) }()