From 0bba671152869e919cae1b31bf80ebb2b2b431df Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Thu, 16 Dec 2021 15:00:23 +0000 Subject: [PATCH] Ensure defaulting is applied in the correct API version during mutation Signed-off-by: James Munnelly --- pkg/webhook/admission/request_handler.go | 120 +++++++++++++----- pkg/webhook/admission/request_handler_test.go | 84 +++++++++++- .../handlers/testdata/apis/testgroup/types.go | 3 + .../testdata/apis/testgroup/v1/defaults.go | 3 + .../testdata/apis/testgroup/v1/types.go | 5 + .../testgroup/v1/zz_generated.conversion.go | 2 + .../testdata/apis/testgroup/v2/defaults.go | 3 + .../testdata/apis/testgroup/v2/types.go | 5 + .../testgroup/v2/zz_generated.conversion.go | 2 + 9 files changed, 191 insertions(+), 36 deletions(-) diff --git a/pkg/webhook/admission/request_handler.go b/pkg/webhook/admission/request_handler.go index 888f7cdca..4778dd5c5 100644 --- a/pkg/webhook/admission/request_handler.go +++ b/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) } diff --git a/pkg/webhook/admission/request_handler_test.go b/pkg/webhook/admission/request_handler_test.go index 7776c4ffd..aede70d80 100644 --- a/pkg/webhook/admission/request_handler_test.go +++ b/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", diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/types.go b/pkg/webhook/handlers/testdata/apis/testgroup/types.go index 30e9622e2..9c8b10b82 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/types.go +++ b/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 } diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/v1/defaults.go b/pkg/webhook/handlers/testdata/apis/testgroup/v1/defaults.go index d846c790c..740bf66b4 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/v1/defaults.go +++ b/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" + } } diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/v1/types.go b/pkg/webhook/handlers/testdata/apis/testgroup/v1/types.go index b7788f968..8516e48be 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/v1/types.go +++ b/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 ( diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/v1/zz_generated.conversion.go b/pkg/webhook/handlers/testdata/apis/testgroup/v1/zz_generated.conversion.go index 61a7746f6..5eb4d516f 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/v1/zz_generated.conversion.go +++ b/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 } diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/v2/defaults.go b/pkg/webhook/handlers/testdata/apis/testgroup/v2/defaults.go index 322813f66..7466c42f3 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/v2/defaults.go +++ b/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" + } } diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/v2/types.go b/pkg/webhook/handlers/testdata/apis/testgroup/v2/types.go index b2e3c5017..a6f49622a 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/v2/types.go +++ b/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 ( diff --git a/pkg/webhook/handlers/testdata/apis/testgroup/v2/zz_generated.conversion.go b/pkg/webhook/handlers/testdata/apis/testgroup/v2/zz_generated.conversion.go index 87b34690f..a432a790e 100644 --- a/pkg/webhook/handlers/testdata/apis/testgroup/v2/zz_generated.conversion.go +++ b/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 }