From c52d4bf32f531f7f22fca79a5e35205bdbf2ddc6 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Wed, 26 Feb 2020 16:59:22 -0500 Subject: [PATCH] Implement changes into volume plugins for skipping chown Add a separate function for walking directories --- pkg/volume/awsebs/aws_ebs.go | 2 +- pkg/volume/azure_dd/azure_mounter.go | 2 +- pkg/volume/cinder/cinder.go | 2 +- pkg/volume/configmap/configmap.go | 2 +- pkg/volume/csi/csi_mounter.go | 7 +- pkg/volume/downwardapi/downwardapi.go | 4 +- pkg/volume/emptydir/empty_dir.go | 4 +- pkg/volume/fc/disk_manager.go | 5 +- pkg/volume/fc/fc.go | 2 +- pkg/volume/flexvolume/mounter.go | 2 +- pkg/volume/flocker/flocker.go | 2 +- pkg/volume/gcepd/gce_pd.go | 2 +- pkg/volume/git_repo/git_repo.go | 4 +- pkg/volume/iscsi/disk_manager.go | 5 +- pkg/volume/iscsi/iscsi.go | 2 +- pkg/volume/local/local.go | 2 +- pkg/volume/portworx/portworx.go | 2 +- pkg/volume/projected/projected.go | 4 +- pkg/volume/rbd/disk_manager.go | 4 +- pkg/volume/rbd/rbd.go | 2 +- pkg/volume/scaleio/sio_volume.go | 2 +- pkg/volume/secret/secret.go | 2 +- pkg/volume/storageos/storageos.go | 2 +- .../operationexecutor/operation_generator.go | 16 +- pkg/volume/volume.go | 5 +- pkg/volume/volume_linux.go | 220 ++++++++++++++---- pkg/volume/volume_linux_test.go | 162 +++++++++++++ pkg/volume/volume_unsupported.go | 6 +- pkg/volume/vsphere_volume/vsphere_volume.go | 2 +- 29 files changed, 392 insertions(+), 86 deletions(-) create mode 100644 pkg/volume/volume_linux_test.go diff --git a/pkg/volume/awsebs/aws_ebs.go b/pkg/volume/awsebs/aws_ebs.go index ba73508391b..a5ae720ac43 100644 --- a/pkg/volume/awsebs/aws_ebs.go +++ b/pkg/volume/awsebs/aws_ebs.go @@ -428,7 +428,7 @@ func (b *awsElasticBlockStoreMounter) SetUpAt(dir string, mounterArgs volume.Mou } if !b.readOnly { - volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + volume.SetVolumeOwnership(b, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) } klog.V(4).Infof("Successfully mounted %s", dir) diff --git a/pkg/volume/azure_dd/azure_mounter.go b/pkg/volume/azure_dd/azure_mounter.go index 2f8d38bd0aa..ed2b49a6b35 100644 --- a/pkg/volume/azure_dd/azure_mounter.go +++ b/pkg/volume/azure_dd/azure_mounter.go @@ -164,7 +164,7 @@ func (m *azureDiskMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) e } if volumeSource.ReadOnly == nil || !*volumeSource.ReadOnly { - volume.SetVolumeOwnership(m, mounterArgs.FsGroup) + volume.SetVolumeOwnership(m, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) } klog.V(2).Infof("azureDisk - successfully mounted disk %s on %s", diskName, dir) diff --git a/pkg/volume/cinder/cinder.go b/pkg/volume/cinder/cinder.go index 9393f86091b..28c5e85d2d0 100644 --- a/pkg/volume/cinder/cinder.go +++ b/pkg/volume/cinder/cinder.go @@ -448,7 +448,7 @@ func (b *cinderVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs } if !b.readOnly { - volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + volume.SetVolumeOwnership(b, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) } klog.V(3).Infof("Cinder volume %s mounted to %s", b.pdName, dir) diff --git a/pkg/volume/configmap/configmap.go b/pkg/volume/configmap/configmap.go index 7b0d900f16e..4b909a100bc 100644 --- a/pkg/volume/configmap/configmap.go +++ b/pkg/volume/configmap/configmap.go @@ -256,7 +256,7 @@ func (b *configMapVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA return err } - err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil) if err != nil { klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup) return err diff --git a/pkg/volume/csi/csi_mounter.go b/pkg/volume/csi/csi_mounter.go index b807326cf72..839f5c470ce 100644 --- a/pkg/volume/csi/csi_mounter.go +++ b/pkg/volume/csi/csi_mounter.go @@ -28,6 +28,7 @@ import ( "k8s.io/klog" api "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" storage "k8s.io/api/storage/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" @@ -281,7 +282,7 @@ func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error // The following logic is derived from https://github.com/kubernetes/kubernetes/issues/66323 // if fstype is "", then skip fsgroup (could be indication of non-block filesystem) // if fstype is provided and pv.AccessMode == ReadWriteOnly, then apply fsgroup - err = c.applyFSGroup(fsType, mounterArgs.FsGroup) + err = c.applyFSGroup(fsType, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) if err != nil { // At this point mount operation is successful: // 1. Since volume can not be used by the pod because of invalid permissions, we must return error @@ -380,7 +381,7 @@ func (c *csiMountMgr) TearDownAt(dir string) error { // from https://github.com/kubernetes/kubernetes/issues/66323 // 1) if fstype is "", then skip fsgroup (could be indication of non-block filesystem) // 2) if fstype is provided and pv.AccessMode == ReadWriteOnly and !c.spec.ReadOnly then apply fsgroup -func (c *csiMountMgr) applyFSGroup(fsType string, fsGroup *int64) error { +func (c *csiMountMgr) applyFSGroup(fsType string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) error { if fsGroup != nil { if fsType == "" { klog.V(4).Info(log("mounter.SetupAt WARNING: skipping fsGroup, fsType not provided")) @@ -402,7 +403,7 @@ func (c *csiMountMgr) applyFSGroup(fsType string, fsGroup *int64) error { return nil } - err := volume.SetVolumeOwnership(c, fsGroup) + err := volume.SetVolumeOwnership(c, fsGroup, fsGroupChangePolicy) if err != nil { return err } diff --git a/pkg/volume/downwardapi/downwardapi.go b/pkg/volume/downwardapi/downwardapi.go index 12746696888..753625b5e34 100644 --- a/pkg/volume/downwardapi/downwardapi.go +++ b/pkg/volume/downwardapi/downwardapi.go @@ -20,7 +20,7 @@ import ( "fmt" "path/filepath" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog" @@ -227,7 +227,7 @@ func (b *downwardAPIVolumeMounter) SetUpAt(dir string, mounterArgs volume.Mounte return err } - err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil) if err != nil { klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup) return err diff --git a/pkg/volume/emptydir/empty_dir.go b/pkg/volume/emptydir/empty_dir.go index 2ab16a93636..ba05c1f7b29 100644 --- a/pkg/volume/emptydir/empty_dir.go +++ b/pkg/volume/emptydir/empty_dir.go @@ -25,7 +25,7 @@ import ( "k8s.io/utils/mount" utilstrings "k8s.io/utils/strings" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -227,7 +227,7 @@ func (ed *emptyDir) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { err = fmt.Errorf("unknown storage medium %q", ed.medium) } - volume.SetVolumeOwnership(ed, mounterArgs.FsGroup) + volume.SetVolumeOwnership(ed, mounterArgs.FsGroup, nil) // If setting up the quota fails, just log a message but don't actually error out. // We'll use the old du mechanism in this case, at least until we support diff --git a/pkg/volume/fc/disk_manager.go b/pkg/volume/fc/disk_manager.go index 8ad90f11b8f..75af0cff342 100644 --- a/pkg/volume/fc/disk_manager.go +++ b/pkg/volume/fc/disk_manager.go @@ -19,6 +19,7 @@ package fc import ( "os" + v1 "k8s.io/api/core/v1" "k8s.io/klog" "k8s.io/utils/mount" @@ -39,7 +40,7 @@ type diskManager interface { } // utility to mount a disk based filesystem -func diskSetUp(manager diskManager, b fcDiskMounter, volPath string, mounter mount.Interface, fsGroup *int64) error { +func diskSetUp(manager diskManager, b fcDiskMounter, volPath string, mounter mount.Interface, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) error { globalPDPath := manager.MakeGlobalPDName(*b.fcDisk) noMnt, err := mounter.IsLikelyNotMountPoint(volPath) @@ -90,7 +91,7 @@ func diskSetUp(manager diskManager, b fcDiskMounter, volPath string, mounter mou } if !b.readOnly { - volume.SetVolumeOwnership(&b, fsGroup) + volume.SetVolumeOwnership(&b, fsGroup, fsGroupChangePolicy) } return nil diff --git a/pkg/volume/fc/fc.go b/pkg/volume/fc/fc.go index caaa206e183..660749e3b0c 100644 --- a/pkg/volume/fc/fc.go +++ b/pkg/volume/fc/fc.go @@ -362,7 +362,7 @@ func (b *fcDiskMounter) SetUp(mounterArgs volume.MounterArgs) error { func (b *fcDiskMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { // diskSetUp checks mountpoints and prevent repeated calls - err := diskSetUp(b.manager, *b, dir, b.mounter, mounterArgs.FsGroup) + err := diskSetUp(b.manager, *b, dir, b.mounter, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) if err != nil { klog.Errorf("fc: failed to setup") } diff --git a/pkg/volume/flexvolume/mounter.go b/pkg/volume/flexvolume/mounter.go index 94229d0d833..2573acb850f 100644 --- a/pkg/volume/flexvolume/mounter.go +++ b/pkg/volume/flexvolume/mounter.go @@ -93,7 +93,7 @@ func (f *flexVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) if !f.readOnly { if f.plugin.capabilities.FSGroup { - volume.SetVolumeOwnership(f, mounterArgs.FsGroup) + volume.SetVolumeOwnership(f, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) } } diff --git a/pkg/volume/flocker/flocker.go b/pkg/volume/flocker/flocker.go index 413f275c722..6ff930eaf3b 100644 --- a/pkg/volume/flocker/flocker.go +++ b/pkg/volume/flocker/flocker.go @@ -361,7 +361,7 @@ func (b *flockerVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArg } if !b.readOnly { - volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + volume.SetVolumeOwnership(b, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) } klog.V(4).Infof("successfully mounted %s", dir) diff --git a/pkg/volume/gcepd/gce_pd.go b/pkg/volume/gcepd/gce_pd.go index e99581b4df7..17e82836c01 100644 --- a/pkg/volume/gcepd/gce_pd.go +++ b/pkg/volume/gcepd/gce_pd.go @@ -429,7 +429,7 @@ func (b *gcePersistentDiskMounter) SetUpAt(dir string, mounterArgs volume.Mounte } if !b.readOnly { - volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + volume.SetVolumeOwnership(b, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) } return nil } diff --git a/pkg/volume/git_repo/git_repo.go b/pkg/volume/git_repo/git_repo.go index aee38f5bbf9..4f4a877e4f2 100644 --- a/pkg/volume/git_repo/git_repo.go +++ b/pkg/volume/git_repo/git_repo.go @@ -22,7 +22,7 @@ import ( "path/filepath" "strings" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/kubernetes/pkg/volume" volumeutil "k8s.io/kubernetes/pkg/volume/util" @@ -236,7 +236,7 @@ func (b *gitRepoVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArg return fmt.Errorf("failed to exec 'git reset --hard': %s: %v", output, err) } - volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil) volumeutil.SetReady(b.getMetaDir()) return nil diff --git a/pkg/volume/iscsi/disk_manager.go b/pkg/volume/iscsi/disk_manager.go index dffd7050e46..928e8d85625 100644 --- a/pkg/volume/iscsi/disk_manager.go +++ b/pkg/volume/iscsi/disk_manager.go @@ -19,6 +19,7 @@ package iscsi import ( "os" + v1 "k8s.io/api/core/v1" "k8s.io/klog" "k8s.io/utils/mount" @@ -41,7 +42,7 @@ type diskManager interface { // utility to mount a disk based filesystem // globalPDPath: global mount path like, /var/lib/kubelet/plugins/kubernetes.io/iscsi/{ifaceName}/{portal-some_iqn-lun-lun_id} // volPath: pod volume dir path like, /var/lib/kubelet/pods/{podUID}/volumes/kubernetes.io~iscsi/{volumeName} -func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter mount.Interface, fsGroup *int64) error { +func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter mount.Interface, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) error { notMnt, err := mounter.IsLikelyNotMountPoint(volPath) if err != nil && !os.IsNotExist(err) { klog.Errorf("cannot validate mountpoint: %s", volPath) @@ -95,7 +96,7 @@ func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter } if !b.readOnly { - volume.SetVolumeOwnership(&b, fsGroup) + volume.SetVolumeOwnership(&b, fsGroup, fsGroupChangePolicy) } return nil diff --git a/pkg/volume/iscsi/iscsi.go b/pkg/volume/iscsi/iscsi.go index 2dbab1e54f5..36bce8d189a 100644 --- a/pkg/volume/iscsi/iscsi.go +++ b/pkg/volume/iscsi/iscsi.go @@ -345,7 +345,7 @@ func (b *iscsiDiskMounter) SetUp(mounterArgs volume.MounterArgs) error { func (b *iscsiDiskMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { // diskSetUp checks mountpoints and prevent repeated calls - err := diskSetUp(b.manager, *b, dir, b.mounter, mounterArgs.FsGroup) + err := diskSetUp(b.manager, *b, dir, b.mounter, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) if err != nil { klog.Errorf("iscsi: failed to setup") } diff --git a/pkg/volume/local/local.go b/pkg/volume/local/local.go index ca71a09bb2a..f5e27f3c9f9 100644 --- a/pkg/volume/local/local.go +++ b/pkg/volume/local/local.go @@ -567,7 +567,7 @@ func (m *localVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) if !m.readOnly { // Volume owner will be written only once on the first volume mount if len(refs) == 0 { - return volume.SetVolumeOwnership(m, mounterArgs.FsGroup) + return volume.SetVolumeOwnership(m, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) } } return nil diff --git a/pkg/volume/portworx/portworx.go b/pkg/volume/portworx/portworx.go index 8c0b6299096..f8f06e48ac8 100644 --- a/pkg/volume/portworx/portworx.go +++ b/pkg/volume/portworx/portworx.go @@ -328,7 +328,7 @@ func (b *portworxVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterAr return err } if !b.readOnly { - volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + volume.SetVolumeOwnership(b, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) } klog.Infof("Portworx Volume %s setup at %s", b.volumeID, dir) return nil diff --git a/pkg/volume/projected/projected.go b/pkg/volume/projected/projected.go index 65e1ac5e2f1..eb9e5101179 100644 --- a/pkg/volume/projected/projected.go +++ b/pkg/volume/projected/projected.go @@ -20,7 +20,7 @@ import ( "fmt" authenticationv1 "k8s.io/api/authentication/v1" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -239,7 +239,7 @@ func (s *projectedVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA return err } - err = volume.SetVolumeOwnership(s, mounterArgs.FsGroup) + err = volume.SetVolumeOwnership(s, mounterArgs.FsGroup, nil) if err != nil { klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup) return err diff --git a/pkg/volume/rbd/disk_manager.go b/pkg/volume/rbd/disk_manager.go index 5c10332606e..69d63d18f7d 100644 --- a/pkg/volume/rbd/disk_manager.go +++ b/pkg/volume/rbd/disk_manager.go @@ -58,7 +58,7 @@ type diskManager interface { } // utility to mount a disk based filesystem -func diskSetUp(manager diskManager, b rbdMounter, volPath string, mounter mount.Interface, fsGroup *int64) error { +func diskSetUp(manager diskManager, b rbdMounter, volPath string, mounter mount.Interface, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) error { globalPDPath := manager.MakeGlobalPDName(*b.rbd) notMnt, err := mounter.IsLikelyNotMountPoint(globalPDPath) if err != nil && !os.IsNotExist(err) { @@ -96,7 +96,7 @@ func diskSetUp(manager diskManager, b rbdMounter, volPath string, mounter mount. klog.V(3).Infof("rbd: successfully bind mount %s to %s with options %v", globalPDPath, volPath, mountOptions) if !b.ReadOnly { - volume.SetVolumeOwnership(&b, fsGroup) + volume.SetVolumeOwnership(&b, fsGroup, fsGroupChangePolicy) } return nil diff --git a/pkg/volume/rbd/rbd.go b/pkg/volume/rbd/rbd.go index c1a76cd432e..72a63bd41ca 100644 --- a/pkg/volume/rbd/rbd.go +++ b/pkg/volume/rbd/rbd.go @@ -838,7 +838,7 @@ func (b *rbdMounter) SetUp(mounterArgs volume.MounterArgs) error { func (b *rbdMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { // diskSetUp checks mountpoints and prevent repeated calls klog.V(4).Infof("rbd: attempting to setup at %s", dir) - err := diskSetUp(b.manager, *b, dir, b.mounter, mounterArgs.FsGroup) + err := diskSetUp(b.manager, *b, dir, b.mounter, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) if err != nil { klog.Errorf("rbd: failed to setup at %s %v", dir, err) } diff --git a/pkg/volume/scaleio/sio_volume.go b/pkg/volume/scaleio/sio_volume.go index 9904147be53..0d9efeb181b 100644 --- a/pkg/volume/scaleio/sio_volume.go +++ b/pkg/volume/scaleio/sio_volume.go @@ -157,7 +157,7 @@ func (v *sioVolume) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { if !v.readOnly && mounterArgs.FsGroup != nil { klog.V(4).Info(log("applying value FSGroup ownership")) - volume.SetVolumeOwnership(v, mounterArgs.FsGroup) + volume.SetVolumeOwnership(v, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) } klog.V(4).Info(log("successfully setup PV %s: volume %s mapped as %s mounted at %s", v.volSpecName, v.volName, devicePath, dir)) diff --git a/pkg/volume/secret/secret.go b/pkg/volume/secret/secret.go index 3eee6d91827..a2974b4099d 100644 --- a/pkg/volume/secret/secret.go +++ b/pkg/volume/secret/secret.go @@ -251,7 +251,7 @@ func (b *secretVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs return err } - err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil) if err != nil { klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup) return err diff --git a/pkg/volume/storageos/storageos.go b/pkg/volume/storageos/storageos.go index a90e384e9a6..42c120469bc 100644 --- a/pkg/volume/storageos/storageos.go +++ b/pkg/volume/storageos/storageos.go @@ -431,7 +431,7 @@ func (b *storageosMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) e } if !b.readOnly { - volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + volume.SetVolumeOwnership(b, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) } klog.V(4).Infof("StorageOS volume setup complete on %s", dir) return nil diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 5a98946c6f3..98a5572568d 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -542,9 +542,14 @@ func (og *operationGenerator) GenerateMountVolumeFunc( } var fsGroup *int64 - if volumeToMount.Pod.Spec.SecurityContext != nil && - volumeToMount.Pod.Spec.SecurityContext.FSGroup != nil { - fsGroup = volumeToMount.Pod.Spec.SecurityContext.FSGroup + var fsGroupChangePolicy *v1.PodFSGroupChangePolicy + if podSc := volumeToMount.Pod.Spec.SecurityContext; podSc != nil { + if podSc.FSGroup != nil { + fsGroup = podSc.FSGroup + } + if podSc.FSGroupChangePolicy != nil { + fsGroupChangePolicy = podSc.FSGroupChangePolicy + } } devicePath := volumeToMount.DevicePath @@ -622,8 +627,9 @@ func (og *operationGenerator) GenerateMountVolumeFunc( // Execute mount mountErr := volumeMounter.SetUp(volume.MounterArgs{ - FsGroup: fsGroup, - DesiredSize: volumeToMount.DesiredSizeLimit, + FsGroup: fsGroup, + DesiredSize: volumeToMount.DesiredSizeLimit, + FSGroupChangePolicy: fsGroupChangePolicy, }) // Update actual state of world markOpts := MarkVolumeOpts{ diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 0efabf92508..6e3f1bdef35 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -103,8 +103,9 @@ type Attributes struct { // MounterArgs provides more easily extensible arguments to Mounter type MounterArgs struct { - FsGroup *int64 - DesiredSize *resource.Quantity + FsGroup *int64 + FSGroupChangePolicy *v1.PodFSGroupChangePolicy + DesiredSize *resource.Quantity } // Mounter interface provides methods to set up/mount the volume. diff --git a/pkg/volume/volume_linux.go b/pkg/volume/volume_linux.go index 5108eb00da8..a0072bf5c38 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -24,7 +24,10 @@ import ( "os" + v1 "k8s.io/api/core/v1" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/klog" + "k8s.io/kubernetes/pkg/features" ) const ( @@ -36,60 +39,187 @@ const ( // SetVolumeOwnership modifies the given volume to be owned by // fsGroup, and sets SetGid so that newly created files are owned by // fsGroup. If fsGroup is nil nothing is done. -func SetVolumeOwnership(mounter Mounter, fsGroup *int64) error { - +func SetVolumeOwnership(mounter Mounter, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) error { if fsGroup == nil { return nil } + fsGroupPolicyEnabled := utilfeature.DefaultFeatureGate.Enabled(features.ConfigurableFSGroupPolicy) + klog.Warningf("Setting volume ownership for %s and fsGroup set. If the volume has a lot of files then setting volume ownership could be slow, see https://github.com/kubernetes/kubernetes/issues/69699", mounter.GetPath()) + // This code exists for legacy purposes, so as old behaviour is entirely preserved when feature gate is disabled + // TODO: remove this when ConfigurableFSGroupPolicy turns beta. + if !fsGroupPolicyEnabled { + return legacyPermissionChange(mounter, fsGroup) + } + + if skipPermissionChange(mounter, fsGroup, fsGroupChangePolicy) { + klog.V(3).Infof("skipping permission and ownership change for volume %s", mounter.GetPath()) + return nil + } + + return walkDeep(mounter.GetPath(), func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + return changeFilePermission(path, fsGroup, mounter.GetAttributes().ReadOnly, info) + }) + +} + +func legacyPermissionChange(mounter Mounter, fsGroup *int64) error { return filepath.Walk(mounter.GetPath(), func(path string, info os.FileInfo, err error) error { if err != nil { return err } - - // chown and chmod pass through to the underlying file for symlinks. - // Symlinks have a mode of 777 but this really doesn't mean anything. - // The permissions of the underlying file are what matter. - // However, if one reads the mode of a symlink then chmods the symlink - // with that mode, it changes the mode of the underlying file, overridden - // the defaultMode and permissions initialized by the volume plugin, which - // is not what we want; thus, we skip chown/chmod for symlinks. - if info.Mode()&os.ModeSymlink != 0 { - return nil - } - - stat, ok := info.Sys().(*syscall.Stat_t) - if !ok { - return nil - } - - if stat == nil { - klog.Errorf("Got nil stat_t for path %v while setting ownership of volume", path) - return nil - } - - err = os.Chown(path, int(stat.Uid), int(*fsGroup)) - if err != nil { - klog.Errorf("Chown failed on %v: %v", path, err) - } - - mask := rwMask - if mounter.GetAttributes().ReadOnly { - mask = roMask - } - - if info.IsDir() { - mask |= os.ModeSetgid - mask |= execMask - } - - err = os.Chmod(path, info.Mode()|mask) - if err != nil { - klog.Errorf("Chmod failed on %v: %v", path, err) - } - - return nil + return changeFilePermission(path, fsGroup, mounter.GetAttributes().ReadOnly, info) }) } + +func changeFilePermission(filename string, fsGroup *int64, readonly bool, info os.FileInfo) error { + // chown and chmod pass through to the underlying file for symlinks. + // Symlinks have a mode of 777 but this really doesn't mean anything. + // The permissions of the underlying file are what matter. + // However, if one reads the mode of a symlink then chmods the symlink + // with that mode, it changes the mode of the underlying file, overridden + // the defaultMode and permissions initialized by the volume plugin, which + // is not what we want; thus, we skip chown/chmod for symlinks. + if info.Mode()&os.ModeSymlink != 0 { + return nil + } + + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return nil + } + + if stat == nil { + klog.Errorf("Got nil stat_t for path %v while setting ownership of volume", filename) + return nil + } + + err := os.Chown(filename, int(stat.Uid), int(*fsGroup)) + if err != nil { + klog.Errorf("Chown failed on %v: %v", filename, err) + } + + mask := rwMask + if readonly { + mask = roMask + } + + if info.IsDir() { + mask |= os.ModeSetgid + mask |= execMask + } + + err = os.Chmod(filename, info.Mode()|mask) + if err != nil { + klog.Errorf("Chmod failed on %v: %v", filename, err) + } + + return nil +} + +func skipPermissionChange(mounter Mounter, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) bool { + dir := mounter.GetPath() + + if fsGroupChangePolicy == nil || *fsGroupChangePolicy != v1.OnRootMismatch { + klog.V(4).Infof("perform recursive ownership change for %s", dir) + return false + } + return !requiresPermissionChange(mounter.GetPath(), fsGroup, mounter.GetAttributes().ReadOnly) +} + +func requiresPermissionChange(rootDir string, fsGroup *int64, readonly bool) bool { + fsInfo, err := os.Stat(rootDir) + if err != nil { + klog.Errorf("performing recursive ownership change on %s because reading permissions of root volume failed: %v", rootDir, err) + return true + } + stat, ok := fsInfo.Sys().(*syscall.Stat_t) + if !ok || stat == nil { + klog.Errorf("performing recursive ownership change on %s because reading permissions of root volume failed", rootDir) + return true + } + + if int(stat.Gid) != int(*fsGroup) { + klog.V(4).Infof("expected group ownership of volume %s did not match with: %d", rootDir, stat.Gid) + return true + } + unixPerms := rwMask + + if readonly { + unixPerms = roMask + } + + // if rootDir is not a directory then we should apply permission change anyways + if !fsInfo.IsDir() { + return true + } + unixPerms |= execMask + filePerm := fsInfo.Mode().Perm() + + // We need to check if actual permissions of root directory is a superset of permissions required by unixPerms + // and setgid bits are set in permissions of the directory. + if (unixPerms&filePerm != unixPerms) || (fsInfo.Mode()&os.ModeSetgid == 0) { + klog.V(4).Infof("performing recursive ownership change on %s because of mismatching mode", rootDir) + return true + } + return false +} + +// readDirNames reads the directory named by dirname and returns +// a list of directory entries. +// We are not using filepath.readDirNames because we do not want to sort files found in a directory before changing +// permissions for performance reasons. +func readDirNames(dirname string) ([]string, error) { + f, err := os.Open(dirname) + if err != nil { + return nil, err + } + names, err := f.Readdirnames(-1) + f.Close() + if err != nil { + return nil, err + } + return names, nil +} + +// walkDeep can be used to traverse directories and has two minor differences +// from filepath.Walk: +// - List of files/dirs is not sorted for performance reasons +// - callback walkFunc is invoked on root directory after visiting children dirs and files +func walkDeep(root string, walkFunc filepath.WalkFunc) error { + info, err := os.Lstat(root) + if err != nil { + return walkFunc(root, nil, err) + } + return walk(root, info, walkFunc) +} + +func walk(path string, info os.FileInfo, walkFunc filepath.WalkFunc) error { + if !info.IsDir() { + return walkFunc(path, info, nil) + } + names, err := readDirNames(path) + if err != nil { + return err + } + for _, name := range names { + filename := filepath.Join(path, name) + fileInfo, err := os.Lstat(filename) + if err != nil { + if err := walkFunc(filename, fileInfo, err); err != nil { + return err + } + } else { + err = walk(filename, fileInfo, walkFunc) + if err != nil { + return err + } + } + } + return walkFunc(path, info, nil) +} diff --git a/pkg/volume/volume_linux_test.go b/pkg/volume/volume_linux_test.go new file mode 100644 index 00000000000..3d0d43d35a4 --- /dev/null +++ b/pkg/volume/volume_linux_test.go @@ -0,0 +1,162 @@ +// +build linux + +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package volume + +import ( + "os" + "syscall" + "testing" + + v1 "k8s.io/api/core/v1" + utiltesting "k8s.io/client-go/util/testing" +) + +type localFakeMounter struct { + path string + attributes Attributes +} + +func (l *localFakeMounter) GetPath() string { + return l.path +} + +func (l *localFakeMounter) GetAttributes() Attributes { + return l.attributes +} + +func (l *localFakeMounter) CanMount() error { + return nil +} + +func (l *localFakeMounter) SetUp(mounterArgs MounterArgs) error { + return nil +} + +func (l *localFakeMounter) SetUpAt(dir string, mounterArgs MounterArgs) error { + return nil +} + +func (l *localFakeMounter) GetMetrics() (*Metrics, error) { + return nil, nil +} + +func TestSkipPermissionChange(t *testing.T) { + always := v1.AlwaysChangeVolumePermission + onrootMismatch := v1.OnRootMismatch + tests := []struct { + description string + fsGroupChangePolicy *v1.PodFSGroupChangePolicy + gidOwnerMatch bool + permissionMatch bool + sgidMatch bool + skipPermssion bool + }{ + { + description: "skippermission=false, policy=nil", + skipPermssion: false, + }, + { + description: "skippermission=false, policy=always", + fsGroupChangePolicy: &always, + skipPermssion: false, + }, + { + description: "skippermission=false, policy=onrootmismatch, gidmatch=false", + fsGroupChangePolicy: &onrootMismatch, + gidOwnerMatch: false, + skipPermssion: false, + }, + { + description: "skippermission=false, policy=onrootmismatch, gidmatch=true, permmatch=false", + fsGroupChangePolicy: &onrootMismatch, + gidOwnerMatch: true, + permissionMatch: false, + skipPermssion: false, + }, + { + description: "skippermission=false, policy=onrootmismatch, gidmatch=true, permmatch=true", + fsGroupChangePolicy: &onrootMismatch, + gidOwnerMatch: true, + permissionMatch: true, + skipPermssion: false, + }, + { + description: "skippermission=false, policy=onrootmismatch, gidmatch=true, permmatch=true, sgidmatch=true", + fsGroupChangePolicy: &onrootMismatch, + gidOwnerMatch: true, + permissionMatch: true, + sgidMatch: true, + skipPermssion: true, + }, + } + + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + tmpDir, err := utiltesting.MkTmpdir("volume_linux_test") + if err != nil { + t.Fatalf("error creating temp dir: %v", err) + } + + defer os.RemoveAll(tmpDir) + + info, err := os.Lstat(tmpDir) + if err != nil { + t.Fatalf("error reading permission of tmpdir: %v", err) + } + + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok || stat == nil { + t.Fatalf("error reading permission stats for tmpdir: %s", tmpDir) + } + + gid := stat.Gid + + var expectedGid int64 + + if test.gidOwnerMatch { + expectedGid = int64(gid) + } else { + expectedGid = int64(gid + 3000) + } + + mask := rwMask + + if test.sgidMatch { + mask |= os.ModeSetgid + } + + if test.permissionMatch { + mask |= execMask + + } + err = os.Chmod(tmpDir, info.Mode()|mask) + if err != nil { + t.Errorf("Chmod failed on %v: %v", tmpDir, err) + } + + mounter := &localFakeMounter{path: tmpDir} + ok = skipPermissionChange(mounter, &expectedGid, test.fsGroupChangePolicy) + if ok != test.skipPermssion { + t.Errorf("for %s expected skipPermission to be %v got %v", test.description, test.skipPermssion, ok) + } + + }) + } + +} diff --git a/pkg/volume/volume_unsupported.go b/pkg/volume/volume_unsupported.go index 45a6cc5ca7f..3c9a50bcbba 100644 --- a/pkg/volume/volume_unsupported.go +++ b/pkg/volume/volume_unsupported.go @@ -18,6 +18,10 @@ limitations under the License. package volume -func SetVolumeOwnership(mounter Mounter, fsGroup *int64) error { +import ( + v1 "k8s.io/api/core/v1" +) + +func SetVolumeOwnership(mounter Mounter, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) error { return nil } diff --git a/pkg/volume/vsphere_volume/vsphere_volume.go b/pkg/volume/vsphere_volume/vsphere_volume.go index cd6741947bb..8f03f9ec1ae 100644 --- a/pkg/volume/vsphere_volume/vsphere_volume.go +++ b/pkg/volume/vsphere_volume/vsphere_volume.go @@ -270,7 +270,7 @@ func (b *vsphereVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArg os.Remove(dir) return err } - volume.SetVolumeOwnership(b, mounterArgs.FsGroup) + volume.SetVolumeOwnership(b, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) klog.V(3).Infof("vSphere volume %s mounted to %s", b.volPath, dir) return nil