Merge pull request kubernetes#128637 from jpbetz/fix-mutating-admission-defaulting

Bug fix: MutatingAdmissionPolicy should default builtin types after each mutation

Author: k8s-ci-robot

staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher.go

@@ -27,6 +27,7 @@ import (
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apiserver/pkg/admission"
@@ -249,9 +250,20 @@ func (d *dispatcher) dispatchOne(
 		return err
 	}
 
+	switch versionedAttributes.VersionedObject.(type) {
+	case *unstructured.Unstructured:
+		// No conversion needed before defaulting for the patch object if the admitted object is unstructured.
+	default:
+		// Before defaulting, if the admitted object is a typed object, convert unstructured patch result back to a typed object.
+		newVersionedObject, err = o.GetObjectConvertor().ConvertToVersion(newVersionedObject, versionedAttributes.GetKind().GroupVersion())
+		if err != nil {
+			return err
+		}
+	}
+	o.GetObjectDefaulter().Default(newVersionedObject)
+
 	versionedAttributes.Dirty = true
 	versionedAttributes.VersionedObject = newVersionedObject
-	o.GetObjectDefaulter().Default(newVersionedObject)
 	return nil
 }
 

staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher_test.go

@@ -122,6 +122,9 @@ func TestDispatcher(t *testing.T) {
 							Volumes: []corev1.Volume{{Name: "x"}},
 						},
 					},
+					Strategy: appsv1.DeploymentStrategy{
+						Type: appsv1.RollingUpdateDeploymentStrategyType,
+					},
 				}},
 		},
 		{
@@ -144,6 +147,9 @@ func TestDispatcher(t *testing.T) {
 							Volumes: []corev1.Volume{{Name: "x"}},
 						},
 					},
+					Strategy: appsv1.DeploymentStrategy{
+						Type: appsv1.RollingUpdateDeploymentStrategyType,
+					},
 				}},
 			params: []runtime.Object{
 				&corev1.ConfigMap{
@@ -212,6 +218,9 @@ func TestDispatcher(t *testing.T) {
 							Volumes: []corev1.Volume{{Name: "x"}},
 						},
 					},
+					Strategy: appsv1.DeploymentStrategy{
+						Type: appsv1.RollingUpdateDeploymentStrategyType,
+					},
 				}},
 		},
 		{
@@ -233,6 +242,9 @@ func TestDispatcher(t *testing.T) {
 							Volumes: []corev1.Volume{{Name: "x"}},
 						},
 					},
+					Strategy: appsv1.DeploymentStrategy{
+						Type: appsv1.RollingUpdateDeploymentStrategyType,
+					},
 				}},
 			policyHooks: []generic.PolicyHook[*Policy, *PolicyBinding, PolicyEvaluator]{
 				{
@@ -317,6 +329,9 @@ func TestDispatcher(t *testing.T) {
 							Volumes: []corev1.Volume{{Name: "x"}},
 						},
 					},
+					Strategy: appsv1.DeploymentStrategy{
+						Type: appsv1.RollingUpdateDeploymentStrategyType,
+					},
 				}},
 		},
 		{
@@ -338,6 +353,9 @@ func TestDispatcher(t *testing.T) {
 							Volumes: []corev1.Volume{{Name: "x"}},
 						},
 					},
+					Strategy: appsv1.DeploymentStrategy{
+						Type: appsv1.RollingUpdateDeploymentStrategyType,
+					},
 				}},
 			policyHooks: []generic.PolicyHook[*Policy, *PolicyBinding, PolicyEvaluator]{
 				{
@@ -444,6 +462,9 @@ func TestDispatcher(t *testing.T) {
 							Volumes: []corev1.Volume{{Name: "x"}},
 						},
 					},
+					Strategy: appsv1.DeploymentStrategy{
+						Type: appsv1.RollingUpdateDeploymentStrategyType,
+					},
 				}},
 		},
 		{
@@ -466,6 +487,9 @@ func TestDispatcher(t *testing.T) {
 							Volumes: []corev1.Volume{{Name: "x"}},
 						},
 					},
+					Strategy: appsv1.DeploymentStrategy{
+						Type: appsv1.RollingUpdateDeploymentStrategyType,
+					},
 				}},
 			policyHooks: []generic.PolicyHook[*Policy, *PolicyBinding, PolicyEvaluator]{
 				{
@@ -571,6 +595,9 @@ func TestDispatcher(t *testing.T) {
 							Volumes: []corev1.Volume{{Name: "x"}},
 						},
 					},
+					Strategy: appsv1.DeploymentStrategy{
+						Type: appsv1.RollingUpdateDeploymentStrategyType,
+					},
 				}},
 		},
 	}
@@ -597,6 +624,11 @@ func TestDispatcher(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Register a fake defaulter since registering the full defaulter adds noise
+	// and creates dep cycles.
+	scheme.AddTypeDefaultingFunc(&appsv1.Deployment{},
+		func(obj interface{}) { fakeSetDefaultForDeployment(obj.(*appsv1.Deployment)) })
+
 	objectInterfaces := admission.NewObjectInterfacesFromScheme(scheme)
 
 	for _, tc := range testCases {
@@ -673,3 +705,11 @@ func (t testParamScope) Name() meta.RESTScopeName {
 }
 
 var _ meta.RESTScope = testParamScope{}
+
+func fakeSetDefaultForDeployment(obj *appsv1.Deployment) {
+	// Just default strategy type so the tests have a defaulted field to observe
+	strategy := &obj.Spec.Strategy
+	if strategy.Type == "" {
+		strategy.Type = appsv1.RollingUpdateDeploymentStrategyType
+	}
+}