Skip to content

correctly handle configuration file saving when it is a relative symlink #5282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 22, 2025
Merged

correctly handle configuration file saving when it is a relative symlink #5282

merged 1 commit into from
May 22, 2025

Conversation

willww64
Copy link
Contributor

@willww64 willww64 commented Jul 23, 2024 •
edited by vvoland
Loading

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 config-company.json ~/.docker/config.json   # create a relative symlink
docker login -u <user>
grep auths ~/.docker/config.json && echo OK || echo Failed

- Description for the changelog

Fix an issue where the CLI would not correctly save the configuration file (`~/.docker/config.json`) if it was a relative symbolic link.

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

@willww64 willww64 force-pushed the fix-configfile-relative-symlink branch from d4a0fb0 to c90705a Compare July 23, 2024 14:17
@willww64 willww64 changed the title fix bug with config file being a relative symlink correctly handle configuration file saving when it is a relative symlink Jul 23, 2024
vvoland
vvoland requested changes Jul 23, 2024
View reviewed changes
Copy link
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have just one suggestion to make the code simpler.

@vvoland vvoland added this to the 28.0.0 milestone Jul 23, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

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
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@willww64 willww64 force-pushed the fix-configfile-relative-symlink branch from d9c9095 to 115fdd9 Compare July 23, 2024 15:19
@willww64
Copy link
Contributor Author

willww64 commented Jul 24, 2024 •
edited
Loading

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 willww64 force-pushed the fix-configfile-relative-symlink branch from 115fdd9 to c90705a Compare July 24, 2024 02:37
@willww64 willww64 requested a review from vvoland July 24, 2024 12:05
@thaJeztah thaJeztah closed this Sep 12, 2024
@thaJeztah thaJeztah reopened this Sep 12, 2024
vvoland
vvoland reviewed Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM! (after squashing the commits)

thaJeztah
thaJeztah reviewed Nov 7, 2024
View reviewed changes
cli/config/configfile/file.go Outdated
cfgFile = f
} else if os.IsNotExist(err) {
// extract the path from the error if the configfile does not exist or is a dangling symlink
cfgFile = err.(*os.PathError).Path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we can hit that situation, but this could panic, because os.IsNotExist also considers syscall errors; https://go.dev/play/p/5VjH5lK-YDB

package main

import (
	"os"
	"syscall"
	"testing"
)

func TestCheckError(t *testing.T) {
	err := func() error {
		return syscall.ENOENT
	}()
	if os.IsNotExist(err) {
		t.Log(err)
		cfgFile := err.(*os.PathError).Path
		t.Log(cfgFile)
	}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to use errors.As;

// extract the path from the error if the configfile does not exist or is a dangling symlink
var pathError *os.PathError
if errors.As(err, &pathError) {
	cfgFile = pathError.Path
}

@thaJeztah thaJeztah modified the milestones: 28.0.0, 28.0.5 Mar 31, 2025
@thaJeztah thaJeztah modified the milestones: 28.1.1, 28.1.2 Apr 23, 2025
@thaJeztah thaJeztah modified the milestones: 28.1.2, 28.2.0 May 15, 2025
@willww64 @vvoland @thaJeztah
fix bug with config file being a relative symlink
1015d15 
- use filepath.EvalSymlink instead of check with filepath.IsAbs
- allow for dangling symlinks
- extract path from error when NotExist error occurs

Co-authored-by: Paweł Gronowski <[email protected]>
Co-authored-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Will Wang <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the fix-configfile-relative-symlink branch from 0118291 to 1015d15 Compare May 22, 2025 09:11
@thaJeztah
Copy link
Member

@vvoland I rebased and squashed this one; also adjusted the error-matching; #5282 (comment)

@vvoland vvoland merged commit f1fb3e3 into docker:master May 22, 2025
87 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah left review comments

@vvoland vvoland vvoland approved these changes

Assignees
No one assigned
Labels
impact/changelog kind/bugfix PR's that fix bugs kind/enhancement status/2-code-review
Projects
None yet
Milestone
28.2.0
Development

Successfully merging this pull request may close these issues.

Can't handle config file properly when it's a relative symlink
4 participants
@willww64 @codecov-commenter @thaJeztah @vvoland