docker should fail if config file is invalid
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 6.1k
- Forks
- 2.2k
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 43
Description
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
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in cli/config/config.go at the referenced load and warning paths, then trace how docker login, logout, and context use handle the returned error. Reproduce the malformed ~/.docker/config.json case and inspect the relevant callers; done means invalid configuration is not treated as an empty file or overwritten during these flows.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- docker, go
- Domain
- cli
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100