From f3d08d59aa402444a311701dc231f4a1e83440df Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 2 Jul 2021 13:25:27 +0200 Subject: [PATCH 1/7] volume/mounts: move some code to correct location, and minor linting/formatting - Remove the windowsparser.HasResource() override, as it was the same on both Windows and Linux - Move the rxLCOWDestination to the lcowParser code - Move the rwModes variable to a generic (non-platform-specific) file, as it's used both for the windowsParser and the linuxParser - Some minor formatting and linting changes Signed-off-by: Sebastiaan van Stijn --- volume/mounts/lcow_parser.go | 8 ++++++++ volume/mounts/linux_parser.go | 9 +-------- volume/mounts/parser.go | 6 ++++++ volume/mounts/parser_test.go | 10 +++------- volume/mounts/volume_unix.go | 4 ---- volume/mounts/volume_windows.go | 3 --- volume/mounts/windows_parser.go | 17 ++++++++--------- 7 files changed, 26 insertions(+), 31 deletions(-) diff --git a/volume/mounts/lcow_parser.go b/volume/mounts/lcow_parser.go index bafb7b07f8..645a731d93 100644 --- a/volume/mounts/lcow_parser.go +++ b/volume/mounts/lcow_parser.go @@ -7,6 +7,14 @@ import ( "github.com/docker/docker/api/types/mount" ) +// rxLCOWDestination is the regex expression for the mount destination for LCOW +// +// Destination (aka container path): +// - Variation on hostdir but can be a drive followed by colon as well +// - If a path, must be absolute. Can include spaces +// - Drive cannot be c: (explicitly checked in code, not RegEx) +const rxLCOWDestination = `(?P/(?:[^\\/:*?"<>\r\n]+[/]?)*)` + var lcowSpecificValidators mountValidator = func(m *mount.Mount) error { if path.Clean(m.Target) == "/" { return ErrVolumeTargetIsRoot diff --git a/volume/mounts/linux_parser.go b/volume/mounts/linux_parser.go index b5ed4af6a3..8dc9afd34c 100644 --- a/volume/mounts/linux_parser.go +++ b/volume/mounts/linux_parser.go @@ -12,8 +12,7 @@ import ( "github.com/docker/docker/volume" ) -type linuxParser struct { -} +type linuxParser struct{} func linuxSplitRawSpec(raw string) ([]string, error) { if strings.Count(raw, ":") > 2 { @@ -115,12 +114,6 @@ func (p *linuxParser) validateMountConfigImpl(mnt *mount.Mount, validateBindSour return nil } -// read-write modes -var rwModes = map[string]bool{ - "rw": true, - "ro": true, -} - // label modes var linuxLabelModes = map[string]bool{ "Z": true, diff --git a/volume/mounts/parser.go b/volume/mounts/parser.go index 73681750ea..976d276a82 100644 --- a/volume/mounts/parser.go +++ b/volume/mounts/parser.go @@ -18,6 +18,12 @@ const ( // It's used by both LCOW and Linux parsers. var ErrVolumeTargetIsRoot = errors.New("invalid specification: destination can't be '/'") +// read-write modes +var rwModes = map[string]bool{ + "rw": true, + "ro": true, +} + // Parser represents a platform specific parser for mount expressions type Parser interface { ParseMountRaw(raw, volumeDriver string) (*MountPoint, error) diff --git a/volume/mounts/parser_test.go b/volume/mounts/parser_test.go index c879c3b152..ef4919b944 100644 --- a/volume/mounts/parser_test.go +++ b/volume/mounts/parser_test.go @@ -10,7 +10,6 @@ import ( "github.com/docker/docker/api/types/mount" "gotest.tools/v3/assert" - "gotest.tools/v3/assert/cmp" ) type parseMountRawTestSet struct { @@ -300,9 +299,7 @@ func TestParseMountRaw(t *testing.T) { winParser := &windowsParser{} lcowParser := &lcowParser{} tester := func(parser Parser, set parseMountRawTestSet) { - for _, path := range set.valid { - if _, err := parser.ParseMountRaw(path, "local"); err != nil { t.Errorf("ParseMountRaw(`%q`) should succeed: error %q", path, err) } @@ -318,10 +315,10 @@ func TestParseMountRaw(t *testing.T) { } } } + tester(linParser, linuxSet) tester(winParser, windowsSet) tester(lcowParser, lcowSet) - } // testParseMountRaw is a structure used by TestParseMountRawSplit for @@ -397,7 +394,7 @@ func TestParseMountRawSplit(t *testing.T) { } if m == nil || err != nil { - t.Errorf("ParseMountRaw failed for spec '%s', driver '%s', error '%v'", c.bind, c.driver, err.Error()) + t.Errorf("ParseMountRaw failed for spec '%s', driver '%s', error '%v'", c.bind, c.driver, err) continue } @@ -525,6 +522,5 @@ func TestParseMountSpecBindWithFileinfoError(t *testing.T) { parser := NewParser(runtime.GOOS) _, err := parser.ParseMountSpec(m) - assert.Assert(t, err != nil) - assert.Assert(t, cmp.Contains(err.Error(), "some crazy error")) + assert.ErrorContains(t, err, "some crazy error") } diff --git a/volume/mounts/volume_unix.go b/volume/mounts/volume_unix.go index c6d51e0710..9f74c9e7c1 100644 --- a/volume/mounts/volume_unix.go +++ b/volume/mounts/volume_unix.go @@ -12,7 +12,3 @@ func (p *linuxParser) HasResource(m *MountPoint, absolutePath string) bool { relPath, err := filepath.Rel(m.Destination, absolutePath) return err == nil && relPath != ".." && !strings.HasPrefix(relPath, fmt.Sprintf("..%c", filepath.Separator)) } - -func (p *windowsParser) HasResource(m *MountPoint, absolutePath string) bool { - return false -} diff --git a/volume/mounts/volume_windows.go b/volume/mounts/volume_windows.go index 773e7db88a..d91f6cee3a 100644 --- a/volume/mounts/volume_windows.go +++ b/volume/mounts/volume_windows.go @@ -1,8 +1,5 @@ package mounts // import "github.com/docker/docker/volume/mounts" -func (p *windowsParser) HasResource(m *MountPoint, absolutePath string) bool { - return false -} func (p *linuxParser) HasResource(m *MountPoint, absolutePath string) bool { return false } diff --git a/volume/mounts/windows_parser.go b/volume/mounts/windows_parser.go index 8f427d8c50..e0f8f5a7d1 100644 --- a/volume/mounts/windows_parser.go +++ b/volume/mounts/windows_parser.go @@ -12,8 +12,7 @@ import ( "github.com/docker/docker/pkg/stringid" ) -type windowsParser struct { -} +type windowsParser struct{} const ( // Spec should be in the format [source:]destination[:mode] @@ -63,12 +62,6 @@ const ( // rxDestination is the regex expression for the mount destination rxDestination = `(?P((?:\\\\\?\\)?([a-z]):((?:[\\/][^\\/:*?"<>\r\n]+)*[\\/]?))|(` + rxPipe + `))` - rxLCOWDestination = `(?P/(?:[^\\/:*?"<>\r\n]+[/]?)*)` - // Destination (aka container path): - // - Variation on hostdir but can be a drive followed by colon as well - // - If a path, must be absolute. Can include spaces - // - Drive cannot be c: (explicitly checked in code, not RegEx) - // rxMode is the regex expression for the mode of the mount // Mode (optional): // - Hopefully self explanatory in comparison to above regex's. @@ -136,8 +129,10 @@ func windowsValidMountMode(mode string) bool { if mode == "" { return true } + // TODO should windows mounts produce an error if any mode was provided (they're a no-op on windows) return rwModes[strings.ToLower(mode)] } + func windowsValidateNotRoot(p string) error { p = strings.ToLower(strings.Replace(p, `/`, `\`, -1)) if p == "c:" || p == `c:\` { @@ -177,7 +172,7 @@ func (p *windowsParser) ReadWrite(mode string) bool { return strings.ToLower(mode) != "ro" } -// IsVolumeNameValid checks a volume name in a platform specific manner. +// ValidateVolumeName checks a volume name in a platform specific manner. func (p *windowsParser) ValidateVolumeName(name string) error { nameExp := regexp.MustCompile(`^` + rxName + `$`) if !nameExp.MatchString(name) { @@ -454,3 +449,7 @@ func (p *windowsParser) IsBackwardCompatible(m *MountPoint) bool { func (p *windowsParser) ValidateTmpfsMountDestination(dest string) error { return errors.New("Platform does not support tmpfs") } + +func (p *windowsParser) HasResource(m *MountPoint, absolutePath string) bool { + return false +} From 300c11c7c9143ca077890050deb83917e67fe250 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 11 Jun 2021 21:01:18 +0200 Subject: [PATCH 2/7] volume/mounts: remove "containerOS" argument from NewParser (LCOW code) This changes mounts.NewParser() to create a parser for the current operatingsystem, instead of one specific to a (possibly non-matching, in case of LCOW) OS. With the OS-specific handling being removed, the "OS" parameter is also removed from `daemon.verifyContainerSettings()`, and various other container-related functions. Signed-off-by: Sebastiaan van Stijn --- container/container.go | 6 +---- container/container_unix.go | 6 ++--- daemon/archive_unix.go | 2 +- daemon/container.go | 34 ++++++++---------------- daemon/container_unix_test.go | 2 +- daemon/create.go | 47 +++++++++++----------------------- daemon/create_windows.go | 2 +- daemon/daemon_linux_test.go | 2 +- daemon/daemon_test.go | 2 +- daemon/daemon_unix.go | 2 +- daemon/oci_linux.go | 2 +- daemon/start.go | 2 +- daemon/update.go | 7 +---- daemon/volumes.go | 5 ++-- daemon/volumes_unit_test.go | 3 +-- volume/mounts/parser.go | 17 +++--------- volume/mounts/parser_test.go | 4 +-- volume/mounts/validate_test.go | 2 +- volume/service/store.go | 4 +-- 19 files changed, 49 insertions(+), 102 deletions(-) diff --git a/container/container.go b/container/container.go index 84ce8e853a..2f3675c54e 100644 --- a/container/container.go +++ b/container/container.go @@ -474,11 +474,7 @@ func (container *Container) ShouldRestart() bool { // AddMountPointWithVolume adds a new mount point configured with a volume to the container. func (container *Container) AddMountPointWithVolume(destination string, vol volume.Volume, rw bool) { - operatingSystem := container.OS - if operatingSystem == "" { - operatingSystem = runtime.GOOS - } - volumeParser := volumemounts.NewParser(operatingSystem) + volumeParser := volumemounts.NewParser() container.MountPoints[destination] = &volumemounts.MountPoint{ Type: mounttypes.TypeVolume, Name: vol.Name(), diff --git a/container/container_unix.go b/container/container_unix.go index 7a49ff55aa..a811192337 100644 --- a/container/container_unix.go +++ b/container/container_unix.go @@ -64,7 +64,7 @@ func (container *Container) BuildHostnameFile() error { func (container *Container) NetworkMounts() []Mount { var mounts []Mount shared := container.HostConfig.NetworkMode.IsContainer() - parser := volumemounts.NewParser(container.OS) + parser := volumemounts.NewParser() if container.ResolvConfPath != "" { if _, err := os.Stat(container.ResolvConfPath); err != nil { logrus.Warnf("ResolvConfPath set to %q, but can't stat this filename (err = %v); skipping", container.ResolvConfPath, err) @@ -199,7 +199,7 @@ func (container *Container) UnmountIpcMount() error { // IpcMounts returns the list of IPC mounts func (container *Container) IpcMounts() []Mount { var mounts []Mount - parser := volumemounts.NewParser(container.OS) + parser := volumemounts.NewParser() if container.HasMountFor("/dev/shm") { return mounts @@ -419,7 +419,6 @@ func copyExistingContents(source, destination string) error { // TmpfsMounts returns the list of tmpfs mounts func (container *Container) TmpfsMounts() ([]Mount, error) { - parser := volumemounts.NewParser(container.OS) var mounts []Mount for dest, data := range container.HostConfig.Tmpfs { mounts = append(mounts, Mount{ @@ -428,6 +427,7 @@ func (container *Container) TmpfsMounts() ([]Mount, error) { Data: data, }) } + parser := volumemounts.NewParser() for dest, mnt := range container.MountPoints { if mnt.Type == mounttypes.TypeTmpfs { data, err := parser.ConvertTmpfsOptions(mnt.Spec.TmpfsOptions, mnt.Spec.ReadOnly) diff --git a/daemon/archive_unix.go b/daemon/archive_unix.go index 50e6fe24be..24bc779efa 100644 --- a/daemon/archive_unix.go +++ b/daemon/archive_unix.go @@ -12,7 +12,7 @@ import ( // cannot be configured with a read-only rootfs. func checkIfPathIsInAVolume(container *container.Container, absPath string) (bool, error) { var toVolume bool - parser := volumemounts.NewParser(container.OS) + parser := volumemounts.NewParser() for _, mnt := range container.MountPoints { if toVolume = parser.HasResource(mnt, absPath); toVolume { if mnt.RW { diff --git a/daemon/container.go b/daemon/container.go index 75a1359dda..ecd9de9ca2 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -3,10 +3,8 @@ package daemon // import "github.com/docker/docker/daemon" import ( "fmt" "os" - "path" "path/filepath" "runtime" - "strings" "time" containertypes "github.com/docker/docker/api/types/container" @@ -231,12 +229,12 @@ func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig * // verifyContainerSettings performs validation of the hostconfig and config // structures. -func (daemon *Daemon) verifyContainerSettings(platform string, hostConfig *containertypes.HostConfig, config *containertypes.Config, update bool) (warnings []string, err error) { +func (daemon *Daemon) verifyContainerSettings(hostConfig *containertypes.HostConfig, config *containertypes.Config, update bool) (warnings []string, err error) { // First perform verification of settings common across all platforms. - if err = validateContainerConfig(config, platform); err != nil { + if err = validateContainerConfig(config); err != nil { return warnings, err } - if err := validateHostConfig(hostConfig, platform); err != nil { + if err := validateHostConfig(hostConfig); err != nil { return warnings, err } @@ -248,11 +246,11 @@ func (daemon *Daemon) verifyContainerSettings(platform string, hostConfig *conta return warnings, err } -func validateContainerConfig(config *containertypes.Config, platform string) error { +func validateContainerConfig(config *containertypes.Config) error { if config == nil { return nil } - if err := translateWorkingDir(config, platform); err != nil { + if err := translateWorkingDir(config); err != nil { return err } if len(config.StopSignal) > 0 { @@ -269,7 +267,7 @@ func validateContainerConfig(config *containertypes.Config, platform string) err return validateHealthCheck(config.Healthcheck) } -func validateHostConfig(hostConfig *containertypes.HostConfig, platform string) error { +func validateHostConfig(hostConfig *containertypes.HostConfig) error { if hostConfig == nil { return nil } @@ -278,7 +276,7 @@ func validateHostConfig(hostConfig *containertypes.HostConfig, platform string) return errors.Errorf("can't create 'AutoRemove' container with restart policy") } // Validate mounts; check if host directories still exist - parser := volumemounts.NewParser(platform) + parser := volumemounts.NewParser() for _, c := range hostConfig.Mounts { cfg := c if err := parser.ValidateMountConfig(&cfg); err != nil { @@ -373,23 +371,13 @@ func validateRestartPolicy(policy containertypes.RestartPolicy) error { // translateWorkingDir translates the working-dir for the target platform, // and returns an error if the given path is not an absolute path. -func translateWorkingDir(config *containertypes.Config, platform string) error { +func translateWorkingDir(config *containertypes.Config) error { if config.WorkingDir == "" { return nil } - wd := config.WorkingDir - switch { - case runtime.GOOS != platform: - // LCOW. Force Unix semantics - wd = strings.Replace(wd, string(os.PathSeparator), "/", -1) - if !path.IsAbs(wd) { - return fmt.Errorf("the working directory '%s' is invalid, it needs to be an absolute path", config.WorkingDir) - } - default: - wd = filepath.FromSlash(wd) // Ensure in platform semantics - if !system.IsAbs(wd) { - return fmt.Errorf("the working directory '%s' is invalid, it needs to be an absolute path", config.WorkingDir) - } + wd := filepath.FromSlash(config.WorkingDir) // Ensure in platform semantics + if !system.IsAbs(wd) { + return fmt.Errorf("the working directory '%s' is invalid, it needs to be an absolute path", config.WorkingDir) } config.WorkingDir = wd return nil diff --git a/daemon/container_unix_test.go b/daemon/container_unix_test.go index 4b35f63db0..cb3395096a 100644 --- a/daemon/container_unix_test.go +++ b/daemon/container_unix_test.go @@ -38,7 +38,7 @@ func TestContainerWarningHostAndPublishPorts(t *testing.T) { }, } d := &Daemon{configStore: cs} - wrns, err := d.verifyContainerSettings("", hostConfig, &containertypes.Config{}, false) + wrns, err := d.verifyContainerSettings(hostConfig, &containertypes.Config{}, false) assert.NilError(t, err) assert.DeepEqual(t, tc.warnings, wrns) } diff --git a/daemon/create.go b/daemon/create.go index 918db07b06..b71e183a64 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -16,7 +16,6 @@ import ( "github.com/docker/docker/errdefs" "github.com/docker/docker/image" "github.com/docker/docker/pkg/idtools" - "github.com/docker/docker/pkg/system" "github.com/docker/docker/runconfig" v1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/opencontainers/selinux/go-selinux" @@ -61,31 +60,23 @@ func (daemon *Daemon) containerCreate(opts createOpts) (containertypes.Container return containertypes.ContainerCreateCreatedBody{}, errdefs.InvalidParameter(errors.New("Config cannot be empty in order to create a container")) } - os := runtime.GOOS - var img *image.Image - if opts.params.Config.Image != "" { - var err error - img, err = daemon.imageService.GetImage(opts.params.Config.Image, opts.params.Platform) - if err == nil { - os = img.OS - } - } - - warnings, err := daemon.verifyContainerSettings(os, opts.params.HostConfig, opts.params.Config, false) + warnings, err := daemon.verifyContainerSettings(opts.params.HostConfig, opts.params.Config, false) if err != nil { return containertypes.ContainerCreateCreatedBody{Warnings: warnings}, errdefs.InvalidParameter(err) } - if img != nil && opts.params.Platform == nil { - p := platforms.DefaultSpec() - imgPlat := v1.Platform{ - OS: img.OS, - Architecture: img.Architecture, - Variant: img.Variant, - } + if opts.params.Platform == nil && opts.params.Config.Image != "" { + if img, _ := daemon.imageService.GetImage(opts.params.Config.Image, opts.params.Platform); img != nil { + p := platforms.DefaultSpec() + imgPlat := v1.Platform{ + OS: img.OS, + Architecture: img.Architecture, + Variant: img.Variant, + } - if !images.OnlyPlatformWithFallback(p).Match(imgPlat) { - warnings = append(warnings, fmt.Sprintf("The requested image's platform (%s) does not match the detected host platform (%s) and no specific platform was requested", platforms.Format(imgPlat), platforms.Format(p))) + if !images.OnlyPlatformWithFallback(p).Match(imgPlat) { + warnings = append(warnings, fmt.Sprintf("The requested image's platform (%s) does not match the detected host platform (%s) and no specific platform was requested", platforms.Format(imgPlat), platforms.Format(p))) + } } } @@ -132,21 +123,13 @@ func (daemon *Daemon) create(opts createOpts) (retC *container.Container, retErr } if img.OS != "" { os = img.OS - } else { - // default to the host OS except on Windows with LCOW - if isWindows && system.LCOWSupported() { - os = "linux" - } } imgID = img.ID() - - if isWindows && img.OS == "linux" && !system.LCOWSupported() { + if isWindows && img.OS == "linux" { return nil, errors.New("operating system on which parent image was created is not Windows") } - } else { - if isWindows { - os = "linux" // 'scratch' case. - } + } else if isWindows { + os = "linux" // 'scratch' case. } // On WCOW, if are not being invoked by the builder to create this container (where diff --git a/daemon/create_windows.go b/daemon/create_windows.go index 3bce278773..de89f693ef 100644 --- a/daemon/create_windows.go +++ b/daemon/create_windows.go @@ -28,7 +28,7 @@ func (daemon *Daemon) createContainerOSSpecificSettings(container *container.Con } hostConfig.Isolation = "hyperv" } - parser := volumemounts.NewParser(container.OS) + parser := volumemounts.NewParser() for spec := range config.Volumes { mp, err := parser.ParseMountRaw(spec, hostConfig.VolumeDriver) diff --git a/daemon/daemon_linux_test.go b/daemon/daemon_linux_test.go index 8e13523fda..32f600d2be 100644 --- a/daemon/daemon_linux_test.go +++ b/daemon/daemon_linux_test.go @@ -116,7 +116,7 @@ func TestNotCleanupMounts(t *testing.T) { func TestValidateContainerIsolationLinux(t *testing.T) { d := Daemon{} - _, err := d.verifyContainerSettings("linux", &containertypes.HostConfig{Isolation: containertypes.IsolationHyperV}, nil, false) + _, err := d.verifyContainerSettings(&containertypes.HostConfig{Isolation: containertypes.IsolationHyperV}, nil, false) assert.Check(t, is.Error(err, "invalid isolation 'hyperv' on linux")) } diff --git a/daemon/daemon_test.go b/daemon/daemon_test.go index 52f8434bd9..87a85616c6 100644 --- a/daemon/daemon_test.go +++ b/daemon/daemon_test.go @@ -305,7 +305,7 @@ func TestMerge(t *testing.T) { func TestValidateContainerIsolation(t *testing.T) { d := Daemon{} - _, err := d.verifyContainerSettings(runtime.GOOS, &containertypes.HostConfig{Isolation: containertypes.Isolation("invalid")}, nil, false) + _, err := d.verifyContainerSettings(&containertypes.HostConfig{Isolation: containertypes.Isolation("invalid")}, nil, false) assert.Check(t, is.Error(err, "invalid isolation 'invalid' on "+runtime.GOOS)) } diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 4818d06f60..385efcc958 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -719,7 +719,7 @@ func verifyPlatformContainerSettings(daemon *Daemon, hostConfig *containertypes. return warnings, fmt.Errorf("Unknown runtime specified %s", hostConfig.Runtime) } - parser := volumemounts.NewParser(runtime.GOOS) + parser := volumemounts.NewParser() for dest := range hostConfig.Tmpfs { if err := parser.ValidateTmpfsMountDestination(dest); err != nil { return warnings, err diff --git a/daemon/oci_linux.go b/daemon/oci_linux.go index 6358450d37..581eebb061 100644 --- a/daemon/oci_linux.go +++ b/daemon/oci_linux.go @@ -570,7 +570,7 @@ func WithMounts(daemon *Daemon, c *container.Container) coci.SpecOpts { for _, m := range mounts { if m.Source == "tmpfs" { data := m.Data - parser := volumemounts.NewParser("linux") + parser := volumemounts.NewParser() options := []string{"noexec", "nosuid", "nodev", string(parser.DefaultPropagationMode())} if data != "" { options = append(options, strings.Split(data, ",")...) diff --git a/daemon/start.go b/daemon/start.go index f92788b0b1..fc5aa5b55f 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -81,7 +81,7 @@ func (daemon *Daemon) ContainerStart(name string, hostConfig *containertypes.Hos // check if hostConfig is in line with the current system settings. // It may happen cgroups are umounted or the like. - if _, err = daemon.verifyContainerSettings(ctr.OS, ctr.HostConfig, nil, false); err != nil { + if _, err = daemon.verifyContainerSettings(ctr.HostConfig, nil, false); err != nil { return errdefs.InvalidParameter(err) } // Adapt for old containers in case we have updates in this function and diff --git a/daemon/update.go b/daemon/update.go index 65ea6885e6..bd8479fc05 100644 --- a/daemon/update.go +++ b/daemon/update.go @@ -13,12 +13,7 @@ import ( func (daemon *Daemon) ContainerUpdate(name string, hostConfig *container.HostConfig) (container.ContainerUpdateOKBody, error) { var warnings []string - c, err := daemon.GetContainer(name) - if err != nil { - return container.ContainerUpdateOKBody{Warnings: warnings}, err - } - - warnings, err = daemon.verifyContainerSettings(c.OS, hostConfig, nil, true) + warnings, err := daemon.verifyContainerSettings(hostConfig, nil, true) if err != nil { return container.ContainerUpdateOKBody{Warnings: warnings}, errdefs.InvalidParameter(err) } diff --git a/daemon/volumes.go b/daemon/volumes.go index c396843831..7327fe35f2 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -62,7 +62,7 @@ func (m mounts) parts(i int) int { func (daemon *Daemon) registerMountPoints(container *container.Container, hostConfig *containertypes.HostConfig) (retErr error) { binds := map[string]bool{} mountPoints := map[string]*volumemounts.MountPoint{} - parser := volumemounts.NewParser(container.OS) + parser := volumemounts.NewParser() ctx := context.TODO() defer func() { @@ -265,8 +265,6 @@ func (daemon *Daemon) backportMountSpec(container *container.Container) { container.Lock() defer container.Unlock() - parser := volumemounts.NewParser(container.OS) - maybeUpdate := make(map[string]bool) for _, mp := range container.MountPoints { if mp.Spec.Source != "" && mp.Type != "" { @@ -283,6 +281,7 @@ func (daemon *Daemon) backportMountSpec(container *container.Container) { mountSpecs[m.Target] = true } + parser := volumemounts.NewParser() binds := make(map[string]*volumemounts.MountPoint, len(container.HostConfig.Binds)) for _, rawSpec := range container.HostConfig.Binds { mp, err := parser.ParseMountRaw(rawSpec, container.HostConfig.VolumeDriver) diff --git a/daemon/volumes_unit_test.go b/daemon/volumes_unit_test.go index 6bdebe467c..9a572215f2 100644 --- a/daemon/volumes_unit_test.go +++ b/daemon/volumes_unit_test.go @@ -1,7 +1,6 @@ package daemon // import "github.com/docker/docker/daemon" import ( - "runtime" "testing" volumemounts "github.com/docker/docker/volume/mounts" @@ -21,7 +20,7 @@ func TestParseVolumesFrom(t *testing.T) { {"foobar:baz", "", "", true}, } - parser := volumemounts.NewParser(runtime.GOOS) + parser := volumemounts.NewParser() for _, c := range cases { id, mode, err := parser.ParseVolumesFrom(c.spec) diff --git a/volume/mounts/parser.go b/volume/mounts/parser.go index 976d276a82..07ac7fd3df 100644 --- a/volume/mounts/parser.go +++ b/volume/mounts/parser.go @@ -7,13 +7,6 @@ import ( "github.com/docker/docker/api/types/mount" ) -const ( - // OSLinux is the same as runtime.GOOS on linux - OSLinux = "linux" - // OSWindows is the same as runtime.GOOS on windows - OSWindows = "windows" -) - // ErrVolumeTargetIsRoot is returned when the target destination is root. // It's used by both LCOW and Linux parsers. var ErrVolumeTargetIsRoot = errors.New("invalid specification: destination can't be '/'") @@ -40,14 +33,10 @@ type Parser interface { ValidateMountConfig(mt *mount.Mount) error } -// NewParser creates a parser for a given container OS, depending on the current host OS (linux on a windows host will resolve to an lcowParser) -func NewParser(containerOS string) Parser { - switch containerOS { - case OSWindows: +// NewParser creates a parser for the current host OS +func NewParser() Parser { + if runtime.GOOS == "windows" { return &windowsParser{} } - if runtime.GOOS == OSWindows { - return &lcowParser{} - } return &linuxParser{} } diff --git a/volume/mounts/parser_test.go b/volume/mounts/parser_test.go index ef4919b944..9aadcc02dc 100644 --- a/volume/mounts/parser_test.go +++ b/volume/mounts/parser_test.go @@ -438,7 +438,7 @@ func TestParseMountSpec(t *testing.T) { t.Fatal(err) } defer os.RemoveAll(testDir) - parser := NewParser(runtime.GOOS) + parser := NewParser() cases := []c{ {mount.Mount{Type: mount.TypeBind, Source: testDir, Target: testDestinationPath, ReadOnly: true}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath, Propagation: parser.DefaultPropagationMode()}}, {mount.Mount{Type: mount.TypeBind, Source: testDir, Target: testDestinationPath}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath, RW: true, Propagation: parser.DefaultPropagationMode()}}, @@ -519,7 +519,7 @@ func TestParseMountSpecBindWithFileinfoError(t *testing.T) { } m := mount.Mount{Type: mount.TypeBind, Source: p, Target: p} - parser := NewParser(runtime.GOOS) + parser := NewParser() _, err := parser.ParseMountSpec(m) assert.ErrorContains(t, err, "some crazy error") diff --git a/volume/mounts/validate_test.go b/volume/mounts/validate_test.go index 4f83856043..e1c1796cd2 100644 --- a/volume/mounts/validate_test.go +++ b/volume/mounts/validate_test.go @@ -48,7 +48,7 @@ func TestValidateMount(t *testing.T) { {mount.Mount{Type: mount.TypeBind, Source: testDir, Target: "/foo"}, nil}, {mount.Mount{Type: "invalid", Target: "/foo"}, errors.New("mount type unknown")}, } - parser := NewParser(runtime.GOOS) + parser := NewParser() for i, x := range cases { err := parser.ValidateMountConfig(&x.input) if err == nil && x.expected == nil { diff --git a/volume/service/store.go b/volume/service/store.go index cb88845408..04e7792ef2 100644 --- a/volume/service/store.go +++ b/volume/service/store.go @@ -6,7 +6,6 @@ import ( "net" "os" "path/filepath" - "runtime" "sync" "time" @@ -578,8 +577,7 @@ func (s *VolumeStore) create(ctx context.Context, name, driverName string, opts, // Validate the name in a platform-specific manner // volume name validation is specific to the host os and not on container image - // windows/lcow should have an equivalent volumename validation logic so we create a parser for current host OS - parser := volumemounts.NewParser(runtime.GOOS) + parser := volumemounts.NewParser() err := parser.ValidateVolumeName(name) if err != nil { return nil, false, err From 536818508d12e44d2598ebd69a4baf8ed602f20e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 2 Jul 2021 13:27:57 +0200 Subject: [PATCH 3/7] volume/mounts: move TestConvertTmpfsOptions It's only testing the LinuxParser, so moving it to a file specific to that code. Signed-off-by: Sebastiaan van Stijn --- volume/mounts/linux_parser_test.go | 50 ++++++++++++++++++++++++++++++ volume/mounts/parser_test.go | 42 ------------------------- 2 files changed, 50 insertions(+), 42 deletions(-) create mode 100644 volume/mounts/linux_parser_test.go diff --git a/volume/mounts/linux_parser_test.go b/volume/mounts/linux_parser_test.go new file mode 100644 index 0000000000..ad56a502f6 --- /dev/null +++ b/volume/mounts/linux_parser_test.go @@ -0,0 +1,50 @@ +package mounts // import "github.com/docker/docker/volume/mounts" + +import ( + "strings" + "testing" + + "github.com/docker/docker/api/types/mount" +) + +func TestConvertTmpfsOptions(t *testing.T) { + type testCase struct { + opt mount.TmpfsOptions + readOnly bool + expectedSubstrings []string + unexpectedSubstrings []string + } + cases := []testCase{ + { + opt: mount.TmpfsOptions{SizeBytes: 1024 * 1024, Mode: 0700}, + readOnly: false, + expectedSubstrings: []string{"size=1m", "mode=700"}, + unexpectedSubstrings: []string{"ro"}, + }, + { + opt: mount.TmpfsOptions{}, + readOnly: true, + expectedSubstrings: []string{"ro"}, + unexpectedSubstrings: []string{}, + }, + } + p := &linuxParser{} + for _, c := range cases { + data, err := p.ConvertTmpfsOptions(&c.opt, c.readOnly) + if err != nil { + t.Fatalf("could not convert %+v (readOnly: %v) to string: %v", + c.opt, c.readOnly, err) + } + t.Logf("data=%q", data) + for _, s := range c.expectedSubstrings { + if !strings.Contains(data, s) { + t.Fatalf("expected substring: %s, got %v (case=%+v)", s, data, c) + } + } + for _, s := range c.unexpectedSubstrings { + if strings.Contains(data, s) { + t.Fatalf("unexpected substring: %s, got %v (case=%+v)", s, data, c) + } + } + } +} diff --git a/volume/mounts/parser_test.go b/volume/mounts/parser_test.go index 9aadcc02dc..029da80857 100644 --- a/volume/mounts/parser_test.go +++ b/volume/mounts/parser_test.go @@ -17,48 +17,6 @@ type parseMountRawTestSet struct { invalid map[string]string } -func TestConvertTmpfsOptions(t *testing.T) { - type testCase struct { - opt mount.TmpfsOptions - readOnly bool - expectedSubstrings []string - unexpectedSubstrings []string - } - cases := []testCase{ - { - opt: mount.TmpfsOptions{SizeBytes: 1024 * 1024, Mode: 0700}, - readOnly: false, - expectedSubstrings: []string{"size=1m", "mode=700"}, - unexpectedSubstrings: []string{"ro"}, - }, - { - opt: mount.TmpfsOptions{}, - readOnly: true, - expectedSubstrings: []string{"ro"}, - unexpectedSubstrings: []string{}, - }, - } - p := &linuxParser{} - for _, c := range cases { - data, err := p.ConvertTmpfsOptions(&c.opt, c.readOnly) - if err != nil { - t.Fatalf("could not convert %+v (readOnly: %v) to string: %v", - c.opt, c.readOnly, err) - } - t.Logf("data=%q", data) - for _, s := range c.expectedSubstrings { - if !strings.Contains(data, s) { - t.Fatalf("expected substring: %s, got %v (case=%+v)", s, data, c) - } - } - for _, s := range c.unexpectedSubstrings { - if strings.Contains(data, s) { - t.Fatalf("unexpected substring: %s, got %v (case=%+v)", s, data, c) - } - } - } -} - type mockFiProvider struct{} func (mockFiProvider) fileInfo(path string) (exists, isDir bool, err error) { From df179a1d6ae02082b02848d590000b9757bde21a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 29 Jun 2021 09:52:37 +0200 Subject: [PATCH 4/7] volume/mounts: split tests per parser Signed-off-by: Sebastiaan van Stijn --- volume/mounts/lcow_parser_test.go | 171 +++++++++++ volume/mounts/linux_parser_test.go | 194 +++++++++++++ volume/mounts/parser_test.go | 410 +-------------------------- volume/mounts/windows_parser_test.go | 214 ++++++++++++++ 4 files changed, 588 insertions(+), 401 deletions(-) create mode 100644 volume/mounts/lcow_parser_test.go create mode 100644 volume/mounts/windows_parser_test.go diff --git a/volume/mounts/lcow_parser_test.go b/volume/mounts/lcow_parser_test.go new file mode 100644 index 0000000000..8e5e2261df --- /dev/null +++ b/volume/mounts/lcow_parser_test.go @@ -0,0 +1,171 @@ +package mounts // import "github.com/docker/docker/volume/mounts" + +import ( + "strings" + "testing" + + "github.com/docker/docker/api/types/mount" +) + +func TestLCOWParseMountRaw(t *testing.T) { + previousProvider := currentFileInfoProvider + defer func() { currentFileInfoProvider = previousProvider }() + currentFileInfoProvider = mockFiProvider{} + + valid := []string{ + `/foo`, + `/foo/`, + `/foo bar`, + `c:\:/foo`, + `c:\windows\:/foo`, + `c:\windows:/s p a c e`, + `c:\windows:/s p a c e:RW`, + `c:\program files:/s p a c e i n h o s t d i r`, + `0123456789name:/foo`, + `MiXeDcAsEnAmE:/foo`, + `name:/foo`, + `name:/foo:rW`, + `name:/foo:RW`, + `name:/foo:RO`, + `c:/:/forward/slashes/are/good/too`, + `c:/:/including with/spaces:ro`, + `/Program Files (x86)`, // With capitals and brackets + } + + invalid := map[string]string{ + ``: "invalid volume specification: ", + `.`: "invalid volume specification: ", + `c:`: "invalid volume specification: ", + `c:\`: "invalid volume specification: ", + `../`: "invalid volume specification: ", + `c:\:../`: "invalid volume specification: ", + `c:\:/foo:xyzzy`: "invalid volume specification: ", + `/`: "destination can't be '/'", + `/..`: "destination can't be '/'", + `c:\notexist:/foo`: `source path does not exist: c:\notexist`, + `c:\windows\system32\ntdll.dll:/foo`: `source path must be a directory`, + `name<:/foo`: `invalid volume specification`, + `name>:/foo`: `invalid volume specification`, + `name::/foo`: `invalid volume specification`, + `name":/foo`: `invalid volume specification`, + `name\:/foo`: `invalid volume specification`, + `name*:/foo`: `invalid volume specification`, + `name|:/foo`: `invalid volume specification`, + `name?:/foo`: `invalid volume specification`, + `name/:/foo`: `invalid volume specification`, + `/foo:rw`: `invalid volume specification`, + `/foo:ro`: `invalid volume specification`, + `con:/foo`: `cannot be a reserved word for Windows filenames`, + `PRN:/foo`: `cannot be a reserved word for Windows filenames`, + `aUx:/foo`: `cannot be a reserved word for Windows filenames`, + `nul:/foo`: `cannot be a reserved word for Windows filenames`, + `com1:/foo`: `cannot be a reserved word for Windows filenames`, + `com2:/foo`: `cannot be a reserved word for Windows filenames`, + `com3:/foo`: `cannot be a reserved word for Windows filenames`, + `com4:/foo`: `cannot be a reserved word for Windows filenames`, + `com5:/foo`: `cannot be a reserved word for Windows filenames`, + `com6:/foo`: `cannot be a reserved word for Windows filenames`, + `com7:/foo`: `cannot be a reserved word for Windows filenames`, + `com8:/foo`: `cannot be a reserved word for Windows filenames`, + `com9:/foo`: `cannot be a reserved word for Windows filenames`, + `lpt1:/foo`: `cannot be a reserved word for Windows filenames`, + `lpt2:/foo`: `cannot be a reserved word for Windows filenames`, + `lpt3:/foo`: `cannot be a reserved word for Windows filenames`, + `lpt4:/foo`: `cannot be a reserved word for Windows filenames`, + `lpt5:/foo`: `cannot be a reserved word for Windows filenames`, + `lpt6:/foo`: `cannot be a reserved word for Windows filenames`, + `lpt7:/foo`: `cannot be a reserved word for Windows filenames`, + `lpt8:/foo`: `cannot be a reserved word for Windows filenames`, + `lpt9:/foo`: `cannot be a reserved word for Windows filenames`, + `\\.\pipe\foo:/foo`: `Linux containers on Windows do not support named pipe mounts`, + } + + parser := &lcowParser{} + + for _, path := range valid { + if _, err := parser.ParseMountRaw(path, "local"); err != nil { + t.Errorf("ParseMountRaw(`%q`) should succeed: error %q", path, err) + } + } + + for path, expectedError := range invalid { + if mp, err := parser.ParseMountRaw(path, "local"); err == nil { + t.Errorf("ParseMountRaw(`%q`) should have failed validation. Err '%v' - MP: %v", path, err, mp) + } else { + if !strings.Contains(err.Error(), expectedError) { + t.Errorf("ParseMountRaw(`%q`) error should contain %q, got %v", path, expectedError, err.Error()) + } + } + } +} + +func TestLCOWParseMountRawSplit(t *testing.T) { + previousProvider := currentFileInfoProvider + defer func() { currentFileInfoProvider = previousProvider }() + currentFileInfoProvider = mockFiProvider{} + + cases := []struct { + bind string + driver string + expType mount.Type + expDest string + expSource string + expName string + expDriver string + expRW bool + fail bool + }{ + {`c:\:/foo`, "local", mount.TypeBind, `/foo`, `c:\`, ``, "", true, false}, + {`c:\:/foo:ro`, "local", mount.TypeBind, `/foo`, `c:\`, ``, "", false, false}, + {`c:\:/foo:rw`, "local", mount.TypeBind, `/foo`, `c:\`, ``, "", true, false}, + {`c:\:/foo:foo`, "local", mount.TypeBind, `/foo`, `c:\`, ``, "", false, true}, + {`name:/foo:rw`, "local", mount.TypeVolume, `/foo`, ``, `name`, "local", true, false}, + {`name:/foo`, "local", mount.TypeVolume, `/foo`, ``, `name`, "local", true, false}, + {`name:/foo:ro`, "local", mount.TypeVolume, `/foo`, ``, `name`, "local", false, false}, + {`name:/`, "", mount.TypeVolume, ``, ``, ``, "", true, true}, + {`driver/name:/`, "", mount.TypeVolume, ``, ``, ``, "", true, true}, + {`\\.\pipe\foo:\\.\pipe\bar`, "local", mount.TypeNamedPipe, `\\.\pipe\bar`, `\\.\pipe\foo`, "", "", true, true}, + {`\\.\pipe\foo:/data`, "local", mount.TypeNamedPipe, ``, ``, "", "", true, true}, + {`c:\foo\bar:\\.\pipe\foo`, "local", mount.TypeNamedPipe, ``, ``, "", "", true, true}, + } + + parser := &lcowParser{} + for i, c := range cases { + t.Logf("case %d", i) + m, err := parser.ParseMountRaw(c.bind, c.driver) + if c.fail { + if err == nil { + t.Errorf("Expected error, was nil, for spec %s\n", c.bind) + } + continue + } + + if m == nil || err != nil { + t.Errorf("ParseMountRaw failed for spec '%s', driver '%s', error '%v'", c.bind, c.driver, err) + continue + } + + if m.Destination != c.expDest { + t.Errorf("Expected destination '%s, was %s', for spec '%s'", c.expDest, m.Destination, c.bind) + } + + if m.Source != c.expSource { + t.Errorf("Expected source '%s', was '%s', for spec '%s'", c.expSource, m.Source, c.bind) + } + + if m.Name != c.expName { + t.Errorf("Expected name '%s', was '%s' for spec '%s'", c.expName, m.Name, c.bind) + } + + if m.Driver != c.expDriver { + t.Errorf("Expected driver '%s', was '%s', for spec '%s'", c.expDriver, m.Driver, c.bind) + } + + if m.RW != c.expRW { + t.Errorf("Expected RW '%v', was '%v' for spec '%s'", c.expRW, m.RW, c.bind) + } + if m.Type != c.expType { + t.Fatalf("Expected type '%s', was '%s', for spec '%s'", c.expType, m.Type, c.bind) + } + } +} diff --git a/volume/mounts/linux_parser_test.go b/volume/mounts/linux_parser_test.go index ad56a502f6..00c8cff47a 100644 --- a/volume/mounts/linux_parser_test.go +++ b/volume/mounts/linux_parser_test.go @@ -1,12 +1,206 @@ package mounts // import "github.com/docker/docker/volume/mounts" import ( + "fmt" "strings" "testing" "github.com/docker/docker/api/types/mount" + "gotest.tools/v3/assert" ) +func TestLinuxParseMountRaw(t *testing.T) { + previousProvider := currentFileInfoProvider + defer func() { currentFileInfoProvider = previousProvider }() + currentFileInfoProvider = mockFiProvider{} + + valid := []string{ + "/home", + "/home:/home", + "/home:/something/else", + "/with space", + "/home:/with space", + "relative:/absolute-path", + "hostPath:/containerPath:ro", + "/hostPath:/containerPath:rw", + "/rw:/ro", + "/hostPath:/containerPath:shared", + "/hostPath:/containerPath:rshared", + "/hostPath:/containerPath:slave", + "/hostPath:/containerPath:rslave", + "/hostPath:/containerPath:private", + "/hostPath:/containerPath:rprivate", + "/hostPath:/containerPath:ro,shared", + "/hostPath:/containerPath:ro,slave", + "/hostPath:/containerPath:ro,private", + "/hostPath:/containerPath:ro,z,shared", + "/hostPath:/containerPath:ro,Z,slave", + "/hostPath:/containerPath:Z,ro,slave", + "/hostPath:/containerPath:slave,Z,ro", + "/hostPath:/containerPath:Z,slave,ro", + "/hostPath:/containerPath:slave,ro,Z", + "/hostPath:/containerPath:rslave,ro,Z", + "/hostPath:/containerPath:ro,rshared,Z", + "/hostPath:/containerPath:ro,Z,rprivate", + } + + invalid := map[string]string{ + "": "invalid volume specification", + "./": "mount path must be absolute", + "../": "mount path must be absolute", + "/:../": "mount path must be absolute", + "/:path": "mount path must be absolute", + ":": "invalid volume specification", + "/tmp:": "invalid volume specification", + ":test": "invalid volume specification", + ":/test": "invalid volume specification", + "tmp:": "invalid volume specification", + ":test:": "invalid volume specification", + "::": "invalid volume specification", + ":::": "invalid volume specification", + "/tmp:::": "invalid volume specification", + ":/tmp::": "invalid volume specification", + "/path:rw": "invalid volume specification", + "/path:ro": "invalid volume specification", + "/rw:rw": "invalid volume specification", + "path:ro": "invalid volume specification", + "/path:/path:sw": `invalid mode`, + "/path:/path:rwz": `invalid mode`, + "/path:/path:ro,rshared,rslave": `invalid mode`, + "/path:/path:ro,z,rshared,rslave": `invalid mode`, + "/path:shared": "invalid volume specification", + "/path:slave": "invalid volume specification", + "/path:private": "invalid volume specification", + "name:/absolute-path:shared": "invalid volume specification", + "name:/absolute-path:rshared": "invalid volume specification", + "name:/absolute-path:slave": "invalid volume specification", + "name:/absolute-path:rslave": "invalid volume specification", + "name:/absolute-path:private": "invalid volume specification", + "name:/absolute-path:rprivate": "invalid volume specification", + } + + parser := &linuxParser{} + + for _, path := range valid { + if _, err := parser.ParseMountRaw(path, "local"); err != nil { + t.Errorf("ParseMountRaw(`%q`) should succeed: error %q", path, err) + } + } + + for path, expectedError := range invalid { + if mp, err := parser.ParseMountRaw(path, "local"); err == nil { + t.Errorf("ParseMountRaw(`%q`) should have failed validation. Err '%v' - MP: %v", path, err, mp) + } else { + if !strings.Contains(err.Error(), expectedError) { + t.Errorf("ParseMountRaw(`%q`) error should contain %q, got %v", path, expectedError, err.Error()) + } + } + } +} + +func TestLinuxParseMountRawSplit(t *testing.T) { + previousProvider := currentFileInfoProvider + defer func() { currentFileInfoProvider = previousProvider }() + currentFileInfoProvider = mockFiProvider{} + + cases := []struct { + bind string + driver string + expType mount.Type + expDest string + expSource string + expName string + expDriver string + expRW bool + fail bool + }{ + {"/tmp:/tmp1", "", mount.TypeBind, "/tmp1", "/tmp", "", "", true, false}, + {"/tmp:/tmp2:ro", "", mount.TypeBind, "/tmp2", "/tmp", "", "", false, false}, + {"/tmp:/tmp3:rw", "", mount.TypeBind, "/tmp3", "/tmp", "", "", true, false}, + {"/tmp:/tmp4:foo", "", mount.TypeBind, "", "", "", "", false, true}, + {"name:/named1", "", mount.TypeVolume, "/named1", "", "name", "", true, false}, + {"name:/named2", "external", mount.TypeVolume, "/named2", "", "name", "external", true, false}, + {"name:/named3:ro", "local", mount.TypeVolume, "/named3", "", "name", "local", false, false}, + {"local/name:/tmp:rw", "", mount.TypeVolume, "/tmp", "", "local/name", "", true, false}, + {"/tmp:tmp", "", mount.TypeBind, "", "", "", "", true, true}, + } + + parser := &linuxParser{} + for i, c := range cases { + t.Logf("case %d", i) + m, err := parser.ParseMountRaw(c.bind, c.driver) + if c.fail { + if err == nil { + t.Errorf("Expected error, was nil, for spec %s\n", c.bind) + } + continue + } + + if m == nil || err != nil { + t.Errorf("ParseMountRaw failed for spec '%s', driver '%s', error '%v'", c.bind, c.driver, err) + continue + } + + if m.Destination != c.expDest { + t.Errorf("Expected destination '%s, was %s', for spec '%s'", c.expDest, m.Destination, c.bind) + } + + if m.Source != c.expSource { + t.Errorf("Expected source '%s', was '%s', for spec '%s'", c.expSource, m.Source, c.bind) + } + + if m.Name != c.expName { + t.Errorf("Expected name '%s', was '%s' for spec '%s'", c.expName, m.Name, c.bind) + } + + if m.Driver != c.expDriver { + t.Errorf("Expected driver '%s', was '%s', for spec '%s'", c.expDriver, m.Driver, c.bind) + } + + if m.RW != c.expRW { + t.Errorf("Expected RW '%v', was '%v' for spec '%s'", c.expRW, m.RW, c.bind) + } + if m.Type != c.expType { + t.Fatalf("Expected type '%s', was '%s', for spec '%s'", c.expType, m.Type, c.bind) + } + } +} + +// TestLinuxParseMountSpecBindWithFileinfoError makes sure that the parser returns +// the error produced by the fileinfo provider. +// +// Some extra context for the future in case of changes and possible wtf are we +// testing this for: +// +// Currently this "fileInfoProvider" returns (bool, bool, error) +// The 1st bool is "does this path exist" +// The 2nd bool is "is this path a dir" +// Then of course the error is an error. +// +// The issue is the parser was ignoring the error and only looking at the +// "does this path exist" boolean, which is always false if there is an error. +// Then the error returned to the caller was a (slightly, maybe) friendlier +// error string than what comes from `os.Stat` +// So ...the caller was always getting an error saying the path doesn't exist +// even if it does exist but got some other error (like a permission error). +// This is confusing to users. +func TestLinuxParseMountSpecBindWithFileinfoError(t *testing.T) { + previousProvider := currentFileInfoProvider + defer func() { currentFileInfoProvider = previousProvider }() + + testErr := fmt.Errorf("some crazy error") + currentFileInfoProvider = &mockFiProviderWithError{err: testErr} + + parser := &linuxParser{} + + _, err := parser.ParseMountSpec(mount.Mount{ + Type: mount.TypeBind, + Source: `/bananas`, + Target: `/bananas`, + }) + assert.ErrorContains(t, err, testErr.Error()) +} + func TestConvertTmpfsOptions(t *testing.T) { type testCase struct { opt mount.TmpfsOptions diff --git a/volume/mounts/parser_test.go b/volume/mounts/parser_test.go index 029da80857..1e0117dae6 100644 --- a/volume/mounts/parser_test.go +++ b/volume/mounts/parser_test.go @@ -1,22 +1,13 @@ package mounts // import "github.com/docker/docker/volume/mounts" import ( - "errors" "io/ioutil" "os" - "runtime" - "strings" "testing" "github.com/docker/docker/api/types/mount" - "gotest.tools/v3/assert" ) -type parseMountRawTestSet struct { - valid []string - invalid map[string]string -} - type mockFiProvider struct{} func (mockFiProvider) fileInfo(path string) (exists, isDir bool, err error) { @@ -41,363 +32,25 @@ func (mockFiProvider) fileInfo(path string) (exists, isDir bool, err error) { return false, false, nil } -func TestParseMountRaw(t *testing.T) { +// always returns the configured error +// this is used to test error handling +type mockFiProviderWithError struct{ err error } - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - currentFileInfoProvider = mockFiProvider{} - windowsSet := parseMountRawTestSet{ - valid: []string{ - `d:\`, - `d:`, - `d:\path`, - `d:\path with space`, - `c:\:d:\`, - `c:\windows\:d:`, - `c:\windows:d:\s p a c e`, - `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`, - `0123456789name:d:`, - `MiXeDcAsEnAmE:d:`, - `name:D:`, - `name:D::rW`, - `name:D::RW`, - `name:D::RO`, - `c:/:d:/forward/slashes/are/good/too`, - `c:/:d:/including with/spaces:ro`, - `c:\Windows`, // With capital - `c:\Program Files (x86)`, // With capitals and brackets - `\\?\c:\windows\:d:`, // Long path handling (source) - `c:\windows\:\\?\d:\`, // Long path handling (target) - `\\.\pipe\foo:\\.\pipe\foo`, // named pipe - `//./pipe/foo://./pipe/foo`, // named pipe forward slashes - }, - invalid: map[string]string{ - ``: "invalid volume specification: ", - `.`: "invalid volume specification: ", - `..\`: "invalid volume specification: ", - `c:\:..\`: "invalid volume specification: ", - `c:\:d:\:xyzzy`: "invalid volume specification: ", - `c:`: "cannot be `c:`", - `c:\`: "cannot be `c:`", - `c:\notexist:d:`: `source path does not exist: c:\notexist`, - `c:\windows\system32\ntdll.dll:d:`: `source path must be a directory`, - `name<:d:`: `invalid volume specification`, - `name>:d:`: `invalid volume specification`, - `name::d:`: `invalid volume specification`, - `name":d:`: `invalid volume specification`, - `name\:d:`: `invalid volume specification`, - `name*:d:`: `invalid volume specification`, - `name|:d:`: `invalid volume specification`, - `name?:d:`: `invalid volume specification`, - `name/:d:`: `invalid volume specification`, - `d:\pathandmode:rw`: `invalid volume specification`, - `d:\pathandmode:ro`: `invalid volume specification`, - `con:d:`: `cannot be a reserved word for Windows filenames`, - `PRN:d:`: `cannot be a reserved word for Windows filenames`, - `aUx:d:`: `cannot be a reserved word for Windows filenames`, - `nul:d:`: `cannot be a reserved word for Windows filenames`, - `com1:d:`: `cannot be a reserved word for Windows filenames`, - `com2:d:`: `cannot be a reserved word for Windows filenames`, - `com3:d:`: `cannot be a reserved word for Windows filenames`, - `com4:d:`: `cannot be a reserved word for Windows filenames`, - `com5:d:`: `cannot be a reserved word for Windows filenames`, - `com6:d:`: `cannot be a reserved word for Windows filenames`, - `com7:d:`: `cannot be a reserved word for Windows filenames`, - `com8:d:`: `cannot be a reserved word for Windows filenames`, - `com9:d:`: `cannot be a reserved word for Windows filenames`, - `lpt1:d:`: `cannot be a reserved word for Windows filenames`, - `lpt2:d:`: `cannot be a reserved word for Windows filenames`, - `lpt3:d:`: `cannot be a reserved word for Windows filenames`, - `lpt4:d:`: `cannot be a reserved word for Windows filenames`, - `lpt5:d:`: `cannot be a reserved word for Windows filenames`, - `lpt6:d:`: `cannot be a reserved word for Windows filenames`, - `lpt7:d:`: `cannot be a reserved word for Windows filenames`, - `lpt8:d:`: `cannot be a reserved word for Windows filenames`, - `lpt9:d:`: `cannot be a reserved word for Windows filenames`, - `c:\windows\system32\ntdll.dll`: `Only directories can be mapped on this platform`, - `\\.\pipe\foo:c:\pipe`: `'c:\pipe' is not a valid pipe path`, - }, - } - lcowSet := parseMountRawTestSet{ - valid: []string{ - `/foo`, - `/foo/`, - `/foo bar`, - `c:\:/foo`, - `c:\windows\:/foo`, - `c:\windows:/s p a c e`, - `c:\windows:/s p a c e:RW`, - `c:\program files:/s p a c e i n h o s t d i r`, - `0123456789name:/foo`, - `MiXeDcAsEnAmE:/foo`, - `name:/foo`, - `name:/foo:rW`, - `name:/foo:RW`, - `name:/foo:RO`, - `c:/:/forward/slashes/are/good/too`, - `c:/:/including with/spaces:ro`, - `/Program Files (x86)`, // With capitals and brackets - }, - invalid: map[string]string{ - ``: "invalid volume specification: ", - `.`: "invalid volume specification: ", - `c:`: "invalid volume specification: ", - `c:\`: "invalid volume specification: ", - `../`: "invalid volume specification: ", - `c:\:../`: "invalid volume specification: ", - `c:\:/foo:xyzzy`: "invalid volume specification: ", - `/`: "destination can't be '/'", - `/..`: "destination can't be '/'", - `c:\notexist:/foo`: `source path does not exist: c:\notexist`, - `c:\windows\system32\ntdll.dll:/foo`: `source path must be a directory`, - `name<:/foo`: `invalid volume specification`, - `name>:/foo`: `invalid volume specification`, - `name::/foo`: `invalid volume specification`, - `name":/foo`: `invalid volume specification`, - `name\:/foo`: `invalid volume specification`, - `name*:/foo`: `invalid volume specification`, - `name|:/foo`: `invalid volume specification`, - `name?:/foo`: `invalid volume specification`, - `name/:/foo`: `invalid volume specification`, - `/foo:rw`: `invalid volume specification`, - `/foo:ro`: `invalid volume specification`, - `con:/foo`: `cannot be a reserved word for Windows filenames`, - `PRN:/foo`: `cannot be a reserved word for Windows filenames`, - `aUx:/foo`: `cannot be a reserved word for Windows filenames`, - `nul:/foo`: `cannot be a reserved word for Windows filenames`, - `com1:/foo`: `cannot be a reserved word for Windows filenames`, - `com2:/foo`: `cannot be a reserved word for Windows filenames`, - `com3:/foo`: `cannot be a reserved word for Windows filenames`, - `com4:/foo`: `cannot be a reserved word for Windows filenames`, - `com5:/foo`: `cannot be a reserved word for Windows filenames`, - `com6:/foo`: `cannot be a reserved word for Windows filenames`, - `com7:/foo`: `cannot be a reserved word for Windows filenames`, - `com8:/foo`: `cannot be a reserved word for Windows filenames`, - `com9:/foo`: `cannot be a reserved word for Windows filenames`, - `lpt1:/foo`: `cannot be a reserved word for Windows filenames`, - `lpt2:/foo`: `cannot be a reserved word for Windows filenames`, - `lpt3:/foo`: `cannot be a reserved word for Windows filenames`, - `lpt4:/foo`: `cannot be a reserved word for Windows filenames`, - `lpt5:/foo`: `cannot be a reserved word for Windows filenames`, - `lpt6:/foo`: `cannot be a reserved word for Windows filenames`, - `lpt7:/foo`: `cannot be a reserved word for Windows filenames`, - `lpt8:/foo`: `cannot be a reserved word for Windows filenames`, - `lpt9:/foo`: `cannot be a reserved word for Windows filenames`, - `\\.\pipe\foo:/foo`: `Linux containers on Windows do not support named pipe mounts`, - }, - } - linuxSet := parseMountRawTestSet{ - valid: []string{ - "/home", - "/home:/home", - "/home:/something/else", - "/with space", - "/home:/with space", - "relative:/absolute-path", - "hostPath:/containerPath:ro", - "/hostPath:/containerPath:rw", - "/rw:/ro", - "/hostPath:/containerPath:shared", - "/hostPath:/containerPath:rshared", - "/hostPath:/containerPath:slave", - "/hostPath:/containerPath:rslave", - "/hostPath:/containerPath:private", - "/hostPath:/containerPath:rprivate", - "/hostPath:/containerPath:ro,shared", - "/hostPath:/containerPath:ro,slave", - "/hostPath:/containerPath:ro,private", - "/hostPath:/containerPath:ro,z,shared", - "/hostPath:/containerPath:ro,Z,slave", - "/hostPath:/containerPath:Z,ro,slave", - "/hostPath:/containerPath:slave,Z,ro", - "/hostPath:/containerPath:Z,slave,ro", - "/hostPath:/containerPath:slave,ro,Z", - "/hostPath:/containerPath:rslave,ro,Z", - "/hostPath:/containerPath:ro,rshared,Z", - "/hostPath:/containerPath:ro,Z,rprivate", - }, - invalid: map[string]string{ - "": "invalid volume specification", - "./": "mount path must be absolute", - "../": "mount path must be absolute", - "/:../": "mount path must be absolute", - "/:path": "mount path must be absolute", - ":": "invalid volume specification", - "/tmp:": "invalid volume specification", - ":test": "invalid volume specification", - ":/test": "invalid volume specification", - "tmp:": "invalid volume specification", - ":test:": "invalid volume specification", - "::": "invalid volume specification", - ":::": "invalid volume specification", - "/tmp:::": "invalid volume specification", - ":/tmp::": "invalid volume specification", - "/path:rw": "invalid volume specification", - "/path:ro": "invalid volume specification", - "/rw:rw": "invalid volume specification", - "path:ro": "invalid volume specification", - "/path:/path:sw": `invalid mode`, - "/path:/path:rwz": `invalid mode`, - "/path:/path:ro,rshared,rslave": `invalid mode`, - "/path:/path:ro,z,rshared,rslave": `invalid mode`, - "/path:shared": "invalid volume specification", - "/path:slave": "invalid volume specification", - "/path:private": "invalid volume specification", - "name:/absolute-path:shared": "invalid volume specification", - "name:/absolute-path:rshared": "invalid volume specification", - "name:/absolute-path:slave": "invalid volume specification", - "name:/absolute-path:rslave": "invalid volume specification", - "name:/absolute-path:private": "invalid volume specification", - "name:/absolute-path:rprivate": "invalid volume specification", - }, - } - - linParser := &linuxParser{} - winParser := &windowsParser{} - lcowParser := &lcowParser{} - tester := func(parser Parser, set parseMountRawTestSet) { - for _, path := range set.valid { - if _, err := parser.ParseMountRaw(path, "local"); err != nil { - t.Errorf("ParseMountRaw(`%q`) should succeed: error %q", path, err) - } - } - - for path, expectedError := range set.invalid { - if mp, err := parser.ParseMountRaw(path, "local"); err == nil { - t.Errorf("ParseMountRaw(`%q`) should have failed validation. Err '%v' - MP: %v", path, err, mp) - } else { - if !strings.Contains(err.Error(), expectedError) { - t.Errorf("ParseMountRaw(`%q`) error should contain %q, got %v", path, expectedError, err.Error()) - } - } - } - } - - tester(linParser, linuxSet) - tester(winParser, windowsSet) - tester(lcowParser, lcowSet) -} - -// testParseMountRaw is a structure used by TestParseMountRawSplit for -// specifying test cases for the ParseMountRaw() function. -type testParseMountRaw struct { - bind string - driver string - expType mount.Type - expDest string - expSource string - expName string - expDriver string - expRW bool - fail bool -} - -func TestParseMountRawSplit(t *testing.T) { - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - currentFileInfoProvider = mockFiProvider{} - windowsCases := []testParseMountRaw{ - {`c:\:d:`, "local", mount.TypeBind, `d:`, `c:\`, ``, "", true, false}, - {`c:\:d:\`, "local", mount.TypeBind, `d:\`, `c:\`, ``, "", true, false}, - {`c:\:d:\:ro`, "local", mount.TypeBind, `d:\`, `c:\`, ``, "", false, false}, - {`c:\:d:\:rw`, "local", mount.TypeBind, `d:\`, `c:\`, ``, "", true, false}, - {`c:\:d:\:foo`, "local", mount.TypeBind, `d:\`, `c:\`, ``, "", false, true}, - {`name:d::rw`, "local", mount.TypeVolume, `d:`, ``, `name`, "local", true, false}, - {`name:d:`, "local", mount.TypeVolume, `d:`, ``, `name`, "local", true, false}, - {`name:d::ro`, "local", mount.TypeVolume, `d:`, ``, `name`, "local", false, false}, - {`name:c:`, "", mount.TypeVolume, ``, ``, ``, "", true, true}, - {`driver/name:c:`, "", mount.TypeVolume, ``, ``, ``, "", true, true}, - {`\\.\pipe\foo:\\.\pipe\bar`, "local", mount.TypeNamedPipe, `\\.\pipe\bar`, `\\.\pipe\foo`, "", "", true, false}, - {`\\.\pipe\foo:c:\foo\bar`, "local", mount.TypeNamedPipe, ``, ``, "", "", true, true}, - {`c:\foo\bar:\\.\pipe\foo`, "local", mount.TypeNamedPipe, ``, ``, "", "", true, true}, - } - lcowCases := []testParseMountRaw{ - {`c:\:/foo`, "local", mount.TypeBind, `/foo`, `c:\`, ``, "", true, false}, - {`c:\:/foo:ro`, "local", mount.TypeBind, `/foo`, `c:\`, ``, "", false, false}, - {`c:\:/foo:rw`, "local", mount.TypeBind, `/foo`, `c:\`, ``, "", true, false}, - {`c:\:/foo:foo`, "local", mount.TypeBind, `/foo`, `c:\`, ``, "", false, true}, - {`name:/foo:rw`, "local", mount.TypeVolume, `/foo`, ``, `name`, "local", true, false}, - {`name:/foo`, "local", mount.TypeVolume, `/foo`, ``, `name`, "local", true, false}, - {`name:/foo:ro`, "local", mount.TypeVolume, `/foo`, ``, `name`, "local", false, false}, - {`name:/`, "", mount.TypeVolume, ``, ``, ``, "", true, true}, - {`driver/name:/`, "", mount.TypeVolume, ``, ``, ``, "", true, true}, - {`\\.\pipe\foo:\\.\pipe\bar`, "local", mount.TypeNamedPipe, `\\.\pipe\bar`, `\\.\pipe\foo`, "", "", true, true}, - {`\\.\pipe\foo:/data`, "local", mount.TypeNamedPipe, ``, ``, "", "", true, true}, - {`c:\foo\bar:\\.\pipe\foo`, "local", mount.TypeNamedPipe, ``, ``, "", "", true, true}, - } - linuxCases := []testParseMountRaw{ - {"/tmp:/tmp1", "", mount.TypeBind, "/tmp1", "/tmp", "", "", true, false}, - {"/tmp:/tmp2:ro", "", mount.TypeBind, "/tmp2", "/tmp", "", "", false, false}, - {"/tmp:/tmp3:rw", "", mount.TypeBind, "/tmp3", "/tmp", "", "", true, false}, - {"/tmp:/tmp4:foo", "", mount.TypeBind, "", "", "", "", false, true}, - {"name:/named1", "", mount.TypeVolume, "/named1", "", "name", "", true, false}, - {"name:/named2", "external", mount.TypeVolume, "/named2", "", "name", "external", true, false}, - {"name:/named3:ro", "local", mount.TypeVolume, "/named3", "", "name", "local", false, false}, - {"local/name:/tmp:rw", "", mount.TypeVolume, "/tmp", "", "local/name", "", true, false}, - {"/tmp:tmp", "", mount.TypeBind, "", "", "", "", true, true}, - } - linParser := &linuxParser{} - winParser := &windowsParser{} - lcowParser := &lcowParser{} - tester := func(parser Parser, cases []testParseMountRaw) { - for i, c := range cases { - t.Logf("case %d", i) - m, err := parser.ParseMountRaw(c.bind, c.driver) - if c.fail { - if err == nil { - t.Errorf("Expected error, was nil, for spec %s\n", c.bind) - } - continue - } - - if m == nil || err != nil { - t.Errorf("ParseMountRaw failed for spec '%s', driver '%s', error '%v'", c.bind, c.driver, err) - continue - } - - if m.Destination != c.expDest { - t.Errorf("Expected destination '%s, was %s', for spec '%s'", c.expDest, m.Destination, c.bind) - } - - if m.Source != c.expSource { - t.Errorf("Expected source '%s', was '%s', for spec '%s'", c.expSource, m.Source, c.bind) - } - - if m.Name != c.expName { - t.Errorf("Expected name '%s', was '%s' for spec '%s'", c.expName, m.Name, c.bind) - } - - if m.Driver != c.expDriver { - t.Errorf("Expected driver '%s', was '%s', for spec '%s'", c.expDriver, m.Driver, c.bind) - } - - if m.RW != c.expRW { - t.Errorf("Expected RW '%v', was '%v' for spec '%s'", c.expRW, m.RW, c.bind) - } - if m.Type != c.expType { - t.Fatalf("Expected type '%s', was '%s', for spec '%s'", c.expType, m.Type, c.bind) - } - } - } - - tester(linParser, linuxCases) - tester(winParser, windowsCases) - tester(lcowParser, lcowCases) +func (m mockFiProviderWithError) fileInfo(path string) (bool, bool, error) { + return false, false, m.err } func TestParseMountSpec(t *testing.T) { - type c struct { - input mount.Mount - expected MountPoint - } testDir, err := ioutil.TempDir("", "test-mount-config") if err != nil { t.Fatal(err) } defer os.RemoveAll(testDir) parser := NewParser() - cases := []c{ + cases := []struct { + input mount.Mount + expected MountPoint + }{ {mount.Mount{Type: mount.TypeBind, Source: testDir, Target: testDestinationPath, ReadOnly: true}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath, Propagation: parser.DefaultPropagationMode()}}, {mount.Mount{Type: mount.TypeBind, Source: testDir, Target: testDestinationPath}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath, RW: true, Propagation: parser.DefaultPropagationMode()}}, {mount.Mount{Type: mount.TypeBind, Source: testDir + string(os.PathSeparator), Target: testDestinationPath, ReadOnly: true}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath, Propagation: parser.DefaultPropagationMode()}}, @@ -437,48 +90,3 @@ func TestParseMountSpec(t *testing.T) { } } - -// always returns the configured error -// this is used to test error handling -type mockFiProviderWithError struct{ err error } - -func (m mockFiProviderWithError) fileInfo(path string) (bool, bool, error) { - return false, false, m.err -} - -// TestParseMountSpecBindWithFileinfoError makes sure that the parser returns -// the error produced by the fileinfo provider. -// -// Some extra context for the future in case of changes and possible wtf are we -// testing this for: -// -// Currently this "fileInfoProvider" returns (bool, bool, error) -// The 1st bool is "does this path exist" -// The 2nd bool is "is this path a dir" -// Then of course the error is an error. -// -// The issue is the parser was ignoring the error and only looking at the -// "does this path exist" boolean, which is always false if there is an error. -// Then the error returned to the caller was a (slightly, maybe) friendlier -// error string than what comes from `os.Stat` -// So ...the caller was always getting an error saying the path doesn't exist -// even if it does exist but got some other error (like a permission error). -// This is confusing to users. -func TestParseMountSpecBindWithFileinfoError(t *testing.T) { - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - - testErr := errors.New("some crazy error") - currentFileInfoProvider = &mockFiProviderWithError{err: testErr} - - p := "/bananas" - if runtime.GOOS == "windows" { - p = `c:\bananas` - } - m := mount.Mount{Type: mount.TypeBind, Source: p, Target: p} - - parser := NewParser() - - _, err := parser.ParseMountSpec(m) - assert.ErrorContains(t, err, "some crazy error") -} diff --git a/volume/mounts/windows_parser_test.go b/volume/mounts/windows_parser_test.go new file mode 100644 index 0000000000..82fce8d0f9 --- /dev/null +++ b/volume/mounts/windows_parser_test.go @@ -0,0 +1,214 @@ +package mounts // import "github.com/docker/docker/volume/mounts" + +import ( + "fmt" + "strings" + "testing" + + "github.com/docker/docker/api/types/mount" + "gotest.tools/v3/assert" +) + +func TestWindowsParseMountRaw(t *testing.T) { + previousProvider := currentFileInfoProvider + defer func() { currentFileInfoProvider = previousProvider }() + currentFileInfoProvider = mockFiProvider{} + + valid := []string{ + `d:\`, + `d:`, + `d:\path`, + `d:\path with space`, + `c:\:d:\`, + `c:\windows\:d:`, + `c:\windows:d:\s p a c e`, + `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`, + `0123456789name:d:`, + `MiXeDcAsEnAmE:d:`, + `name:D:`, + `name:D::rW`, + `name:D::RW`, + `name:D::RO`, + `c:/:d:/forward/slashes/are/good/too`, + `c:/:d:/including with/spaces:ro`, + `c:\Windows`, // With capital + `c:\Program Files (x86)`, // With capitals and brackets + `\\?\c:\windows\:d:`, // Long path handling (source) + `c:\windows\:\\?\d:\`, // Long path handling (target) + `\\.\pipe\foo:\\.\pipe\foo`, // named pipe + `//./pipe/foo://./pipe/foo`, // named pipe forward slashes + } + + invalid := map[string]string{ + ``: "invalid volume specification: ", + `.`: "invalid volume specification: ", + `..\`: "invalid volume specification: ", + `c:\:..\`: "invalid volume specification: ", + `c:\:d:\:xyzzy`: "invalid volume specification: ", + `c:`: "cannot be `c:`", + `c:\`: "cannot be `c:`", + `c:\notexist:d:`: `source path does not exist: c:\notexist`, + `c:\windows\system32\ntdll.dll:d:`: `source path must be a directory`, + `name<:d:`: `invalid volume specification`, + `name>:d:`: `invalid volume specification`, + `name::d:`: `invalid volume specification`, + `name":d:`: `invalid volume specification`, + `name\:d:`: `invalid volume specification`, + `name*:d:`: `invalid volume specification`, + `name|:d:`: `invalid volume specification`, + `name?:d:`: `invalid volume specification`, + `name/:d:`: `invalid volume specification`, + `d:\pathandmode:rw`: `invalid volume specification`, + `d:\pathandmode:ro`: `invalid volume specification`, + `con:d:`: `cannot be a reserved word for Windows filenames`, + `PRN:d:`: `cannot be a reserved word for Windows filenames`, + `aUx:d:`: `cannot be a reserved word for Windows filenames`, + `nul:d:`: `cannot be a reserved word for Windows filenames`, + `com1:d:`: `cannot be a reserved word for Windows filenames`, + `com2:d:`: `cannot be a reserved word for Windows filenames`, + `com3:d:`: `cannot be a reserved word for Windows filenames`, + `com4:d:`: `cannot be a reserved word for Windows filenames`, + `com5:d:`: `cannot be a reserved word for Windows filenames`, + `com6:d:`: `cannot be a reserved word for Windows filenames`, + `com7:d:`: `cannot be a reserved word for Windows filenames`, + `com8:d:`: `cannot be a reserved word for Windows filenames`, + `com9:d:`: `cannot be a reserved word for Windows filenames`, + `lpt1:d:`: `cannot be a reserved word for Windows filenames`, + `lpt2:d:`: `cannot be a reserved word for Windows filenames`, + `lpt3:d:`: `cannot be a reserved word for Windows filenames`, + `lpt4:d:`: `cannot be a reserved word for Windows filenames`, + `lpt5:d:`: `cannot be a reserved word for Windows filenames`, + `lpt6:d:`: `cannot be a reserved word for Windows filenames`, + `lpt7:d:`: `cannot be a reserved word for Windows filenames`, + `lpt8:d:`: `cannot be a reserved word for Windows filenames`, + `lpt9:d:`: `cannot be a reserved word for Windows filenames`, + `c:\windows\system32\ntdll.dll`: `Only directories can be mapped on this platform`, + `\\.\pipe\foo:c:\pipe`: `'c:\pipe' is not a valid pipe path`, + } + + parser := &windowsParser{} + + for _, path := range valid { + if _, err := parser.ParseMountRaw(path, "local"); err != nil { + t.Errorf("ParseMountRaw(`%q`) should succeed: error %q", path, err) + } + } + + for path, expectedError := range invalid { + if mp, err := parser.ParseMountRaw(path, "local"); err == nil { + t.Errorf("ParseMountRaw(`%q`) should have failed validation. Err '%v' - MP: %v", path, err, mp) + } else { + if !strings.Contains(err.Error(), expectedError) { + t.Errorf("ParseMountRaw(`%q`) error should contain %q, got %v", path, expectedError, err.Error()) + } + } + } +} + +func TestWindowsParseMountRawSplit(t *testing.T) { + previousProvider := currentFileInfoProvider + defer func() { currentFileInfoProvider = previousProvider }() + currentFileInfoProvider = mockFiProvider{} + + cases := []struct { + bind string + driver string + expType mount.Type + expDest string + expSource string + expName string + expDriver string + expRW bool + fail bool + }{ + {`c:\:d:`, "local", mount.TypeBind, `d:`, `c:\`, ``, "", true, false}, + {`c:\:d:\`, "local", mount.TypeBind, `d:\`, `c:\`, ``, "", true, false}, + {`c:\:d:\:ro`, "local", mount.TypeBind, `d:\`, `c:\`, ``, "", false, false}, + {`c:\:d:\:rw`, "local", mount.TypeBind, `d:\`, `c:\`, ``, "", true, false}, + {`c:\:d:\:foo`, "local", mount.TypeBind, `d:\`, `c:\`, ``, "", false, true}, + {`name:d::rw`, "local", mount.TypeVolume, `d:`, ``, `name`, "local", true, false}, + {`name:d:`, "local", mount.TypeVolume, `d:`, ``, `name`, "local", true, false}, + {`name:d::ro`, "local", mount.TypeVolume, `d:`, ``, `name`, "local", false, false}, + {`name:c:`, "", mount.TypeVolume, ``, ``, ``, "", true, true}, + {`driver/name:c:`, "", mount.TypeVolume, ``, ``, ``, "", true, true}, + {`\\.\pipe\foo:\\.\pipe\bar`, "local", mount.TypeNamedPipe, `\\.\pipe\bar`, `\\.\pipe\foo`, "", "", true, false}, + {`\\.\pipe\foo:c:\foo\bar`, "local", mount.TypeNamedPipe, ``, ``, "", "", true, true}, + {`c:\foo\bar:\\.\pipe\foo`, "local", mount.TypeNamedPipe, ``, ``, "", "", true, true}, + } + + parser := &windowsParser{} + for i, c := range cases { + t.Logf("case %d", i) + m, err := parser.ParseMountRaw(c.bind, c.driver) + if c.fail { + if err == nil { + t.Errorf("Expected error, was nil, for spec %s\n", c.bind) + } + continue + } + + if m == nil || err != nil { + t.Errorf("ParseMountRaw failed for spec '%s', driver '%s', error '%v'", c.bind, c.driver, err) + continue + } + + if m.Destination != c.expDest { + t.Errorf("Expected destination '%s, was %s', for spec '%s'", c.expDest, m.Destination, c.bind) + } + + if m.Source != c.expSource { + t.Errorf("Expected source '%s', was '%s', for spec '%s'", c.expSource, m.Source, c.bind) + } + + if m.Name != c.expName { + t.Errorf("Expected name '%s', was '%s' for spec '%s'", c.expName, m.Name, c.bind) + } + + if m.Driver != c.expDriver { + t.Errorf("Expected driver '%s', was '%s', for spec '%s'", c.expDriver, m.Driver, c.bind) + } + + if m.RW != c.expRW { + t.Errorf("Expected RW '%v', was '%v' for spec '%s'", c.expRW, m.RW, c.bind) + } + if m.Type != c.expType { + t.Fatalf("Expected type '%s', was '%s', for spec '%s'", c.expType, m.Type, c.bind) + } + } +} + +// TestWindowsParseMountSpecBindWithFileinfoError makes sure that the parser returns +// the error produced by the fileinfo provider. +// +// Some extra context for the future in case of changes and possible wtf are we +// testing this for: +// +// Currently this "fileInfoProvider" returns (bool, bool, error) +// The 1st bool is "does this path exist" +// The 2nd bool is "is this path a dir" +// Then of course the error is an error. +// +// The issue is the parser was ignoring the error and only looking at the +// "does this path exist" boolean, which is always false if there is an error. +// Then the error returned to the caller was a (slightly, maybe) friendlier +// error string than what comes from `os.Stat` +// So ...the caller was always getting an error saying the path doesn't exist +// even if it does exist but got some other error (like a permission error). +// This is confusing to users. +func TestWindowsParseMountSpecBindWithFileinfoError(t *testing.T) { + previousProvider := currentFileInfoProvider + defer func() { currentFileInfoProvider = previousProvider }() + + testErr := fmt.Errorf("some crazy error") + currentFileInfoProvider = &mockFiProviderWithError{err: testErr} + + parser := &windowsParser{} + + _, err := parser.ParseMountSpec(mount.Mount{ + Type: mount.TypeBind, + Source: `c:\bananas`, + Target: `c:\bananas`, + }) + assert.ErrorContains(t, err, testErr.Error()) +} From 28b0f47599e9ff32d2abebb54ee4b2a60977f722 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 2 Jul 2021 13:41:47 +0200 Subject: [PATCH 5/7] volume/mounts: add constructors for each parser This adds constructors for the Linux, Windows, and LCOW, to allow using these parsers externally. Signed-off-by: Sebastiaan van Stijn --- volume/mounts/lcow_parser.go | 5 +++++ volume/mounts/lcow_parser_test.go | 4 ++-- volume/mounts/linux_parser.go | 5 +++++ volume/mounts/linux_parser_test.go | 8 ++++---- volume/mounts/parser.go | 4 ++-- volume/mounts/validate_test.go | 2 +- volume/mounts/windows_parser.go | 5 +++++ volume/mounts/windows_parser_test.go | 6 +++--- 8 files changed, 27 insertions(+), 12 deletions(-) diff --git a/volume/mounts/lcow_parser.go b/volume/mounts/lcow_parser.go index 645a731d93..150285f98a 100644 --- a/volume/mounts/lcow_parser.go +++ b/volume/mounts/lcow_parser.go @@ -7,6 +7,11 @@ import ( "github.com/docker/docker/api/types/mount" ) +// NewLCOWParser creates a parser with Linux Containers on Windows semantics. +func NewLCOWParser() Parser { + return &lcowParser{} +} + // rxLCOWDestination is the regex expression for the mount destination for LCOW // // Destination (aka container path): diff --git a/volume/mounts/lcow_parser_test.go b/volume/mounts/lcow_parser_test.go index 8e5e2261df..800e1f1d75 100644 --- a/volume/mounts/lcow_parser_test.go +++ b/volume/mounts/lcow_parser_test.go @@ -80,7 +80,7 @@ func TestLCOWParseMountRaw(t *testing.T) { `\\.\pipe\foo:/foo`: `Linux containers on Windows do not support named pipe mounts`, } - parser := &lcowParser{} + parser := NewLCOWParser() for _, path := range valid { if _, err := parser.ParseMountRaw(path, "local"); err != nil { @@ -129,7 +129,7 @@ func TestLCOWParseMountRawSplit(t *testing.T) { {`c:\foo\bar:\\.\pipe\foo`, "local", mount.TypeNamedPipe, ``, ``, "", "", true, true}, } - parser := &lcowParser{} + parser := NewLCOWParser() for i, c := range cases { t.Logf("case %d", i) m, err := parser.ParseMountRaw(c.bind, c.driver) diff --git a/volume/mounts/linux_parser.go b/volume/mounts/linux_parser.go index 8dc9afd34c..7dc6a1dd0a 100644 --- a/volume/mounts/linux_parser.go +++ b/volume/mounts/linux_parser.go @@ -12,6 +12,11 @@ import ( "github.com/docker/docker/volume" ) +// NewLinuxParser creates a parser with Linux semantics. +func NewLinuxParser() Parser { + return &linuxParser{} +} + type linuxParser struct{} func linuxSplitRawSpec(raw string) ([]string, error) { diff --git a/volume/mounts/linux_parser_test.go b/volume/mounts/linux_parser_test.go index 00c8cff47a..9b09af3a92 100644 --- a/volume/mounts/linux_parser_test.go +++ b/volume/mounts/linux_parser_test.go @@ -79,7 +79,7 @@ func TestLinuxParseMountRaw(t *testing.T) { "name:/absolute-path:rprivate": "invalid volume specification", } - parser := &linuxParser{} + parser := NewLinuxParser() for _, path := range valid { if _, err := parser.ParseMountRaw(path, "local"); err != nil { @@ -125,7 +125,7 @@ func TestLinuxParseMountRawSplit(t *testing.T) { {"/tmp:tmp", "", mount.TypeBind, "", "", "", "", true, true}, } - parser := &linuxParser{} + parser := NewLinuxParser() for i, c := range cases { t.Logf("case %d", i) m, err := parser.ParseMountRaw(c.bind, c.driver) @@ -191,7 +191,7 @@ func TestLinuxParseMountSpecBindWithFileinfoError(t *testing.T) { testErr := fmt.Errorf("some crazy error") currentFileInfoProvider = &mockFiProviderWithError{err: testErr} - parser := &linuxParser{} + parser := NewLinuxParser() _, err := parser.ParseMountSpec(mount.Mount{ Type: mount.TypeBind, @@ -222,7 +222,7 @@ func TestConvertTmpfsOptions(t *testing.T) { unexpectedSubstrings: []string{}, }, } - p := &linuxParser{} + p := NewLinuxParser() for _, c := range cases { data, err := p.ConvertTmpfsOptions(&c.opt, c.readOnly) if err != nil { diff --git a/volume/mounts/parser.go b/volume/mounts/parser.go index 07ac7fd3df..58107f490c 100644 --- a/volume/mounts/parser.go +++ b/volume/mounts/parser.go @@ -36,7 +36,7 @@ type Parser interface { // NewParser creates a parser for the current host OS func NewParser() Parser { if runtime.GOOS == "windows" { - return &windowsParser{} + return NewWindowsParser() } - return &linuxParser{} + return NewLinuxParser() } diff --git a/volume/mounts/validate_test.go b/volume/mounts/validate_test.go index e1c1796cd2..ff9ff57595 100644 --- a/volume/mounts/validate_test.go +++ b/volume/mounts/validate_test.go @@ -59,7 +59,7 @@ func TestValidateMount(t *testing.T) { } } if runtime.GOOS == "windows" { - parser = &lcowParser{} + parser = NewLCOWParser() for i, x := range lcowCases { err := parser.ValidateMountConfig(&x.input) if err == nil && x.expected == nil { diff --git a/volume/mounts/windows_parser.go b/volume/mounts/windows_parser.go index e0f8f5a7d1..41254ad1f9 100644 --- a/volume/mounts/windows_parser.go +++ b/volume/mounts/windows_parser.go @@ -12,6 +12,11 @@ import ( "github.com/docker/docker/pkg/stringid" ) +// NewWindowsParser creates a parser with Windows semantics. +func NewWindowsParser() Parser { + return &windowsParser{} +} + type windowsParser struct{} const ( diff --git a/volume/mounts/windows_parser_test.go b/volume/mounts/windows_parser_test.go index 82fce8d0f9..bf62e3b59d 100644 --- a/volume/mounts/windows_parser_test.go +++ b/volume/mounts/windows_parser_test.go @@ -87,7 +87,7 @@ func TestWindowsParseMountRaw(t *testing.T) { `\\.\pipe\foo:c:\pipe`: `'c:\pipe' is not a valid pipe path`, } - parser := &windowsParser{} + parser := NewWindowsParser() for _, path := range valid { if _, err := parser.ParseMountRaw(path, "local"); err != nil { @@ -137,7 +137,7 @@ func TestWindowsParseMountRawSplit(t *testing.T) { {`c:\foo\bar:\\.\pipe\foo`, "local", mount.TypeNamedPipe, ``, ``, "", "", true, true}, } - parser := &windowsParser{} + parser := NewWindowsParser() for i, c := range cases { t.Logf("case %d", i) m, err := parser.ParseMountRaw(c.bind, c.driver) @@ -203,7 +203,7 @@ func TestWindowsParseMountSpecBindWithFileinfoError(t *testing.T) { testErr := fmt.Errorf("some crazy error") currentFileInfoProvider = &mockFiProviderWithError{err: testErr} - parser := &windowsParser{} + parser := NewWindowsParser() _, err := parser.ParseMountSpec(mount.Mount{ Type: mount.TypeBind, From 73378d20423ba5566295eca6b58eb07aa1f4166f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 29 Jun 2021 13:19:02 +0200 Subject: [PATCH 6/7] volume/mounts: don't use global variable for fileinfoprovider This allows stubbing the provider for a test without side effects for other tests, making it no longer needed to reset it to its original value in a defer() Signed-off-by: Sebastiaan van Stijn --- volume/mounts/lcow_parser.go | 6 +++++- volume/mounts/lcow_parser_test.go | 15 +++++++-------- volume/mounts/linux_parser.go | 10 +++++++--- volume/mounts/linux_parser_test.go | 25 +++++++++++-------------- volume/mounts/windows_parser.go | 19 ++++++++++--------- volume/mounts/windows_parser_test.go | 25 +++++++++++-------------- 6 files changed, 51 insertions(+), 49 deletions(-) diff --git a/volume/mounts/lcow_parser.go b/volume/mounts/lcow_parser.go index 150285f98a..b9e2208249 100644 --- a/volume/mounts/lcow_parser.go +++ b/volume/mounts/lcow_parser.go @@ -9,7 +9,11 @@ import ( // NewLCOWParser creates a parser with Linux Containers on Windows semantics. func NewLCOWParser() Parser { - return &lcowParser{} + return &lcowParser{ + windowsParser{ + fi: defaultFileInfoProvider{}, + }, + } } // rxLCOWDestination is the regex expression for the mount destination for LCOW diff --git a/volume/mounts/lcow_parser_test.go b/volume/mounts/lcow_parser_test.go index 800e1f1d75..097531e862 100644 --- a/volume/mounts/lcow_parser_test.go +++ b/volume/mounts/lcow_parser_test.go @@ -8,10 +8,6 @@ import ( ) func TestLCOWParseMountRaw(t *testing.T) { - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - currentFileInfoProvider = mockFiProvider{} - valid := []string{ `/foo`, `/foo/`, @@ -81,6 +77,9 @@ func TestLCOWParseMountRaw(t *testing.T) { } parser := NewLCOWParser() + if p, ok := parser.(*lcowParser); ok { + p.fi = mockFiProvider{} + } for _, path := range valid { if _, err := parser.ParseMountRaw(path, "local"); err != nil { @@ -100,10 +99,6 @@ func TestLCOWParseMountRaw(t *testing.T) { } func TestLCOWParseMountRawSplit(t *testing.T) { - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - currentFileInfoProvider = mockFiProvider{} - cases := []struct { bind string driver string @@ -130,6 +125,10 @@ func TestLCOWParseMountRawSplit(t *testing.T) { } parser := NewLCOWParser() + if p, ok := parser.(*lcowParser); ok { + p.fi = mockFiProvider{} + } + for i, c := range cases { t.Logf("case %d", i) m, err := parser.ParseMountRaw(c.bind, c.driver) diff --git a/volume/mounts/linux_parser.go b/volume/mounts/linux_parser.go index 7dc6a1dd0a..e9d7a9c5de 100644 --- a/volume/mounts/linux_parser.go +++ b/volume/mounts/linux_parser.go @@ -14,10 +14,14 @@ import ( // NewLinuxParser creates a parser with Linux semantics. func NewLinuxParser() Parser { - return &linuxParser{} + return &linuxParser{ + fi: defaultFileInfoProvider{}, + } } -type linuxParser struct{} +type linuxParser struct { + fi fileInfoProvider +} func linuxSplitRawSpec(raw string) ([]string, error) { if strings.Count(raw, ":") > 2 { @@ -86,7 +90,7 @@ func (p *linuxParser) validateMountConfigImpl(mnt *mount.Mount, validateBindSour } if validateBindSourceExists { - exists, _, err := currentFileInfoProvider.fileInfo(mnt.Source) + exists, _, err := p.fi.fileInfo(mnt.Source) if err != nil { return &errMountConfig{mnt, err} } diff --git a/volume/mounts/linux_parser_test.go b/volume/mounts/linux_parser_test.go index 9b09af3a92..895eaef48f 100644 --- a/volume/mounts/linux_parser_test.go +++ b/volume/mounts/linux_parser_test.go @@ -10,10 +10,6 @@ import ( ) func TestLinuxParseMountRaw(t *testing.T) { - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - currentFileInfoProvider = mockFiProvider{} - valid := []string{ "/home", "/home:/home", @@ -80,6 +76,9 @@ func TestLinuxParseMountRaw(t *testing.T) { } parser := NewLinuxParser() + if p, ok := parser.(*linuxParser); ok { + p.fi = mockFiProvider{} + } for _, path := range valid { if _, err := parser.ParseMountRaw(path, "local"); err != nil { @@ -99,10 +98,6 @@ func TestLinuxParseMountRaw(t *testing.T) { } func TestLinuxParseMountRawSplit(t *testing.T) { - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - currentFileInfoProvider = mockFiProvider{} - cases := []struct { bind string driver string @@ -126,6 +121,10 @@ func TestLinuxParseMountRawSplit(t *testing.T) { } parser := NewLinuxParser() + if p, ok := parser.(*linuxParser); ok { + p.fi = mockFiProvider{} + } + for i, c := range cases { t.Logf("case %d", i) m, err := parser.ParseMountRaw(c.bind, c.driver) @@ -185,13 +184,11 @@ func TestLinuxParseMountRawSplit(t *testing.T) { // even if it does exist but got some other error (like a permission error). // This is confusing to users. func TestLinuxParseMountSpecBindWithFileinfoError(t *testing.T) { - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - - testErr := fmt.Errorf("some crazy error") - currentFileInfoProvider = &mockFiProviderWithError{err: testErr} - parser := NewLinuxParser() + testErr := fmt.Errorf("some crazy error") + if pr, ok := parser.(*linuxParser); ok { + pr.fi = &mockFiProviderWithError{err: testErr} + } _, err := parser.ParseMountSpec(mount.Mount{ Type: mount.TypeBind, diff --git a/volume/mounts/windows_parser.go b/volume/mounts/windows_parser.go index 41254ad1f9..af06dc12b9 100644 --- a/volume/mounts/windows_parser.go +++ b/volume/mounts/windows_parser.go @@ -14,10 +14,14 @@ import ( // NewWindowsParser creates a parser with Windows semantics. func NewWindowsParser() Parser { - return &windowsParser{} + return &windowsParser{ + fi: defaultFileInfoProvider{}, + } } -type windowsParser struct{} +type windowsParser struct { + fi fileInfoProvider +} const ( // Spec should be in the format [source:]destination[:mode] @@ -76,7 +80,7 @@ const ( type mountValidator func(mnt *mount.Mount) error -func windowsSplitRawSpec(raw, destRegex string) ([]string, error) { +func (p *windowsParser) windowsSplitRawSpec(raw, destRegex string) ([]string, error) { specExp := regexp.MustCompile(`^` + rxSource + destRegex + rxMode + `$`) match := specExp.FindStringSubmatch(strings.ToLower(raw)) @@ -119,8 +123,7 @@ func windowsSplitRawSpec(raw, destRegex string) ([]string, error) { return nil, fmt.Errorf("volume name %q cannot be a reserved word for Windows filenames", matchgroups["destination"]) } } else { - - exists, isDir, _ := currentFileInfoProvider.fileInfo(matchgroups["destination"]) + exists, isDir, _ := p.fi.fileInfo(matchgroups["destination"]) if exists && !isDir { return nil, fmt.Errorf("file '%s' cannot be mapped. Only directories can be mapped on this platform", matchgroups["destination"]) @@ -211,8 +214,6 @@ func (defaultFileInfoProvider) fileInfo(path string) (exist, isDir bool, err err return true, fi.IsDir(), nil } -var currentFileInfoProvider fileInfoProvider = defaultFileInfoProvider{} - func (p *windowsParser) validateMountConfigReg(mnt *mount.Mount, destRegex string, additionalValidators ...mountValidator) error { for _, v := range additionalValidators { @@ -247,7 +248,7 @@ func (p *windowsParser) validateMountConfigReg(mnt *mount.Mount, destRegex strin return &errMountConfig{mnt, err} } - exists, isdir, err := currentFileInfoProvider.fileInfo(mnt.Source) + exists, isdir, err := p.fi.fileInfo(mnt.Source) if err != nil { return &errMountConfig{mnt, err} } @@ -302,7 +303,7 @@ func (p *windowsParser) ParseMountRaw(raw, volumeDriver string) (*MountPoint, er } func (p *windowsParser) parseMountRaw(raw, volumeDriver, destRegex string, convertTargetToBackslash bool, additionalValidators ...mountValidator) (*MountPoint, error) { - arr, err := windowsSplitRawSpec(raw, destRegex) + arr, err := p.windowsSplitRawSpec(raw, destRegex) if err != nil { return nil, err } diff --git a/volume/mounts/windows_parser_test.go b/volume/mounts/windows_parser_test.go index bf62e3b59d..f8ff763307 100644 --- a/volume/mounts/windows_parser_test.go +++ b/volume/mounts/windows_parser_test.go @@ -10,10 +10,6 @@ import ( ) func TestWindowsParseMountRaw(t *testing.T) { - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - currentFileInfoProvider = mockFiProvider{} - valid := []string{ `d:\`, `d:`, @@ -88,6 +84,9 @@ func TestWindowsParseMountRaw(t *testing.T) { } parser := NewWindowsParser() + if p, ok := parser.(*windowsParser); ok { + p.fi = mockFiProvider{} + } for _, path := range valid { if _, err := parser.ParseMountRaw(path, "local"); err != nil { @@ -107,10 +106,6 @@ func TestWindowsParseMountRaw(t *testing.T) { } func TestWindowsParseMountRawSplit(t *testing.T) { - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - currentFileInfoProvider = mockFiProvider{} - cases := []struct { bind string driver string @@ -138,6 +133,10 @@ func TestWindowsParseMountRawSplit(t *testing.T) { } parser := NewWindowsParser() + if p, ok := parser.(*windowsParser); ok { + p.fi = mockFiProvider{} + } + for i, c := range cases { t.Logf("case %d", i) m, err := parser.ParseMountRaw(c.bind, c.driver) @@ -197,13 +196,11 @@ func TestWindowsParseMountRawSplit(t *testing.T) { // even if it does exist but got some other error (like a permission error). // This is confusing to users. func TestWindowsParseMountSpecBindWithFileinfoError(t *testing.T) { - previousProvider := currentFileInfoProvider - defer func() { currentFileInfoProvider = previousProvider }() - - testErr := fmt.Errorf("some crazy error") - currentFileInfoProvider = &mockFiProviderWithError{err: testErr} - parser := NewWindowsParser() + testErr := fmt.Errorf("some crazy error") + if pr, ok := parser.(*windowsParser); ok { + pr.fi = &mockFiProviderWithError{err: testErr} + } _, err := parser.ParseMountSpec(mount.Mount{ Type: mount.TypeBind, From 8e3f9fd032ddf9c42844a05c5a5ea7c6c002f6f2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 29 Jun 2021 14:04:41 +0200 Subject: [PATCH 7/7] volume/mounts: use sub-tests, and use gotest.tools Signed-off-by: Sebastiaan van Stijn --- volume/mounts/lcow_parser_test.go | 50 +++++++++------------------- volume/mounts/linux_parser_test.go | 48 ++++++++------------------ volume/mounts/windows_parser_test.go | 48 ++++++++------------------ 3 files changed, 44 insertions(+), 102 deletions(-) diff --git a/volume/mounts/lcow_parser_test.go b/volume/mounts/lcow_parser_test.go index 097531e862..c62309b143 100644 --- a/volume/mounts/lcow_parser_test.go +++ b/volume/mounts/lcow_parser_test.go @@ -1,10 +1,12 @@ package mounts // import "github.com/docker/docker/volume/mounts" import ( + "fmt" "strings" "testing" "github.com/docker/docker/api/types/mount" + "gotest.tools/v3/assert" ) func TestLCOWParseMountRaw(t *testing.T) { @@ -130,41 +132,21 @@ func TestLCOWParseMountRawSplit(t *testing.T) { } for i, c := range cases { - t.Logf("case %d", i) - m, err := parser.ParseMountRaw(c.bind, c.driver) - if c.fail { - if err == nil { - t.Errorf("Expected error, was nil, for spec %s\n", c.bind) + c := c + t.Run(fmt.Sprintf("%d_%s", i, c.bind), func(t *testing.T) { + m, err := parser.ParseMountRaw(c.bind, c.driver) + if c.fail { + assert.ErrorContains(t, err, "", "expected an error") + return } - continue - } - if m == nil || err != nil { - t.Errorf("ParseMountRaw failed for spec '%s', driver '%s', error '%v'", c.bind, c.driver, err) - continue - } - - if m.Destination != c.expDest { - t.Errorf("Expected destination '%s, was %s', for spec '%s'", c.expDest, m.Destination, c.bind) - } - - if m.Source != c.expSource { - t.Errorf("Expected source '%s', was '%s', for spec '%s'", c.expSource, m.Source, c.bind) - } - - if m.Name != c.expName { - t.Errorf("Expected name '%s', was '%s' for spec '%s'", c.expName, m.Name, c.bind) - } - - if m.Driver != c.expDriver { - t.Errorf("Expected driver '%s', was '%s', for spec '%s'", c.expDriver, m.Driver, c.bind) - } - - if m.RW != c.expRW { - t.Errorf("Expected RW '%v', was '%v' for spec '%s'", c.expRW, m.RW, c.bind) - } - if m.Type != c.expType { - t.Fatalf("Expected type '%s', was '%s', for spec '%s'", c.expType, m.Type, c.bind) - } + assert.NilError(t, err) + assert.Equal(t, m.Destination, c.expDest) + assert.Equal(t, m.Source, c.expSource) + assert.Equal(t, m.Name, c.expName) + assert.Equal(t, m.Driver, c.expDriver) + assert.Equal(t, m.RW, c.expRW) + assert.Equal(t, m.Type, c.expType) + }) } } diff --git a/volume/mounts/linux_parser_test.go b/volume/mounts/linux_parser_test.go index 895eaef48f..bcfca72b25 100644 --- a/volume/mounts/linux_parser_test.go +++ b/volume/mounts/linux_parser_test.go @@ -126,42 +126,22 @@ func TestLinuxParseMountRawSplit(t *testing.T) { } for i, c := range cases { - t.Logf("case %d", i) - m, err := parser.ParseMountRaw(c.bind, c.driver) - if c.fail { - if err == nil { - t.Errorf("Expected error, was nil, for spec %s\n", c.bind) + c := c + t.Run(fmt.Sprintf("%d_%s", i, c.bind), func(t *testing.T) { + m, err := parser.ParseMountRaw(c.bind, c.driver) + if c.fail { + assert.ErrorContains(t, err, "", "expected an error") + return } - continue - } - if m == nil || err != nil { - t.Errorf("ParseMountRaw failed for spec '%s', driver '%s', error '%v'", c.bind, c.driver, err) - continue - } - - if m.Destination != c.expDest { - t.Errorf("Expected destination '%s, was %s', for spec '%s'", c.expDest, m.Destination, c.bind) - } - - if m.Source != c.expSource { - t.Errorf("Expected source '%s', was '%s', for spec '%s'", c.expSource, m.Source, c.bind) - } - - if m.Name != c.expName { - t.Errorf("Expected name '%s', was '%s' for spec '%s'", c.expName, m.Name, c.bind) - } - - if m.Driver != c.expDriver { - t.Errorf("Expected driver '%s', was '%s', for spec '%s'", c.expDriver, m.Driver, c.bind) - } - - if m.RW != c.expRW { - t.Errorf("Expected RW '%v', was '%v' for spec '%s'", c.expRW, m.RW, c.bind) - } - if m.Type != c.expType { - t.Fatalf("Expected type '%s', was '%s', for spec '%s'", c.expType, m.Type, c.bind) - } + assert.NilError(t, err) + assert.Equal(t, m.Destination, c.expDest) + assert.Equal(t, m.Source, c.expSource) + assert.Equal(t, m.Name, c.expName) + assert.Equal(t, m.Driver, c.expDriver) + assert.Equal(t, m.RW, c.expRW) + assert.Equal(t, m.Type, c.expType) + }) } } diff --git a/volume/mounts/windows_parser_test.go b/volume/mounts/windows_parser_test.go index f8ff763307..c968ce946b 100644 --- a/volume/mounts/windows_parser_test.go +++ b/volume/mounts/windows_parser_test.go @@ -138,42 +138,22 @@ func TestWindowsParseMountRawSplit(t *testing.T) { } for i, c := range cases { - t.Logf("case %d", i) - m, err := parser.ParseMountRaw(c.bind, c.driver) - if c.fail { - if err == nil { - t.Errorf("Expected error, was nil, for spec %s\n", c.bind) + c := c + t.Run(fmt.Sprintf("%d_%s", i, c.bind), func(t *testing.T) { + m, err := parser.ParseMountRaw(c.bind, c.driver) + if c.fail { + assert.ErrorContains(t, err, "", "expected an error") + return } - continue - } - if m == nil || err != nil { - t.Errorf("ParseMountRaw failed for spec '%s', driver '%s', error '%v'", c.bind, c.driver, err) - continue - } - - if m.Destination != c.expDest { - t.Errorf("Expected destination '%s, was %s', for spec '%s'", c.expDest, m.Destination, c.bind) - } - - if m.Source != c.expSource { - t.Errorf("Expected source '%s', was '%s', for spec '%s'", c.expSource, m.Source, c.bind) - } - - if m.Name != c.expName { - t.Errorf("Expected name '%s', was '%s' for spec '%s'", c.expName, m.Name, c.bind) - } - - if m.Driver != c.expDriver { - t.Errorf("Expected driver '%s', was '%s', for spec '%s'", c.expDriver, m.Driver, c.bind) - } - - if m.RW != c.expRW { - t.Errorf("Expected RW '%v', was '%v' for spec '%s'", c.expRW, m.RW, c.bind) - } - if m.Type != c.expType { - t.Fatalf("Expected type '%s', was '%s', for spec '%s'", c.expType, m.Type, c.bind) - } + assert.NilError(t, err) + assert.Equal(t, m.Destination, c.expDest) + assert.Equal(t, m.Source, c.expSource) + assert.Equal(t, m.Name, c.expName) + assert.Equal(t, m.Driver, c.expDriver) + assert.Equal(t, m.RW, c.expRW) + assert.Equal(t, m.Type, c.expType) + }) } }