From 063f8158f8c52c19f8d21b250e77657c4d2a25c7 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Tue, 4 Sep 2018 17:52:23 -0700 Subject: [PATCH] Sort volume mount. Signed-off-by: Lantao Liu --- pkg/server/container_create.go | 58 ++++++++++++++++++++++++++--- pkg/server/container_create_test.go | 49 ++++++++++++++++++------ pkg/server/helpers.go | 28 ++++++++++++++ pkg/server/helpers_test.go | 23 ++++++++++++ 4 files changed, 140 insertions(+), 18 deletions(-) diff --git a/pkg/server/container_create.go b/pkg/server/container_create.go index 69bd05997..fbe010a77 100644 --- a/pkg/server/container_create.go +++ b/pkg/server/container_create.go @@ -19,6 +19,7 @@ package server import ( "os" "path/filepath" + "sort" "strconv" "strings" "time" @@ -349,8 +350,8 @@ func (c *criService) generateContainerSpec(id string, sandboxID string, sandboxP return nil, errors.Wrapf(err, "failed to init selinux options %+v", securityContext.GetSelinuxOptions()) } - // Add extra mounts first so that CRI specified mounts can override. - mounts := append(extraMounts, config.GetMounts()...) + // Merge extra mounts and CRI mounts. + mounts := mergeMounts(config.GetMounts(), extraMounts) if err := c.addOCIBindMounts(&g, mounts, mountLabel); err != nil { return nil, errors.Wrapf(err, "failed to set OCI bind mounts %+v", mounts) } @@ -596,6 +597,10 @@ func setOCIDevicesPrivileged(g *generate.Generator) error { // addOCIBindMounts adds bind mounts. func (c *criService) addOCIBindMounts(g *generate.Generator, mounts []*runtime.Mount, mountLabel string) error { + // Sort mounts in number of parts. This ensures that high level mounts don't + // shadow other mounts. + sort.Sort(orderedMounts(mounts)) + // Mount cgroup into the container as readonly, which inherits docker's behavior. g.AddMount(runtimespec.Mount{ Source: "cgroup", @@ -603,6 +608,29 @@ func (c *criService) addOCIBindMounts(g *generate.Generator, mounts []*runtime.M Type: "cgroup", Options: []string{"nosuid", "noexec", "nodev", "relatime", "ro"}, }) + + // Copy all mounts from default mounts, except for + // - mounts overriden by supplied mount; + // - all mounts under /dev if a supplied /dev is present. + mountSet := make(map[string]struct{}) + for _, m := range mounts { + mountSet[filepath.Clean(m.ContainerPath)] = struct{}{} + } + defaultMounts := g.Mounts() + g.ClearMounts() + for _, m := range defaultMounts { + dst := filepath.Clean(m.Destination) + if _, ok := mountSet[dst]; ok { + // filter out mount overridden by a supplied mount + continue + } + if _, mountDev := mountSet["/dev"]; mountDev && strings.HasPrefix(dst, "/dev/") { + // filter out everything under /dev if /dev is a supplied mount + continue + } + g.AddMount(m) + } + for _, mount := range mounts { dst := mount.GetContainerPath() src := mount.GetHostPath() @@ -821,10 +849,6 @@ func defaultRuntimeSpec(id string) (*runtimespec.Spec, error) { if mount.Destination == "/run" { continue } - // CRI plugin handles `/dev/shm` itself. - if mount.Destination == "/dev/shm" { - continue - } mounts = append(mounts, mount) } spec.Mounts = mounts @@ -968,3 +992,25 @@ func generateUserString(username string, uid, gid *runtime.Int64Value) (string, } return userstr, nil } + +// mergeMounts merge CRI mounts with extra mounts. If a mount destination +// is mounted by both a CRI mount and an extra mount, the CRI mount will +// be kept. +func mergeMounts(criMounts, extraMounts []*runtime.Mount) []*runtime.Mount { + var mounts []*runtime.Mount + mounts = append(mounts, criMounts...) + // Copy all mounts from extra mounts, except for mounts overriden by CRI. + for _, e := range extraMounts { + found := false + for _, c := range criMounts { + if filepath.Clean(e.ContainerPath) == filepath.Clean(c.ContainerPath) { + found = true + break + } + } + if !found { + mounts = append(mounts, e) + } + } + return mounts +} diff --git a/pkg/server/container_create_test.go b/pkg/server/container_create_test.go index fc665febf..a39a6b3fd 100644 --- a/pkg/server/container_create_test.go +++ b/pkg/server/container_create_test.go @@ -19,6 +19,7 @@ package server import ( "path/filepath" "reflect" + "strings" "testing" "github.com/containerd/containerd/contrib/apparmor" @@ -312,26 +313,50 @@ func TestContainerSpecWithExtraMounts(t *testing.T) { Readonly: false, } config.Mounts = append(config.Mounts, mountInConfig) - extraMount := &runtime.Mount{ - ContainerPath: "test-container-path", - HostPath: "test-host-path-extra", - Readonly: true, + extraMounts := []*runtime.Mount{ + { + ContainerPath: "test-container-path", + HostPath: "test-host-path-extra", + Readonly: true, + }, + { + ContainerPath: "/sys", + HostPath: "test-sys-extra", + Readonly: false, + }, + { + ContainerPath: "/dev", + HostPath: "test-dev-extra", + Readonly: false, + }, } - spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, []*runtime.Mount{extraMount}) + spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, extraMounts) require.NoError(t, err) specCheck(t, testID, testSandboxID, testPid, spec) - var mounts []runtimespec.Mount + var mounts, sysMounts, devMounts []runtimespec.Mount for _, m := range spec.Mounts { if m.Destination == "test-container-path" { mounts = append(mounts, m) + } else if m.Destination == "/sys" { + sysMounts = append(sysMounts, m) + } else if strings.HasPrefix(m.Destination, "/dev") { + devMounts = append(devMounts, m) } } - t.Logf("Extra mounts should come first") - require.Len(t, mounts, 2) - assert.Equal(t, "test-host-path-extra", mounts[0].Source) - assert.Contains(t, mounts[0].Options, "ro") - assert.Equal(t, "test-host-path", mounts[1].Source) - assert.Contains(t, mounts[1].Options, "rw") + t.Logf("CRI mount should override extra mount") + require.Len(t, mounts, 1) + assert.Equal(t, "test-host-path", mounts[0].Source) + assert.Contains(t, mounts[0].Options, "rw") + + t.Logf("Extra mount should override default mount") + require.Len(t, sysMounts, 1) + assert.Equal(t, "test-sys-extra", sysMounts[0].Source) + assert.Contains(t, sysMounts[0].Options, "rw") + + t.Logf("Dev mount should override all default dev mounts") + require.Len(t, devMounts, 1) + assert.Equal(t, "test-dev-extra", devMounts[0].Source) + assert.Contains(t, devMounts[0].Options, "rw") } func TestContainerAndSandboxPrivileged(t *testing.T) { diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index 93db8eae3..f2f171a26 100644 --- a/pkg/server/helpers.go +++ b/pkg/server/helpers.go @@ -19,6 +19,7 @@ package server import ( "encoding/json" "fmt" + "os" "path" "path/filepath" "regexp" @@ -472,3 +473,30 @@ func toRuntimeAuthConfig(a criconfig.AuthConfig) *runtime.AuthConfig { IdentityToken: a.IdentityToken, } } + +// mounts defines how to sort runtime.Mount. +// This is the same with the Docker implementation: +// https://github.com/moby/moby/blob/17.05.x/daemon/volumes.go#L26 +type orderedMounts []*runtime.Mount + +// Len returns the number of mounts. Used in sorting. +func (m orderedMounts) Len() int { + return len(m) +} + +// Less returns true if the number of parts (a/b/c would be 3 parts) in the +// mount indexed by parameter 1 is less than that of the mount indexed by +// parameter 2. Used in sorting. +func (m orderedMounts) Less(i, j int) bool { + return m.parts(i) < m.parts(j) +} + +// Swap swaps two items in an array of mounts. Used in sorting +func (m orderedMounts) Swap(i, j int) { + m[i], m[j] = m[j], m[i] +} + +// parts returns the number of parts in the destination of a mount. Used in sorting. +func (m orderedMounts) parts(i int) int { + return strings.Count(filepath.Clean(m[i].ContainerPath), string(os.PathSeparator)) +} diff --git a/pkg/server/helpers_test.go b/pkg/server/helpers_test.go index 078631a49..b84efed44 100644 --- a/pkg/server/helpers_test.go +++ b/pkg/server/helpers_test.go @@ -17,6 +17,7 @@ limitations under the License. package server import ( + "sort" "testing" "github.com/containerd/containerd" @@ -25,6 +26,7 @@ import ( imagedigest "github.com/opencontainers/go-digest" "github.com/stretchr/testify/assert" "golang.org/x/net/context" + runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" criconfig "github.com/containerd/cri/pkg/config" "github.com/containerd/cri/pkg/util" @@ -190,3 +192,24 @@ func TestGetRuntimeConfigFromContainerInfo(t *testing.T) { }) } } + +func TestOrderedMounts(t *testing.T) { + mounts := []*runtime.Mount{ + {ContainerPath: "/a/b/c"}, + {ContainerPath: "/a/b"}, + {ContainerPath: "/a/b/c/d"}, + {ContainerPath: "/a"}, + {ContainerPath: "/b"}, + {ContainerPath: "/b/c"}, + } + expected := []*runtime.Mount{ + {ContainerPath: "/a"}, + {ContainerPath: "/b"}, + {ContainerPath: "/a/b"}, + {ContainerPath: "/b/c"}, + {ContainerPath: "/a/b/c"}, + {ContainerPath: "/a/b/c/d"}, + } + sort.Stable(orderedMounts(mounts)) + assert.Equal(t, expected, mounts) +}