From 38384d5c13756d72f924c8349e6f822da188a15a Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 9 Feb 2021 12:15:42 +0100 Subject: [PATCH 1/3] PSP: conditional support for generic volume type When introducing the new "generic" volume type for generic ephemeral inline volumes, the storage policy for PodSecurityPolicy objects should have been extended so that this new type is valid only if the generic ephemeral volume feature is enabled or an existing object already has it. Adding the new type to the internal API was also missed. --- pkg/api/podsecuritypolicy/util.go | 22 ++++++ pkg/api/podsecuritypolicy/util_test.go | 79 +++++++++++++++++++++ pkg/apis/policy/types.go | 1 + pkg/security/podsecuritypolicy/util/util.go | 4 ++ 4 files changed, 106 insertions(+) diff --git a/pkg/api/podsecuritypolicy/util.go b/pkg/api/podsecuritypolicy/util.go index a0df4f83ac4a..acd856a73c01 100644 --- a/pkg/api/podsecuritypolicy/util.go +++ b/pkg/api/podsecuritypolicy/util.go @@ -31,6 +31,16 @@ func DropDisabledFields(pspSpec, oldPSPSpec *policy.PodSecurityPolicySpec) { if !utilfeature.DefaultFeatureGate.Enabled(features.CSIInlineVolume) { pspSpec.AllowedCSIDrivers = nil } + var volumes []policy.FSType + for _, volume := range pspSpec.Volumes { + if volume != policy.Ephemeral || + utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) || + volumeInUse(oldPSPSpec, volume) { + // Keep it. + volumes = append(volumes, volume) + } + } + pspSpec.Volumes = volumes } func allowedProcMountTypesInUse(oldPSPSpec *policy.PodSecurityPolicySpec) bool { @@ -45,3 +55,15 @@ func allowedProcMountTypesInUse(oldPSPSpec *policy.PodSecurityPolicySpec) bool { return false } + +func volumeInUse(oldPSPSpec *policy.PodSecurityPolicySpec, volume policy.FSType) bool { + if oldPSPSpec == nil { + return false + } + for _, v := range oldPSPSpec.Volumes { + if v == volume { + return true + } + } + return false +} diff --git a/pkg/api/podsecuritypolicy/util_test.go b/pkg/api/podsecuritypolicy/util_test.go index 1535b04128c4..775b6a29b2ce 100644 --- a/pkg/api/podsecuritypolicy/util_test.go +++ b/pkg/api/podsecuritypolicy/util_test.go @@ -107,3 +107,82 @@ func TestDropAllowedProcMountTypes(t *testing.T) { } } } + +func TestDropEphemeralVolumeType(t *testing.T) { + allowedVolumeTypes := []policy.FSType{policy.Ephemeral} + pspWithoutGenericVolume := func() *policy.PodSecurityPolicySpec { + return &policy.PodSecurityPolicySpec{} + } + pspWithGenericVolume := func() *policy.PodSecurityPolicySpec { + return &policy.PodSecurityPolicySpec{ + Volumes: allowedVolumeTypes, + } + } + + pspInfo := []struct { + description string + hasGenericVolume bool + psp func() *policy.PodSecurityPolicySpec + }{ + { + description: "PodSecurityPolicySpec Without GenericVolume", + hasGenericVolume: false, + psp: pspWithoutGenericVolume, + }, + { + description: "PodSecurityPolicySpec With GenericVolume", + hasGenericVolume: true, + psp: pspWithGenericVolume, + }, + { + description: "is nil", + hasGenericVolume: false, + psp: func() *policy.PodSecurityPolicySpec { return nil }, + }, + } + + for _, enabled := range []bool{true, false} { + for _, oldPSPSpecInfo := range pspInfo { + for _, newPSPSpecInfo := range pspInfo { + oldPSPSpecHasGenericVolume, oldPSPSpec := oldPSPSpecInfo.hasGenericVolume, oldPSPSpecInfo.psp() + newPSPSpecHasGenericVolume, newPSPSpec := newPSPSpecInfo.hasGenericVolume, newPSPSpecInfo.psp() + if newPSPSpec == nil { + continue + } + + t.Run(fmt.Sprintf("feature enabled=%v, old PodSecurityPolicySpec %v, new PodSecurityPolicySpec %v", enabled, oldPSPSpecInfo.description, newPSPSpecInfo.description), func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, enabled)() + + DropDisabledFields(newPSPSpec, oldPSPSpec) + + // old PodSecurityPolicySpec should never be changed + if !reflect.DeepEqual(oldPSPSpec, oldPSPSpecInfo.psp()) { + t.Errorf("old PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(oldPSPSpec, oldPSPSpecInfo.psp())) + } + + switch { + case enabled || oldPSPSpecHasGenericVolume: + // new PodSecurityPolicySpec should not be changed if the feature is enabled, or if the old PodSecurityPolicySpec had GenericVolume + if !reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.psp()) { + t.Errorf("new PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(newPSPSpec, newPSPSpecInfo.psp())) + } + case newPSPSpecHasGenericVolume: + // new PodSecurityPolicySpec should be changed + if reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.psp()) { + t.Errorf("new PodSecurityPolicySpec was not changed") + } + // new PodSecurityPolicySpec should not have GenericVolume + if !reflect.DeepEqual(newPSPSpec, pspWithoutGenericVolume()) { + t.Errorf("new PodSecurityPolicySpec had PodSecurityPolicySpecGenericVolume: %v", diff.ObjectReflectDiff(newPSPSpec, pspWithoutGenericVolume())) + } + default: + // new PodSecurityPolicySpec should not need to be changed + if !reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.psp()) { + t.Errorf("new PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(newPSPSpec, newPSPSpecInfo.psp())) + } + } + }) + } + } + } +} diff --git a/pkg/apis/policy/types.go b/pkg/apis/policy/types.go index e4ef8360a738..b3884d5e4809 100644 --- a/pkg/apis/policy/types.go +++ b/pkg/apis/policy/types.go @@ -309,6 +309,7 @@ const ( PortworxVolume FSType = "portworxVolume" ScaleIO FSType = "scaleIO" CSI FSType = "csi" + Ephemeral FSType = "ephemeral" All FSType = "*" ) diff --git a/pkg/security/podsecuritypolicy/util/util.go b/pkg/security/podsecuritypolicy/util/util.go index c607a23a6401..1405714f8bbc 100644 --- a/pkg/security/podsecuritypolicy/util/util.go +++ b/pkg/security/podsecuritypolicy/util/util.go @@ -29,6 +29,8 @@ const ( ValidatedPSPAnnotation = "kubernetes.io/psp" ) +// GetAllFSTypesExcept returns the result of GetAllFSTypesAsSet minus +// the given exceptions. func GetAllFSTypesExcept(exceptions ...string) sets.String { fstypes := GetAllFSTypesAsSet() for _, e := range exceptions { @@ -37,6 +39,8 @@ func GetAllFSTypesExcept(exceptions ...string) sets.String { return fstypes } +// GetAllFSTypesAsSet returns all actual volume types, regardless +// of feature gates. The special policy.All pseudo type is not included. func GetAllFSTypesAsSet() sets.String { fstypes := sets.NewString() fstypes.Insert( From aa4f8ae793954c01025f59f7594bf4f3af668990 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Fri, 5 Feb 2021 11:46:19 +0100 Subject: [PATCH 2/3] security: another test case for generic ephemeral inline volumes When the PSP contains some other volume types, generic ephemeral inline volumes must be rejected. --- pkg/security/podsecuritypolicy/provider_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/security/podsecuritypolicy/provider_test.go b/pkg/security/podsecuritypolicy/provider_test.go index a28372b3a5c9..76c5a753d75a 100644 --- a/pkg/security/podsecuritypolicy/provider_test.go +++ b/pkg/security/podsecuritypolicy/provider_test.go @@ -502,6 +502,15 @@ func TestValidatePodFailures(t *testing.T) { psp: defaultPSP(), expectedError: "ephemeral volumes are not allowed to be used", }, + "generic ephemeral volumes with other volume type allowed": { + pod: failGenericEphemeralPod, + psp: func() *policy.PodSecurityPolicy { + psp := defaultPSP() + psp.Spec.Volumes = []policy.FSType{policy.NFS} + return psp + }(), + expectedError: "ephemeral volumes are not allowed to be used", + }, } for name, test := range errorCases { t.Run(name, func(t *testing.T) { From fb4b380fe2101211d2ac5ce75337bf04e7267a00 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Sun, 7 Mar 2021 10:53:17 +0100 Subject: [PATCH 3/3] PSP: validation errors for generic volume type It's not enough to silently drop the volume type if the feature is disabled. Instead, the policy should fail validation, just as it would have if the API server didn't know about the feature at all. --- pkg/api/podsecuritypolicy/util.go | 22 ---- pkg/api/podsecuritypolicy/util_test.go | 79 ------------ pkg/apis/policy/validation/validation.go | 25 ++-- pkg/apis/policy/validation/validation_test.go | 96 +++++++++++++- .../policy/podsecuritypolicy/strategy.go | 28 ++++- .../policy/podsecuritypolicy/strategy_test.go | 117 ++++++++++++++++++ 6 files changed, 252 insertions(+), 115 deletions(-) create mode 100644 pkg/registry/policy/podsecuritypolicy/strategy_test.go diff --git a/pkg/api/podsecuritypolicy/util.go b/pkg/api/podsecuritypolicy/util.go index acd856a73c01..a0df4f83ac4a 100644 --- a/pkg/api/podsecuritypolicy/util.go +++ b/pkg/api/podsecuritypolicy/util.go @@ -31,16 +31,6 @@ func DropDisabledFields(pspSpec, oldPSPSpec *policy.PodSecurityPolicySpec) { if !utilfeature.DefaultFeatureGate.Enabled(features.CSIInlineVolume) { pspSpec.AllowedCSIDrivers = nil } - var volumes []policy.FSType - for _, volume := range pspSpec.Volumes { - if volume != policy.Ephemeral || - utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) || - volumeInUse(oldPSPSpec, volume) { - // Keep it. - volumes = append(volumes, volume) - } - } - pspSpec.Volumes = volumes } func allowedProcMountTypesInUse(oldPSPSpec *policy.PodSecurityPolicySpec) bool { @@ -55,15 +45,3 @@ func allowedProcMountTypesInUse(oldPSPSpec *policy.PodSecurityPolicySpec) bool { return false } - -func volumeInUse(oldPSPSpec *policy.PodSecurityPolicySpec, volume policy.FSType) bool { - if oldPSPSpec == nil { - return false - } - for _, v := range oldPSPSpec.Volumes { - if v == volume { - return true - } - } - return false -} diff --git a/pkg/api/podsecuritypolicy/util_test.go b/pkg/api/podsecuritypolicy/util_test.go index 775b6a29b2ce..1535b04128c4 100644 --- a/pkg/api/podsecuritypolicy/util_test.go +++ b/pkg/api/podsecuritypolicy/util_test.go @@ -107,82 +107,3 @@ func TestDropAllowedProcMountTypes(t *testing.T) { } } } - -func TestDropEphemeralVolumeType(t *testing.T) { - allowedVolumeTypes := []policy.FSType{policy.Ephemeral} - pspWithoutGenericVolume := func() *policy.PodSecurityPolicySpec { - return &policy.PodSecurityPolicySpec{} - } - pspWithGenericVolume := func() *policy.PodSecurityPolicySpec { - return &policy.PodSecurityPolicySpec{ - Volumes: allowedVolumeTypes, - } - } - - pspInfo := []struct { - description string - hasGenericVolume bool - psp func() *policy.PodSecurityPolicySpec - }{ - { - description: "PodSecurityPolicySpec Without GenericVolume", - hasGenericVolume: false, - psp: pspWithoutGenericVolume, - }, - { - description: "PodSecurityPolicySpec With GenericVolume", - hasGenericVolume: true, - psp: pspWithGenericVolume, - }, - { - description: "is nil", - hasGenericVolume: false, - psp: func() *policy.PodSecurityPolicySpec { return nil }, - }, - } - - for _, enabled := range []bool{true, false} { - for _, oldPSPSpecInfo := range pspInfo { - for _, newPSPSpecInfo := range pspInfo { - oldPSPSpecHasGenericVolume, oldPSPSpec := oldPSPSpecInfo.hasGenericVolume, oldPSPSpecInfo.psp() - newPSPSpecHasGenericVolume, newPSPSpec := newPSPSpecInfo.hasGenericVolume, newPSPSpecInfo.psp() - if newPSPSpec == nil { - continue - } - - t.Run(fmt.Sprintf("feature enabled=%v, old PodSecurityPolicySpec %v, new PodSecurityPolicySpec %v", enabled, oldPSPSpecInfo.description, newPSPSpecInfo.description), func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, enabled)() - - DropDisabledFields(newPSPSpec, oldPSPSpec) - - // old PodSecurityPolicySpec should never be changed - if !reflect.DeepEqual(oldPSPSpec, oldPSPSpecInfo.psp()) { - t.Errorf("old PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(oldPSPSpec, oldPSPSpecInfo.psp())) - } - - switch { - case enabled || oldPSPSpecHasGenericVolume: - // new PodSecurityPolicySpec should not be changed if the feature is enabled, or if the old PodSecurityPolicySpec had GenericVolume - if !reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.psp()) { - t.Errorf("new PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(newPSPSpec, newPSPSpecInfo.psp())) - } - case newPSPSpecHasGenericVolume: - // new PodSecurityPolicySpec should be changed - if reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.psp()) { - t.Errorf("new PodSecurityPolicySpec was not changed") - } - // new PodSecurityPolicySpec should not have GenericVolume - if !reflect.DeepEqual(newPSPSpec, pspWithoutGenericVolume()) { - t.Errorf("new PodSecurityPolicySpec had PodSecurityPolicySpecGenericVolume: %v", diff.ObjectReflectDiff(newPSPSpec, pspWithoutGenericVolume())) - } - default: - // new PodSecurityPolicySpec should not need to be changed - if !reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.psp()) { - t.Errorf("new PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(newPSPSpec, newPSPSpecInfo.psp())) - } - } - }) - } - } - } -} diff --git a/pkg/apis/policy/validation/validation.go b/pkg/apis/policy/validation/validation.go index 51e4c211ff81..a18d850ce6c7 100644 --- a/pkg/apis/policy/validation/validation.go +++ b/pkg/apis/policy/validation/validation.go @@ -92,19 +92,26 @@ func ValidatePodDisruptionBudgetStatusUpdate(status, oldStatus policy.PodDisrupt // trailing dashes are allowed. var ValidatePodSecurityPolicyName = apimachineryvalidation.NameIsDNSSubdomain +// PodSecurityPolicyValidationOptions contains additional parameters for ValidatePodSecurityPolicy. +type PodSecurityPolicyValidationOptions struct { + // AllowEphemeralVolumeType determines whether Ephemeral is a valid entry + // in PodSecurityPolicySpec.Volumes. + AllowEphemeralVolumeType bool +} + // ValidatePodSecurityPolicy validates a PodSecurityPolicy and returns an ErrorList // with any errors. -func ValidatePodSecurityPolicy(psp *policy.PodSecurityPolicy) field.ErrorList { +func ValidatePodSecurityPolicy(psp *policy.PodSecurityPolicy, opts PodSecurityPolicyValidationOptions) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateObjectMeta(&psp.ObjectMeta, false, ValidatePodSecurityPolicyName, field.NewPath("metadata"))...) allErrs = append(allErrs, ValidatePodSecurityPolicySpecificAnnotations(psp.Annotations, field.NewPath("metadata").Child("annotations"))...) - allErrs = append(allErrs, ValidatePodSecurityPolicySpec(&psp.Spec, field.NewPath("spec"))...) + allErrs = append(allErrs, ValidatePodSecurityPolicySpec(&psp.Spec, opts, field.NewPath("spec"))...) return allErrs } // ValidatePodSecurityPolicySpec validates a PodSecurityPolicySpec and returns an ErrorList // with any errors. -func ValidatePodSecurityPolicySpec(spec *policy.PodSecurityPolicySpec, fldPath *field.Path) field.ErrorList { +func ValidatePodSecurityPolicySpec(spec *policy.PodSecurityPolicySpec, opts PodSecurityPolicyValidationOptions, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, validatePSPRunAsUser(fldPath.Child("runAsUser"), &spec.RunAsUser)...) @@ -112,7 +119,7 @@ func ValidatePodSecurityPolicySpec(spec *policy.PodSecurityPolicySpec, fldPath * allErrs = append(allErrs, validatePSPSELinux(fldPath.Child("seLinux"), &spec.SELinux)...) allErrs = append(allErrs, validatePSPSupplementalGroup(fldPath.Child("supplementalGroups"), &spec.SupplementalGroups)...) allErrs = append(allErrs, validatePSPFSGroup(fldPath.Child("fsGroup"), &spec.FSGroup)...) - allErrs = append(allErrs, validatePodSecurityPolicyVolumes(fldPath, spec.Volumes)...) + allErrs = append(allErrs, validatePodSecurityPolicyVolumes(opts, fldPath, spec.Volumes)...) if len(spec.RequiredDropCapabilities) > 0 && hasCap(policy.AllowAllCapabilities, spec.AllowedCapabilities) { allErrs = append(allErrs, field.Invalid(field.NewPath("requiredDropCapabilities"), spec.RequiredDropCapabilities, "must be empty when all capabilities are allowed by a wildcard")) @@ -320,11 +327,15 @@ func validatePSPSupplementalGroup(fldPath *field.Path, groupOptions *policy.Supp } // validatePodSecurityPolicyVolumes validates the volume fields of PodSecurityPolicy. -func validatePodSecurityPolicyVolumes(fldPath *field.Path, volumes []policy.FSType) field.ErrorList { +func validatePodSecurityPolicyVolumes(opts PodSecurityPolicyValidationOptions, fldPath *field.Path, volumes []policy.FSType) field.ErrorList { allErrs := field.ErrorList{} allowed := psputil.GetAllFSTypesAsSet() // add in the * value since that is a pseudo type that is not included by default allowed.Insert(string(policy.All)) + // Ephemeral may or may not be allowed. + if !opts.AllowEphemeralVolumeType { + allowed.Delete(string(policy.Ephemeral)) + } for _, v := range volumes { if !allowed.Has(string(v)) { allErrs = append(allErrs, field.NotSupported(fldPath.Child("volumes"), v, allowed.List())) @@ -519,11 +530,11 @@ func validateRuntimeClassStrategy(fldPath *field.Path, rc *policy.RuntimeClassSt } // ValidatePodSecurityPolicyUpdate validates a PSP for updates. -func ValidatePodSecurityPolicyUpdate(old *policy.PodSecurityPolicy, new *policy.PodSecurityPolicy) field.ErrorList { +func ValidatePodSecurityPolicyUpdate(old *policy.PodSecurityPolicy, new *policy.PodSecurityPolicy, opts PodSecurityPolicyValidationOptions) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&new.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata"))...) allErrs = append(allErrs, ValidatePodSecurityPolicySpecificAnnotations(new.Annotations, field.NewPath("metadata").Child("annotations"))...) - allErrs = append(allErrs, ValidatePodSecurityPolicySpec(&new.Spec, field.NewPath("spec"))...) + allErrs = append(allErrs, ValidatePodSecurityPolicySpec(&new.Spec, opts, field.NewPath("spec"))...) return allErrs } diff --git a/pkg/apis/policy/validation/validation_test.go b/pkg/apis/policy/validation/validation_test.go index 7b74a1c3b871..165f1948294e 100644 --- a/pkg/apis/policy/validation/validation_test.go +++ b/pkg/apis/policy/validation/validation_test.go @@ -590,7 +590,7 @@ func TestValidatePodSecurityPolicy(t *testing.T) { } for k, v := range errorCases { - errs := ValidatePodSecurityPolicy(v.psp) + errs := ValidatePodSecurityPolicy(v.psp, PodSecurityPolicyValidationOptions{}) if len(errs) == 0 { t.Errorf("%s expected errors but got none", k) continue @@ -613,7 +613,7 @@ func TestValidatePodSecurityPolicy(t *testing.T) { // Should not be able to update to an invalid policy. for k, v := range errorCases { v.psp.ResourceVersion = "444" // Required for updates. - errs := ValidatePodSecurityPolicyUpdate(validPSP(), v.psp) + errs := ValidatePodSecurityPolicyUpdate(validPSP(), v.psp, PodSecurityPolicyValidationOptions{}) if len(errs) == 0 { t.Errorf("[%s] expected update errors but got none", k) continue @@ -743,13 +743,13 @@ func TestValidatePodSecurityPolicy(t *testing.T) { } for k, v := range successCases { - if errs := ValidatePodSecurityPolicy(v.psp); len(errs) != 0 { + if errs := ValidatePodSecurityPolicy(v.psp, PodSecurityPolicyValidationOptions{}); len(errs) != 0 { t.Errorf("Expected success for %s, got %v", k, errs) } // Should be able to update to a valid PSP. v.psp.ResourceVersion = "444" // Required for updates. - if errs := ValidatePodSecurityPolicyUpdate(validPSP(), v.psp); len(errs) != 0 { + if errs := ValidatePodSecurityPolicyUpdate(validPSP(), v.psp, PodSecurityPolicyValidationOptions{}); len(errs) != 0 { t.Errorf("Expected success for %s update, got %v", k, errs) } } @@ -786,7 +786,7 @@ func TestValidatePSPVolumes(t *testing.T) { for _, strVolume := range volumes.List() { psp := validPSP() psp.Spec.Volumes = []policy.FSType{policy.FSType(strVolume)} - errs := ValidatePodSecurityPolicy(psp) + errs := ValidatePodSecurityPolicy(psp, PodSecurityPolicyValidationOptions{AllowEphemeralVolumeType: true}) if len(errs) != 0 { t.Errorf("%s validation expected no errors but received %v", strVolume, errs) } @@ -1063,3 +1063,89 @@ func TestValidateRuntimeClassStrategy(t *testing.T) { }) } } + +func TestAllowEphemeralVolumeType(t *testing.T) { + pspWithoutGenericVolume := func() *policy.PodSecurityPolicy { + return &policy.PodSecurityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "psp", + ResourceVersion: "1", + }, + Spec: policy.PodSecurityPolicySpec{ + RunAsUser: policy.RunAsUserStrategyOptions{ + Rule: policy.RunAsUserStrategyMustRunAs, + }, + SupplementalGroups: policy.SupplementalGroupsStrategyOptions{ + Rule: policy.SupplementalGroupsStrategyMustRunAs, + }, + SELinux: policy.SELinuxStrategyOptions{ + Rule: policy.SELinuxStrategyMustRunAs, + }, + FSGroup: policy.FSGroupStrategyOptions{ + Rule: policy.FSGroupStrategyMustRunAs, + }, + }, + } + } + pspWithGenericVolume := func() *policy.PodSecurityPolicy { + psp := pspWithoutGenericVolume() + psp.Spec.Volumes = append(psp.Spec.Volumes, policy.Ephemeral) + return psp + } + pspNil := func() *policy.PodSecurityPolicy { + return nil + } + + pspInfo := []struct { + description string + hasGenericVolume bool + psp func() *policy.PodSecurityPolicy + }{ + { + description: "PodSecurityPolicySpec Without GenericVolume", + hasGenericVolume: false, + psp: pspWithoutGenericVolume, + }, + { + description: "PodSecurityPolicySpec With GenericVolume", + hasGenericVolume: true, + psp: pspWithGenericVolume, + }, + { + description: "is nil", + hasGenericVolume: false, + psp: pspNil, + }, + } + + for _, allowed := range []bool{true, false} { + for _, oldPSPInfo := range pspInfo { + for _, newPSPInfo := range pspInfo { + oldPSP := oldPSPInfo.psp() + newPSP := newPSPInfo.psp() + if newPSP == nil { + continue + } + + t.Run(fmt.Sprintf("feature enabled=%v, old PodSecurityPolicySpec %v, new PodSecurityPolicySpec %v", allowed, oldPSPInfo.description, newPSPInfo.description), func(t *testing.T) { + opts := PodSecurityPolicyValidationOptions{ + AllowEphemeralVolumeType: allowed, + } + var errs field.ErrorList + expectErrors := newPSPInfo.hasGenericVolume && !allowed + if oldPSP == nil { + errs = ValidatePodSecurityPolicy(newPSP, opts) + } else { + errs = ValidatePodSecurityPolicyUpdate(oldPSP, newPSP, opts) + } + if expectErrors && len(errs) == 0 { + t.Error("expected errors, got none") + } + if !expectErrors && len(errs) > 0 { + t.Errorf("expected no errors, got: %v", errs) + } + }) + } + } + } +} diff --git a/pkg/registry/policy/podsecuritypolicy/strategy.go b/pkg/registry/policy/podsecuritypolicy/strategy.go index e09d76239e3f..9403eb36ac5a 100644 --- a/pkg/registry/policy/podsecuritypolicy/strategy.go +++ b/pkg/registry/policy/podsecuritypolicy/strategy.go @@ -23,10 +23,12 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage/names" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" psputil "k8s.io/kubernetes/pkg/api/podsecuritypolicy" "k8s.io/kubernetes/pkg/apis/policy" "k8s.io/kubernetes/pkg/apis/policy/validation" + "k8s.io/kubernetes/pkg/features" ) // strategy implements behavior for PodSecurityPolicy objects @@ -72,9 +74,31 @@ func (strategy) Canonicalize(obj runtime.Object) { } func (strategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { - return validation.ValidatePodSecurityPolicy(obj.(*policy.PodSecurityPolicy)) + opts := validation.PodSecurityPolicyValidationOptions{ + // Only allowed if the feature is enabled. + AllowEphemeralVolumeType: utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume), + } + return validation.ValidatePodSecurityPolicy(obj.(*policy.PodSecurityPolicy), opts) } func (strategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { - return validation.ValidatePodSecurityPolicyUpdate(old.(*policy.PodSecurityPolicy), obj.(*policy.PodSecurityPolicy)) + opts := validation.PodSecurityPolicyValidationOptions{ + // Allowed if the feature is enabled or the old policy already had it. + // A policy that had the type set when that was valid must remain valid. + AllowEphemeralVolumeType: utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) || + volumeInUse(old.(*policy.PodSecurityPolicy), policy.Ephemeral), + } + return validation.ValidatePodSecurityPolicyUpdate(old.(*policy.PodSecurityPolicy), obj.(*policy.PodSecurityPolicy), opts) +} + +func volumeInUse(oldPSP *policy.PodSecurityPolicy, volume policy.FSType) bool { + if oldPSP == nil { + return false + } + for _, v := range oldPSP.Spec.Volumes { + if v == volume { + return true + } + } + return false } diff --git a/pkg/registry/policy/podsecuritypolicy/strategy_test.go b/pkg/registry/policy/podsecuritypolicy/strategy_test.go new file mode 100644 index 000000000000..912ccc122cbb --- /dev/null +++ b/pkg/registry/policy/podsecuritypolicy/strategy_test.go @@ -0,0 +1,117 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package podsecuritypolicy + +import ( + "context" + "fmt" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/kubernetes/pkg/apis/policy" + "k8s.io/kubernetes/pkg/features" +) + +func TestAllowEphemeralVolumeType(t *testing.T) { + pspWithoutGenericVolume := func() *policy.PodSecurityPolicy { + return &policy.PodSecurityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "psp", + ResourceVersion: "1", + }, + Spec: policy.PodSecurityPolicySpec{ + RunAsUser: policy.RunAsUserStrategyOptions{ + Rule: policy.RunAsUserStrategyMustRunAs, + }, + SupplementalGroups: policy.SupplementalGroupsStrategyOptions{ + Rule: policy.SupplementalGroupsStrategyMustRunAs, + }, + SELinux: policy.SELinuxStrategyOptions{ + Rule: policy.SELinuxStrategyMustRunAs, + }, + FSGroup: policy.FSGroupStrategyOptions{ + Rule: policy.FSGroupStrategyMustRunAs, + }, + }, + } + } + pspWithGenericVolume := func() *policy.PodSecurityPolicy { + psp := pspWithoutGenericVolume() + psp.Spec.Volumes = append(psp.Spec.Volumes, policy.Ephemeral) + return psp + } + pspNil := func() *policy.PodSecurityPolicy { + return nil + } + + pspInfo := []struct { + description string + hasGenericVolume bool + psp func() *policy.PodSecurityPolicy + }{ + { + description: "PodSecurityPolicySpec Without GenericVolume", + hasGenericVolume: false, + psp: pspWithoutGenericVolume, + }, + { + description: "PodSecurityPolicySpec With GenericVolume", + hasGenericVolume: true, + psp: pspWithGenericVolume, + }, + { + description: "is nil", + hasGenericVolume: false, + psp: pspNil, + }, + } + + for _, enabled := range []bool{true, false} { + for _, oldPSPInfo := range pspInfo { + for _, newPSPInfo := range pspInfo { + oldPSP := oldPSPInfo.psp() + newPSP := newPSPInfo.psp() + if newPSP == nil { + continue + } + + t.Run(fmt.Sprintf("feature enabled=%v, old PodSecurityPolicySpec %v, new PodSecurityPolicySpec %v", enabled, oldPSPInfo.description, newPSPInfo.description), func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, enabled)() + + var errs field.ErrorList + var expectErrors bool + if oldPSP == nil { + errs = Strategy.Validate(context.Background(), newPSP) + expectErrors = newPSPInfo.hasGenericVolume && !enabled + } else { + errs = Strategy.ValidateUpdate(context.Background(), newPSP, oldPSP) + expectErrors = !oldPSPInfo.hasGenericVolume && newPSPInfo.hasGenericVolume && !enabled + } + if expectErrors && len(errs) == 0 { + t.Error("expected errors, got none") + } + if !expectErrors && len(errs) > 0 { + t.Errorf("expected no errors, got: %v", errs) + } + }) + } + } + } +}