From 732261f774f724a44f79c3138459cf190cb01558 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 9 May 2017 19:21:17 -0400 Subject: [PATCH] Use compose volume spec parser for container volume flag Restore testcases for Volume spec parsing. And correctly interpret the parsed volume. Signed-off-by: Daniel Nephin --- cli/command/container/opts.go | 65 +------------------- cli/command/container/opts_test.go | 96 +++++++----------------------- cli/compose/loader/loader.go | 2 +- cli/compose/loader/volume.go | 8 ++- cli/compose/loader/volume_test.go | 77 +++++++++++++++++++----- 5 files changed, 92 insertions(+), 156 deletions(-) diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index 82d7426c75..cf1f931b29 100644 --- a/cli/command/container/opts.go +++ b/cli/command/container/opts.go @@ -12,6 +12,7 @@ import ( "time" "github.com/Sirupsen/logrus" + "github.com/docker/cli/cli/compose/loader" "github.com/docker/cli/opts" "github.com/docker/docker/api/types/container" networktypes "github.com/docker/docker/api/types/network" @@ -332,7 +333,8 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*containerConfig, err volumes := copts.volumes.GetMap() // add any bind targets to the list of container volumes for bind := range copts.volumes.GetMap() { - if arr := volumeSplitN(bind, 2); len(arr) > 1 { + parsed, _ := loader.ParseVolume(bind) + if parsed.Source != "" { // 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) @@ -827,67 +829,6 @@ func validatePath(val string, validator func(string) bool) (string, error) { return val, nil } -// volumeSplitN splits raw into a maximum of n parts, separated by a separator colon. -// A separator colon is the last `:` character in the regex `[:\\]?[a-zA-Z]:` (note `\\` is `\` escaped). -// In Windows driver letter appears in two situations: -// a. `^[a-zA-Z]:` (A colon followed by `^[a-zA-Z]:` is OK as colon is the separator in volume option) -// b. A string in the format like `\\?\C:\Windows\...` (UNC). -// Therefore, a driver letter can only follow either a `:` or `\\` -// This allows to correctly split strings such as `C:\foo:D:\:rw` or `/tmp/q:/foo`. -func volumeSplitN(raw string, n int) []string { - var array []string - if len(raw) == 0 || raw[0] == ':' { - // invalid - return nil - } - // numberOfParts counts the number of parts separated by a separator colon - numberOfParts := 0 - // left represents the left-most cursor in raw, updated at every `:` character considered as a separator. - left := 0 - // right represents the right-most cursor in raw incremented with the loop. Note this - // starts at index 1 as index 0 is already handle above as a special case. - for right := 1; right < len(raw); right++ { - // stop parsing if reached maximum number of parts - if n >= 0 && numberOfParts >= n { - break - } - if raw[right] != ':' { - continue - } - potentialDriveLetter := raw[right-1] - if (potentialDriveLetter >= 'A' && potentialDriveLetter <= 'Z') || (potentialDriveLetter >= 'a' && potentialDriveLetter <= 'z') { - if right > 1 { - beforePotentialDriveLetter := raw[right-2] - // Only `:` or `\\` are checked (`/` could fall into the case of `/tmp/q:/foo`) - if beforePotentialDriveLetter != ':' && beforePotentialDriveLetter != '\\' { - // e.g. `C:` is not preceded by any delimiter, therefore it was not a drive letter but a path ending with `C:`. - array = append(array, raw[left:right]) - left = right + 1 - numberOfParts++ - } - // else, `C:` is considered as a drive letter and not as a delimiter, so we continue parsing. - } - // if right == 1, then `C:` is the beginning of the raw string, therefore `:` is again not considered a delimiter and we continue parsing. - } else { - // if `:` is not preceded by a potential drive letter, then consider it as a delimiter. - array = append(array, raw[left:right]) - left = right + 1 - numberOfParts++ - } - } - // need to take care of the last part - if left < len(raw) { - if n >= 0 && numberOfParts >= n { - // if the maximum number of parts is reached, just append the rest to the last part - // left-1 is at the last `:` that needs to be included since not considered a separator. - array[n-1] += raw[left-1:] - } else { - array = append(array, raw[left:]) - } - } - return array -} - // validateAttach validates that the specified string is a valid attach option. func validateAttach(val string) (string, error) { s := strings.ToLower(val) diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go index 6001cde2c0..25e5b04fc8 100644 --- a/cli/command/container/opts_test.go +++ b/cli/command/container/opts_test.go @@ -19,6 +19,7 @@ import ( "github.com/pkg/errors" "github.com/spf13/pflag" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestValidateAttach(t *testing.T) { @@ -366,51 +367,46 @@ func TestParseDevice(t *testing.T) { func TestParseModes(t *testing.T) { // ipc ko - if _, _, _, err := parseRun([]string{"--ipc=container:", "img", "cmd"}); err == nil || err.Error() != "--ipc: invalid IPC mode" { - t.Fatalf("Expected an error with message '--ipc: invalid IPC mode', got %v", err) - } + _, _, _, err := parseRun([]string{"--ipc=container:", "img", "cmd"}) + testutil.ErrorContains(t, err, "--ipc: invalid IPC mode") + // ipc ok _, hostconfig, _, err := parseRun([]string{"--ipc=host", "img", "cmd"}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) if !hostconfig.IpcMode.Valid() { t.Fatalf("Expected a valid IpcMode, got %v", hostconfig.IpcMode) } + // pid ko - if _, _, _, err := parseRun([]string{"--pid=container:", "img", "cmd"}); err == nil || err.Error() != "--pid: invalid PID mode" { - t.Fatalf("Expected an error with message '--pid: invalid PID mode', got %v", err) - } + _, _, _, err = parseRun([]string{"--pid=container:", "img", "cmd"}) + testutil.ErrorContains(t, err, "--pid: invalid PID mode") + // pid ok _, hostconfig, _, err = parseRun([]string{"--pid=host", "img", "cmd"}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) if !hostconfig.PidMode.Valid() { t.Fatalf("Expected a valid PidMode, got %v", hostconfig.PidMode) } + // uts ko - if _, _, _, err := parseRun([]string{"--uts=container:", "img", "cmd"}); err == nil || err.Error() != "--uts: invalid UTS mode" { - t.Fatalf("Expected an error with message '--uts: invalid UTS mode', got %v", err) - } + _, _, _, err = parseRun([]string{"--uts=container:", "img", "cmd"}) + testutil.ErrorContains(t, err, "--uts: invalid UTS mode") + // uts ok _, hostconfig, _, err = parseRun([]string{"--uts=host", "img", "cmd"}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) if !hostconfig.UTSMode.Valid() { t.Fatalf("Expected a valid UTSMode, got %v", hostconfig.UTSMode) } + // shm-size ko expectedErr := `invalid argument "a128m" for --shm-size=a128m: invalid size: 'a128m'` - if _, _, _, err = parseRun([]string{"--shm-size=a128m", "img", "cmd"}); err == nil || err.Error() != expectedErr { - t.Fatalf("Expected an error with message '%v', got %v", expectedErr, err) - } + _, _, _, err = parseRun([]string{"--shm-size=a128m", "img", "cmd"}) + testutil.ErrorContains(t, err, expectedErr) + // shm-size ok _, hostconfig, _, err = parseRun([]string{"--shm-size=128m", "img", "cmd"}) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) if hostconfig.ShmSize != 134217728 { t.Fatalf("Expected a valid ShmSize, got %d", hostconfig.ShmSize) } @@ -754,58 +750,6 @@ func callDecodeContainerConfig(volumes []string, binds []string) (*container.Con return c, h, err } -func TestVolumeSplitN(t *testing.T) { - for _, x := range []struct { - input string - n int - expected []string - }{ - {`C:\foo:d:`, -1, []string{`C:\foo`, `d:`}}, - {`:C:\foo:d:`, -1, nil}, - {`/foo:/bar:ro`, 3, []string{`/foo`, `/bar`, `ro`}}, - {`/foo:/bar:ro`, 2, []string{`/foo`, `/bar:ro`}}, - {`C:\foo\:/foo`, -1, []string{`C:\foo\`, `/foo`}}, - - {`d:\`, -1, []string{`d:\`}}, - {`d:`, -1, []string{`d:`}}, - {`d:\path`, -1, []string{`d:\path`}}, - {`d:\path with space`, -1, []string{`d:\path with space`}}, - {`d:\pathandmode:rw`, -1, []string{`d:\pathandmode`, `rw`}}, - {`c:\:d:\`, -1, []string{`c:\`, `d:\`}}, - {`c:\windows\:d:`, -1, []string{`c:\windows\`, `d:`}}, - {`c:\windows:d:\s p a c e`, -1, []string{`c:\windows`, `d:\s p a c e`}}, - {`c:\windows:d:\s p a c e:RW`, -1, []string{`c:\windows`, `d:\s p a c e`, `RW`}}, - {`c:\program files:d:\s p a c e i n h o s t d i r`, -1, []string{`c:\program files`, `d:\s p a c e i n h o s t d i r`}}, - {`0123456789name:d:`, -1, []string{`0123456789name`, `d:`}}, - {`MiXeDcAsEnAmE:d:`, -1, []string{`MiXeDcAsEnAmE`, `d:`}}, - {`name:D:`, -1, []string{`name`, `D:`}}, - {`name:D::rW`, -1, []string{`name`, `D:`, `rW`}}, - {`name:D::RW`, -1, []string{`name`, `D:`, `RW`}}, - {`c:/:d:/forward/slashes/are/good/too`, -1, []string{`c:/`, `d:/forward/slashes/are/good/too`}}, - {`c:\Windows`, -1, []string{`c:\Windows`}}, - {`c:\Program Files (x86)`, -1, []string{`c:\Program Files (x86)`}}, - - {``, -1, nil}, - {`.`, -1, []string{`.`}}, - {`..\`, -1, []string{`..\`}}, - {`c:\:..\`, -1, []string{`c:\`, `..\`}}, - {`c:\:d:\:xyzzy`, -1, []string{`c:\`, `d:\`, `xyzzy`}}, - - // Cover directories with one-character name - {`/tmp/x/y:/foo/x/y`, -1, []string{`/tmp/x/y`, `/foo/x/y`}}, - } { - res := volumeSplitN(x.input, x.n) - if len(res) < len(x.expected) { - t.Fatalf("input: %v, expected: %v, got: %v", x.input, x.expected, res) - } - for i, e := range res { - if e != x.expected[i] { - t.Fatalf("input: %v, expected: %v, got: %v", x.input, x.expected, res) - } - } - } -} - func TestValidateDevice(t *testing.T) { valid := []string{ "/home", diff --git a/cli/compose/loader/loader.go b/cli/compose/loader/loader.go index b9d601664c..b456e1824d 100644 --- a/cli/compose/loader/loader.go +++ b/cli/compose/loader/loader.go @@ -564,7 +564,7 @@ func transformStringSourceMap(data interface{}) (interface{}, error) { func transformServiceVolumeConfig(data interface{}) (interface{}, error) { switch value := data.(type) { case string: - return parseVolume(value) + return ParseVolume(value) case map[string]interface{}: return data, nil default: diff --git a/cli/compose/loader/volume.go b/cli/compose/loader/volume.go index f39f27292b..b026cf150f 100644 --- a/cli/compose/loader/volume.go +++ b/cli/compose/loader/volume.go @@ -12,7 +12,8 @@ import ( const endOfSpec = rune(0) -func parseVolume(spec string) (types.ServiceVolumeConfig, error) { +// ParseVolume parses a volume spec without any knowledge of the target platform +func ParseVolume(spec string) (types.ServiceVolumeConfig, error) { volume := types.ServiceVolumeConfig{} switch len(spec) { @@ -31,6 +32,7 @@ func parseVolume(spec string) (types.ServiceVolumeConfig, error) { buffer = append(buffer, char) case char == ':' || char == endOfSpec: if err := populateFieldFromBuffer(char, buffer, &volume); err != nil { + populateType(&volume) return volume, errors.Wrapf(err, "invalid spec: %s", spec) } buffer = []rune{} @@ -38,6 +40,7 @@ func parseVolume(spec string) (types.ServiceVolumeConfig, error) { buffer = append(buffer, char) } } + populateType(&volume) return volume, nil } @@ -75,9 +78,8 @@ func populateFieldFromBuffer(char rune, buffer []rune, volume *types.ServiceVolu default: if isBindOption(option) { volume.Bind = &types.ServiceVolumeBind{Propagation: option} - } else { - return errors.Errorf("unknown option: %s", option) } + // ignore unknown options } } return nil diff --git a/cli/compose/loader/volume_test.go b/cli/compose/loader/volume_test.go index d2d1afe9ea..f1b90fe8ea 100644 --- a/cli/compose/loader/volume_test.go +++ b/cli/compose/loader/volume_test.go @@ -1,6 +1,7 @@ package loader import ( + "fmt" "testing" "github.com/docker/cli/cli/compose/types" @@ -10,7 +11,7 @@ import ( func TestParseVolumeAnonymousVolume(t *testing.T) { for _, path := range []string{"/path", "/path/foo"} { - volume, err := parseVolume(path) + volume, err := ParseVolume(path) expected := types.ServiceVolumeConfig{Type: "volume", Target: path} assert.NoError(t, err) assert.Equal(t, expected, volume) @@ -19,7 +20,7 @@ func TestParseVolumeAnonymousVolume(t *testing.T) { func TestParseVolumeAnonymousVolumeWindows(t *testing.T) { for _, path := range []string{"C:\\path", "Z:\\path\\foo"} { - volume, err := parseVolume(path) + volume, err := ParseVolume(path) expected := types.ServiceVolumeConfig{Type: "volume", Target: path} assert.NoError(t, err) assert.Equal(t, expected, volume) @@ -27,13 +28,13 @@ func TestParseVolumeAnonymousVolumeWindows(t *testing.T) { } func TestParseVolumeTooManyColons(t *testing.T) { - _, err := parseVolume("/foo:/foo:ro:foo") + _, err := ParseVolume("/foo:/foo:ro:foo") assert.EqualError(t, err, "invalid spec: /foo:/foo:ro:foo: too many colons") } func TestParseVolumeShortVolumes(t *testing.T) { for _, path := range []string{".", "/a"} { - volume, err := parseVolume(path) + volume, err := ParseVolume(path) expected := types.ServiceVolumeConfig{Type: "volume", Target: path} assert.NoError(t, err) assert.Equal(t, expected, volume) @@ -42,14 +43,14 @@ func TestParseVolumeShortVolumes(t *testing.T) { func TestParseVolumeMissingSource(t *testing.T) { for _, spec := range []string{":foo", "/foo::ro"} { - _, err := parseVolume(spec) + _, err := ParseVolume(spec) testutil.ErrorContains(t, err, "empty section between colons") } } func TestParseVolumeBindMount(t *testing.T) { for _, path := range []string{"./foo", "~/thing", "../other", "/foo", "/home/user"} { - volume, err := parseVolume(path + ":/target") + volume, err := ParseVolume(path + ":/target") expected := types.ServiceVolumeConfig{ Type: "bind", Source: path, @@ -67,7 +68,7 @@ func TestParseVolumeRelativeBindMountWindows(t *testing.T) { "../other", "D:\\path", "/home/user", } { - volume, err := parseVolume(path + ":d:\\target") + volume, err := ParseVolume(path + ":d:\\target") expected := types.ServiceVolumeConfig{ Type: "bind", Source: path, @@ -79,7 +80,7 @@ func TestParseVolumeRelativeBindMountWindows(t *testing.T) { } func TestParseVolumeWithBindOptions(t *testing.T) { - volume, err := parseVolume("/source:/target:slave") + volume, err := ParseVolume("/source:/target:slave") expected := types.ServiceVolumeConfig{ Type: "bind", Source: "/source", @@ -91,7 +92,7 @@ func TestParseVolumeWithBindOptions(t *testing.T) { } func TestParseVolumeWithBindOptionsWindows(t *testing.T) { - volume, err := parseVolume("C:\\source\\foo:D:\\target:ro,rprivate") + volume, err := ParseVolume("C:\\source\\foo:D:\\target:ro,rprivate") expected := types.ServiceVolumeConfig{ Type: "bind", Source: "C:\\source\\foo", @@ -104,12 +105,12 @@ func TestParseVolumeWithBindOptionsWindows(t *testing.T) { } func TestParseVolumeWithInvalidVolumeOptions(t *testing.T) { - _, err := parseVolume("name:/target:bogus") - assert.EqualError(t, err, "invalid spec: name:/target:bogus: unknown option: bogus") + _, err := ParseVolume("name:/target:bogus") + assert.NoError(t, err) } func TestParseVolumeWithVolumeOptions(t *testing.T) { - volume, err := parseVolume("name:/target:nocopy") + volume, err := ParseVolume("name:/target:nocopy") expected := types.ServiceVolumeConfig{ Type: "volume", Source: "name", @@ -122,7 +123,7 @@ func TestParseVolumeWithVolumeOptions(t *testing.T) { func TestParseVolumeWithReadOnly(t *testing.T) { for _, path := range []string{"./foo", "/home/user"} { - volume, err := parseVolume(path + ":/target:ro") + volume, err := ParseVolume(path + ":/target:ro") expected := types.ServiceVolumeConfig{ Type: "bind", Source: path, @@ -136,7 +137,7 @@ func TestParseVolumeWithReadOnly(t *testing.T) { func TestParseVolumeWithRW(t *testing.T) { for _, path := range []string{"./foo", "/home/user"} { - volume, err := parseVolume(path + ":/target:rw") + volume, err := ParseVolume(path + ":/target:rw") expected := types.ServiceVolumeConfig{ Type: "bind", Source: path, @@ -151,3 +152,51 @@ func TestParseVolumeWithRW(t *testing.T) { func TestIsFilePath(t *testing.T) { assert.False(t, isFilePath("a界")) } + +// Preserve the test cases for VolumeSplitN +func TestParseVolumeSplitCases(t *testing.T) { + for casenumber, x := range []struct { + input string + n int + expected []string + }{ + {`C:\foo:d:`, -1, []string{`C:\foo`, `d:`}}, + {`:C:\foo:d:`, -1, nil}, + {`/foo:/bar:ro`, 3, []string{`/foo`, `/bar`, `ro`}}, + {`/foo:/bar:ro`, 2, []string{`/foo`, `/bar:ro`}}, + {`C:\foo\:/foo`, -1, []string{`C:\foo\`, `/foo`}}, + {`d:\`, -1, []string{`d:\`}}, + {`d:`, -1, []string{`d:`}}, + {`d:\path`, -1, []string{`d:\path`}}, + {`d:\path with space`, -1, []string{`d:\path with space`}}, + {`d:\pathandmode:rw`, -1, []string{`d:\pathandmode`, `rw`}}, + + {`c:\:d:\`, -1, []string{`c:\`, `d:\`}}, + {`c:\windows\:d:`, -1, []string{`c:\windows\`, `d:`}}, + {`c:\windows:d:\s p a c e`, -1, []string{`c:\windows`, `d:\s p a c e`}}, + {`c:\windows:d:\s p a c e:RW`, -1, []string{`c:\windows`, `d:\s p a c e`, `RW`}}, + {`c:\program files:d:\s p a c e i n h o s t d i r`, -1, []string{`c:\program files`, `d:\s p a c e i n h o s t d i r`}}, + {`0123456789name:d:`, -1, []string{`0123456789name`, `d:`}}, + {`MiXeDcAsEnAmE:d:`, -1, []string{`MiXeDcAsEnAmE`, `d:`}}, + {`name:D:`, -1, []string{`name`, `D:`}}, + {`name:D::rW`, -1, []string{`name`, `D:`, `rW`}}, + {`name:D::RW`, -1, []string{`name`, `D:`, `RW`}}, + + {`c:/:d:/forward/slashes/are/good/too`, -1, []string{`c:/`, `d:/forward/slashes/are/good/too`}}, + {`c:\Windows`, -1, []string{`c:\Windows`}}, + {`c:\Program Files (x86)`, -1, []string{`c:\Program Files (x86)`}}, + {``, -1, nil}, + {`.`, -1, []string{`.`}}, + {`..\`, -1, []string{`..\`}}, + {`c:\:..\`, -1, []string{`c:\`, `..\`}}, + {`c:\:d:\:xyzzy`, -1, []string{`c:\`, `d:\`, `xyzzy`}}, + // Cover directories with one-character name + {`/tmp/x/y:/foo/x/y`, -1, []string{`/tmp/x/y`, `/foo/x/y`}}, + } { + parsed, _ := ParseVolume(x.input) + + expected := len(x.expected) > 1 + msg := fmt.Sprintf("Case %d: %s", casenumber, x.input) + assert.Equal(t, expected, parsed.Source != "", msg) + } +}