Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass additional flags to subpath mount to avoid flakes in certain conditions #104253

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pkg/volume/util/subpath/subpath_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to set fstype here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about unmount, does it need to add flag?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when bind mounting, fstype is not needed

return "", fmt.Errorf("error mounting %s: %s", subpath.Path, err)
}
success = true
Expand Down
4 changes: 4 additions & 0 deletions staging/src/k8s.io/mount-utils/fake_mounter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 2 additions & 0 deletions staging/src/k8s.io/mount-utils/mount.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
35 changes: 26 additions & 9 deletions staging/src/k8s.io/mount-utils/mount_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}{
Expand All @@ -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{}{
Expand All @@ -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 := MakeMountArgsSensitiveWithMountFlags(source, target, fstype, options, sensitiveOptions, mountFlags)
if len(mounterPath) > 0 {
mountArgs = append([]string{mountCmd}, mountArgs...)
mountArgsLogStr = mountCmd + " " + mountArgsLogStr
Expand Down Expand Up @@ -217,10 +222,22 @@ func MakeMountArgs(source, target, fstype string, options []string) (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) {
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 [-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, " ")
Expand Down
48 changes: 39 additions & 9 deletions staging/src/k8s.io/mount-utils/mount_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,7 @@ func TestSensitiveMountOptions(t *testing.T) {
fstype string
options []string
sensitiveOptions []string
mountFlags []string
}{
{

Expand All @@ -455,6 +456,7 @@ func TestSensitiveMountOptions(t *testing.T) {
fstype: "myFS",
options: []string{"o1", "o2"},
sensitiveOptions: []string{"s1", "s2"},
mountFlags: []string{},
},
{

Expand All @@ -463,6 +465,7 @@ func TestSensitiveMountOptions(t *testing.T) {
fstype: "myFS",
options: []string{},
sensitiveOptions: []string{"s1", "s2"},
mountFlags: []string{},
},
{

Expand All @@ -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 := MakeMountArgsSensitiveWithMountFlags(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)
Expand All @@ -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)
}
5 changes: 5 additions & 0 deletions staging/src/k8s.io/mount-utils/mount_unsupported.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions staging/src/k8s.io/mount-utils/mount_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down