github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2026-07-18 13:57:38 +00:00
Code Issues Projects Releases Wiki Activity

Merge pull request #132165 from gavinkflam/130690/fix-admission-control-response-codes

Browse Source 
bug: Fix misleading response codes in admission control metrics
This commit is contained in:
Kubernetes Prow Robot
2025-06-12 16:07:01 -07:00
committed by GitHub
parent 146196c317 9251b2a7a5
commit 060ed4e99a
8 changed files with 114 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go
View File

@@ -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()

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

@@ -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}
}

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

@@ -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")

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

@@ -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{{

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

@@ -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{})

4
staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go
View File

@@ -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}
}

19
staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go
View File

@@ -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")

1
staging/src/k8s.io/component-base/metrics/testutil/promlint.go
View File

@@ -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",