Merge pull request #63045 from msau42/fix-subpath-readonly

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

passthrough readOnly to subpath

**What this PR does / why we need it**:
If a volume is mounted as readonly, or subpath volumeMount is configured as readonly, then the subpath bind mount should be readonly.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #62752

**Special notes for your reviewer**:

**Release note**:

```release-note
Fixes issue where subpath readOnly mounts failed
```

pkg/kubelet/kubelet_pods.go

@@ -207,6 +207,7 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h
 				VolumePath:       volumePath,
 				PodDir:           podDir,
 				ContainerName:    container.Name,
+				ReadOnly:         mount.ReadOnly || vol.Mounter.GetAttributes().ReadOnly,
 			})
 			if err != nil {
 				// Don't pass detailed error back to the user because it could give information about host filesystem

pkg/util/mount/exec_mount_test.go

@@ -67,7 +67,7 @@ func TestBindMount(t *testing.T) {
 			expectedArgs = []string{"-t", fsType, "-o", "bind", sourcePath, destinationPath}
 		case 2:
 			// mount -t fstype -o "remount,opts" source target
-			expectedArgs = []string{"-t", fsType, "-o", "remount," + strings.Join(mountOptions, ","), sourcePath, destinationPath}
+			expectedArgs = []string{"-t", fsType, "-o", "bind,remount," + strings.Join(mountOptions, ","), sourcePath, destinationPath}
 		}
 		if !reflect.DeepEqual(expectedArgs, args) {
 			t.Errorf("expected arguments %q, got %q", strings.Join(expectedArgs, " "), strings.Join(args, " "))

pkg/util/mount/fake.go

@@ -59,8 +59,10 @@ func (f *FakeMounter) Mount(source string, target string, fstype string, options
 	f.mutex.Lock()
 	defer f.mutex.Unlock()
 
-	// find 'bind' option
+	opts := []string{}
+
 	for _, option := range options {
+		// find 'bind' option
 		if option == "bind" {
 			// This is a bind-mount. In order to mimic linux behaviour, we must
 			// use the original device of the bind-mount as the real source.
@@ -79,7 +81,11 @@ func (f *FakeMounter) Mount(source string, target string, fstype string, options
 					break
 				}
 			}
-			break
+		}
+		// find 'ro' option
+		if option == "ro" {
+			// reuse MountPoint.Opts field to mark mount as readonly
+			opts = append(opts, "ro")
 		}
 	}
 
@@ -89,7 +95,7 @@ func (f *FakeMounter) Mount(source string, target string, fstype string, options
 		absTarget = target
 	}
 
-	f.MountPoints = append(f.MountPoints, MountPoint{Device: source, Path: absTarget, Type: fstype})
+	f.MountPoints = append(f.MountPoints, MountPoint{Device: source, Path: absTarget, Type: fstype, Opts: opts})
 	glog.V(5).Infof("Fake mounter: mounted %s to %s", source, absTarget)
 	f.Log = append(f.Log, FakeAction{Action: FakeActionMount, Target: absTarget, Source: source, FSType: fstype})
 	return nil

pkg/util/mount/mount.go

@@ -129,6 +129,8 @@ type Subpath struct {
 	PodDir string
 	// Name of the container
 	ContainerName string
+	// True if the mount needs to be readonly
+	ReadOnly bool
 }
 
 // Exec executes command where mount utilities are. This can be either the host,
@@ -277,7 +279,13 @@ func IsNotMountPoint(mounter Interface, file string) (bool, error) {
 // The list equals:
 //   options - 'bind' + 'remount' (no duplicate)
 func isBind(options []string) (bool, []string) {
-	bindRemountOpts := []string{"remount"}
+	// Because we have an FD opened on the subpath bind mount, the "bind" option
+	// needs to be included, otherwise the mount target will error as busy if you
+	// remount as readonly.
+	//
+	// As a consequence, all read only bind mounts will no longer change the underlying
+	// volume mount to be read only.
+	bindRemountOpts := []string{"bind", "remount"}
 	bind := false
 
 	if len(options) != 0 {

pkg/util/mount/mount_linux.go

@@ -777,8 +777,13 @@ func doBindSubPath(mounter Interface, subpath Subpath, kubeletPid int) (hostPath
 	mountSource := fmt.Sprintf("/proc/%d/fd/%v", kubeletPid, fd)
 
 	// Do the bind mount
+	options := []string{"bind"}
+	if subpath.ReadOnly {
+		options = append(options, "ro")
+	}
+
 	glog.V(5).Infof("bind mounting %q at %q", mountSource, bindPathTarget)
-	if err = mounter.Mount(mountSource, bindPathTarget, "" /*fstype*/, []string{"bind"}); err != nil {
+	if err = mounter.Mount(mountSource, bindPathTarget, "" /*fstype*/, options); err != nil {
 		return "", fmt.Errorf("error mounting %s: %s", subpath.Path, err)
 	}
 

pkg/util/mount/mount_linux_test.go

@@ -1009,6 +1009,7 @@ func getTestPaths(base string) (string, string) {
 
 func TestBindSubPath(t *testing.T) {
 	defaultPerm := os.FileMode(0750)
+	readOnlyPerm := os.FileMode(0444)
 
 	tests := []struct {
 		name string
@@ -1016,6 +1017,7 @@ func TestBindSubPath(t *testing.T) {
 		// base.
 		prepare     func(base string) ([]string, string, string, error)
 		expectError bool
+		readOnly    bool
 	}{
 		{
 			name: "subpath-dir",
@@ -1220,6 +1222,55 @@ func TestBindSubPath(t *testing.T) {
 			},
 			expectError: false,
 		},
+		{
+			name: "subpath-dir-readonly",
+			prepare: func(base string) ([]string, string, string, error) {
+				volpath, _ := getTestPaths(base)
+				subpath := filepath.Join(volpath, "dir0")
+				return nil, volpath, subpath, os.MkdirAll(subpath, defaultPerm)
+			},
+			expectError: false,
+			readOnly:    true,
+		},
+		{
+			name: "subpath-file-readonly",
+			prepare: func(base string) ([]string, string, string, error) {
+				volpath, _ := getTestPaths(base)
+				subpath := filepath.Join(volpath, "file0")
+				if err := os.MkdirAll(volpath, defaultPerm); err != nil {
+					return nil, "", "", err
+				}
+				return nil, volpath, subpath, ioutil.WriteFile(subpath, []byte{}, defaultPerm)
+			},
+			expectError: false,
+			readOnly:    true,
+		},
+		{
+			name: "subpath-dir-and-volume-readonly",
+			prepare: func(base string) ([]string, string, string, error) {
+				volpath, _ := getTestPaths(base)
+				subpath := filepath.Join(volpath, "dir0")
+				if err := os.MkdirAll(subpath, defaultPerm); err != nil {
+					return nil, "", "", err
+				}
+				return nil, volpath, subpath, os.Chmod(subpath, readOnlyPerm)
+			},
+			expectError: false,
+			readOnly:    true,
+		},
+		{
+			name: "subpath-file-and-vol-readonly",
+			prepare: func(base string) ([]string, string, string, error) {
+				volpath, _ := getTestPaths(base)
+				subpath := filepath.Join(volpath, "file0")
+				if err := os.MkdirAll(volpath, defaultPerm); err != nil {
+					return nil, "", "", err
+				}
+				return nil, volpath, subpath, ioutil.WriteFile(subpath, []byte{}, readOnlyPerm)
+			},
+			expectError: false,
+			readOnly:    true,
+		},
 	}
 
 	for _, test := range tests {
@@ -1244,6 +1295,7 @@ func TestBindSubPath(t *testing.T) {
 			VolumePath:       volPath,
 			PodDir:           filepath.Join(base, "pod0"),
 			ContainerName:    testContainer,
+			ReadOnly:         test.readOnly,
 		}
 
 		_, subpathMount := getTestPaths(base)
@@ -1269,12 +1321,39 @@ func TestBindSubPath(t *testing.T) {
 			if err = validateFileExists(subpathMount); err != nil {
 				t.Errorf("test %q failed: %v", test.name, err)
 			}
+			if err = validateReadOnlyMount(test.readOnly, bindPathTarget, fm); err != nil {
+				t.Errorf("test %q failed: %v", test.name, err)
+			}
 		}
 
 		os.RemoveAll(base)
 	}
 }
 
+func validateReadOnlyMount(expectedReadOnly bool, bindPathTarget string, mounter *FakeMounter) error {
+	mps, err := mounter.List()
+	if err != nil {
+		return fmt.Errorf("fakeMounter.List() returned error: %v", err)
+	}
+	for _, mp := range mps {
+		if mp.Path == bindPathTarget {
+			foundReadOnly := false
+			for _, opts := range mp.Opts {
+				if opts == "ro" {
+					foundReadOnly = true
+					break
+				}
+			}
+			if expectedReadOnly != foundReadOnly {
+				return fmt.Errorf("expected readOnly %v, got %v for mount point %v", expectedReadOnly, foundReadOnly, bindPathTarget)
+			} else {
+				return nil
+			}
+		}
+	}
+	return fmt.Errorf("failed to find mountPoint %v", bindPathTarget)
+}
+
 func TestParseMountInfo(t *testing.T) {
 	info :=
 		`62 0 253:0 / / rw,relatime shared:1 - ext4 /dev/mapper/ssd-root rw,seclabel,data=ordered

test/e2e/storage/subpath.go

@@ -56,13 +56,14 @@ type volInfo struct {
 type volSource interface {
 	createVolume(f *framework.Framework) volInfo
 	cleanupVolume(f *framework.Framework)
+	getReadOnlyVolumeSpec() *v1.VolumeSource
 }
 
 var initVolSources = map[string]func() volSource{
 	"hostPath":         initHostpath,
 	"hostPathSymlink":  initHostpathSymlink,
 	"emptyDir":         initEmptydir,
-	"gcePDPVC":         initGCEPD,
+	"gcePDPVC":         initGCEPDPVC,
 	"gcePDPartitioned": initGCEPDPartition,
 	"nfs":              initNFS,
 	"nfsPVC":           initNFSPVC,
@@ -271,6 +272,46 @@ var _ = utils.SIGDescribe("Subpath", func() {
 				}
 				testSubpathReconstruction(f, pod, true)
 			})
+
+			It("should support readOnly directory specified in the volumeMount", func() {
+				// Create the directory
+				setInitCommand(pod, fmt.Sprintf("mkdir -p %s", subPathDir))
+
+				// Write the file in the volume from container 1
+				setWriteCommand(filePathInVolume, &pod.Spec.Containers[1])
+
+				// Read it from inside the subPath from container 0
+				pod.Spec.Containers[0].VolumeMounts[0].ReadOnly = true
+				testReadFile(f, filePathInSubpath, pod, 0)
+			})
+
+			It("should support readOnly file specified in the volumeMount", func() {
+				// Create the file
+				setInitCommand(pod, fmt.Sprintf("touch %s", subPathDir))
+
+				// Write the file in the volume from container 1
+				setWriteCommand(subPathDir, &pod.Spec.Containers[1])
+
+				// Read it from inside the subPath from container 0
+				pod.Spec.Containers[0].VolumeMounts[0].ReadOnly = true
+				testReadFile(f, volumePath, pod, 0)
+			})
+
+			It("should support readOnly directory specified in the volumeSource", func() {
+				roVol := vol.getReadOnlyVolumeSpec()
+				if roVol == nil {
+					framework.Skipf("Volume type %v doesn't support readOnly source", curVolType)
+				}
+
+				// Initialize content in the volume while it's writable
+				initVolumeContent(f, pod, filePathInVolume, filePathInSubpath)
+
+				// Set volume source to read only
+				pod.Spec.Volumes[0].VolumeSource = *roVol
+
+				// Read it from inside the subPath from container 0
+				testReadFile(f, filePathInSubpath, pod, 0)
+			})
 		})
 	}
 
@@ -375,6 +416,12 @@ func testPodSubpath(f *framework.Framework, subpath, volumeType string, source *
 	}
 }
 
+func clearSubpathPodCommands(pod *v1.Pod) {
+	pod.Spec.InitContainers[0].Command = nil
+	pod.Spec.Containers[0].Args = nil
+	pod.Spec.Containers[1].Args = nil
+}
+
 func setInitCommand(pod *v1.Pod, command string) {
 	pod.Spec.InitContainers[0].Command = []string{"/bin/sh", "-ec", command}
 }
@@ -570,6 +617,23 @@ func testSubpathReconstruction(f *framework.Framework, pod *v1.Pod, forceDelete
 	utils.TestVolumeUnmountsFromDeletedPodWithForceOption(f.ClientSet, f, pod, forceDelete, true)
 }
 
+func initVolumeContent(f *framework.Framework, pod *v1.Pod, volumeFilepath, subpathFilepath string) {
+	setWriteCommand(volumeFilepath, &pod.Spec.Containers[1])
+	setReadCommand(subpathFilepath, &pod.Spec.Containers[0])
+
+	By(fmt.Sprintf("Creating pod to write volume content %s", pod.Name))
+	f.TestContainerOutput("subpath", pod, 0, []string{
+		"content of file \"" + subpathFilepath + "\": mount-tester new file",
+	})
+
+	By(fmt.Sprintf("Deleting pod %s", pod.Name))
+	err := framework.DeletePodWithWait(f, f.ClientSet, pod)
+	Expect(err).NotTo(HaveOccurred(), "while deleting pod")
+
+	// This pod spec is going to be reused; reset all the commands
+	clearSubpathPodCommands(pod)
+}
+
 func podContainerExec(pod *v1.Pod, containerIndex int, bashExec string) (string, error) {
 	return framework.RunKubectl("exec", fmt.Sprintf("--namespace=%s", pod.Namespace), pod.Name, "--container", pod.Spec.Containers[containerIndex].Name, "--", "/bin/sh", "-c", bashExec)
 }
@@ -591,6 +655,10 @@ func (s *hostpathSource) createVolume(f *framework.Framework) volInfo {
 	}
 }
 
+func (s *hostpathSource) getReadOnlyVolumeSpec() *v1.VolumeSource {
+	return nil
+}
+
 func (s *hostpathSource) cleanupVolume(f *framework.Framework) {
 }
 
@@ -666,6 +734,10 @@ func (s *hostpathSymlinkSource) createVolume(f *framework.Framework) volInfo {
 	}
 }
 
+func (s *hostpathSymlinkSource) getReadOnlyVolumeSpec() *v1.VolumeSource {
+	return nil
+}
+
 func (s *hostpathSymlinkSource) cleanupVolume(f *framework.Framework) {
 }
 
@@ -684,19 +756,23 @@ func (s *emptydirSource) createVolume(f *framework.Framework) volInfo {
 	}
 }
 
+func (s *emptydirSource) getReadOnlyVolumeSpec() *v1.VolumeSource {
+	return nil
+}
+
 func (s *emptydirSource) cleanupVolume(f *framework.Framework) {
 }
 
-type gcepdSource struct {
+type gcepdPVCSource struct {
 	pvc *v1.PersistentVolumeClaim
 }
 
-func initGCEPD() volSource {
+func initGCEPDPVC() volSource {
 	framework.SkipUnlessProviderIs("gce", "gke")
-	return &gcepdSource{}
+	return &gcepdPVCSource{}
 }
 
-func (s *gcepdSource) createVolume(f *framework.Framework) volInfo {
+func (s *gcepdPVCSource) createVolume(f *framework.Framework) volInfo {
 	var err error
 
 	framework.Logf("Creating GCE PD volume via dynamic provisioning")
@@ -718,7 +794,16 @@ func (s *gcepdSource) createVolume(f *framework.Framework) volInfo {
 	}
 }
 
-func (s *gcepdSource) cleanupVolume(f *framework.Framework) {
+func (s *gcepdPVCSource) getReadOnlyVolumeSpec() *v1.VolumeSource {
+	return &v1.VolumeSource{
+		PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{
+			ClaimName: s.pvc.Name,
+			ReadOnly:  true,
+		},
+	}
+}
+
+func (s *gcepdPVCSource) cleanupVolume(f *framework.Framework) {
 	if s.pvc != nil {
 		err := f.ClientSet.CoreV1().PersistentVolumeClaims(f.Namespace.Name).Delete(s.pvc.Name, nil)
 		framework.ExpectNoError(err, "Error deleting PVC")
@@ -756,6 +841,10 @@ func (s *gcepdPartitionSource) createVolume(f *framework.Framework) volInfo {
 	}
 }
 
+func (s *gcepdPartitionSource) getReadOnlyVolumeSpec() *v1.VolumeSource {
+	return nil
+}
+
 func (s *gcepdPartitionSource) cleanupVolume(f *framework.Framework) {
 	if s.diskName != "" {
 		// err := framework.DeletePDWithRetry(s.diskName)
@@ -765,6 +854,7 @@ func (s *gcepdPartitionSource) cleanupVolume(f *framework.Framework) {
 
 type nfsSource struct {
 	serverPod *v1.Pod
+	serverIP  string
 }
 
 func initNFS() volSource {
@@ -772,21 +862,29 @@ func initNFS() volSource {
 }
 
 func (s *nfsSource) createVolume(f *framework.Framework) volInfo {
-	var serverIP string
-
 	framework.Logf("Creating NFS server")
-	_, s.serverPod, serverIP = framework.NewNFSServer(f.ClientSet, f.Namespace.Name, []string{"-G", "777", "/exports"})
+	_, s.serverPod, s.serverIP = framework.NewNFSServer(f.ClientSet, f.Namespace.Name, []string{"-G", "777", "/exports"})
 
 	return volInfo{
 		source: &v1.VolumeSource{
 			NFS: &v1.NFSVolumeSource{
-				Server: serverIP,
+				Server: s.serverIP,
 				Path:   "/exports",
 			},
 		},
 	}
 }
 
+func (s *nfsSource) getReadOnlyVolumeSpec() *v1.VolumeSource {
+	return &v1.VolumeSource{
+		NFS: &v1.NFSVolumeSource{
+			Server:   s.serverIP,
+			Path:     "/exports",
+			ReadOnly: true,
+		},
+	}
+}
+
 func (s *nfsSource) cleanupVolume(f *framework.Framework) {
 	if s.serverPod != nil {
 		framework.DeletePodWithWait(f, f.ClientSet, s.serverPod)
@@ -816,6 +914,16 @@ func (s *glusterSource) createVolume(f *framework.Framework) volInfo {
 	}
 }
 
+func (s *glusterSource) getReadOnlyVolumeSpec() *v1.VolumeSource {
+	return &v1.VolumeSource{
+		Glusterfs: &v1.GlusterfsVolumeSource{
+			EndpointsName: "gluster-server",
+			Path:          "test_vol",
+			ReadOnly:      true,
+		},
+	}
+}
+
 func (s *glusterSource) cleanupVolume(f *framework.Framework) {
 	if s.serverPod != nil {
 		framework.DeletePodWithWait(f, f.ClientSet, s.serverPod)
@@ -875,6 +983,15 @@ func (s *nfsPVCSource) createVolume(f *framework.Framework) volInfo {
 	}
 }
 
+func (s *nfsPVCSource) getReadOnlyVolumeSpec() *v1.VolumeSource {
+	return &v1.VolumeSource{
+		PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{
+			ClaimName: s.pvc.Name,
+			ReadOnly:  true,
+		},
+	}
+}
+
 func (s *nfsPVCSource) cleanupVolume(f *framework.Framework) {
 	if s.pvc != nil || s.pv != nil {
 		if errs := framework.PVPVCCleanup(f.ClientSet, f.Namespace.Name, s.pv, s.pvc); len(errs) != 0 {