diff --git a/pkg/kubelet/events/event.go b/pkg/kubelet/events/event.go index 0eaa82fc77f..526a67cdb2b 100644 --- a/pkg/kubelet/events/event.go +++ b/pkg/kubelet/events/event.go @@ -61,6 +61,7 @@ const ( VolumeResizeFailed = "VolumeResizeFailed" VolumeResizeSuccess = "VolumeResizeSuccessful" FileSystemResizeFailed = "FileSystemResizeFailed" + VolumePermissionChangeInProgress = "VolumePermissionChangeInProgress" FileSystemResizeSuccess = "FileSystemResizeSuccessful" FailedMapVolume = "FailedMapVolume" WarnAlreadyMountedVolume = "AlreadyMountedVolume" diff --git a/pkg/volume/configmap/configmap.go b/pkg/volume/configmap/configmap.go index 53d03bb70b4..8bbf50c4b59 100644 --- a/pkg/volume/configmap/configmap.go +++ b/pkg/volume/configmap/configmap.go @@ -246,7 +246,8 @@ 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, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + ownerShipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + return ownerShipChanger.ChangePermissions() } err = writer.Write(payload, setPerms) if err != nil { diff --git a/pkg/volume/csi/csi_mounter.go b/pkg/volume/csi/csi_mounter.go index b31a777c85a..4f024201955 100644 --- a/pkg/volume/csi/csi_mounter.go +++ b/pkg/volume/csi/csi_mounter.go @@ -335,7 +335,9 @@ 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, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(c.plugin, c.spec)) + ownershipChanger := volume.NewVolumeOwnership(c, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(c.plugin, c.spec)) + ownershipChanger.AddProgressNotifier(c.pod, mounterArgs.Recorder) + err = ownershipChanger.ChangePermissions() 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 4c544516a52..d3790b4cd56 100644 --- a/pkg/volume/downwardapi/downwardapi.go +++ b/pkg/volume/downwardapi/downwardapi.go @@ -217,7 +217,8 @@ 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, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + return ownershipChanger.ChangePermissions() } err = writer.Write(data, setPerms) if err != nil { diff --git a/pkg/volume/emptydir/empty_dir.go b/pkg/volume/emptydir/empty_dir.go index ed2f206fb38..a9128f661fa 100644 --- a/pkg/volume/emptydir/empty_dir.go +++ b/pkg/volume/emptydir/empty_dir.go @@ -18,10 +18,11 @@ package emptydir import ( "fmt" - "k8s.io/kubernetes/pkg/kubelet/util/swap" "os" "path/filepath" + "k8s.io/kubernetes/pkg/kubelet/util/swap" + "k8s.io/klog/v2" "k8s.io/mount-utils" utilstrings "k8s.io/utils/strings" @@ -278,7 +279,8 @@ func (ed *emptyDir) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { err = fmt.Errorf("unknown storage medium %q", ed.medium) } - volume.SetVolumeOwnership(ed, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(ed.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(ed, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(ed.plugin, nil)) + _ = ownershipChanger.ChangePermissions() // 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 02e15c4f85c..05b2df553ec 100644 --- a/pkg/volume/fc/disk_manager.go +++ b/pkg/volume/fc/disk_manager.go @@ -91,7 +91,9 @@ func diskSetUp(manager diskManager, b fcDiskMounter, volPath string, mounter mou } if !b.readOnly { - volume.SetVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + // TODO: Handle error returned here properly. + _ = ownershipChanger.ChangePermissions() } return nil diff --git a/pkg/volume/flexvolume/mounter.go b/pkg/volume/flexvolume/mounter.go index 3821af7e923..6b240887641 100644 --- a/pkg/volume/flexvolume/mounter.go +++ b/pkg/volume/flexvolume/mounter.go @@ -95,7 +95,8 @@ 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, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(f.plugin, f.spec)) + ownershipChanger := volume.NewVolumeOwnership(f, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(f.plugin, f.spec)) + _ = ownershipChanger.ChangePermissions() } } diff --git a/pkg/volume/git_repo/git_repo.go b/pkg/volume/git_repo/git_repo.go index 9211bf8bc6f..3a4bf850f58 100644 --- a/pkg/volume/git_repo/git_repo.go +++ b/pkg/volume/git_repo/git_repo.go @@ -229,8 +229,9 @@ 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, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) - + ownershipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + // We do not care about return value, this plugin is deprecated + _ = ownershipChanger.ChangePermissions() volumeutil.SetReady(b.getMetaDir()) return nil } diff --git a/pkg/volume/iscsi/disk_manager.go b/pkg/volume/iscsi/disk_manager.go index 6aa8652bd6b..4f4ff341e71 100644 --- a/pkg/volume/iscsi/disk_manager.go +++ b/pkg/volume/iscsi/disk_manager.go @@ -19,7 +19,6 @@ package iscsi import ( "os" - v1 "k8s.io/api/core/v1" "k8s.io/klog/v2" "k8s.io/mount-utils" @@ -42,7 +41,9 @@ 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, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) error { +func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter mount.Interface, mounterArgs volume.MounterArgs) error { + fsGroup := mounterArgs.FsGroup + fsGroupChangePolicy := mounterArgs.FSGroupChangePolicy notMnt, err := mounter.IsLikelyNotMountPoint(volPath) if err != nil && !os.IsNotExist(err) { klog.Errorf("cannot validate mountpoint: %s", volPath) @@ -96,7 +97,9 @@ func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter } if !b.readOnly { - volume.SetVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + // This code requires larger refactor to monitor progress of ownership change + ownershipChanger := volume.NewVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + _ = ownershipChanger.ChangePermissions() } return nil diff --git a/pkg/volume/iscsi/iscsi.go b/pkg/volume/iscsi/iscsi.go index 9300d006491..0e94a0c84e8 100644 --- a/pkg/volume/iscsi/iscsi.go +++ b/pkg/volume/iscsi/iscsi.go @@ -377,7 +377,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, mounterArgs.FSGroupChangePolicy) + err := diskSetUp(b.manager, *b, dir, b.mounter, mounterArgs) if err != nil { klog.Errorf("iscsi: failed to setup") } diff --git a/pkg/volume/local/local.go b/pkg/volume/local/local.go index 19ad6fb661b..4d78faefb5e 100644 --- a/pkg/volume/local/local.go +++ b/pkg/volume/local/local.go @@ -610,7 +610,9 @@ 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, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(m.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(m, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(m.plugin, nil)) + ownershipChanger.AddProgressNotifier(m.pod, mounterArgs.Recorder) + return ownershipChanger.ChangePermissions() } } return nil diff --git a/pkg/volume/portworx/portworx.go b/pkg/volume/portworx/portworx.go index baa623ab4b4..bc56928c517 100644 --- a/pkg/volume/portworx/portworx.go +++ b/pkg/volume/portworx/portworx.go @@ -331,7 +331,9 @@ func (b *portworxVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterAr return err } if !b.readOnly { - volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + // Since portworxVolume is in process of being removed from in-tree, we avoid larger refactor to add progress tracking for ownership operation + ownershipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + _ = ownershipChanger.ChangePermissions() } 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 ae18a81af8f..291fc88e8ae 100644 --- a/pkg/volume/projected/projected.go +++ b/pkg/volume/projected/projected.go @@ -229,7 +229,8 @@ 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, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(s.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(s, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(s.plugin, nil)) + return ownershipChanger.ChangePermissions() } err = writer.Write(data, setPerms) if err != nil { diff --git a/pkg/volume/secret/secret.go b/pkg/volume/secret/secret.go index 6daa2c47732..06cb178fdc5 100644 --- a/pkg/volume/secret/secret.go +++ b/pkg/volume/secret/secret.go @@ -242,7 +242,8 @@ 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, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + return ownershipChanger.ChangePermissions() } err = writer.Write(payload, setPerms) if err != nil { diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 5523b61a5ec..0537edae52c 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -584,6 +584,7 @@ func (og *operationGenerator) GenerateMountVolumeFunc( FsGroup: fsGroup, DesiredSize: volumeToMount.DesiredSizeLimit, FSGroupChangePolicy: fsGroupChangePolicy, + Recorder: og.recorder, SELinuxLabel: volumeToMount.SELinuxLabel, }) // Update actual state of world diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 70bf7decc5b..f5f933e0c6a 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -17,12 +17,15 @@ limitations under the License. package volume import ( + "sync/atomic" "time" 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" + "k8s.io/client-go/tools/record" + volumetypes "k8s.io/kubernetes/pkg/volume/util/types" ) // Volume represents a directory used by pods or hosts on a node. All method @@ -130,6 +133,20 @@ type MounterArgs struct { FSGroupChangePolicy *v1.PodFSGroupChangePolicy DesiredSize *resource.Quantity SELinuxLabel string + Recorder record.EventRecorder +} + +type VolumeOwnership struct { + mounter Mounter + dir string + fsGroup *int64 + fsGroupChangePolicy *v1.PodFSGroupChangePolicy + completionCallback func(volumetypes.CompleteFuncParam) + + // for monitoring progress of permission change operation + pod *v1.Pod + fileCounter atomic.Int64 + recorder record.EventRecorder } // 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 ec7f6da4bfe..9d023abfa05 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -20,14 +20,19 @@ limitations under the License. package volume import ( + "context" + "fmt" "path/filepath" + "strings" "syscall" "os" "time" v1 "k8s.io/api/core/v1" + "k8s.io/client-go/tools/record" "k8s.io/klog/v2" + "k8s.io/kubernetes/pkg/kubelet/events" "k8s.io/kubernetes/pkg/volume/util/types" ) @@ -37,38 +42,110 @@ const ( execMask = os.FileMode(0110) ) -// 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, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) error { - if fsGroup == nil { +var ( + // function that will be used for changing file permissions on linux + // mainly stored here as a variable so as it can replaced in tests + filePermissionChangeFunc = changeFilePermission + progressReportDuration = 60 * time.Second + firstEventReportDuration = 30 * time.Second +) + +// NewVolumeOwnership returns an interface that can be used to recursively change volume permissions and ownership +func NewVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) *VolumeOwnership { + vo := &VolumeOwnership{ + mounter: mounter, + dir: dir, + fsGroup: fsGroup, + fsGroupChangePolicy: fsGroupChangePolicy, + completionCallback: completeFunc, + } + vo.fileCounter.Store(0) + return vo +} + +func (vo *VolumeOwnership) AddProgressNotifier(pod *v1.Pod, recorder record.EventRecorder) *VolumeOwnership { + vo.pod = pod + vo.recorder = recorder + return vo +} + +func (vo *VolumeOwnership) ChangePermissions() error { + if vo.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", dir) + if skipPermissionChange(vo.mounter, vo.dir, vo.fsGroup, vo.fsGroupChangePolicy) { + klog.V(3).InfoS("Skipping permission and ownership change for volume", "path", vo.dir) + return nil + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + timer := time.AfterFunc(firstEventReportDuration, func() { + vo.initiateProgressMonitor(ctx) }) defer timer.Stop() - if skipPermissionChange(mounter, dir, fsGroup, fsGroupChangePolicy) { - klog.V(3).InfoS("Skipping permission and ownership change for volume", "path", dir) - return nil - } + return vo.changePermissionsRecursively() +} - err := walkDeep(dir, func(path string, info os.FileInfo, err error) error { +func (vo *VolumeOwnership) initiateProgressMonitor(ctx context.Context) { + 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", vo.dir) + if vo.pod != nil { + go vo.monitorProgress(ctx) + } +} + +func (vo *VolumeOwnership) changePermissionsRecursively() error { + err := walkDeep(vo.dir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } - return changeFilePermission(path, fsGroup, mounter.GetAttributes().ReadOnly, info) + vo.fileCounter.Add(1) + return filePermissionChangeFunc(path, vo.fsGroup, vo.mounter.GetAttributes().ReadOnly, info) }) - if completeFunc != nil { - completeFunc(types.CompleteFuncParam{ + + if vo.completionCallback != nil { + vo.completionCallback(types.CompleteFuncParam{ Err: &err, }) } return err } +func (vo *VolumeOwnership) monitorProgress(ctx context.Context) { + dirName := getDirnameToReport(vo.dir, string(vo.pod.UID)) + msg := fmt.Sprintf("Setting volume ownership for %s is taking longer than expected, consider using OnRootMismatch - https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods", dirName) + vo.recorder.Event(vo.pod, v1.EventTypeWarning, events.VolumePermissionChangeInProgress, msg) + ticker := time.NewTicker(progressReportDuration) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + vo.logWarning() + } + } +} + +// report everything after podUID in dir string, including podUID +func getDirnameToReport(dir, podUID string) string { + podUIDIndex := strings.Index(dir, podUID) + if podUIDIndex == -1 { + return dir + } + return dir[podUIDIndex:] +} + +func (vo *VolumeOwnership) logWarning() { + dirName := getDirnameToReport(vo.dir, string(vo.pod.UID)) + msg := fmt.Sprintf("Setting volume ownership for %s, processed %d files.", dirName, vo.fileCounter.Load()) + klog.Warning(msg) + vo.recorder.Event(vo.pod, v1.EventTypeWarning, events.VolumePermissionChangeInProgress, msg) +} + func changeFilePermission(filename string, fsGroup *int64, readonly bool, info os.FileInfo) error { err := os.Lchown(filename, -1, int(*fsGroup)) if err != nil { diff --git a/pkg/volume/volume_linux_test.go b/pkg/volume/volume_linux_test.go index 180e6d16465..171b7c63f00 100644 --- a/pkg/volume/volume_linux_test.go +++ b/pkg/volume/volume_linux_test.go @@ -25,8 +25,11 @@ import ( "path/filepath" "syscall" "testing" + "time" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" utiltesting "k8s.io/client-go/util/testing" ) @@ -123,8 +126,12 @@ func TestSkipPermissionChange(t *testing.T) { if err != nil { t.Fatalf("error creating temp dir: %v", err) } - - defer os.RemoveAll(tmpDir) + defer func() { + err := os.RemoveAll(tmpDir) + if err != nil { + t.Fatalf("error removing tmpDir %s: %v", tmpDir, err) + } + }() info, err := os.Lstat(tmpDir) if err != nil { @@ -138,12 +145,12 @@ func TestSkipPermissionChange(t *testing.T) { gid := stat.Gid - var expectedGid int64 + var expectedGID int64 if test.gidOwnerMatch { - expectedGid = int64(gid) + expectedGID = int64(gid) } else { - expectedGid = int64(gid + 3000) + expectedGID = int64(gid + 3000) } mask := rwMask @@ -166,7 +173,7 @@ func TestSkipPermissionChange(t *testing.T) { } mounter := &localFakeMounter{path: tmpDir} - ok = skipPermissionChange(mounter, tmpDir, &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) } @@ -285,7 +292,13 @@ func TestSetVolumeOwnershipMode(t *testing.T) { t.Fatalf("error creating temp dir: %v", err) } - defer os.RemoveAll(tmpDir) + defer func() { + err := os.RemoveAll(tmpDir) + if err != nil { + t.Fatalf("error removing tmpDir %s: %v", tmpDir, err) + } + }() + info, err := os.Lstat(tmpDir) if err != nil { t.Fatalf("error reading permission of tmpdir: %v", err) @@ -296,14 +309,15 @@ func TestSetVolumeOwnershipMode(t *testing.T) { t.Fatalf("error reading permission stats for tmpdir: %s", tmpDir) } - var expectedGid int64 = int64(stat.Gid) + var expectedGID = int64(stat.Gid) err = test.setupFunc(tmpDir) if err != nil { t.Errorf("for %s error running setup with: %v", test.description, err) } mounter := &localFakeMounter{path: "FAKE_DIR_DOESNT_EXIST"} // SetVolumeOwnership() must rely on tmpDir - err = SetVolumeOwnership(mounter, tmpDir, &expectedGid, test.fsGroupChangePolicy, nil) + ownershipChanger := NewVolumeOwnership(mounter, tmpDir, &expectedGID, test.fsGroupChangePolicy, nil) + err = ownershipChanger.ChangePermissions() if err != nil { t.Errorf("for %s error changing ownership with: %v", test.description, err) } @@ -315,6 +329,130 @@ func TestSetVolumeOwnershipMode(t *testing.T) { } } +func TestProgressTracking(t *testing.T) { + alwaysApplyPolicy := v1.FSGroupChangeAlways + var expectedGID int64 = 9999 + var permissionSleepDuration = 5 * time.Millisecond + // how often to report the events + progressReportDuration = 200 * time.Millisecond + firstEventReportDuration = 50 * time.Millisecond + + filePermissionChangeFunc = func(filename string, fsGroup *int64, _ bool, _ os.FileInfo) error { + t.Logf("Calling file permission change for %s with gid %d", filename, *fsGroup) + time.Sleep(permissionSleepDuration) + return nil + } + pod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + UID: "pod1uid", + }, + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + Name: "volume-name", + VolumeSource: v1.VolumeSource{ + GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{ + PDName: "fake-device1", + }, + }, + }, + }, + }, + } + + tests := []struct { + name string + filePermissionChangeTimeDuration time.Duration + totalWaitTime time.Duration + currentPod *v1.Pod + expectedEvents []string + }{ + { + name: "When permission change finishes quickly, no events should be logged", + filePermissionChangeTimeDuration: 30 * time.Millisecond, + totalWaitTime: 1 * time.Second, + currentPod: pod, + expectedEvents: []string{}, + }, + { + name: "When no pod is specified, no events should be logged", + filePermissionChangeTimeDuration: 300 * time.Millisecond, + totalWaitTime: 1 * time.Second, + currentPod: nil, + expectedEvents: []string{}, + }, + { + name: "When permission change takes loo long and pod is specified", + filePermissionChangeTimeDuration: 300 * time.Millisecond, + totalWaitTime: 1 * time.Second, + currentPod: pod, + expectedEvents: []string{ + "Warning VolumePermissionChangeInProgress Setting volume ownership for pod1uid/volumes/faketype is taking longer than expected, consider using OnRootMismatch - https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods", + "Warning VolumePermissionChangeInProgress Setting volume ownership for pod1uid/volumes/faketype, processed 1 files.", + }, + }, + } + + for i := range tests { + tc := tests[i] + t.Run(tc.name, func(t *testing.T) { + tmpDir, err := utiltesting.MkTmpdir("volume_linux_ownership") + if err != nil { + t.Fatalf("error creating temp dir: %v", err) + } + podUID := "placeholder" + if tc.currentPod != nil { + podUID = string(tc.currentPod.UID) + } + volumePath := filepath.Join(tmpDir, podUID, "volumes", "faketype") + err = os.MkdirAll(volumePath, 0770) + if err != nil { + t.Fatalf("error creating volumePath %s: %v", volumePath, err) + } + defer func() { + err := os.RemoveAll(tmpDir) + if err != nil { + t.Fatalf("error removing tmpDir %s: %v", tmpDir, err) + } + }() + + mounter := &localFakeMounter{path: "FAKE_DIR_DOESNT_EXIST"} // SetVolumeOwnership() must rely on tmpDir + + fakeRecorder := record.NewFakeRecorder(100) + recordedEvents := []string{} + + // Set how long file permission change takes + permissionSleepDuration = tc.filePermissionChangeTimeDuration + + ownershipChanger := NewVolumeOwnership(mounter, volumePath, &expectedGID, &alwaysApplyPolicy, nil) + if tc.currentPod != nil { + ownershipChanger.AddProgressNotifier(tc.currentPod, fakeRecorder) + } + err = ownershipChanger.ChangePermissions() + if err != nil { + t.Errorf("unexpected error: %+v", err) + } + time.Sleep(tc.totalWaitTime) + actualEventCount := len(fakeRecorder.Events) + if len(tc.expectedEvents) == 0 && actualEventCount != len(tc.expectedEvents) { + t.Errorf("expected 0 events got %d", actualEventCount) + } + + for range actualEventCount { + event := <-fakeRecorder.Events + recordedEvents = append(recordedEvents, event) + } + + for i, event := range tc.expectedEvents { + if event != recordedEvents[i] { + t.Errorf("expected event %d to be %s, got: %s", i, event, recordedEvents[i]) + } + } + }) + } +} + // verifyDirectoryPermission checks if given path has directory permissions // that is expected by k8s. If returns true if it does otherwise false func verifyDirectoryPermission(path string, readonly bool) bool { @@ -346,7 +484,7 @@ func TestSetVolumeOwnershipOwner(t *testing.T) { if currentUid != 0 { t.Skip("running as non-root") } - currentGid := os.Getgid() + currentGID := os.Getgid() tests := []struct { description string @@ -368,7 +506,7 @@ func TestSetVolumeOwnershipOwner(t *testing.T) { }, assertFunc: func(path string) error { filename := filepath.Join(path, "file.txt") - if !verifyFileOwner(filename, currentUid, currentGid) { + if !verifyFileOwner(filename, currentUid, currentGID) { return fmt.Errorf("invalid owner on %s", filename) } return nil @@ -430,7 +568,12 @@ func TestSetVolumeOwnershipOwner(t *testing.T) { t.Fatalf("error creating temp dir: %v", err) } - defer os.RemoveAll(tmpDir) + defer func() { + err := os.RemoveAll(tmpDir) + if err != nil { + t.Fatalf("error removing tmpDir %s: %v", tmpDir, err) + } + }() err = test.setupFunc(tmpDir) if err != nil { @@ -439,7 +582,8 @@ func TestSetVolumeOwnershipOwner(t *testing.T) { mounter := &localFakeMounter{path: tmpDir} always := v1.FSGroupChangeAlways - err = SetVolumeOwnership(mounter, tmpDir, test.fsGroup, &always, nil) + ownershipChanger := NewVolumeOwnership(mounter, tmpDir, test.fsGroup, &always, nil) + err = ownershipChanger.ChangePermissions() 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 4cef01bad8d..cfedeaa3221 100644 --- a/pkg/volume/volume_unsupported.go +++ b/pkg/volume/volume_unsupported.go @@ -21,11 +21,19 @@ package volume import ( v1 "k8s.io/api/core/v1" + "k8s.io/client-go/tools/record" "k8s.io/kubernetes/pkg/volume/util/types" ) -// SetVolumeOwnership sets the ownership of a volume to the specified user and group. -// It typically modifies the user and group ownership of the volume's file system. -func SetVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) error { +// NewVolumeOwnership returns an interface that can be used to recursively change volume permissions and ownership +func NewVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) *VolumeOwnership { + return nil +} + +func (vo *VolumeOwnership) AddProgressNotifier(pod *v1.Pod, recorder record.EventRecorder) *VolumeOwnership { + return vo +} + +func (vo *VolumeOwnership) ChangePermissions() error { return nil }