Merge pull request #892 from Random-Liu/fix-volume-mount-order

Sort volume mount.

pkg/server/container_create.go

@@ -19,6 +19,7 @@ package server
 import (
 	"os"
 	"path/filepath"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -351,8 +352,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.
+	// Merge extra mounts and CRI mounts.
-	mounts := append(extraMounts, config.GetMounts()...)
+	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)
 	}
@@ -616,6 +617,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",
@@ -623,6 +628,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()
@@ -841,10 +869,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
@@ -988,3 +1012,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
+}

pkg/server/container_create_test.go

@@ -19,6 +19,7 @@ package server
 import (
 	"path/filepath"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/containerd/containerd/contrib/apparmor"
@@ -311,26 +312,50 @@ func TestContainerSpecWithExtraMounts(t *testing.T) {
 		Readonly:      false,
 	}
 	config.Mounts = append(config.Mounts, mountInConfig)
-	extraMount := &runtime.Mount{
+	extraMounts := []*runtime.Mount{
-		ContainerPath: "test-container-path",
+		{
-		HostPath:      "test-host-path-extra",
+			ContainerPath: "test-container-path",
-		Readonly:      true,
+			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")
+	t.Logf("CRI mount should override extra mount")
-	require.Len(t, mounts, 2)
+	require.Len(t, mounts, 1)
-	assert.Equal(t, "test-host-path-extra", mounts[0].Source)
+	assert.Equal(t, "test-host-path", mounts[0].Source)
-	assert.Contains(t, mounts[0].Options, "ro")
+	assert.Contains(t, mounts[0].Options, "rw")
-	assert.Equal(t, "test-host-path", mounts[1].Source)
+
-	assert.Contains(t, mounts[1].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) {

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))
+}

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)
+}