From 300c11c7c9143ca077890050deb83917e67fe250 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 11 Jun 2021 21:01:18 +0200 Subject: [PATCH] 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