From 32769fe96566ad4184644c8b35d850f782376404 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Thu, 23 Dec 2021 11:07:38 -0800 Subject: [PATCH] Mount `/dev/shm` as shared mount between containers in pod K8s mounts `/dev/shm` as a `bind` mount pointing to the same location for all containers in a pod, so it can be shared amoung them. This causes gVisor with VFS2 to mount `/dev/shm` over 9p to the gofer. This confuses glibc which looks for a tmpfs mount for the directory to use with shm_open(3). The shim now detects this intent and add makes the corresponding changes to the spec to make `/dev/shm` be mounted as a pod shared tmpfs volume. It consists of annotations to indicate that the volume is pod shared, and changes to mount type from `bind` to `tmpfs`. It also required changes in runsc to allow shared mounts with more restrictive options, so it doesn't break containers that mount `/dev/shm` with less restrictive options, like `ro`. Closes #6943 PiperOrigin-RevId: 418035851 --- pkg/shim/utils/volumes.go | 71 ++++++++++++- pkg/shim/utils/volumes_test.go | 124 ++++++++++++++++++++-- runsc/boot/BUILD | 1 + runsc/boot/fs.go | 23 ++++- runsc/boot/vfs.go | 53 +++++----- runsc/boot/vfs_test.go | 82 +++++++++++++++ runsc/container/multi_container_test.go | 131 +++++++++++++++++++++++- 7 files changed, 448 insertions(+), 37 deletions(-) create mode 100644 runsc/boot/vfs_test.go diff --git a/pkg/shim/utils/volumes.go b/pkg/shim/utils/volumes.go index 6bc75139d..f8090109d 100644 --- a/pkg/shim/utils/volumes.go +++ b/pkg/shim/utils/volumes.go @@ -22,7 +22,13 @@ import ( specs "github.com/opencontainers/runtime-spec/specs-go" ) -const volumeKeyPrefix = "dev.gvisor.spec.mount." +const ( + volumeKeyPrefix = "dev.gvisor.spec.mount." + + // devshmName is the volume name used for /dev/shm. Pick a name that is + // unlikely to be used. + devshmName = "gvisorinternaldevshm" +) var kubeletPodsDir = "/var/lib/kubelet/pods" @@ -99,7 +105,7 @@ func UpdateVolumeAnnotations(s *specs.Spec) (bool, error) { return false, nil } } - var updated bool + updated := false for k, v := range s.Annotations { if !isVolumeKey(k) { continue @@ -136,6 +142,67 @@ func UpdateVolumeAnnotations(s *specs.Spec) (bool, error) { } } } + + if ok, err := configureShm(s); err != nil { + return false, err + } else if ok { + updated = true + } + + return updated, nil +} + +// configureShm sets up annotations to mount /dev/shm as a pod shared tmpfs +// mount inside containers. +// +// Pods are configured to mount /dev/shm to a common path in the host, so it's +// shared among containers in the same pod. In gVisor, /dev/shm must be +// converted to a tmpfs mount inside the sandbox, otherwise shm_open(3) doesn't +// use it (see where_is_shmfs() in glibc). Mount annotation hints are used to +// instruct runsc to mount the same tmpfs volume in all containers inside the +// pod. +func configureShm(s *specs.Spec) (bool, error) { + const ( + shmPath = "/dev/shm" + devshmType = "tmpfs" + ) + + // Some containers contain a duplicate mount entry for /dev/shm using tmpfs. + // If this is detected, remove the extraneous entry to ensure the correct one + // is used. + duplicate := -1 + for i, m := range s.Mounts { + if m.Destination == shmPath && m.Type == devshmType { + duplicate = i + break + } + } + + updated := false + for i := range s.Mounts { + m := &s.Mounts[i] + if m.Destination == shmPath && m.Type == "bind" { + if IsSandbox(s) { + s.Annotations["dev.gvisor.spec.mount."+devshmName+".source"] = m.Source + s.Annotations["dev.gvisor.spec.mount."+devshmName+".type"] = devshmType + s.Annotations["dev.gvisor.spec.mount."+devshmName+".share"] = "pod" + // Given that we don't have visibility into mount options for all + // containers, assume broad access for the master mount (it's tmpfs + // inside the sandbox anyways) and apply options to subcontainers as + // they bind mount individually. + s.Annotations["dev.gvisor.spec.mount."+devshmName+".options"] = "rw" + } + + changeMountType(m, devshmType) + updated = true + + // Remove the duplicate entry now that we found the shared /dev/shm mount. + if duplicate >= 0 { + s.Mounts = append(s.Mounts[:duplicate], s.Mounts[duplicate+1:]...) + } + break + } + } return updated, nil } diff --git a/pkg/shim/utils/volumes_test.go b/pkg/shim/utils/volumes_test.go index 5db43cdf1..cc6ab44fc 100644 --- a/pkg/shim/utils/volumes_test.go +++ b/pkg/shim/utils/volumes_test.go @@ -307,23 +307,135 @@ func TestUpdateVolumeAnnotations(t *testing.T) { }, expectUpdate: true, }, + { + name: "shm-sandbox", + spec: &specs.Spec{ + Annotations: map[string]string{ + sandboxLogDirAnnotation: testLogDirPath, + ContainerTypeAnnotation: containerTypeSandbox, + }, + Mounts: []specs.Mount{ + { + Destination: "/dev/shm", + Type: "bind", + Source: testVolumePath, + Options: []string{"ro", "foo"}, + }, + }, + }, + expected: &specs.Spec{ + Annotations: map[string]string{ + sandboxLogDirAnnotation: testLogDirPath, + ContainerTypeAnnotation: containerTypeSandbox, + volumeKeyPrefix + devshmName + ".share": "pod", + volumeKeyPrefix + devshmName + ".type": "tmpfs", + volumeKeyPrefix + devshmName + ".options": "rw", + volumeKeyPrefix + devshmName + ".source": testVolumePath, + }, + Mounts: []specs.Mount{ + { + Destination: "/dev/shm", + Type: "tmpfs", + Source: testVolumePath, + Options: []string{"ro", "foo"}, + }, + }, + }, + expectUpdate: true, + }, + { + name: "shm-container", + spec: &specs.Spec{ + Annotations: map[string]string{ + ContainerTypeAnnotation: ContainerTypeContainer, + }, + Mounts: []specs.Mount{ + { + Destination: "/dev/shm", + Type: "bind", + Source: testVolumePath, + Options: []string{"ro", "foo"}, + }, + }, + }, + expected: &specs.Spec{ + Annotations: map[string]string{ + ContainerTypeAnnotation: ContainerTypeContainer, + }, + Mounts: []specs.Mount{ + { + Destination: "/dev/shm", + Type: "tmpfs", + Source: testVolumePath, + Options: []string{"ro", "foo"}, + }, + }, + }, + expectUpdate: true, + }, + { + name: "shm-duplicate", + spec: &specs.Spec{ + Annotations: map[string]string{ + ContainerTypeAnnotation: ContainerTypeContainer, + }, + Mounts: []specs.Mount{ + { + Destination: "/dev/shm", + Type: "bind", + Source: testVolumePath, + Options: []string{"ro", "foo"}, + }, + { + Destination: "/dev/shm", + Type: "tmpfs", + }, + { + Destination: "/home", + Type: "bind", + Source: "/another/mount", + Options: []string{"rw"}, + }, + }, + }, + expected: &specs.Spec{ + Annotations: map[string]string{ + ContainerTypeAnnotation: ContainerTypeContainer, + }, + Mounts: []specs.Mount{ + { + Destination: "/dev/shm", + Type: "tmpfs", + Source: testVolumePath, + Options: []string{"ro", "foo"}, + }, + { + Destination: "/home", + Type: "bind", + Source: "/another/mount", + Options: []string{"rw"}, + }, + }, + }, + expectUpdate: true, + }, } { t.Run(test.name, func(t *testing.T) { updated, err := UpdateVolumeAnnotations(test.spec) if test.expectErr { if err == nil { - t.Fatal("Expected error, but got nil") + t.Fatal("UpdateVolumeAnnotations(spec): nil, want: error") } return } if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if !reflect.DeepEqual(test.expected, test.spec) { - t.Fatalf("Expected %+v, got %+v", test.expected, test.spec) + t.Fatalf("UpdateVolumeAnnotations(spec): %v", err) } if test.expectUpdate != updated { - t.Errorf("Expected %v, got %v", test.expected, updated) + t.Errorf("want: %v, got: %v", test.expectUpdate, updated) + } + if !reflect.DeepEqual(test.expected, test.spec) { + t.Fatalf("want: %+v, got: %+v", test.expected, test.spec) } }) } diff --git a/runsc/boot/BUILD b/runsc/boot/BUILD index 19328f89c..187b700b9 100644 --- a/runsc/boot/BUILD +++ b/runsc/boot/BUILD @@ -132,6 +132,7 @@ go_test( "compat_test.go", "fs_test.go", "loader_test.go", + "vfs_test.go", ], library = ":boot", deps = [ diff --git a/runsc/boot/fs.go b/runsc/boot/fs.go index ceeccf483..f02818743 100644 --- a/runsc/boot/fs.go +++ b/runsc/boot/fs.go @@ -492,10 +492,10 @@ func (m *mountHint) isSupported() bool { // For now enforce that all options are the same. Once bind mount is properly // supported, then we should ensure the master is less restrictive than the // container, e.g. master can be 'rw' while container mounts as 'ro'. -func (m *mountHint) checkCompatible(mount *specs.Mount) error { +func (m *mountHint) checkCompatible(replica *specs.Mount) error { // Remove options that don't affect to mount's behavior. masterOpts := filterUnsupportedOptions(&m.mount) - replicaOpts := filterUnsupportedOptions(mount) + replicaOpts := filterUnsupportedOptions(replica) if len(masterOpts) != len(replicaOpts) { return fmt.Errorf("mount options in annotations differ from container mount, annotation: %s, mount: %s", masterOpts, replicaOpts) @@ -511,6 +511,25 @@ func (m *mountHint) checkCompatible(mount *specs.Mount) error { return nil } +// checkCompatibleVFS2 verifies that shared mount is compatible with master. +// Master options must be the same or less restrictive than the container mount, +// e.g. master can be 'rw' while container mounts as 'ro'. +func (m *mountHint) checkCompatibleVFS2(replica *specs.Mount) error { + masterOpts := parseMountOptionsVFS2(m.mount.Options) + replicaOpts := parseMountOptionsVFS2(replica.Options) + + if masterOpts.ReadOnly && !replicaOpts.ReadOnly { + return fmt.Errorf("cannot mount read-write shared mount because master is read-only, mount: %+v", replica) + } + if masterOpts.Flags.NoExec && !replicaOpts.Flags.NoExec { + return fmt.Errorf("cannot mount exec enabled shared mount because master is noexec, mount: %+v", replica) + } + if masterOpts.Flags.NoATime && !replicaOpts.Flags.NoATime { + return fmt.Errorf("cannot mount atime enabled shared mount because master is noatime, mount: %+v", replica) + } + return nil +} + func (m *mountHint) fileAccessType() config.FileAccessType { if m.share == container { return config.FileAccessExclusive diff --git a/runsc/boot/vfs.go b/runsc/boot/vfs.go index 4d0cfc94a..a2b18abb9 100644 --- a/runsc/boot/vfs.go +++ b/runsc/boot/vfs.go @@ -540,29 +540,10 @@ func (c *containerMounter) getMountNameAndOptionsVFS2(conf *config.Config, m *mo return "", nil, false, nil } - opts := &vfs.MountOptions{ - GetFilesystemOptions: vfs.GetFilesystemOptions{ - Data: strings.Join(data, ","), - InternalData: internalData, - }, - InternalMount: true, - } - - for _, o := range m.mount.Options { - switch o { - case "rw": - opts.ReadOnly = false - case "ro": - opts.ReadOnly = true - case "noatime": - opts.Flags.NoATime = true - case "noexec": - opts.Flags.NoExec = true - case "bind", "rbind": - // These are the same as a mount with type="bind". - default: - log.Warningf("ignoring unknown mount option %q", o) - } + opts := parseMountOptionsVFS2(m.mount.Options) + opts.GetFilesystemOptions = vfs.GetFilesystemOptions{ + Data: strings.Join(data, ","), + InternalData: internalData, } if verityRequested { @@ -582,6 +563,30 @@ func (c *containerMounter) getMountNameAndOptionsVFS2(conf *config.Config, m *mo return fsName, opts, useOverlay, nil } +func parseMountOptionsVFS2(opts []string) *vfs.MountOptions { + mountOpts := &vfs.MountOptions{ + InternalMount: true, + } + // Note: update mountHint.CheckCompatibleVFS2 when more options are added. + for _, o := range opts { + switch o { + case "ro": + mountOpts.ReadOnly = true + case "noatime": + mountOpts.Flags.NoATime = true + case "noexec": + mountOpts.Flags.NoExec = true + case "rw", "atime", "exec": + // These use the default value and don't need to be set. + case "bind", "rbind": + // These are the same as a mount with type="bind". + default: + log.Warningf("ignoring unknown mount option %q", o) + } + } + return mountOpts +} + func parseKeyValue(s string) (string, string, bool) { tokens := strings.SplitN(s, "=", 2) if len(tokens) < 2 { @@ -759,7 +764,7 @@ func (c *containerMounter) mountSharedMasterVFS2(ctx context.Context, conf *conf // mountSharedSubmount binds mount to a previously mounted volume that is shared // among containers in the same pod. func (c *containerMounter) mountSharedSubmountVFS2(ctx context.Context, conf *config.Config, mns *vfs.MountNamespace, creds *auth.Credentials, mount *specs.Mount, source *mountHint) (*vfs.Mount, error) { - if err := source.checkCompatible(mount); err != nil { + if err := source.checkCompatibleVFS2(mount); err != nil { return nil, err } diff --git a/runsc/boot/vfs_test.go b/runsc/boot/vfs_test.go new file mode 100644 index 000000000..73cd0a962 --- /dev/null +++ b/runsc/boot/vfs_test.go @@ -0,0 +1,82 @@ +// Copyright 2021 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package boot + +import ( + "strings" + "testing" + + specs "github.com/opencontainers/runtime-spec/specs-go" +) + +func TestHintsCheckCompatible(t *testing.T) { + for _, tc := range []struct { + name string + masterOpts []string + replicaOpts []string + err string + }{ + { + name: "empty", + }, + { + name: "same", + masterOpts: []string{"ro", "noatime", "noexec"}, + replicaOpts: []string{"ro", "noatime", "noexec"}, + }, + { + name: "compatible", + masterOpts: []string{"rw", "atime", "exec"}, + replicaOpts: []string{"ro", "noatime", "noexec"}, + }, + { + name: "unsupported", + masterOpts: []string{"nofoo", "nodev"}, + replicaOpts: []string{"foo", "dev"}, + }, + { + name: "incompatible-ro", + masterOpts: []string{"ro"}, + replicaOpts: []string{"rw"}, + err: "read-write", + }, + { + name: "incompatible-atime", + masterOpts: []string{"noatime"}, + replicaOpts: []string{"atime"}, + err: "noatime", + }, + { + name: "incompatible-exec", + masterOpts: []string{"noexec"}, + replicaOpts: []string{"exec"}, + err: "noexec", + }, + } { + t.Run(tc.name, func(t *testing.T) { + master := mountHint{mount: specs.Mount{Options: tc.masterOpts}} + replica := specs.Mount{Options: tc.replicaOpts} + if err := master.checkCompatibleVFS2(&replica); err != nil { + if !strings.Contains(err.Error(), tc.err) { + t.Fatalf("wrong error, want: %q, got: %q", tc.err, err) + } + } else { + if len(tc.err) > 0 { + t.Fatalf("error %q expected", tc.err) + } + } + }) + } +} diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go index a620944a3..d0e700fb3 100644 --- a/runsc/container/multi_container_test.go +++ b/runsc/container/multi_container_test.go @@ -101,8 +101,9 @@ func startContainers(conf *config.Config, specs []*specs.Spec, ids []string) ([] type execDesc struct { c *Container cmd []string - want int name string + want int + err string } func execMany(t *testing.T, conf *config.Config, execs []execDesc) { @@ -110,9 +111,13 @@ func execMany(t *testing.T, conf *config.Config, execs []execDesc) { t.Run(exec.name, func(t *testing.T) { args := &control.ExecArgs{Argv: exec.cmd} if ws, err := exec.c.executeSync(conf, args); err != nil { - t.Errorf("error executing %+v: %v", args, err) + if len(exec.err) == 0 || !strings.Contains(err.Error(), exec.err) { + t.Errorf("error executing %+v: %v", args, err) + } + } else if len(exec.err) > 0 { + t.Errorf("exec %q didn't fail as expected", exec.cmd) } else if ws.ExitStatus() != exec.want { - t.Errorf("%q: exec %q got exit status: %d, want: %d", exec.name, exec.cmd, ws.ExitStatus(), exec.want) + t.Errorf("exec %q got exit status: %d, want: %d", exec.cmd, ws.ExitStatus(), exec.want) } }) } @@ -1387,6 +1392,75 @@ func TestMultiContainerSharedMountReadonly(t *testing.T) { } } +// Test that pod mounts can be mounted with less restrictive options in +// container mounts. +func TestMultiContainerSharedMountCompatible(t *testing.T) { + rootDir, cleanup, err := testutil.SetupRootDir() + if err != nil { + t.Fatalf("error creating root dir: %v", err) + } + defer cleanup() + conf := testutil.TestConfig(t) + conf.RootDir = rootDir + + sleep := []string{"sleep", "100"} + podSpec, ids := createSpecs(sleep, sleep) + + // Init container and annotations allow read-write and exec. + mnt0 := specs.Mount{ + Destination: "/mydir/test", + Source: "/some/dir", + Type: "tmpfs", + Options: []string{"rw", "exec"}, + } + podSpec[0].Mounts = append(podSpec[0].Mounts, mnt0) + + // While subcontainer mount has more restrictive options: read-only, noexec. + mnt1 := mnt0 + mnt1.Destination = "/mydir2/test2" + mnt1.Options = []string{"ro", "noexec"} + podSpec[1].Mounts = append(podSpec[1].Mounts, mnt1) + + createSharedMount(mnt0, "test-mount", podSpec...) + + containers, cleanup, err := startContainers(conf, podSpec, ids) + if err != nil { + t.Fatalf("error starting containers: %v", err) + } + defer cleanup() + + execs := []execDesc{ + { + c: containers[1], + cmd: []string{"/bin/touch", path.Join(mnt1.Destination, "fail")}, + want: 1, + name: "fails write to container1", + }, + { + c: containers[0], + cmd: []string{"/bin/cp", "/usr/bin/test", mnt0.Destination}, + name: "writes to container0", + }, + { + c: containers[1], + cmd: []string{"/usr/bin/test", "-f", path.Join(mnt1.Destination, "test")}, + name: "file appears in container1", + }, + { + c: containers[0], + cmd: []string{path.Join(mnt0.Destination, "test"), "-d", mnt0.Destination}, + name: "container0 can execute", + }, + { + c: containers[1], + cmd: []string{path.Join(mnt1.Destination, "test"), "-d", mnt1.Destination}, + err: "permission denied", + name: "container1 cannot execute", + }, + } + execMany(t, conf, execs) +} + // Test that shared pod mounts continue to work after container is restarted. func TestMultiContainerSharedMountRestart(t *testing.T) { for name, conf := range configs(t, all...) { @@ -2088,3 +2162,54 @@ func TestDuplicateEnvVariable(t *testing.T) { } } } + +// Test that /dev/shm can be shared between containers. +func TestMultiContainerShm(t *testing.T) { + conf := testutil.TestConfig(t) + + rootDir, cleanup, err := testutil.SetupRootDir() + if err != nil { + t.Fatalf("error creating root dir: %v", err) + } + defer cleanup() + conf.RootDir = rootDir + + sleep := []string{"sleep", "100"} + testSpecs, ids := createSpecs(sleep, sleep) + + sharedMount := specs.Mount{ + Destination: "/dev/shm", + Source: "/some/path", + Type: "tmpfs", + } + + // Add shared /dev/shm mount to all containers. + for _, spec := range testSpecs { + spec.Mounts = append(spec.Mounts, sharedMount) + } + + // Create annotation hints for the init container. + createSharedMount(sharedMount, "devshm", testSpecs[0]) + + containers, cleanup, err := startContainers(conf, testSpecs, ids) + if err != nil { + t.Fatalf("error starting containers: %v", err) + } + defer cleanup() + + // Write file to shared /dev/shm directory in one container. + const output = "/dev/shm/file.txt" + exec0 := fmt.Sprintf("echo 123 > %s", output) + if ws, err := execute(conf, containers[0], "/bin/sh", "-c", exec0); err != nil || ws.ExitStatus() != 0 { + t.Fatalf("exec failed, ws: %v, err: %v", ws, err) + } + + // Check that file can be found in the other container. + out, err := executeCombinedOutput(conf, containers[1], "/bin/cat", output) + if err != nil { + t.Fatalf("exec failed: %v", err) + } + if want := "123\n"; string(out) != want { + t.Fatalf("wrong output, want: %q, got: %v", want, out) + } +}