Skip to content

Automated cherry pick of #117022: Fix directory mismatch for volume.SetVolumeOwnership() #117574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 3, 2023
Merged

Automated cherry pick of #117022: Fix directory mismatch for volume.SetVolumeOwnership() #117574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/volume/configmap/configmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ func (b *configMapVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA
setPerms := func(_ string) error {
// This may be the first time writing and new files get created outside the timestamp subdirectory:
// change the permissions on the whole volume and not only in the timestamp directory.
return volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
return volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
}
err = writer.Write(payload, setPerms)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/csi/csi_mounter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/downwardapi/downwardapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func (b *downwardAPIVolumeMounter) SetUpAt(dir string, mounterArgs volume.Mounte
setPerms := func(_ string) error {
// This may be the first time writing and new files get created outside the timestamp subdirectory:
// change the permissions on the whole volume and not only in the timestamp directory.
return volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
return volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
}
err = writer.Write(data, setPerms)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/emptydir/empty_dir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/fc/disk_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/flexvolume/mounter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/gcepd/gce_pd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/git_repo/git_repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/iscsi/disk_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/local/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/portworx/portworx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/projected/projected.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ func (s *projectedVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA
setPerms := func(_ string) error {
// This may be the first time writing and new files get created outside the timestamp subdirectory:
// change the permissions on the whole volume and not only in the timestamp directory.
return volume.SetVolumeOwnership(s, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(s.plugin, nil))
return volume.SetVolumeOwnership(s, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(s.plugin, nil))
}
err = writer.Write(data, setPerms)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/rbd/disk_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/secret/secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func (b *secretVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs
setPerms := func(_ string) error {
// This may be the first time writing and new files get created outside the timestamp subdirectory:
// change the permissions on the whole volume and not only in the timestamp directory.
return volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
return volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
}
err = writer.Write(payload, setPerms)
if err != nil {
Expand Down
16 changes: 7 additions & 9 deletions pkg/volume/volume_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions pkg/volume/volume_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/volume_unsupported.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
2 changes: 1 addition & 1 deletion pkg/volume/vsphere_volume/vsphere_volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down