diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go index dc63caf0b5c..00f42cfe7c7 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go @@ -153,6 +153,7 @@ func BeforeUpdate(strategy RESTUpdateStrategy, ctx context.Context, obj, old run errs = append(errs, strategy.ValidateUpdate(ctx, obj, old)...) if len(errs) > 0 { + RecordDuplicateValidationErrors(ctx, kind.GroupKind(), errs) return errors.NewInvalid(kind.GroupKind(), objectMeta.GetName(), errs) } diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go index 2235a476011..c220fa93b7a 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go @@ -19,6 +19,7 @@ package rest import ( "context" "fmt" + "slices" "strings" "k8s.io/apimachinery/pkg/api/operation" @@ -320,3 +321,20 @@ func panicSafeValidateFunc( return validateUpdateFunc(ctx, scheme, obj, oldObj, o) } } + +// RecordDuplicateValidationErrors increments a metric and log the error when duplicate validation errors are found. +func RecordDuplicateValidationErrors(ctx context.Context, qualifiedKind schema.GroupKind, errs field.ErrorList) { + logger := klog.FromContext(ctx) + seenErrs := make([]string, 0, len(errs)) + + for _, err := range errs { + errStr := fmt.Sprintf("%v", err) + + if slices.Contains(seenErrs, errStr) { + logger.Info("Found duplicate validation error", "kind", qualifiedKind.String(), "error", errStr) + validationmetrics.Metrics.IncDuplicateValidationErrorMetric() + } else { + seenErrs = append(seenErrs, errStr) + } + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go index 19447192963..cd2d77877cf 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go @@ -34,6 +34,9 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/validation" + "k8s.io/component-base/metrics/legacyregistry" + "k8s.io/component-base/metrics/testutil" "k8s.io/klog/v2" ) @@ -678,6 +681,71 @@ func TestValidateUpdateDeclarativelyWithRecovery(t *testing.T) { }) } +func TestRecordDuplicateValidationErrors(t *testing.T) { + ctx := context.Background() + + testCases := []struct { + name string + qualifiedKind schema.GroupKind + errs field.ErrorList + expectedMetric string + }{ + { + name: "detect duplicates and increment metric", + qualifiedKind: schema.GroupKind{Group: "apps", Kind: "ReplicaSet"}, + errs: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("replicas"), -1, "must be greater than or equal to 0").WithOrigin("minimum"), + field.Invalid(field.NewPath("spec").Child("replicas"), -1, "must be greater than or equal to 0").WithOrigin("minimum"), + field.Invalid(field.NewPath("spec").Child("selector"), &metav1.LabelSelector{MatchLabels: map[string]string{}, MatchExpressions: []metav1.LabelSelectorRequirement{}}, "empty selector is invalid for deployment"), + field.Invalid(field.NewPath("spec").Child("selector"), &metav1.LabelSelector{MatchLabels: map[string]string{}, MatchExpressions: []metav1.LabelSelectorRequirement{}}, "empty selector is invalid for deployment"), + }, + expectedMetric: ` + # HELP apiserver_validation_duplicate_validation_error_total [INTERNAL] Number of duplicate validation errors during validation. + # TYPE apiserver_validation_duplicate_validation_error_total counter + apiserver_validation_duplicate_validation_error_total 2 + `, + }, + { + name: "detect duplicates with all fields but origin being equal", + qualifiedKind: schema.GroupKind{Group: "apps", Kind: "ReplicaSet"}, + errs: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("replicas"), -1, "must be greater than or equal to 0").WithOrigin("minimum"), + field.Invalid(field.NewPath("spec").Child("replicas"), -1, "must be greater than or equal to 0").WithOrigin("min"), + }, + expectedMetric: ` + # HELP apiserver_validation_duplicate_validation_error_total [INTERNAL] Number of duplicate validation errors during validation. + # TYPE apiserver_validation_duplicate_validation_error_total counter + apiserver_validation_duplicate_validation_error_total 1 + `, + }, + { + name: "no duplicates", + qualifiedKind: schema.GroupKind{Group: "apps", Kind: "ReplicaSet"}, + errs: field.ErrorList{ + field.Invalid(field.NewPath("spec").Child("replicas"), -1, "must be greater than or equal to 0").WithOrigin("minimum"), + field.Invalid(field.NewPath("spec").Child("selector"), &metav1.LabelSelector{MatchLabels: map[string]string{}, MatchExpressions: []metav1.LabelSelectorRequirement{}}, "empty selector is invalid for deployment"), + }, + expectedMetric: ` + # HELP apiserver_validation_duplicate_validation_error_total [INTERNAL] Number of duplicate validation errors during validation. + # TYPE apiserver_validation_duplicate_validation_error_total counter + apiserver_validation_duplicate_validation_error_total 0 + `, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + defer legacyregistry.Reset() + defer validation.ResetValidationMetricsInstance() + RecordDuplicateValidationErrors(ctx, tc.qualifiedKind, tc.errs) + + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(tc.expectedMetric), "apiserver_validation_duplicate_validation_error_total"); err != nil { + t.Fatal(err) + } + }) + } +} + func equalErrorLists(a, b field.ErrorList) bool { // If both are nil, consider them equal if a == nil && b == nil { diff --git a/staging/src/k8s.io/apiserver/pkg/validation/metrics.go b/staging/src/k8s.io/apiserver/pkg/validation/metrics.go index 256c23c4b1e..0e520cf743f 100644 --- a/staging/src/k8s.io/apiserver/pkg/validation/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/validation/metrics.go @@ -30,6 +30,7 @@ const ( type ValidationMetrics interface { IncDeclarativeValidationMismatchMetric() IncDeclarativeValidationPanicMetric() + IncDuplicateValidationErrorMetric() Reset() } @@ -52,6 +53,15 @@ var validationMetricsInstance = &validationMetrics{ StabilityLevel: metrics.BETA, }, ), + DuplicateValidationErrorCounter: metrics.NewCounter( + &metrics.CounterOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "duplicate_validation_error_total", + Help: "Number of duplicate validation errors during validation.", + StabilityLevel: metrics.INTERNAL, + }, + ), } // Metrics provides access to validation metrics. @@ -60,17 +70,20 @@ var Metrics ValidationMetrics = validationMetricsInstance func init() { legacyregistry.MustRegister(validationMetricsInstance.DeclarativeValidationMismatchCounter) legacyregistry.MustRegister(validationMetricsInstance.DeclarativeValidationPanicCounter) + legacyregistry.MustRegister(validationMetricsInstance.DuplicateValidationErrorCounter) } type validationMetrics struct { DeclarativeValidationMismatchCounter *metrics.Counter DeclarativeValidationPanicCounter *metrics.Counter + DuplicateValidationErrorCounter *metrics.Counter } // Reset resets the validation metrics. func (m *validationMetrics) Reset() { m.DeclarativeValidationMismatchCounter.Reset() m.DeclarativeValidationPanicCounter.Reset() + m.DuplicateValidationErrorCounter.Reset() } // IncDeclarativeValidationMismatchMetric increments the counter for the declarative_validation_mismatch_total metric. @@ -83,6 +96,11 @@ func (m *validationMetrics) IncDeclarativeValidationPanicMetric() { m.DeclarativeValidationPanicCounter.Inc() } +// IncDuplicateValidationErrorMetric increments the counter for the duplicate_validation_error_total metric. +func (m *validationMetrics) IncDuplicateValidationErrorMetric() { + m.DuplicateValidationErrorCounter.Inc() +} + func ResetValidationMetricsInstance() { validationMetricsInstance.Reset() } diff --git a/staging/src/k8s.io/apiserver/pkg/validation/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/validation/metrics_test.go index e93118a00b7..5b038a5444b 100644 --- a/staging/src/k8s.io/apiserver/pkg/validation/metrics_test.go +++ b/staging/src/k8s.io/apiserver/pkg/validation/metrics_test.go @@ -38,7 +38,7 @@ func TestDeclarativeValidationMismatchMetric(t *testing.T) { apiserver_validation_declarative_validation_mismatch_total 1 ` - if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total"); err != nil { + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_declarative_validation_mismatch_total"); err != nil { t.Fatal(err) } } @@ -57,7 +57,26 @@ func TestDeclarativeValidationPanicMetric(t *testing.T) { apiserver_validation_declarative_validation_panic_total 1 ` - if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_panic_total"); err != nil { + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_declarative_validation_panic_total"); err != nil { + t.Fatal(err) + } +} + +// TestDuplicateValidationErrorMetric tests that the duplicate error metric correctly increments once +func TestDuplicateValidationErrorMetric(t *testing.T) { + defer legacyregistry.Reset() + defer ResetValidationMetricsInstance() + + // Increment the metric once + Metrics.IncDuplicateValidationErrorMetric() + + expected := ` + # HELP apiserver_validation_duplicate_validation_error_total [INTERNAL] Number of duplicate validation errors during validation. + # TYPE apiserver_validation_duplicate_validation_error_total counter + apiserver_validation_duplicate_validation_error_total 1 + ` + + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_duplicate_validation_error_total"); err != nil { t.Fatal(err) } } @@ -78,7 +97,7 @@ func TestDeclarativeValidationMismatchMetricMultiple(t *testing.T) { apiserver_validation_declarative_validation_mismatch_total 3 ` - if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total"); err != nil { + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_declarative_validation_mismatch_total"); err != nil { t.Fatal(err) } } @@ -99,7 +118,28 @@ func TestDeclarativeValidationPanicMetricMultiple(t *testing.T) { apiserver_validation_declarative_validation_panic_total 3 ` - if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_panic_total"); err != nil { + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_declarative_validation_panic_total"); err != nil { + t.Fatal(err) + } +} + +// TestDuplicateValidationErrorMetricMultiple tests that the duplicate error metric correctly increments multiple times +func TestDuplicateValidationErrorMetricMultiple(t *testing.T) { + defer legacyregistry.Reset() + defer ResetValidationMetricsInstance() + + // Increment the metric three times + Metrics.IncDuplicateValidationErrorMetric() + Metrics.IncDuplicateValidationErrorMetric() + Metrics.IncDuplicateValidationErrorMetric() + + expected := ` + # HELP apiserver_validation_duplicate_validation_error_total [INTERNAL] Number of duplicate validation errors during validation. + # TYPE apiserver_validation_duplicate_validation_error_total counter + apiserver_validation_duplicate_validation_error_total 3 + ` + + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_duplicate_validation_error_total"); err != nil { t.Fatal(err) } } @@ -112,6 +152,7 @@ func TestDeclarativeValidationMetricsReset(t *testing.T) { // Increment both metrics Metrics.IncDeclarativeValidationMismatchMetric() Metrics.IncDeclarativeValidationPanicMetric() + Metrics.IncDuplicateValidationErrorMetric() // Reset the metrics Metrics.Reset() @@ -124,15 +165,22 @@ func TestDeclarativeValidationMetricsReset(t *testing.T) { # HELP apiserver_validation_declarative_validation_panic_total [BETA] Number of times declarative validation has panicked during validation. # TYPE apiserver_validation_declarative_validation_panic_total counter apiserver_validation_declarative_validation_panic_total 0 + # HELP apiserver_validation_duplicate_validation_error_total [INTERNAL] Number of duplicate validation errors during validation. + # TYPE apiserver_validation_duplicate_validation_error_total counter + apiserver_validation_duplicate_validation_error_total 0 ` - if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total", "declarative_validation_panic_total"); err != nil { + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), + "apiserver_validation_declarative_validation_mismatch_total", + "apiserver_validation_declarative_validation_panic_total", + "apiserver_validation_duplicate_validation_error_total"); err != nil { t.Fatal(err) } // Increment the metrics again to ensure they're still functional Metrics.IncDeclarativeValidationMismatchMetric() Metrics.IncDeclarativeValidationPanicMetric() + Metrics.IncDuplicateValidationErrorMetric() // Verify they've been incremented correctly expected = ` @@ -142,9 +190,15 @@ func TestDeclarativeValidationMetricsReset(t *testing.T) { # HELP apiserver_validation_declarative_validation_panic_total [BETA] Number of times declarative validation has panicked during validation. # TYPE apiserver_validation_declarative_validation_panic_total counter apiserver_validation_declarative_validation_panic_total 1 + # HELP apiserver_validation_duplicate_validation_error_total [INTERNAL] Number of duplicate validation errors during validation. + # TYPE apiserver_validation_duplicate_validation_error_total counter + apiserver_validation_duplicate_validation_error_total 1 ` - if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total", "declarative_validation_panic_total"); err != nil { + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), + "apiserver_validation_declarative_validation_mismatch_total", + "apiserver_validation_declarative_validation_panic_total", + "apiserver_validation_duplicate_validation_error_total"); err != nil { t.Fatal(err) } }