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

Ensure 4xx+ response codes from webhook rejections #77022

Merged
merged 1 commit into from
May 4, 2019
Merged

Ensure 4xx+ response codes from webhook rejections #77022

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
10 changes: 10 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package errors

import (
"fmt"
"net/http"

apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -32,6 +33,15 @@ func ToStatusErr(webhookName string, result *metav1.Status) *apierrors.StatusErr
result = &metav1.Status{Status: metav1.StatusFailure}
}

// Make sure we don't return < 400 status codes along with a rejection
if result.Code < http.StatusBadRequest {
result.Code = http.StatusBadRequest
liggitt marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about switching this to a 500 in the api-machinery call today, but the more I consider this, I think that would be misleading to the end user. The reason their request was rejected was because their request was judged to be bad, not because the server had some error.

I see this as akin to what we do here:

kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go

Lines 119 to 126 in 7289c1e

_, serializer, err := negotiation.NegotiateOutputMediaType(req, s, restrictions)
if err != nil {
// if original statusCode was not successful we need to return the original error
// we cannot hide it behind negotiation problems
if statusCode < http.StatusOK || statusCode >= http.StatusBadRequest {
WriteRawJSON(int(statusCode), object, w)
return
}

if we encounter a negotiation error in the process of trying to write an error response, we don't switch to a 500, we preserve the original error code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally find that argument compelling.

}
// Make sure we don't return "" or "Success" status along with a rejection
if result.Status == "" || result.Status == metav1.StatusSuccess {
liggitt marked this conversation as resolved.
Show resolved Hide resolved
result.Status = metav1.StatusFailure
}

switch {
case len(result.Message) > 0:
result.Message = fmt.Sprintf("%s: %s", deniedBy, result.Message)
Expand Down
87 changes: 84 additions & 3 deletions staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,28 +27,36 @@ func TestToStatusErr(t *testing.T) {
hookName := "foo"
deniedBy := fmt.Sprintf("admission webhook %q denied the request", hookName)
tests := []struct {
name string
result *metav1.Status
expectedError string
name string
result *metav1.Status
expectedError string
expectedCode int32
expectedStatus string
}{
{
"nil result",
nil,
deniedBy + " without explanation",
400,
metav1.StatusFailure,
},
{
"only message",
&metav1.Status{
Message: "you shall not pass",
},
deniedBy + ": you shall not pass",
400,
metav1.StatusFailure,
},
{
"only reason",
&metav1.Status{
Reason: metav1.StatusReasonForbidden,
},
deniedBy + ": Forbidden",
400,
metav1.StatusFailure,
},
{
"message and reason",
Expand All @@ -57,17 +65,90 @@ func TestToStatusErr(t *testing.T) {
Reason: metav1.StatusReasonForbidden,
},
deniedBy + ": you shall not pass",
400,
metav1.StatusFailure,
},
{
"no message, no reason",
&metav1.Status{},
deniedBy + " without explanation",
400,
metav1.StatusFailure,
},
{
"custom 4xx status code",
&metav1.Status{Code: 401},
deniedBy + " without explanation",
401,
metav1.StatusFailure,
},
{
"custom 5xx status code",
&metav1.Status{Code: 500},
deniedBy + " without explanation",
500,
metav1.StatusFailure,
},
{
"200 status code",
&metav1.Status{Code: 200},
deniedBy + " without explanation",
400,
metav1.StatusFailure,
},
{
"300 status code",
&metav1.Status{Code: 300},
deniedBy + " without explanation",
400,
metav1.StatusFailure,
},
{
"399 status code",
&metav1.Status{Code: 399},
deniedBy + " without explanation",
400,
metav1.StatusFailure,
},
{
"missing status",
&metav1.Status{},
deniedBy + " without explanation",
400,
metav1.StatusFailure,
},
{
"success status overridden",
&metav1.Status{Status: metav1.StatusSuccess},
deniedBy + " without explanation",
400,
metav1.StatusFailure,
},
{
"failure status preserved",
&metav1.Status{Status: metav1.StatusFailure},
deniedBy + " without explanation",
400,
metav1.StatusFailure,
},
{
"custom status preserved",
&metav1.Status{Status: "custom"},
deniedBy + " without explanation",
400,
"custom",
},
}
for _, test := range tests {
err := ToStatusErr(hookName, test.result)
if err == nil || err.Error() != test.expectedError {
t.Errorf("%s: expected an error saying %q, but got %v", test.name, test.expectedError, err)
}
if err.ErrStatus.Code != test.expectedCode {
t.Errorf("%s: expected code %d, got %d", test.name, test.expectedCode, err.ErrStatus.Code)
}
if err.ErrStatus.Status != test.expectedStatus {
t.Errorf("%s: expected code %q, got %q", test.name, test.expectedStatus, err.ErrStatus.Status)
}
}
}