From b846c39047289e69d932ea9d5d4dadc6856ad0c7 Mon Sep 17 00:00:00 2001 From: Jiahui Feng Date: Tue, 23 Apr 2024 16:54:47 -0700 Subject: [PATCH 1/5] errors improvement. --- .../pkg/admission/plugin/cel/compile.go | 5 +- .../pkg/admission/plugin/cel/filter.go | 4 + .../plugin/policy/validating/validator.go | 8 +- .../policy/validating/validator_test.go | 1 + .../src/k8s.io/apiserver/pkg/cel/errors.go | 79 ++++++++++++++++++- .../k8s.io/apiserver/pkg/cel/errors_test.go | 63 +++++++++++++++ 6 files changed, 151 insertions(+), 9 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/cel/errors_test.go diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go index bb5e233d453..4e85e0c8927 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go @@ -163,11 +163,12 @@ type variableDeclEnvs map[OptionalVariableDeclarations]*environment.EnvSet // CompileCELExpression returns a compiled CEL expression. // perCallLimit was added for testing purpose only. Callers should always use const PerCallLimit from k8s.io/apiserver/pkg/apis/cel/config.go as input. func (c compiler) CompileCELExpression(expressionAccessor ExpressionAccessor, options OptionalVariableDeclarations, envType environment.Type) CompilationResult { - resultError := func(errorString string, errType apiservercel.ErrorType) CompilationResult { + resultError := func(errorString string, errType apiservercel.ErrorType, errors ...error) CompilationResult { return CompilationResult{ Error: &apiservercel.Error{ Type: errType, Detail: errorString, + Errors: errors, }, ExpressionAccessor: expressionAccessor, } @@ -180,7 +181,7 @@ func (c compiler) CompileCELExpression(expressionAccessor ExpressionAccessor, op ast, issues := env.Compile(expressionAccessor.GetExpression()) if issues != nil { - return resultError("compilation failed: "+issues.String(), apiservercel.ErrorTypeInvalid) + return resultError("compilation failed: "+issues.String(), apiservercel.ErrorTypeInvalid, apiservercel.NewCompilationError(issues)) } found := false returnTypes := expressionAccessor.ReturnTypes() diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go index 3e2a63e75ca..1f773bfd088 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go @@ -192,6 +192,7 @@ func (f *filter) ForInput(ctx context.Context, versionedAttr *admission.Versione evaluation.Error = &cel.Error{ Type: cel.ErrorTypeInvalid, Detail: fmt.Sprintf("compilation error: %v", compilationResult.Error), + Errors: []error{compilationResult.Error}, } continue } @@ -211,6 +212,7 @@ func (f *filter) ForInput(ctx context.Context, versionedAttr *admission.Versione return nil, -1, &cel.Error{ Type: cel.ErrorTypeInvalid, Detail: fmt.Sprintf("validation failed due to running out of cost budget, no further validation rules will be run"), + Errors: []error{cel.ErrOutOfBudget}, } } remainingBudget -= compositionCost @@ -228,12 +230,14 @@ func (f *filter) ForInput(ctx context.Context, versionedAttr *admission.Versione return nil, -1, &cel.Error{ Type: cel.ErrorTypeInvalid, Detail: fmt.Sprintf("runtime cost could not be calculated for expression: %v, no further expression will be run", compilationResult.ExpressionAccessor.GetExpression()), + Errors: []error{cel.ErrOutOfBudget}, } } else { if *rtCost > math.MaxInt64 || int64(*rtCost) > remainingBudget { return nil, -1, &cel.Error{ Type: cel.ErrorTypeInvalid, Detail: fmt.Sprintf("validation failed due to running out of cost budget, no further validation rules will be run"), + Errors: []error{cel.ErrOutOfBudget}, } } remainingBudget -= int64(*rtCost) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator.go index da2ea1c10f8..c429ae22fcf 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator.go @@ -18,6 +18,7 @@ package validating import ( "context" + "errors" "fmt" "strings" @@ -132,19 +133,14 @@ func (v *validator) Validate(ctx context.Context, matchedResource schema.GroupVe } var messageResult *cel.EvaluationResult - var messageError *apiservercel.Error if len(messageResults) > i { messageResult = &messageResults[i] } - messageError, _ = err.(*apiservercel.Error) if evalResult.Error != nil { decision.Action = policyDecisionActionForError(f) decision.Evaluation = EvalError decision.Message = evalResult.Error.Error() - } else if messageError != nil && - (messageError.Type == apiservercel.ErrorTypeInternal || - (messageError.Type == apiservercel.ErrorTypeInvalid && - strings.HasPrefix(messageError.Detail, "validation failed due to running out of cost budget"))) { + } else if errors.Is(err, apiservercel.ErrInternal) || errors.Is(err, apiservercel.ErrOutOfBudget) { decision.Action = policyDecisionActionForError(f) decision.Evaluation = EvalError decision.Message = fmt.Sprintf("failed messageExpression: %s", err) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go index 99b3556b544..3d6680ccca9 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go @@ -53,6 +53,7 @@ func (f *fakeCelFilter) ForInput(ctx context.Context, versionedAttr *admission.V return nil, -1, &apiservercel.Error{ Type: apiservercel.ErrorTypeInvalid, Detail: fmt.Sprintf("validation failed due to running out of cost budget, no further validation rules will be run"), + Errors: []error{apiservercel.ErrOutOfBudget}, } } if f.throwError { diff --git a/staging/src/k8s.io/apiserver/pkg/cel/errors.go b/staging/src/k8s.io/apiserver/pkg/cel/errors.go index 907ca6ec8f1..bd3c0773167 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/errors.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/errors.go @@ -16,11 +16,46 @@ limitations under the License. package cel +import ( + "fmt" + + "github.com/google/cel-go/cel" +) + +// ErrInternal the basic error that occurs when the expression fails to evaluate +// due to internal reasons. Any Error that has the Type of +// ErrorInternal is considered equal to ErrInternal +var ErrInternal = fmt.Errorf("internal") + +// ErrInvalid is the basic error that occurs when the expression fails to +// evaluate but not due to internal reasons. Any Error that has the Type of +// ErrorInvalid is considered equal to ErrInvalid. +var ErrInvalid = fmt.Errorf("invalid") + +// ErrRequired is the basic error that occurs when the expression is required +// but absent. +// Any Error that has the Type of ErrorRequired is considered equal +// to ErrRequired. +var ErrRequired = fmt.Errorf("required") + +// ErrCompilation is the basic error that occurs when the expression fails to +// compile. Any CompilationError wraps ErrCompilation. +// ErrCompilation wraps ErrInvalid +var ErrCompilation = fmt.Errorf("%w: compilation error", ErrInvalid) + +// ErrOutOfBudget is the basic error that occurs when the expression fails due to +// exceeding budget. +var ErrOutOfBudget = fmt.Errorf("out of budget") + // Error is an implementation of the 'error' interface, which represents a // XValidation error. type Error struct { Type ErrorType Detail string + + // Errors are optional wrapped errors that can be useful to + // programmatically retrieve detailed errors. + Errors []error } var _ error = &Error{} @@ -30,7 +65,24 @@ func (v *Error) Error() string { return v.Detail } -// ErrorType is a machine readable value providing more detail about why +func (v *Error) Is(err error) bool { + switch v.Type { + case ErrorTypeRequired: + return err == ErrRequired + case ErrorTypeInvalid: + return err == ErrInvalid + case ErrorTypeInternal: + return err == ErrInternal + } + return false +} + +// Unwrap returns wrapped errors. +func (v *Error) Unwrap() []error { + return v.Errors +} + +// ErrorType is a machine-readable value providing more detail about why // a XValidation is invalid. type ErrorType string @@ -45,3 +97,28 @@ const ( // to user input. See InternalError(). ErrorTypeInternal ErrorType = "InternalError" ) + +// CompilationError indicates an error during expression compilation. +// It wraps ErrCompilation. +type CompilationError struct { + err *Error + Issues *cel.Issues +} + +// NewCompilationError wraps a cel.Issues to indicate a compilation failure. +func NewCompilationError(issues *cel.Issues) *CompilationError { + return &CompilationError{ + Issues: issues, + err: &Error{ + Type: ErrorTypeInvalid, + Detail: fmt.Sprintf("compilation error: %s", issues), + }} +} + +func (e *CompilationError) Error() string { + return e.err.Error() +} + +func (e *CompilationError) Unwrap() []error { + return []error{e.err, ErrCompilation} +} diff --git a/staging/src/k8s.io/apiserver/pkg/cel/errors_test.go b/staging/src/k8s.io/apiserver/pkg/cel/errors_test.go new file mode 100644 index 00000000000..40a0d6a9a1d --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/cel/errors_test.go @@ -0,0 +1,63 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cel + +import ( + "errors" + "testing" + + "github.com/google/cel-go/cel" +) + +func TestOutOfBudgetError(t *testing.T) { + err := &Error{ + Type: ErrorTypeInvalid, + Detail: "expression out of budget", + Errors: []error{ErrOutOfBudget}, + } + if !errors.Is(err, ErrOutOfBudget) { + t.Errorf("unexpected %v is not %v", err, ErrOutOfBudget) + } + if !errors.Is(err, ErrInvalid) { + t.Errorf("unexpected %v is not %v", err, ErrInvalid) + } +} + +func TestCompilationError(t *testing.T) { + if !errors.Is(ErrCompilation, ErrInvalid) { + t.Errorf("unexpected %v is not %v", ErrCompilation, ErrInvalid) + } + issues := &cel.Issues{} + err := &Error{ + Type: ErrorTypeInvalid, + Detail: "fake compilation failed", + Errors: []error{NewCompilationError(issues)}, + } + if !errors.Is(err, ErrCompilation) { + t.Errorf("unexpected %v is not %v", err, ErrCompilation) + } + if !errors.Is(err, ErrInvalid) { + t.Errorf("unexpected %v is not %v", err, ErrInvalid) + } + var compilationErr *CompilationError + if errors.As(err, &compilationErr); compilationErr == nil { + t.Errorf("unexpected %v cannot be fitted into CompilationError", err) + } + if compilationErr.Issues != issues { + t.Errorf("retrieved issues is not the original") + } +} From ce45a82346623d19168b0b85cbba5ba4ff164417 Mon Sep 17 00:00:00 2001 From: Jiahui Feng Date: Thu, 25 Apr 2024 17:59:32 -0700 Subject: [PATCH 2/5] make Err wrap one or zero error. --- .../apiserver/pkg/admission/plugin/cel/compile.go | 12 ++++++------ .../apiserver/pkg/admission/plugin/cel/filter.go | 8 ++++---- .../plugin/policy/validating/validator_test.go | 2 +- staging/src/k8s.io/apiserver/pkg/cel/errors.go | 10 +++++----- staging/src/k8s.io/apiserver/pkg/cel/errors_test.go | 4 ++-- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go index 4e85e0c8927..06035f6b9e9 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go @@ -163,12 +163,12 @@ type variableDeclEnvs map[OptionalVariableDeclarations]*environment.EnvSet // CompileCELExpression returns a compiled CEL expression. // perCallLimit was added for testing purpose only. Callers should always use const PerCallLimit from k8s.io/apiserver/pkg/apis/cel/config.go as input. func (c compiler) CompileCELExpression(expressionAccessor ExpressionAccessor, options OptionalVariableDeclarations, envType environment.Type) CompilationResult { - resultError := func(errorString string, errType apiservercel.ErrorType, errors ...error) CompilationResult { + resultError := func(errorString string, errType apiservercel.ErrorType, cause error) CompilationResult { return CompilationResult{ Error: &apiservercel.Error{ Type: errType, Detail: errorString, - Errors: errors, + Cause: cause, }, ExpressionAccessor: expressionAccessor, } @@ -176,7 +176,7 @@ func (c compiler) CompileCELExpression(expressionAccessor ExpressionAccessor, op env, err := c.varEnvs[options].Env(envType) if err != nil { - return resultError(fmt.Sprintf("unexpected error loading CEL environment: %v", err), apiservercel.ErrorTypeInternal) + return resultError(fmt.Sprintf("unexpected error loading CEL environment: %v", err), apiservercel.ErrorTypeInternal, nil) } ast, issues := env.Compile(expressionAccessor.GetExpression()) @@ -199,19 +199,19 @@ func (c compiler) CompileCELExpression(expressionAccessor ExpressionAccessor, op reason = fmt.Sprintf("must evaluate to one of %v", returnTypes) } - return resultError(reason, apiservercel.ErrorTypeInvalid) + return resultError(reason, apiservercel.ErrorTypeInvalid, nil) } _, err = cel.AstToCheckedExpr(ast) if err != nil { // should be impossible since env.Compile returned no issues - return resultError("unexpected compilation error: "+err.Error(), apiservercel.ErrorTypeInternal) + return resultError("unexpected compilation error: "+err.Error(), apiservercel.ErrorTypeInternal, nil) } prog, err := env.Program(ast, cel.InterruptCheckFrequency(celconfig.CheckFrequency), ) if err != nil { - return resultError("program instantiation failed: "+err.Error(), apiservercel.ErrorTypeInternal) + return resultError("program instantiation failed: "+err.Error(), apiservercel.ErrorTypeInternal, nil) } return CompilationResult{ Program: prog, diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go index 1f773bfd088..216a474d23e 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go @@ -192,7 +192,7 @@ func (f *filter) ForInput(ctx context.Context, versionedAttr *admission.Versione evaluation.Error = &cel.Error{ Type: cel.ErrorTypeInvalid, Detail: fmt.Sprintf("compilation error: %v", compilationResult.Error), - Errors: []error{compilationResult.Error}, + Cause: compilationResult.Error, } continue } @@ -212,7 +212,7 @@ func (f *filter) ForInput(ctx context.Context, versionedAttr *admission.Versione return nil, -1, &cel.Error{ Type: cel.ErrorTypeInvalid, Detail: fmt.Sprintf("validation failed due to running out of cost budget, no further validation rules will be run"), - Errors: []error{cel.ErrOutOfBudget}, + Cause: cel.ErrOutOfBudget, } } remainingBudget -= compositionCost @@ -230,14 +230,14 @@ func (f *filter) ForInput(ctx context.Context, versionedAttr *admission.Versione return nil, -1, &cel.Error{ Type: cel.ErrorTypeInvalid, Detail: fmt.Sprintf("runtime cost could not be calculated for expression: %v, no further expression will be run", compilationResult.ExpressionAccessor.GetExpression()), - Errors: []error{cel.ErrOutOfBudget}, + Cause: cel.ErrOutOfBudget, } } else { if *rtCost > math.MaxInt64 || int64(*rtCost) > remainingBudget { return nil, -1, &cel.Error{ Type: cel.ErrorTypeInvalid, Detail: fmt.Sprintf("validation failed due to running out of cost budget, no further validation rules will be run"), - Errors: []error{cel.ErrOutOfBudget}, + Cause: cel.ErrOutOfBudget, } } remainingBudget -= int64(*rtCost) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go index 3d6680ccca9..44d79e60f88 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go @@ -53,7 +53,7 @@ func (f *fakeCelFilter) ForInput(ctx context.Context, versionedAttr *admission.V return nil, -1, &apiservercel.Error{ Type: apiservercel.ErrorTypeInvalid, Detail: fmt.Sprintf("validation failed due to running out of cost budget, no further validation rules will be run"), - Errors: []error{apiservercel.ErrOutOfBudget}, + Cause: apiservercel.ErrOutOfBudget, } } if f.throwError { diff --git a/staging/src/k8s.io/apiserver/pkg/cel/errors.go b/staging/src/k8s.io/apiserver/pkg/cel/errors.go index bd3c0773167..d7b052fc9e7 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/errors.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/errors.go @@ -53,9 +53,9 @@ type Error struct { Type ErrorType Detail string - // Errors are optional wrapped errors that can be useful to + // Cause is an optional wrapped errors that can be useful to // programmatically retrieve detailed errors. - Errors []error + Cause error } var _ error = &Error{} @@ -77,9 +77,9 @@ func (v *Error) Is(err error) bool { return false } -// Unwrap returns wrapped errors. -func (v *Error) Unwrap() []error { - return v.Errors +// Unwrap returns the wrapped Cause. +func (v *Error) Unwrap() error { + return v.Cause } // ErrorType is a machine-readable value providing more detail about why diff --git a/staging/src/k8s.io/apiserver/pkg/cel/errors_test.go b/staging/src/k8s.io/apiserver/pkg/cel/errors_test.go index 40a0d6a9a1d..2822aaab714 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/errors_test.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/errors_test.go @@ -27,7 +27,7 @@ func TestOutOfBudgetError(t *testing.T) { err := &Error{ Type: ErrorTypeInvalid, Detail: "expression out of budget", - Errors: []error{ErrOutOfBudget}, + Cause: ErrOutOfBudget, } if !errors.Is(err, ErrOutOfBudget) { t.Errorf("unexpected %v is not %v", err, ErrOutOfBudget) @@ -45,7 +45,7 @@ func TestCompilationError(t *testing.T) { err := &Error{ Type: ErrorTypeInvalid, Detail: "fake compilation failed", - Errors: []error{NewCompilationError(issues)}, + Cause: NewCompilationError(issues), } if !errors.Is(err, ErrCompilation) { t.Errorf("unexpected %v is not %v", err, ErrCompilation) From 8e9232ef46d5b08ab4f95ad6c1e93671ef1bd5ba Mon Sep 17 00:00:00 2001 From: Jiahui Feng Date: Thu, 25 Apr 2024 18:30:26 -0700 Subject: [PATCH 3/5] remove unused policy_definition_total metric and state label --- .../plugin/policy/validating/dispatcher.go | 10 ++-- .../policy/validating/metrics/metrics.go | 55 ++++++----------- .../policy/validating/metrics/metrics_test.go | 59 ++++++++----------- 3 files changed, 48 insertions(+), 76 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go index edf5a788673..ad6530ffb7f 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go @@ -223,7 +223,7 @@ func (c *dispatcher) Dispatch(ctx context.Context, a admission.Attributes, o adm switch decision.Action { case ActionAdmit: if decision.Evaluation == EvalError { - celmetrics.Metrics.ObserveAdmissionWithError(ctx, decision.Elapsed, definition.Name, binding.Name, "active") + celmetrics.Metrics.ObserveAdmissionWithError(ctx, decision.Elapsed, definition.Name, binding.Name) } case ActionDeny: for _, action := range binding.Spec.ValidationActions { @@ -234,13 +234,13 @@ func (c *dispatcher) Dispatch(ctx context.Context, a admission.Attributes, o adm Binding: binding, PolicyDecision: decision, }) - celmetrics.Metrics.ObserveRejection(ctx, decision.Elapsed, definition.Name, binding.Name, "active") + celmetrics.Metrics.ObserveRejection(ctx, decision.Elapsed, definition.Name, binding.Name) case admissionregistrationv1.Audit: publishValidationFailureAnnotation(binding, i, decision, versionedAttr) - celmetrics.Metrics.ObserveAudit(ctx, decision.Elapsed, definition.Name, binding.Name, "active") + celmetrics.Metrics.ObserveAudit(ctx, decision.Elapsed, definition.Name, binding.Name) case admissionregistrationv1.Warn: warning.AddWarning(ctx, "", fmt.Sprintf("Validation failed for ValidatingAdmissionPolicy '%s' with binding '%s': %s", definition.Name, binding.Name, decision.Message)) - celmetrics.Metrics.ObserveWarn(ctx, decision.Elapsed, definition.Name, binding.Name, "active") + celmetrics.Metrics.ObserveWarn(ctx, decision.Elapsed, definition.Name, binding.Name) } } default: @@ -269,7 +269,7 @@ func (c *dispatcher) Dispatch(ctx context.Context, a admission.Attributes, o adm Elapsed: auditAnnotation.Elapsed, }, }) - celmetrics.Metrics.ObserveRejection(ctx, auditAnnotation.Elapsed, definition.Name, binding.Name, "active") + celmetrics.Metrics.ObserveRejection(ctx, auditAnnotation.Elapsed, definition.Name, binding.Name) case AuditAnnotationActionExclude: // skip it default: return fmt.Errorf("unsupported AuditAnnotation Action: %s", auditAnnotation.Action) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go index 9f8a941105a..d1707eb60d2 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go @@ -36,9 +36,8 @@ var ( // ValidatingAdmissionPolicyMetrics aggregates Prometheus metrics related to validation admission control. type ValidatingAdmissionPolicyMetrics struct { - policyCheck *metrics.CounterVec - policyDefinition *metrics.CounterVec - policyLatency *metrics.HistogramVec + policyCheck *metrics.CounterVec + policyLatency *metrics.HistogramVec } func newValidationAdmissionMetrics() *ValidatingAdmissionPolicyMetrics { @@ -47,25 +46,16 @@ func newValidationAdmissionMetrics() *ValidatingAdmissionPolicyMetrics { Namespace: metricsNamespace, Subsystem: metricsSubsystem, Name: "check_total", - Help: "Validation admission policy check total, labeled by policy and further identified by binding, enforcement action taken, and state.", + Help: "Validation admission policy check total, labeled by policy and further identified by binding and enforcement action taken.", StabilityLevel: metrics.ALPHA, }, - []string{"policy", "policy_binding", "enforcement_action", "state"}, - ) - definition := metrics.NewCounterVec(&metrics.CounterOpts{ - Namespace: metricsNamespace, - Subsystem: metricsSubsystem, - Name: "definition_total", - Help: "Validation admission policy count total, labeled by state and enforcement action.", - StabilityLevel: metrics.ALPHA, - }, - []string{"state", "enforcement_action"}, + []string{"policy", "policy_binding", "enforcement_action"}, ) latency := metrics.NewHistogramVec(&metrics.HistogramOpts{ Namespace: metricsNamespace, Subsystem: metricsSubsystem, Name: "check_duration_seconds", - Help: "Validation admission latency for individual validation expressions in seconds, labeled by policy and further including binding, state and enforcement action taken.", + Help: "Validation admission latency for individual validation expressions in seconds, labeled by policy and further including binding and enforcement action taken.", // the bucket distribution here is based oo the benchmark suite at // github.com/DangerOnTheRanger/cel-benchmark performed on 16-core Intel Xeon // the lowest bucket was based around the 180ns/op figure for BenchmarkAccess, @@ -77,47 +67,40 @@ func newValidationAdmissionMetrics() *ValidatingAdmissionPolicyMetrics { Buckets: []float64{0.0000005, 0.001, 0.01, 0.1, 1.0}, StabilityLevel: metrics.ALPHA, }, - []string{"policy", "policy_binding", "enforcement_action", "state"}, + []string{"policy", "policy_binding", "enforcement_action"}, ) legacyregistry.MustRegister(check) - legacyregistry.MustRegister(definition) legacyregistry.MustRegister(latency) - return &ValidatingAdmissionPolicyMetrics{policyCheck: check, policyDefinition: definition, policyLatency: latency} + return &ValidatingAdmissionPolicyMetrics{policyCheck: check, policyLatency: latency} } // Reset resets all validation admission-related Prometheus metrics. func (m *ValidatingAdmissionPolicyMetrics) Reset() { m.policyCheck.Reset() - m.policyDefinition.Reset() m.policyLatency.Reset() } -// ObserveDefinition observes a policy definition. -func (m *ValidatingAdmissionPolicyMetrics) ObserveDefinition(ctx context.Context, state, enforcementAction string) { - m.policyDefinition.WithContext(ctx).WithLabelValues(state, enforcementAction).Inc() -} - // ObserveAdmissionWithError observes a policy validation error that was ignored due to failure policy. -func (m *ValidatingAdmissionPolicyMetrics) ObserveAdmissionWithError(ctx context.Context, elapsed time.Duration, policy, binding, state string) { - m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "allow", state).Inc() - m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "allow", state).Observe(elapsed.Seconds()) +func (m *ValidatingAdmissionPolicyMetrics) ObserveAdmissionWithError(ctx context.Context, elapsed time.Duration, policy, binding string) { + m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "allow").Inc() + m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "allow").Observe(elapsed.Seconds()) } // ObserveRejection observes a policy validation error that was at least one of the reasons for a deny. -func (m *ValidatingAdmissionPolicyMetrics) ObserveRejection(ctx context.Context, elapsed time.Duration, policy, binding, state string) { - m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "deny", state).Inc() - m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "deny", state).Observe(elapsed.Seconds()) +func (m *ValidatingAdmissionPolicyMetrics) ObserveRejection(ctx context.Context, elapsed time.Duration, policy, binding string) { + m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "deny").Inc() + m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "deny").Observe(elapsed.Seconds()) } // ObserveAudit observes a policy validation audit annotation was published for a validation failure. -func (m *ValidatingAdmissionPolicyMetrics) ObserveAudit(ctx context.Context, elapsed time.Duration, policy, binding, state string) { - m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "audit", state).Inc() - m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "audit", state).Observe(elapsed.Seconds()) +func (m *ValidatingAdmissionPolicyMetrics) ObserveAudit(ctx context.Context, elapsed time.Duration, policy, binding string) { + m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "audit").Inc() + m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "audit").Observe(elapsed.Seconds()) } // ObserveWarn observes a policy validation warning was published for a validation failure. -func (m *ValidatingAdmissionPolicyMetrics) ObserveWarn(ctx context.Context, elapsed time.Duration, policy, binding, state string) { - m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "warn", state).Inc() - m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "warn", state).Observe(elapsed.Seconds()) +func (m *ValidatingAdmissionPolicyMetrics) ObserveWarn(ctx context.Context, elapsed time.Duration, policy, binding string) { + m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "warn").Inc() + m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "warn").Observe(elapsed.Seconds()) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go index 7c9e3e75998..682cf691638 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go @@ -31,7 +31,6 @@ type metricsObserver func() func TestNoUtils(t *testing.T) { metrics := []string{ - "apiserver_validating_admission_policy_definition_total", "apiserver_validating_admission_policy_check_total", "apiserver_validating_admission_policy_check_duration_seconds", } @@ -44,53 +43,43 @@ func TestNoUtils(t *testing.T) { { desc: "observe policy admission", want: ` - # HELP apiserver_validating_admission_policy_check_duration_seconds [ALPHA] Validation admission latency for individual validation expressions in seconds, labeled by policy and further including binding, state and enforcement action taken. + # HELP apiserver_validating_admission_policy_check_duration_seconds [ALPHA] Validation admission latency for individual validation expressions in seconds, labeled by policy and further including binding and enforcement action taken. # TYPE apiserver_validating_admission_policy_check_duration_seconds histogram - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="0.0000005"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="0.001"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="0.01"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="0.1"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="1"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="+Inf"} 1 - apiserver_validating_admission_policy_check_duration_seconds_sum{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",state="active"} 10 - apiserver_validating_admission_policy_check_duration_seconds_count{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",state="active"} 1 - # HELP apiserver_validating_admission_policy_check_total [ALPHA] Validation admission policy check total, labeled by policy and further identified by binding, enforcement action taken, and state. + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="0.0000005"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="0.001"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="0.01"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="0.1"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="1"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="+Inf"} 1 + apiserver_validating_admission_policy_check_duration_seconds_sum{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com"} 10 + apiserver_validating_admission_policy_check_duration_seconds_count{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com"} 1 + # HELP apiserver_validating_admission_policy_check_total [ALPHA] Validation admission policy check total, labeled by policy and further identified by binding and enforcement action taken. # TYPE apiserver_validating_admission_policy_check_total counter - apiserver_validating_admission_policy_check_total{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",state="active"} 1 + apiserver_validating_admission_policy_check_total{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com"} 1 `, observer: func() { - Metrics.ObserveAdmissionWithError(context.TODO(), time.Duration(10)*time.Second, "policy.example.com", "binding.example.com", "active") + Metrics.ObserveAdmissionWithError(context.TODO(), time.Duration(10)*time.Second, "policy.example.com", "binding.example.com") }, }, { desc: "observe policy rejection", want: ` - # HELP apiserver_validating_admission_policy_check_duration_seconds [ALPHA] Validation admission latency for individual validation expressions in seconds, labeled by policy and further including binding, state and enforcement action taken. + # HELP apiserver_validating_admission_policy_check_duration_seconds [ALPHA] Validation admission latency for individual validation expressions in seconds, labeled by policy and further including binding and enforcement action taken. # TYPE apiserver_validating_admission_policy_check_duration_seconds histogram - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="0.0000005"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="0.001"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="0.01"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="0.1"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="1"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",state="active",le="+Inf"} 1 - apiserver_validating_admission_policy_check_duration_seconds_sum{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",state="active"} 10 - apiserver_validating_admission_policy_check_duration_seconds_count{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",state="active"} 1 - # HELP apiserver_validating_admission_policy_check_total [ALPHA] Validation admission policy check total, labeled by policy and further identified by binding, enforcement action taken, and state. + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="0.0000005"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="0.001"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="0.01"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="0.1"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="1"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="+Inf"} 1 + apiserver_validating_admission_policy_check_duration_seconds_sum{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com"} 10 + apiserver_validating_admission_policy_check_duration_seconds_count{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com"} 1 + # HELP apiserver_validating_admission_policy_check_total [ALPHA] Validation admission policy check total, labeled by policy and further identified by binding and enforcement action taken. # TYPE apiserver_validating_admission_policy_check_total counter - apiserver_validating_admission_policy_check_total{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",state="active"} 1 + apiserver_validating_admission_policy_check_total{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com"} 1 `, observer: func() { - Metrics.ObserveRejection(context.TODO(), time.Duration(10)*time.Second, "policy.example.com", "binding.example.com", "active") - }, - }, - { - desc: "observe policy definition", - want: `# HELP apiserver_validating_admission_policy_definition_total [ALPHA] Validation admission policy count total, labeled by state and enforcement action. - # TYPE apiserver_validating_admission_policy_definition_total counter - apiserver_validating_admission_policy_definition_total{enforcement_action="deny",state="active"} 1 - `, - observer: func() { - Metrics.ObserveDefinition(context.TODO(), "active", "deny") + Metrics.ObserveRejection(context.TODO(), time.Duration(10)*time.Second, "policy.example.com", "binding.example.com") }, }, } From d61edc51b84774c158b3866ab9a0678d4ddaba96 Mon Sep 17 00:00:00 2001 From: Jiahui Feng Date: Fri, 26 Apr 2024 11:49:44 -0700 Subject: [PATCH 4/5] make use of new error reporting in the dispatcher. --- .../plugin/policy/validating/dispatcher.go | 2 +- .../plugin/policy/validating/errors.go | 38 +++++++++++++++++++ .../policy/validating/metrics/errors.go | 38 +++++++++++++++++++ .../policy/validating/metrics/metrics.go | 20 +++++++++- .../policy/validating/metrics/metrics_test.go | 2 +- 5 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/errors.go create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/errors.go diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go index ad6530ffb7f..7b2349ddff6 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go @@ -223,7 +223,7 @@ func (c *dispatcher) Dispatch(ctx context.Context, a admission.Attributes, o adm switch decision.Action { case ActionAdmit: if decision.Evaluation == EvalError { - celmetrics.Metrics.ObserveAdmissionWithError(ctx, decision.Elapsed, definition.Name, binding.Name) + celmetrics.Metrics.ObserveAdmission(ctx, decision.Elapsed, definition.Name, binding.Name, ErrorType(&decision)) } case ActionDeny: for _, action := range binding.Spec.ValidationActions { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/errors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/errors.go new file mode 100644 index 00000000000..c8380baa9bf --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/errors.go @@ -0,0 +1,38 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validating + +import ( + "strings" + + celmetrics "k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics" +) + +// ErrorType decodes the error to determine the error type +// that the metrics understand. +func ErrorType(decision *PolicyDecision) celmetrics.ValidationErrorType { + if decision.Evaluation == EvalAdmit { + return celmetrics.ValidationNoError + } + if strings.HasPrefix(decision.Message, "compilation") { + return celmetrics.ValidationCompileError + } + if strings.HasPrefix(decision.Message, "validation failed due to running out of cost budget") { + return celmetrics.ValidatingOutOfBudget + } + return celmetrics.ValidatingInternalError +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/errors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/errors.go new file mode 100644 index 00000000000..e6dac566929 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/errors.go @@ -0,0 +1,38 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cel + +import ( + "errors" + + apiservercel "k8s.io/apiserver/pkg/cel" +) + +// ErrorType decodes the error to determine the error type +// that the metrics understand. +func ErrorType(err error) ValidationErrorType { + if err == nil { + return ValidationNoError + } + if errors.Is(err, apiservercel.ErrCompilation) { + return ValidationCompileError + } + if errors.Is(err, apiservercel.ErrOutOfBudget) { + return ValidatingOutOfBudget + } + return ValidatingInternalError +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go index d1707eb60d2..3be9afa8e65 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go @@ -29,6 +29,22 @@ const ( metricsSubsystem = "validating_admission_policy" ) +// ValidationErrorType defines different error types that happen to a validation expression +type ValidationErrorType string + +const ( + // ValidationCompileError indicates that the expression fails to compile. + ValidationCompileError ValidationErrorType = "compile_error" + // ValidatingInternalError indicates that the expression fails due to internal + // errors that are out of the control of the user. + ValidatingInternalError ValidationErrorType = "internal_error" + // ValidatingOutOfBudget indicates that the expression fails due to running + // out of cost budget, or the budget cannot be obtained. + ValidatingOutOfBudget ValidationErrorType = "out_of_budget" + // ValidationNoError indicates that the expression returns without an error. + ValidationNoError ValidationErrorType = "no_error" +) + var ( // Metrics provides access to validation admission metrics. Metrics = newValidationAdmissionMetrics() @@ -81,8 +97,8 @@ func (m *ValidatingAdmissionPolicyMetrics) Reset() { m.policyLatency.Reset() } -// ObserveAdmissionWithError observes a policy validation error that was ignored due to failure policy. -func (m *ValidatingAdmissionPolicyMetrics) ObserveAdmissionWithError(ctx context.Context, elapsed time.Duration, policy, binding string) { +// ObserveAdmission observes a policy validation, with an optional error to indicate the error that may occur but ignored. +func (m *ValidatingAdmissionPolicyMetrics) ObserveAdmission(ctx context.Context, elapsed time.Duration, policy, binding string, errorType ValidationErrorType) { m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "allow").Inc() m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "allow").Observe(elapsed.Seconds()) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go index 682cf691638..e575e0faad0 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go @@ -58,7 +58,7 @@ func TestNoUtils(t *testing.T) { apiserver_validating_admission_policy_check_total{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com"} 1 `, observer: func() { - Metrics.ObserveAdmissionWithError(context.TODO(), time.Duration(10)*time.Second, "policy.example.com", "binding.example.com") + Metrics.ObserveAdmission(context.TODO(), time.Duration(10)*time.Second, "policy.example.com", "binding.example.com") }, }, { From b7821078b36f1cb25d903774ddf37a97966c2eac Mon Sep 17 00:00:00 2001 From: Cici Huang Date: Tue, 16 Jul 2024 08:27:36 -0700 Subject: [PATCH 5/5] Fix the error type, Add into observation, Fix tests. --- .../plugin/policy/validating/dispatcher.go | 13 +++--- .../plugin/policy/validating/errors.go | 2 +- .../policy/validating/metrics/errors.go | 2 +- .../policy/validating/metrics/metrics.go | 30 +++++++------- .../policy/validating/metrics/metrics_test.go | 40 +++++++++---------- 5 files changed, 44 insertions(+), 43 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go index 7b2349ddff6..f0601142530 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/dispatcher.go @@ -234,13 +234,13 @@ func (c *dispatcher) Dispatch(ctx context.Context, a admission.Attributes, o adm Binding: binding, PolicyDecision: decision, }) - celmetrics.Metrics.ObserveRejection(ctx, decision.Elapsed, definition.Name, binding.Name) + celmetrics.Metrics.ObserveRejection(ctx, decision.Elapsed, definition.Name, binding.Name, ErrorType(&decision)) case admissionregistrationv1.Audit: publishValidationFailureAnnotation(binding, i, decision, versionedAttr) - celmetrics.Metrics.ObserveAudit(ctx, decision.Elapsed, definition.Name, binding.Name) + celmetrics.Metrics.ObserveAudit(ctx, decision.Elapsed, definition.Name, binding.Name, ErrorType(&decision)) case admissionregistrationv1.Warn: warning.AddWarning(ctx, "", fmt.Sprintf("Validation failed for ValidatingAdmissionPolicy '%s' with binding '%s': %s", definition.Name, binding.Name, decision.Message)) - celmetrics.Metrics.ObserveWarn(ctx, decision.Elapsed, definition.Name, binding.Name) + celmetrics.Metrics.ObserveWarn(ctx, decision.Elapsed, definition.Name, binding.Name, ErrorType(&decision)) } } default: @@ -259,7 +259,7 @@ func (c *dispatcher) Dispatch(ctx context.Context, a admission.Attributes, o adm auditAnnotationCollector.add(auditAnnotation.Key, value) case AuditAnnotationActionError: // When failurePolicy=fail, audit annotation errors result in deny - deniedDecisions = append(deniedDecisions, policyDecisionWithMetadata{ + d := policyDecisionWithMetadata{ Definition: definition, Binding: binding, PolicyDecision: PolicyDecision{ @@ -268,8 +268,9 @@ func (c *dispatcher) Dispatch(ctx context.Context, a admission.Attributes, o adm Message: auditAnnotation.Error, Elapsed: auditAnnotation.Elapsed, }, - }) - celmetrics.Metrics.ObserveRejection(ctx, auditAnnotation.Elapsed, definition.Name, binding.Name) + } + deniedDecisions = append(deniedDecisions, d) + celmetrics.Metrics.ObserveRejection(ctx, auditAnnotation.Elapsed, definition.Name, binding.Name, ErrorType(&d.PolicyDecision)) case AuditAnnotationActionExclude: // skip it default: return fmt.Errorf("unsupported AuditAnnotation Action: %s", auditAnnotation.Action) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/errors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/errors.go index c8380baa9bf..0fcf40ff0d4 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/errors.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/errors.go @@ -34,5 +34,5 @@ func ErrorType(decision *PolicyDecision) celmetrics.ValidationErrorType { if strings.HasPrefix(decision.Message, "validation failed due to running out of cost budget") { return celmetrics.ValidatingOutOfBudget } - return celmetrics.ValidatingInternalError + return celmetrics.ValidatingInvalidError } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/errors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/errors.go index e6dac566929..4327252610a 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/errors.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/errors.go @@ -34,5 +34,5 @@ func ErrorType(err error) ValidationErrorType { if errors.Is(err, apiservercel.ErrOutOfBudget) { return ValidatingOutOfBudget } - return ValidatingInternalError + return ValidatingInvalidError } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go index 3be9afa8e65..15e6fbe5421 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics.go @@ -35,9 +35,9 @@ type ValidationErrorType string const ( // ValidationCompileError indicates that the expression fails to compile. ValidationCompileError ValidationErrorType = "compile_error" - // ValidatingInternalError indicates that the expression fails due to internal + // ValidatingInvalidError indicates that the expression fails due to internal // errors that are out of the control of the user. - ValidatingInternalError ValidationErrorType = "internal_error" + ValidatingInvalidError ValidationErrorType = "invalid_error" // ValidatingOutOfBudget indicates that the expression fails due to running // out of cost budget, or the budget cannot be obtained. ValidatingOutOfBudget ValidationErrorType = "out_of_budget" @@ -65,7 +65,7 @@ func newValidationAdmissionMetrics() *ValidatingAdmissionPolicyMetrics { Help: "Validation admission policy check total, labeled by policy and further identified by binding and enforcement action taken.", StabilityLevel: metrics.ALPHA, }, - []string{"policy", "policy_binding", "enforcement_action"}, + []string{"policy", "policy_binding", "error_type", "enforcement_action"}, ) latency := metrics.NewHistogramVec(&metrics.HistogramOpts{ Namespace: metricsNamespace, @@ -83,7 +83,7 @@ func newValidationAdmissionMetrics() *ValidatingAdmissionPolicyMetrics { Buckets: []float64{0.0000005, 0.001, 0.01, 0.1, 1.0}, StabilityLevel: metrics.ALPHA, }, - []string{"policy", "policy_binding", "enforcement_action"}, + []string{"policy", "policy_binding", "error_type", "enforcement_action"}, ) legacyregistry.MustRegister(check) @@ -99,24 +99,24 @@ func (m *ValidatingAdmissionPolicyMetrics) Reset() { // ObserveAdmission observes a policy validation, with an optional error to indicate the error that may occur but ignored. func (m *ValidatingAdmissionPolicyMetrics) ObserveAdmission(ctx context.Context, elapsed time.Duration, policy, binding string, errorType ValidationErrorType) { - m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "allow").Inc() - m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "allow").Observe(elapsed.Seconds()) + m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, string(errorType), "allow").Inc() + m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, string(errorType), "allow").Observe(elapsed.Seconds()) } // ObserveRejection observes a policy validation error that was at least one of the reasons for a deny. -func (m *ValidatingAdmissionPolicyMetrics) ObserveRejection(ctx context.Context, elapsed time.Duration, policy, binding string) { - m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "deny").Inc() - m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "deny").Observe(elapsed.Seconds()) +func (m *ValidatingAdmissionPolicyMetrics) ObserveRejection(ctx context.Context, elapsed time.Duration, policy, binding string, errorType ValidationErrorType) { + m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, string(errorType), "deny").Inc() + m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, string(errorType), "deny").Observe(elapsed.Seconds()) } // ObserveAudit observes a policy validation audit annotation was published for a validation failure. -func (m *ValidatingAdmissionPolicyMetrics) ObserveAudit(ctx context.Context, elapsed time.Duration, policy, binding string) { - m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "audit").Inc() - m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "audit").Observe(elapsed.Seconds()) +func (m *ValidatingAdmissionPolicyMetrics) ObserveAudit(ctx context.Context, elapsed time.Duration, policy, binding string, errorType ValidationErrorType) { + m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, string(errorType), "audit").Inc() + m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, string(errorType), "audit").Observe(elapsed.Seconds()) } // ObserveWarn observes a policy validation warning was published for a validation failure. -func (m *ValidatingAdmissionPolicyMetrics) ObserveWarn(ctx context.Context, elapsed time.Duration, policy, binding string) { - m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "warn").Inc() - m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "warn").Observe(elapsed.Seconds()) +func (m *ValidatingAdmissionPolicyMetrics) ObserveWarn(ctx context.Context, elapsed time.Duration, policy, binding string, errorType ValidationErrorType) { + m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, string(errorType), "warn").Inc() + m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, string(errorType), "warn").Observe(elapsed.Seconds()) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go index e575e0faad0..ad450001b90 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/metrics/metrics_test.go @@ -45,20 +45,20 @@ func TestNoUtils(t *testing.T) { want: ` # HELP apiserver_validating_admission_policy_check_duration_seconds [ALPHA] Validation admission latency for individual validation expressions in seconds, labeled by policy and further including binding and enforcement action taken. # TYPE apiserver_validating_admission_policy_check_duration_seconds histogram - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="0.0000005"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="0.001"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="0.01"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="0.1"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="1"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com",le="+Inf"} 1 - apiserver_validating_admission_policy_check_duration_seconds_sum{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com"} 10 - apiserver_validating_admission_policy_check_duration_seconds_count{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com"} 1 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="0.0000005"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="0.001"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="0.01"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="0.1"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="1"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="allow",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="+Inf"} 1 + apiserver_validating_admission_policy_check_duration_seconds_sum{enforcement_action="allow",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com"} 10 + apiserver_validating_admission_policy_check_duration_seconds_count{enforcement_action="allow",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com"} 1 # HELP apiserver_validating_admission_policy_check_total [ALPHA] Validation admission policy check total, labeled by policy and further identified by binding and enforcement action taken. # TYPE apiserver_validating_admission_policy_check_total counter - apiserver_validating_admission_policy_check_total{enforcement_action="allow",policy="policy.example.com",policy_binding="binding.example.com"} 1 + apiserver_validating_admission_policy_check_total{enforcement_action="allow",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com"} 1 `, observer: func() { - Metrics.ObserveAdmission(context.TODO(), time.Duration(10)*time.Second, "policy.example.com", "binding.example.com") + Metrics.ObserveAdmission(context.TODO(), time.Duration(10)*time.Second, "policy.example.com", "binding.example.com", ValidatingInvalidError) }, }, { @@ -66,20 +66,20 @@ func TestNoUtils(t *testing.T) { want: ` # HELP apiserver_validating_admission_policy_check_duration_seconds [ALPHA] Validation admission latency for individual validation expressions in seconds, labeled by policy and further including binding and enforcement action taken. # TYPE apiserver_validating_admission_policy_check_duration_seconds histogram - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="0.0000005"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="0.001"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="0.01"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="0.1"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="1"} 0 - apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com",le="+Inf"} 1 - apiserver_validating_admission_policy_check_duration_seconds_sum{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com"} 10 - apiserver_validating_admission_policy_check_duration_seconds_count{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com"} 1 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="0.0000005"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="0.001"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="0.01"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="0.1"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="1"} 0 + apiserver_validating_admission_policy_check_duration_seconds_bucket{enforcement_action="deny",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com",le="+Inf"} 1 + apiserver_validating_admission_policy_check_duration_seconds_sum{enforcement_action="deny",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com"} 10 + apiserver_validating_admission_policy_check_duration_seconds_count{enforcement_action="deny",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com"} 1 # HELP apiserver_validating_admission_policy_check_total [ALPHA] Validation admission policy check total, labeled by policy and further identified by binding and enforcement action taken. # TYPE apiserver_validating_admission_policy_check_total counter - apiserver_validating_admission_policy_check_total{enforcement_action="deny",policy="policy.example.com",policy_binding="binding.example.com"} 1 + apiserver_validating_admission_policy_check_total{enforcement_action="deny",error_type="invalid_error",policy="policy.example.com",policy_binding="binding.example.com"} 1 `, observer: func() { - Metrics.ObserveRejection(context.TODO(), time.Duration(10)*time.Second, "policy.example.com", "binding.example.com") + Metrics.ObserveRejection(context.TODO(), time.Duration(10)*time.Second, "policy.example.com", "binding.example.com", ValidatingInvalidError) }, }, }