From 49eb38a5d41f39b6da823d58de6ac91fe52b7fcf Mon Sep 17 00:00:00 2001 From: Yanqiang Miao Date: Fri, 8 Sep 2017 22:58:39 +0800 Subject: [PATCH] Ensure the mount point is propagated mount with `rshared`, the host path should be shared. mount with `rslave`, the host pash should be shared or slave. Signed-off-by: Yanqiang Miao --- pkg/os/os.go | 6 ++ pkg/os/testing/fake_os.go | 15 +++ pkg/server/container_create.go | 48 +++++++++ pkg/server/container_create_test.go | 145 +++++++++++++++++++++------- pkg/server/helpers.go | 34 +++++++ 5 files changed, 212 insertions(+), 36 deletions(-) diff --git a/pkg/os/os.go b/pkg/os/os.go index ad5be5c0b..664d2e272 100644 --- a/pkg/os/os.go +++ b/pkg/os/os.go @@ -40,6 +40,7 @@ type OS interface { WriteFile(filename string, data []byte, perm os.FileMode) error Mount(source string, target string, fstype string, flags uintptr, data string) error Unmount(target string, flags int) error + GetMounts() ([]*mount.Info, error) } // RealOS is used to dispatch the real system level operations. @@ -114,3 +115,8 @@ func (RealOS) Unmount(target string, flags int) error { } return unix.Unmount(target, flags) } + +// GetMounts retrieves a list of mounts for the current running process. +func (RealOS) GetMounts() ([]*mount.Info, error) { + return mount.GetMounts() +} diff --git a/pkg/os/testing/fake_os.go b/pkg/os/testing/fake_os.go index 9ac5029b5..f6b0e80ef 100644 --- a/pkg/os/testing/fake_os.go +++ b/pkg/os/testing/fake_os.go @@ -21,6 +21,7 @@ import ( "os" "sync" + "github.com/docker/docker/pkg/mount" "golang.org/x/net/context" osInterface "github.com/kubernetes-incubator/cri-containerd/pkg/os" @@ -48,6 +49,7 @@ type FakeOS struct { WriteFileFn func(string, []byte, os.FileMode) error MountFn func(source string, target string, fstype string, flags uintptr, data string) error UnmountFn func(target string, flags int) error + GetMountsFn func() ([]*mount.Info, error) calls []CalledDetail errors map[string]error } @@ -225,3 +227,16 @@ func (f *FakeOS) Unmount(target string, flags int) error { } return nil } + +// GetMounts retrieves a list of mounts for the current running process. +func (f *FakeOS) GetMounts() ([]*mount.Info, error) { + f.appendCalls("GetMounts") + if err := f.getError("GetMounts"); err != nil { + return nil, err + } + + if f.GetMountsFn != nil { + return f.GetMountsFn() + } + return nil, nil +} diff --git a/pkg/server/container_create.go b/pkg/server/container_create.go index 427501339..6f8be1e50 100644 --- a/pkg/server/container_create.go +++ b/pkg/server/container_create.go @@ -25,6 +25,7 @@ import ( "github.com/containerd/containerd" "github.com/containerd/containerd/contrib/apparmor" + "github.com/docker/docker/pkg/mount" "github.com/golang/glog" imagespec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/opencontainers/runc/libcontainer/devices" @@ -551,13 +552,24 @@ func (c *criContainerdService) addOCIBindMounts(g *generate.Generator, mounts [] if err != nil { return fmt.Errorf("failed to resolve symlink %q: %v", src, err) } + + mountInfos, err := c.os.GetMounts() + if err != nil { + return err + } options := []string{"rbind"} switch mount.GetPropagation() { case runtime.MountPropagation_PROPAGATION_PRIVATE: options = append(options, "rprivate") case runtime.MountPropagation_PROPAGATION_BIDIRECTIONAL: + if err := ensureShared(src, mountInfos); err != nil { + return err + } options = append(options, "rshared") case runtime.MountPropagation_PROPAGATION_HOST_TO_CONTAINER: + if err := ensureSharedOrSlave(src, mountInfos); err != nil { + return err + } options = append(options, "rslave") default: glog.Warningf("Unknown propagation mode for hostPath %q", mount.HostPath) @@ -696,3 +708,39 @@ func defaultRuntimeSpec() (*runtimespec.Spec, error) { spec.Mounts = mounts return spec, nil } + +// Ensure mount point on which path is mounted, is shared. +func ensureShared(path string, mountInfos []*mount.Info) error { + sourceMount, optionalOpts, err := getSourceMount(path, mountInfos) + if err != nil { + return err + } + + // Make sure source mount point is shared. + optsSplit := strings.Split(optionalOpts, " ") + for _, opt := range optsSplit { + if strings.HasPrefix(opt, "shared:") { + return nil + } + } + + return fmt.Errorf("path %q is mounted on %q but it is not a shared mount", path, sourceMount) +} + +// Ensure mount point on which path is mounted, is either shared or slave. +func ensureSharedOrSlave(path string, mountInfos []*mount.Info) error { + sourceMount, optionalOpts, err := getSourceMount(path, mountInfos) + if err != nil { + return err + } + // Make sure source mount point is shared. + optsSplit := strings.Split(optionalOpts, " ") + for _, opt := range optsSplit { + if strings.HasPrefix(opt, "shared:") { + return nil + } else if strings.HasPrefix(opt, "master:") { + return nil + } + } + return fmt.Errorf("path %q is mounted on %q but it is not a shared or slave mount", path, sourceMount) +} diff --git a/pkg/server/container_create_test.go b/pkg/server/container_create_test.go index 39b9aade0..059a13248 100644 --- a/pkg/server/container_create_test.go +++ b/pkg/server/container_create_test.go @@ -20,6 +20,7 @@ import ( "path/filepath" "testing" + "github.com/docker/docker/pkg/mount" imagespec "github.com/opencontainers/image-spec/specs-go/v1" runtimespec "github.com/opencontainers/runtime-spec/specs-go" "github.com/opencontainers/runtime-tools/generate" @@ -27,6 +28,7 @@ import ( "github.com/stretchr/testify/require" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" + ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing" "github.com/kubernetes-incubator/cri-containerd/pkg/util" ) @@ -80,37 +82,6 @@ func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandbox HostPath: "host-path-2", Readonly: true, }, - // Propagation private - { - ContainerPath: "container-path-3", - HostPath: "host-path-3", - Propagation: runtime.MountPropagation_PROPAGATION_PRIVATE, - }, - // Propagation rslave - { - ContainerPath: "container-path-4", - HostPath: "host-path-4", - Propagation: runtime.MountPropagation_PROPAGATION_HOST_TO_CONTAINER, - }, - // Propagation rshared - { - ContainerPath: "container-path-5", - HostPath: "host-path-5", - Propagation: runtime.MountPropagation_PROPAGATION_BIDIRECTIONAL, - }, - // Propagation unknown (falls back to private) - { - ContainerPath: "container-path-6", - HostPath: "host-path-6", - Propagation: runtime.MountPropagation(42), - }, - // Everything - { - ContainerPath: "container-path-7", - HostPath: "host-path-7", - Readonly: true, - Propagation: runtime.MountPropagation_PROPAGATION_BIDIRECTIONAL, - }, }, Labels: map[string]string{"a": "b"}, Annotations: map[string]string{"c": "d"}, @@ -158,11 +129,6 @@ func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandbox t.Logf("Check bind mount") checkMount(t, spec.Mounts, "host-path-1", "container-path-1", "bind", []string{"rbind", "rprivate", "rw"}, nil) checkMount(t, spec.Mounts, "host-path-2", "container-path-2", "bind", []string{"rbind", "rprivate", "ro"}, nil) - checkMount(t, spec.Mounts, "host-path-3", "container-path-3", "bind", []string{"rbind", "rprivate", "rw"}, nil) - checkMount(t, spec.Mounts, "host-path-4", "container-path-4", "bind", []string{"rbind", "rslave", "rw"}, nil) - checkMount(t, spec.Mounts, "host-path-5", "container-path-5", "bind", []string{"rbind", "rshared", "rw"}, nil) - checkMount(t, spec.Mounts, "host-path-6", "container-path-6", "bind", []string{"rbind", "rprivate", "rw"}, nil) - checkMount(t, spec.Mounts, "host-path-7", "container-path-7", "bind", []string{"rbind", "rshared", "ro"}, nil) t.Logf("Check resource limits") assert.EqualValues(t, *spec.Linux.Resources.CPU.Period, 100) @@ -610,6 +576,113 @@ func TestPrivilegedBindMount(t *testing.T) { } } +func TestMountPropagation(t *testing.T) { + sharedGetMountsFn := func() ([]*mount.Info, error) { + return []*mount.Info{ + { + Mountpoint: "host-path", + Optional: "shared:", + }, + }, nil + } + + slaveGetMountsFn := func() ([]*mount.Info, error) { + return []*mount.Info{ + { + Mountpoint: "host-path", + Optional: "master:", + }, + }, nil + } + + othersGetMountsFn := func() ([]*mount.Info, error) { + return []*mount.Info{ + { + Mountpoint: "host-path", + Optional: "others", + }, + }, nil + } + + for desc, test := range map[string]struct { + criMount *runtime.Mount + fakeGetMountsFn func() ([]*mount.Info, error) + optionsCheck []string + expectErr bool + }{ + "HostPath should mount as 'rprivate' if propagation is MountPropagation_PROPAGATION_PRIVATE": { + criMount: &runtime.Mount{ + ContainerPath: "container-path", + HostPath: "host-path", + Propagation: runtime.MountPropagation_PROPAGATION_PRIVATE, + }, + fakeGetMountsFn: nil, + optionsCheck: []string{"rbind", "rprivate"}, + expectErr: false, + }, + "HostPath should mount as 'rslave' if propagation is MountPropagation_PROPAGATION_HOST_TO_CONTAINER": { + criMount: &runtime.Mount{ + ContainerPath: "container-path", + HostPath: "host-path", + Propagation: runtime.MountPropagation_PROPAGATION_HOST_TO_CONTAINER, + }, + fakeGetMountsFn: slaveGetMountsFn, + optionsCheck: []string{"rbind", "rslave"}, + expectErr: false, + }, + "HostPath should mount as 'rshared' if propagation is MountPropagation_PROPAGATION_BIDIRECTIONAL": { + criMount: &runtime.Mount{ + ContainerPath: "container-path", + HostPath: "host-path", + Propagation: runtime.MountPropagation_PROPAGATION_BIDIRECTIONAL, + }, + fakeGetMountsFn: sharedGetMountsFn, + optionsCheck: []string{"rbind", "rshared"}, + expectErr: false, + }, + "HostPath should mount as 'rprivate' if propagation is illegal": { + criMount: &runtime.Mount{ + ContainerPath: "container-path", + HostPath: "host-path", + Propagation: runtime.MountPropagation(42), + }, + fakeGetMountsFn: nil, + optionsCheck: []string{"rbind", "rprivate"}, + expectErr: false, + }, + "Expect an error if HostPath isn't shared and mount propagation is MountPropagation_PROPAGATION_BIDIRECTIONAL": { + criMount: &runtime.Mount{ + ContainerPath: "container-path", + HostPath: "host-path", + Propagation: runtime.MountPropagation_PROPAGATION_BIDIRECTIONAL, + }, + fakeGetMountsFn: slaveGetMountsFn, + expectErr: true, + }, + "Expect an error if HostPath isn't slave or shared and mount propagation is MountPropagation_PROPAGATION_HOST_TO_CONTAINER": { + criMount: &runtime.Mount{ + ContainerPath: "container-path", + HostPath: "host-path", + Propagation: runtime.MountPropagation_PROPAGATION_HOST_TO_CONTAINER, + }, + fakeGetMountsFn: othersGetMountsFn, + expectErr: true, + }, + } { + t.Logf("TestCase %q", desc) + g := generate.New() + c := newTestCRIContainerdService() + c.os.(*ostesting.FakeOS).GetMountsFn = test.fakeGetMountsFn + err := c.addOCIBindMounts(&g, []*runtime.Mount{test.criMount}, "") + if test.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + checkMount(t, g.Spec().Mounts, test.criMount.HostPath, test.criMount.ContainerPath, "bind", test.optionsCheck, nil) + } + } +} + func TestPidNamespace(t *testing.T) { testID := "test-id" testPid := uint32(1234) diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index 30482d702..bc4f94d91 100644 --- a/pkg/server/helpers.go +++ b/pkg/server/helpers.go @@ -29,6 +29,7 @@ import ( "github.com/containerd/containerd/content" "github.com/containerd/containerd/errdefs" "github.com/docker/distribution/reference" + "github.com/docker/docker/pkg/mount" imagedigest "github.com/opencontainers/go-digest" "github.com/opencontainers/image-spec/identity" imagespec "github.com/opencontainers/image-spec/specs-go/v1" @@ -384,3 +385,36 @@ func isInCRIMounts(dst string, mounts []*runtime.Mount) bool { } return false } + +//TODO: Replace with `mount.Lookup()`in containerd after #257 is marged +func getMountInfo(mountInfos []*mount.Info, dir string) *mount.Info { + for _, m := range mountInfos { + if m.Mountpoint == dir { + return m + } + } + return nil +} + +func getSourceMount(source string, mountInfos []*mount.Info) (string, string, error) { + mountinfo := getMountInfo(mountInfos, source) + if mountinfo != nil { + return source, mountinfo.Optional, nil + } + + path := source + for { + path = filepath.Dir(path) + mountinfo = getMountInfo(mountInfos, path) + if mountinfo != nil { + return path, mountinfo.Optional, nil + } + + if path == "/" { + break + } + } + + // If we are here, we did not find parent mount. Something is wrong. + return "", "", fmt.Errorf("Could not find source mount of %s", source) +}