cli icon indicating copy to clipboard operation
cli copied to clipboard

docker should fail if config file is invalid

Open thaJeztah opened this issue 2 months ago • 0 comments

Description

In situations where the config-file is invalid, we ignore the error, and proceed, which can lead to the config file being overwritten for an "empty" file (only some defaults included)

Reproduce

docker run -it --rm -v /var/run/docker.sock:/var/run/docker.sock docker:26.1.2-cli sh
mkdir -p ~/.docker
echo '{"imagesFormat":"some special format I have carefully hand-crafted, artisinally"' > ~/.docker/config.json

docker logout
WARNING: Error loading config file: /root/.docker/config.json: unexpected EOF
Removing login credentials for https://index.docker.io/v1/

cat ~/.docker/config.json
{
	"auths": {}
}

Expected behavior

The problematic area here is that any error returned from loading the config-file is printed as a warning; https://github.com/docker/cli/blob/61fe22f21a1a618d73cba67fec498f3f9d3b1422/cli/config/config.go#L125

A result of that is that if the file fails to load (malformed file, or other reason), load returns an error AND an empty configFile struct; https://github.com/docker/cli/blob/61fe22f21a1a618d73cba67fec498f3f9d3b1422/cli/config/config.go#L117

In this case, we notify the user (warning), but continue the regular flow, which may involve "updating the file" (login / logout, perhaps docker context use)

I was curious though about the "warning" instead of "error"; wondering if it was intended to ignore "not found" errors only, but I don't think that's the case; I think this is mostly because code moved around too much. First time this became a warning looks to be from https://github.com/moby/moby/commit/18c9b6c6455f116ae59cde8544413b3d7d294a5e

Which unlined some of the code; before that, the separate function returned an error, but was not handled. Going back further; it looks like this commit started to ignore errors https://github.com/moby/moby/commit/3bae188b8dc51911a44ea1c7b5681f9f07f9d3af And https://github.com/moby/moby/commit/18962d0ff33cfa6ba2976aa459c766acfd23c1bf looks to settle that by fully ignoring.

So Tl;DR; I don't think there's a good reason / motivation for ignoring the error other than "convenience" (prefer happy path) but changing will mean that we need to change the signature of some functions to return.

docker version

Client:
 Version:           26.1.2
 API version:       1.44 (downgraded from 1.45)
 Go version:        go1.21.10
 Git commit:        211e74b
 Built:             Wed May  8 13:59:48 2024
 OS/Arch:           linux/arm64
 Context:           default

docker info

not relevant

Additional Info

No response

thaJeztah avatar May 15 '24 14:05 thaJeztah