diff --git a/pkg/api/podsecuritypolicy/BUILD b/pkg/api/podsecuritypolicy/BUILD index 92c5c73c67b..0219cfc6b9f 100644 --- a/pkg/api/podsecuritypolicy/BUILD +++ b/pkg/api/podsecuritypolicy/BUILD @@ -38,6 +38,7 @@ go_test( "//pkg/apis/core:go_default_library", "//pkg/apis/policy:go_default_library", "//pkg/features:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature/testing:go_default_library", ], diff --git a/pkg/api/podsecuritypolicy/util.go b/pkg/api/podsecuritypolicy/util.go index 35572c83534..234a152455a 100644 --- a/pkg/api/podsecuritypolicy/util.go +++ b/pkg/api/podsecuritypolicy/util.go @@ -25,11 +25,8 @@ import ( // DropDisabledFields removes disabled fields from the pod security policy spec. // This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a od security policy spec. func DropDisabledFields(pspSpec, oldPSPSpec *policy.PodSecurityPolicySpec) { - if !utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) { + if !utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) && !allowedProcMountTypesInUse(oldPSPSpec) { pspSpec.AllowedProcMountTypes = nil - if oldPSPSpec != nil { - oldPSPSpec.AllowedProcMountTypes = nil - } } if !utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) { pspSpec.RunAsGroup = nil @@ -38,3 +35,16 @@ func DropDisabledFields(pspSpec, oldPSPSpec *policy.PodSecurityPolicySpec) { } } } + +func allowedProcMountTypesInUse(oldPSPSpec *policy.PodSecurityPolicySpec) bool { + if oldPSPSpec == nil { + return false + } + + if oldPSPSpec.AllowedProcMountTypes != nil { + return true + } + + return false + +} diff --git a/pkg/api/podsecuritypolicy/util_test.go b/pkg/api/podsecuritypolicy/util_test.go index 3d7715fcf4f..433d4241968 100644 --- a/pkg/api/podsecuritypolicy/util_test.go +++ b/pkg/api/podsecuritypolicy/util_test.go @@ -17,8 +17,11 @@ limitations under the License. package podsecuritypolicy import ( + "fmt" + "reflect" "testing" + "k8s.io/apimachinery/pkg/util/diff" utilfeature "k8s.io/apiserver/pkg/util/feature" utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing" api "k8s.io/kubernetes/pkg/apis/core" @@ -26,39 +29,81 @@ import ( "k8s.io/kubernetes/pkg/features" ) -func TestDropAlphaProcMountType(t *testing.T) { - // PodSecurityPolicy with AllowedProcMountTypes set - psp := policy.PodSecurityPolicy{ - Spec: policy.PodSecurityPolicySpec{ - AllowedProcMountTypes: []api.ProcMountType{api.UnmaskedProcMount}, +func TestDropAllowedProcMountTypes(t *testing.T) { + allowedProcMountTypes := []api.ProcMountType{api.UnmaskedProcMount} + scWithoutAllowedProcMountTypes := func() *policy.PodSecurityPolicySpec { + return &policy.PodSecurityPolicySpec{} + } + scWithAllowedProcMountTypes := func() *policy.PodSecurityPolicySpec { + return &policy.PodSecurityPolicySpec{ + AllowedProcMountTypes: allowedProcMountTypes, + } + } + + scInfo := []struct { + description string + hasAllowedProcMountTypes bool + sc func() *policy.PodSecurityPolicySpec + }{ + { + description: "PodSecurityPolicySpec Without AllowedProcMountTypes", + hasAllowedProcMountTypes: false, + sc: scWithoutAllowedProcMountTypes, + }, + { + description: "PodSecurityPolicySpec With AllowedProcMountTypes", + hasAllowedProcMountTypes: true, + sc: scWithAllowedProcMountTypes, + }, + { + description: "is nil", + hasAllowedProcMountTypes: false, + sc: func() *policy.PodSecurityPolicySpec { return nil }, }, } - // Enable alpha feature ProcMountType - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ProcMountType, true)() + for _, enabled := range []bool{true, false} { + for _, oldPSPSpecInfo := range scInfo { + for _, newPSPSpecInfo := range scInfo { + oldPSPSpecHasAllowedProcMountTypes, oldPSPSpec := oldPSPSpecInfo.hasAllowedProcMountTypes, oldPSPSpecInfo.sc() + newPSPSpecHasAllowedProcMountTypes, newPSPSpec := newPSPSpecInfo.hasAllowedProcMountTypes, newPSPSpecInfo.sc() + if newPSPSpec == nil { + continue + } - // now test dropping the fields - should not be dropped - DropDisabledFields(&psp.Spec, nil) + t.Run(fmt.Sprintf("feature enabled=%v, old PodSecurityPolicySpec %v, new PodSecurityPolicySpec %v", enabled, oldPSPSpecInfo.description, newPSPSpecInfo.description), func(t *testing.T) { + defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ProcMountType, enabled)() - // check to make sure AllowedProcMountTypes is still present - // if featureset is set to true - if utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) { - if psp.Spec.AllowedProcMountTypes == nil { - t.Error("AllowedProcMountTypes in pvc.Spec should not have been dropped based on feature-gate") - } - } + DropDisabledFields(newPSPSpec, oldPSPSpec) - // Disable alpha feature ProcMountType - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ProcMountType, false)() + // old PodSecurityPolicySpec should never be changed + if !reflect.DeepEqual(oldPSPSpec, oldPSPSpecInfo.sc()) { + t.Errorf("old PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(oldPSPSpec, oldPSPSpecInfo.sc())) + } - // now test dropping the fields - DropDisabledFields(&psp.Spec, nil) - - // check to make sure AllowedProcMountTypes is nil - // if featureset is set to false - if utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) { - if psp.Spec.AllowedProcMountTypes != nil { - t.Error("DropDisabledFields AllowedProcMountTypes for psp.Spec failed") + switch { + case enabled || oldPSPSpecHasAllowedProcMountTypes: + // new PodSecurityPolicySpec should not be changed if the feature is enabled, or if the old PodSecurityPolicySpec had AllowedProcMountTypes + if !reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.sc()) { + t.Errorf("new PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(newPSPSpec, newPSPSpecInfo.sc())) + } + case newPSPSpecHasAllowedProcMountTypes: + // new PodSecurityPolicySpec should be changed + if reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.sc()) { + t.Errorf("new PodSecurityPolicySpec was not changed") + } + // new PodSecurityPolicySpec should not have AllowedProcMountTypes + if !reflect.DeepEqual(newPSPSpec, scWithoutAllowedProcMountTypes()) { + t.Errorf("new PodSecurityPolicySpec had PodSecurityPolicySpecAllowedProcMountTypes: %v", diff.ObjectReflectDiff(newPSPSpec, scWithoutAllowedProcMountTypes())) + } + default: + // new PodSecurityPolicySpec should not need to be changed + if !reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.sc()) { + t.Errorf("new PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(newPSPSpec, newPSPSpecInfo.sc())) + } + } + }) + } } } }