From b0f1337200fd7f7f89b833848553eb0ed0f4c112 Mon Sep 17 00:00:00 2001 From: Maxim Patlasov Date: Thu, 30 Mar 2023 13:27:58 -0700 Subject: [PATCH] Fix directory mismatch for `volume.SetVolumeOwnership()` In most cases `dir` arg of `SetUpAt()` method of `volume.Mounter` interface is the same as `mounter.GetPath()` because we usually call `SetUpAt()` from `SetUp()` like this:" ``` func (ed *emptyDir) SetUp(mounterArgs volume.MounterArgs) error { return ed.SetUpAt(ed.GetPath(), mounterArgs) } ``` (this example is from `volume/emptydir/empty_dir.go`, but there are plenty other examples like that in `volume/*`) However, there is currently one exception. This is from `volume/projected/projected.go`: ``` if err := wrapped.SetUpAt(dir, mounterArgs); err != nil { return err } ``` (see https://github.com/kubernetes/kubernetes/blob/96306f144a3c917a97fe27c9ad5dd89e4213f741/pkg/volume/projected/projected.go#L203) In this case `dir` is not equal to `wrapped.GetPath()` and `volume.SetVolumeOwnership()` fails when called from `SetUpAt()` of wrapped volume: ``` lstat /var/lib/kubelet/pods/a2f6e58f-7edf-4c48-a97c-ef1b8fd3caf6/volumes/kubernetes.io~empty-dir/wrapped_kube-api-access-knvkv: no such file or directory ``` To fix the issue let's pass `dir` arg to `volume.SetVolumeOwnership()` explicitly, and use it instead of `mounter.GetPath()`. --- pkg/volume/awsebs/aws_ebs.go | 2 +- pkg/volume/azuredd/azure_mounter.go | 2 +- pkg/volume/configmap/configmap.go | 2 +- pkg/volume/csi/csi_mounter.go | 2 +- pkg/volume/downwardapi/downwardapi.go | 2 +- pkg/volume/emptydir/empty_dir.go | 2 +- pkg/volume/fc/disk_manager.go | 2 +- pkg/volume/flexvolume/mounter.go | 2 +- pkg/volume/gcepd/gce_pd.go | 2 +- pkg/volume/git_repo/git_repo.go | 2 +- pkg/volume/iscsi/disk_manager.go | 2 +- pkg/volume/local/local.go | 2 +- pkg/volume/portworx/portworx.go | 2 +- pkg/volume/projected/projected.go | 2 +- pkg/volume/rbd/disk_manager.go | 2 +- pkg/volume/secret/secret.go | 2 +- pkg/volume/volume_linux.go | 16 +++++++--------- pkg/volume/volume_linux_test.go | 8 ++++---- pkg/volume/volume_unsupported.go | 2 +- pkg/volume/vsphere_volume/vsphere_volume.go | 2 +- 20 files changed, 29 insertions(+), 31 deletions(-) diff --git a/pkg/volume/awsebs/aws_ebs.go b/pkg/volume/awsebs/aws_ebs.go index 0801982ffcd8..2e675a70d77c 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, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) } klog.V(4).Infof("Successfully mounted %s", dir) diff --git a/pkg/volume/azuredd/azure_mounter.go b/pkg/volume/azuredd/azure_mounter.go index 0fe6eec52372..789e75168569 100644 --- a/pkg/volume/azuredd/azure_mounter.go +++ b/pkg/volume/azuredd/azure_mounter.go @@ -160,7 +160,7 @@ func (m *azureDiskMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) e } if volumeSource.ReadOnly == nil || !*volumeSource.ReadOnly { - volume.SetVolumeOwnership(m, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(m.plugin, m.spec)) + volume.SetVolumeOwnership(m, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(m.plugin, m.spec)) } klog.V(2).Infof("azureDisk - successfully mounted disk %s on %s", diskName, dir) diff --git a/pkg/volume/configmap/configmap.go b/pkg/volume/configmap/configmap.go index cdfba9480cd7..181dd55fec45 100644 --- a/pkg/volume/configmap/configmap.go +++ b/pkg/volume/configmap/configmap.go @@ -255,7 +255,7 @@ func (b *configMapVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA return err } - err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + err = volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, 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 1974b0367531..468f882b8845 100644 --- a/pkg/volume/csi/csi_mounter.go +++ b/pkg/volume/csi/csi_mounter.go @@ -333,7 +333,7 @@ func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error // Driver doesn't support applying FSGroup. Kubelet must apply it instead. // fullPluginName helps to distinguish different driver from csi plugin - err := volume.SetVolumeOwnership(c, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(c.plugin, c.spec)) + err := volume.SetVolumeOwnership(c, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(c.plugin, c.spec)) 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 diff --git a/pkg/volume/downwardapi/downwardapi.go b/pkg/volume/downwardapi/downwardapi.go index 8338850ac42e..3edd3090df01 100644 --- a/pkg/volume/downwardapi/downwardapi.go +++ b/pkg/volume/downwardapi/downwardapi.go @@ -226,7 +226,7 @@ func (b *downwardAPIVolumeMounter) SetUpAt(dir string, mounterArgs volume.Mounte return err } - err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + err = volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, 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 9ad981c54bd4..e75bccd4927e 100644 --- a/pkg/volume/emptydir/empty_dir.go +++ b/pkg/volume/emptydir/empty_dir.go @@ -280,7 +280,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, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(ed.plugin, nil)) + volume.SetVolumeOwnership(ed, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(ed.plugin, 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 bb054ea16618..02e15c4f85c5 100644 --- a/pkg/volume/fc/disk_manager.go +++ b/pkg/volume/fc/disk_manager.go @@ -91,7 +91,7 @@ func diskSetUp(manager diskManager, b fcDiskMounter, volPath string, mounter mou } if !b.readOnly { - volume.SetVolumeOwnership(&b, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + volume.SetVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) } return nil diff --git a/pkg/volume/flexvolume/mounter.go b/pkg/volume/flexvolume/mounter.go index 8098cfdb66ee..3821af7e9235 100644 --- a/pkg/volume/flexvolume/mounter.go +++ b/pkg/volume/flexvolume/mounter.go @@ -95,7 +95,7 @@ func (f *flexVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) if !f.readOnly { if f.plugin.capabilities.FSGroup { // fullPluginName helps to distinguish different driver from flex volume plugin - volume.SetVolumeOwnership(f, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(f.plugin, f.spec)) + volume.SetVolumeOwnership(f, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(f.plugin, f.spec)) } } diff --git a/pkg/volume/gcepd/gce_pd.go b/pkg/volume/gcepd/gce_pd.go index 0df9ef10f5a1..372a8f6837f0 100644 --- a/pkg/volume/gcepd/gce_pd.go +++ b/pkg/volume/gcepd/gce_pd.go @@ -430,7 +430,7 @@ func (b *gcePersistentDiskMounter) SetUpAt(dir string, mounterArgs volume.Mounte klog.V(4).Infof("mount of disk %s succeeded", dir) if !b.readOnly { - if err := volume.SetVolumeOwnership(b, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)); err != nil { + if err := volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)); err != nil { klog.Errorf("SetVolumeOwnership returns error %v", err) } } diff --git a/pkg/volume/git_repo/git_repo.go b/pkg/volume/git_repo/git_repo.go index fe890032e27d..995018d90072 100644 --- a/pkg/volume/git_repo/git_repo.go +++ b/pkg/volume/git_repo/git_repo.go @@ -235,7 +235,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, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) volumeutil.SetReady(b.getMetaDir()) return nil diff --git a/pkg/volume/iscsi/disk_manager.go b/pkg/volume/iscsi/disk_manager.go index 6d60e44efaf0..6aa8652bd6b0 100644 --- a/pkg/volume/iscsi/disk_manager.go +++ b/pkg/volume/iscsi/disk_manager.go @@ -96,7 +96,7 @@ func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter } if !b.readOnly { - volume.SetVolumeOwnership(&b, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + volume.SetVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) } return nil diff --git a/pkg/volume/local/local.go b/pkg/volume/local/local.go index c55a3502e79f..f1192b9688a9 100644 --- a/pkg/volume/local/local.go +++ b/pkg/volume/local/local.go @@ -615,7 +615,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, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(m.plugin, nil)) + return volume.SetVolumeOwnership(m, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(m.plugin, nil)) } } return nil diff --git a/pkg/volume/portworx/portworx.go b/pkg/volume/portworx/portworx.go index 6cba50327164..7be543a54edc 100644 --- a/pkg/volume/portworx/portworx.go +++ b/pkg/volume/portworx/portworx.go @@ -335,7 +335,7 @@ func (b *portworxVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterAr return err } if !b.readOnly { - volume.SetVolumeOwnership(b, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) } 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 a48567b2a6a0..27fa3f0fe097 100644 --- a/pkg/volume/projected/projected.go +++ b/pkg/volume/projected/projected.go @@ -236,7 +236,7 @@ func (s *projectedVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA return err } - err = volume.SetVolumeOwnership(s, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(s.plugin, nil)) + err = volume.SetVolumeOwnership(s, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(s.plugin, 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 edff33540f4d..2131c7ecedd8 100644 --- a/pkg/volume/rbd/disk_manager.go +++ b/pkg/volume/rbd/disk_manager.go @@ -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, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + volume.SetVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) } return nil diff --git a/pkg/volume/secret/secret.go b/pkg/volume/secret/secret.go index fa06d879303e..e12d5cf86b81 100644 --- a/pkg/volume/secret/secret.go +++ b/pkg/volume/secret/secret.go @@ -250,7 +250,7 @@ func (b *secretVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs return err } - err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + err = volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) if err != nil { klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup) return err diff --git a/pkg/volume/volume_linux.go b/pkg/volume/volume_linux.go index 57c02815029a..ec7f6da4bfe9 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -40,22 +40,22 @@ 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, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) error { +func SetVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) error { if fsGroup == nil { return nil } timer := time.AfterFunc(30*time.Second, func() { - 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()) + 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", dir) }) defer timer.Stop() - if skipPermissionChange(mounter, fsGroup, fsGroupChangePolicy) { - klog.V(3).InfoS("Skipping permission and ownership change for volume", "path", mounter.GetPath()) + if skipPermissionChange(mounter, dir, fsGroup, fsGroupChangePolicy) { + klog.V(3).InfoS("Skipping permission and ownership change for volume", "path", dir) return nil } - err := walkDeep(mounter.GetPath(), func(path string, info os.FileInfo, err error) error { + err := walkDeep(dir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } @@ -104,14 +104,12 @@ func changeFilePermission(filename string, fsGroup *int64, readonly bool, info o return nil } -func skipPermissionChange(mounter Mounter, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) bool { - dir := mounter.GetPath() - +func skipPermissionChange(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) bool { if fsGroupChangePolicy == nil || *fsGroupChangePolicy != v1.FSGroupChangeOnRootMismatch { klog.V(4).InfoS("Perform recursive ownership change for directory", "path", dir) return false } - return !requiresPermissionChange(mounter.GetPath(), fsGroup, mounter.GetAttributes().ReadOnly) + return !requiresPermissionChange(dir, fsGroup, mounter.GetAttributes().ReadOnly) } func requiresPermissionChange(rootDir string, fsGroup *int64, readonly bool) bool { diff --git a/pkg/volume/volume_linux_test.go b/pkg/volume/volume_linux_test.go index a4078879ab4d..180e6d164654 100644 --- a/pkg/volume/volume_linux_test.go +++ b/pkg/volume/volume_linux_test.go @@ -166,7 +166,7 @@ func TestSkipPermissionChange(t *testing.T) { } mounter := &localFakeMounter{path: tmpDir} - ok = skipPermissionChange(mounter, &expectedGid, test.fsGroupChangePolicy) + ok = skipPermissionChange(mounter, tmpDir, &expectedGid, test.fsGroupChangePolicy) if ok != test.skipPermssion { t.Errorf("for %s expected skipPermission to be %v got %v", test.description, test.skipPermssion, ok) } @@ -302,8 +302,8 @@ func TestSetVolumeOwnershipMode(t *testing.T) { t.Errorf("for %s error running setup with: %v", test.description, err) } - mounter := &localFakeMounter{path: tmpDir} - err = SetVolumeOwnership(mounter, &expectedGid, test.fsGroupChangePolicy, nil) + mounter := &localFakeMounter{path: "FAKE_DIR_DOESNT_EXIST"} // SetVolumeOwnership() must rely on tmpDir + err = SetVolumeOwnership(mounter, tmpDir, &expectedGid, test.fsGroupChangePolicy, nil) if err != nil { t.Errorf("for %s error changing ownership with: %v", test.description, err) } @@ -439,7 +439,7 @@ func TestSetVolumeOwnershipOwner(t *testing.T) { mounter := &localFakeMounter{path: tmpDir} always := v1.FSGroupChangeAlways - err = SetVolumeOwnership(mounter, test.fsGroup, &always, nil) + err = SetVolumeOwnership(mounter, tmpDir, test.fsGroup, &always, nil) if err != nil { t.Errorf("for %s error changing ownership with: %v", test.description, err) } diff --git a/pkg/volume/volume_unsupported.go b/pkg/volume/volume_unsupported.go index 20c56d4b63e2..3b5a200a6160 100644 --- a/pkg/volume/volume_unsupported.go +++ b/pkg/volume/volume_unsupported.go @@ -24,6 +24,6 @@ import ( "k8s.io/kubernetes/pkg/volume/util/types" ) -func SetVolumeOwnership(mounter Mounter, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) error { +func SetVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) error { return nil } diff --git a/pkg/volume/vsphere_volume/vsphere_volume.go b/pkg/volume/vsphere_volume/vsphere_volume.go index 6f7d02974c56..2fe779b9713a 100644 --- a/pkg/volume/vsphere_volume/vsphere_volume.go +++ b/pkg/volume/vsphere_volume/vsphere_volume.go @@ -277,7 +277,7 @@ func (b *vsphereVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArg os.Remove(dir) return err } - volume.SetVolumeOwnership(b, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) klog.V(3).Infof("vSphere volume %s mounted to %s", b.volPath, dir) return nil