From f3f1a047056baca105afadfc8288a977d627dcfd Mon Sep 17 00:00:00 2001 From: Michelle Au Date: Fri, 25 May 2018 16:30:27 -0700 Subject: [PATCH] Only mount subpath as readonly if specified in volumeMount --- pkg/kubelet/kubelet_pods.go | 1 - pkg/util/mount/mount.go | 2 - pkg/util/mount/mount_linux.go | 4 -- pkg/util/mount/mount_linux_test.go | 79 -------------------------- test/e2e/storage/subpath.go | 90 +++++++++++++++++++++++++++--- 5 files changed, 82 insertions(+), 94 deletions(-) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index d62c57e8302..50209f77019 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/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 diff --git a/pkg/util/mount/mount.go b/pkg/util/mount/mount.go index 49351394dd8..f5ce194cac6 100644 --- a/pkg/util/mount/mount.go +++ b/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, diff --git a/pkg/util/mount/mount_linux.go b/pkg/util/mount/mount_linux.go index b02ca84f036..e80dad590ad 100644 --- a/pkg/util/mount/mount_linux.go +++ b/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) diff --git a/pkg/util/mount/mount_linux_test.go b/pkg/util/mount/mount_linux_test.go index 4c0cbd7b21c..0324bf88d9d 100644 --- a/pkg/util/mount/mount_linux_test.go +++ b/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 diff --git a/test/e2e/storage/subpath.go b/test/e2e/storage/subpath.go index 4b7394fa3db..4e357b6eb9c 100644 --- a/test/e2e/storage/subpath.go +++ b/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{