webhook: respect the status error from webhook

today, apiserver generates an internal server error for any call
to mutatingwebhook if it gives allowed=false. this is not right as
it is really not an intenal error, it can be a forbidden as well
if the webhook wants it to be.

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

@@ -72,8 +72,9 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr *generic.Version
 				continue
 			}
 			klog.Warningf("Failed calling webhook, failing closed %v: %v", hook.Name, err)
+			return apierrors.NewInternalError(err)
 		}
-		return apierrors.NewInternalError(err)
+		return err
 	}
 
 	// convert attr.VersionedObject to the internal version in the underlying admission.Attributes

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go

@@ -93,8 +93,12 @@ func TestAdmit(t *testing.T) {
 				t.Errorf("%s: expected an error saying %q, but got: %v", tt.Name, tt.ErrorContains, err)
 			}
 		}
-		if _, isStatusErr := err.(*errors.StatusError); err != nil && !isStatusErr {
+		if statusErr, isStatusErr := err.(*errors.StatusError); err != nil && !isStatusErr {
 			t.Errorf("%s: expected a StatusError, got %T", tt.Name, err)
+		} else if isStatusErr {
+			if statusErr.ErrStatus.Code != tt.ExpectStatusCode {
+				t.Errorf("%s: expected status code %d, got %d", tt.Name, tt.ExpectStatusCode, statusErr.ErrStatus.Code)
+			}
 		}
 		fakeAttr, ok := attr.(*webhooktesting.FakeAttributes)
 		if !ok {

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go

@@ -17,6 +17,7 @@ limitations under the License.
 package testing
 
 import (
+	"net/http"
 	"net/url"
 	"sync"
 
@@ -191,6 +192,7 @@ type Test struct {
 	ExpectAllow       bool
 	ErrorContains     string
 	ExpectAnnotations map[string]string
+	ExpectStatusCode  int32
 }
 
 // NewNonMutatingTestCases returns test cases with a given base url.
@@ -237,7 +239,8 @@ func NewNonMutatingTestCases(url *url.URL) []Test {
 				NamespaceSelector:       &metav1.LabelSelector{},
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-			ErrorContains: "without explanation",
+			ExpectStatusCode: http.StatusForbidden,
+			ErrorContains:    "without explanation",
 		},
 		{
 			Name: "match & disallow ii",
@@ -248,8 +251,8 @@ func NewNonMutatingTestCases(url *url.URL) []Test {
 				NamespaceSelector:       &metav1.LabelSelector{},
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-
+			ExpectStatusCode: http.StatusForbidden,
-			ErrorContains: "you shall not pass",
+			ErrorContains:    "you shall not pass",
 		},
 		{
 			Name: "match & disallow & but allowed because namespaceSelector exempt the ns",
@@ -334,7 +337,8 @@ func NewNonMutatingTestCases(url *url.URL) []Test {
 				Rules:                   matchEverythingRules,
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-			ExpectAllow: false,
+			ExpectStatusCode: http.StatusInternalServerError,
+			ExpectAllow:      false,
 		},
 		{
 			Name: "match & fail (but fail because fail closed)",
@@ -360,7 +364,8 @@ func NewNonMutatingTestCases(url *url.URL) []Test {
 				FailurePolicy:           &policyFail,
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-			ExpectAllow: false,
+			ExpectStatusCode: http.StatusInternalServerError,
+			ExpectAllow:      false,
 		},
 		{
 			Name: "match & allow (url)",
@@ -383,7 +388,8 @@ func NewNonMutatingTestCases(url *url.URL) []Test {
 				NamespaceSelector:       &metav1.LabelSelector{},
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-			ErrorContains: "without explanation",
+			ExpectStatusCode: http.StatusForbidden,
+			ErrorContains:    "without explanation",
 		}, {
 			Name: "absent response and fail open",
 			Webhooks: []registrationv1beta1.Webhook{{
@@ -406,7 +412,8 @@ func NewNonMutatingTestCases(url *url.URL) []Test {
 				NamespaceSelector:       &metav1.LabelSelector{},
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-			ErrorContains: "Webhook response was absent",
+			ExpectStatusCode: http.StatusInternalServerError,
+			ErrorContains:    "Webhook response was absent",
 		},
 		{
 			Name: "no match dry run",
@@ -433,8 +440,9 @@ func NewNonMutatingTestCases(url *url.URL) []Test {
 				SideEffects:             &sideEffectsUnknown,
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-			IsDryRun:      true,
+			IsDryRun:         true,
-			ErrorContains: "does not support dry run",
+			ExpectStatusCode: http.StatusBadRequest,
+			ErrorContains:    "does not support dry run",
 		},
 		{
 			Name: "match dry run side effects None",
@@ -460,8 +468,9 @@ func NewNonMutatingTestCases(url *url.URL) []Test {
 				SideEffects:             &sideEffectsSome,
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-			IsDryRun:      true,
+			IsDryRun:         true,
-			ErrorContains: "does not support dry run",
+			ExpectStatusCode: http.StatusBadRequest,
+			ErrorContains:    "does not support dry run",
 		},
 		{
 			Name: "match dry run side effects NoneOnDryRun",
@@ -561,7 +570,8 @@ func NewMutatingTestCases(url *url.URL) []Test {
 				NamespaceSelector:       &metav1.LabelSelector{},
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-			ErrorContains: "invalid character",
+			ExpectStatusCode: http.StatusInternalServerError,
+			ErrorContains:    "invalid character",
 		},
 		{
 			Name: "match & remove label dry run unsupported",
@@ -573,8 +583,9 @@ func NewMutatingTestCases(url *url.URL) []Test {
 				SideEffects:             &sideEffectsUnknown,
 				AdmissionReviewVersions: []string{"v1beta1"},
 			}},
-			IsDryRun:      true,
+			IsDryRun:         true,
-			ErrorContains: "does not support dry run",
+			ExpectStatusCode: http.StatusBadRequest,
+			ErrorContains:    "does not support dry run",
 		},
 		// No need to test everything with the url case, since only the
 		// connection is different.

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/webhook_server.go

@@ -66,6 +66,9 @@ func webhookHandler(w http.ResponseWriter, r *http.Request) {
 		json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{
 			Response: &v1beta1.AdmissionResponse{
 				Allowed: false,
+				Result: &metav1.Status{
+					Code: http.StatusForbidden,
+				},
 			},
 		})
 	case "/disallowReason":
@@ -75,6 +78,7 @@ func webhookHandler(w http.ResponseWriter, r *http.Request) {
 				Allowed: false,
 				Result: &metav1.Status{
 					Message: "you shall not pass",
+					Code:    http.StatusForbidden,
 				},
 			},
 		})