Skip to content

Don't filter out registries to logout from with config file contents #2583

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.

Sign up for GitHub

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

Merged
thaJeztah merged 1 commit into from
Jun 17, 2020
Merged

Don't filter out registries to logout from with config file contents #2583

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 12 additions & 19 deletions cli/command/registry/logout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,36 +39,29 @@ func runLogout(dockerCli command.Cli, serverAddress string) error {
}

var (
loggedIn bool
regsToLogout []string
regsToLogout = []string{serverAddress}
hostnameAddress = serverAddress
regsToTry = []string{serverAddress}
)
if !isDefaultRegistry {
hostnameAddress = registry.ConvertToHostname(serverAddress)
// the tries below are kept for backward compatibility where a user could have
// saved the registry in one of the following format.
regsToTry = append(regsToTry, hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress)
}

// check if we're logged in based on the records in the config file
// which means it couldn't have user/pass cause they may be in the creds store
for _, s := range regsToTry {
if _, ok := dockerCli.ConfigFile().AuthConfigs[s]; ok {
loggedIn = true
regsToLogout = append(regsToLogout, s)
}
}

if !loggedIn {
fmt.Fprintf(dockerCli.Out(), "Not logged in to %s\n", hostnameAddress)
return nil
regsToLogout = append(regsToLogout, hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress)
Copy link
Member

@thaJeztah thaJeztah Jun 16, 2020

Choose a reason for hiding this comment

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

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

Suggested change
regsToLogout = append(regsToLogout, hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress)
regsToLogout[hostnameAddress] = []string{hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress}

wdyt? 🤔

Copy link
Contributor Author

@simonferquel simonferquel Jun 16, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@thaJeztah thaJeztah Jun 17, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@simonferquel simonferquel Jun 17, 2020

Choose a reason for hiding this comment

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

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)

}

fmt.Fprintf(dockerCli.Out(), "Removing login credentials for %s\n", hostnameAddress)
errs := make(map[string]error)
for _, r := range regsToLogout {
if err := dockerCli.ConfigFile().GetCredentialsStore(r).Erase(r); err != nil {
fmt.Fprintf(dockerCli.Err(), "WARNING: could not erase credentials: %v\n", err)
errs[r] = err
}
}

// if at least one removal succeeded, report success. Otherwise report errors
if len(errs) == len(regsToLogout) {
Copy link
Member

@thaJeztah thaJeztah Jun 16, 2020

Choose a reason for hiding this comment

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

Should we check if len(errs) > 0 or len(regsToLogout) > 0 as well?

Copy link
Contributor Author

@simonferquel simonferquel Jun 16, 2020

Choose a reason for hiding this comment

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

regsToLogout cannot be empty (it is initialized with one value, and we append other candidates in it if it is not the default registry)

fmt.Fprintln(dockerCli.Err(), "WARNING: could not erase credentials:")
for k, v := range errs {
fmt.Fprintf(dockerCli.Err(), "%s: %s\n", k, v)
}
}

Expand Down