diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror.go index df38afdcb04..00bbf54d2cd 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror.go @@ -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" @@ -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 + } + // Make sure we don't return "" or "Success" status along with a rejection + if result.Status == "" || result.Status == metav1.StatusSuccess { + result.Status = metav1.StatusFailure + } + switch { case len(result.Message) > 0: result.Message = fmt.Sprintf("%s: %s", deniedBy, result.Message) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror_test.go index 98b780b4517..eea28327e24 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror_test.go @@ -27,14 +27,18 @@ 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", @@ -42,6 +46,8 @@ func TestToStatusErr(t *testing.T) { Message: "you shall not pass", }, deniedBy + ": you shall not pass", + 400, + metav1.StatusFailure, }, { "only reason", @@ -49,6 +55,8 @@ func TestToStatusErr(t *testing.T) { Reason: metav1.StatusReasonForbidden, }, deniedBy + ": Forbidden", + 400, + metav1.StatusFailure, }, { "message and reason", @@ -57,11 +65,78 @@ 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 { @@ -69,5 +144,11 @@ func TestToStatusErr(t *testing.T) { 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) + } } }