From f7509d277efa63f1fa5c7e324c0ae061cf147265 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Fri, 21 Feb 2020 16:35:52 -0500 Subject: [PATCH] Define new type for storing volume fsgroupchangepolicy Address review comments for api change --- pkg/api/pod/util.go | 23 ++++ pkg/api/pod/util_test.go | 127 ++++++++++++++++++++ pkg/apis/core/types.go | 24 ++++ pkg/apis/core/validation/validation.go | 16 ++- pkg/apis/core/validation/validation_test.go | 58 +++++++++ pkg/features/kube_features.go | 7 ++ staging/src/k8s.io/api/core/v1/types.go | 24 ++++ 7 files changed, 278 insertions(+), 1 deletion(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index ecad9594026..c36fada38f0 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -387,6 +387,8 @@ func dropDisabledFields( dropDisabledRunAsGroupField(podSpec, oldPodSpec) + dropDisabledFSGroupFields(podSpec, oldPodSpec) + if !utilfeature.DefaultFeatureGate.Enabled(features.RuntimeClass) && !runtimeClassInUse(oldPodSpec) { // Set RuntimeClassName to nil only if feature is disabled and it is not used podSpec.RuntimeClassName = nil @@ -447,6 +449,16 @@ func dropDisabledProcMountField(podSpec, oldPodSpec *api.PodSpec) { } } +func dropDisabledFSGroupFields(podSpec, oldPodSpec *api.PodSpec) { + if !utilfeature.DefaultFeatureGate.Enabled(features.ConfigurableFSGroupPolicy) && !fsGroupPolicyInUse(oldPodSpec) { + // if oldPodSpec had no FSGroupChangePolicy set then we should prevent new pod from having this field + // if ConfigurableFSGroupPolicy feature is disabled + if podSpec.SecurityContext != nil { + podSpec.SecurityContext.FSGroupChangePolicy = nil + } + } +} + // dropDisabledCSIVolumeSourceAlphaFields removes disabled alpha fields from []CSIVolumeSource. // This should be called from PrepareForCreate/PrepareForUpdate for all pod specs resources containing a CSIVolumeSource func dropDisabledCSIVolumeSourceAlphaFields(podSpec, oldPodSpec *api.PodSpec) { @@ -464,6 +476,17 @@ func ephemeralContainersInUse(podSpec *api.PodSpec) bool { return len(podSpec.EphemeralContainers) > 0 } +func fsGroupPolicyInUse(podSpec *api.PodSpec) bool { + if podSpec == nil { + return false + } + securityContext := podSpec.SecurityContext + if securityContext != nil && securityContext.FSGroupChangePolicy != nil { + return true + } + return false +} + // subpathInUse returns true if the pod spec is non-nil and has a volume mount that makes use of the subPath feature func subpathInUse(podSpec *api.PodSpec) bool { if podSpec == nil { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 53d099351ed..8860aa6c50a 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -435,6 +435,133 @@ func TestPodConfigmaps(t *testing.T) { } } +func TestDropFSGroupFields(t *testing.T) { + nofsGroupPod := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + Containers: []api.Container{ + { + Name: "container1", + Image: "testimage", + }, + }, + }, + } + } + + var podFSGroup int64 = 100 + changePolicy := api.FSGroupChangeAlways + + fsGroupPod := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + Containers: []api.Container{ + { + Name: "container1", + Image: "testimage", + }, + }, + SecurityContext: &api.PodSecurityContext{ + FSGroup: &podFSGroup, + FSGroupChangePolicy: &changePolicy, + }, + }, + } + } + podInfos := []struct { + description string + featureEnabled bool + newPodHasFSGroupChangePolicy bool + pod func() *api.Pod + expectPolicyInPod bool + }{ + { + description: "oldPod.FSGroupChangePolicy=nil, feature=true, newPod.FSGroupChangePolicy=true", + featureEnabled: true, + pod: nofsGroupPod, + newPodHasFSGroupChangePolicy: true, + expectPolicyInPod: true, + }, + { + description: "oldPod=nil, feature=false, newPod.FSGroupChangePolicy=true", + featureEnabled: false, + pod: func() *api.Pod { return nil }, + newPodHasFSGroupChangePolicy: true, + expectPolicyInPod: false, + }, + { + description: "oldPod=nil, feature=true, newPod.FSGroupChangePolicy=true", + featureEnabled: true, + pod: func() *api.Pod { return nil }, + newPodHasFSGroupChangePolicy: true, + expectPolicyInPod: true, + }, + { + description: "oldPod.FSGroupChangePolicy=nil, feature=false, newPod.FSGroupChangePolicy=true", + featureEnabled: false, + pod: nofsGroupPod, + newPodHasFSGroupChangePolicy: true, + expectPolicyInPod: false, + }, + { + description: "oldPod.FSGroupChangePolicy=true, feature=false, newPod.FSGroupChangePolicy=true", + featureEnabled: false, + pod: fsGroupPod, + newPodHasFSGroupChangePolicy: true, + expectPolicyInPod: true, + }, + { + description: "oldPod.FSGroupChangePolicy=true, feature=false, newPod.FSGroupChangePolicy=false", + featureEnabled: false, + pod: fsGroupPod, + newPodHasFSGroupChangePolicy: false, + expectPolicyInPod: false, + }, + { + description: "oldPod.FSGroupChangePolicy=true, feature=true, newPod.FSGroupChangePolicy=false", + featureEnabled: true, + pod: fsGroupPod, + newPodHasFSGroupChangePolicy: false, + expectPolicyInPod: false, + }, + } + for _, podInfo := range podInfos { + t.Run(podInfo.description, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ConfigurableFSGroupPolicy, podInfo.featureEnabled)() + oldPod := podInfo.pod() + newPod := oldPod.DeepCopy() + if oldPod == nil && podInfo.newPodHasFSGroupChangePolicy { + newPod = fsGroupPod() + } + + if oldPod != nil { + if podInfo.newPodHasFSGroupChangePolicy { + newPod.Spec.SecurityContext = &api.PodSecurityContext{ + FSGroup: &podFSGroup, + FSGroupChangePolicy: &changePolicy, + } + } else { + newPod.Spec.SecurityContext = &api.PodSecurityContext{} + } + } + DropDisabledPodFields(newPod, oldPod) + + if podInfo.expectPolicyInPod { + secContext := newPod.Spec.SecurityContext + if secContext == nil || secContext.FSGroupChangePolicy == nil { + t.Errorf("for %s, expected fsGroupChangepolicy found none", podInfo.description) + } + } else { + secConext := newPod.Spec.SecurityContext + if secConext != nil && secConext.FSGroupChangePolicy != nil { + t.Errorf("for %s, unexpected fsGroupChangepolicy set", podInfo.description) + } + } + }) + } + +} + func TestDropSubPath(t *testing.T) { podWithSubpaths := func() *api.Pod { return &api.Pod{ diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 4ada5951cc6..160a4da0b37 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -2784,6 +2784,22 @@ type Sysctl struct { Value string } +// PodFSGroupChangePolicy holds policies that will be used for applying fsGroup to a volume +// when volume is mounted. +type PodFSGroupChangePolicy string + +const ( + // FSGroupChangeOnRootMismatch indicates that volume's ownership and permissions will be changed + // only when permission and ownership of root directory does not match with expected + // permissions on the volume. This can help shorten the time it takes to change + // ownership and permissions of a volume. + FSGroupChangeOnRootMismatch PodFSGroupChangePolicy = "OnRootMismatch" + // FSGroupChangeAlways indicates that volume's ownership and permissions + // should always be changed whenever volume is mounted inside a Pod. This the default + // behavior. + FSGroupChangeAlways PodFSGroupChangePolicy = "Always" +) + // PodSecurityContext holds pod-level security attributes and common container settings. // Some fields are also present in container.securityContext. Field values of // container.securityContext take precedence over field values of PodSecurityContext. @@ -2863,6 +2879,14 @@ type PodSecurityContext struct { // If unset, the Kubelet will not modify the ownership and permissions of any volume. // +optional FSGroup *int64 + // fsGroupChangePolicy defines behavior of changing ownership and permission of the volume + // before being exposed inside Pod. This field will only apply to + // volume types which support fsGroup based ownership(and permissions). + // It will have no effect on ephemeral volume types such as: secret, configmaps + // and emptydir. + // Valid values are "OnRootMismatch" and "Always". If not specified defaults to "Always". + // +optional + FSGroupChangePolicy *PodFSGroupChangePolicy // Sysctls hold a list of namespaced sysctls used for the pod. Pods with unsupported // sysctls (by the container runtime) might fail to launch. // +optional diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 5dfea2c69d5..6f782fa588b 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -31,7 +31,7 @@ import ( "k8s.io/klog" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/resource" apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation" @@ -2824,6 +2824,16 @@ func validateDNSPolicy(dnsPolicy *core.DNSPolicy, fldPath *field.Path) field.Err return allErrors } +var validFSGroupChangePolicies = sets.NewString(string(core.FSGroupChangeOnRootMismatch), string(core.FSGroupChangeAlways)) + +func validateFSGroupChangePolicy(fsGroupPolicy *core.PodFSGroupChangePolicy, fldPath *field.Path) field.ErrorList { + allErrors := field.ErrorList{} + if !validFSGroupChangePolicies.Has(string(*fsGroupPolicy)) { + allErrors = append(allErrors, field.NotSupported(fldPath, fsGroupPolicy, validFSGroupChangePolicies.List())) + } + return allErrors +} + const ( // Limits on various DNS parameters. These are derived from // restrictions in Linux libc name resolution handling. @@ -3667,6 +3677,10 @@ func ValidatePodSecurityContext(securityContext *core.PodSecurityContext, spec * allErrs = append(allErrs, validateSysctls(securityContext.Sysctls, fldPath.Child("sysctls"))...) } + if securityContext.FSGroupChangePolicy != nil { + allErrs = append(allErrs, validateFSGroupChangePolicy(securityContext.FSGroupChangePolicy, fldPath.Child("fsGroupChangePolicy"))...) + } + allErrs = append(allErrs, validateWindowsSecurityContextOptions(securityContext.WindowsOptions, fldPath.Child("windowsOptions"))...) } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index e2a87a5d407..b6f45b6ebbf 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -6559,6 +6559,9 @@ func TestValidatePodSpec(t *testing.T) { maxUserID := int64(2147483647) minGroupID := int64(0) maxGroupID := int64(2147483647) + goodfsGroupChangePolicy := core.FSGroupChangeAlways + badfsGroupChangePolicy1 := core.PodFSGroupChangePolicy("invalid") + badfsGroupChangePolicy2 := core.PodFSGroupChangePolicy("") successCases := []core.PodSpec{ { // Populate basic fields, leave defaults for most. @@ -6706,6 +6709,14 @@ func TestValidatePodSpec(t *testing.T) { RuntimeClassName: utilpointer.StringPtr("valid-sandbox"), Overhead: core.ResourceList{}, }, + { + Containers: []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, + SecurityContext: &core.PodSecurityContext{ + FSGroupChangePolicy: &goodfsGroupChangePolicy, + }, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, + }, } for i := range successCases { if errs := ValidatePodSpec(&successCases[i], field.NewPath("field")); len(errs) != 0 { @@ -6893,6 +6904,22 @@ func TestValidatePodSpec(t *testing.T) { DNSPolicy: core.DNSClusterFirst, RuntimeClassName: utilpointer.StringPtr("invalid/sandbox"), }, + "bad empty fsGroupchangepolicy": { + Containers: []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, + SecurityContext: &core.PodSecurityContext{ + FSGroupChangePolicy: &badfsGroupChangePolicy2, + }, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, + }, + "bad invalid fsgroupchangepolicy": { + Containers: []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}, + SecurityContext: &core.PodSecurityContext{ + FSGroupChangePolicy: &badfsGroupChangePolicy1, + }, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, + }, } for k, v := range failureCases { if errs := ValidatePodSpec(&v, field.NewPath("field")); len(errs) == 0 { @@ -8270,6 +8297,7 @@ func TestValidatePodUpdate(t *testing.T) { activeDeadlineSecondsNegative = int64(-30) activeDeadlineSecondsPositive = int64(30) activeDeadlineSecondsLarger = int64(31) + validfsGroupChangePolicy = core.FSGroupChangeOnRootMismatch now = metav1.Now() grace = int64(30) @@ -8720,6 +8748,36 @@ func TestValidatePodUpdate(t *testing.T) { "spec: Forbidden: pod updates may not change fields", "cpu change", }, + { + core.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: core.PodSpec{ + Containers: []core.Container{ + { + Image: "foo:V1", + }, + }, + SecurityContext: &core.PodSecurityContext{ + FSGroupChangePolicy: &validfsGroupChangePolicy, + }, + }, + }, + core.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: core.PodSpec{ + Containers: []core.Container{ + { + Image: "foo:V2", + }, + }, + SecurityContext: &core.PodSecurityContext{ + FSGroupChangePolicy: nil, + }, + }, + }, + "spec: Forbidden: pod updates may not change fields", + "fsGroupChangePolicy change", + }, { core.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index c130a683b97..97777bd0f85 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -418,6 +418,12 @@ const ( // Expects Azure File CSI Driver to be installed and configured on all nodes. CSIMigrationAzureFileComplete featuregate.Feature = "CSIMigrationAzureFileComplete" + // owner: @gnufied + // alpha: v1.18 + // Allows user to configure volume permission change policy for fsGroups when mounting + // a volume in a Pod. + ConfigurableFSGroupPolicy featuregate.Feature = "ConfigurableFSGroupPolicy" + // owner: @RobertKrawitz // beta: v1.15 // @@ -621,6 +627,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS CSIMigrationOpenStack: {Default: false, PreRelease: featuregate.Beta}, // Off by default (requires OpenStack Cinder CSI driver) CSIMigrationOpenStackComplete: {Default: false, PreRelease: featuregate.Alpha}, VolumeSubpath: {Default: true, PreRelease: featuregate.GA}, + ConfigurableFSGroupPolicy: {Default: false, PreRelease: featuregate.Alpha}, BalanceAttachedNodeVolumes: {Default: false, PreRelease: featuregate.Alpha}, VolumeSubpathEnvExpansion: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.19, CSIBlockVolume: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.20 diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index f436f82d2d5..7e629bc9246 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -3122,6 +3122,22 @@ type HostAlias struct { Hostnames []string `json:"hostnames,omitempty" protobuf:"bytes,2,rep,name=hostnames"` } +// PodFSGroupChangePolicy holds policies that will be used for applying fsGroup to a volume +// when volume is mounted. +type PodFSGroupChangePolicy string + +const ( + // FSGroupChangeOnRootMismatch indicates that volume's ownership and permissions will be changed + // only when permission and ownership of root directory does not match with expected + // permissions on the volume. This can help shorten the time it takes to change + // ownership and permissions of a volume. + FSGroupChangeOnRootMismatch PodFSGroupChangePolicy = "OnRootMismatch" + // FSGroupChangeAlways indicates that volume's ownership and permissions + // should always be changed whenever volume is mounted inside a Pod. This the default + // behavior. + FSGroupChangeAlways PodFSGroupChangePolicy = "Always" +) + // PodSecurityContext holds pod-level security attributes and common container settings. // Some fields are also present in container.securityContext. Field values of // container.securityContext take precedence over field values of PodSecurityContext. @@ -3180,6 +3196,14 @@ type PodSecurityContext struct { // sysctls (by the container runtime) might fail to launch. // +optional Sysctls []Sysctl `json:"sysctls,omitempty" protobuf:"bytes,7,rep,name=sysctls"` + // fsGroupChangePolicy defines behavior of changing ownership and permission of the volume + // before being exposed inside Pod. This field will only apply to + // volume types which support fsGroup based ownership(and permissions). + // It will have no effect on ephemeral volume types such as: secret, configmaps + // and emptydir. + // Valid values are "OnRootMismatch" and "Always". If not specified defaults to "Always". + // +optional + FSGroupChangePolicy *PodFSGroupChangePolicy `json:"fsGroupChangePolicy,omitempty" protobuf:"bytes,9,opt,name=fsGroupChangePolicy"` } // PodQOSClass defines the supported qos classes of Pods.