From fdca3f35f8a3395ab8ef6acce1426f2d2ab4c03a Mon Sep 17 00:00:00 2001 From: Zach Loafman Date: Mon, 12 Dec 2022 19:53:06 +0000 Subject: [PATCH] Skip validation errors in mutating webhooks Request processing by kube-apiserver is roughly: { mutating webhooks } => { OpenAPI schema validation } => { validating webhooks } In this PR, we disable validation errors (at least, JSON unmarshalling validation errors) during mutating webhook processing. It seems counter-intuitive not to return an error in mutating webhooks for a serious violation such as invalid JSON, but doing so allows OpenAPI schema validation to proceed. Schema validation gives us nice errors, so e.g.: instead of: ``` The Fleet "" is invalid ``` we get: ``` The Fleet "fleet-example" is invalid: spec.template.spec.sdkServer.grpcPort: Invalid value: 33332229357: spec.template.spec.sdkServer.grpcPort in body should be less than or equal to 65535 ``` This will become all the more important if we eventually move to CEL based validations. I'm also changing the error in #2863 to an internal error - after this PR, any hard error from a handler (vs a cause) is unexpected. Fixes #1770 --- pkg/fleetautoscalers/controller.go | 7 ++++--- pkg/fleetautoscalers/controller_test.go | 13 ++++++------- pkg/fleets/controller.go | 6 ++++-- pkg/fleets/controller_test.go | 7 ++++--- pkg/gameservers/controller.go | 8 ++++---- pkg/gameservers/controller_test.go | 13 ++++++------- pkg/gameserversets/controller.go | 2 +- pkg/gameserversets/controller_test.go | 2 +- pkg/util/webhooks/webhooks.go | 3 ++- 9 files changed, 32 insertions(+), 29 deletions(-) diff --git a/pkg/fleetautoscalers/controller.go b/pkg/fleetautoscalers/controller.go index bc9dddcc35..1eb423086b 100644 --- a/pkg/fleetautoscalers/controller.go +++ b/pkg/fleetautoscalers/controller.go @@ -198,8 +198,9 @@ func (c *Controller) mutationHandler(review admissionv1.AdmissionReview) (admiss fas := &autoscalingv1.FleetAutoscaler{} err := json.Unmarshal(obj.Raw, fas) if err != nil { - c.baseLogger.WithField("review", review).WithError(err).Error("validationHandler") - return review, errors.Wrapf(err, "error unmarshalling original FleetAutoscaler json: %s", obj.Raw) + // If the JSON is invalid during mutation, fall through to validation. This allows OpenAPI schema validation + // to proceed, resulting in a more user friendly error message. + return review, nil } fas.ApplyDefaults() @@ -234,7 +235,7 @@ func (c *Controller) validationHandler(review admissionv1.AdmissionReview) (admi err := json.Unmarshal(obj.Raw, fas) if err != nil { c.baseLogger.WithField("review", review).WithError(err).Error("validationHandler") - return review, errors.Wrapf(err, "error unmarshalling original FleetAutoscaler json: %s", obj.Raw) + return review, errors.Wrapf(err, "error unmarshalling FleetAutoscaler json after schema validation: %s", obj.Raw) } fas.ApplyDefaults() var causes []metav1.StatusCause diff --git a/pkg/fleetautoscalers/controller_test.go b/pkg/fleetautoscalers/controller_test.go index 58889577a1..cc1525f3f4 100644 --- a/pkg/fleetautoscalers/controller_test.go +++ b/pkg/fleetautoscalers/controller_test.go @@ -57,7 +57,7 @@ func TestControllerCreationMutationHandler(t *testing.T) { type expected struct { responseAllowed bool patches []jsonpatch.JsonPatchOperation - err string + nilPatch bool } var testCases = []struct { @@ -134,9 +134,7 @@ func TestControllerCreationMutationHandler(t *testing.T) { { description: "Wrong request object, err expected", fixture: "WRONG DATA", - expected: expected{ - err: `error unmarshalling original FleetAutoscaler json: "WRONG DATA": json: cannot unmarshal string into Go value of type v1.FleetAutoscaler`, - }, + expected: expected{nilPatch: true}, }, } @@ -160,8 +158,9 @@ func TestControllerCreationMutationHandler(t *testing.T) { result, err := c.mutationHandler(review) - if err != nil && tc.expected.err != "" { - require.Equal(t, tc.expected.err, err.Error()) + assert.NoError(t, err) + if tc.expected.nilPatch { + require.Nil(t, result.Response.PatchType) } else { assert.True(t, result.Response.Allowed) assert.Equal(t, admissionv1.PatchTypeJSONPatch, *result.Response.PatchType) @@ -233,7 +232,7 @@ func TestControllerCreationValidationHandler(t *testing.T) { _, err = c.validationHandler(review) if assert.NotNil(t, err) { - assert.Equal(t, "error unmarshalling original FleetAutoscaler json: \"MQ==\": json: cannot unmarshal string into Go value of type v1.FleetAutoscaler", err.Error()) + assert.Equal(t, "error unmarshalling FleetAutoscaler json after schema validation: \"MQ==\": json: cannot unmarshal string into Go value of type v1.FleetAutoscaler", err.Error()) } }) } diff --git a/pkg/fleets/controller.go b/pkg/fleets/controller.go index 5671c2a2d5..b2b0e7b74a 100644 --- a/pkg/fleets/controller.go +++ b/pkg/fleets/controller.go @@ -136,7 +136,9 @@ func (c *Controller) creationMutationHandler(review admissionv1.AdmissionReview) fleet := &agonesv1.Fleet{} err := json.Unmarshal(obj.Raw, fleet) if err != nil { - return review, errors.Wrapf(err, "error unmarshalling original Fleet json: %s", obj.Raw) + // If the JSON is invalid during mutation, fall through to validation. This allows OpenAPI schema validation + // to proceed, resulting in a more user friendly error message. + return review, nil } // This is the main logic of this function @@ -176,7 +178,7 @@ func (c *Controller) creationValidationHandler(review admissionv1.AdmissionRevie fleet := &agonesv1.Fleet{} err := json.Unmarshal(obj.Raw, fleet) if err != nil { - return review, errors.Wrapf(err, "error unmarshalling original Fleet json: %s", obj.Raw) + return review, errors.Wrapf(err, "error unmarshalling Fleet json after schema validation: %s", obj.Raw) } causes, ok := fleet.Validate() diff --git a/pkg/fleets/controller_test.go b/pkg/fleets/controller_test.go index de0b5381ee..0d8d10bbcf 100644 --- a/pkg/fleets/controller_test.go +++ b/pkg/fleets/controller_test.go @@ -418,7 +418,7 @@ func TestControllerCreationValidationHandler(t *testing.T) { review := getAdmissionReview(raw) _, err = c.creationValidationHandler(review) - assert.EqualError(t, err, "error unmarshalling original Fleet json: \"MQ==\": json: cannot unmarshal string into Go value of type v1.Fleet") + assert.EqualError(t, err, "error unmarshalling Fleet json after schema validation: \"MQ==\": json: cannot unmarshal string into Go value of type v1.Fleet") }) t.Run("invalid fleet", func(t *testing.T) { @@ -492,8 +492,9 @@ func TestControllerCreationMutationHandler(t *testing.T) { require.NoError(t, err) review := getAdmissionReview(raw) - _, err = c.creationMutationHandler(review) - assert.EqualError(t, err, "error unmarshalling original Fleet json: \"MQ==\": json: cannot unmarshal string into Go value of type v1.Fleet") + result, err := c.creationMutationHandler(review) + assert.NoError(t, err) + require.Nil(t, result.Response.PatchType) }) } diff --git a/pkg/gameservers/controller.go b/pkg/gameservers/controller.go index 75c49a9a00..bc7f76da54 100644 --- a/pkg/gameservers/controller.go +++ b/pkg/gameservers/controller.go @@ -232,8 +232,9 @@ func (c *Controller) creationMutationHandler(review admissionv1.AdmissionReview) gs := &agonesv1.GameServer{} err := json.Unmarshal(obj.Raw, gs) if err != nil { - c.baseLogger.WithField("review", review).WithError(err).Error("creationMutationHandler failed to unmarshal JSON") - return review, errors.Wrapf(err, "error unmarshalling original GameServer json: %s", obj.Raw) + // If the JSON is invalid during mutation, fall through to validation. This allows OpenAPI schema validation + // to proceed, resulting in a more user friendly error message. + return review, nil } // This is the main logic of this function @@ -281,8 +282,7 @@ func (c *Controller) creationValidationHandler(review admissionv1.AdmissionRevie gs := &agonesv1.GameServer{} err := json.Unmarshal(obj.Raw, gs) if err != nil { - c.baseLogger.WithField("review", review).WithError(err).Error("creationValidationHandler failed to unmarshal JSON") - return review, errors.Wrapf(err, "error unmarshalling original GameServer json: %s", obj.Raw) + return review, errors.Wrapf(err, "error unmarshalling GameServer json after schema validation: %s", obj.Raw) } c.loggerForGameServer(gs).WithField("review", review).Debug("creationValidationHandler") diff --git a/pkg/gameservers/controller_test.go b/pkg/gameservers/controller_test.go index 95271949c0..f84fd133b0 100644 --- a/pkg/gameservers/controller_test.go +++ b/pkg/gameservers/controller_test.go @@ -378,7 +378,7 @@ func TestControllerCreationMutationHandler(t *testing.T) { type expected struct { responseAllowed bool patches []jsonpatch.JsonPatchOperation - err string + nilPatch bool } var testCases = []struct { @@ -400,9 +400,7 @@ func TestControllerCreationMutationHandler(t *testing.T) { { description: "Wrong request object, err expected", fixture: "WRONG DATA", - expected: expected{ - err: `error unmarshalling original GameServer json: "WRONG DATA": json: cannot unmarshal string into Go value of type v1.GameServer`, - }, + expected: expected{nilPatch: true}, }, } @@ -426,8 +424,9 @@ func TestControllerCreationMutationHandler(t *testing.T) { result, err := c.creationMutationHandler(review) - if err != nil && tc.expected.err != "" { - require.Equal(t, tc.expected.err, err.Error()) + assert.NoError(t, err) + if tc.expected.nilPatch { + require.Nil(t, result.Response.PatchType) } else { assert.True(t, result.Response.Allowed) assert.Equal(t, admissionv1.PatchTypeJSONPatch, *result.Response.PatchType) @@ -534,7 +533,7 @@ func TestControllerCreationValidationHandler(t *testing.T) { _, err = c.creationValidationHandler(review) if assert.Error(t, err) { - assert.Equal(t, `error unmarshalling original GameServer json: "WRONG DATA": json: cannot unmarshal string into Go value of type v1.GameServer`, err.Error()) + assert.Equal(t, `error unmarshalling GameServer json after schema validation: "WRONG DATA": json: cannot unmarshal string into Go value of type v1.GameServer`, err.Error()) } }) } diff --git a/pkg/gameserversets/controller.go b/pkg/gameserversets/controller.go index ff4ec1c04e..fd8bd21823 100644 --- a/pkg/gameserversets/controller.go +++ b/pkg/gameserversets/controller.go @@ -217,7 +217,7 @@ func (c *Controller) creationValidationHandler(review admissionv1.AdmissionRevie newObj := review.Request.Object if err := json.Unmarshal(newObj.Raw, newGss); err != nil { - return review, errors.Wrapf(err, "error unmarshalling new GameServerSet json: %s", newObj.Raw) + return review, errors.Wrapf(err, "error unmarshalling GameServerSet json after schema validation: %s", newObj.Raw) } causes, ok := newGss.Validate() diff --git a/pkg/gameserversets/controller_test.go b/pkg/gameserversets/controller_test.go index 373d6d3352..2524b334ef 100644 --- a/pkg/gameserversets/controller_test.go +++ b/pkg/gameserversets/controller_test.go @@ -928,7 +928,7 @@ func TestCreationValidationHandler(t *testing.T) { _, err := c.creationValidationHandler(review) require.Error(t, err) - assert.Equal(t, "error unmarshalling new GameServerSet json: : unexpected end of JSON input", err.Error()) + assert.Equal(t, "error unmarshalling GameServerSet json after schema validation: : unexpected end of JSON input", err.Error()) }) t.Run("invalid gameserverset create", func(t *testing.T) { diff --git a/pkg/util/webhooks/webhooks.go b/pkg/util/webhooks/webhooks.go index ef0160b423..a0c2898a50 100644 --- a/pkg/util/webhooks/webhooks.go +++ b/pkg/util/webhooks/webhooks.go @@ -100,13 +100,14 @@ func (wh *WebHook) handle(path string, w http.ResponseWriter, r *http.Request) e Group: review.Request.Kind.Group, Kind: review.Request.Kind.Kind, Causes: []metav1.StatusCause{{ + Type: metav1.CauseType("InternalError"), Message: err.Error(), }}, } review.Response.Result = &metav1.Status{ Status: metav1.StatusFailure, Message: err.Error(), - Reason: metav1.StatusReasonInvalid, + Reason: metav1.StatusReasonInternalError, Details: &details, } }