diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go index b8bcb6a690a..661575e021e 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go @@ -305,6 +305,11 @@ func (m *AdmissionMetrics) ObserveWebhookRejection(ctx context.Context, name, st m.webhookRejection.WithContext(ctx).WithLabelValues(name, stepType, operation, string(errorType), strconv.Itoa(rejectionCode)).Inc() } +// WebhookRejectionGathererForTest exposes admission webhook rejection metric for access by unit test. +func (m *AdmissionMetrics) WebhookRejectionGathererForTest() *metrics.CounterVec { + return m.webhookRejection +} + // ObserveWebhookFailOpen records validating or mutating webhook that fail open. func (m *AdmissionMetrics) ObserveWebhookFailOpen(ctx context.Context, name, stepType string) { m.webhookFailOpen.WithContext(ctx).WithLabelValues(name, stepType).Inc() 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 0622ff41f12..cb241caae7a 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 @@ -261,7 +261,7 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *admiss // Make the webhook request client, err := invocation.Webhook.GetRESTClient(a.cm) if err != nil { - return false, &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("could not get REST client: %w", err), Status: apierrors.NewBadRequest("error getting REST client")} + return false, &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("could not get REST client: %w", err), Status: apierrors.NewInternalError(err)} } ctx, span := tracing.Start(ctx, "Call mutating webhook", attribute.String("configuration", configurationName), @@ -305,7 +305,7 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *admiss if se, ok := err.(*apierrors.StatusError); ok { status = se } else { - status = apierrors.NewBadRequest("error calling webhook") + status = apierrors.NewServiceUnavailable("error calling webhook") } return false, &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("failed to call webhook: %w", err), Status: status} } 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 5da27169bfc..9479964e032 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 @@ -26,14 +26,16 @@ import ( "testing" "github.com/stretchr/testify/assert" - "k8s.io/apiserver/pkg/endpoints/request" - clocktesting "k8s.io/utils/clock/testing" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/admission" + admissionmetrics "k8s.io/apiserver/pkg/admission/metrics" webhooktesting "k8s.io/apiserver/pkg/admission/plugin/webhook/testing" auditinternal "k8s.io/apiserver/pkg/apis/audit" + "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/component-base/metrics/testutil" + clocktesting "k8s.io/utils/clock/testing" ) // BenchmarkAdmit tests the performance cost of invoking a mutating webhook @@ -156,6 +158,9 @@ func TestAdmit(t *testing.T) { attr = webhooktesting.NewAttribute(ns, tt.AdditionalLabels, tt.IsDryRun) } + if len(tt.ExpectRejectionMetrics) > 0 { + admissionmetrics.Metrics.WebhookRejectionGathererForTest().Reset() + } err = wh.Admit(context.TODO(), attr, objectInterfaces) if tt.ExpectAllow != (err == nil) { t.Errorf("expected allowed=%v, but got err=%v", tt.ExpectAllow, err) @@ -178,6 +183,15 @@ func TestAdmit(t *testing.T) { t.Errorf("expected status code %d, got %d", tt.ExpectStatusCode, statusErr.ErrStatus.Code) } } + if len(tt.ExpectRejectionMetrics) > 0 { + expectedMetrics := ` +# HELP apiserver_admission_webhook_rejection_count [ALPHA] Admission webhook rejection count, identified by name and broken out for each admission type (validating or admit) and operation. Additional labels specify an error type (calling_webhook_error or apiserver_internal_error if an error occurred; no_error otherwise) and optionally a non-zero rejection code if the webhook rejects the request with an HTTP status code (honored by the apiserver when the code is greater or equal to 400). Codes greater than 600 are truncated to 600, to keep the metrics cardinality bounded. +# TYPE apiserver_admission_webhook_rejection_count counter +` + tt.ExpectRejectionMetrics + "\n" + if err := testutil.CollectAndCompare(admissionmetrics.Metrics.WebhookRejectionGathererForTest(), strings.NewReader(expectedMetrics), "apiserver_admission_webhook_rejection_count"); err != nil { + t.Errorf("unexpected collecting result:\n%s", err) + } + } fakeAttr, ok := attr.(*webhooktesting.FakeAttributes) if !ok { t.Errorf("Unexpected error, failed to convert attr to webhooktesting.FakeAttributes") 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 da597a3f38d..4581b8e0e7c 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 @@ -225,6 +225,7 @@ type ValidatingTest struct { ErrorContains string ExpectAnnotations map[string]string ExpectStatusCode int32 + ExpectRejectionMetrics string ExpectReinvokeWebhooks map[string]bool } @@ -242,6 +243,7 @@ type MutatingTest struct { ErrorContains string ExpectAnnotations map[string]string ExpectStatusCode int32 + ExpectRejectionMetrics string ExpectReinvokeWebhooks map[string]bool } @@ -289,7 +291,8 @@ func ConvertToMutatingTestCases(tests []ValidatingTest, configurationName string t.ExpectAnnotations[newKey] = value delete(t.ExpectAnnotations, key) } - r[i] = MutatingTest{t.Name, ConvertToMutatingWebhooks(t.Webhooks), t.Path, t.IsCRD, t.IsDryRun, t.AdditionalLabels, t.SkipBenchmark, t.ExpectLabels, t.ExpectAllow, t.ErrorContains, t.ExpectAnnotations, t.ExpectStatusCode, t.ExpectReinvokeWebhooks} + expectedMetrics := strings.ReplaceAll(t.ExpectRejectionMetrics, `type="validating"`, `type="admit"`) + r[i] = MutatingTest{t.Name, ConvertToMutatingWebhooks(t.Webhooks), t.Path, t.IsCRD, t.IsDryRun, t.AdditionalLabels, t.SkipBenchmark, t.ExpectLabels, t.ExpectAllow, t.ErrorContains, t.ExpectAnnotations, t.ExpectStatusCode, expectedMetrics, t.ExpectReinvokeWebhooks} } return r } @@ -507,6 +510,34 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ExpectStatusCode: http.StatusInternalServerError, ExpectAllow: false, }, + { + Name: "match & invalid client config", + Webhooks: []registrationv1.ValidatingWebhook{{ + Name: "invalidClientConfig", + ClientConfig: registrationv1.WebhookClientConfig{}, + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + ExpectStatusCode: http.StatusInternalServerError, + ExpectRejectionMetrics: `apiserver_admission_webhook_rejection_count{error_type="calling_webhook_error",name="invalidClientConfig",operation="UPDATE",rejection_code="500",type="validating"} 1`, + ErrorContains: "could not get REST client", + }, + { + Name: "match & non-status error", + Webhooks: []registrationv1.ValidatingWebhook{{ + Name: "nonStatusError", + ClientConfig: ccfgSVC("nonStatusError"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + ExpectStatusCode: http.StatusInternalServerError, + ExpectRejectionMetrics: `apiserver_admission_webhook_rejection_count{error_type="calling_webhook_error",name="nonStatusError",operation="UPDATE",rejection_code="503",type="validating"} 1`, + ErrorContains: "failed to call webhook", + }, { Name: "match & allow (url)", Webhooks: []registrationv1.ValidatingWebhook{{ @@ -932,6 +963,40 @@ func NewMutatingTestCases(url *url.URL, configurationName string) []MutatingTest "mutation.webhook.admission.k8s.io/round_0_index_0": mutationAnnotationValue(configurationName, "invalidPatch", false), }, }, + { + Name: "match & invalid client config", + Webhooks: []registrationv1.MutatingWebhook{{ + Name: "invalidClientConfig", + ClientConfig: registrationv1.WebhookClientConfig{}, + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + ExpectStatusCode: http.StatusInternalServerError, + ExpectRejectionMetrics: `apiserver_admission_webhook_rejection_count{error_type="calling_webhook_error",name="invalidClientConfig",operation="UPDATE",rejection_code="500",type="admit"} 1`, + ErrorContains: "could not get REST client", + ExpectAnnotations: map[string]string{ + "mutation.webhook.admission.k8s.io/round_0_index_0": mutationAnnotationValue(configurationName, "invalidClientConfig", false), + }, + }, + { + Name: "match & non-status error", + Webhooks: []registrationv1.MutatingWebhook{{ + Name: "nonStatusError", + ClientConfig: ccfgSVC("nonStatusError"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + ExpectStatusCode: http.StatusInternalServerError, + ExpectRejectionMetrics: `apiserver_admission_webhook_rejection_count{error_type="calling_webhook_error",name="nonStatusError",operation="UPDATE",rejection_code="503",type="admit"} 1`, + ErrorContains: "failed to call webhook", + ExpectAnnotations: map[string]string{ + "mutation.webhook.admission.k8s.io/round_0_index_0": mutationAnnotationValue(configurationName, "nonStatusError", false), + }, + }, { Name: "match & remove label dry run unsupported", Webhooks: []registrationv1.MutatingWebhook{{ 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 88c29d60556..d4d232f2d00 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 @@ -157,6 +157,11 @@ func webhookHandler(w http.ResponseWriter, r *http.Request) { Patch: []byte(`[{"op": "add", "CORRUPTED_KEY":}]`), }, }) + case "/nonStatusError": + hj, _ := w.(http.Hijacker) + conn, _, _ := hj.Hijack() + defer conn.Close() //nolint:errcheck + conn.Write([]byte("bad-http")) //nolint:errcheck case "/nilResponse": w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{}) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go index 1a633d24177..77ebb50401e 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go @@ -262,7 +262,7 @@ func (d *validatingDispatcher) callHook(ctx context.Context, h *v1.ValidatingWeb // Make the webhook request client, err := invocation.Webhook.GetRESTClient(d.cm) if err != nil { - return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("could not get REST client: %w", err), Status: apierrors.NewBadRequest("error getting REST client")} + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("could not get REST client: %w", err), Status: apierrors.NewInternalError(err)} } ctx, span := tracing.Start(ctx, "Call validating webhook", attribute.String("configuration", invocation.Webhook.GetConfigurationName()), @@ -306,7 +306,7 @@ func (d *validatingDispatcher) callHook(ctx context.Context, h *v1.ValidatingWeb if se, ok := err.(*apierrors.StatusError); ok { status = se } else { - status = apierrors.NewBadRequest("error calling webhook") + status = apierrors.NewServiceUnavailable("error calling webhook") } return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("failed to call webhook: %w", err), Status: status} } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go index a7c8e1dbedc..d37fa84e3e7 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go @@ -24,12 +24,14 @@ import ( "testing" "github.com/stretchr/testify/assert" - "k8s.io/apiserver/pkg/endpoints/request" - clocktesting "k8s.io/utils/clock/testing" "k8s.io/apimachinery/pkg/api/errors" + admissionmetrics "k8s.io/apiserver/pkg/admission/metrics" webhooktesting "k8s.io/apiserver/pkg/admission/plugin/webhook/testing" auditinternal "k8s.io/apiserver/pkg/apis/audit" + "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/component-base/metrics/testutil" + clocktesting "k8s.io/utils/clock/testing" ) // BenchmarkValidate tests that ValidatingWebhook#Validate works as expected @@ -135,6 +137,10 @@ func TestValidate(t *testing.T) { continue } + if len(tt.ExpectRejectionMetrics) > 0 { + admissionmetrics.Metrics.WebhookRejectionGathererForTest().Reset() + } + attr := webhooktesting.NewAttribute(ns, nil, tt.IsDryRun) err = wh.Validate(context.TODO(), attr, objectInterfaces) if tt.ExpectAllow != (err == nil) { @@ -149,6 +155,15 @@ func TestValidate(t *testing.T) { if _, isStatusErr := err.(*errors.StatusError); err != nil && !isStatusErr { t.Errorf("%s: expected a StatusError, got %T", tt.Name, err) } + if len(tt.ExpectRejectionMetrics) > 0 { + expectedMetrics := ` +# HELP apiserver_admission_webhook_rejection_count [ALPHA] Admission webhook rejection count, identified by name and broken out for each admission type (validating or admit) and operation. Additional labels specify an error type (calling_webhook_error or apiserver_internal_error if an error occurred; no_error otherwise) and optionally a non-zero rejection code if the webhook rejects the request with an HTTP status code (honored by the apiserver when the code is greater or equal to 400). Codes greater than 600 are truncated to 600, to keep the metrics cardinality bounded. +# TYPE apiserver_admission_webhook_rejection_count counter +` + tt.ExpectRejectionMetrics + "\n" + if err := testutil.CollectAndCompare(admissionmetrics.Metrics.WebhookRejectionGathererForTest(), strings.NewReader(expectedMetrics), "apiserver_admission_webhook_rejection_count"); err != nil { + t.Errorf("unexpected collecting result:\n%s", err) + } + } fakeAttr, ok := attr.(*webhooktesting.FakeAttributes) if !ok { t.Errorf("Unexpected error, failed to convert attr to webhooktesting.FakeAttributes") diff --git a/staging/src/k8s.io/component-base/metrics/testutil/promlint.go b/staging/src/k8s.io/component-base/metrics/testutil/promlint.go index 550de013818..355e81d0e41 100644 --- a/staging/src/k8s.io/component-base/metrics/testutil/promlint.go +++ b/staging/src/k8s.io/component-base/metrics/testutil/promlint.go @@ -42,6 +42,7 @@ var exceptionMetrics = []string{ // kube-apiserver "aggregator_openapi_v2_regeneration_count", + "apiserver_admission_webhook_rejection_count", // counter metrics should have "_total" suffix,apiserver_admission_webhook_rejection_count:non-histogram and non-summary metrics should not have "_count" suffix "apiserver_admission_step_admission_duration_seconds_summary", "apiserver_current_inflight_requests", "apiserver_longrunning_gauge",