Skip to content

Commit ed05112

Browse files
committed
cli/command: DockerCli.Initialize: make sure context-store config is set
In most situations, the CLI is created through the `NewDockerCli` constructor, however, it's possible to construct a CLI manually (`&DockerCli{}`). We should probably prevent this (and un-export the `DockerCli` implementation), but currently have some code-paths that depend on the type being exported. When constructing the CLI with this approach, the CLI would not be fully initialized and not have the context-store configuration set up. Using the default context store without a config set will result in Endpoints from contexts not being type-mapped correctly, and used as a generic `map[string]any`, instead of a [docker.EndpointMeta]. When looking up the API endpoint (using [EndpointFromContext]), no endpoint will be found, and a default, empty endpoint will be used instead which in its turn, causes [newAPIClientFromEndpoint] to be initialized with the default config instead of settings for the current context (which may mean; connecting with the wrong endpoint and/or TLS Config to be missing). I'm not sure if this situation could happen in practice, but it caused some of our unit-tests ([TestInitializeFromClient] among others) to fail when running outside of the dev-container on a host that used Docker Desktop's "desktop-linux" context. In that situation, the test would produce the wrong "Ping" results (using defaults, instead of the results produced in the test). This patch: - updates the contextStoreConfig field to be a pointer, so that we are able to detect if a config was already set. - updates the `Initialize` function to set the default context-store config if no config was found (technically the field is mostly immutable, and can only set through `WithDefaultContextStoreConfig`, so this may be slightly redundant). We should update this code to be less error-prone to use; the combination of an exported type (`DockerCli`), a constructor `NewDockerCli` and a `Initialize` function (as well as some internal contructors to allow lazy initialization) make constructing the "CLI" hard to use, and there's various codepaths where it can be in a partially initialized state. The same applies to the default context store, which also requires too much "domain" knowledge to use properly. I'm leaving improvements around that for a follow-up. [EndpointFromContext]: https://github.com/docker/cli/blob/33494921b80fd0b5a06acc3a34fa288de4bb2e6b/cli/context/docker/load.go#L139-L149 [docker.EndpointMeta]: https://github.com/docker/cli/blob/33494921b80fd0b5a06acc3a34fa288de4bb2e6b/cli/context/docker/load.go#L19-L21 [newAPIClientFromEndpoint]: https://github.com/docker/cli/blob/33494921b80fd0b5a06acc3a34fa288de4bb2e6b/cli/command/cli.go#L295-L305 [TestInitializeFromClient]: https://github.com/docker/cli/blob/33494921b80fd0b5a06acc3a34fa288de4bb2e6b/cli/command/cli_test.go#L157-L205 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
1 parent 3349492 commit ed05112

2 files changed

Lines changed: 28 additions & 5 deletions

File tree

cli/command/cli.go

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,9 @@ type Cli interface {
5959
}
6060

6161
// DockerCli is an instance the docker command line client.
62-
// Instances of the client can be returned from NewDockerCli.
62+
// Instances of the client should be created using the [NewDockerCli]
63+
// constructor to make sure they are properly initialized with defaults
64+
// set.
6365
type DockerCli struct {
6466
configFile *configfile.ConfigFile
6567
options *cliflags.ClientOptions
@@ -74,7 +76,7 @@ type DockerCli struct {
7476
init sync.Once
7577
initErr error
7678
dockerEndpoint docker.Endpoint
77-
contextStoreConfig store.Config
79+
contextStoreConfig *store.Config
7880
initTimeout time.Duration
7981
res telemetryResource
8082

@@ -250,13 +252,33 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...CLIOption)
250252
return errors.New("conflicting options: cannot specify both --host and --context")
251253
}
252254

255+
if cli.contextStoreConfig == nil {
256+
// This path can be hit when calling Initialize on a DockerCli that's
257+
// not constructed through [NewDockerCli]. Using the default context
258+
// store without a config set will result in Endpoints from contexts
259+
// not being type-mapped correctly, and used as a generic "map[string]any",
260+
// instead of a [docker.EndpointMeta].
261+
//
262+
// When looking up the API endpoint (using [EndpointFromContext]), no
263+
// endpoint will be found, and a default, empty endpoint will be used
264+
// instead which in its turn, causes newAPIClientFromEndpoint to
265+
// be initialized with the default config instead of settings for
266+
// the current context (which may mean; connecting with the wrong
267+
// endpoint and/or TLS Config to be missing).
268+
//
269+
// [EndpointFromContext]: https://github.com/docker/cli/blob/33494921b80fd0b5a06acc3a34fa288de4bb2e6b/cli/context/docker/load.go#L139-L149
270+
if err := WithDefaultContextStoreConfig()(cli); err != nil {
271+
return err
272+
}
273+
}
274+
253275
cli.options = opts
254276
cli.configFile = config.LoadDefaultConfigFile(cli.err)
255277
cli.currentContext = resolveContextName(cli.options, cli.configFile)
256278
cli.contextStore = &ContextStoreWithDefault{
257-
Store: store.New(config.ContextStoreDir(), cli.contextStoreConfig),
279+
Store: store.New(config.ContextStoreDir(), *cli.contextStoreConfig),
258280
Resolver: func() (*DefaultContext, error) {
259-
return ResolveDefaultContext(cli.options, cli.contextStoreConfig)
281+
return ResolveDefaultContext(cli.options, *cli.contextStoreConfig)
260282
},
261283
}
262284

cli/command/cli_options.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,8 @@ func WithContentTrust(enabled bool) CLIOption {
101101
// WithDefaultContextStoreConfig configures the cli to use the default context store configuration.
102102
func WithDefaultContextStoreConfig() CLIOption {
103103
return func(cli *DockerCli) error {
104-
cli.contextStoreConfig = DefaultContextStoreConfig()
104+
cfg := DefaultContextStoreConfig()
105+
cli.contextStoreConfig = &cfg
105106
return nil
106107
}
107108
}

0 commit comments

Comments
 (0)