Don't filter out registries to logout from with config file contents #2583
Conversation
Previously, if a registry AuthInfo was not present in the CLI config file, docker logout could not be used to ask the credential helper to forget about it. It causes problem for people working with multiple alternative config files, and it causes problems for cases like Docker Desktop w/ WSL 2, as it uses the same win32 credential helper as the Windows CLI, but a different config file, leading to bugs where I cannot logout from a registry from wsl2 if I logged in from Windows and vice-versa. Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
simonferquel
force-pushed
the
logout-config-out-of-sync2
branch
from
2c04ba0 to
6248f2f
Compare
Codecov Report
@@ Coverage Diff @@
## master #2583 +/- ##
==========================================
+ Coverage 58.04% 58.05% +0.01%
==========================================
Files 295 295
Lines 21166 21162 -4
==========================================
Hits 12285 12285
+ Misses 7979 7975 -4
Partials 902 902 |
| } | ||
|
|
||
| // if at least one removal succeeded, report success. Otherwise report errors | ||
| if len(errs) == len(regsToLogout) { |
There was a problem hiding this comment.
Should we check if len(errs) > 0 or len(regsToLogout) > 0 as well?
There was a problem hiding this comment.
regsToLogout cannot be empty (it is initialized with one value, and we append other candidates in it if it is not the default registry)
|
I see another PR also added some tests; #2346. Haven't looked closely, but could you check if those tests would be useful to have? |
| if !loggedIn { | ||
| fmt.Fprintf(dockerCli.Out(), "Not logged in to %s\n", hostnameAddress) | ||
| return nil | ||
| regsToLogout = append(regsToLogout, hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress) |
There was a problem hiding this comment.
Actually, given that we duplicate hostnameAddress and perform a logout three times for each registry, I'm wondering if we should make regsToLogout a map[string][]string, so that we (at most) generate a single error per registry
| regsToLogout = append(regsToLogout, hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress) | |
| regsToLogout[hostnameAddress] = []string{hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress} |
wdyt? 🤔
There was a problem hiding this comment.
Actually, we logout from a single registry here. It is just that we generate a list of candidates so that docker logout toto tries to logout from both toto, http://toto and https://toto. So there will always be only one entry in this map.
I wonder though if we could only print the first error...
There was a problem hiding this comment.
Ah, yes, had my wires crossed, and thinking we're logging out from all registries. Perhaps it's ok for now, considering this a "these are the URLs we tried to logout from, and this is why they failed".
Only thing I'm wondering is if we produce an error if "no entry was found"; if so, and if we would be able to detect that cause, we (IMO) should ignore those, and treat it as an idempotent action.
There was a problem hiding this comment.
The "plaintext" credstore implementation is already idempotent, and the wincred helper as well. I am not sure about the mac one, and 3rd parties. But I agree we should document the intended behavior should be idempotent (now that the CLI code is)
Hmm, the thing is those tests do not really apply there I think, as it changes the errors reported here (and especially, as a side effect, logout is idempotent - at least for creds helper that don't report an error when loging out from a not logged-in registry) |
supersedes / closes
closes #1917
closes #2346
fixes #204
- What I did
Previously, if a registry AuthInfo was not present in the CLI config file, docker logout could not be used
to ask the credential helper to forget about it. It causes problem for people working with
multiple alternative config files, and it causes problems for cases like Docker Desktop w/ WSL 2, as
it uses the same win32 credential helper as the Windows CLI, but a different config file, leading to
bugs where I cannot logout from a registry from wsl2 if I logged in from Windows and vice-versa.
- How I did it
Change the logic to call the credential helper for each candidate entry, and comparing the number of returned errors to the number of tries.
- How to verify it
docker login(with a credential helper)~/.docker/config.jsondocker logout- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)