Skip to content

Conversation

@simonferquel
Copy link
Contributor

@simonferquel simonferquel commented Jun 12, 2020

- What I did

Previously, when the "CurrentContext" field of the config file referenced a context that did not exist in the store, the CLI would fail (even for simple things like docker context ls).
This causes an issue with Docker Desktop "Contexts Syncing" feature (that uses the same store between win32 and WSL2):
If win32 cli removes the "current context" of the WSL 2 CLI (which it can do if it is not the "current context" in the win32 config file), it would make the WSL 2 CLI unusable, asking the user to modify the config file by hand.

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,...).

This commit relaxes this behavior a bit: explicitly referencing a missing context (using the --context flag or DOCKER_CONTEXT env var) will still produce an error, but implicitly referencing such a missing context (via the CurrentContext config field) will fallback to use the "default" context.

- How I did it

In the function resolving the current context name, instead of erroring with asking the user to modify its config file when the config file references a missing context, fallback to default

The function resolving the current context name does not error when the config file references a missing context. It returns a hint about the source of context name.

The check for a missing context is done in the CLI init code. If we find out that the context is missing and that the source of the selected context name is the config file, instead of failing, we initialize the api client with an always failing client (that reports the error during dialing). This allows to change the current context easily, without having to edit the config file by hand.

- How to verify it

Edit the config file, set CurrentConfig to a value that is not an existing context name, check that the CLI fallbacks to using "default".

- Description for the changelog

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

return "", errors.Errorf("Current context %q is not found on the file system, please check your config file at %s", config.CurrentContext, config.Filename)
// If config file references a context that does not exist, fallback to default
// Explicitly requested contexts (flag or env-var based) will still get the error
return DefaultContextName, nil
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should silently fall back, as this would lead to unexpected results (similar to outlined in #1637)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would writing a message to stderr feel good enough ?

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't think a message would be enough, because that wouldn't make the command fail.

Consider a situation where I am in a specific context, and that context is (somehow) removed, and all of a sudden my command completes successfully but against the wrong server. Depending on the command I ran that could mean I just (e.g.) removed a volume or image from the wrong environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Would deferring the error to the http call be enough?

Copy link
Member

Choose a reason for hiding this comment

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

If we can defer to the moment it's actually needed, yes, that'd work (probably also when doing docker context inspect, but I guess that would already be the case)

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2020

Codecov Report

Merging #2576 into master will increase coverage by 0.03%.
The diff coverage is 68.42%.

@@            Coverage Diff             @@
##           master    #2576      +/-   ##
==========================================
+ Coverage   58.04%   58.07%   +0.03%     
==========================================
  Files         295      295              
  Lines       21166    21171       +5     
==========================================
+ Hits        12285    12295      +10     
+ Misses       7979     7975       -4     
+ Partials      902      901       -1     

…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 <simon.ferquel@docker.com>
@simonferquel simonferquel force-pushed the auto-fallback-to-default-context branch from b127c9e to a193465 Compare June 17, 2020 08:50
@simonferquel simonferquel changed the title Fallback to default context when config file references a missing context Defer error to first API call when config file references a missing context Jun 17, 2020
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants