From 296b30f14367a42d43f25ad0774d10be55b49f4d Mon Sep 17 00:00:00 2001 From: Mauricio Poppe Date: Thu, 5 Aug 2021 22:31:38 +0000 Subject: [PATCH 1/4] Pass additional flags to subpath mount to avoid flakes in certain conditions --- pkg/volume/util/subpath/subpath_linux.go | 3 +- .../src/k8s.io/mount-utils/fake_mounter.go | 4 +++ staging/src/k8s.io/mount-utils/mount.go | 2 ++ staging/src/k8s.io/mount-utils/mount_linux.go | 31 ++++++++++++------- .../src/k8s.io/mount-utils/mount_windows.go | 6 ++++ 5 files changed, 34 insertions(+), 12 deletions(-) diff --git a/pkg/volume/util/subpath/subpath_linux.go b/pkg/volume/util/subpath/subpath_linux.go index 1140f75ce5d..84cdf5e1051 100644 --- a/pkg/volume/util/subpath/subpath_linux.go +++ b/pkg/volume/util/subpath/subpath_linux.go @@ -209,8 +209,9 @@ func doBindSubPath(mounter mount.Interface, subpath Subpath) (hostPath string, e // Do the bind mount options := []string{"bind"} + mountFlags := []string{"--no-canonicalize"} klog.V(5).Infof("bind mounting %q at %q", mountSource, bindPathTarget) - if err = mounter.MountSensitiveWithoutSystemd(mountSource, bindPathTarget, "" /*fstype*/, options, nil); err != nil { + if err = mounter.MountSensitiveWithoutSystemdWithMountFlags(mountSource, bindPathTarget, "" /*fstype*/, options, nil /* sensitiveOptions */, mountFlags); err != nil { return "", fmt.Errorf("error mounting %s: %s", subpath.Path, err) } success = true diff --git a/staging/src/k8s.io/mount-utils/fake_mounter.go b/staging/src/k8s.io/mount-utils/fake_mounter.go index 393ed043ba0..55ea5e2986b 100644 --- a/staging/src/k8s.io/mount-utils/fake_mounter.go +++ b/staging/src/k8s.io/mount-utils/fake_mounter.go @@ -136,6 +136,10 @@ func (f *FakeMounter) MountSensitiveWithoutSystemd(source string, target string, return f.MountSensitive(source, target, fstype, options, nil /* sensitiveOptions */) } +func (f *FakeMounter) MountSensitiveWithoutSystemdWithMountFlags(source string, target string, fstype string, options []string, sensitiveOptions []string, mountFlags []string) error { + return f.MountSensitive(source, target, fstype, options, nil /* sensitiveOptions */) +} + // Unmount records the unmount event and updates the in-memory mount points for FakeMounter func (f *FakeMounter) Unmount(target string) error { f.mutex.Lock() diff --git a/staging/src/k8s.io/mount-utils/mount.go b/staging/src/k8s.io/mount-utils/mount.go index 93b60d3f922..a882fcc7399 100644 --- a/staging/src/k8s.io/mount-utils/mount.go +++ b/staging/src/k8s.io/mount-utils/mount.go @@ -49,6 +49,8 @@ type Interface interface { MountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error // MountSensitiveWithoutSystemd is the same as MountSensitive() but this method disable using systemd mount. MountSensitiveWithoutSystemd(source string, target string, fstype string, options []string, sensitiveOptions []string) error + // MountSensitiveWithoutSystemdWithMountFlags is the same as MountSensitiveWithoutSystemd() with additional mount flags + MountSensitiveWithoutSystemdWithMountFlags(source string, target string, fstype string, options []string, sensitiveOptions []string, mountFlags []string) error // Unmount unmounts given target. Unmount(target string) error // List returns a list of all mounted filesystems. This can be large. diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index 10a1c3f0106..e62b344134e 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -87,11 +87,11 @@ func (mounter *Mounter) MountSensitive(source string, target string, fstype stri mounterPath := "" bind, bindOpts, bindRemountOpts, bindRemountOptsSensitive := MakeBindOptsSensitive(options, sensitiveOptions) if bind { - err := mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindOpts, bindRemountOptsSensitive, true) + err := mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindOpts, bindRemountOptsSensitive, nil /* mountFlags */, true) if err != nil { return err } - return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindRemountOpts, bindRemountOptsSensitive, true) + return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindRemountOpts, bindRemountOptsSensitive, nil /* mountFlags */, true) } // The list of filesystems that require containerized mounter on GCI image cluster fsTypesNeedMounter := map[string]struct{}{ @@ -103,19 +103,24 @@ func (mounter *Mounter) MountSensitive(source string, target string, fstype stri if _, ok := fsTypesNeedMounter[fstype]; ok { mounterPath = mounter.mounterPath } - return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, options, sensitiveOptions, true) + return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, options, sensitiveOptions, nil /* mountFlags */, true) } // MountSensitiveWithoutSystemd is the same as MountSensitive() but disable using systemd mount. func (mounter *Mounter) MountSensitiveWithoutSystemd(source string, target string, fstype string, options []string, sensitiveOptions []string) error { + return mounter.MountSensitiveWithoutSystemdWithMountFlags(source, target, fstype, options, sensitiveOptions, nil /* mountFlags */) +} + +// MountSensitiveWithoutSystemdWithMountFlags is the same as MountSensitiveWithoutSystemd with additional mount flags. +func (mounter *Mounter) MountSensitiveWithoutSystemdWithMountFlags(source string, target string, fstype string, options []string, sensitiveOptions []string, mountFlags []string) error { mounterPath := "" bind, bindOpts, bindRemountOpts, bindRemountOptsSensitive := MakeBindOptsSensitive(options, sensitiveOptions) if bind { - err := mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindOpts, bindRemountOptsSensitive, false) + err := mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindOpts, bindRemountOptsSensitive, mountFlags, false) if err != nil { return err } - return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindRemountOpts, bindRemountOptsSensitive, false) + return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindRemountOpts, bindRemountOptsSensitive, mountFlags, false) } // The list of filesystems that require containerized mounter on GCI image cluster fsTypesNeedMounter := map[string]struct{}{ @@ -127,14 +132,14 @@ func (mounter *Mounter) MountSensitiveWithoutSystemd(source string, target strin if _, ok := fsTypesNeedMounter[fstype]; ok { mounterPath = mounter.mounterPath } - return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, options, sensitiveOptions, false) + return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, options, sensitiveOptions, mountFlags, false) } // doMount runs the mount command. mounterPath is the path to mounter binary if containerized mounter is used. // sensitiveOptions is an extension of options except they will not be logged (because they may contain sensitive material) // systemdMountRequired is an extension of option to decide whether uses systemd mount. -func (mounter *Mounter) doMount(mounterPath string, mountCmd string, source string, target string, fstype string, options []string, sensitiveOptions []string, systemdMountRequired bool) error { - mountArgs, mountArgsLogStr := MakeMountArgsSensitive(source, target, fstype, options, sensitiveOptions) +func (mounter *Mounter) doMount(mounterPath string, mountCmd string, source string, target string, fstype string, options []string, sensitiveOptions []string, mountFlags []string, systemdMountRequired bool) error { + mountArgs, mountArgsLogStr := MakeMountArgsSensitive(source, target, fstype, options, sensitiveOptions, mountFlags) if len(mounterPath) > 0 { mountArgs = append([]string{mountCmd}, mountArgs...) mountArgsLogStr = mountCmd + " " + mountArgsLogStr @@ -210,17 +215,21 @@ func detectSystemd() bool { // MakeMountArgs makes the arguments to the mount(8) command. // options MUST not contain sensitive material (like passwords). func MakeMountArgs(source, target, fstype string, options []string) (mountArgs []string) { - mountArgs, _ = MakeMountArgsSensitive(source, target, fstype, options, nil /* sensitiveOptions */) + mountArgs, _ = MakeMountArgsSensitive(source, target, fstype, options, nil /* sensitiveOptions */, nil /* mountFlags */) return mountArgs } // MakeMountArgsSensitive makes the arguments to the mount(8) command. // sensitiveOptions is an extension of options except they will not be logged (because they may contain sensitive material) -func MakeMountArgsSensitive(source, target, fstype string, options []string, sensitiveOptions []string) (mountArgs []string, mountArgsLogStr string) { +func MakeMountArgsSensitive(source, target, fstype string, options []string, sensitiveOptions []string, mountFlags []string) (mountArgs []string, mountArgsLogStr string) { // Build mount command as follows: - // mount [-t $fstype] [-o $options] [$source] $target + // mount [--$mountFlags] [-t $fstype] [-o $options] [$source] $target mountArgs = []string{} mountArgsLogStr = "" + + mountArgs = append(mountArgs, mountFlags...) + mountArgsLogStr += strings.Join(mountFlags, " ") + if len(fstype) > 0 { mountArgs = append(mountArgs, "-t", fstype) mountArgsLogStr += strings.Join(mountArgs, " ") diff --git a/staging/src/k8s.io/mount-utils/mount_windows.go b/staging/src/k8s.io/mount-utils/mount_windows.go index 3706b38fefd..0d1e99fa5d3 100644 --- a/staging/src/k8s.io/mount-utils/mount_windows.go +++ b/staging/src/k8s.io/mount-utils/mount_windows.go @@ -64,6 +64,12 @@ func (mounter *Mounter) MountSensitiveWithoutSystemd(source string, target strin return mounter.MountSensitive(source, target, fstype, options, sensitiveOptions /* sensitiveOptions */) } +// MountSensitiveWithoutSystemdWithMountFlags is the same as MountSensitiveWithoutSystemd with additional mount flags +// Windows not supported systemd mount, this function degrades to MountSensitive(). +func (mounter *Mounter) MountSensitiveWithoutSystemdWithMountFlags(source string, target string, fstype string, options []string, sensitiveOptions []string, mountFlags []string) error { + return mounter.MountSensitive(source, target, fstype, options, sensitiveOptions /* sensitiveOptions */) +} + // MountSensitive is the same as Mount() but this method allows // sensitiveOptions to be passed in a separate parameter from the normal // mount options and ensures the sensitiveOptions are never logged. This From 338f8ba0bf8d4ea5ae13d25065308dd9935b214f Mon Sep 17 00:00:00 2001 From: Mauricio Poppe Date: Mon, 9 Aug 2021 22:43:14 +0000 Subject: [PATCH 2/4] Add missing interface method in mount_unsupported.go --- staging/src/k8s.io/mount-utils/mount_unsupported.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/staging/src/k8s.io/mount-utils/mount_unsupported.go b/staging/src/k8s.io/mount-utils/mount_unsupported.go index 0e8e683ae3a..d2aac9a7483 100644 --- a/staging/src/k8s.io/mount-utils/mount_unsupported.go +++ b/staging/src/k8s.io/mount-utils/mount_unsupported.go @@ -53,6 +53,11 @@ func (mounter *Mounter) MountSensitiveWithoutSystemd(source string, target strin return errUnsupported } +// MountSensitiveWithoutSystemdWithMountFlags always returns an error on unsupported platforms +func (mounter *Mounter) MountSensitiveWithoutSystemdWithMountFlags(source string, target string, fstype string, options []string, sensitiveOptions []string, mountFlags []string) error { + return errUnsupported +} + // Unmount always returns an error on unsupported platforms func (mounter *Mounter) Unmount(target string) error { return errUnsupported From b9b76dba6ee901d330de5dfef39b83e5acb76906 Mon Sep 17 00:00:00 2001 From: Mauricio Poppe Date: Mon, 9 Aug 2021 23:46:50 +0000 Subject: [PATCH 3/4] Update the unit tests to handle mountFlags --- .../k8s.io/mount-utils/mount_linux_test.go | 48 +++++++++++++++---- 1 file changed, 39 insertions(+), 9 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux_test.go b/staging/src/k8s.io/mount-utils/mount_linux_test.go index dc7d4fd882f..2af754e5901 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux_test.go +++ b/staging/src/k8s.io/mount-utils/mount_linux_test.go @@ -447,6 +447,7 @@ func TestSensitiveMountOptions(t *testing.T) { fstype string options []string sensitiveOptions []string + mountFlags []string }{ { @@ -455,6 +456,7 @@ func TestSensitiveMountOptions(t *testing.T) { fstype: "myFS", options: []string{"o1", "o2"}, sensitiveOptions: []string{"s1", "s2"}, + mountFlags: []string{}, }, { @@ -463,6 +465,7 @@ func TestSensitiveMountOptions(t *testing.T) { fstype: "myFS", options: []string{}, sensitiveOptions: []string{"s1", "s2"}, + mountFlags: []string{}, }, { @@ -471,26 +474,44 @@ func TestSensitiveMountOptions(t *testing.T) { fstype: "myFS", options: []string{"o1", "o2"}, sensitiveOptions: []string{}, + mountFlags: []string{}, + }, + { + + source: "mySrc", + target: "myTarget", + fstype: "myFS", + options: []string{"o1", "o2"}, + sensitiveOptions: []string{"s1", "s2"}, + mountFlags: []string{"--no-canonicalize"}, }, } for _, v := range testcases { // Act - mountArgs, mountArgsLogStr := MakeMountArgsSensitive(v.source, v.target, v.fstype, v.options, v.sensitiveOptions) + mountArgs, mountArgsLogStr := MakeMountArgsSensitive(v.source, v.target, v.fstype, v.options, v.sensitiveOptions, v.mountFlags) // Assert t.Logf("\r\nmountArgs =%q\r\nmountArgsLogStr=%q", mountArgs, mountArgsLogStr) + for _, mountFlag := range v.mountFlags { + if found := mountArgsContainString(t, mountArgs, mountFlag); !found { + t.Errorf("Expected mountFlag (%q) to exist in returned mountArgs (%q), but it does not", mountFlag, mountArgs) + } + if !strings.Contains(mountArgsLogStr, mountFlag) { + t.Errorf("Expected mountFlag (%q) to exist in returned mountArgsLogStr (%q), but it does", mountFlag, mountArgsLogStr) + } + } for _, option := range v.options { - if found := contains(mountArgs, option, t); !found { - t.Errorf("Expected option (%q) to exist in returned mountArts (%q), but it does not", option, mountArgs) + if found := mountArgsContainOption(t, mountArgs, option); !found { + t.Errorf("Expected option (%q) to exist in returned mountArgs (%q), but it does not", option, mountArgs) } if !strings.Contains(mountArgsLogStr, option) { t.Errorf("Expected option (%q) to exist in returned mountArgsLogStr (%q), but it does", option, mountArgsLogStr) } } for _, sensitiveOption := range v.sensitiveOptions { - if found := contains(mountArgs, sensitiveOption, t); !found { - t.Errorf("Expected sensitiveOption (%q) to exist in returned mountArts (%q), but it does not", sensitiveOption, mountArgs) + if found := mountArgsContainOption(t, mountArgs, sensitiveOption); !found { + t.Errorf("Expected sensitiveOption (%q) to exist in returned mountArgs (%q), but it does not", sensitiveOption, mountArgs) } if strings.Contains(mountArgsLogStr, sensitiveOption) { t.Errorf("Expected sensitiveOption (%q) to not exist in returned mountArgsLogStr (%q), but it does", sensitiveOption, mountArgsLogStr) @@ -499,18 +520,27 @@ func TestSensitiveMountOptions(t *testing.T) { } } -func contains(slice []string, str string, t *testing.T) bool { +func mountArgsContainString(t *testing.T, mountArgs []string, wanted string) bool { + for _, mountArg := range mountArgs { + if mountArg == wanted { + return true + } + } + return false +} + +func mountArgsContainOption(t *testing.T, mountArgs []string, option string) bool { optionsIndex := -1 - for i, s := range slice { + for i, s := range mountArgs { if s == "-o" { optionsIndex = i + 1 break } } - if optionsIndex < 0 || optionsIndex >= len(slice) { + if optionsIndex < 0 || optionsIndex >= len(mountArgs) { return false } - return strings.Contains(slice[optionsIndex], str) + return strings.Contains(mountArgs[optionsIndex], option) } From 08bec6da0fcf418f351f86c113620edc7be1849c Mon Sep 17 00:00:00 2001 From: Mauricio Poppe Date: Tue, 10 Aug 2021 17:31:10 +0000 Subject: [PATCH 4/4] Keep MakeMountArgSensitive and add a new signature that receives flags --- staging/src/k8s.io/mount-utils/mount_linux.go | 16 ++++++++++++---- .../src/k8s.io/mount-utils/mount_linux_test.go | 2 +- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index e62b344134e..7097eae0876 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -139,7 +139,7 @@ func (mounter *Mounter) MountSensitiveWithoutSystemdWithMountFlags(source string // sensitiveOptions is an extension of options except they will not be logged (because they may contain sensitive material) // systemdMountRequired is an extension of option to decide whether uses systemd mount. func (mounter *Mounter) doMount(mounterPath string, mountCmd string, source string, target string, fstype string, options []string, sensitiveOptions []string, mountFlags []string, systemdMountRequired bool) error { - mountArgs, mountArgsLogStr := MakeMountArgsSensitive(source, target, fstype, options, sensitiveOptions, mountFlags) + mountArgs, mountArgsLogStr := MakeMountArgsSensitiveWithMountFlags(source, target, fstype, options, sensitiveOptions, mountFlags) if len(mounterPath) > 0 { mountArgs = append([]string{mountCmd}, mountArgs...) mountArgsLogStr = mountCmd + " " + mountArgsLogStr @@ -215,15 +215,23 @@ func detectSystemd() bool { // MakeMountArgs makes the arguments to the mount(8) command. // options MUST not contain sensitive material (like passwords). func MakeMountArgs(source, target, fstype string, options []string) (mountArgs []string) { - mountArgs, _ = MakeMountArgsSensitive(source, target, fstype, options, nil /* sensitiveOptions */, nil /* mountFlags */) + mountArgs, _ = MakeMountArgsSensitive(source, target, fstype, options, nil /* sensitiveOptions */) return mountArgs } // MakeMountArgsSensitive makes the arguments to the mount(8) command. // sensitiveOptions is an extension of options except they will not be logged (because they may contain sensitive material) -func MakeMountArgsSensitive(source, target, fstype string, options []string, sensitiveOptions []string, mountFlags []string) (mountArgs []string, mountArgsLogStr string) { +func MakeMountArgsSensitive(source, target, fstype string, options []string, sensitiveOptions []string) (mountArgs []string, mountArgsLogStr string) { + return MakeMountArgsSensitiveWithMountFlags(source, target, fstype, options, sensitiveOptions, nil /* mountFlags */) +} + +// MakeMountArgsSensitiveWithMountFlags makes the arguments to the mount(8) command. +// sensitiveOptions is an extension of options except they will not be logged (because they may contain sensitive material) +// mountFlags are additional mount flags that are not related with the fstype +// and mount options +func MakeMountArgsSensitiveWithMountFlags(source, target, fstype string, options []string, sensitiveOptions []string, mountFlags []string) (mountArgs []string, mountArgsLogStr string) { // Build mount command as follows: - // mount [--$mountFlags] [-t $fstype] [-o $options] [$source] $target + // mount [$mountFlags] [-t $fstype] [-o $options] [$source] $target mountArgs = []string{} mountArgsLogStr = "" diff --git a/staging/src/k8s.io/mount-utils/mount_linux_test.go b/staging/src/k8s.io/mount-utils/mount_linux_test.go index 2af754e5901..b212e879dc8 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux_test.go +++ b/staging/src/k8s.io/mount-utils/mount_linux_test.go @@ -489,7 +489,7 @@ func TestSensitiveMountOptions(t *testing.T) { for _, v := range testcases { // Act - mountArgs, mountArgsLogStr := MakeMountArgsSensitive(v.source, v.target, v.fstype, v.options, v.sensitiveOptions, v.mountFlags) + mountArgs, mountArgsLogStr := MakeMountArgsSensitiveWithMountFlags(v.source, v.target, v.fstype, v.options, v.sensitiveOptions, v.mountFlags) // Assert t.Logf("\r\nmountArgs =%q\r\nmountArgsLogStr=%q", mountArgs, mountArgsLogStr)