cli icon indicating copy to clipboard operation
cli copied to clipboard

correctly handle configuration file saving when it is a relative symlink

Open willww64 opened this issue 1 year ago • 2 comments

fixes: #5281

- What I did Correctly hand the cases with config file being a relative symlink.

- How I did it Check whether the result path of os.Readlink(cfgFile) is a relative path, if so, get the absolute path of it.

- How to verify it

mkdir ~/.docker
echo '{}' > ~/.docker/config-company.json
ln -sf ~/.docker/config-company.json ~/.docker/config.json
docker login -u <user>
grep auths ~/.docker/config.json && echo OK || echo Failed

- Description for the changelog

correctly handle configuration file saving when it is a relative symlink

- A picture of a cute animal (not mandatory but encouraged)

willww64 avatar Jul 23 '24 14:07 willww64

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 55.04%. Comparing base (ae922ec) to head (1015d15). Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5282      +/-   ##
==========================================
+ Coverage   55.01%   55.04%   +0.02%     
==========================================
  Files         361      361              
  Lines       30137    30142       +5     
==========================================
+ Hits        16581    16591      +10     
+ Misses      12598    12594       -4     
+ Partials      958      957       -1     
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov-commenter avatar Jul 23 '24 14:07 codecov-commenter

The TestLoadDanglingSymlink test failed.

According to that test's comment, Docker CLI currently doesn't treat a dangling symlink as an error. However, filepath.EvalSymlinks does treat a dangling symlink as an error. Consequently, when the symlink is dangling, the value of cfgFile isn't assigned to the "resolved" path of the symlink, causing the last assertion in TestLoadDanglingSymlink to fail.

After conducting local tests, I've compiled the following table:

os.Readlink() filepath.EvalSymlinks()
Real file
Symlink
Dangling symlink
Resolve nested symlink

In summary:

  • os.Readlink is to check whether a file is a link or not and simply returns the raw target of that link without trying to resolve the path
  • filepath.EvalSymlinks checks if the file is valid (regardless of whether it's a symlink) and returns its resolved path.

~It appears that os.Readlink is the only suitable solution in this case. I've reset the code back to os.Readlink and see if it can pass the test.~

Update: After more investigation, I found a limitation of os.Readlink: it only dereferences one layer of symlinks and doesn't handle "nested" symlinks (i.e., a symlink pointing to another symlink). So filetype.EvalSymlinks is better suited for supporting nested symlinks. To allow for dangling symlinks with filepath.EvalSymlinks, I've added some code to get around this issue, by extracting the path from the "NotExist" error. I've run docker buildx bake test on my local machine, and it passed successfully.

willww64 avatar Jul 24 '24 02:07 willww64

@vvoland I rebased and squashed this one; also adjusted the error-matching; https://github.com/docker/cli/pull/5282#discussion_r2102051077

thaJeztah avatar May 22 '25 09:05 thaJeztah