Merge pull request kubernetes#72389 from mason1kwok/feature_gate_Allo…

…wedProcMountTypes AllowedProcMountTypes - Update DropDisabled[Alpha]Fields behaviour
atsivakumar · Dec 29, 2018 · 173846b · 173846b
2 parents 9cdfdba + 3453da2
commit 173846b
Show file tree

Hide file tree

Showing 3 changed files with 86 additions and 30 deletions.
diff --git 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
@@ -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
@@ -17,48 +17,93 @@ 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"
 	"k8s.io/kubernetes/pkg/apis/policy"
 	"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,
+		}
 	}
 
-	// Enable alpha feature ProcMountType
-	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ProcMountType, true)()
+	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 },
+		},
+	}
 
-	// now test dropping the fields - should not be dropped
-	DropDisabledFields(&psp.Spec, nil)
+	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
+				}
 
-	// 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")
-		}
-	}
+				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)()
 
-	// Disable alpha feature ProcMountType
-	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ProcMountType, false)()
+					DropDisabledFields(newPSPSpec, oldPSPSpec)
 
-	// now test dropping the fields
-	DropDisabledFields(&psp.Spec, nil)
+					// old PodSecurityPolicySpec should never be changed
+					if !reflect.DeepEqual(oldPSPSpec, oldPSPSpecInfo.sc()) {
+						t.Errorf("old PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(oldPSPSpec, oldPSPSpecInfo.sc()))
+					}
 
-	// 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()))
+						}
+					}
+				})
+			}
 		}
 	}
 }