From 085bdf8ff59ccb1e6ce244c2b4829c471e60637c Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Sat, 28 Oct 2017 23:43:20 +0200 Subject: [PATCH] container_create: sort mounts before adding them to the spec Signed-off-by: Antonio Murdaca --- server/container_create.go | 106 +++++++++++++----- test/crio_secrets.bats | 34 ------ test/default_mounts.bats | 69 ++++++++++++ .../container_redis_default_mounts.json | 67 +++++++++++ 4 files changed, 213 insertions(+), 63 deletions(-) delete mode 100644 test/crio_secrets.bats create mode 100644 test/default_mounts.bats create mode 100644 test/testdata/container_redis_default_mounts.json diff --git a/server/container_create.go b/server/container_create.go index d1da4cf1..04f7c2ea 100644 --- a/server/container_create.go +++ b/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 { - specgen.AddBindMount(m.Source, m.Destination, nil) - } - return nil + return mounts, 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 } diff --git a/test/crio_secrets.bats b/test/crio_secrets.bats deleted file mode 100644 index 2f36d18d..00000000 --- a/test/crio_secrets.bats +++ /dev/null @@ -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 -} diff --git a/test/default_mounts.bats b/test/default_mounts.bats new file mode 100644 index 00000000..8e727085 --- /dev/null +++ b/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 +} diff --git a/test/testdata/container_redis_default_mounts.json b/test/testdata/container_redis_default_mounts.json new file mode 100644 index 00000000..dff3db5a --- /dev/null +++ b/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" + ] + } + } + } +}