Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skip validation errors in mutating webhooks #2865

Merged
merged 1 commit into from
Dec 13, 2022
Merged

Skip validation errors in mutating webhooks #2865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions pkg/fleetautoscalers/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down
13 changes: 6 additions & 7 deletions pkg/fleetautoscalers/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func TestControllerCreationMutationHandler(t *testing.T) {
type expected struct {
responseAllowed bool
patches []jsonpatch.JsonPatchOperation
err string
nilPatch bool
}

var testCases = []struct {
Expand Down Expand Up @@ -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},
},
}

Expand All @@ -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)
Expand Down Expand Up @@ -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())
}
})
}
Expand Down
6 changes: 4 additions & 2 deletions pkg/fleets/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
7 changes: 4 additions & 3 deletions pkg/fleets/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
})
}

Expand Down
8 changes: 4 additions & 4 deletions pkg/gameservers/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down
13 changes: 6 additions & 7 deletions pkg/gameservers/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ func TestControllerCreationMutationHandler(t *testing.T) {
type expected struct {
responseAllowed bool
patches []jsonpatch.JsonPatchOperation
err string
nilPatch bool
}

var testCases = []struct {
Expand All @@ -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},
},
}

Expand All @@ -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)
Expand Down Expand Up @@ -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())
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/gameserversets/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion pkg/gameserversets/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion pkg/util/webhooks/webhooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}
Expand Down