From d5b271c155926057d33c2128ff57e49274455813 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Mon, 9 Apr 2018 10:09:30 +0200 Subject: [PATCH 1/4] add check for local volume option Description: When using local volume option such as size=10G, type=tmpfs, if we provide wrong options, we could create volume successfully. But when we are ready to use it, it will fail to start container by failing to mount the local volume(invalid option). We should check the options at when we create it. Signed-off-by: Wentao Zhang Signed-off-by: Vincent Demeester Signed-off-by: Sebastiaan van Stijn --- integration-cli/docker_api_containers_test.go | 65 ++++++++++++++++--- volume/local/local.go | 8 +++ volume/local/local_unix.go | 4 ++ volume/local/local_windows.go | 5 +- 4 files changed, 72 insertions(+), 10 deletions(-) diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index ef67894106..d013012c9f 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -1835,14 +1835,62 @@ func (s *DockerSuite) TestContainersAPICreateMountsValidation(c *check.C) { Image: "busybox", }, hostConfig: containertypes.HostConfig{ - Mounts: []mounttypes.Mount{{ - Type: "volume", - Source: "hello3", - Target: destPath, - VolumeOptions: &mounttypes.VolumeOptions{ - DriverConfig: &mounttypes.Driver{ - Name: "local", - Options: map[string]string{"o": "size=1"}}}}}}, + Mounts: []mounttypes.Mount{ + { + Type: "volume", + Source: "missing-device-opt", + Target: destPath, + VolumeOptions: &mounttypes.VolumeOptions{ + DriverConfig: &mounttypes.Driver{ + Name: "local", + Options: map[string]string{"type": "tmpfs"}, + }, + }, + }, + }, + }, + msg: `missing required option: "device"`, + }, + { + config: containertypes.Config{ + Image: "busybox", + }, + hostConfig: containertypes.HostConfig{ + Mounts: []mounttypes.Mount{ + { + Type: "volume", + Source: "missing-type-opt", + Target: destPath, + VolumeOptions: &mounttypes.VolumeOptions{ + DriverConfig: &mounttypes.Driver{ + Name: "local", + Options: map[string]string{"device": "tmpfs"}, + }, + }, + }, + }, + }, + msg: `missing required option: "type"`, + }, + { + config: containertypes.Config{ + Image: "busybox", + }, + hostConfig: containertypes.HostConfig{ + Mounts: []mounttypes.Mount{ + { + Type: "volume", + Source: "hello4", + Target: destPath, + VolumeOptions: &mounttypes.VolumeOptions{ + DriverConfig: &mounttypes.Driver{ + Name: "local", + Options: map[string]string{"o": "size=1", "type": "tmpfs", "device": "tmpfs"}, + }, + }, + }, + }, + }, msg: "", }, { @@ -1869,7 +1917,6 @@ func (s *DockerSuite) TestContainersAPICreateMountsValidation(c *check.C) { }}}}, msg: "", }, - { config: containertypes.Config{ Image: "busybox", diff --git a/volume/local/local.go b/volume/local/local.go index d3119cb2ff..1c844cf0e3 100644 --- a/volume/local/local.go +++ b/volume/local/local.go @@ -353,11 +353,19 @@ func (v *localVolume) unmount() error { } func validateOpts(opts map[string]string) error { + if len(opts) == 0 { + return nil + } for opt := range opts { if !validOpts[opt] { return validationError(fmt.Sprintf("invalid option key: %q", opt)) } } + for opt := range mandatoryOpts { + if _, ok := opts[opt]; !ok { + return errdefs.InvalidParameter(errors.Errorf("missing required option: %q", opt)) + } + } return nil } diff --git a/volume/local/local_unix.go b/volume/local/local_unix.go index 5ee2ed894b..1d25c330da 100644 --- a/volume/local/local_unix.go +++ b/volume/local/local_unix.go @@ -27,6 +27,10 @@ var ( "o": true, // generic mount options "device": true, // device to mount from } + mandatoryOpts = map[string]struct{}{ + "device": {}, + "type": {}, + } ) type optsConfig struct { diff --git a/volume/local/local_windows.go b/volume/local/local_windows.go index d96fc0f594..5748681bb0 100644 --- a/volume/local/local_windows.go +++ b/volume/local/local_windows.go @@ -14,7 +14,10 @@ import ( type optsConfig struct{} -var validOpts map[string]bool +var ( + validOpts map[string]bool + mandatoryOpts map[string]struct{} +) // scopedPath verifies that the path where the volume is located // is under Docker's root and the valid local paths. From 342f7a357ad00164c5f8130127ab54a1d823b1de Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 22 Dec 2018 02:23:17 +0100 Subject: [PATCH 2/4] Use a map[string]struct{} for validOpts For consistency with `mandatoryOpts`, and because it is a tiny-tiny bit more efficient. Signed-off-by: Sebastiaan van Stijn --- volume/local/local.go | 2 +- volume/local/local_unix.go | 8 ++++---- volume/local/local_windows.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/volume/local/local.go b/volume/local/local.go index 1c844cf0e3..6bf2aa9015 100644 --- a/volume/local/local.go +++ b/volume/local/local.go @@ -357,7 +357,7 @@ func validateOpts(opts map[string]string) error { return nil } for opt := range opts { - if !validOpts[opt] { + if _, ok := validOpts[opt]; !ok { return validationError(fmt.Sprintf("invalid option key: %q", opt)) } } diff --git a/volume/local/local_unix.go b/volume/local/local_unix.go index 1d25c330da..e8f12c9030 100644 --- a/volume/local/local_unix.go +++ b/volume/local/local_unix.go @@ -22,10 +22,10 @@ import ( var ( oldVfsDir = filepath.Join("vfs", "dir") - validOpts = map[string]bool{ - "type": true, // specify the filesystem type for mount, e.g. nfs - "o": true, // generic mount options - "device": true, // device to mount from + validOpts = map[string]struct{}{ + "type": {}, // specify the filesystem type for mount, e.g. nfs + "o": {}, // generic mount options + "device": {}, // device to mount from } mandatoryOpts = map[string]struct{}{ "device": {}, diff --git a/volume/local/local_windows.go b/volume/local/local_windows.go index 5748681bb0..eb19e79747 100644 --- a/volume/local/local_windows.go +++ b/volume/local/local_windows.go @@ -15,7 +15,7 @@ import ( type optsConfig struct{} var ( - validOpts map[string]bool + validOpts map[string]struct{} mandatoryOpts map[string]struct{} ) From 11b88be2471ed460c3b957463a40e8812f90d795 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 22 Dec 2018 02:28:30 +0100 Subject: [PATCH 3/4] Remove validationError type, and use errdefs.InvalidParameter Using `errors.Errorf()` passes the error with the stack trace for debugging purposes. Also using `errdefs.InvalidParameter` for Windows, so that the API will return a 4xx status, instead of a 5xx, and added tests for both validations. Signed-off-by: Sebastiaan van Stijn --- integration-cli/docker_api_containers_test.go | 48 +++++++++++++++++++ volume/local/local.go | 14 ++---- volume/local/local_unix.go | 3 +- volume/local/local_windows.go | 6 ++- 4 files changed, 56 insertions(+), 15 deletions(-) diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index d013012c9f..33559fcfb9 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -1828,8 +1828,55 @@ func (s *DockerSuite) TestContainersAPICreateMountsValidation(c *check.C) { }...) } + if DaemonIsWindows() { + cases = append(cases, []testCase{ + { + config: containertypes.Config{ + Image: "busybox", + }, + hostConfig: containertypes.HostConfig{ + Mounts: []mounttypes.Mount{ + { + Type: "volume", + Source: "not-supported-on-windows", + Target: destPath, + VolumeOptions: &mounttypes.VolumeOptions{ + DriverConfig: &mounttypes.Driver{ + Name: "local", + Options: map[string]string{"type": "tmpfs"}, + }, + }, + }, + }, + }, + msg: `options are not supported on this platform`, + }, + }...) + } + if DaemonIsLinux() { cases = append(cases, []testCase{ + { + config: containertypes.Config{ + Image: "busybox", + }, + hostConfig: containertypes.HostConfig{ + Mounts: []mounttypes.Mount{ + { + Type: "volume", + Source: "missing-device-opt", + Target: destPath, + VolumeOptions: &mounttypes.VolumeOptions{ + DriverConfig: &mounttypes.Driver{ + Name: "local", + Options: map[string]string{"foobar": "foobaz"}, + }, + }, + }, + }, + }, + msg: `invalid option: "foobar"`, + }, { config: containertypes.Config{ Image: "busybox", @@ -1935,6 +1982,7 @@ func (s *DockerSuite) TestContainersAPICreateMountsValidation(c *check.C) { c.Assert(err, checker.IsNil) defer cli.Close() + // TODO add checks for statuscode returned by API for i, x := range cases { c.Logf("case %d", i) _, err = cli.ContainerCreate(context.Background(), &x.config, &x.hostConfig, &networktypes.NetworkingConfig{}, "") diff --git a/volume/local/local.go b/volume/local/local.go index 6bf2aa9015..7981f5836a 100644 --- a/volume/local/local.go +++ b/volume/local/local.go @@ -248,20 +248,12 @@ func (r *Root) Scope() string { return volume.LocalScope } -type validationError string - -func (e validationError) Error() string { - return string(e) -} - -func (e validationError) InvalidParameter() {} - func (r *Root) validateName(name string) error { if len(name) == 1 { - return validationError("volume name is too short, names should be at least two alphanumeric characters") + return errdefs.InvalidParameter(errors.New("volume name is too short, names should be at least two alphanumeric characters")) } if !volumeNameRegex.MatchString(name) { - return validationError(fmt.Sprintf("%q includes invalid characters for a local volume name, only %q are allowed. If you intended to pass a host directory, use absolute path", name, names.RestrictedNameChars)) + return errdefs.InvalidParameter(errors.Errorf("%q includes invalid characters for a local volume name, only %q are allowed. If you intended to pass a host directory, use absolute path", name, names.RestrictedNameChars)) } return nil } @@ -358,7 +350,7 @@ func validateOpts(opts map[string]string) error { } for opt := range opts { if _, ok := validOpts[opt]; !ok { - return validationError(fmt.Sprintf("invalid option key: %q", opt)) + return errdefs.InvalidParameter(errors.Errorf("invalid option: %q", opt)) } } for opt := range mandatoryOpts { diff --git a/volume/local/local_unix.go b/volume/local/local_unix.go index e8f12c9030..62dfdc1748 100644 --- a/volume/local/local_unix.go +++ b/volume/local/local_unix.go @@ -14,9 +14,8 @@ import ( "syscall" "time" - "github.com/pkg/errors" - "github.com/docker/docker/pkg/mount" + "github.com/pkg/errors" ) var ( diff --git a/volume/local/local_windows.go b/volume/local/local_windows.go index eb19e79747..cb8b632cc4 100644 --- a/volume/local/local_windows.go +++ b/volume/local/local_windows.go @@ -4,12 +4,14 @@ package local // import "github.com/docker/docker/volume/local" import ( - "fmt" "os" "path/filepath" "strings" "syscall" "time" + + "github.com/docker/docker/errdefs" + "github.com/pkg/errors" ) type optsConfig struct{} @@ -30,7 +32,7 @@ func (r *Root) scopedPath(realPath string) bool { func setOpts(v *localVolume, opts map[string]string) error { if len(opts) > 0 { - return fmt.Errorf("options are not supported on this platform") + return errdefs.InvalidParameter(errors.New("options are not supported on this platform")) } return nil } From 0d6dd91e1398350cfd02e95186a3a3f7e9d8d2ef Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 22 Dec 2018 03:02:28 +0100 Subject: [PATCH 4/4] Move `validateOpts()` to local_unix.go as it is not used on Windows Signed-off-by: Sebastiaan van Stijn --- volume/local/local.go | 17 ----------------- volume/local/local_unix.go | 18 ++++++++++++++++++ volume/local/local_windows.go | 5 ----- 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/volume/local/local.go b/volume/local/local.go index 7981f5836a..6dc894873d 100644 --- a/volume/local/local.go +++ b/volume/local/local.go @@ -344,23 +344,6 @@ func (v *localVolume) unmount() error { return nil } -func validateOpts(opts map[string]string) error { - if len(opts) == 0 { - return nil - } - for opt := range opts { - if _, ok := validOpts[opt]; !ok { - return errdefs.InvalidParameter(errors.Errorf("invalid option: %q", opt)) - } - } - for opt := range mandatoryOpts { - if _, ok := opts[opt]; !ok { - return errdefs.InvalidParameter(errors.Errorf("missing required option: %q", opt)) - } - } - return nil -} - func (v *localVolume) Status() map[string]interface{} { return nil } diff --git a/volume/local/local_unix.go b/volume/local/local_unix.go index 62dfdc1748..0b1771091f 100644 --- a/volume/local/local_unix.go +++ b/volume/local/local_unix.go @@ -14,6 +14,7 @@ import ( "syscall" "time" + "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/mount" "github.com/pkg/errors" ) @@ -74,6 +75,23 @@ func setOpts(v *localVolume, opts map[string]string) error { return nil } +func validateOpts(opts map[string]string) error { + if len(opts) == 0 { + return nil + } + for opt := range opts { + if _, ok := validOpts[opt]; !ok { + return errdefs.InvalidParameter(errors.Errorf("invalid option: %q", opt)) + } + } + for opt := range mandatoryOpts { + if _, ok := opts[opt]; !ok { + return errdefs.InvalidParameter(errors.Errorf("missing required option: %q", opt)) + } + } + return nil +} + func (v *localVolume) mount() error { if v.opts.MountDevice == "" { return fmt.Errorf("missing device in volume options") diff --git a/volume/local/local_windows.go b/volume/local/local_windows.go index cb8b632cc4..d9bc8f5298 100644 --- a/volume/local/local_windows.go +++ b/volume/local/local_windows.go @@ -16,11 +16,6 @@ import ( type optsConfig struct{} -var ( - validOpts map[string]struct{} - mandatoryOpts map[string]struct{} -) - // scopedPath verifies that the path where the volume is located // is under Docker's root and the valid local paths. func (r *Root) scopedPath(realPath string) bool {