Merge pull request #64351 from msau42/fix-readonly

Automatic merge from submit-queue (batch tested with PRs 62266, 64351, 64366, 64235, 64560). 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>.

Bind mount subpath with same read/write settings as underlying volume

**What this PR does / why we need it**:
https://github.com/kubernetes/kubernetes/pull/63045 broke two scenarios:
* If volumeMount path already exists in container image, container runtime will try to chown the volume
* In SELinux system, we will try to set SELinux labels when starting the container

This fix makes it so that the subpath bind mount will inherit the read/write settings of the underlying volume mount. It does this by using the "bind,remount" mount options when doing the bind mount.

The underlying volume mount is ro when the volumeSource.readOnly flag is set. This is for persistent volume types like PVC, GCE PD, NFS, etc.  When this is set, we won't try to configure SELinux labels.  Also in this mode, subpaths have to already exist in the volume, we cannot make new directories on a read only volume.

When volumeMount.readOnly is set, the container runtime is in charge of making the volume in the container readOnly, but the underlying volume mount on the host can be writable. This can be set for any volume type, and is permanently set for atomic volume types like configmaps, secrets.  In this case, SELinux labels will be applied before the container runtime makes the volume readOnly.  And subpaths don't have to exist.

**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 #64120

**Special notes for your reviewer**:

**Release note**:

```release-note
Fixes issue for readOnly subpath mounts for SELinux systems and when the volume mountPath already existed in the container image.
```

pkg/kubelet/kubelet_pods.go

@@ -206,7 +206,6 @@ 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/mount.go

@@ -136,8 +136,6 @@ 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,

pkg/util/mount/mount_linux.go

@@ -884,10 +884,6 @@ func doBindSubPath(mounter Interface, subpath Subpath) (hostPath string, err err
 
 	// 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*/, options); err != nil {
 		return "", fmt.Errorf("error mounting %s: %s", subpath.Path, err)

pkg/util/mount/mount_linux_test.go

@@ -1009,7 +1009,6 @@ func getTestPaths(base string) (string, string) {
 
 func TestBindSubPath(t *testing.T) {
 	defaultPerm := os.FileMode(0750)
-	readOnlyPerm := os.FileMode(0444)
 
 	tests := []struct {
 		name string
@@ -1017,7 +1016,6 @@ func TestBindSubPath(t *testing.T) {
 		// base.
 		prepare     func(base string) ([]string, string, string, error)
 		expectError bool
-		readOnly    bool
 	}{
 		{
 			name: "subpath-dir",
@@ -1214,55 +1212,6 @@ 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 {
@@ -1287,7 +1236,6 @@ func TestBindSubPath(t *testing.T) {
 			VolumePath:       volPath,
 			PodDir:           filepath.Join(base, "pod0"),
 			ContainerName:    testContainer,
-			ReadOnly:         test.readOnly,
 		}
 
 		_, subpathMount := getTestPaths(base)
@@ -1313,39 +1261,12 @@ 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

@@ -110,6 +110,13 @@ var _ = utils.SIGDescribe("Subpath", func() {
 			testBasicSubpath(f, "configmap-value", pod)
 		})
 
+		It("should support subpaths with configmap pod with mountPath of existing file", func() {
+			pod := testPodSubpath(f, "configmap-key", "configmap", &v1.VolumeSource{ConfigMap: &v1.ConfigMapVolumeSource{LocalObjectReference: v1.LocalObjectReference{Name: "my-configmap"}}})
+			file := "/etc/resolv.conf"
+			pod.Spec.Containers[0].VolumeMounts[0].MountPath = file
+			testBasicSubpathFile(f, "configmap-value", pod, file)
+		})
+
 		It("should support subpaths with downward pod", func() {
 			pod := testPodSubpath(f, "downward/podname", "downwardAPI", &v1.VolumeSource{
 				DownwardAPI: &v1.DownwardAPIVolumeSource{
@@ -199,7 +206,7 @@ var _ = utils.SIGDescribe("Subpath", func() {
 				setInitCommand(pod, fmt.Sprintf("ln -s /bin %s", subPathDir))
 
 				// Pod should fail
-				testPodFailSupath(f, pod)
+				testPodFailSubpath(f, pod)
 			})
 
 			It("should fail if subpath file is outside the volume [Slow]", func() {
@@ -207,7 +214,7 @@ var _ = utils.SIGDescribe("Subpath", func() {
 				setInitCommand(pod, fmt.Sprintf("ln -s /bin/sh %s", subPathDir))
 
 				// Pod should fail
-				testPodFailSupath(f, pod)
+				testPodFailSubpath(f, pod)
 			})
 
 			It("should fail if non-existent subpath is outside the volume [Slow]", func() {
@@ -215,7 +222,7 @@ var _ = utils.SIGDescribe("Subpath", func() {
 				setInitCommand(pod, fmt.Sprintf("ln -s /bin/notanexistingpath %s", subPathDir))
 
 				// Pod should fail
-				testPodFailSupath(f, pod)
+				testPodFailSubpath(f, pod)
 			})
 
 			It("should fail if subpath with backstepping is outside the volume [Slow]", func() {
@@ -223,7 +230,7 @@ var _ = utils.SIGDescribe("Subpath", func() {
 				setInitCommand(pod, fmt.Sprintf("ln -s ../ %s", subPathDir))
 
 				// Pod should fail
-				testPodFailSupath(f, pod)
+				testPodFailSubpath(f, pod)
 			})
 
 			It("should support creating multiple subpath from same volumes [Slow]", func() {
@@ -297,7 +304,7 @@ var _ = utils.SIGDescribe("Subpath", func() {
 				testReadFile(f, volumePath, pod, 0)
 			})
 
-			It("should support readOnly directory specified in the volumeSource", func() {
+			It("should support existing directories when readOnly specified in the volumeSource", func() {
 				roVol := vol.getReadOnlyVolumeSpec()
 				if roVol == nil {
 					framework.Skipf("Volume type %v doesn't support readOnly source", curVolType)
@@ -312,6 +319,19 @@ var _ = utils.SIGDescribe("Subpath", func() {
 				// Read it from inside the subPath from container 0
 				testReadFile(f, filePathInSubpath, pod, 0)
 			})
+
+			It("should fail for new directories when readOnly specified in the volumeSource", func() {
+				roVol := vol.getReadOnlyVolumeSpec()
+				if roVol == nil {
+					framework.Skipf("Volume type %v doesn't support readOnly source", curVolType)
+				}
+
+				// Set volume source to read only
+				pod.Spec.Volumes[0].VolumeSource = *roVol
+
+				// Pod should fail
+				testPodFailSubpathError(f, pod, "")
+			})
 		})
 	}
 
@@ -319,7 +339,11 @@ var _ = utils.SIGDescribe("Subpath", func() {
 })
 
 func testBasicSubpath(f *framework.Framework, contents string, pod *v1.Pod) {
-	setReadCommand(volumePath, &pod.Spec.Containers[0])
+	testBasicSubpathFile(f, contents, pod, volumePath)
+}
+
+func testBasicSubpathFile(f *framework.Framework, contents string, pod *v1.Pod, filepath string) {
+	setReadCommand(filepath, &pod.Spec.Containers[0])
 
 	By(fmt.Sprintf("Creating pod %s", pod.Name))
 	f.TestContainerOutput("atomic-volume-subpath", pod, 0, []string{contents})
@@ -412,6 +436,11 @@ func testPodSubpath(f *framework.Framework, subpath, volumeType string, source *
 					},
 				},
 			},
+			SecurityContext: &v1.PodSecurityContext{
+				SELinuxOptions: &v1.SELinuxOptions{
+					Level: "s0:c0,c1",
+				},
+			},
 		},
 	}
 }
@@ -473,7 +502,11 @@ func testReadFile(f *framework.Framework, file string, pod *v1.Pod, containerInd
 	Expect(err).NotTo(HaveOccurred(), "while deleting pod")
 }
 
-func testPodFailSupath(f *framework.Framework, pod *v1.Pod) {
+func testPodFailSubpath(f *framework.Framework, pod *v1.Pod) {
+	testPodFailSubpathError(f, pod, "subPath")
+}
+
+func testPodFailSubpathError(f *framework.Framework, pod *v1.Pod, errorMsg string) {
 	By(fmt.Sprintf("Creating pod %s", pod.Name))
 	pod, err := f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(pod)
 	Expect(err).ToNot(HaveOccurred(), "while creating pod")
@@ -494,7 +527,7 @@ func testPodFailSupath(f *framework.Framework, pod *v1.Pod) {
 	events, err := f.ClientSet.CoreV1().Events(f.Namespace.Name).List(options)
 	Expect(err).NotTo(HaveOccurred(), "while getting pod events")
 	Expect(len(events.Items)).NotTo(Equal(0), "no events found")
-	Expect(events.Items[0].Message).To(ContainSubstring("subPath"), "subpath error not found")
+	Expect(events.Items[0].Message).To(ContainSubstring(errorMsg), fmt.Sprintf("%q error not found", errorMsg))
 }
 
 // Tests that the existing subpath mount is detected when a container restarts
@@ -785,6 +818,47 @@ func (s *gcepdPVCSource) createVolume(f *framework.Framework) volInfo {
 	s.pvc, err = framework.CreatePVC(f.ClientSet, f.Namespace.Name, pvc)
 	framework.ExpectNoError(err, "Error creating PVC")
 
+	// Launch pod to format the PD first
+	pod := &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: fmt.Sprintf("gcepd-prep-%s", f.Namespace.Name),
+		},
+		Spec: v1.PodSpec{
+			Containers: []v1.Container{
+				{
+					Name:    fmt.Sprintf("init-volume-%s", f.Namespace.Name),
+					Image:   "busybox",
+					Command: []string{"/bin/sh", "-ec", "echo nothing"},
+					VolumeMounts: []v1.VolumeMount{
+						{
+							Name:      volumeName,
+							MountPath: "/vol",
+						},
+					},
+				},
+			},
+			RestartPolicy: v1.RestartPolicyNever,
+			Volumes: []v1.Volume{
+				{
+					Name: volumeName,
+					VolumeSource: v1.VolumeSource{
+						PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{
+							ClaimName: s.pvc.Name,
+						},
+					},
+				},
+			},
+		},
+	}
+	pod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(pod)
+	Expect(err).ToNot(HaveOccurred(), "while creating gce pd init pod")
+
+	err = framework.WaitForPodSuccessInNamespace(f.ClientSet, pod.Name, pod.Namespace)
+	Expect(err).ToNot(HaveOccurred(), "while waiting for gce pd init pod to succeed")
+
+	err = framework.DeletePodWithWait(f, f.ClientSet, pod)
+	Expect(err).ToNot(HaveOccurred(), "while deleting gce pd init pod")
+
 	return volInfo{
 		source: &v1.VolumeSource{
 			PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{