From c2fcdc818be1441dd788cae22648c04b1650d3af Mon Sep 17 00:00:00 2001 From: zhouhaibing089 Date: Wed, 9 Jan 2019 14:28:33 -0800 Subject: [PATCH] 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. --- .../plugin/webhook/mutating/dispatcher.go | 3 +- .../plugin/webhook/mutating/plugin_test.go | 6 ++- .../plugin/webhook/testing/testcase.go | 39 ++++++++++++------- .../plugin/webhook/testing/webhook_server.go | 4 ++ 4 files changed, 36 insertions(+), 16 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go index 441e64f838d..80449dc6106 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go +++ b/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 diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go index e4ff80295e8..4a1a1c328b4 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go +++ b/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 { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go index 7a82411744c..b99d8ab45b1 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go +++ b/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"}, }}, - - ErrorContains: "you shall not pass", + ExpectStatusCode: http.StatusForbidden, + 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, - ErrorContains: "does not support dry run", + IsDryRun: true, + 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, - ErrorContains: "does not support dry run", + IsDryRun: true, + 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, - ErrorContains: "does not support dry run", + IsDryRun: true, + ExpectStatusCode: http.StatusBadRequest, + ErrorContains: "does not support dry run", }, // No need to test everything with the url case, since only the // connection is different. diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/webhook_server.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/webhook_server.go index 0af53633532..5d080745dca 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/webhook_server.go +++ b/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, }, }, })