diff --git a/pkg/volume/configmap/configmap.go b/pkg/volume/configmap/configmap.go index 4b909a100bc..0e74dd0a1d8 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, nil) + err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/) if err != nil { klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup) return err diff --git a/pkg/volume/downwardapi/downwardapi.go b/pkg/volume/downwardapi/downwardapi.go index 753625b5e34..a1779c0dac9 100644 --- a/pkg/volume/downwardapi/downwardapi.go +++ b/pkg/volume/downwardapi/downwardapi.go @@ -227,7 +227,7 @@ func (b *downwardAPIVolumeMounter) SetUpAt(dir string, mounterArgs volume.Mounte return err } - err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil) + err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/) 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 ba05c1f7b29..0a25d2b684c 100644 --- a/pkg/volume/emptydir/empty_dir.go +++ b/pkg/volume/emptydir/empty_dir.go @@ -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, nil) + volume.SetVolumeOwnership(ed, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/) // 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/git_repo/git_repo.go b/pkg/volume/git_repo/git_repo.go index 4f4a877e4f2..d01e28a7258 100644 --- a/pkg/volume/git_repo/git_repo.go +++ b/pkg/volume/git_repo/git_repo.go @@ -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, nil) + volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/) volumeutil.SetReady(b.getMetaDir()) return nil diff --git a/pkg/volume/projected/projected.go b/pkg/volume/projected/projected.go index eb9e5101179..0f65a97610c 100644 --- a/pkg/volume/projected/projected.go +++ b/pkg/volume/projected/projected.go @@ -239,7 +239,7 @@ func (s *projectedVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA return err } - err = volume.SetVolumeOwnership(s, mounterArgs.FsGroup, nil) + err = volume.SetVolumeOwnership(s, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/) if err != nil { klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup) return err diff --git a/pkg/volume/secret/secret.go b/pkg/volume/secret/secret.go index a2974b4099d..a195c59ddd8 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, nil) + err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/) 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 a0072bf5c38..fdbd9016806 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -49,9 +49,9 @@ func SetVolumeOwnership(mounter Mounter, fsGroup *int64, fsGroupChangePolicy *v1 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. + // TODO: remove this when ConfigurableFSGroupPolicy turns GA. if !fsGroupPolicyEnabled { - return legacyPermissionChange(mounter, fsGroup) + return legacyOwnershipChange(mounter, fsGroup) } if skipPermissionChange(mounter, fsGroup, fsGroupChangePolicy) { @@ -68,7 +68,7 @@ func SetVolumeOwnership(mounter Mounter, fsGroup *int64, fsGroupChangePolicy *v1 } -func legacyPermissionChange(mounter Mounter, fsGroup *int64) error { +func legacyOwnershipChange(mounter Mounter, fsGroup *int64) error { return filepath.Walk(mounter.GetPath(), func(path string, info os.FileInfo, err error) error { if err != nil { return err @@ -125,7 +125,7 @@ func changeFilePermission(filename string, fsGroup *int64, readonly bool, info o func skipPermissionChange(mounter Mounter, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) bool { dir := mounter.GetPath() - if fsGroupChangePolicy == nil || *fsGroupChangePolicy != v1.OnRootMismatch { + if fsGroupChangePolicy == nil || *fsGroupChangePolicy != v1.FSGroupChangeOnRootMismatch { klog.V(4).Infof("perform recursive ownership change for %s", dir) return false } @@ -161,8 +161,13 @@ func requiresPermissionChange(rootDir string, fsGroup *int64, readonly bool) boo 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. + // We need to check if actual permissions of root directory is a superset of permissions required by unixPerms. + // This is done by checking if permission bits expected in unixPerms is set in actual permissions of the directory. + // We use bitwise AND operation to check set bits. For example: + // unixPerms: 770, filePerms: 775 : 770&775 = 770 (perms on directory is a superset) + // unixPerms: 770, filePerms: 770 : 770&770 = 770 (perms on directory is a superset) + // unixPerms: 770, filePerms: 750 : 770&750 = 750 (perms on directory is NOT a superset) + // We also need to check if 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 diff --git a/pkg/volume/volume_linux_test.go b/pkg/volume/volume_linux_test.go index 3d0d43d35a4..76834de103a 100644 --- a/pkg/volume/volume_linux_test.go +++ b/pkg/volume/volume_linux_test.go @@ -19,12 +19,17 @@ limitations under the License. package volume import ( + "fmt" "os" + "path/filepath" "syscall" "testing" v1 "k8s.io/api/core/v1" + utilfeature "k8s.io/apiserver/pkg/util/feature" utiltesting "k8s.io/client-go/util/testing" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/kubernetes/pkg/features" ) type localFakeMounter struct { @@ -57,8 +62,8 @@ func (l *localFakeMounter) GetMetrics() (*Metrics, error) { } func TestSkipPermissionChange(t *testing.T) { - always := v1.AlwaysChangeVolumePermission - onrootMismatch := v1.OnRootMismatch + always := v1.FSGroupChangeAlways + onrootMismatch := v1.FSGroupChangeOnRootMismatch tests := []struct { description string fsGroupChangePolicy *v1.PodFSGroupChangePolicy @@ -76,6 +81,18 @@ func TestSkipPermissionChange(t *testing.T) { fsGroupChangePolicy: &always, skipPermssion: false, }, + { + description: "skippermission=false, policy=always, gidmatch=true", + fsGroupChangePolicy: &always, + skipPermssion: false, + gidOwnerMatch: true, + }, + { + description: "skippermission=false, policy=nil, gidmatch=true", + fsGroupChangePolicy: nil, + skipPermssion: false, + gidOwnerMatch: true, + }, { description: "skippermission=false, policy=onrootmismatch, gidmatch=false", fsGroupChangePolicy: &onrootMismatch, @@ -158,5 +175,173 @@ func TestSkipPermissionChange(t *testing.T) { }) } - +} + +func TestSetVolumeOwnership(t *testing.T) { + always := v1.FSGroupChangeAlways + onrootMismatch := v1.FSGroupChangeOnRootMismatch + expectedMask := rwMask | os.ModeSetgid | execMask + + tests := []struct { + description string + fsGroupChangePolicy *v1.PodFSGroupChangePolicy + setupFunc func(path string) error + assertFunc func(path string) error + featureGate bool + }{ + { + description: "featuregate=on, fsgroupchangepolicy=always", + fsGroupChangePolicy: &always, + featureGate: true, + setupFunc: func(path string) error { + info, err := os.Lstat(path) + if err != nil { + return err + } + // change mode of root folder to be right + err = os.Chmod(path, info.Mode()|expectedMask) + if err != nil { + return err + } + + // create a subdirectory with invalid permissions + rogueDir := filepath.Join(path, "roguedir") + err = os.Mkdir(rogueDir, info.Mode()) + if err != nil { + return err + } + return nil + }, + assertFunc: func(path string) error { + rogueDir := filepath.Join(path, "roguedir") + hasCorrectPermissions := verifyDirectoryPermission(rogueDir, false /*readOnly*/) + if !hasCorrectPermissions { + return fmt.Errorf("invalid permissions on %s", rogueDir) + } + return nil + }, + }, + { + description: "featuregate=on, fsgroupchangepolicy=onrootmismatch,rootdir=validperm", + fsGroupChangePolicy: &onrootMismatch, + featureGate: true, + setupFunc: func(path string) error { + info, err := os.Lstat(path) + if err != nil { + return err + } + // change mode of root folder to be right + err = os.Chmod(path, info.Mode()|expectedMask) + if err != nil { + return err + } + + // create a subdirectory with invalid permissions + rogueDir := filepath.Join(path, "roguedir") + err = os.Mkdir(rogueDir, rwMask) + if err != nil { + return err + } + return nil + }, + assertFunc: func(path string) error { + rogueDir := filepath.Join(path, "roguedir") + hasCorrectPermissions := verifyDirectoryPermission(rogueDir, false /*readOnly*/) + if hasCorrectPermissions { + return fmt.Errorf("invalid permissions on %s", rogueDir) + } + return nil + }, + }, + { + description: "featuregate=on, fsgroupchangepolicy=onrootmismatch,rootdir=invalidperm", + fsGroupChangePolicy: &onrootMismatch, + featureGate: true, + setupFunc: func(path string) error { + // change mode of root folder to be right + err := os.Chmod(path, 0770) + if err != nil { + return err + } + + // create a subdirectory with invalid permissions + rogueDir := filepath.Join(path, "roguedir") + err = os.Mkdir(rogueDir, rwMask) + if err != nil { + return err + } + return nil + }, + assertFunc: func(path string) error { + rogueDir := filepath.Join(path, "roguedir") + hasCorrectPermissions := verifyDirectoryPermission(rogueDir, false /*readOnly*/) + if !hasCorrectPermissions { + return fmt.Errorf("invalid permissions on %s", rogueDir) + } + return nil + }, + }, + } + + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ConfigurableFSGroupPolicy, test.featureGate)() + tmpDir, err := utiltesting.MkTmpdir("volume_linux_ownership") + 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) + } + + var expectedGid int64 = 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: tmpDir} + err = SetVolumeOwnership(mounter, &expectedGid, test.fsGroupChangePolicy) + if err != nil { + t.Errorf("for %s error changing ownership with: %v", test.description, err) + } + err = test.assertFunc(tmpDir) + if err != nil { + t.Errorf("for %s error verifying permissions with: %v", test.description, err) + } + }) + } +} + +// 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 { + info, err := os.Lstat(path) + if err != nil { + return false + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok || stat == nil { + return false + } + unixPerms := rwMask + + if readonly { + unixPerms = roMask + } + + unixPerms |= execMask + filePerm := info.Mode().Perm() + if (unixPerms&filePerm == unixPerms) && (info.Mode()&os.ModeSetgid != 0) { + return true + } + return false }