From 6b1947c9ba8544a99db12cb38944b75f14d4db3d Mon Sep 17 00:00:00 2001 From: Michelle Au Date: Mon, 23 Apr 2018 17:13:27 -0700 Subject: [PATCH] passthrough readOnly to subpath --- pkg/kubelet/kubelet_pods.go | 1 + pkg/util/mount/exec_mount_test.go | 2 +- pkg/util/mount/fake.go | 12 ++- pkg/util/mount/mount.go | 10 ++- pkg/util/mount/mount_linux.go | 7 +- pkg/util/mount/mount_linux_test.go | 79 +++++++++++++++++ test/e2e/storage/subpath.go | 137 ++++++++++++++++++++++++++--- 7 files changed, 232 insertions(+), 16 deletions(-) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 6411848fe2..bf348a5231 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/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 diff --git a/pkg/util/mount/exec_mount_test.go b/pkg/util/mount/exec_mount_test.go index 248f49631b..20e841621e 100644 --- a/pkg/util/mount/exec_mount_test.go +++ b/pkg/util/mount/exec_mount_test.go @@ -66,7 +66,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, " ")) diff --git a/pkg/util/mount/fake.go b/pkg/util/mount/fake.go index 4bc32ff21c..c4cf35080f 100644 --- a/pkg/util/mount/fake.go +++ b/pkg/util/mount/fake.go @@ -58,8 +58,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. @@ -78,7 +80,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") } } @@ -88,7 +94,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 diff --git a/pkg/util/mount/mount.go b/pkg/util/mount/mount.go index 244710064b..305bf0f5b4 100644 --- a/pkg/util/mount/mount.go +++ b/pkg/util/mount/mount.go @@ -123,6 +123,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, @@ -274,7 +276,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 { diff --git a/pkg/util/mount/mount_linux.go b/pkg/util/mount/mount_linux.go index 2eac05a7cc..3656801569 100644 --- a/pkg/util/mount/mount_linux.go +++ b/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) } diff --git a/pkg/util/mount/mount_linux_test.go b/pkg/util/mount/mount_linux_test.go index d6b3c497c2..d55df62ef4 100644 --- a/pkg/util/mount/mount_linux_test.go +++ b/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 diff --git a/test/e2e/storage/subpath.go b/test/e2e/storage/subpath.go index f32a3ae51e..4b7394fa3d 100644 --- a/test/e2e/storage/subpath.go +++ b/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 {