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.

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

cli/command: DockerCli.Initialize: make sure context-store config is set #5983

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

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.

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

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>
@thaJeztah thaJeztah added status/2-code-review kind/bugfix PR's that fix bugs kind/refactor PR's that refactor, or clean-up code labels Apr 1, 2025
@thaJeztah thaJeztah added this to the 28.0.5 milestone Apr 1, 2025
@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Project coverage is 59.13%. Comparing base (3349492) to head (ed05112).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5983      +/-   ##
==========================================
+ Coverage   59.12%   59.13%   +0.01%     
==========================================
  Files         355      355              
  Lines       29740    29759      +19     
==========================================
+ Hits        17583    17599      +16     
- Misses      11182    11184       +2     
- Partials      975      976       +1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thaJeztah thaJeztah requested a review from Benehiko April 4, 2025 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bugfix PR's that fix bugs kind/refactor PR's that refactor, or clean-up code status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants