diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index aa3c6d2061f..bdb8784931b 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -39,7 +39,6 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" - fldtest "k8s.io/apimachinery/pkg/util/validation/field/testing" "k8s.io/apimachinery/pkg/util/version" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/component-base/featuregate" @@ -16714,7 +16713,7 @@ func TestValidateReplicationControllerUpdate(t *testing.T) { tc.old.ObjectMeta.ResourceVersion = "1" tc.update.ObjectMeta.ResourceVersion = "1" errs := ValidateReplicationControllerUpdate(&tc.update, &tc.old, PodValidationOptions{}) - matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin().ByDetailSubstring() + matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin().ByDetailSubstring() matcher.Test(t, tc.expectedErrs, errs) }) } @@ -16864,7 +16863,7 @@ func TestValidateReplicationController(t *testing.T) { for k, tc := range errorCases { t.Run(k, func(t *testing.T) { errs := ValidateReplicationController(&tc.input, PodValidationOptions{}) - matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin().ByDetailSubstring() + matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin().ByDetailSubstring() matcher.Test(t, tc.expectedErrs, errs) }) } @@ -20770,7 +20769,7 @@ func TestValidateEndpointsCreate(t *testing.T) { t.Run(k, func(t *testing.T) { errs := ValidateEndpointsCreate(&v.endpoints) // TODO: set .RequireOriginWhenInvalid() once metadata is done - matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin() + matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() matcher.Test(t, v.expectedErrs, errs) }) } @@ -22767,7 +22766,7 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { errs := validateTopologySpreadConstraints(tc.constraints, fieldPath, tc.opts) - matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin().RequireOriginWhenInvalid() + matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin().RequireOriginWhenInvalid() matcher.Test(t, tc.wantFieldErrors, errs) }) } diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/scheme_test.go b/staging/src/k8s.io/apimachinery/pkg/runtime/scheme_test.go index fd87634d60a..b2712c107d7 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/scheme_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/scheme_test.go @@ -35,7 +35,6 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" - fieldtesting "k8s.io/apimachinery/pkg/util/validation/field/testing" ) type testConversions struct { @@ -1089,7 +1088,7 @@ func TestRegisterValidate(t *testing.T) { } else { results = s.ValidateUpdate(ctx, tc.options, tc.object, tc.oldObject, tc.subresource...) } - matcher := fieldtesting.ErrorMatcher{}.ByType().ByField().ByOrigin() + matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() matcher.Test(t, tc.expected, results) }) } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/testing/error_matcher.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher.go similarity index 88% rename from staging/src/k8s.io/apimachinery/pkg/util/validation/field/testing/error_matcher.go rename to staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher.go index 40530035e57..1d15deae868 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/testing/error_matcher.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher.go @@ -14,19 +14,16 @@ See the License for the specific language governing permissions and limitations under the License. */ -package testing +package field import ( "fmt" "reflect" "regexp" "strings" - "testing" - - field "k8s.io/apimachinery/pkg/util/validation/field" ) -// ErrorMatcher is a helper for comparing field.Error objects. +// ErrorMatcher is a helper for comparing Error objects. type ErrorMatcher struct { // TODO(thockin): consider whether type is ever NOT required, maybe just // assume it. @@ -42,9 +39,9 @@ type ErrorMatcher struct { requireOriginWhenInvalid bool } -// Matches returns true if the two field.Error objects match according to the +// Matches returns true if the two Error objects match according to the // configured criteria. -func (m ErrorMatcher) Matches(want, got *field.Error) bool { +func (m ErrorMatcher) Matches(want, got *Error) bool { if m.matchType && want.Type != got.Type { return false } @@ -58,7 +55,7 @@ func (m ErrorMatcher) Matches(want, got *field.Error) bool { if want.Origin != got.Origin { return false } - if m.requireOriginWhenInvalid && want.Type == field.ErrorTypeInvalid { + if m.requireOriginWhenInvalid && want.Type == ErrorTypeInvalid { if want.Origin == "" || got.Origin == "" { return false } @@ -72,7 +69,7 @@ func (m ErrorMatcher) Matches(want, got *field.Error) bool { // Render returns a string representation of the specified Error object, // according to the criteria configured in the ErrorMatcher. -func (m ErrorMatcher) Render(e *field.Error) string { +func (m ErrorMatcher) Render(e *Error) string { buf := strings.Builder{} comma := func() { @@ -93,7 +90,7 @@ func (m ErrorMatcher) Render(e *field.Error) string { comma() buf.WriteString(fmt.Sprintf("Value=%v", e.BadValue)) } - if m.matchOrigin || m.requireOriginWhenInvalid && e.Type == field.ErrorTypeInvalid { + if m.matchOrigin || m.requireOriginWhenInvalid && e.Type == ErrorTypeInvalid { comma() buf.WriteString(fmt.Sprintf("Origin=%q", e.Origin)) } @@ -170,17 +167,25 @@ func (m ErrorMatcher) ByDetailRegexp() ErrorMatcher { return m } +// TestIntf lets users pass a testing.T while not coupling this package to Go's +// testing package. +type TestIntf interface { + Helper() + Errorf(format string, args ...any) + Logf(format string, args ...any) +} + // Test compares two ErrorLists by the criteria configured in this matcher, and // fails the test if they don't match. If a given "want" error matches multiple // "got" errors, they will all be consumed. This might be OK (e.g. if there are // multiple errors on the same field from the same origin) or it might be an // insufficiently specific matcher, so these will be logged. -func (m ErrorMatcher) Test(tb testing.TB, want, got field.ErrorList) { +func (m ErrorMatcher) Test(tb TestIntf, want, got ErrorList) { tb.Helper() remaining := got for _, w := range want { - tmp := make(field.ErrorList, 0, len(remaining)) + tmp := make(ErrorList, 0, len(remaining)) n := 0 for _, g := range remaining { if m.Matches(w, g) { 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 1fa6dc1f552..02f01ce9526 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go @@ -26,6 +26,8 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + validationmetrics "k8s.io/apiserver/pkg/validation" + "k8s.io/klog/v2" ) // ValidateDeclaratively validates obj against declarative validation tags @@ -106,3 +108,212 @@ func parseSubresourcePath(subresourcePath string) ([]string, error) { parts := strings.Split(subresourcePath[1:], "/") return parts, nil } + +// CompareDeclarativeErrorsAndEmitMismatches checks for mismatches between imperative and declarative validation +// and logs + emits metrics when inconsistencies are found +func CompareDeclarativeErrorsAndEmitMismatches(ctx context.Context, imperativeErrs, declarativeErrs field.ErrorList, takeover bool) { + logger := klog.FromContext(ctx) + mismatchDetails := gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs, takeover) + for _, detail := range mismatchDetails { + // Log information about the mismatch using contextual logger + logger.Info(detail) + + // Increment the metric for the mismatch + validationmetrics.Metrics.IncDeclarativeValidationMismatchMetric() + } +} + +// gatherDeclarativeValidationMismatches compares imperative and declarative validation errors +// and returns detailed information about any mismatches found. Errors are compared via type, field, and origin +func gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs field.ErrorList, takeover bool) []string { + var mismatchDetails []string + // short circuit here to minimize allocs for usual case of 0 validation errors + if len(imperativeErrs) == 0 && len(declarativeErrs) == 0 { + return mismatchDetails + } + // recommendation based on takeover status + recommendation := "This difference should not affect system operation since hand written validation is authoritative." + if takeover { + recommendation = "Consider disabling the DeclarativeValidationTakeover feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes." + } + fuzzyMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin().RequireOriginWhenInvalid() + exactMatcher := field.ErrorMatcher{}.Exactly() + + // Dedupe imperative errors of exact error matches as they are + // not intended and come from (buggy) duplicate validation calls + // This is necessary as without deduping we could get unmatched + // imperative errors for cases that are correct (matching) + dedupedImperativeErrs := field.ErrorList{} + for _, err := range imperativeErrs { + found := false + for _, existingErr := range dedupedImperativeErrs { + if exactMatcher.Matches(existingErr, err) { + found = true + break + } + } + if !found { + dedupedImperativeErrs = append(dedupedImperativeErrs, err) + } + } + imperativeErrs = dedupedImperativeErrs + + // Create a copy of declarative errors to track remaining ones + remaining := make(field.ErrorList, len(declarativeErrs)) + copy(remaining, declarativeErrs) + + // Match each "covered" imperative error to declarative errors. + // We use a fuzzy matching approach to find corresponding declarative errors + // for each imperative error marked as CoveredByDeclarative. + // As matches are found, they're removed from the 'remaining' list. + // They are removed from `remaining` with a "1:many" mapping: for a given + // imperative error we mark as matched all matching declarative errors + // This allows us to: + // 1. Detect imperative errors that should have matching declarative errors but don't + // 2. Identify extra declarative errors with no imperative counterpart + // Both cases indicate issues with the declarative validation implementation. + for _, iErr := range imperativeErrs { + if !iErr.CoveredByDeclarative { + continue + } + + tmp := make(field.ErrorList, 0, len(remaining)) + matchCount := 0 + + for _, dErr := range remaining { + if fuzzyMatcher.Matches(iErr, dErr) { + matchCount++ + } else { + tmp = append(tmp, dErr) + } + } + + if matchCount == 0 { + mismatchDetails = append(mismatchDetails, + fmt.Sprintf( + "Unexpected difference between hand written validation and declarative validation error results, unmatched error(s) found %s. "+ + "This indicates an issue with declarative validation. %s", + fuzzyMatcher.Render(iErr), + recommendation, + ), + ) + } + + remaining = tmp + } + + // Any remaining unmatched declarative errors are considered "extra" + for _, dErr := range remaining { + mismatchDetails = append(mismatchDetails, + fmt.Sprintf( + "Unexpected difference between hand written validation and declarative validation error results, extra error(s) found %s. "+ + "This indicates an issue with declarative validation. %s", + fuzzyMatcher.Render(dErr), + recommendation, + ), + ) + } + + return mismatchDetails +} + +// createDeclarativeValidationPanicHandler returns a function with panic recovery logic +// that will increment the panic metric and either log or append errors based on the takeover parameter. +func createDeclarativeValidationPanicHandler(ctx context.Context, errs *field.ErrorList, takeover bool) func() { + logger := klog.FromContext(ctx) + return func() { + if r := recover(); r != nil { + // Increment the panic metric counter + validationmetrics.Metrics.IncDeclarativeValidationPanicMetric() + + const errorFmt = "panic during declarative validation: %v" + if takeover { + // If takeover is enabled, output as a validation error as authoritative validator panicked and validation should error + *errs = append(*errs, field.InternalError(nil, fmt.Errorf(errorFmt, r))) + } else { + // if takeover not enabled, log the panic as an info message + logger.Info(fmt.Sprintf(errorFmt, r)) + } + } + } +} + +// withRecover wraps a validation function with panic recovery logic. +// It takes a validation function with the ValidateDeclaratively signature +// and returns a function with the same signature. +// The returned function will execute the wrapped function and handle any panics by +// incrementing the panic metric, and logging an error message +// if takeover=false, and adding a validation error if takeover=true. +func withRecover( + validateFunc func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj runtime.Object) field.ErrorList, + takeover bool, +) func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj runtime.Object) field.ErrorList { + return func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj runtime.Object) (errs field.ErrorList) { + defer createDeclarativeValidationPanicHandler(ctx, &errs, takeover)() + + return validateFunc(ctx, options, scheme, obj) + } +} + +// withRecoverUpdate wraps an update validation function with panic recovery logic. +// It takes a validation function with the ValidateUpdateDeclaratively signature +// and returns a function with the same signature. +// The returned function will execute the wrapped function and handle any panics by +// incrementing the panic metric, and logging an error message +// if takeover=false, and adding a validation error if takeover=true. +func withRecoverUpdate( + validateUpdateFunc func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj, oldObj runtime.Object) field.ErrorList, + takeover bool, +) func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj, oldObj runtime.Object) field.ErrorList { + return func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj, oldObj runtime.Object) (errs field.ErrorList) { + defer createDeclarativeValidationPanicHandler(ctx, &errs, takeover)() + + return validateUpdateFunc(ctx, options, scheme, obj, oldObj) + } +} + +// ValidateDeclarativelyWithRecovery validates obj against declarative validation tags +// with panic recovery logic. It uses the API version extracted from ctx and the +// provided scheme for validation. +// +// The ctx MUST contain requestInfo, which determines the target API for +// validation. The obj is converted to the API version using the provided scheme +// before validation occurs. The scheme MUST have the declarative validation +// registered for the requested resource/subresource. +// +// option should contain any validation options that the declarative validation +// tags expect. +// +// takeover determines if panic recovery should return validation errors (true) or +// just log warnings (false). +// +// Returns a field.ErrorList containing any validation errors. An internal error +// is included if requestInfo is missing from the context, if version +// conversion fails, or if a panic occurs during validation when +// takeover is true. +func ValidateDeclarativelyWithRecovery(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj runtime.Object, takeover bool) field.ErrorList { + return withRecover(ValidateDeclaratively, takeover)(ctx, options, scheme, obj) +} + +// ValidateUpdateDeclarativelyWithRecovery validates obj and oldObj against declarative +// validation tags with panic recovery logic. It uses the API version extracted from +// ctx and the provided scheme for validation. +// +// The ctx MUST contain requestInfo, which determines the target API for +// validation. The obj is converted to the API version using the provided scheme +// before validation occurs. The scheme MUST have the declarative validation +// registered for the requested resource/subresource. +// +// option should contain any validation options that the declarative validation +// tags expect. +// +// takeover determines if panic recovery should return validation errors (true) or +// just log warnings (false). +// +// Returns a field.ErrorList containing any validation errors. An internal error +// is included if requestInfo is missing from the context, if version +// conversion fails, or if a panic occurs during validation when +// takeover is true. +func ValidateUpdateDeclarativelyWithRecovery(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj, oldObj runtime.Object, takeover bool) field.ErrorList { + return withRecoverUpdate(ValidateUpdateDeclaratively, takeover)(ctx, options, scheme, obj, oldObj) +} 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 8ed9ed9adee..642901d6c93 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 @@ -17,8 +17,12 @@ limitations under the License. package rest import ( + "bytes" "context" "fmt" + "reflect" + "regexp" + "strings" "testing" v1 "k8s.io/api/core/v1" @@ -29,8 +33,8 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" - fieldtesting "k8s.io/apimachinery/pkg/util/validation/field/testing" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/klog/v2" ) func TestValidateDeclaratively(t *testing.T) { @@ -154,7 +158,7 @@ func TestValidateDeclaratively(t *testing.T) { } else { results = ValidateUpdateDeclaratively(ctx, tc.options, scheme, tc.object, tc.oldObject) } - matcher := fieldtesting.ErrorMatcher{}.ByType().ByField().ByOrigin() + matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() matcher.Test(t, tc.expected, results) }) } @@ -182,3 +186,500 @@ func (p Pod) DeepCopyObject() runtime.Object { RestartPolicy: p.RestartPolicy, } } + +// TestGatherDeclarativeValidationMismatches tests all mismatch +// scenarios across imperative and declarative errors for +// the gatherDeclarativeValidationMismatches function +func TestGatherDeclarativeValidationMismatches(t *testing.T) { + replicasPath := field.NewPath("spec").Child("replicas") + minReadySecondsPath := field.NewPath("spec").Child("minReadySeconds") + selectorPath := field.NewPath("spec").Child("selector") + + errA := field.Invalid(replicasPath, nil, "regular error A") + errB := field.Invalid(minReadySecondsPath, -1, "covered error B").WithOrigin("minimum") + coveredErrB := field.Invalid(minReadySecondsPath, -1, "covered error B").WithOrigin("minimum") + errBWithDiffDetail := field.Invalid(minReadySecondsPath, -1, "covered error B - different detail").WithOrigin("minimum") + coveredErrB.CoveredByDeclarative = true + errC := field.Invalid(replicasPath, nil, "covered error C").WithOrigin("minimum") + coveredErrC := field.Invalid(replicasPath, nil, "covered error C").WithOrigin("minimum") + coveredErrC.CoveredByDeclarative = true + errCWithDiffOrigin := field.Invalid(replicasPath, nil, "covered error C").WithOrigin("maximum") + errD := field.Invalid(selectorPath, nil, "regular error D") + + testCases := []struct { + name string + imperativeErrors field.ErrorList + declarativeErrors field.ErrorList + takeover bool + expectMismatches bool + expectDetailsContaining []string + }{ + { + name: "Declarative and imperative return 0 errors - no mismatch", + imperativeErrors: field.ErrorList{}, + declarativeErrors: field.ErrorList{}, + takeover: false, + expectMismatches: false, + expectDetailsContaining: []string{}, + }, + { + name: "Declarative returns multiple errors with different origins, errors match - no mismatch", + imperativeErrors: field.ErrorList{ + errA, + coveredErrB, + coveredErrC, + errD, + }, + declarativeErrors: field.ErrorList{ + errB, + errC, + }, + takeover: false, + expectMismatches: false, + expectDetailsContaining: []string{}, + }, + { + name: "Declarative returns multiple errors with different origins, errors don't match - mismatch case", + imperativeErrors: field.ErrorList{ + errA, + coveredErrB, + coveredErrC, + }, + declarativeErrors: field.ErrorList{ + errB, + errCWithDiffOrigin, + }, + takeover: true, + expectMismatches: true, + expectDetailsContaining: []string{ + "Unexpected difference between hand written validation and declarative validation error results", + "unmatched error(s) found", + "extra error(s) found", + "replicas", + "Consider disabling the DeclarativeValidationTakeover feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes", + }, + }, + { + name: "Declarative and imperative return exactly 1 error, errors match - no mismatch", + imperativeErrors: field.ErrorList{ + coveredErrB, + }, + declarativeErrors: field.ErrorList{ + errB, + }, + takeover: false, + expectMismatches: false, + expectDetailsContaining: []string{}, + }, + { + name: "Declarative and imperative exactly 1 error, errors don't match - mismatch", + imperativeErrors: field.ErrorList{ + coveredErrB, + }, + declarativeErrors: field.ErrorList{ + errC, + }, + takeover: false, + expectMismatches: true, + expectDetailsContaining: []string{ + "Unexpected difference between hand written validation and declarative validation error results", + "unmatched error(s) found", + "minReadySeconds", + "extra error(s) found", + "replicas", + "This difference should not affect system operation since hand written validation is authoritative", + }, + }, + { + name: "Declarative returns 0 errors, imperative returns 1 covered error - mismatch", + imperativeErrors: field.ErrorList{ + coveredErrB, + }, + declarativeErrors: field.ErrorList{}, + takeover: true, + expectMismatches: true, + expectDetailsContaining: []string{ + "Unexpected difference between hand written validation and declarative validation error results", + "unmatched error(s) found", + "minReadySeconds", + "Consider disabling the DeclarativeValidationTakeover feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes", + }, + }, + { + name: "Declarative returns 0 errors, imperative returns 1 uncovered error - no mismatch", + imperativeErrors: field.ErrorList{ + errB, + }, + declarativeErrors: field.ErrorList{}, + takeover: false, + expectMismatches: false, + expectDetailsContaining: []string{}, + }, + { + name: "Declarative returns 1 error, imperative returns 0 error - mismatch", + imperativeErrors: field.ErrorList{}, + declarativeErrors: field.ErrorList{ + errB, + }, + takeover: false, + expectMismatches: true, + expectDetailsContaining: []string{ + "Unexpected difference between hand written validation and declarative validation error results", + "extra error(s) found", + "minReadySeconds", + "This difference should not affect system operation since hand written validation is authoritative", + }, + }, + { + name: "Declarative returns 1 error, imperative returns 3 matching errors - no mismatch", + imperativeErrors: field.ErrorList{ + coveredErrB, + }, + declarativeErrors: field.ErrorList{ + errB, + errB, + errBWithDiffDetail, + }, + takeover: false, + expectMismatches: false, + expectDetailsContaining: []string{}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + details := gatherDeclarativeValidationMismatches(tc.imperativeErrors, tc.declarativeErrors, tc.takeover) + // Check if mismatches were found if expected + if tc.expectMismatches && len(details) == 0 { + t.Errorf("Expected mismatches but got none") + } + // Check if details contain expected text + detailsStr := strings.Join(details, " ") + for _, expectedContent := range tc.expectDetailsContaining { + if !strings.Contains(detailsStr, expectedContent) { + t.Errorf("Expected details to contain: %q, but they didn't.\nDetails were:\n%s", + expectedContent, strings.Join(details, "\n")) + } + } + // If we don't expect any details, make sure none provided + if len(tc.expectDetailsContaining) == 0 && len(details) > 0 { + t.Errorf("Expected no details, but got %d details: %v", len(details), details) + } + }) + } +} + +// TestCompareDeclarativeErrorsAndEmitMismatches tests expected +// logging of mismatch information given match & mismatch error conditions. +func TestCompareDeclarativeErrorsAndEmitMismatches(t *testing.T) { + replicasPath := field.NewPath("spec").Child("replicas") + minReadySecondsPath := field.NewPath("spec").Child("minReadySeconds") + + errA := field.Invalid(replicasPath, nil, "regular error A") + errB := field.Invalid(minReadySecondsPath, -1, "covered error B").WithOrigin("minimum") + coveredErrB := field.Invalid(minReadySecondsPath, -1, "covered error B").WithOrigin("minimum") + coveredErrB.CoveredByDeclarative = true + + testCases := []struct { + name string + imperativeErrs field.ErrorList + declarativeErrs field.ErrorList + takeover bool + expectLogs bool + expectedRegex string + }{ + { + name: "mismatched errors, log info", + imperativeErrs: field.ErrorList{coveredErrB}, + declarativeErrs: field.ErrorList{errA}, + takeover: true, + expectLogs: true, + // logs have a prefix of the form - I0309 21:05:33.865030 1926106 validate.go:199] + expectedRegex: "I.*Unexpected difference between hand written validation and declarative validation error results.*Consider disabling the DeclarativeValidationTakeover feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes", + }, + { + name: "matching errors, don't log info", + imperativeErrs: field.ErrorList{coveredErrB}, + declarativeErrs: field.ErrorList{errB}, + takeover: true, + expectLogs: false, + expectedRegex: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + klog.SetOutput(&buf) + klog.LogToStderr(false) + defer klog.LogToStderr(true) + ctx := context.Background() + + CompareDeclarativeErrorsAndEmitMismatches(ctx, tc.imperativeErrs, tc.declarativeErrs, tc.takeover) + + klog.Flush() + logOutput := buf.String() + + if tc.expectLogs { + matched, err := regexp.MatchString(tc.expectedRegex, logOutput) + if err != nil { + t.Fatalf("Bad regex: %v", err) + } + if !matched { + t.Errorf("Expected log output to match %q, but got:\n%s", tc.expectedRegex, logOutput) + } + } else if len(logOutput) > 0 { + t.Errorf("Expected no mismatch logs, but found: %s", logOutput) + } + }) + } +} + +func TestWithRecover(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + options := sets.New[string]() + obj := &runtime.Unknown{} + + testCases := []struct { + name string + validateFn func(context.Context, sets.Set[string], *runtime.Scheme, runtime.Object) field.ErrorList + takeoverEnabled bool + wantErrs field.ErrorList + expectLogRegex string + }{ + { + name: "no panic", + validateFn: func(context.Context, sets.Set[string], *runtime.Scheme, runtime.Object) field.ErrorList { + return field.ErrorList{ + field.Invalid(field.NewPath("field"), "value", "reason"), + } + }, + takeoverEnabled: false, + wantErrs: field.ErrorList{ + field.Invalid(field.NewPath("field"), "value", "reason"), + }, + expectLogRegex: "", + }, + { + name: "panic with takeover disabled", + validateFn: func(context.Context, sets.Set[string], *runtime.Scheme, runtime.Object) field.ErrorList { + panic("test panic") + }, + takeoverEnabled: false, + wantErrs: nil, + // logs have a prefix of the form - I0309 21:05:33.865030 1926106 validate.go:199] + expectLogRegex: "I.*panic during declarative validation: test panic", + }, + { + name: "panic with takeover enabled", + validateFn: func(context.Context, sets.Set[string], *runtime.Scheme, runtime.Object) field.ErrorList { + panic("test panic") + }, + takeoverEnabled: true, + wantErrs: field.ErrorList{ + field.InternalError(nil, fmt.Errorf("panic during declarative validation: test panic")), + }, + expectLogRegex: "", + }, + { + name: "nil return, no panic", + validateFn: func(context.Context, sets.Set[string], *runtime.Scheme, runtime.Object) field.ErrorList { + return nil + }, + takeoverEnabled: false, + wantErrs: nil, + expectLogRegex: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + klog.SetOutput(&buf) + klog.LogToStderr(false) + defer klog.LogToStderr(true) + + // Pass the takeover flag to withRecover instead of relying on the feature gate + wrapped := withRecover(tc.validateFn, tc.takeoverEnabled) + gotErrs := wrapped(ctx, options, scheme, obj) + + klog.Flush() + logOutput := buf.String() + + // Compare gotErrs vs. tc.wantErrs + if !equalErrorLists(gotErrs, tc.wantErrs) { + t.Errorf("withRecover() gotErrs = %#v, want %#v", gotErrs, tc.wantErrs) + } + + // Check logs if needed + if tc.expectLogRegex != "" { + matched, err := regexp.MatchString(tc.expectLogRegex, logOutput) + if err != nil { + t.Fatalf("Bad regex: %v", err) + } + if !matched { + t.Errorf("Expected log output %q, but got:\n%s", tc.expectLogRegex, logOutput) + } + } else if strings.Contains(logOutput, "panic during declarative validation") { + t.Errorf("Unexpected panic log found: %s", logOutput) + } + }) + } +} + +func TestWithRecoverUpdate(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + options := sets.New[string]() + obj := &runtime.Unknown{} + oldObj := &runtime.Unknown{} + + testCases := []struct { + name string + validateFn func(context.Context, sets.Set[string], *runtime.Scheme, runtime.Object, runtime.Object) field.ErrorList + takeoverEnabled bool + wantErrs field.ErrorList + expectLogRegex string + }{ + { + name: "no panic", + validateFn: func(context.Context, sets.Set[string], *runtime.Scheme, runtime.Object, runtime.Object) field.ErrorList { + return field.ErrorList{ + field.Invalid(field.NewPath("field"), "value", "reason"), + } + }, + takeoverEnabled: false, + wantErrs: field.ErrorList{ + field.Invalid(field.NewPath("field"), "value", "reason"), + }, + expectLogRegex: "", + }, + { + name: "panic with takeover disabled", + validateFn: func(context.Context, sets.Set[string], *runtime.Scheme, runtime.Object, runtime.Object) field.ErrorList { + panic("test update panic") + }, + takeoverEnabled: false, + wantErrs: nil, + // logs have a prefix of the form - I0309 21:05:33.865030 1926106 validate.go:199] + expectLogRegex: "I.*panic during declarative validation: test update panic", + }, + { + name: "panic with takeover enabled", + validateFn: func(context.Context, sets.Set[string], *runtime.Scheme, runtime.Object, runtime.Object) field.ErrorList { + panic("test update panic") + }, + takeoverEnabled: true, + wantErrs: field.ErrorList{ + field.InternalError(nil, fmt.Errorf("panic during declarative validation: test update panic")), + }, + expectLogRegex: "", + }, + { + name: "nil return, no panic", + validateFn: func(context.Context, sets.Set[string], *runtime.Scheme, runtime.Object, runtime.Object) field.ErrorList { + return nil + }, + takeoverEnabled: false, + wantErrs: nil, + expectLogRegex: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + klog.SetOutput(&buf) + klog.LogToStderr(false) + defer klog.LogToStderr(true) + + // Pass the takeover flag to withRecoverUpdate instead of relying on the feature gate + wrapped := withRecoverUpdate(tc.validateFn, tc.takeoverEnabled) + gotErrs := wrapped(ctx, options, scheme, obj, oldObj) + + klog.Flush() + logOutput := buf.String() + + // Compare gotErrs with wantErrs + if !equalErrorLists(gotErrs, tc.wantErrs) { + t.Errorf("withRecoverUpdate() gotErrs = %#v, want %#v", gotErrs, tc.wantErrs) + } + + // Verify log output + if tc.expectLogRegex != "" { + matched, err := regexp.MatchString(tc.expectLogRegex, logOutput) + if err != nil { + t.Fatalf("Bad regex: %v", err) + } + if !matched { + t.Errorf("Expected log pattern %q, but got:\n%s", tc.expectLogRegex, logOutput) + } + } else if strings.Contains(logOutput, "panic during declarative validation") { + t.Errorf("Unexpected panic log found: %s", logOutput) + } + }) + } +} + +func TestValidateDeclarativelyWithRecovery(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + options := sets.New[string]() + obj := &runtime.Unknown{} + + // Simple test for the ValidateDeclarativelyWithRecovery function + t.Run("with takeover disabled", func(t *testing.T) { + errs := ValidateDeclarativelyWithRecovery(ctx, options, scheme, obj, false) + if errs == nil { + // This is expected to error since the request info is missing + t.Errorf("Expected errors but got nil") + } + }) + + t.Run("with takeover enabled", func(t *testing.T) { + errs := ValidateDeclarativelyWithRecovery(ctx, options, scheme, obj, true) + if errs == nil { + // This is expected to error since the request info is missing + t.Errorf("Expected errors but got nil") + } + }) +} + +func TestValidateUpdateDeclarativelyWithRecovery(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + options := sets.New[string]() + obj := &runtime.Unknown{} + oldObj := &runtime.Unknown{} + + // Simple test for the ValidateUpdateDeclarativelyWithRecovery function + t.Run("with takeover disabled", func(t *testing.T) { + errs := ValidateUpdateDeclarativelyWithRecovery(ctx, options, scheme, obj, oldObj, false) + if errs == nil { + // This is expected to error since the request info is missing + t.Errorf("Expected errors but got nil") + } + }) + + t.Run("with takeover enabled", func(t *testing.T) { + errs := ValidateUpdateDeclarativelyWithRecovery(ctx, options, scheme, obj, oldObj, true) + if errs == nil { + // This is expected to error since the request info is missing + t.Errorf("Expected errors but got nil") + } + }) +} + +func equalErrorLists(a, b field.ErrorList) bool { + // If both are nil, consider them equal + if a == nil && b == nil { + return true + } + // If one is nil and the other not, they're different + if (a == nil && b != nil) || (a != nil && b == nil) { + return false + } + // Both non-nil: do a normal DeepEqual + return reflect.DeepEqual(a, b) +} diff --git a/staging/src/k8s.io/apiserver/pkg/validation/metrics.go b/staging/src/k8s.io/apiserver/pkg/validation/metrics.go new file mode 100644 index 00000000000..256c23c4b1e --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/validation/metrics.go @@ -0,0 +1,88 @@ +/* +Copyright 2025 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 validation + +import ( + "k8s.io/component-base/metrics" + "k8s.io/component-base/metrics/legacyregistry" +) + +const ( + namespace = "apiserver" // Keep it consistent; apiserver is handling it + subsystem = "validation" +) + +// ValidationMetrics is the interface for validation metrics. +type ValidationMetrics interface { + IncDeclarativeValidationMismatchMetric() + IncDeclarativeValidationPanicMetric() + Reset() +} + +var validationMetricsInstance = &validationMetrics{ + DeclarativeValidationMismatchCounter: metrics.NewCounter( + &metrics.CounterOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "declarative_validation_mismatch_total", + Help: "Number of times declarative validation results differed from handwritten validation results for core types.", + StabilityLevel: metrics.BETA, + }, + ), + DeclarativeValidationPanicCounter: metrics.NewCounter( + &metrics.CounterOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "declarative_validation_panic_total", + Help: "Number of times declarative validation has panicked during validation.", + StabilityLevel: metrics.BETA, + }, + ), +} + +// Metrics provides access to validation metrics. +var Metrics ValidationMetrics = validationMetricsInstance + +func init() { + legacyregistry.MustRegister(validationMetricsInstance.DeclarativeValidationMismatchCounter) + legacyregistry.MustRegister(validationMetricsInstance.DeclarativeValidationPanicCounter) +} + +type validationMetrics struct { + DeclarativeValidationMismatchCounter *metrics.Counter + DeclarativeValidationPanicCounter *metrics.Counter +} + +// Reset resets the validation metrics. +func (m *validationMetrics) Reset() { + m.DeclarativeValidationMismatchCounter.Reset() + m.DeclarativeValidationPanicCounter.Reset() +} + +// IncDeclarativeValidationMismatchMetric increments the counter for the declarative_validation_mismatch_total metric. +func (m *validationMetrics) IncDeclarativeValidationMismatchMetric() { + m.DeclarativeValidationMismatchCounter.Inc() +} + +// IncDeclarativeValidationPanicMetric increments the counter for the declarative_validation_panic_total metric. +func (m *validationMetrics) IncDeclarativeValidationPanicMetric() { + m.DeclarativeValidationPanicCounter.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 new file mode 100644 index 00000000000..e93118a00b7 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/validation/metrics_test.go @@ -0,0 +1,150 @@ +/* +Copyright 2025 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 validation + +import ( + "strings" + "testing" + + "k8s.io/component-base/metrics/legacyregistry" + "k8s.io/component-base/metrics/testutil" +) + +// TestDeclarativeValidationMismatchMetric tests that the mismatch metric correctly increments once +func TestDeclarativeValidationMismatchMetric(t *testing.T) { + defer legacyregistry.Reset() + defer ResetValidationMetricsInstance() + + // Increment the metric once + Metrics.IncDeclarativeValidationMismatchMetric() + + expected := ` + # HELP apiserver_validation_declarative_validation_mismatch_total [BETA] Number of times declarative validation results differed from handwritten validation results for core types. + # TYPE apiserver_validation_declarative_validation_mismatch_total counter + apiserver_validation_declarative_validation_mismatch_total 1 + ` + + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total"); err != nil { + t.Fatal(err) + } +} + +// TestDeclarativeValidationPanicMetric tests that the panic metric correctly increments once +func TestDeclarativeValidationPanicMetric(t *testing.T) { + defer legacyregistry.Reset() + defer ResetValidationMetricsInstance() + + // Increment the metric once + Metrics.IncDeclarativeValidationPanicMetric() + + expected := ` + # 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 + ` + + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_panic_total"); err != nil { + t.Fatal(err) + } +} + +// TestDeclarativeValidationMismatchMetricMultiple tests that the mismatch metric correctly increments multiple times +func TestDeclarativeValidationMismatchMetricMultiple(t *testing.T) { + defer legacyregistry.Reset() + defer ResetValidationMetricsInstance() + + // Increment the metric three times + Metrics.IncDeclarativeValidationMismatchMetric() + Metrics.IncDeclarativeValidationMismatchMetric() + Metrics.IncDeclarativeValidationMismatchMetric() + + expected := ` + # HELP apiserver_validation_declarative_validation_mismatch_total [BETA] Number of times declarative validation results differed from handwritten validation results for core types. + # TYPE apiserver_validation_declarative_validation_mismatch_total counter + apiserver_validation_declarative_validation_mismatch_total 3 + ` + + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total"); err != nil { + t.Fatal(err) + } +} + +// TestDeclarativeValidationPanicMetricMultiple tests that the panic metric correctly increments multiple times +func TestDeclarativeValidationPanicMetricMultiple(t *testing.T) { + defer legacyregistry.Reset() + defer ResetValidationMetricsInstance() + + // Increment the metric three times + Metrics.IncDeclarativeValidationPanicMetric() + Metrics.IncDeclarativeValidationPanicMetric() + Metrics.IncDeclarativeValidationPanicMetric() + + expected := ` + # 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 3 + ` + + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_panic_total"); err != nil { + t.Fatal(err) + } +} + +// TestDeclarativeValidationMetricsReset tests that the Reset function correctly resets the metrics to zero +func TestDeclarativeValidationMetricsReset(t *testing.T) { + defer legacyregistry.Reset() + defer ResetValidationMetricsInstance() + + // Increment both metrics + Metrics.IncDeclarativeValidationMismatchMetric() + Metrics.IncDeclarativeValidationPanicMetric() + + // Reset the metrics + Metrics.Reset() + + // Verify they've been reset to zero + expected := ` + # HELP apiserver_validation_declarative_validation_mismatch_total [BETA] Number of times declarative validation results differed from handwritten validation results for core types. + # TYPE apiserver_validation_declarative_validation_mismatch_total counter + apiserver_validation_declarative_validation_mismatch_total 0 + # 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 + ` + + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total", "declarative_validation_panic_total"); err != nil { + t.Fatal(err) + } + + // Increment the metrics again to ensure they're still functional + Metrics.IncDeclarativeValidationMismatchMetric() + Metrics.IncDeclarativeValidationPanicMetric() + + // Verify they've been incremented correctly + expected = ` + # HELP apiserver_validation_declarative_validation_mismatch_total [BETA] Number of times declarative validation results differed from handwritten validation results for core types. + # TYPE apiserver_validation_declarative_validation_mismatch_total counter + apiserver_validation_declarative_validation_mismatch_total 1 + # 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 + ` + + if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total", "declarative_validation_panic_total"); err != nil { + t.Fatal(err) + } +} diff --git a/test/instrumentation/testdata/stable-metrics-list.yaml b/test/instrumentation/testdata/stable-metrics-list.yaml index b64e847be3d..151d2162c0a 100644 --- a/test/instrumentation/testdata/stable-metrics-list.yaml +++ b/test/instrumentation/testdata/stable-metrics-list.yaml @@ -122,6 +122,19 @@ - error_type - policy - policy_binding +- name: declarative_validation_mismatch_total + subsystem: validation + namespace: apiserver + help: Number of times declarative validation results differed from handwritten validation + results for core types. + type: Counter + stabilityLevel: BETA +- name: declarative_validation_panic_total + subsystem: validation + namespace: apiserver + help: Number of times declarative validation has panicked during validation. + type: Counter + stabilityLevel: BETA - name: disabled_metrics_total help: The count of disabled metrics. type: Counter