Skip to content

Handle relative source mounts#3469

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

Merged
thaJeztah merged 1 commit into from
Mar 14, 2022
Merged

Handle relative source mounts #3469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion cli/command/container/opts.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,6 +6,7 @@ import (
"fmt"
"os"
"path"
"path/filepath"
"reflect"
"regexp"
"strconv"
Expand All@@ -15,6 +16,7 @@ import (
"github.com/docker/cli/cli/compose/loader"
"github.com/docker/cli/opts"
"github.com/docker/docker/api/types/container"
mounttypes "github.com/docker/docker/api/types/mount"
networktypes "github.com/docker/docker/api/types/network"
"github.com/docker/docker/api/types/strslice"
"github.com/docker/docker/api/types/versions"
Expand DownExpand Up@@ -348,10 +350,25 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con
// add any bind targets to the list of container volumes
for bind := range copts.volumes.GetMap() {
parsed, _ := loader.ParseVolume(bind)

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

@thaJeztahthaJeztahMar 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@rumplrumplMar 14, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.


if parsed.Type == string(mounttypes.TypeBind) {
if arr := strings.SplitN(bind, ":", 2); len(arr) == 2 {
hostPart := arr[0]
if strings.HasPrefix(hostPart, "."+string(filepath.Separator)) || hostPart == "." {
if absHostPart, err := filepath.Abs(hostPart); err == nil {
hostPart = absHostPart
}
}
toBind = hostPart + ":" + arr[1]
}
}

// after creating the bind mount we want to delete it from the copts.volumes values because
// we do not want bind mounts being committed to image configs
binds = append(binds, bind)
binds = append(binds, toBind)
// We should delete from the map (`volumes`) here, as deleting from copts.volumes will not work if
// there are duplicates entries.
delete(volumes, bind)
Expand Down
6 changes: 6 additions & 0 deletions opts/mount.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -4,6 +4,7 @@ import (
"encoding/csv"
"fmt"
"os"
"path/filepath"
"strconv"
"strings"

Expand DownExpand Up@@ -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 == "." {

@thaJeztahthaJeztahMar 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

  • also allowing ../ (relative parent directories)
  • wondering if we should always do a filepath.Clean()

@rumplrumplMar 14, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

For the second point, filepath.Abs already calls Clean

@thaJeztahthaJeztahMar 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

if abs, err := filepath.Abs(value); err == nil {

@thaJeztahthaJeztahMar 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

mount.Source = abs
}
}
case "target", "dst", "destination":
mount.Target = value
case "readonly", "ro":
Expand Down
35 changes: 35 additions & 0 deletions opts/mount_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,6 +2,7 @@ package opts

import (
"os"
"path/filepath"
"testing"

mounttypes "github.com/docker/docker/api/types/mount"
Expand All@@ -28,6 +29,40 @@ func TestMountOptString(t *testing.T) {
assert.Check(t, is.Equal(expected, mount.String()))
}

func TestMountRelative(t *testing.T) {

for _, testcase := range []struct {
name string
path string
bind string
}{
{
name: "Current path",
path: ".",
bind: "type=bind,source=.,target=/target",
}, {
name: "Current path with slash",
path: "./",
bind: "type=bind,source=./,target=/target",
},
} {
t.Run(testcase.name, func(t *testing.T) {
var mount MountOpt
assert.NilError(t, mount.Set(testcase.bind))

mounts := mount.Value()
assert.Assert(t, is.Len(mounts, 1))
abs, err := filepath.Abs(testcase.path)
assert.NilError(t, err)
assert.Check(t, is.DeepEqual(mounttypes.Mount{
Type: mounttypes.TypeBind,
Source: abs,
Target: "/target",
}, mounts[0]))
})
}
}

func TestMountOptSetBindNoErrorBind(t *testing.T) {
for _, testcase := range []string{
// tests several aliases that should have same result.
Expand Down