Ensure defaulting is applied in the correct API version during mutation

Signed-off-by: James Munnelly <jmunnelly@apple.com>

pkg/webhook/admission/request_handler.go

@@ -47,6 +47,8 @@ import (
 // This means that all resources passed to mutating admission plugins will have default
 // values applied before converting them into the internal version.
 type RequestHandler struct {
+	scheme *runtime.Scheme
+
 	// codecFactory used to create encoders and decoders
 	codecFactory serializer.CodecFactory
 
@@ -68,6 +70,7 @@ type RequestHandler struct {
 func NewRequestHandler(scheme *runtime.Scheme, validator ValidationInterface, mutator MutationInterface) *RequestHandler {
 	cf := serializer.NewCodecFactory(scheme)
 	return &RequestHandler{
+		scheme:       scheme,
 		codecFactory: cf,
 		serializer:   apijson.NewSerializerWithOptions(apijson.DefaultMetaFactory, scheme, scheme, apijson.SerializerOptions{}),
 		decoder:      cf.UniversalDecoder(),
@@ -95,12 +98,7 @@ func (rh *RequestHandler) Validate(ctx context.Context, admissionSpec *admission
 	// decode new version of object
 	obj, _, err := rh.decoder.Decode(admissionSpec.Object.Raw, nil, nil)
 	if err != nil {
-		status.Allowed = false
-		status.Result = &metav1.Status{
-			Status: metav1.StatusFailure, Code: http.StatusBadRequest, Reason: metav1.StatusReasonBadRequest,
-			Message: err.Error(),
-		}
-		return status
+		return badRequestError(status, err)
 	}
 
 	// attempt to decode old object
@@ -108,12 +106,7 @@ func (rh *RequestHandler) Validate(ctx context.Context, admissionSpec *admission
 	if len(admissionSpec.OldObject.Raw) > 0 {
 		oldObj, _, err = rh.decoder.Decode(admissionSpec.OldObject.Raw, nil, nil)
 		if err != nil {
-			status.Allowed = false
-			status.Result = &metav1.Status{
-				Status: metav1.StatusFailure, Code: http.StatusBadRequest, Reason: metav1.StatusReasonBadRequest,
-				Message: err.Error(),
-			}
-			return status
+			return badRequestError(status, err)
 		}
 	}
 
@@ -145,35 +138,32 @@ func (rh *RequestHandler) Mutate(ctx context.Context, admissionSpec *admissionv1
 		return status
 	}
 
-	obj, _, err := rh.decoder.Decode(admissionSpec.Object.Raw, nil, nil)
-	if err != nil {
-		status.Allowed = false
-		status.Result = &metav1.Status{
-			Status: metav1.StatusFailure, Code: http.StatusBadRequest, Reason: metav1.StatusReasonBadRequest,
-			Message: err.Error(),
-		}
-		return status
+	// If the resource submitted to the webhook is in a different version to the request version,
+	// we must take special steps to ensure the correct defaults are applied to the resource (as
+	// defaults are applied by the decoder when the resource is decoded in the version of the
+	// encoded resource).
+	obj, errResponse := rh.decodeRequestObject(status, admissionSpec.Kind, *admissionSpec.RequestKind, admissionSpec.Object.Raw)
+	if errResponse != nil {
+		return errResponse
 	}
 
 	if rh.mutator.Handles(admissionSpec.Operation) {
 		if err := rh.mutator.Mutate(ctx, *admissionSpec, obj); err != nil {
-			status.Allowed = false
-			status.Result = &metav1.Status{
-				Status: metav1.StatusFailure, Code: http.StatusInternalServerError, Reason: metav1.StatusReasonInternalError,
-				Message: err.Error(),
-			}
-			return status
+			return internalServerError(status, err)
 		}
 	}
 
-	patch, err := rh.createMutatePatch(admissionSpec, obj)
+	// Convert the object into the original version that was submitted to the webhook
+	// before generating the patch.
+	outputGroupVersioner := runtime.NewMultiGroupVersioner(schema.GroupVersion{Group: admissionSpec.Kind.Group, Version: admissionSpec.Kind.Version})
+	finalObject, err := rh.scheme.ConvertToVersion(obj, outputGroupVersioner)
 	if err != nil {
-		status.Allowed = false
-		status.Result = &metav1.Status{
-			Status: metav1.StatusFailure, Code: http.StatusInternalServerError, Reason: metav1.StatusReasonInternalError,
-			Message: err.Error(),
-		}
-		return status
+		return internalServerError(status, err)
+	}
+
+	patch, err := rh.createMutatePatch(admissionSpec, finalObject)
+	if err != nil {
+		return internalServerError(status, err)
 	}
 
 	patchType := admissionv1.PatchTypeJSONPatch
@@ -183,12 +173,74 @@ func (rh *RequestHandler) Mutate(ctx context.Context, admissionSpec *admissionv1
 	return status
 }
 
+// decodeRequestObject will decode the given 'bytes' into the internal API version.
+// It will apply defaults using the 'defaultsInGVK', regardless of what API version
+// the encoded bytes are in.
+func (rh *RequestHandler) decodeRequestObject(status *admissionv1.AdmissionResponse, objectGVK, defaultInGVK metav1.GroupVersionKind, bytes []byte) (runtime.Object, *admissionv1.AdmissionResponse) {
+	if objectGVK == defaultInGVK {
+		obj, _, err := rh.decoder.Decode(bytes, nil, nil)
+		if err != nil {
+			return nil, badRequestError(status, err)
+		}
+		return obj, nil
+	}
+
+	// First, use the UniversalDeserializer to decode the bytes (which does not perform
+	// conversion or defaulting).
+	encodedObj, _, err := rh.codecFactory.UniversalDeserializer().Decode(bytes, nil, nil)
+	if err != nil {
+		return nil, badRequestError(status, err)
+	}
+
+	// Then convert into the internal version of the object
+	internalObj, err := rh.scheme.ConvertToVersion(encodedObj, runtime.InternalGroupVersioner)
+	if err != nil {
+		return nil, internalServerError(status, err)
+	}
+
+	// Now convert into the request version so we can apply the appropriate defaults
+	requestGroupVersioner := runtime.NewMultiGroupVersioner(schema.GroupVersion{Group: defaultInGVK.Group, Version: defaultInGVK.Version})
+	requestObj, err := rh.scheme.ConvertToVersion(internalObj, requestGroupVersioner)
+	if err != nil {
+		return nil, internalServerError(status, err)
+	}
+
+	// At last, apply defaults in the request API version
+	rh.scheme.Default(requestObj)
+
+	// Finally, convert the resource back to the internal version so regular admission can proceed
+	obj, err := rh.scheme.ConvertToVersion(requestObj, runtime.InternalGroupVersioner)
+	if err != nil {
+		return nil, internalServerError(status, err)
+	}
+
+	return obj, nil
+}
+
+func badRequestError(status *admissionv1.AdmissionResponse, err error) *admissionv1.AdmissionResponse {
+	status.Allowed = false
+	status.Result = &metav1.Status{
+		Status: metav1.StatusFailure, Code: http.StatusBadRequest, Reason: metav1.StatusReasonBadRequest,
+		Message: err.Error(),
+	}
+	return status
+}
+
+func internalServerError(status *admissionv1.AdmissionResponse, err error) *admissionv1.AdmissionResponse {
+	status.Allowed = false
+	status.Result = &metav1.Status{
+		Status: metav1.StatusFailure, Code: http.StatusInternalServerError, Reason: metav1.StatusReasonInternalError,
+		Message: err.Error(),
+	}
+	return status
+}
+
 // createMutatePatch will generate a JSON patch based upon the given original
 // raw object, and the mutated typed object.
 func (rh *RequestHandler) createMutatePatch(req *admissionv1.AdmissionRequest, obj runtime.Object) ([]byte, error) {
 	var buf bytes.Buffer
 
-	encoder := rh.codecFactory.EncoderForVersion(rh.serializer, schema.GroupVersion{Group: req.RequestKind.Group, Version: req.RequestKind.Version})
+	encoder := rh.codecFactory.EncoderForVersion(rh.serializer, schema.GroupVersion{Group: req.Kind.Group, Version: req.Kind.Version})
 	if err := encoder.Encode(obj, &buf); err != nil {
 		return nil, fmt.Errorf("failed to encode object after mutation: %s", err)
 	}

pkg/webhook/admission/request_handler_test.go

@@ -56,6 +56,11 @@ func TestRequestHandler_MutateAppliesDefaultValues(t *testing.T) {
 	inputRequest := admissionv1.AdmissionRequest{
 		UID:       types.UID("abc"),
 		Operation: admissionv1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "testgroup.testing.cert-manager.io",
+			Version: "v1",
+			Kind:    "TestType",
+		},
 		RequestKind: &metav1.GroupVersionKind{
 			Group:   "testgroup.testing.cert-manager.io",
 			Version: "v1",
@@ -71,7 +76,8 @@ func TestRequestHandler_MutateAppliesDefaultValues(t *testing.T) {
 		"namespace": "abc",
 		"creationTimestamp": null
 	},
-	"testFieldImmutable": "abc"
+	"testFieldImmutable": "abc",
+	"testDefaultingField": "set-to-something"
 }
 `),
 		},
@@ -100,6 +106,69 @@ func TestRequestHandler_MutateAppliesDefaultValues(t *testing.T) {
 	}
 }
 
+func TestRequestHandler_MutateAppliesDefaultsInRequestVersion(t *testing.T) {
+	scheme := runtime.NewScheme()
+	install.Install(scheme)
+
+	rh := admission.NewRequestHandler(scheme, nil, testMutator{
+		handles: true,
+		mutate: func(_ context.Context, _ admissionv1.AdmissionRequest, obj runtime.Object) error {
+			// Doesn't do anything as the request handler itself will generate patches to apply
+			// defaults instead of it being applied within a particular admission plugin.
+			return nil
+		},
+	})
+	inputRequest := admissionv1.AdmissionRequest{
+		UID:       types.UID("abc"),
+		Operation: admissionv1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "testgroup.testing.cert-manager.io",
+			Version: "v1",
+			Kind:    "TestType",
+		},
+		RequestKind: &metav1.GroupVersionKind{
+			Group: "testgroup.testing.cert-manager.io",
+			// Because the API version is v2, we expect the `testDefaultingField` field to be set to `set-in-v2`.
+			// In v1, the field will be set to `set-in-v1`.
+			Version: "v2",
+			Kind:    "TestType",
+		},
+		Object: runtime.RawExtension{
+			Raw: []byte(`
+{
+	"apiVersion": "testgroup.testing.cert-manager.io/v1",
+	"kind": "TestType",
+	"metadata": {
+		"name": "testing",
+		"namespace": "abc",
+		"creationTimestamp": null
+	},
+	"testField": "set-to-something-to-avoid-extra-mutations",
+	"testFieldImmutable": "set-to-something-to-avoid-extra-mutations",
+	"testFieldPtr": "set-to-something-to-avoid-extra-mutations"
+}
+`),
+		},
+	}
+	expectedResponse := admissionv1.AdmissionResponse{
+		UID:     types.UID("abc"),
+		Allowed: true,
+		Patch: responseForOperations(
+			jsonpatch.JsonPatchOperation{
+				Operation: "add",
+				Path:      "/testDefaultingField",
+				Value:     "set-in-v2",
+			},
+		),
+		PatchType: &jsonPatchType,
+	}
+
+	resp := rh.Mutate(context.TODO(), &inputRequest)
+	if !reflect.DeepEqual(&expectedResponse, resp) {
+		t.Errorf("Response was not as expected: %v", diff.ObjectGoPrintSideBySide(&expectedResponse, resp))
+	}
+}
+
 // Tests to ensure that the RequestHandler skips running mutation handlers
 // that do not return true to Handles, but still applies scheme based defaulting.
 func TestRequestHandler_MutateSkipsMutation(t *testing.T) {
@@ -112,6 +181,11 @@ func TestRequestHandler_MutateSkipsMutation(t *testing.T) {
 	inputRequest := admissionv1.AdmissionRequest{
 		UID:       types.UID("abc"),
 		Operation: admissionv1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "testgroup.testing.cert-manager.io",
+			Version: "v1",
+			Kind:    "TestType",
+		},
 		RequestKind: &metav1.GroupVersionKind{
 			Group:   "testgroup.testing.cert-manager.io",
 			Version: "v1",
@@ -128,7 +202,8 @@ func TestRequestHandler_MutateSkipsMutation(t *testing.T) {
 		"creationTimestamp": null
 	},
 	"testField": "some-value",
-	"testFieldImmutable": "abc"
+	"testFieldImmutable": "abc",
+	"testDefaultingField": "set-to-something"
 }
 `),
 		},
@@ -164,6 +239,11 @@ func TestRequestHandler_ValidateReturnsErrorsAndWarnings(t *testing.T) {
 	inputRequest := admissionv1.AdmissionRequest{
 		UID:       types.UID("abc"),
 		Operation: admissionv1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "testgroup.testing.cert-manager.io",
+			Version: "v1",
+			Kind:    "TestType",
+		},
 		RequestKind: &metav1.GroupVersionKind{
 			Group:   "testgroup.testing.cert-manager.io",
 			Version: "v1",

pkg/webhook/handlers/testdata/apis/testgroup/types.go

@@ -33,4 +33,7 @@ type TestType struct {
 
 	// TestFieldImmutable cannot be changed after being set to a non-zero value
 	TestFieldImmutable string
+
+	// TestDefaultingField is used to test defaulting.
+	TestDefaultingField string
 }

pkg/webhook/handlers/testdata/apis/testgroup/v1/defaults.go

@@ -29,4 +29,7 @@ func SetDefaults_TestType(obj *TestType) {
 	if obj.TestFieldPtr == nil {
 		obj.TestFieldPtr = pointer.StringPtr("teststr")
 	}
+	if obj.TestDefaultingField == "" {
+		obj.TestDefaultingField = "set-in-v1"
+	}
 }

pkg/webhook/handlers/testdata/apis/testgroup/v1/types.go

@@ -33,6 +33,11 @@ type TestType struct {
 
 	// TestFieldImmutable cannot be changed after being set to a non-zero value
 	TestFieldImmutable string `json:"testFieldImmutable"`
+
+	// TestDefaultingField is used to test defaulting.
+	// In the v1 API, it defaults to `set-in-v1`.
+	// In the v2 API, it defaults to `set-in-v2`.
+	TestDefaultingField string `json:"testDefaultingField,omitempty"`
 }
 
 const (

pkg/webhook/handlers/testdata/apis/testgroup/v1/zz_generated.conversion.go

@@ -54,6 +54,7 @@ func autoConvert_v1_TestType_To_testgroup_TestType(in *TestType, out *testgroup.
 	out.TestField = in.TestField
 	out.TestFieldPtr = (*string)(unsafe.Pointer(in.TestFieldPtr))
 	out.TestFieldImmutable = in.TestFieldImmutable
+	out.TestDefaultingField = in.TestDefaultingField
 	return nil
 }
 
@@ -67,6 +68,7 @@ func autoConvert_testgroup_TestType_To_v1_TestType(in *testgroup.TestType, out *
 	out.TestField = in.TestField
 	out.TestFieldPtr = (*string)(unsafe.Pointer(in.TestFieldPtr))
 	out.TestFieldImmutable = in.TestFieldImmutable
+	out.TestDefaultingField = in.TestDefaultingField
 	return nil
 }
 

pkg/webhook/handlers/testdata/apis/testgroup/v2/defaults.go

@@ -29,4 +29,7 @@ func SetDefaults_TestType(obj *TestType) {
 	if obj.TestFieldPtrAlt == nil {
 		obj.TestFieldPtrAlt = pointer.StringPtr("teststr")
 	}
+	if obj.TestDefaultingField == "" {
+		obj.TestDefaultingField = "set-in-v2"
+	}
 }

pkg/webhook/handlers/testdata/apis/testgroup/v2/types.go

@@ -36,6 +36,11 @@ type TestType struct {
 
 	// TestFieldImmutable cannot be changed after being set to a non-zero value
 	TestFieldImmutable string `json:"testFieldImmutable"`
+
+	// TestDefaultingField is used to test defaulting.
+	// In the v1 API, it defaults to `set-in-v1`.
+	// In the v2 API, it defaults to `set-in-v2`.
+	TestDefaultingField string `json:"testDefaultingField,omitempty"`
 }
 
 const (

pkg/webhook/handlers/testdata/apis/testgroup/v2/zz_generated.conversion.go

@@ -52,6 +52,7 @@ func autoConvert_v2_TestType_To_testgroup_TestType(in *TestType, out *testgroup.
 	out.TestField = in.TestField
 	// WARNING: in.TestFieldPtrAlt requires manual conversion: does not exist in peer-type
 	out.TestFieldImmutable = in.TestFieldImmutable
+	out.TestDefaultingField = in.TestDefaultingField
 	return nil
 }
 
@@ -60,5 +61,6 @@ func autoConvert_testgroup_TestType_To_v2_TestType(in *testgroup.TestType, out *
 	out.TestField = in.TestField
 	// WARNING: in.TestFieldPtr requires manual conversion: does not exist in peer-type
 	out.TestFieldImmutable = in.TestFieldImmutable
+	out.TestDefaultingField = in.TestDefaultingField
 	return nil
 }