Merge pull request #152 from dnephin/improve-volume-parse

Improve volume spec parsing

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)

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" {
+	_, _, _, err := parseRun([]string{"--ipc=container:", "img", "cmd"})
-		t.Fatalf("Expected an error with message '--ipc: invalid IPC mode', got %v", err)
+	testutil.ErrorContains(t, err, "--ipc: invalid IPC mode")
-	}
+
 	// ipc ok
 	_, hostconfig, _, err := parseRun([]string{"--ipc=host", "img", "cmd"})
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatal(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" {
+	_, _, _, err = parseRun([]string{"--pid=container:", "img", "cmd"})
-		t.Fatalf("Expected an error with message '--pid: invalid PID mode', got %v", err)
+	testutil.ErrorContains(t, err, "--pid: invalid PID mode")
-	}
+
 	// pid ok
 	_, hostconfig, _, err = parseRun([]string{"--pid=host", "img", "cmd"})
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatal(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" {
+	_, _, _, err = parseRun([]string{"--uts=container:", "img", "cmd"})
-		t.Fatalf("Expected an error with message '--uts: invalid UTS mode', got %v", err)
+	testutil.ErrorContains(t, err, "--uts: invalid UTS mode")
-	}
+
 	// uts ok
 	_, hostconfig, _, err = parseRun([]string{"--uts=host", "img", "cmd"})
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatal(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 {
+	_, _, _, err = parseRun([]string{"--shm-size=a128m", "img", "cmd"})
-		t.Fatalf("Expected an error with message '%v', got %v", expectedErr, err)
+	testutil.ErrorContains(t, err, expectedErr)
-	}
+
 	// shm-size ok
 	_, hostconfig, _, err = parseRun([]string{"--shm-size=128m", "img", "cmd"})
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatal(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",

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:

cli/compose/loader/volume.go

@@ -10,7 +10,10 @@ import (
 	"github.com/pkg/errors"
 )
 
-func parseVolume(spec string) (types.ServiceVolumeConfig, error) {
+const endOfSpec = rune(0)
+
+// 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) {
@@ -23,12 +26,13 @@ func parseVolume(spec string) (types.ServiceVolumeConfig, error) {
 	}
 
 	buffer := []rune{}
-	for _, char := range spec {
+	for _, char := range spec + string(endOfSpec) {
 		switch {
-		case isWindowsDrive(char, buffer, volume):
+		case isWindowsDrive(buffer, char):
 			buffer = append(buffer, char)
-		case 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{}
@@ -37,14 +41,11 @@ func parseVolume(spec string) (types.ServiceVolumeConfig, error) {
 		}
 	}
 
-	if err := populateFieldFromBuffer(rune(0), buffer, &volume); err != nil {
-		return volume, errors.Wrapf(err, "invalid spec: %s", spec)
-	}
 	populateType(&volume)
 	return volume, nil
 }
 
-func isWindowsDrive(char rune, buffer []rune, volume types.ServiceVolumeConfig) bool {
+func isWindowsDrive(buffer []rune, char rune) bool {
 	return char == ':' && len(buffer) == 1 && unicode.IsLetter(buffer[0])
 }
 
@@ -54,7 +55,7 @@ func populateFieldFromBuffer(char rune, buffer []rune, volume *types.ServiceVolu
 	case len(buffer) == 0:
 		return errors.New("empty section between colons")
 	// Anonymous volume
-	case volume.Source == "" && char == rune(0):
+	case volume.Source == "" && char == endOfSpec:
 		volume.Target = strBuffer
 		return nil
 	case volume.Source == "":
@@ -77,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
@@ -112,7 +112,6 @@ func isFilePath(source string) bool {
 		return true
 	}
 
-	// Windows absolute path
+	first, nextIndex := utf8.DecodeRuneInString(source)
-	first, next := utf8.DecodeRuneInString(source)
+	return isWindowsDrive([]rune{first}, rune(source[nextIndex]))
-	return unicode.IsLetter(first) && source[next] == ':'
 }

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")
+	_, err := ParseVolume("name:/target:bogus")
-	assert.EqualError(t, err, "invalid spec: name:/target:bogus: unknown option: 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,
@@ -147,3 +148,55 @@ func TestParseVolumeWithRW(t *testing.T) {
 		assert.Equal(t, expected, volume)
 	}
 }
+
+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)
+	}
+}