Merge pull request #77022 from liggitt/webhook-error-success

Ensure 4xx+ response codes from webhook rejections

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)

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror_test.go

@@ -30,11 +30,15 @@ func TestToStatusErr(t *testing.T) {
 		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)
+		}
 	}
 }