Handle errors on close in config file write.

[cpuguy83]

I'm not sure if this fixes anything, however I have seen some weird
behavior on Windows where temp config files are left around and there
doesn't seem to be any errors reported, and an empty config.json.

[codecov-commenter]

Codecov Report

Merging #2595 into master will decrease coverage by 0.00%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##           master    #2595      +/-   ##
==========================================
- Coverage   58.05%   58.04%   -0.01%     
==========================================
  Files         295      295              
  Lines       21165    21170       +5     
==========================================
+ Hits        12288    12289       +1     
- Misses       7975     7977       +2     
- Partials      902      904       +2     

[cpuguy83]

The existence of https://success.docker.com/article/warning-error-loading-configjson seems to indicate there is a problem here 😂

[thaJeztah]

The existence of https://success.docker.com/article/warning-error-loading-configjson seems to indicate there is a problem here 😂

That seems to be about loading the config file, not saving? Or is that hitting this code?

WARNING: Error loading config file:.docker\config.json - EOF

Also wondering if that's related to empty config files, and if we should ignore empty files (just like we ignore empty {} JSON) 🤔

Testing if we're still producing an error for empty files, and it looks like we do;

mkdir -p ~/.docker && touch ~/.docker/config.json

docker pull busybox
WARNING: Error loading config file: /root/.docker/config.json: EOF

[thaJeztah]

Given that in this case, we have exited early, and didn't update the actual ~/.docker/config.json, perhaps we should print this as a warning?

Something like

WARNING: Failed to update config file: error cleaning up temp file 

WDYT?

[cpuguy83]

We could, but we should already get an error returned.

[cpuguy83]

That seems to be about loading the config file, not saving? Or is that hitting this code?

Right but how did it get that way?

[cpuguy83]

The symptom I've seen is multiple temp files left behind and an empty config file, btw.

[thaJeztah]

Not sure if there's a situation where that's possible (thinking if an error occurs on os.Rename(); not sure how atomic that is); should we ignore errors if the file didn't exist?

Suggested change

	if err := os.Remove(temp.Name()); err != nil {
	if err := os.Remove(temp.Name()); err != nil && !os.IsNotExist(err) {

Guess it's not needed 😅

[thaJeztah]

Should we consider using something like https://github.com/moby/moby/blob/4609153995b59f54b507446eb2b2cca9ebbbf8e8/pkg/ioutils/fswriters.go#L13 or https://github.com/google/renameio (which seems to handle specific corner-cases)?

Problem I see with that approach is that we would (at least not "out of the box") loose the copyFilePermissions() that would preserve ownership of the file.

Hm.. looks like we may need to have a look at that in general. Here's some fun thing I tried;

mkdir -p ~/.docker
touch ~/real-config.json
ln -s ~/real-config.json ~/.docker/config.json

ls -la ~/.docker/config.json
# lrwxrwxrwx 1 root root 22 Jun 23 12:34 /root/.docker/config.json -> /root/real-config.json

docker login
# Username: thajeztah
# Password:

# Login Succeeded

ls -la ~/.docker/config.json
-rw-r--r-- 1 root root 229 Jun 23 12:36 /root/.docker/config.json

Looks like we need to resolve symlinks before we rename. Something like;

cfgFile := configFile.Filename
if f, err := os.Readlink(cfgFile); err == nil {
	cfgFile = f
}
// Try copying the current config file (if any) ownership and permissions
copyFilePermissions(cfgFile, temp.Name())
return os.Rename(temp.Name(), cfgFile)

[thaJeztah]

Opened #2599 for consideration (if we think it's ok to ignore an empty config file); #2595 (comment)

[cpuguy83]

Ping

[thaJeztah]

LGTM

[silvin-lubecki]

LGTM