From 06e040de40fb7bb75dd156312bfa8609ae55204c Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Wed, 6 Oct 2021 08:14:33 -0400 Subject: [PATCH 1/3] Add check for subpaths --- pkg/volume/util/hostutil/hostutil_linux.go | 22 ++++++++++++++++++++++ pkg/volume/util/subpath/subpath_linux.go | 7 +++++++ 2 files changed, 29 insertions(+) diff --git a/pkg/volume/util/hostutil/hostutil_linux.go b/pkg/volume/util/hostutil/hostutil_linux.go index 03748508ba6..3ef8d22be37 100644 --- a/pkg/volume/util/hostutil/hostutil_linux.go +++ b/pkg/volume/util/hostutil/hostutil_linux.go @@ -165,6 +165,28 @@ func (hu *HostUtil) FindMountInfo(path string) (mount.MountInfo, error) { return findMountInfo(path, procMountInfoPath) } +// FindExactMountInfo returns exact mount point that matches given path +func (hu *HostUtil) FindExactMountInfo(path string) (mount.MountInfo, error) { + infos, err := mount.ParseMountInfo(procMountInfoPath) + if err != nil { + return mount.MountInfo{}, err + } + + // process /proc/xxx/mountinfo in backward order and find the first mount + // point that is prefix of 'path' - that's the mount where path resides + var info *mount.MountInfo + for i := len(infos) - 1; i >= 0; i-- { + if path == infos[i].MountPoint { + info = &infos[i] + break + } + } + if info == nil { + return mount.MountInfo{}, fmt.Errorf("cannot find mount point for %q", path) + } + return *info, nil +} + // isShared returns true, if given path is on a mount point that has shared // mount propagation. func isShared(mount string, mountInfoPath string) (bool, error) { diff --git a/pkg/volume/util/subpath/subpath_linux.go b/pkg/volume/util/subpath/subpath_linux.go index 84661ac1c59..70a828db210 100644 --- a/pkg/volume/util/subpath/subpath_linux.go +++ b/pkg/volume/util/subpath/subpath_linux.go @@ -116,6 +116,13 @@ func prepareSubpathTarget(mounter mount.Interface, subpath Subpath) (bool, strin return false, "", fmt.Errorf("error calling findMountInfo for %s: %s", bindPathTarget, err) } if mntInfo.Root != subpath.Path { + volumeMountInfo, err := linuxHostUtil.FindExactMountInfo(subpath.VolumePath) + if err == nil && mount.PathWithinBase(subpath.Path, subpath.VolumePath) && + (volumeMountInfo.Major == mntInfo.Major && volumeMountInfo.Minor == mntInfo.Minor) { + klog.V(5).Infof("Skipping bind-mounting subpath %s, volumePath: %s, path: %s", bindPathTarget, subpath.VolumePath, subpath.Path) + return true, bindPathTarget, nil + } + // It's already mounted but not what we want, unmount it if err = mounter.Unmount(bindPathTarget); err != nil { return false, "", fmt.Errorf("error ummounting %s: %s", bindPathTarget, err) From 7a73168a5964e87543ca13d4a9dc5e06c672539a Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Tue, 9 Nov 2021 13:29:58 -0500 Subject: [PATCH 2/3] Check subpath file --- pkg/volume/util/subpath/subpath_linux.go | 33 +++++++++++++++--------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/pkg/volume/util/subpath/subpath_linux.go b/pkg/volume/util/subpath/subpath_linux.go index 70a828db210..3e38f904fd8 100644 --- a/pkg/volume/util/subpath/subpath_linux.go +++ b/pkg/volume/util/subpath/subpath_linux.go @@ -30,7 +30,6 @@ import ( "golang.org/x/sys/unix" "k8s.io/klog/v2" - "k8s.io/kubernetes/pkg/volume/util/hostutil" "k8s.io/mount-utils" ) @@ -110,19 +109,12 @@ func prepareSubpathTarget(mounter mount.Interface, subpath Subpath) (bool, strin notMount = true } if !notMount { - linuxHostUtil := hostutil.NewHostUtil() - mntInfo, err := linuxHostUtil.FindMountInfo(bindPathTarget) + // It's already mounted, so check if it's bind-mounted to the same path + samePath, err := checkSubPathFileEqual(subpath, bindPathTarget) if err != nil { - return false, "", fmt.Errorf("error calling findMountInfo for %s: %s", bindPathTarget, err) + return false, "", fmt.Errorf("error checking subpath mount info for %s: %s", bindPathTarget, err) } - if mntInfo.Root != subpath.Path { - volumeMountInfo, err := linuxHostUtil.FindExactMountInfo(subpath.VolumePath) - if err == nil && mount.PathWithinBase(subpath.Path, subpath.VolumePath) && - (volumeMountInfo.Major == mntInfo.Major && volumeMountInfo.Minor == mntInfo.Minor) { - klog.V(5).Infof("Skipping bind-mounting subpath %s, volumePath: %s, path: %s", bindPathTarget, subpath.VolumePath, subpath.Path) - return true, bindPathTarget, nil - } - + if !samePath { // It's already mounted but not what we want, unmount it if err = mounter.Unmount(bindPathTarget); err != nil { return false, "", fmt.Errorf("error ummounting %s: %s", bindPathTarget, err) @@ -163,6 +155,23 @@ func prepareSubpathTarget(mounter mount.Interface, subpath Subpath) (bool, strin return false, bindPathTarget, nil } +func checkSubPathFileEqual(subpath Subpath, bindMountTarget string) (bool, error) { + s, err := os.Stat(subpath.Path) + if err != nil { + return false, fmt.Errorf("stat %s failed: %s", subpath.Path, err) + } + + t, err := os.Lstat(bindMountTarget) + if err != nil { + return false, fmt.Errorf("lstat %s failed: %s", bindMountTarget, err) + } + + if !os.SameFile(s, t) { + return false, nil + } + return true, nil +} + func getSubpathBindTarget(subpath Subpath) string { // containerName is DNS label, i.e. safe as a directory name. return filepath.Join(subpath.PodDir, containerSubPathDirectoryName, subpath.VolumeName, subpath.ContainerName, strconv.Itoa(subpath.VolumeMountIndex)) From 467bcd8b89ecbdb917148cfc4ff9f1d9b8e0e21d Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Tue, 9 Nov 2021 15:43:03 -0500 Subject: [PATCH 3/3] Add tests for checking bind mounts --- pkg/volume/util/hostutil/hostutil_linux.go | 22 --- pkg/volume/util/subpath/subpath_linux.go | 2 +- pkg/volume/util/subpath/subpath_linux_test.go | 137 ++++++++++++++++++ 3 files changed, 138 insertions(+), 23 deletions(-) diff --git a/pkg/volume/util/hostutil/hostutil_linux.go b/pkg/volume/util/hostutil/hostutil_linux.go index 3ef8d22be37..03748508ba6 100644 --- a/pkg/volume/util/hostutil/hostutil_linux.go +++ b/pkg/volume/util/hostutil/hostutil_linux.go @@ -165,28 +165,6 @@ func (hu *HostUtil) FindMountInfo(path string) (mount.MountInfo, error) { return findMountInfo(path, procMountInfoPath) } -// FindExactMountInfo returns exact mount point that matches given path -func (hu *HostUtil) FindExactMountInfo(path string) (mount.MountInfo, error) { - infos, err := mount.ParseMountInfo(procMountInfoPath) - if err != nil { - return mount.MountInfo{}, err - } - - // process /proc/xxx/mountinfo in backward order and find the first mount - // point that is prefix of 'path' - that's the mount where path resides - var info *mount.MountInfo - for i := len(infos) - 1; i >= 0; i-- { - if path == infos[i].MountPoint { - info = &infos[i] - break - } - } - if info == nil { - return mount.MountInfo{}, fmt.Errorf("cannot find mount point for %q", path) - } - return *info, nil -} - // isShared returns true, if given path is on a mount point that has shared // mount propagation. func isShared(mount string, mountInfoPath string) (bool, error) { diff --git a/pkg/volume/util/subpath/subpath_linux.go b/pkg/volume/util/subpath/subpath_linux.go index 3e38f904fd8..7da8ca10a95 100644 --- a/pkg/volume/util/subpath/subpath_linux.go +++ b/pkg/volume/util/subpath/subpath_linux.go @@ -156,7 +156,7 @@ func prepareSubpathTarget(mounter mount.Interface, subpath Subpath) (bool, strin } func checkSubPathFileEqual(subpath Subpath, bindMountTarget string) (bool, error) { - s, err := os.Stat(subpath.Path) + s, err := os.Lstat(subpath.Path) if err != nil { return false, fmt.Errorf("stat %s failed: %s", subpath.Path, err) } diff --git a/pkg/volume/util/subpath/subpath_linux_test.go b/pkg/volume/util/subpath/subpath_linux_test.go index c1dbdf24681..86f24da11ff 100644 --- a/pkg/volume/util/subpath/subpath_linux_test.go +++ b/pkg/volume/util/subpath/subpath_linux_test.go @@ -921,6 +921,143 @@ func TestBindSubPath(t *testing.T) { } } +func TestSubpath_PrepareSafeSubpath(t *testing.T) { + //complete code + defaultPerm := os.FileMode(0750) + + tests := []struct { + name string + // Function that prepares directory structure for the test under given + // base. + prepare func(base string) ([]string, string, string, error) + expectError bool + expectAction []mount.FakeAction + mountExists bool + }{ + { + name: "subpath-mount-already-exists-with-mismatching-mount", + prepare: func(base string) ([]string, string, string, error) { + volpath, subpathMount := getTestPaths(base) + mounts := []string{subpathMount} + if err := os.MkdirAll(subpathMount, defaultPerm); err != nil { + return nil, "", "", err + } + + subpath := filepath.Join(volpath, "dir0") + return mounts, volpath, subpath, os.MkdirAll(subpath, defaultPerm) + }, + expectError: false, + expectAction: []mount.FakeAction{{Action: "unmount"}}, + mountExists: false, + }, + { + name: "subpath-mount-already-exists-with-samefile", + prepare: func(base string) ([]string, string, string, error) { + volpath, subpathMount := getTestPaths(base) + mounts := []string{subpathMount} + subpathMountRoot := filepath.Dir(subpathMount) + + if err := os.MkdirAll(subpathMountRoot, defaultPerm); err != nil { + return nil, "", "", err + } + targetFile, err := os.Create(subpathMount) + if err != nil { + return nil, "", "", err + } + defer targetFile.Close() + + if err := os.MkdirAll(volpath, defaultPerm); err != nil { + return nil, "", "", err + } + subpath := filepath.Join(volpath, "file0") + // using hard link to simulate bind mounts + err = os.Link(subpathMount, subpath) + if err != nil { + return nil, "", "", err + } + return mounts, volpath, subpath, nil + }, + expectError: false, + expectAction: []mount.FakeAction{}, + mountExists: true, + }, + } + for _, test := range tests { + klog.V(4).Infof("test %q", test.name) + base, err := ioutil.TempDir("", "bind-subpath-"+test.name+"-") + if err != nil { + t.Fatalf(err.Error()) + } + defer os.RemoveAll(base) + + mounts, volPath, subPath, err := test.prepare(base) + if err != nil { + os.RemoveAll(base) + t.Fatalf("failed to prepare test %q: %v", test.name, err.Error()) + } + + fm := setupFakeMounter(mounts) + + subpath := Subpath{ + VolumeMountIndex: testSubpath, + Path: subPath, + VolumeName: testVol, + VolumePath: volPath, + PodDir: filepath.Join(base, "pod0"), + ContainerName: testContainer, + } + + _, subpathMount := getTestPaths(base) + bindMountExists, bindPathTarget, err := prepareSubpathTarget(fm, subpath) + + if bindMountExists != test.mountExists { + t.Errorf("test %q failed: expected bindMountExists %v, got %v", test.name, test.mountExists, bindMountExists) + } + + logActions := fm.GetLog() + if len(test.expectAction) == 0 && len(logActions) > 0 { + t.Errorf("test %q failed: expected no actions, got %v", test.name, logActions) + } + + if len(test.expectAction) > 0 { + foundMatchingAction := false + testAction := test.expectAction[0] + for _, action := range logActions { + if action.Action == testAction.Action { + foundMatchingAction = true + break + } + } + if !foundMatchingAction { + t.Errorf("test %q failed: expected action %q, got %v", test.name, testAction.Action, logActions) + } + } + + if test.expectError { + if err == nil { + t.Errorf("test %q failed: expected error, got success", test.name) + } + if bindPathTarget != "" { + t.Errorf("test %q failed: expected empty bindPathTarget, got %v", test.name, bindPathTarget) + } + if err = validateDirNotExists(subpathMount); err != nil { + t.Errorf("test %q failed: %v", test.name, err) + } + } + if !test.expectError { + if err != nil { + t.Errorf("test %q failed: %v", test.name, err) + } + if bindPathTarget != subpathMount { + t.Errorf("test %q failed: expected bindPathTarget %v, got %v", test.name, subpathMount, bindPathTarget) + } + if err = validateFileExists(subpathMount); err != nil { + t.Errorf("test %q failed: %v", test.name, err) + } + } + } +} + func TestSafeOpen(t *testing.T) { defaultPerm := os.FileMode(0750)