diff --git a/pkg/api/testing/validation.go b/pkg/api/testing/validation.go index 5a87eb97757..753fe877439 100644 --- a/pkg/api/testing/validation.go +++ b/pkg/api/testing/validation.go @@ -156,6 +156,28 @@ func convertToInternal(t *testing.T, scheme *runtime.Scheme, obj runtime.Object) return scheme.ConvertToVersion(obj, schema.GroupVersion{Group: gvk.Group, Version: runtime.APIVersionInternal}) } +type ValidationTestConfig func(*validationOption) + +// validationOptions encapsulates optional parameters for validation equivalence tests. +type validationOption struct { + // SubResources are the subresources to validate. + SubResources []string + // NormalizationRules are the rules to apply to field paths before comparison. + NormalizationRules []field.NormalizationRule +} + +func WithSubResources(subResources ...string) ValidationTestConfig { + return func(o *validationOption) { + o.SubResources = subResources + } +} + +func WithNormalizationRules(rules ...field.NormalizationRule) ValidationTestConfig { + return func(o *validationOption) { + o.NormalizationRules = rules + } +} + // VerifyValidationEquivalence provides a helper for testing the migration from // hand-written imperative validation to declarative validation. It ensures that // the validation logic remains consistent before and after the feature is enabled. @@ -169,12 +191,16 @@ func convertToInternal(t *testing.T, scheme *runtime.Scheme, obj runtime.Object) // guaranteeing a safe migration. It also checks the errors against an expected set. // It compares errors by field, origin and type; all three should match to be called equivalent. // It also make sure all versions of the given API returns equivalent errors. -func VerifyValidationEquivalence(t *testing.T, ctx context.Context, obj runtime.Object, validateFn ValidateFunc, expectedErrs field.ErrorList, subResources ...string) { +func VerifyValidationEquivalence(t *testing.T, ctx context.Context, obj runtime.Object, validateFn ValidateFunc, expectedErrs field.ErrorList, testConfigs ...ValidationTestConfig) { t.Helper() + opts := &validationOption{} + for _, testcfg := range testConfigs { + testcfg(opts) + } verifyValidationEquivalence(t, expectedErrs, func() field.ErrorList { return validateFn(ctx, obj) - }) - VerifyVersionedValidationEquivalence(t, obj, nil, subResources...) + }, opts) + VerifyVersionedValidationEquivalence(t, obj, nil, opts.SubResources...) } // VerifyUpdateValidationEquivalence provides a helper for testing the migration from @@ -190,22 +216,31 @@ func VerifyValidationEquivalence(t *testing.T, ctx context.Context, obj runtime. // guaranteeing a safe migration. It also checks the errors against an expected set. // It compares errors by field, origin and type; all three should match to be called equivalent. // It also make sure all versions of the given API returns equivalent errors. -func VerifyUpdateValidationEquivalence(t *testing.T, ctx context.Context, obj, old runtime.Object, validateUpdateFn ValidateUpdateFunc, expectedErrs field.ErrorList, subResources ...string) { +func VerifyUpdateValidationEquivalence(t *testing.T, ctx context.Context, obj, old runtime.Object, validateUpdateFn ValidateUpdateFunc, expectedErrs field.ErrorList, testConfigs ...ValidationTestConfig) { t.Helper() + opts := &validationOption{} + for _, testcfg := range testConfigs { + testcfg(opts) + } verifyValidationEquivalence(t, expectedErrs, func() field.ErrorList { return validateUpdateFn(ctx, obj, old) - }) - VerifyVersionedValidationEquivalence(t, obj, old, subResources...) + }, opts) + VerifyVersionedValidationEquivalence(t, obj, old, opts.SubResources...) } // verifyValidationEquivalence is a generic helper that verifies validation equivalence with and without declarative validation. -func verifyValidationEquivalence(t *testing.T, expectedErrs field.ErrorList, runValidations func() field.ErrorList) { +func verifyValidationEquivalence(t *testing.T, expectedErrs field.ErrorList, runValidations func() field.ErrorList, opt *validationOption) { t.Helper() var declarativeTakeoverErrs field.ErrorList var imperativeErrs field.ErrorList // The errOutputMatcher is used to verify the output matches the expected errors in test cases. - errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() + errOutputMatcher := field.ErrorMatcher{}.ByType().ByOrigin() + if len(opt.NormalizationRules) > 0 { + errOutputMatcher = errOutputMatcher.ByFieldNormalized(opt.NormalizationRules) + } else { + errOutputMatcher = errOutputMatcher.ByField() + } // We only need to test both gate enabled and disabled together, because // 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled. @@ -241,7 +276,12 @@ func verifyValidationEquivalence(t *testing.T, expectedErrs field.ErrorList, run // The equivalenceMatcher is used to verify the output errors from hand-written imperative validation // are equivalent to the output errors when DeclarativeValidationTakeover is enabled. - equivalenceMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() + equivalenceMatcher := field.ErrorMatcher{}.ByType().ByOrigin() + if len(opt.NormalizationRules) > 0 { + equivalenceMatcher = equivalenceMatcher.ByFieldNormalized(opt.NormalizationRules) + } else { + equivalenceMatcher = equivalenceMatcher.ByField() + } // The imperative validation may produce duplicate errors, which is not supported by the ErrorMatcher. // TODO: remove this once ErrorMatcher has been extended to handle this form of deduplication. 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 18d6bfd2e63..04ff4a5018c 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go @@ -68,12 +68,20 @@ func WithSubresourceMapper(subresourceMapper GroupVersionKindProvider) Validatio } } +// WithNormalizationRules sets the normalization rules for validation. +func WithNormalizationRules(rules []field.NormalizationRule) ValidationConfig { + return func(config *validationConfigOption) { + config.normalizationRules = rules + } +} + type validationConfigOption struct { opType operation.Type options []string takeover bool subresourceGVKMapper GroupVersionKindProvider validationIdentifier string + normalizationRules []field.NormalizationRule } // validateDeclaratively validates obj and oldObj against declarative @@ -146,9 +154,9 @@ func parseSubresourcePath(subresourcePath string) ([]string, error) { // 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, validationIdentifier string) { +func compareDeclarativeErrorsAndEmitMismatches(ctx context.Context, imperativeErrs, declarativeErrs field.ErrorList, takeover bool, validationIdentifier string, normalizationRules []field.NormalizationRule) { logger := klog.FromContext(ctx) - mismatchDetails := gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs, takeover) + mismatchDetails := gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs, takeover, normalizationRules) for _, detail := range mismatchDetails { // Log information about the mismatch using contextual logger logger.Error(nil, detail) @@ -160,7 +168,7 @@ func compareDeclarativeErrorsAndEmitMismatches(ctx context.Context, imperativeEr // 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 { +func gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs field.ErrorList, takeover bool, normalizationRules []field.NormalizationRule) []string { var mismatchDetails []string // short circuit here to minimize allocs for usual case of 0 validation errors if len(imperativeErrs) == 0 && len(declarativeErrs) == 0 { @@ -171,7 +179,12 @@ func gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs field 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() + fuzzyMatcher := field.ErrorMatcher{}.ByType().ByOrigin().RequireOriginWhenInvalid() + if len(normalizationRules) > 0 { + fuzzyMatcher = fuzzyMatcher.ByFieldNormalized(normalizationRules) + } else { + fuzzyMatcher = fuzzyMatcher.ByField() + } exactMatcher := field.ErrorMatcher{}.Exactly() // Dedupe imperative errors of exact error matches as they are @@ -353,8 +366,7 @@ func ValidateDeclarativelyWithMigrationChecks(ctx context.Context, scheme *runti // Call the panic-safe wrapper with the real validation function. declarativeErrs := panicSafeValidateFunc(validateDeclaratively, cfg.takeover, cfg.validationIdentifier)(ctx, scheme, obj, oldObj, cfg) - compareDeclarativeErrorsAndEmitMismatches(ctx, errs, declarativeErrs, takeover, validationIdentifier) - + compareDeclarativeErrorsAndEmitMismatches(ctx, errs, declarativeErrs, takeover, validationIdentifier, cfg.normalizationRules) if takeover { errs = append(errs.RemoveCoveredByDeclarative(), declarativeErrs...) } 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 6de4d2f3e09..81a1786abf3 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 @@ -213,6 +213,7 @@ func TestGatherDeclarativeValidationMismatches(t *testing.T) { 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") + errBWithDiffPath := field.Invalid(field.NewPath("spec").Child("fakeminReadySeconds"), -1, "covered error B").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") @@ -227,6 +228,7 @@ func TestGatherDeclarativeValidationMismatches(t *testing.T) { takeover bool expectMismatches bool expectDetailsContaining []string + normalizedRules []field.NormalizationRule }{ { name: "Declarative and imperative return 0 errors - no mismatch", @@ -358,11 +360,29 @@ func TestGatherDeclarativeValidationMismatches(t *testing.T) { expectMismatches: false, expectDetailsContaining: []string{}, }, + { + name: "Field normalization, errors don't match - mismatch", + imperativeErrors: field.ErrorList{ + coveredErrB, + }, + declarativeErrors: field.ErrorList{ + errBWithDiffPath, + }, + normalizedRules: []field.NormalizationRule{ + { + Regexp: regexp.MustCompile(`spec.fakeminReadySeconds`), + Replacement: "spec.minReadySeconds", + }, + }, + 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) + details := gatherDeclarativeValidationMismatches(tc.imperativeErrors, tc.declarativeErrors, tc.takeover, tc.normalizedRules) // Check if mismatches were found if expected if tc.expectMismatches && len(details) == 0 { t.Errorf("Expected mismatches but got none") @@ -429,7 +449,7 @@ func TestCompareDeclarativeErrorsAndEmitMismatches(t *testing.T) { defer klog.LogToStderr(true) ctx := context.Background() - compareDeclarativeErrorsAndEmitMismatches(ctx, tc.imperativeErrs, tc.declarativeErrs, tc.takeover, "test_validationIdentifier") + compareDeclarativeErrorsAndEmitMismatches(ctx, tc.imperativeErrs, tc.declarativeErrs, tc.takeover, "test_validationIdentifier", nil) klog.Flush() logOutput := buf.String()