Start adding tests for verifying correct modes

Add an example for permission bits checking
This commit is contained in:
Hemant Kumar 2020-02-28 12:23:31 -05:00
parent c52d4bf32f
commit b132959687
8 changed files with 205 additions and 15 deletions

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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
}