From c75ea1d23cac9255441af819f6016a0d3e5e39d0 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 6 Aug 2024 10:35:39 -0400 Subject: [PATCH] docker: fix delimiter for selinux label for read-only volumes 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 00000000000..ba66df0239e --- /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 44174399974..f96aa877a70 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 534ee1b62b4..a3cc8248f30 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,17 +789,18 @@ 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 } - 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) + if opts != "" { + bind += ":" + opts } + binds = append(binds, bind) } return binds, nil diff --git a/drivers/docker/driver_test.go b/drivers/docker/driver_test.go index 6a486869752..e00d43223bd 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) - require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) + 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", + } + + 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) {