Skip to content

Improve volume spec parsing #152

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
dnephin merged 2 commits into from
Jun 21, 2017
Merged

Improve volume spec parsing #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db6ff35
Some improvements to compose volume spec parsing
dnephin Jun 4, 2017
732261f
Use compose volume spec parser for container volume flag
dnephin May 9, 2017
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
65 changes: 3 additions & 62 deletions cli/command/container/opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
96 changes: 20 additions & 76 deletions cli/command/container/opts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion cli/compose/loader/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
27 changes: 13 additions & 14 deletions cli/compose/loader/volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -23,12 +26,13 @@ func parseVolume(spec string) (types.ServiceVolumeConfig, error) {
}

buffer := []rune{}
for _, char := range spec {
for _, char := range spec + string(endOfSpec) {
Copy link

@aaronlehmann aaronlehmann Jun 7, 2017

Choose a reason for hiding this comment

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

Interesting. I wonder if it would make sense to iterate over bytes instead of runes, so that we could just check if the iteration count is equal to the length of the sequence minus one. I don't think Windows drive letters can be non-ASCII, but I could be wrong about this.

Copy link
Contributor Author

@dnephin dnephin Jun 7, 2017

Choose a reason for hiding this comment

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

That's kind of what the old code did. Instead of using this token it would just repeat this case outside of the for loop. We still need a token to pass to populateFieldFromBuffer() to identify "end of string", so I realized it was actually easier to just append the token to the string and add it to the case.

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{}
Expand All @@ -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])
}

Expand All @@ -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 == "":
Expand All @@ -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
Expand Down Expand Up @@ -112,7 +112,6 @@ func isFilePath(source string) bool {
return true
}

// Windows absolute path
first, next := utf8.DecodeRuneInString(source)
return unicode.IsLetter(first) && source[next] == ':'
first, nextIndex := utf8.DecodeRuneInString(source)
return isWindowsDrive([]rune{first}, rune(source[nextIndex]))
}
Loading