diff --git a/pkg/api/podsecuritypolicy/util.go b/pkg/api/podsecuritypolicy/util.go index acd856a73c0..a0df4f83ac4 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 775b6a29b2c..1535b04128c 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 51e4c211ff8..a18d850ce6c 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 7b74a1c3b87..165f1948294 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 e09d76239e3..9403eb36ac5 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 00000000000..912ccc122cb --- /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) + } + }) + } + } + } +}