Repository navigation
Fix login logout when engine is down and credential helper is in use - #1789
Conversation
Previously if the Docker engine was not running the behaviour of
commands would vary depending on whether the --debug flag was provided.
For example, consider `docker logout`:
$ docker logout
Not logged in to
-- note the missing server URL
$ docker --debug logout
Warning: failed to get default registry endpoint from daemon (Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?). Using system default: https://index.docker.io/v1/
Not logged in to https://index.docker.io/v1/
-- note the server URL is present
This patch makes only the debug printing conditional on the `--debug` flag,
not the return value.
Signed-off-by: David Scott <dave.scott@docker.com>
Instead of using an `if else if else`, switch to a sequence of independent `if` blocks containing a `return`. Instead of defining a return variable and updating it in the `if` blocks and returning at the end, make each `if` block return the desired value independenly. Signed-off-by: David Scott <dave.scott@docker.com>
Codecov Report
@@ Coverage Diff @@
## master #1789 +/- ##
==========================================
+ Coverage 56.26% 56.26% +<.01%
==========================================
Files 308 308
Lines 21293 21295 +2
==========================================
+ Hits 11980 11982 +2
Misses 8439 8439
Partials 874 874 |
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #1789 +/- ##
==========================================
+ Coverage 56.26% 56.26% +<.01%
==========================================
Files 308 308
Lines 21293 21295 +2
==========================================
+ Hits 11980 11982 +2
Misses 8439 8439
Partials 874 874 |
| fmt.Fprintf(cli.Err(), "Warning: Empty registry endpoint from daemon. Using system default: %s\n", serverAddress) | ||
| } else { | ||
| serverAddress = info.IndexServerAddress | ||
| info, err := cli.Client().Info(ctx) |
There was a problem hiding this comment.
Should we use /_ping for this? (/info is a bit heavy just to know if the engine is up and running)
| } | ||
| return registry.IndexServer | ||
| } | ||
| if info.IndexServerAddress == "" { |
There was a problem hiding this comment.
Oh, never mind, I see we need this information 🤔
There was a problem hiding this comment.
Actually looks like it's hard-coded, and never updated anywhere 🤔 https://github.com/moby/moby/blob/fc01c2b481097a6057bec3cd1ab2d7b4488c50c4/registry/config.go#L29-L48
| return registry.IndexServer | ||
| } | ||
| if info.IndexServerAddress == "" { | ||
| if debug.IsEnabled() { |
There was a problem hiding this comment.
We definitely need a real log policy here: we should write something like log.Warning(...) and let the debug flag change only the output level, not spreading it everywhere. But of course that's another issue.
| @@ -28,16 +28,22 @@ func ElectAuthServer(ctx context.Context, cli Cli) string { | |||
| // used. This is essential in cross-platforms environment, where for | |||
| // example a Linux client might be interacting with a Windows daemon, hence | |||
There was a problem hiding this comment.
Looks like this was added in moby/moby#19891 at a time where a temporary/special Windows registry was set up: moby/moby#18019 https://github.com/moby/moby/blob/960710bd810e70b7c06b77b956c7bf8cdfeb15f8/registry/config_windows.go#L9-L19
Which was removed in moby/moby#21100
Actually thinking if that handling is now just dead code and no longer needed
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
I'll have a look at removing the handling of "special" URLs for windows; guess this could be removed altogether
|
Thanks for the very quick response (and merge)! |
- What I did
Fixed
docker loginanddocker logoutwhen the engine is not running and when using a credential helper.- How I did it
The logic in
ElectAuthServeraccidentally combined together checking the error from the engine with the state of the debug flag when deciding which value to return.- How to verify it
I used Docker Desktop on Mac with the
docker-credential-osxkeychainenabled (which is the default). I shutdown the app. Without this patch I ran:-- note the missing server URL
-- note the server URL is present
This shows that the
--debugflag is changing the actual behaviour of the program.I ran
docker loginand observed an error from the credential helperWhen I rebuilt the CLI with this PR
docker loginanddocker logoutoperated as expected.I separated this PR into 2 patches:
ElectAuthServereasier to read but which is optional- Description for the changelog
Fix
docker loginanddocker logoutwhen the engine is not running and a credential helper is in use.- A picture of a cute animal (not mandatory but encouraged)