Skip to content

Commit 732261f

Browse files
dnephindnephin
committed
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 <[email protected]>
1 parent db6ff35 commit 732261f

5 files changed

Lines changed: 92 additions & 156 deletions

File tree

‎cli/command/container/opts.go‎

Lines changed: 3 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"time"
1313

1414
"github.com/Sirupsen/logrus"
15+
"github.com/docker/cli/cli/compose/loader"
1516
"github.com/docker/cli/opts"
1617
"github.com/docker/docker/api/types/container"
1718
networktypes "github.com/docker/docker/api/types/network"
@@ -332,7 +333,8 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*containerConfig, err
332333
volumes := copts.volumes.GetMap()
333334
// add any bind targets to the list of container volumes
334335
for bind := range copts.volumes.GetMap() {
335-
if arr := volumeSplitN(bind, 2); len(arr) > 1 {
336+
parsed, _ := loader.ParseVolume(bind)
337+
if parsed.Source != "" {
336338
// after creating the bind mount we want to delete it from the copts.volumes values because
337339
// we do not want bind mounts being committed to image configs
338340
binds = append(binds, bind)
@@ -827,67 +829,6 @@ func validatePath(val string, validator func(string) bool) (string, error) {
827829
return val, nil
828830
}
829831

830-
// volumeSplitN splits raw into a maximum of n parts, separated by a separator colon.
831-
// A separator colon is the last `:` character in the regex `[:\\]?[a-zA-Z]:` (note `\\` is `\` escaped).
832-
// In Windows driver letter appears in two situations:
833-
// a. `^[a-zA-Z]:` (A colon followed by `^[a-zA-Z]:` is OK as colon is the separator in volume option)
834-
// b. A string in the format like `\\?\C:\Windows\...` (UNC).
835-
// Therefore, a driver letter can only follow either a `:` or `\\`
836-
// This allows to correctly split strings such as `C:\foo:D:\:rw` or `/tmp/q:/foo`.
837-
func volumeSplitN(raw string, n int) []string {
838-
var array []string
839-
if len(raw) == 0 || raw[0] == ':' {
840-
// invalid
841-
return nil
842-
}
843-
// numberOfParts counts the number of parts separated by a separator colon
844-
numberOfParts := 0
845-
// left represents the left-most cursor in raw, updated at every `:` character considered as a separator.
846-
left := 0
847-
// right represents the right-most cursor in raw incremented with the loop. Note this
848-
// starts at index 1 as index 0 is already handle above as a special case.
849-
for right := 1; right < len(raw); right++ {
850-
// stop parsing if reached maximum number of parts
851-
if n >= 0 && numberOfParts >= n {
852-
break
853-
}
854-
if raw[right] != ':' {
855-
continue
856-
}
857-
potentialDriveLetter := raw[right-1]
858-
if (potentialDriveLetter >= 'A' && potentialDriveLetter <= 'Z') || (potentialDriveLetter >= 'a' && potentialDriveLetter <= 'z') {
859-
if right > 1 {
860-
beforePotentialDriveLetter := raw[right-2]
861-
// Only `:` or `\\` are checked (`/` could fall into the case of `/tmp/q:/foo`)
862-
if beforePotentialDriveLetter != ':' && beforePotentialDriveLetter != '\\' {
863-
// e.g. `C:` is not preceded by any delimiter, therefore it was not a drive letter but a path ending with `C:`.
864-
array = append(array, raw[left:right])
865-
left = right + 1
866-
numberOfParts++
867-
}
868-
// else, `C:` is considered as a drive letter and not as a delimiter, so we continue parsing.
869-
}
870-
// if right == 1, then `C:` is the beginning of the raw string, therefore `:` is again not considered a delimiter and we continue parsing.
871-
} else {
872-
// if `:` is not preceded by a potential drive letter, then consider it as a delimiter.
873-
array = append(array, raw[left:right])
874-
left = right + 1
875-
numberOfParts++
876-
}
877-
}
878-
// need to take care of the last part
879-
if left < len(raw) {
880-
if n >= 0 && numberOfParts >= n {
881-
// if the maximum number of parts is reached, just append the rest to the last part
882-
// left-1 is at the last `:` that needs to be included since not considered a separator.
883-
array[n-1] += raw[left-1:]
884-
} else {
885-
array = append(array, raw[left:])
886-
}
887-
}
888-
return array
889-
}
890-
891832
// validateAttach validates that the specified string is a valid attach option.
892833
func validateAttach(val string) (string, error) {
893834
s := strings.ToLower(val)

‎cli/command/container/opts_test.go‎

Lines changed: 20 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/pkg/errors"
2020
"github.com/spf13/pflag"
2121
"github.com/stretchr/testify/assert"
22+
"github.com/stretchr/testify/require"
2223
)
2324

2425
func TestValidateAttach(t *testing.T) {
@@ -366,51 +367,46 @@ func TestParseDevice(t *testing.T) {
366367

367368
func TestParseModes(t *testing.T) {
368369
// ipc ko
369-
if _, _, _, err := parseRun([]string{"--ipc=container:", "img", "cmd"}); err == nil || err.Error() != "--ipc: invalid IPC mode" {
370-
t.Fatalf("Expected an error with message '--ipc: invalid IPC mode', got %v", err)
371-
}
370+
_, _, _, err := parseRun([]string{"--ipc=container:", "img", "cmd"})
371+
testutil.ErrorContains(t, err, "--ipc: invalid IPC mode")
372+
372373
// ipc ok
373374
_, hostconfig, _, err := parseRun([]string{"--ipc=host", "img", "cmd"})
374-
if err != nil {
375-
t.Fatal(err)
376-
}
375+
require.NoError(t, err)
377376
if !hostconfig.IpcMode.Valid() {
378377
t.Fatalf("Expected a valid IpcMode, got %v", hostconfig.IpcMode)
379378
}
379+
380380
// pid ko
381-
if _, _, _, err := parseRun([]string{"--pid=container:", "img", "cmd"}); err == nil || err.Error() != "--pid: invalid PID mode" {
382-
t.Fatalf("Expected an error with message '--pid: invalid PID mode', got %v", err)
383-
}
381+
_, _, _, err = parseRun([]string{"--pid=container:", "img", "cmd"})
382+
testutil.ErrorContains(t, err, "--pid: invalid PID mode")
383+
384384
// pid ok
385385
_, hostconfig, _, err = parseRun([]string{"--pid=host", "img", "cmd"})
386-
if err != nil {
387-
t.Fatal(err)
388-
}
386+
require.NoError(t, err)
389387
if !hostconfig.PidMode.Valid() {
390388
t.Fatalf("Expected a valid PidMode, got %v", hostconfig.PidMode)
391389
}
390+
392391
// uts ko
393-
if _, _, _, err := parseRun([]string{"--uts=container:", "img", "cmd"}); err == nil || err.Error() != "--uts: invalid UTS mode" {
394-
t.Fatalf("Expected an error with message '--uts: invalid UTS mode', got %v", err)
395-
}
392+
_, _, _, err = parseRun([]string{"--uts=container:", "img", "cmd"})
393+
testutil.ErrorContains(t, err, "--uts: invalid UTS mode")
394+
396395
// uts ok
397396
_, hostconfig, _, err = parseRun([]string{"--uts=host", "img", "cmd"})
398-
if err != nil {
399-
t.Fatal(err)
400-
}
397+
require.NoError(t, err)
401398
if !hostconfig.UTSMode.Valid() {
402399
t.Fatalf("Expected a valid UTSMode, got %v", hostconfig.UTSMode)
403400
}
401+
404402
// shm-size ko
405403
expectedErr := `invalid argument "a128m" for --shm-size=a128m: invalid size: 'a128m'`
406-
if _, _, _, err = parseRun([]string{"--shm-size=a128m", "img", "cmd"}); err == nil || err.Error() != expectedErr {
407-
t.Fatalf("Expected an error with message '%v', got %v", expectedErr, err)
408-
}
404+
_, _, _, err = parseRun([]string{"--shm-size=a128m", "img", "cmd"})
405+
testutil.ErrorContains(t, err, expectedErr)
406+
409407
// shm-size ok
410408
_, hostconfig, _, err = parseRun([]string{"--shm-size=128m", "img", "cmd"})
411-
if err != nil {
412-
t.Fatal(err)
413-
}
409+
require.NoError(t, err)
414410
if hostconfig.ShmSize != 134217728 {
415411
t.Fatalf("Expected a valid ShmSize, got %d", hostconfig.ShmSize)
416412
}
@@ -754,58 +750,6 @@ func callDecodeContainerConfig(volumes []string, binds []string) (*container.Con
754750
return c, h, err
755751
}
756752

757-
func TestVolumeSplitN(t *testing.T) {
758-
for _, x := range []struct {
759-
input string
760-
n int
761-
expected []string
762-
}{
763-
{`C:\foo:d:`, -1, []string{`C:\foo`, `d:`}},
764-
{`:C:\foo:d:`, -1, nil},
765-
{`/foo:/bar:ro`, 3, []string{`/foo`, `/bar`, `ro`}},
766-
{`/foo:/bar:ro`, 2, []string{`/foo`, `/bar:ro`}},
767-
{`C:\foo\:/foo`, -1, []string{`C:\foo\`, `/foo`}},
768-
769-
{`d:\`, -1, []string{`d:\`}},
770-
{`d:`, -1, []string{`d:`}},
771-
{`d:\path`, -1, []string{`d:\path`}},
772-
{`d:\path with space`, -1, []string{`d:\path with space`}},
773-
{`d:\pathandmode:rw`, -1, []string{`d:\pathandmode`, `rw`}},
774-
{`c:\:d:\`, -1, []string{`c:\`, `d:\`}},
775-
{`c:\windows\:d:`, -1, []string{`c:\windows\`, `d:`}},
776-
{`c:\windows:d:\s p a c e`, -1, []string{`c:\windows`, `d:\s p a c e`}},
777-
{`c:\windows:d:\s p a c e:RW`, -1, []string{`c:\windows`, `d:\s p a c e`, `RW`}},
778-
{`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`}},
779-
{`0123456789name:d:`, -1, []string{`0123456789name`, `d:`}},
780-
{`MiXeDcAsEnAmE:d:`, -1, []string{`MiXeDcAsEnAmE`, `d:`}},
781-
{`name:D:`, -1, []string{`name`, `D:`}},
782-
{`name:D::rW`, -1, []string{`name`, `D:`, `rW`}},
783-
{`name:D::RW`, -1, []string{`name`, `D:`, `RW`}},
784-
{`c:/:d:/forward/slashes/are/good/too`, -1, []string{`c:/`, `d:/forward/slashes/are/good/too`}},
785-
{`c:\Windows`, -1, []string{`c:\Windows`}},
786-
{`c:\Program Files (x86)`, -1, []string{`c:\Program Files (x86)`}},
787-
788-
{``, -1, nil},
789-
{`.`, -1, []string{`.`}},
790-
{`..\`, -1, []string{`..\`}},
791-
{`c:\:..\`, -1, []string{`c:\`, `..\`}},
792-
{`c:\:d:\:xyzzy`, -1, []string{`c:\`, `d:\`, `xyzzy`}},
793-
794-
// Cover directories with one-character name
795-
{`/tmp/x/y:/foo/x/y`, -1, []string{`/tmp/x/y`, `/foo/x/y`}},
796-
} {
797-
res := volumeSplitN(x.input, x.n)
798-
if len(res) < len(x.expected) {
799-
t.Fatalf("input: %v, expected: %v, got: %v", x.input, x.expected, res)
800-
}
801-
for i, e := range res {
802-
if e != x.expected[i] {
803-
t.Fatalf("input: %v, expected: %v, got: %v", x.input, x.expected, res)
804-
}
805-
}
806-
}
807-
}
808-
809753
func TestValidateDevice(t *testing.T) {
810754
valid := []string{
811755
"/home",

‎cli/compose/loader/loader.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -564,7 +564,7 @@ func transformStringSourceMap(data interface{}) (interface{}, error) {
564564
func transformServiceVolumeConfig(data interface{}) (interface{}, error) {
565565
switch value := data.(type) {
566566
case string:
567-
return parseVolume(value)
567+
return ParseVolume(value)
568568
case map[string]interface{}:
569569
return data, nil
570570
default:

‎cli/compose/loader/volume.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ import (
1212

1313
const endOfSpec = rune(0)
1414

15-
func parseVolume(spec string) (types.ServiceVolumeConfig, error) {
15+
// ParseVolume parses a volume spec without any knowledge of the target platform
16+
func ParseVolume(spec string) (types.ServiceVolumeConfig, error) {
1617
volume := types.ServiceVolumeConfig{}
1718

1819
switch len(spec) {
@@ -31,13 +32,15 @@ func parseVolume(spec string) (types.ServiceVolumeConfig, error) {
3132
buffer = append(buffer, char)
3233
case char == ':' || char == endOfSpec:
3334
if err := populateFieldFromBuffer(char, buffer, &volume); err != nil {
35+
populateType(&volume)
3436
return volume, errors.Wrapf(err, "invalid spec: %s", spec)
3537
}
3638
buffer = []rune{}
3739
default:
3840
buffer = append(buffer, char)
3941
}
4042
}
43+
4144
populateType(&volume)
4245
return volume, nil
4346
}
@@ -75,9 +78,8 @@ func populateFieldFromBuffer(char rune, buffer []rune, volume *types.ServiceVolu
7578
default:
7679
if isBindOption(option) {
7780
volume.Bind = &types.ServiceVolumeBind{Propagation: option}
78-
} else {
79-
return errors.Errorf("unknown option: %s", option)
8081
}
82+
// ignore unknown options
8183
}
8284
}
8385
return nil

0 commit comments

Comments
 (0)