Skip to content
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.

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

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

willww64
Copy link

@willww64 willww64 commented Jul 23, 2024

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

correctly handle configuration file saving when it is a relative symlink

- 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
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.

cli/config/configfile/file.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.75%. Comparing base (a69c036) to head (0118291).
Report is 127 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5282      +/-   ##
==========================================
- Coverage   61.46%   59.75%   -1.71%     
==========================================
  Files         299      345      +46     
  Lines       20854    23430    +2576     
==========================================
+ Hits        12818    14001    +1183     
- Misses       7120     8455    +1335     
- Partials      916      974      +58     

use filepath.EvalSymlink instead of check with filepath.IsAbs

Co-authored-by: Paweł Gronowski <[email protected]>
Signed-off-by: Will Wang <[email protected]>
@willww64 willww64 force-pushed the fix-configfile-relative-symlink branch from d9c9095 to 115fdd9 Compare July 23, 2024 15:19
@willww64
Copy link
Author

willww64 commented Jul 24, 2024

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
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)

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)
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't handle config file properly when it's a relative symlink
4 participants