Handle relative source mounts #3469

rumpl · 2022-03-14T11:09:38Z

- What I did

With this change it is now possible to give a relative path to the --volume and
--mount flags.

$ docker run --mount type=bind,source=./,target=/test ...

$ docker run -v .:/test ...

Fixes #1203

- How I did it

Updated opts parsing for volumes and mounts

- How to verify it

Run docker run --rm -v .:/test busybox ls -l /test and you should see the listing of your current working dir

- Description for the changelog
The run command accepts relative source paths in -v/--volume and -m/--mount flags

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

thaJeztah · 2022-03-14T13:04:03Z

opts/mount.go

@@ -92,6 +93,11 @@ func (m *MountOpt) Set(value string) error {
 			mount.Type = mounttypes.Type(strings.ToLower(value))
 		case "source", "src":
 			mount.Source = value
+			if strings.HasPrefix(value, "."+string(filepath.Separator)) || value == "." {
+				if abs, err := filepath.Abs(value); err == nil {


Do we need to return the error if this fails? (Not entirely sure if needed; it seems like the only case this would fail is if it's failing to resolve os.GetCWD() on Linux (although the Windows code seems to have more possible error conditions)

thaJeztah · 2022-03-14T13:06:35Z

opts/mount_test.go

+func TestMountRelative(t *testing.T) {
+	var mount MountOpt
+
+	assert.NilError(t, mount.Set("type=bind,source=.,target=/target"))


Can you add test-cases for ./ as well? Might be worth creating an array/table with test-cases (and using sub-tests) if that helps making it less DRY.

thaJeztah · 2022-03-14T13:08:43Z

opts/mount.go

@@ -92,6 +93,11 @@ func (m *MountOpt) Set(value string) error {
 			mount.Type = mounttypes.Type(strings.ToLower(value))
 		case "source", "src":
 			mount.Source = value
+			if strings.HasPrefix(value, "."+string(filepath.Separator)) || value == "." {


I guess as a follow-up, we should consider;

also allowing ../ (relative parent directories)

wondering if we should always do a filepath.Clean()

For the second point, filepath.Abs already calls Clean

Ah, sorry I should've been clearer; so I was thinking of the "non-relative path" case, so in a case where we don't hit this if

(but definitely for a follow-up)

thaJeztah · 2022-03-14T13:21:13Z

cli/command/container/opts.go

 		if parsed.Source != "" {
+			toBind := bind


This code is a bit hairy (not your fault), and could use some cleaning up at least (we're parsing the bind into a structured type, but effectively only to do some mild "validation" (however, we're throwing away any error caused by loader.ParseVolume().

One thing we can do already (if I see it correctly), is to use the parsed.Type field to only run this code if it's a bind-mount.

Ideally (but that bit might be ok to be done separate), we should

possibly make have loader.ParseVolume() already do the resolving (I guess that won't work for the compose case, as it would have to resolve relative to the compose-file)

if the above is not an option because of compose-file, we could update the parsed.Source, and;

change types.ServiceVolumeConfig (which is returned by loader.ParseVolume()) to have a .String() (or a separate utility for that), which returns parsed in its canonical string representation.

I added the check for the parsed.Type, I didn't really want to touch the loader code since it touches the compose/swarm things and figured it would surely break something else. I do agree we could have something somewhere that can return a canonical string representation, we should also try and think about how we can make volume and mounts options parsing be in the same place.

With this change it is now possible to give a relative path to the --volume and --mount flags. $ docker run --mount type=bind,source=./,target=/test ... $ docker run -v .:/test ... Fixes docker#1203 Signed-off-by: Djordje Lukic <[email protected]>

codecov-commenter · 2022-03-14T14:33:47Z

Codecov Report

Merging #3469 (ab35e3f) into master (2b52f62) will increase coverage by 0.00%.
The diff coverage is 75.00%.

@@           Coverage Diff           @@
##           master    #3469   +/-   ##
=======================================
  Coverage   58.99%   58.99%           
=======================================
  Files         284      284           
  Lines       23848    23860   +12     
=======================================
+ Hits        14068    14076    +8     
- Misses       8920     8923    +3     
- Partials      860      861    +1

thaJeztah

LGTM

jdruizip · 2023-05-03T16:28:44Z

Although the fix seems to work with paths such as . or ./test, a longer path such as ./test/case seems to always trigger an error on Windows.

When I try
docker run --rm -v ./foo/bar:/test busybox ls -l /test
I always encounter the error:
docker: Error response from daemon: create foo/bar: "foo/bar" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9_.-]" are allowed. If you intended to pass a host directory, use absolute path.

I am on Windows, and a path such as:

docker run --rm -v .\foo\bar:/test busybox ls -l /test (using \ instead of /)

Will run successfully, but then it is no longer valid on unix. It was my understanding that this change was implemented to help cross-platform compatibility, and to sidestep the need for shell-specific pwd calls. docker-compose is able to handle this well, using / on Windows without issue. Even -v %cd%/foo/bar:/foo/bar works well on Windows, so its really not obvious why only backslashes would work in this context.

In any case, even this isn't fixed to be fully cross-platform, the errors should be updated to reflect that not only absolute paths are usable.

thaJeztah · 2023-05-05T11:36:51Z

@jdruizip thanks for reporting; could you open a ticket for that? Things can easily get lost when commented on merged pull-requests 🙈

rumpl force-pushed the relative-mount branch from 6dcf191 to ed9a0d3 Compare March 14, 2022 11:10

thaJeztah added impact/changelog kind/enhancement status/2-code-review area/volumes labels Mar 14, 2022

thaJeztah added this to the 21.xx milestone Mar 14, 2022

rumpl requested review from ndeloof and thaJeztah March 14, 2022 13:00

ndeloof approved these changes Mar 14, 2022

View reviewed changes

thaJeztah reviewed Mar 14, 2022

View reviewed changes

rumpl force-pushed the relative-mount branch from ed9a0d3 to 1e9e6cd Compare March 14, 2022 14:03

Handle relative source mounts

ab35e3f

With this change it is now possible to give a relative path to the --volume and --mount flags. $ docker run --mount type=bind,source=./,target=/test ... $ docker run -v .:/test ... Fixes docker#1203 Signed-off-by: Djordje Lukic <[email protected]>

rumpl force-pushed the relative-mount branch from 1e9e6cd to ab35e3f Compare March 14, 2022 14:18

thaJeztah approved these changes Mar 14, 2022

View reviewed changes

thaJeztah merged commit 21c5391 into docker:master Mar 14, 2022

matthewwo mentioned this pull request Mar 14, 2022

Support for relative path in bind mounts #1273

Closed

rumpl mentioned this pull request Mar 14, 2022

Update mount.go to allow relative file paths #1223

Closed

thaJeztah mentioned this pull request Jan 2, 2023

Reviving #6343 as it looks like the fix wasn't migrated to v2 docker/compose#9410

Closed

thaJeztah mentioned this pull request Jan 15, 2023

Change CLI / Client to use Mounts API as default API #3974

Open

thaJeztah added the docs/revisit label Apr 6, 2023

thaJeztah mentioned this pull request Apr 7, 2023

Docs freshness updates to Docker run command docs #4144

Merged

thaJeztah mentioned this pull request May 19, 2023

docker run add shortcut for mounting PWD #3046

Closed

ciarancourtney mentioned this pull request May 24, 2023

Windows/MSYS2 volume mount path conversion doesn't work rancher-sandbox/rancher-desktop#4380

Open

thaJeztah mentioned this pull request Aug 14, 2023

Add new option --cwd as shortcut for '-v "$PWD:/cwd" -w /cwd' #861

Closed

Benehiko mentioned this pull request Mar 26, 2024

feat: relative parent paths on bind mount src #4966

Open

jbs4bmx mentioned this pull request Aug 15, 2024

SPT Docker mod loading issue jbs4bmx/Olympus#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle relative source mounts #3469

Handle relative source mounts #3469

rumpl commented Mar 14, 2022

thaJeztah Mar 14, 2022

thaJeztah Mar 14, 2022

rumpl Mar 14, 2022

thaJeztah Mar 14, 2022

rumpl Mar 14, 2022

thaJeztah Mar 14, 2022

thaJeztah Mar 14, 2022

rumpl Mar 14, 2022

codecov-commenter commented Mar 14, 2022

thaJeztah left a comment

jdruizip commented May 3, 2023

thaJeztah commented May 5, 2023

Handle relative source mounts #3469

Handle relative source mounts #3469

Conversation

rumpl commented Mar 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 14, 2022

Codecov Report

thaJeztah left a comment

Choose a reason for hiding this comment

jdruizip commented May 3, 2023

thaJeztah commented May 5, 2023