Merge pull request #62903 from cofyc/fixfsgroupcheckinlocal

pkg/kubelet/cm/container_manager_linux_test.go

@@ -19,6 +19,7 @@ limitations under the License.
 package cm
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -107,6 +108,14 @@ func (mi *fakeMountInterface) SafeMakeDir(_, _ string, _ os.FileMode) error {
 	return nil
 }
 
+func (mi *fakeMountInterface) GetMountRefs(pathname string) ([]string, error) {
+	return nil, errors.New("not implemented")
+}
+
+func (mi *fakeMountInterface) GetFSGroup(pathname string) (int64, error) {
+	return -1, errors.New("not implemented")
+}
+
 func fakeContainerMgrMountInt() mount.Interface {
 	return &fakeMountInterface{
 		[]mount.MountPoint{

pkg/util/mount/exec_mount.go

@@ -151,3 +151,11 @@ func (m *execMounter) CleanSubPaths(podDir string, volumeName string) error {
 func (m *execMounter) SafeMakeDir(pathname string, base string, perm os.FileMode) error {
 	return m.wrappedMounter.SafeMakeDir(pathname, base, perm)
 }
+
+func (m *execMounter) GetMountRefs(pathname string) ([]string, error) {
+	return m.wrappedMounter.GetMountRefs(pathname)
+}
+
+func (m *execMounter) GetFSGroup(pathname string) (int64, error) {
+	return m.wrappedMounter.GetFSGroup(pathname)
+}

pkg/util/mount/exec_mount_test.go

@@ -19,6 +19,7 @@ limitations under the License.
 package mount
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"reflect"
@@ -163,3 +164,11 @@ func (fm *fakeMounter) CleanSubPaths(podDir string, volumeName string) error {
 func (fm *fakeMounter) SafeMakeDir(pathname string, base string, perm os.FileMode) error {
 	return nil
 }
+
+func (fm *fakeMounter) GetMountRefs(pathname string) ([]string, error) {
+	return nil, errors.New("not implemented")
+}
+
+func (fm *fakeMounter) GetFSGroup(pathname string) (int64, error) {
+	return -1, errors.New("not implemented")
+}

pkg/util/mount/exec_mount_unsupported.go

@@ -98,3 +98,11 @@ func (mounter *execMounter) CleanSubPaths(podDir string, volumeName string) erro
 func (mounter *execMounter) SafeMakeDir(pathname string, base string, perm os.FileMode) error {
 	return nil
 }
+
+func (mounter *execMounter) GetMountRefs(pathname string) ([]string, error) {
+	return nil, errors.New("not implemented")
+}
+
+func (mounter *execMounter) GetFSGroup(pathname string) (int64, error) {
+	return -1, errors.New("not implemented")
+}

pkg/util/mount/fake.go

@@ -17,6 +17,7 @@ limitations under the License.
 package mount
 
 import (
+	"errors"
 	"os"
 	"path/filepath"
 	"sync"
@@ -208,3 +209,16 @@ func (f *FakeMounter) CleanSubPaths(podDir string, volumeName string) error {
 func (mounter *FakeMounter) SafeMakeDir(pathname string, base string, perm os.FileMode) error {
 	return nil
 }
+
+func (f *FakeMounter) GetMountRefs(pathname string) ([]string, error) {
+	realpath, err := filepath.EvalSymlinks(pathname)
+	if err != nil {
+		// Ignore error in FakeMounter, because we actually didn't create files.
+		realpath = pathname
+	}
+	return getMountRefsByDev(f, realpath)
+}
+
+func (f *FakeMounter) GetFSGroup(pathname string) (int64, error) {
+	return -1, errors.New("GetFSGroup not implemented")
+}

pkg/util/mount/mount.go

@@ -108,6 +108,12 @@ type Interface interface {
 	// subpath starts. On the other hand, Interface.CleanSubPaths must be called
 	// when the pod finishes.
 	PrepareSafeSubpath(subPath Subpath) (newHostPath string, cleanupAction func(), err error)
+	// GetMountRefs finds all mount references to the path, returns a
+	// list of paths. Path could be a mountpoint path, device or a normal
+	// directory (for bind mount).
+	GetMountRefs(pathname string) ([]string, error)
+	// GetFSGroup returns FSGroup of the path.
+	GetFSGroup(pathname string) (int64, error)
 }
 
 type Subpath struct {
@@ -166,22 +172,19 @@ func (mounter *SafeFormatAndMount) FormatAndMount(source string, target string,
 	return mounter.formatAndMount(source, target, fstype, options)
 }
 
-// GetMountRefsByDev finds all references to the device provided
+// getMountRefsByDev finds all references to the device provided
 // by mountPath; returns a list of paths.
-func GetMountRefsByDev(mounter Interface, mountPath string) ([]string, error) {
+// Note that mountPath should be path after the evaluation of any symblolic links.
+func getMountRefsByDev(mounter Interface, mountPath string) ([]string, error) {
 	mps, err := mounter.List()
 	if err != nil {
 		return nil, err
 	}
-	slTarget, err := filepath.EvalSymlinks(mountPath)
-	if err != nil {
-		slTarget = mountPath
-	}
 
 	// Finding the device mounted to mountPath
 	diskDev := ""
 	for i := range mps {
-		if slTarget == mps[i].Path {
+		if mountPath == mps[i].Path {
 			diskDev = mps[i].Device
 			break
 		}
@@ -190,8 +193,8 @@ func GetMountRefsByDev(mounter Interface, mountPath string) ([]string, error) {
 	// Find all references to the device.
 	var refs []string
 	for i := range mps {
-		if mps[i].Device == diskDev || mps[i].Device == slTarget {
-			if mps[i].Path != slTarget {
+		if mps[i].Device == diskDev || mps[i].Device == mountPath {
+			if mps[i].Path != mountPath {
 				refs = append(refs, mps[i].Path)
 			}
 		}

pkg/util/mount/mount_linux.go

@@ -921,6 +921,31 @@ func (mounter *Mounter) SafeMakeDir(pathname string, base string, perm os.FileMo
 	return doSafeMakeDir(pathname, base, perm)
 }
 
+func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
+	realpath, err := filepath.EvalSymlinks(pathname)
+	if err != nil {
+		return nil, err
+	}
+	return getMountRefsByDev(mounter, realpath)
+}
+
+func (mounter *Mounter) GetFSGroup(pathname string) (int64, error) {
+	realpath, err := filepath.EvalSymlinks(pathname)
+	if err != nil {
+		return 0, err
+	}
+	return getFSGroup(realpath)
+}
+
+// This implementation is shared between Linux and NsEnterMounter
+func getFSGroup(pathname string) (int64, error) {
+	info, err := os.Stat(pathname)
+	if err != nil {
+		return 0, err
+	}
+	return int64(info.Sys().(*syscall.Stat_t).Gid), nil
+}
+
 // This implementation is shared between Linux and NsEnterMounter
 func doSafeMakeDir(pathname string, base string, perm os.FileMode) error {
 	glog.V(4).Infof("Creating directory %q within base %q", pathname, base)

pkg/util/mount/mount_linux_test.go

@@ -203,7 +203,7 @@ func TestGetMountRefsByDev(t *testing.T) {
 
 	for i, test := range tests {
 
-		if refs, err := GetMountRefsByDev(fm, test.mountPath); err != nil || !setEquivalent(test.expectedRefs, refs) {
+		if refs, err := getMountRefsByDev(fm, test.mountPath); err != nil || !setEquivalent(test.expectedRefs, refs) {
 			t.Errorf("%d. getMountRefsByDev(%q) = %v, %v; expected %v, nil", i, test.mountPath, refs, err, test.expectedRefs)
 		}
 	}

pkg/util/mount/mount_unsupported.go

@@ -126,3 +126,11 @@ func (mounter *Mounter) CleanSubPaths(podDir string, volumeName string) error {
 func (mounter *Mounter) SafeMakeDir(pathname string, base string, perm os.FileMode) error {
 	return unsupportedErr
 }
+
+func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
+	return nil, errors.New("not implemented")
+}
+
+func (mounter *Mounter) GetFSGroup(pathname string) (int64, error) {
+	return -1, errors.New("not implemented")
+}

pkg/util/mount/mount_windows.go

@@ -438,6 +438,20 @@ func getAllParentLinks(path string) ([]string, error) {
 	return links, nil
 }
 
+func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
+	realpath, err := filepath.EvalSymlinks(pathname)
+	if err != nil {
+		return nil, err
+	}
+	return getMountRefsByDev(mounter, realpath)
+}
+
+// Note that on windows, it always returns 0. We actually don't set FSGroup on
+// windows platform, see SetVolumeOwnership implementation.
+func (mounter *Mounter) GetFSGroup(pathname string) (int64, error) {
+	return 0, nil
+}
+
 // SafeMakeDir makes sure that the created directory does not escape given base directory mis-using symlinks.
 func (mounter *Mounter) SafeMakeDir(pathname string, base string, perm os.FileMode) error {
 	return doSafeMakeDir(pathname, base, perm)

pkg/util/mount/nsenter_mount.go

@@ -327,3 +327,19 @@ func (mounter *NsenterMounter) PrepareSafeSubpath(subPath Subpath) (newHostPath
 func (mounter *NsenterMounter) SafeMakeDir(pathname string, base string, perm os.FileMode) error {
 	return doSafeMakeDir(pathname, base, perm)
 }
+
+func (mounter *NsenterMounter) GetMountRefs(pathname string) ([]string, error) {
+	hostpath, err := mounter.ne.EvalSymlinks(pathname)
+	if err != nil {
+		return nil, err
+	}
+	return getMountRefsByDev(mounter, hostpath)
+}
+
+func (mounter *NsenterMounter) GetFSGroup(pathname string) (int64, error) {
+	kubeletpath, err := mounter.ne.KubeletPath(pathname)
+	if err != nil {
+		return 0, err
+	}
+	return getFSGroup(kubeletpath)
+}

pkg/util/mount/nsenter_mount_unsupported.go

@@ -98,3 +98,11 @@ func (*NsenterMounter) PrepareSafeSubpath(subPath Subpath) (newHostPath string,
 func (*NsenterMounter) CleanSubPaths(podDir string, volumeName string) error {
 	return nil
 }
+
+func (*NsenterMounter) GetMountRefs(pathname string) ([]string, error) {
+	return nil, errors.New("not implemented")
+}
+
+func (*NsenterMounter) GetFSGroup(pathname string) (int64, error) {
+	return -1, errors.New("not implemented")
+}

pkg/util/nsenter/nsenter.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"k8s.io/utils/exec"
 
@@ -124,3 +125,25 @@ func (ne *Nsenter) SupportsSystemd() (string, bool) {
 	systemdRunPath, ok := ne.paths["systemd-run"]
 	return systemdRunPath, ok && systemdRunPath != ""
 }
+
+// EvalSymlinks returns the path name on the host after evaluating symlinks on the
+// host.
+func (ne *Nsenter) EvalSymlinks(pathname string) (string, error) {
+	args := []string{"-m", pathname}
+	outBytes, err := ne.Exec("realpath", args).CombinedOutput()
+	if err != nil {
+		glog.Infof("failed to resolve symbolic links on %s: %v", pathname, err)
+		return "", err
+	}
+	return strings.TrimSpace(string(outBytes)), nil
+}
+
+// KubeletPath returns the path name that can be accessed by containerized
+// kubelet, after evaluating symlinks on the host.
+func (ne *Nsenter) KubeletPath(pathname string) (string, error) {
+	hostpath, err := ne.EvalSymlinks(pathname)
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(hostRootFsPath, hostpath), nil
+}

pkg/util/removeall/removeall_test.go

@@ -91,6 +91,14 @@ func (mounter *fakeMounter) SafeMakeDir(_, _ string, _ os.FileMode) error {
 	return nil
 }
 
+func (mounter *fakeMounter) GetMountRefs(pathname string) ([]string, error) {
+	return nil, errors.New("not implemented")
+}
+
+func (mounter *fakeMounter) GetFSGroup(pathname string) (int64, error) {
+	return -1, errors.New("not implemented")
+}
+
 func (mounter *fakeMounter) IsLikelyNotMountPoint(file string) (bool, error) {
 	name := path.Base(file)
 	if strings.HasPrefix(name, "mount") {

pkg/volume/host_path/host_path_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package host_path
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"testing"
@@ -388,6 +389,14 @@ func (fftc *fakeFileTypeChecker) SafeMakeDir(_, _ string, _ os.FileMode) error {
 	return nil
 }
 
+func (fftc *fakeFileTypeChecker) GetMountRefs(pathname string) ([]string, error) {
+	return nil, errors.New("not implemented")
+}
+
+func (fftc *fakeFileTypeChecker) GetFSGroup(pathname string) (int64, error) {
+	return -1, errors.New("not implemented")
+}
+
 func setUp() error {
 	err := os.MkdirAll("/tmp/ExistingFolder", os.FileMode(0755))
 	if err != nil {

pkg/volume/local/local.go

@@ -274,7 +274,7 @@ func (m *localVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 	if !notMnt {
 		return nil
 	}
-	refs, err := mount.GetMountRefsByDev(m.mounter, m.globalPath)
+	refs, err := m.mounter.GetMountRefs(m.globalPath)
 	if fsGroup != nil {
 		if err != nil {
 			glog.Errorf("cannot collect mounting information: %s %v", m.globalPath, err)
@@ -285,11 +285,11 @@ func (m *localVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 		refs = m.filterPodMounts(refs)
 		if len(refs) > 0 {
 			fsGroupNew := int64(*fsGroup)
-			fsGroupSame, fsGroupOld, err := volume.IsSameFSGroup(m.globalPath, fsGroupNew)
+			fsGroupOld, err := m.mounter.GetFSGroup(m.globalPath)
 			if err != nil {
 				return fmt.Errorf("failed to check fsGroup for %s (%v)", m.globalPath, err)
 			}
-			if !fsGroupSame {
+			if fsGroupNew != fsGroupOld {
 				m.plugin.recorder.Eventf(m.pod, v1.EventTypeWarning, events.WarnAlreadyMountedVolume, "The requested fsGroup is %d, but the volume %s has GID %d. The volume may not be shareable.", fsGroupNew, m.volName, fsGroupOld)
 			}
 		}

pkg/volume/rbd/rbd.go

@@ -508,7 +508,7 @@ func (plugin *rbdPlugin) newUnmapperInternal(volName string, podUID types.UID, m
 }
 
 func (plugin *rbdPlugin) getDeviceNameFromOldMountPath(mounter mount.Interface, mountPath string) (string, error) {
-	refs, err := mount.GetMountRefsByDev(mounter, mountPath)
+	refs, err := mounter.GetMountRefs(mountPath)
 	if err != nil {
 		return "", err
 	}

pkg/volume/volume_linux.go

@@ -89,17 +89,3 @@ func SetVolumeOwnership(mounter Mounter, fsGroup *int64) error {
 		return nil
 	})
 }
-
-// IsSameFSGroup is called only for requests to mount an already mounted
-// volume. It checks if fsGroup of new mount request is the same or not.
-// It returns false if it not the same. It also returns current Gid of a path
-// provided for dir variable.
-func IsSameFSGroup(dir string, fsGroup int64) (bool, int, error) {
-	info, err := os.Stat(dir)
-	if err != nil {
-		glog.Errorf("Error getting stats for %s (%v)", dir, err)
-		return false, 0, err
-	}
-	s := info.Sys().(*syscall.Stat_t)
-	return int(s.Gid) == int(fsGroup), int(s.Gid), nil
-}

pkg/volume/volume_unsupported.go

@@ -21,7 +21,3 @@ package volume
 func SetVolumeOwnership(mounter Mounter, fsGroup *int64) error {
 	return nil
 }
-
-func IsSameFSGroup(dir string, fsGroup int64) (bool, int, error) {
-	return true, int(fsGroup), nil
-}