I noticed this "WARNING" line when reading the code;
|
func LoadDefaultConfigFile(stderr io.Writer) *configfile.ConfigFile { |
|
configFile, err := Load(Dir()) |
|
if err != nil { |
|
fmt.Fprintf(stderr, "WARNING: Error loading config file: %v\n", err) |
|
} |
|
if !configFile.ContainsAuth() { |
|
configFile.CredentialsStore = credentials.DetectDefaultStore(configFile.CredentialsStore) |
|
} |
|
return configFile |
|
} |
From that code, I think we're handling configuration files incorrectly, so I did some testing, and wrote down what I propose to be the correct behavior for these situations
Prepare the test;
mkdir config-example && cd config-example
mkdir ./non-existing-config
mkdir -p ./not-a-file-config/config.json
touch ./not-a-directory-config
mkdir ./invalid-config && echo "Invalid JSON" > ./invalid-config/config.json
mkdir ./empty-config && touch ./empty-config/config.json
mkdir ./whitespace-config && echo '' > ./whitespace-config/config.json
mkdir ./no-access-config && echo "{}" > ./no-access-config/config.json
chmod 600 ./no-access-config/config.json
sudo chown 0:0 ./no-access-config/config.json1. missing configuration file
Specifying a non-existing config.json silently ignores the missing file;
docker --config=./non-existing-config version
Client: Docker Engine - Community
Version: 18.09.1
API version: 1.39
...
Proposed behavior
We should
- fail if a user explicitly specifies a configuration file to use, and the file is missing.
- if no configuration file is specified; ignore the missing file (as having a configuration file is optional)
2. Invalid configuration file
Using an invalid config.json will print a warning, but continue anyway;
docker --config=./invalid-config version
WARNING: Error loading config file: invalid-config/config.json: invalid character 'I' looking for beginning of value
Client: Docker Engine - Community
Version: 18.09.1
API version: 1.39
...
Reporting a "successful" exit-code;
Same for non-accessible, non-file configurations;
docker --config=./not-a-file-config/ version
WARNING: Error loading config file: not-a-file-config/config.json: read not-a-file-config/config.json: is a directory
Client: Docker Engine - Community
docker --config=./no-access-config/config.json
WARNING: Error loading config file: no-access-config/config.json: open no-access-config/config.json: permission denied
docker --config=./not-a-directory-config version
WARNING: Error loading config file: not-a-directory-config/config.json: stat not-a-directory-config/config.json: not a directory
Proposed behavior
We should always fail if the configuration file is invalid. The current behavior only warns about the missing file, then continues with the default configuration.
3. Don't fallback to old file if --config is set
If a non-existing config-path is set, the problem is silenty ignored, and the CLI falls back to using the old (~/.dockercfg) configuration file:
echo '{}' > ~/.dockercfg
docker --config=./non-existing-config version
Client: Docker Engine - Community
Version: 18.09.1
...Proposed behavior
We should only fallback to the old configuration file if no --config path is specified by the user.
If a --config path is set, and that file is not found, we must produce an error because the specified file could not be loaded.
4. Fail on invalid legacy configuration file;
echo 'Invalid JSON' > ~/.dockercfg
docker version
WARNING: Error loading config file: /Users/sebastiaan/.docker/config.json: Invalid Auth config file
Proposed behavior
Same as for the default configuration file: even though the ~/.dockercfg file is optional; if a file is present, it must be valid, otherwise make it a hard fail.
5. (bug) Wrong path is printed when falling back to an invalid ~/.dockercfg
mv ~/.docker/config.json ~/.docker/config.json-temp
echo 'Invalid JSON' > ~/.dockercfg
docker version
WARNING: Error loading config file: /Users/sebastiaan/.docker/config.json: Invalid Auth config file
docker --config=./non-existing-config version
WARNING: Error loading config file: non-existing-config/config.json: Invalid Auth config file
# restore your config.json ;-)
mv ~/.docker/config.json-temp ~/.docker/config.json
Proposed behavior
The warning/error should show the actual path of the invalid file; the current behavior is confusing, because the user will be looking for a ~/.docker/config.json, but won't find one
6. (tbd) Ignore empty configuration file?
Not sure what's best here; ignore empty files, or fail? If we decide to fail, the error message could be improved (e.g. "the file is empty" instead of EOF).
docker --config=./empty-config version
WARNING: Error loading config file: empty-config/config.json: EOF
docker --config=./whitespace-config version
WARNING: Error loading config file: whitespace-config/config.json: EOF
I noticed this "WARNING" line when reading the code;
cli/cli/config/config.go
Lines 117 to 126 in 48bd4c6
From that code, I think we're handling configuration files incorrectly, so I did some testing, and wrote down what I propose to be the correct behavior for these situations
Prepare the test;
1. missing configuration file
Specifying a non-existing
config.jsonsilently ignores the missing file;Proposed behavior
We should
2. Invalid configuration file
Using an invalid
config.jsonwill print a warning, but continue anyway;Reporting a "successful" exit-code;
Same for non-accessible, non-file configurations;
docker --config=./not-a-file-config/ version WARNING: Error loading config file: not-a-file-config/config.json: read not-a-file-config/config.json: is a directory Client: Docker Engine - Community docker --config=./no-access-config/config.json WARNING: Error loading config file: no-access-config/config.json: open no-access-config/config.json: permission denied docker --config=./not-a-directory-config version WARNING: Error loading config file: not-a-directory-config/config.json: stat not-a-directory-config/config.json: not a directoryProposed behavior
We should always fail if the configuration file is invalid. The current behavior only warns about the missing file, then continues with the default configuration.
3. Don't fallback to old file if
--configis setIf a non-existing config-path is set, the problem is silenty ignored, and the CLI falls back to using the old (
~/.dockercfg) configuration file:Proposed behavior
We should only fallback to the old configuration file if no
--configpath is specified by the user.If a
--configpath is set, and that file is not found, we must produce an error because the specified file could not be loaded.4. Fail on invalid legacy configuration file;
Proposed behavior
Same as for the default configuration file: even though the
~/.dockercfgfile is optional; if a file is present, it must be valid, otherwise make it a hard fail.5. (bug) Wrong path is printed when falling back to an invalid
~/.dockercfgProposed behavior
The warning/error should show the actual path of the invalid file; the current behavior is confusing, because the user will be looking for a
~/.docker/config.json, but won't find one6. (tbd) Ignore empty configuration file?
Not sure what's best here; ignore empty files, or fail? If we decide to fail, the error message could be improved (e.g. "the file is empty" instead of
EOF).