From 01b9f4b6eb819e4cd4a6d192d703961b34841f18 Mon Sep 17 00:00:00 2001 From: Igor Velichkovich Date: Thu, 13 Jul 2023 19:59:27 -0500 Subject: [PATCH] matchCondition metrics --- .../pkg/admission/metrics/metrics.go | 67 ++++++++-- .../pkg/admission/metrics/metrics_test.go | 115 ++++++++++++++++++ .../controller_reconcile.go | 2 +- .../pkg/admission/plugin/webhook/accessors.go | 15 ++- .../plugin/webhook/generic/webhook.go | 5 +- .../plugin/webhook/matchconditions/matcher.go | 12 +- .../webhook/matchconditions/matcher_test.go | 2 +- 7 files changed, 197 insertions(+), 21 deletions(-) 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 26b82c37e39..19b33a17b30 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go @@ -54,6 +54,8 @@ var ( type ObserverFunc func(ctx context.Context, elapsed time.Duration, rejected bool, attr admission.Attributes, stepType string, extraLabels ...string) const ( + kindWebhook = "webhook" + kindPolicy = "policy" stepValidate = "validate" stepAdmit = "admit" ) @@ -112,13 +114,15 @@ func (p pluginHandlerWithMetrics) Validate(ctx context.Context, a admission.Attr // AdmissionMetrics instruments admission with prometheus metrics. type AdmissionMetrics struct { - step *metricSet - controller *metricSet - webhook *metricSet - webhookRejection *metrics.CounterVec - webhookFailOpen *metrics.CounterVec - webhookRequest *metrics.CounterVec - matchConditionEvalErrors *metrics.CounterVec + step *metricSet + controller *metricSet + webhook *metricSet + webhookRejection *metrics.CounterVec + webhookFailOpen *metrics.CounterVec + webhookRequest *metrics.CounterVec + matchConditionEvalErrors *metrics.CounterVec + matchConditionExclusions *metrics.CounterVec + matchConditionEvaluationSeconds *metricSet } // newAdmissionMetrics create a new AdmissionMetrics, configured with default metric names. @@ -222,20 +226,47 @@ func newAdmissionMetrics() *AdmissionMetrics { &metrics.CounterOpts{ Namespace: namespace, Subsystem: subsystem, - Name: "admission_match_condition_evaluation_errors_total", - Help: "Admission match condition evaluation errors count, identified by name of resource containing the match condition and broken out for each admission type (validating or mutating).", + Name: "match_condition_evaluation_errors_total", + Help: "Admission match condition evaluation errors count, identified by name of resource containing the match condition and broken out for each kind containing matchConditions (webhook or policy), operation and admission type (validate or admit).", StabilityLevel: metrics.ALPHA, }, - []string{"name", "type"}) + []string{"name", "kind", "type", "operation"}) + + matchConditionExclusions := metrics.NewCounterVec( + &metrics.CounterOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "match_condition_exclusions_total", + Help: "Admission match condition evaluation exclusions count, identified by name of resource containing the match condition and broken out for each kind containing matchConditions (webhook or policy), operation and admission type (validate or admit).", + StabilityLevel: metrics.ALPHA, + }, + []string{"name", "kind", "type", "operation"}) + + matchConditionEvaluationSeconds := &metricSet{ + latencies: metrics.NewHistogramVec( + &metrics.HistogramOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "match_condition_evaluation_seconds", + Help: "Admission match condition evaluation time in seconds, identified by name and broken out for each kind containing matchConditions (webhook or policy), operation and type (validate or admit).", + Buckets: []float64{0.005, 0.025, 0.1, 0.5, 1.0, 2.5}, + StabilityLevel: metrics.ALPHA, + }, + []string{"name", "kind", "type", "operation"}, + ), + latenciesSummary: nil, + } step.mustRegister() controller.mustRegister() webhook.mustRegister() + matchConditionEvaluationSeconds.mustRegister() legacyregistry.MustRegister(webhookRejection) legacyregistry.MustRegister(webhookFailOpen) legacyregistry.MustRegister(webhookRequest) legacyregistry.MustRegister(matchConditionEvalError) - return &AdmissionMetrics{step: step, controller: controller, webhook: webhook, webhookRejection: webhookRejection, webhookFailOpen: webhookFailOpen, webhookRequest: webhookRequest, matchConditionEvalErrors: matchConditionEvalError} + legacyregistry.MustRegister(matchConditionExclusions) + return &AdmissionMetrics{step: step, controller: controller, webhook: webhook, webhookRejection: webhookRejection, webhookFailOpen: webhookFailOpen, webhookRequest: webhookRequest, matchConditionEvalErrors: matchConditionEvalError, matchConditionExclusions: matchConditionExclusions, matchConditionEvaluationSeconds: matchConditionEvaluationSeconds} } func (m *AdmissionMetrics) reset() { @@ -280,8 +311,18 @@ func (m *AdmissionMetrics) ObserveWebhookFailOpen(ctx context.Context, name, ste } // ObserveMatchConditionEvalError records validating or mutating webhook that are not called due to match conditions -func (m *AdmissionMetrics) ObserveMatchConditionEvalError(ctx context.Context, name, stepType string) { - m.matchConditionEvalErrors.WithContext(ctx).WithLabelValues(name, stepType).Inc() +func (m *AdmissionMetrics) ObserveMatchConditionEvalError(ctx context.Context, name, kind, stepType, operation string) { + m.matchConditionEvalErrors.WithContext(ctx).WithLabelValues(name, kind, stepType, operation).Inc() +} + +// ObserveMatchConditionExclusion records validating or mutating webhook that are not called due to match conditions +func (m *AdmissionMetrics) ObserveMatchConditionExclusion(ctx context.Context, name, kind, stepType, operation string) { + m.matchConditionExclusions.WithContext(ctx).WithLabelValues(name, kind, stepType, operation).Inc() +} + +// ObserveMatchConditionEvaluationTime records duration of match condition evaluation process. +func (m *AdmissionMetrics) ObserveMatchConditionEvaluationTime(ctx context.Context, elapsed time.Duration, name, kind, stepType, operation string) { + m.matchConditionEvaluationSeconds.observe(ctx, elapsed, name, kind, stepType, operation) } type metricSet struct { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics_test.go index 1026b04b39b..f4cd30cb404 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics_test.go @@ -176,6 +176,121 @@ func TestObserveWebhookFailOpen(t *testing.T) { expectCounterValue(t, "apiserver_admission_webhook_fail_open_count", wantLabelsCounterValidate, 1) } +func TestObserveMatchConditionEvalError(t *testing.T) { + defer Metrics.reset() + defer legacyregistry.Reset() + Metrics.ObserveMatchConditionEvalError(context.TODO(), "x", kindWebhook, stepAdmit, string(admission.Create)) + Metrics.ObserveMatchConditionEvalError(context.TODO(), "x", kindWebhook, stepValidate, string(admission.Create)) + Metrics.ObserveMatchConditionEvalError(context.TODO(), "x", kindPolicy, stepAdmit, string(admission.Create)) + Metrics.ObserveMatchConditionEvalError(context.TODO(), "x", kindPolicy, stepValidate, string(admission.Create)) + wantLabelsCounterPolicyAdmit := map[string]string{ + "name": "x", + "type": "admit", + "kind": "policy", + "operation": string(admission.Create), + } + wantLabelsCounterPolicyValidate := map[string]string{ + "name": "x", + "type": "validate", + "kind": "policy", + "operation": string(admission.Create), + } + wantLabelsCounterWebhookAdmit := map[string]string{ + "name": "x", + "type": "admit", + "kind": "webhook", + "operation": string(admission.Create), + } + wantLabelsCounterWebhookValidate := map[string]string{ + "name": "x", + "type": "validate", + "kind": "webhook", + "operation": string(admission.Create), + } + expectCounterValue(t, "apiserver_admission_match_condition_evaluation_errors_total", wantLabelsCounterPolicyAdmit, 1) + expectCounterValue(t, "apiserver_admission_match_condition_evaluation_errors_total", wantLabelsCounterPolicyValidate, 1) + expectCounterValue(t, "apiserver_admission_match_condition_evaluation_errors_total", wantLabelsCounterWebhookAdmit, 1) + expectCounterValue(t, "apiserver_admission_match_condition_evaluation_errors_total", wantLabelsCounterWebhookValidate, 1) +} + +func TestObserveMatchConditionExclusion(t *testing.T) { + defer Metrics.reset() + defer legacyregistry.Reset() + Metrics.ObserveMatchConditionExclusion(context.TODO(), "x", kindWebhook, stepAdmit, string(admission.Create)) + Metrics.ObserveMatchConditionExclusion(context.TODO(), "x", kindWebhook, stepValidate, string(admission.Create)) + Metrics.ObserveMatchConditionExclusion(context.TODO(), "x", kindPolicy, stepAdmit, string(admission.Create)) + Metrics.ObserveMatchConditionExclusion(context.TODO(), "x", kindPolicy, stepValidate, string(admission.Create)) + wantLabelsCounterPolicyAdmit := map[string]string{ + "name": "x", + "type": "admit", + "kind": "policy", + "operation": string(admission.Create), + } + wantLabelsCounterPolicyValidate := map[string]string{ + "name": "x", + "type": "validate", + "kind": "policy", + "operation": string(admission.Create), + } + wantLabelsCounterWebhookAdmit := map[string]string{ + "name": "x", + "type": "admit", + "kind": "webhook", + "operation": string(admission.Create), + } + wantLabelsCounterWebhookValidate := map[string]string{ + "name": "x", + "type": "validate", + "kind": "webhook", + "operation": string(admission.Create), + } + metrics, _ := legacyregistry.DefaultGatherer.Gather() + for _, mf := range metrics { + t.Logf("%v", *mf.Name) + } + expectCounterValue(t, "apiserver_admission_match_condition_exclusions_total", wantLabelsCounterPolicyAdmit, 1) + expectCounterValue(t, "apiserver_admission_match_condition_exclusions_total", wantLabelsCounterPolicyValidate, 1) + expectCounterValue(t, "apiserver_admission_match_condition_exclusions_total", wantLabelsCounterWebhookAdmit, 1) + expectCounterValue(t, "apiserver_admission_match_condition_exclusions_total", wantLabelsCounterWebhookValidate, 1) +} + +func TestObserveMatchConditionEvaluationTime(t *testing.T) { + defer Metrics.reset() + defer legacyregistry.Reset() + Metrics.ObserveMatchConditionEvaluationTime(context.TODO(), 2*time.Second, "x", kindPolicy, stepAdmit, string(admission.Create)) + Metrics.ObserveMatchConditionEvaluationTime(context.TODO(), 2*time.Second, "x", kindPolicy, stepValidate, string(admission.Create)) + Metrics.ObserveMatchConditionEvaluationTime(context.TODO(), 2*time.Second, "x", kindWebhook, stepAdmit, string(admission.Create)) + Metrics.ObserveMatchConditionEvaluationTime(context.TODO(), 2*time.Second, "x", kindWebhook, stepValidate, string(admission.Create)) + wantLabelsCounterPolicyAdmit := map[string]string{ + "name": "x", + "type": "admit", + "kind": "policy", + "operation": string(admission.Create), + } + wantLabelsCounterPolicyValidate := map[string]string{ + "name": "x", + "type": "validate", + "kind": "policy", + "operation": string(admission.Create), + } + wantLabelsCounterWebhookAdmit := map[string]string{ + "name": "x", + "type": "admit", + "kind": "webhook", + "operation": string(admission.Create), + } + wantLabelsCounterWebhookValidate := map[string]string{ + "name": "x", + "type": "validate", + "kind": "webhook", + "operation": string(admission.Create), + } + expectHistogramCountTotal(t, "apiserver_admission_match_condition_evaluation_seconds", wantLabelsCounterPolicyAdmit, 1) + expectHistogramCountTotal(t, "apiserver_admission_match_condition_evaluation_seconds", wantLabelsCounterPolicyValidate, 1) + expectHistogramCountTotal(t, "apiserver_admission_match_condition_evaluation_seconds", wantLabelsCounterWebhookAdmit, 1) + expectHistogramCountTotal(t, "apiserver_admission_match_condition_evaluation_seconds", wantLabelsCounterWebhookValidate, 1) +} + func TestWithMetrics(t *testing.T) { defer Metrics.reset() defer legacyregistry.Reset() diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go index ea987ee2652..5e0773d8bd7 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go @@ -481,7 +481,7 @@ func (c *policyController) latestPolicyData() []policyData { for i := range matchConditions { matchExpressionAccessors[i] = (*matchconditions.MatchCondition)(&matchConditions[i]) } - matcher = matchconditions.NewMatcher(filterCompiler.Compile(matchExpressionAccessors, optionalVars, environment.StoredExpressions), failurePolicy, "validatingadmissionpolicy", definitionInfo.lastReconciledValue.Name) + matcher = matchconditions.NewMatcher(filterCompiler.Compile(matchExpressionAccessors, optionalVars, environment.StoredExpressions), failurePolicy, "policy", "validate", definitionInfo.lastReconciledValue.Name) } bindingInfo.validator = c.newValidator( filterCompiler.Compile(convertv1alpha1Validations(definitionInfo.lastReconciledValue.Spec.Validations), optionalVars, environment.StoredExpressions), diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go index 88d8120f545..e60d245a621 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go @@ -80,6 +80,9 @@ type WebhookAccessor interface { GetMutatingWebhook() (*v1.MutatingWebhook, bool) // GetValidatingWebhook if the accessor contains a ValidatingWebhook, returns it and true, else returns false. GetValidatingWebhook() (*v1.ValidatingWebhook, bool) + + // GetType returns the type of the accessor (validate or admit) + GetType() string } // NewMutatingWebhookAccessor creates an accessor for a MutatingWebhook. @@ -123,6 +126,10 @@ func (m *mutatingWebhookAccessor) GetRESTClient(clientManager *webhookutil.Clien return m.client, m.clientErr } +func (m *mutatingWebhookAccessor) GetType() string { + return "admit" +} + func (m *mutatingWebhookAccessor) GetCompiledMatcher(compiler cel.FilterCompiler) matchconditions.Matcher { m.compileMatcher.Do(func() { expressions := make([]cel.ExpressionAccessor, len(m.MutatingWebhook.MatchConditions)) @@ -139,7 +146,7 @@ func (m *mutatingWebhookAccessor) GetCompiledMatcher(compiler cel.FilterCompiler HasAuthorizer: true, }, environment.StoredExpressions, - ), m.FailurePolicy, "validating", m.Name) + ), m.FailurePolicy, "webhook", "admit", m.Name) }) return m.compiledMatcher } @@ -267,7 +274,7 @@ func (v *validatingWebhookAccessor) GetCompiledMatcher(compiler cel.FilterCompil HasAuthorizer: true, }, environment.StoredExpressions, - ), v.FailurePolicy, "validating", v.Name) + ), v.FailurePolicy, "webhook", "validating", v.Name) }) return v.compiledMatcher } @@ -286,6 +293,10 @@ func (v *validatingWebhookAccessor) GetParsedObjectSelector() (labels.Selector, return v.objectSelector, v.objectSelectorErr } +func (m *validatingWebhookAccessor) GetType() string { + return "validate" +} + func (v *validatingWebhookAccessor) GetName() string { return v.Name } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook.go index f5c6eec137f..89e6cd36c04 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook.go @@ -21,6 +21,8 @@ import ( "fmt" "io" + admissionmetrics "k8s.io/apiserver/pkg/admission/metrics" + admissionv1 "k8s.io/api/admission/v1" admissionv1beta1 "k8s.io/api/admission/v1beta1" v1 "k8s.io/api/admissionregistration/v1" @@ -217,7 +219,7 @@ func (a *Webhook) ShouldCallHook(ctx context.Context, h webhook.WebhookAccessor, if matchObjErr != nil { return nil, matchObjErr } - + //TODO: maybe this sould be before the invocations are created matchConditions := h.GetMatchConditions() if len(matchConditions) > 0 { versionedAttr, err := v.VersionedAttribute(invocation.Kind) @@ -232,6 +234,7 @@ func (a *Webhook) ShouldCallHook(ctx context.Context, h webhook.WebhookAccessor, klog.Warningf("Failed evaluating match conditions, failing closed %v: %v", h.GetName(), matchResult.Error) return nil, apierrors.NewForbidden(attr.GetResource().GroupResource(), attr.GetName(), matchResult.Error) } else if !matchResult.Matches { + admissionmetrics.Metrics.ObserveMatchConditionExclusion(ctx, h.GetName(), "webhook", h.GetType(), string(attr.GetOperation())) // if no match, always skip webhook return nil, nil } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions/matcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions/matcher.go index 90a8f90b2e3..62aeadaf44f 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions/matcher.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions/matcher.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "time" "github.com/google/cel-go/cel" celtypes "github.com/google/cel-go/common/types" @@ -55,10 +56,11 @@ type matcher struct { filter celplugin.Filter failPolicy v1.FailurePolicyType matcherType string + matcherKind string objectName string } -func NewMatcher(filter celplugin.Filter, failPolicy *v1.FailurePolicyType, matcherType, objectName string) Matcher { +func NewMatcher(filter celplugin.Filter, failPolicy *v1.FailurePolicyType, matcherKind, matcherType, objectName string) Matcher { var f v1.FailurePolicyType if failPolicy == nil { f = v1.Fail @@ -68,18 +70,21 @@ func NewMatcher(filter celplugin.Filter, failPolicy *v1.FailurePolicyType, match return &matcher{ filter: filter, failPolicy: f, + matcherKind: matcherKind, matcherType: matcherType, objectName: objectName, } } func (m *matcher) Match(ctx context.Context, versionedAttr *admission.VersionedAttributes, versionedParams runtime.Object, authz authorizer.Authorizer) MatchResult { + t := time.Now() evalResults, _, err := m.filter.ForInput(ctx, versionedAttr, celplugin.CreateAdmissionRequest(versionedAttr.Attributes), celplugin.OptionalVariableBindings{ VersionedParams: versionedParams, Authorizer: authz, }, celconfig.RuntimeCELCostBudgetMatchConditions) if err != nil { + admissionmetrics.Metrics.ObserveMatchConditionEvaluationTime(ctx, time.Since(t), m.objectName, m.matcherKind, m.matcherType, string(versionedAttr.GetOperation())) // filter returning error is unexpected and not an evaluation error so not incrementing metric here if m.failPolicy == v1.Fail { return MatchResult{ @@ -104,10 +109,10 @@ func (m *matcher) Match(ctx context.Context, versionedAttr *admission.VersionedA } if evalResult.Error != nil { errorList = append(errorList, evalResult.Error) - //TODO: what's the best way to handle this metric since its reused by VAP for match conditions - admissionmetrics.Metrics.ObserveMatchConditionEvalError(ctx, m.objectName, m.matcherType) + admissionmetrics.Metrics.ObserveMatchConditionEvalError(ctx, m.objectName, m.matcherKind, m.matcherType, string(versionedAttr.GetOperation())) } if evalResult.EvalResult == celtypes.False { + admissionmetrics.Metrics.ObserveMatchConditionEvaluationTime(ctx, time.Since(t), m.objectName, m.matcherKind, m.matcherType, string(versionedAttr.GetOperation())) // If any condition false, skip calling webhook always return MatchResult{ Matches: false, @@ -116,6 +121,7 @@ func (m *matcher) Match(ctx context.Context, versionedAttr *admission.VersionedA } } if len(errorList) > 0 { + admissionmetrics.Metrics.ObserveMatchConditionEvaluationTime(ctx, time.Since(t), m.objectName, m.matcherKind, m.matcherType, string(versionedAttr.GetOperation())) // If mix of true and eval errors then resort to fail policy if m.failPolicy == v1.Fail { // mix of true and errors with fail policy fail should fail request without calling webhook diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions/matcher_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions/matcher_test.go index 30f500cf557..76ed88c3ea5 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions/matcher_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions/matcher_test.go @@ -334,7 +334,7 @@ func TestMatch(t *testing.T) { m := NewMatcher(&fakeCelFilter{ evaluations: tc.evaluations, throwError: tc.throwError, - }, tc.failPolicy, "test", "testhook") + }, tc.failPolicy, "webhook", "test", "testhook") ctx := context.TODO() matchResult := m.Match(ctx, fakeVersionedAttr, nil, nil)