Merge pull request #1089 from runcom/sort-mounts-v1

[release-1.0] container_create: sort mounts before adding them to the spec

server/container_create.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -46,29 +47,54 @@ const (
 	defaultSystemdParent  = "system.slice"
 )
 
-func addOCIBindMounts(mountLabel string, containerConfig *pb.ContainerConfig, specgen *generate.Generator) ([]oci.ContainerVolume, error) {
+type orderedMounts []rspec.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].Destination), string(os.PathSeparator))
+}
+
+func addOCIBindMounts(mountLabel string, containerConfig *pb.ContainerConfig, specgen *generate.Generator) ([]oci.ContainerVolume, []rspec.Mount, error) {
 	volumes := []oci.ContainerVolume{}
+	ociMounts := []rspec.Mount{}
 	mounts := containerConfig.GetMounts()
 	for _, mount := range mounts {
 		dest := mount.ContainerPath
 		if dest == "" {
-			return nil, fmt.Errorf("Mount.ContainerPath is empty")
+			return nil, nil, fmt.Errorf("Mount.ContainerPath is empty")
 		}
 
 		src := mount.HostPath
 		if src == "" {
-			return nil, fmt.Errorf("Mount.HostPath is empty")
+			return nil, nil, fmt.Errorf("Mount.HostPath is empty")
 		}
 
 		if _, err := os.Stat(src); err != nil && os.IsNotExist(err) {
 			if err1 := os.MkdirAll(src, 0644); err1 != nil {
-				return nil, fmt.Errorf("Failed to mkdir %s: %s", src, err)
+				return nil, nil, fmt.Errorf("Failed to mkdir %s: %s", src, err)
 			}
 		}
 
 		src, err := resolveSymbolicLink(src)
 		if err != nil {
-			return nil, fmt.Errorf("failed to resolve symlink %q: %v", src, err)
+			return nil, nil, fmt.Errorf("failed to resolve symlink %q: %v", src, err)
 		}
 
 		options := []string{"rw"}
@@ -80,7 +106,7 @@ func addOCIBindMounts(mountLabel string, containerConfig *pb.ContainerConfig, sp
 		if mount.SelinuxRelabel {
 			// Need a way in kubernetes to determine if the volume is shared or private
 			if err := label.Relabel(src, mountLabel, true); err != nil && err != unix.ENOTSUP {
-				return nil, fmt.Errorf("relabel failed %s: %v", src, err)
+				return nil, nil, fmt.Errorf("relabel failed %s: %v", src, err)
 			}
 		}
 
@@ -90,45 +116,55 @@ func addOCIBindMounts(mountLabel string, containerConfig *pb.ContainerConfig, sp
 			Readonly:      mount.Readonly,
 		})
 
-		specgen.AddBindMount(src, dest, options)
+		ociMounts = append(ociMounts, rspec.Mount{
+			Source:      src,
+			Destination: dest,
+			Options:     options,
+		})
 	}
 
-	return volumes, nil
+	return volumes, ociMounts, nil
 }
 
-func addImageVolumes(rootfs string, s *Server, containerInfo *storage.ContainerInfo, specgen *generate.Generator, mountLabel string) error {
+func addImageVolumes(rootfs string, s *Server, containerInfo *storage.ContainerInfo, specgen *generate.Generator, mountLabel string) ([]rspec.Mount, error) {
+	mounts := []rspec.Mount{}
 	for dest := range containerInfo.Config.Config.Volumes {
 		fp, err := symlink.FollowSymlinkInScope(filepath.Join(rootfs, dest), rootfs)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		switch s.config.ImageVolumes {
 		case libkpod.ImageVolumesMkdir:
 			if err1 := os.MkdirAll(fp, 0644); err1 != nil {
-				return err1
+				return nil, err1
 			}
 		case libkpod.ImageVolumesBind:
 			volumeDirName := stringid.GenerateNonCryptoID()
 			src := filepath.Join(containerInfo.RunDir, "mounts", volumeDirName)
 			if err1 := os.MkdirAll(src, 0644); err1 != nil {
-				return err1
+				return nil, err1
 			}
 			// Label the source with the sandbox selinux mount label
 			if mountLabel != "" {
 				if err1 := label.Relabel(src, mountLabel, true); err1 != nil && err1 != unix.ENOTSUP {
-					return fmt.Errorf("relabel failed %s: %v", src, err1)
+					return nil, fmt.Errorf("relabel failed %s: %v", src, err1)
 				}
 			}
 
 			logrus.Debugf("Adding bind mounted volume: %s to %s", src, dest)
-			specgen.AddBindMount(src, dest, []string{"rw"})
+			mounts = append(mounts, rspec.Mount{
+				Source:      src,
+				Destination: dest,
+				Options:     []string{"rw"},
+			})
+
 		case libkpod.ImageVolumesIgnore:
 			logrus.Debugf("Ignoring volume %v", dest)
 		default:
 			logrus.Fatalf("Unrecognized image volumes setting")
 		}
 	}
-	return nil
+	return mounts, nil
 }
 
 // resolveSymbolicLink resolves a possbile symlink path. If the path is a symlink, returns resolved
@@ -385,16 +421,13 @@ func ensureSaneLogPath(logPath string) error {
 }
 
 // addSecretsBindMounts mounts user defined secrets to the container
-func addSecretsBindMounts(mountLabel, ctrRunDir string, defaultMounts []string, specgen generate.Generator) error {
+func addSecretsBindMounts(mountLabel, ctrRunDir string, defaultMounts []string, specgen generate.Generator) ([]rspec.Mount, error) {
 	containerMounts := specgen.Spec().Mounts
 	mounts, err := secretMounts(defaultMounts, mountLabel, ctrRunDir, containerMounts)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	for _, m := range mounts {
+	return mounts, nil
-		specgen.AddBindMount(m.Source, m.Destination, nil)
-	}
-	return nil
 }
 
 // CreateContainer creates a new container in specified PodSandbox
@@ -562,7 +595,7 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string,
 		}
 	}
 
-	containerVolumes, err := addOCIBindMounts(mountLabel, containerConfig, &specgen)
+	containerVolumes, ociMounts, err := addOCIBindMounts(mountLabel, containerConfig, &specgen)
 	if err != nil {
 		return nil, err
 	}
@@ -934,12 +967,6 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string,
 		return nil, err
 	}
 
-	if len(s.config.DefaultMounts) > 0 {
-		if err = addSecretsBindMounts(mountLabel, containerInfo.RunDir, s.config.DefaultMounts, specgen); err != nil {
-			return nil, fmt.Errorf("failed to mount secrets: %v", err)
-		}
-	}
-
 	mountPoint, err := s.StorageRuntimeServer().StartContainer(containerID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to mount container %s(%s): %v", containerName, containerID, err)
@@ -957,7 +984,8 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string,
 	}
 
 	// Add image volumes
-	if err := addImageVolumes(mountPoint, s, &containerInfo, &specgen, mountLabel); err != nil {
+	volumeMounts, err := addImageVolumes(mountPoint, s, &containerInfo, &specgen, mountLabel)
+	if err != nil {
 		return nil, err
 	}
 
@@ -1008,6 +1036,26 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string,
 	}
 	specgen.SetProcessCwd(containerCwd)
 
+	var secretMounts []rspec.Mount
+	if len(s.config.DefaultMounts) > 0 {
+		var err error
+		secretMounts, err = addSecretsBindMounts(mountLabel, containerInfo.RunDir, s.config.DefaultMounts, specgen)
+		if err != nil {
+			return nil, fmt.Errorf("failed to mount secrets: %v", err)
+		}
+	}
+
+	mounts := []rspec.Mount{}
+	mounts = append(mounts, ociMounts...)
+	mounts = append(mounts, volumeMounts...)
+	mounts = append(mounts, secretMounts...)
+
+	sort.Sort(orderedMounts(mounts))
+
+	for _, m := range mounts {
+		specgen.AddBindMount(m.Source, m.Destination, m.Options)
+	}
+
 	if err := s.setupOCIHooks(&specgen, sb, containerConfig, processArgs[0]); err != nil {
 		return nil, err
 	}

test/crio_secrets.bats

@@ -1,34 +0,0 @@
-#!/usr/bin/env bats
-
-load helpers
-
-IMAGE="redis:alpine"
-
-function teardown() {
-	cleanup_test
-}
-
-@test "bind secrets mounts to container" {
-    start_crio
-    run crioctl pod run --config "$TESTDATA"/sandbox_config.json
-    echo "$output"
-    [ "$status" -eq 0 ]
-    pod_id="$output"
-    run crioctl image pull "$IMAGE"
-    [ "$status" -eq 0 ]
-    run crioctl ctr create --config "$TESTDATA"/container_redis.json --pod "$pod_id"
-    echo "$output"
-    [ "$status" -eq 0 ]
-    ctr_id="$output"
-    run crioctl ctr execsync --id "$ctr_id" mount
-    echo "$output"
-    [ "$status" -eq 0 ]
-    mount_info="$output"
-    grep /container/path1 <<< "$mount_info"
-    echo "$output"
-    [ "$status" -eq 0 ]
-    rm -rf MOUNT_PATH
-    cleanup_ctrs
-    cleanup_pods
-    stop_crio
-}

test/default_mounts.bats

@@ -0,0 +1,69 @@
+#!/usr/bin/env bats
+
+load helpers
+
+IMAGE="redis:alpine"
+
+function teardown() {
+	cleanup_test
+}
+
+@test "bind secrets mounts to container" {
+    start_crio
+    run crioctl pod run --config "$TESTDATA"/sandbox_config.json
+    echo "$output"
+    [ "$status" -eq 0 ]
+    pod_id="$output"
+    run crioctl image pull "$IMAGE"
+    [ "$status" -eq 0 ]
+    run crioctl ctr create --config "$TESTDATA"/container_redis.json --pod "$pod_id"
+    echo "$output"
+    [ "$status" -eq 0 ]
+    ctr_id="$output"
+    run crioctl ctr execsync --id "$ctr_id" cat /proc/mounts
+    echo "$output"
+    [ "$status" -eq 0 ]
+    mount_info="$output"
+    run grep /container/path1 <<< "$mount_info"
+    echo "$output"
+    [ "$status" -eq 0 ]
+    cleanup_ctrs
+    cleanup_pods
+    stop_crio
+}
+
+@test "default mounts correctly sorted with other mounts" {
+    start_crio
+    run crioctl pod run --config "$TESTDATA"/sandbox_config.json
+    echo "$output"
+    [ "$status" -eq 0 ]
+    pod_id="$output"
+    run crioctl image pull "$IMAGE"
+    [ "$status" -eq 0 ]
+    host_path="$TESTDIR"/clash
+    mkdir "$host_path"
+    echo "clashing..." > "$host_path"/clashing.txt
+    sed -e "s,%HPATH%,$host_path,g" "$TESTDATA"/container_redis_default_mounts.json > "$TESTDIR"/defmounts_pre.json
+    sed -e 's,%CPATH%,\/container\/path1\/clash,g' "$TESTDIR"/defmounts_pre.json > "$TESTDIR"/defmounts.json
+    run crioctl ctr create --config "$TESTDIR"/defmounts.json --pod "$pod_id"
+    echo "$output"
+    [ "$status" -eq 0 ]
+    ctr_id="$output"
+    run crioctl ctr execsync --id "$ctr_id" ls -la /container/path1/clash
+    echo "$output"
+    [ "$status" -eq 0 ]
+    run crioctl ctr execsync --id "$ctr_id" cat /container/path1/clash/clashing.txt
+    echo "$output"
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ "clashing..." ]]
+    run crioctl ctr execsync --id "$ctr_id" ls -la /container/path1
+    echo "$output"
+    [ "$status" -eq 0 ]
+    run crioctl ctr execsync --id "$ctr_id" cat /container/path1/test.txt
+    echo "$output"
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ "Testing secrets mounts!" ]]
+    cleanup_ctrs
+    cleanup_pods
+    stop_crio
+}

test/testdata/container_redis_default_mounts.json

@@ -0,0 +1,67 @@
+{
+	"metadata": {
+		"name": "podsandbox1-redis"
+	},
+	"image": {
+		"image": "redis:alpine"
+	},
+	"args": [
+                "docker-entrypoint.sh",
+                "redis-server"
+	],
+	"mounts": [
+		{
+			"container_path": "%CPATH%",
+			"host_path": "%HPATH%"
+		}
+	],
+	"working_dir": "/data",
+	"envs": [
+		{
+			"key": "PATH",
+			"value": "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
+		},
+		{
+			"key": "TERM",
+			"value": "xterm"
+		},
+		{
+			"key": "REDIS_VERSION",
+			"value": "3.2.3"
+		},
+		{
+			"key": "REDIS_DOWNLOAD_URL",
+			"value": "http://download.redis.io/releases/redis-3.2.3.tar.gz"
+		},
+		{
+			"key": "REDIS_DOWNLOAD_SHA1",
+			"value": "92d6d93ef2efc91e595c8bf578bf72baff397507"
+		}
+	],
+	"labels": {
+		"tier": "backend"
+	},
+	"annotations": {
+		"pod": "podsandbox1"
+	},
+	"readonly_rootfs": false,
+	"log_path": "",
+	"stdin": false,
+	"stdin_once": false,
+	"tty": false,
+	"linux": {
+		"resources": {
+			"cpu_period": 10000,
+			"cpu_quota": 20000,
+			"cpu_shares": 512,
+			"oom_score_adj": 30
+		},
+		"security_context": {
+			"capabilities": {
+				"add_capabilities": [
+					"sys_admin"
+				]
+			}
+		}
+	}
+}