From 9543e740af72d1b94dc1a907d6de3c673c7aec52 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 8 Aug 2024 09:08:01 -0400 Subject: [PATCH] docker: fix delimiter for selinux label for read-only volumes (#23750) The Docker driver's `volume` field to specify bind-mounts takes a list of strings that consist of three `:`-delimited fields: source, destination, and options. We append the SELinux label from the plugin configuration as the third field. But when the user has already specified the volume is read-only with `:ro`, we're incorrectly appending the SELinux label with another `:` instead of the required `,`. Combine the options into a single field value before appending them to the bind mounts configuration. Updated the tests to split out Windows behavior (which doesn't accept options) and to ensure the test task has the expected environment for bind mounts. Fixes: https://github.com/hashicorp/nomad/issues/23690 --- .changelog/23750.txt | 3 + client/testutil/driver_compatible.go | 10 ++- drivers/docker/driver.go | 34 +++++--- drivers/docker/driver_test.go | 122 +++++++++++++++++++++++---- 4 files changed, 137 insertions(+), 32 deletions(-) create mode 100644 .changelog/23750.txt diff --git a/.changelog/23750.txt b/.changelog/23750.txt new file mode 100644 index 000000000..ba66df023 --- /dev/null +++ b/.changelog/23750.txt @@ -0,0 +1,3 @@ +```release-note:bug +docker: Fixed a bug where plugin SELinux labels would conflict with read-only `volume` options +``` diff --git a/client/testutil/driver_compatible.go b/client/testutil/driver_compatible.go index 441743999..f96aa877a 100644 --- a/client/testutil/driver_compatible.go +++ b/client/testutil/driver_compatible.go @@ -63,13 +63,21 @@ func RequireLinux(t *testing.T) { } // RequireNotWindows skips tests whenever: -// - running on Window +// - running on Windows func RequireNotWindows(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Test requires non-Windows") } } +// RequireWindows skips tests whenever: +// - not running on Windows +func RequireWindows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("Test requires non-Windows") + } +} + // ExecCompatible skips tests unless: // - running as root // - running on Linux diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 534ee1b62..a3cc8248f 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -745,15 +745,22 @@ func (d *Driver) convertAllocPathsForWindowsLCOW(task *drivers.TaskConfig, image } func (d *Driver) containerBinds(task *drivers.TaskConfig, driverConfig *TaskConfig) ([]string, error) { + taskLocalBindVolume := driverConfig.VolumeDriver == "" + if !d.config.Volumes.Enabled && !taskLocalBindVolume { + return nil, fmt.Errorf("volumes are not enabled; cannot use volume driver %q", driverConfig.VolumeDriver) + } + allocDirBind := fmt.Sprintf("%s:%s", task.TaskDir().SharedAllocDir, task.Env[taskenv.AllocDir]) taskLocalBind := fmt.Sprintf("%s:%s", task.TaskDir().LocalDir, task.Env[taskenv.TaskLocalDir]) secretDirBind := fmt.Sprintf("%s:%s", task.TaskDir().SecretsDir, task.Env[taskenv.SecretsDir]) binds := []string{allocDirBind, taskLocalBind, secretDirBind} - taskLocalBindVolume := driverConfig.VolumeDriver == "" - - if !d.config.Volumes.Enabled && !taskLocalBindVolume { - return nil, fmt.Errorf("volumes are not enabled; cannot use volume driver %q", driverConfig.VolumeDriver) + selinuxLabel := d.config.Volumes.SelinuxLabel + if selinuxLabel != "" { + // Apply SELinux Label to each built-in bind + for i := range binds { + binds[i] = fmt.Sprintf("%s:%s", binds[i], selinuxLabel) + } } for _, userbind := range driverConfig.Volumes { @@ -782,19 +789,20 @@ func (d *Driver) containerBinds(task *drivers.TaskConfig, driverConfig *TaskConf } bind := src + ":" + dst - if mode != "" { - bind += ":" + mode + opts := mode + if opts != "" { + if selinuxLabel != "" { + opts += "," + selinuxLabel + } + } else { + opts = selinuxLabel + } + if opts != "" { + bind += ":" + opts } binds = append(binds, bind) } - if selinuxLabel := d.config.Volumes.SelinuxLabel; selinuxLabel != "" { - // Apply SELinux Label to each volume - for i := range binds { - binds[i] = fmt.Sprintf("%s:%s", binds[i], selinuxLabel) - } - } - return binds, nil } diff --git a/drivers/docker/driver_test.go b/drivers/docker/driver_test.go index 6a4868697..e00d43223 100644 --- a/drivers/docker/driver_test.go +++ b/drivers/docker/driver_test.go @@ -12,7 +12,6 @@ import ( "reflect" "runtime" "runtime/debug" - "sort" "strings" "syscall" "testing" @@ -92,7 +91,10 @@ func dockerTask(t *testing.T) (*drivers.TaskConfig, *TaskConfig, []int) { Name: "redis-demo", AllocID: uuid.Generate(), Env: map[string]string{ - "test": t.Name(), + "test": t.Name(), + "NOMAD_ALLOC_DIR": "/alloc", + "NOMAD_TASK_DIR": "/local", + "NOMAD_SECRETS_DIR": "/secrets", }, DeviceEnv: make(map[string]string), Resources: &drivers.Resources{ @@ -119,6 +121,12 @@ func dockerTask(t *testing.T) (*drivers.TaskConfig, *TaskConfig, []int) { }, } + if runtime.GOOS == "windows" { + task.Env["NOMAD_ALLOC_DIR"] = "c:/alloc" + task.Env["NOMAD_TASK_DIR"] = "c:/local" + task.Env["NOMAD_SECRETS_DIR"] = "c:/secrets" + } + require.NoError(t, task.EncodeConcreteDriverConfig(&cfg)) return task, &cfg, ports @@ -1285,6 +1293,7 @@ func TestDockerDriver_CreateContainerConfig_Logging(t *testing.T) { func TestDockerDriver_CreateContainerConfig_Mounts(t *testing.T) { ci.Parallel(t) + testutil.RequireLinux(t) task, cfg, _ := dockerTask(t) @@ -1310,17 +1319,26 @@ func TestDockerDriver_CreateContainerConfig_Mounts(t *testing.T) { Target: "/list-tmpfs-target", }, } - - expectedSrcPrefix := "/" - if runtime.GOOS == "windows" { - expectedSrcPrefix = "redis-demo\\" + cfg.Volumes = []string{ + "/etc/ssl/certs:/etc/ssl/certs:ro", + "/var/www:/srv/www", } - expected := []docker.HostMount{ + + must.NoError(t, task.EncodeConcreteDriverConfig(cfg)) + dh := dockerDriverHarness(t, nil) + driver := dh.Impl().(*Driver) + driver.config.Volumes.Enabled = true + driver.config.Volumes.SelinuxLabel = "z" + + cc, err := driver.createContainerConfig(task, cfg, "org/repo:0.1") + must.NoError(t, err) + + must.Eq(t, []docker.HostMount{ // from mount map { Type: "bind", Target: "/map-bind-target", - Source: expectedSrcPrefix + "map-source", + Source: "/map-source", BindOptions: &docker.BindOptions{}, }, { @@ -1332,7 +1350,7 @@ func TestDockerDriver_CreateContainerConfig_Mounts(t *testing.T) { { Type: "bind", Target: "/list-bind-target", - Source: expectedSrcPrefix + "list-source", + Source: "/list-source", BindOptions: &docker.BindOptions{}, }, { @@ -1340,24 +1358,92 @@ func TestDockerDriver_CreateContainerConfig_Mounts(t *testing.T) { Target: "/list-tmpfs-target", TempfsOptions: &docker.TempfsOptions{}, }, + }, cc.HostConfig.Mounts) + + must.Eq(t, []string{ + "alloc:/alloc:z", + "redis-demo/local:/local:z", + "redis-demo/secrets:/secrets:z", + "/etc/ssl/certs:/etc/ssl/certs:ro,z", + "/var/www:/srv/www:z", + }, cc.HostConfig.Binds) +} + +func TestDockerDriver_CreateContainerConfig_Mounts_Windows(t *testing.T) { + ci.Parallel(t) + testutil.RequireWindows(t) + + task, cfg, _ := dockerTask(t) + + cfg.Mounts = []DockerMount{ + { + Type: "bind", + Target: "/map-bind-target", + Source: "/map-source", + }, + { + Type: "tmpfs", + Target: "/map-tmpfs-target", + }, + } + cfg.MountsList = []DockerMount{ + { + Type: "bind", + Target: "/list-bind-target", + Source: "/list-source", + }, + { + Type: "tmpfs", + Target: "/list-tmpfs-target", + }, + } + cfg.Volumes = []string{ + "c:/etc/ssl/certs:c:/etc/ssl/certs", + "c:/var/www:c:/srv/www", } - require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) - + must.NoError(t, task.EncodeConcreteDriverConfig(cfg)) dh := dockerDriverHarness(t, nil) driver := dh.Impl().(*Driver) driver.config.Volumes.Enabled = true cc, err := driver.createContainerConfig(task, cfg, "org/repo:0.1") - require.NoError(t, err) + must.NoError(t, err) - found := cc.HostConfig.Mounts - sort.Slice(found, func(i, j int) bool { return strings.Compare(found[i].Target, found[j].Target) < 0 }) - sort.Slice(expected, func(i, j int) bool { - return strings.Compare(expected[i].Target, expected[j].Target) < 0 - }) + must.Eq(t, []docker.HostMount{ + // from mount map + { + Type: "bind", + Target: "/map-bind-target", + Source: "redis-demo\\map-source", + BindOptions: &docker.BindOptions{}, + }, + { + Type: "tmpfs", + Target: "/map-tmpfs-target", + TempfsOptions: &docker.TempfsOptions{}, + }, + // from mount list + { + Type: "bind", + Target: "/list-bind-target", + Source: "redis-demo\\list-source", + BindOptions: &docker.BindOptions{}, + }, + { + Type: "tmpfs", + Target: "/list-tmpfs-target", + TempfsOptions: &docker.TempfsOptions{}, + }, + }, cc.HostConfig.Mounts) - require.Equal(t, expected, found) + must.Eq(t, []string{ + `alloc:c:/alloc`, + `redis-demo\local:c:/local`, + `redis-demo\secrets:c:/secrets`, + `c:\etc\ssl\certs:c:/etc/ssl/certs`, + `c:\var\www:c:/srv/www`, + }, cc.HostConfig.Binds) } func TestDockerDriver_CreateContainerConfigWithRuntimes(t *testing.T) {