From 92dcd02459fcd3da8567023b576fa289b1b4373b Mon Sep 17 00:00:00 2001 From: Lalit Chauhan Date: Mon, 8 Sep 2025 17:06:51 +0000 Subject: [PATCH] Add helpers for declarative validation tests Introduces new testing helpers to simplify testing of declarative validation rules. The new `VerifyValidationEquivalence` and `VerifyUpdateValidationEquivalence` functions reduce boilerplate by encapsulating the logic for: - Toggling the `DeclarativeValidation` and `DeclarativeValidationTakeover` feature gates. - Comparing the validation output from the imperative and declarative paths. The declarative validation tests for CertificateSigningRequest and ReplicationController are updated to use these new, simpler helpers. --- pkg/api/testing/validation.go | 125 ++++++++++++++++-- .../validation/declarative_validation_test.go | 40 +----- .../declarative_validation_test.go | 77 +---------- .../declarative_validation_test.go | 77 +---------- 4 files changed, 126 insertions(+), 193 deletions(-) diff --git a/pkg/api/testing/validation.go b/pkg/api/testing/validation.go index ecf635f20d8..137e7e9a906 100644 --- a/pkg/api/testing/validation.go +++ b/pkg/api/testing/validation.go @@ -18,19 +18,29 @@ package testing import ( "bytes" + "context" "sort" "strconv" "testing" - k8sruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" runtimetest "k8s.io/apimachinery/pkg/runtime/testing" "k8s.io/apimachinery/pkg/util/validation/field" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/api/legacyscheme" + "k8s.io/kubernetes/pkg/features" ) +// ValidateFunc is a function that runs validation. +type ValidateFunc func(ctx context.Context, obj runtime.Object) field.ErrorList + +// ValidateUpdateFunc is a function that runs update validation. +type ValidateUpdateFunc func(ctx context.Context, obj, old runtime.Object) field.ErrorList + // VerifyVersionedValidationEquivalence tests that all versions of an API return equivalent validation errors. -func VerifyVersionedValidationEquivalence(t *testing.T, obj, old k8sruntime.Object, subresources ...string) { +func VerifyVersionedValidationEquivalence(t *testing.T, obj, old runtime.Object, subResources ...string) { t.Helper() // Accumulate errors from all versioned validation, per version. @@ -48,7 +58,7 @@ func VerifyVersionedValidationEquivalence(t *testing.T, obj, old k8sruntime.Obje return } if old == nil { - runtimetest.RunValidationForEachVersion(t, legacyscheme.Scheme, []string{}, internalObj, accumulate, subresources...) + runtimetest.RunValidationForEachVersion(t, legacyscheme.Scheme, []string{}, internalObj, accumulate, subResources...) } else { // Convert old versioned object to internal format before validation. // runtimetest.RunUpdateValidationForEachVersion requires unversioned (internal) objects as input. @@ -59,7 +69,7 @@ func VerifyVersionedValidationEquivalence(t *testing.T, obj, old k8sruntime.Obje if internalOld == nil { return } - runtimetest.RunUpdateValidationForEachVersion(t, legacyscheme.Scheme, []string{}, internalObj, internalOld, accumulate, subresources...) + runtimetest.RunUpdateValidationForEachVersion(t, legacyscheme.Scheme, []string{}, internalObj, internalOld, accumulate, subResources...) } // Make a copy so we can modify it. @@ -124,7 +134,7 @@ func fmtErrs(errs field.ErrorList) string { return buf.String() } -func convertToInternal(t *testing.T, scheme *k8sruntime.Scheme, obj k8sruntime.Object) (k8sruntime.Object, error) { +func convertToInternal(t *testing.T, scheme *runtime.Scheme, obj runtime.Object) (runtime.Object, error) { t.Helper() gvks, _, err := scheme.ObjectKinds(obj) @@ -135,13 +145,112 @@ func convertToInternal(t *testing.T, scheme *k8sruntime.Scheme, obj k8sruntime.O t.Fatal("no GVKs found for object") } gvk := gvks[0] - if gvk.Version == k8sruntime.APIVersionInternal { + if gvk.Version == runtime.APIVersionInternal { return obj, nil } - gvk.Version = k8sruntime.APIVersionInternal + gvk.Version = runtime.APIVersionInternal if !scheme.Recognizes(gvk) { t.Logf("no internal object found for GroupKind %s", gvk.GroupKind().String()) return nil, nil } - return scheme.ConvertToVersion(obj, schema.GroupVersion{Group: gvk.Group, Version: k8sruntime.APIVersionInternal}) + return scheme.ConvertToVersion(obj, schema.GroupVersion{Group: gvk.Group, Version: runtime.APIVersionInternal}) +} + +// 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. +// +// The function operates by running the provided validation function under two scenarios: +// 1. With DeclarativeValidation and DeclarativeValidationTakeover feature gates disabled, +// simulating the legacy hand-written validation. +// 2. With both feature gates enabled, using the new declarative validation rules. +// +// It then asserts that the validation errors produced in both scenarios are equivalent, +// 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) { + t.Helper() + verifyValidationEquivalence(t, expectedErrs, func() field.ErrorList { + return validateFn(ctx, obj) + }) + VerifyVersionedValidationEquivalence(t, obj, nil, subResources...) +} + +// VerifyUpdateValidationEquivalence provides a helper for testing the migration from +// hand-written imperative validation to declarative validation for update operations. +// It ensures that the validation logic remains consistent before and after the feature is enabled. +// +// The function operates by running the provided validation function under two scenarios: +// 1. With DeclarativeValidation and DeclarativeValidationTakeover feature gates disabled, +// simulating the legacy hand-written validation. +// 2. With both feature gates enabled, using the new declarative validation rules. +// +// It then asserts that the validation errors produced in both scenarios are equivalent, +// 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) { + t.Helper() + verifyValidationEquivalence(t, expectedErrs, func() field.ErrorList { + return validateUpdateFn(ctx, obj, old) + }) + VerifyVersionedValidationEquivalence(t, obj, old, 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) { + t.Helper() + var declarativeTakeoverErrs field.ErrorList + var imperativeErrs field.ErrorList + + for _, gateVal := range []bool{true, false} { + // We only need to test both gate enabled and disabled together, because + // 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled. + // 2) the validation output, when only DeclarativeValidation is enabled, is the same as when both gates are disabled. + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidation, gateVal) + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidationTakeover, gateVal) + errs := runValidations() + if gateVal { + declarativeTakeoverErrs = errs + } else { + imperativeErrs = errs + } + + // The errOutputMatcher is used to verify the output matches the expected errors in test cases. + errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() + if len(expectedErrs) > 0 { + errOutputMatcher.Test(t, expectedErrs, errs) + } else if len(errs) != 0 { + t.Errorf("expected no errors, but got: %v", errs) + } + } + + // 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() + + // 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. + imperativeErrs = deDuplicateErrors(imperativeErrs, equivalenceMatcher) + + equivalenceMatcher.Test(t, imperativeErrs, declarativeTakeoverErrs) +} + +// deDuplicateErrors removes duplicate errors from an ErrorList based on the provided matcher. +func deDuplicateErrors(errs field.ErrorList, matcher field.ErrorMatcher) field.ErrorList { + var deduped field.ErrorList + for _, err := range errs { + found := false + for _, existingErr := range deduped { + if matcher.Matches(existingErr, err) { + found = true + break + } + } + if !found { + deduped = append(deduped, err) + } + } + return deduped } diff --git a/pkg/apis/autoscaling/validation/declarative_validation_test.go b/pkg/apis/autoscaling/validation/declarative_validation_test.go index 1e46787be25..a42b948dc35 100644 --- a/pkg/apis/autoscaling/validation/declarative_validation_test.go +++ b/pkg/apis/autoscaling/validation/declarative_validation_test.go @@ -17,19 +17,17 @@ limitations under the License. package validation import ( - "fmt" + "context" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/api/legacyscheme" apitesting "k8s.io/kubernetes/pkg/api/testing" "k8s.io/kubernetes/pkg/apis/autoscaling" - "k8s.io/kubernetes/pkg/features" ) func TestValidateScaleForDeclarative(t *testing.T) { @@ -58,37 +56,9 @@ func TestValidateScaleForDeclarative(t *testing.T) { } for k, tc := range testCases { t.Run(k, func(t *testing.T) { - var declarativeTakeoverErrs field.ErrorList - var imperativeErrs field.ErrorList - for _, gateVal := range []bool{true, false} { - t.Run(fmt.Sprintf("gates=%v", gateVal), func(t *testing.T) { - // We only need to test both gate enabled and disabled together, because - // 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled. - // 2) the validation output, when only DeclarativeValidation is enabled, is the same as when both gates are disabled. - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidation, gateVal) - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidationTakeover, gateVal) - - errs := rest.ValidateDeclaratively(ctx, legacyscheme.Scheme, &tc.input) - if gateVal { - declarativeTakeoverErrs = errs - } else { - imperativeErrs = errs - } - // The errOutputMatcher is used to verify the output matches the expected errors in test cases. - errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() - if len(tc.expectedErrs) > 0 { - errOutputMatcher.Test(t, tc.expectedErrs, errs) - } else if len(errs) != 0 { - t.Errorf("expected no errors, but got: %v", errs) - } - }) - } - // The equivalenceMatcher is used to verify the output errors from handwritten imperative validation - // are equivalent to the output errors when DeclarativeValidationTakeover is enabled. - equivalenceMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() - equivalenceMatcher.Test(t, imperativeErrs, declarativeTakeoverErrs) - - apitesting.VerifyVersionedValidationEquivalence(t, &tc.input, nil, "scale") + apitesting.VerifyValidationEquivalence(t, ctx, &tc.input, func(ctx context.Context, obj runtime.Object) field.ErrorList { + return rest.ValidateDeclaratively(ctx, legacyscheme.Scheme, obj) + }, tc.expectedErrs, "scale") }) } } diff --git a/pkg/registry/certificates/certificates/declarative_validation_test.go b/pkg/registry/certificates/certificates/declarative_validation_test.go index 7e8fd4c59bc..d82e559eca6 100644 --- a/pkg/registry/certificates/certificates/declarative_validation_test.go +++ b/pkg/registry/certificates/certificates/declarative_validation_test.go @@ -29,12 +29,9 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" apitesting "k8s.io/kubernetes/pkg/api/testing" api "k8s.io/kubernetes/pkg/apis/certificates" "k8s.io/kubernetes/pkg/apis/core" - "k8s.io/kubernetes/pkg/features" "k8s.io/utils/ptr" ) @@ -93,35 +90,7 @@ func testDeclarativeValidateForDeclarative(t *testing.T, apiVersion string) { } for k, tc := range testCases { t.Run(k, func(t *testing.T) { - var declarativeTakeoverErrs field.ErrorList - var imperativeErrs field.ErrorList - for _, gateVal := range []bool{true, false} { - // We only need to test both gate enabled and disabled together, because - // 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled. - // 2) the validation output, when only DeclarativeValidation is enabled, is the same as when both gates are disabled. - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidation, gateVal) - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidationTakeover, gateVal) - - errs := Strategy.Validate(ctx, &tc.input) - if gateVal { - declarativeTakeoverErrs = errs - } else { - imperativeErrs = errs - } - // The errOutputMatcher is used to verify the output matches the expected errors in test cases. - errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() - if len(tc.expectedErrs) > 0 { - errOutputMatcher.Test(t, tc.expectedErrs, errs) - } else if len(errs) != 0 { - t.Errorf("expected no errors, but got: %v", errs) - } - } - // 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.Test(t, imperativeErrs, declarativeTakeoverErrs) - - apitesting.VerifyVersionedValidationEquivalence(t, &tc.input, nil) + apitesting.VerifyValidationEquivalence(t, ctx, &tc.input, Strategy.Validate, tc.expectedErrs) }) } } @@ -226,49 +195,7 @@ func testValidateUpdateForDeclarative(t *testing.T, apiVersion string) { tc.old.ResourceVersion = "1" tc.update.ResourceVersion = "1" - var declarativeTakeoverErrs field.ErrorList - var imperativeErrs field.ErrorList - for _, gateVal := range []bool{true, false} { - // We only need to test both gate enabled and disabled together, because - // 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled. - // 2) the validation output, when only DeclarativeValidation is enabled, is the same as when both gates are disabled. - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidation, gateVal) - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidationTakeover, gateVal) - errs := strategy.ValidateUpdate(ctx, &tc.update, &tc.old) - if gateVal { - declarativeTakeoverErrs = errs - } else { - imperativeErrs = errs - } - // The errOutputMatcher is used to verify the output matches the expected errors in test cases. - errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() - - if len(tc.expectedErrs) > 0 { - errOutputMatcher.Test(t, tc.expectedErrs, errs) - } else if len(errs) != 0 { - t.Errorf("expected no errors, but got: %v", errs) - } - } - // 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() - // TODO: remove this once ErrorMatcher has been extended to handle this form of deduplication. - dedupedImperativeErrs := field.ErrorList{} - for _, err := range imperativeErrs { - found := false - for _, existingErr := range dedupedImperativeErrs { - if equivalenceMatcher.Matches(existingErr, err) { - found = true - break - } - } - if !found { - dedupedImperativeErrs = append(dedupedImperativeErrs, err) - } - } - equivalenceMatcher.Test(t, dedupedImperativeErrs, declarativeTakeoverErrs) - - apitesting.VerifyVersionedValidationEquivalence(t, &tc.update, &tc.old) + apitesting.VerifyUpdateValidationEquivalence(t, ctx, &tc.update, &tc.old, strategy.ValidateUpdate, tc.expectedErrs) }) } } diff --git a/pkg/registry/core/replicationcontroller/declarative_validation_test.go b/pkg/registry/core/replicationcontroller/declarative_validation_test.go index 099b0402f42..e10b4c0555a 100644 --- a/pkg/registry/core/replicationcontroller/declarative_validation_test.go +++ b/pkg/registry/core/replicationcontroller/declarative_validation_test.go @@ -22,12 +22,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" podtest "k8s.io/kubernetes/pkg/api/pod/testing" apitesting "k8s.io/kubernetes/pkg/api/testing" api "k8s.io/kubernetes/pkg/apis/core" - "k8s.io/kubernetes/pkg/features" "k8s.io/utils/ptr" ) @@ -87,35 +84,7 @@ func TestDeclarativeValidateForDeclarative(t *testing.T) { } for k, tc := range testCases { t.Run(k, func(t *testing.T) { - var declarativeTakeoverErrs field.ErrorList - var imperativeErrs field.ErrorList - for _, gateVal := range []bool{true, false} { - // We only need to test both gate enabled and disabled together, because - // 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled. - // 2) the validation output, when only DeclarativeValidation is enabled, is the same as when both gates are disabled. - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidation, gateVal) - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidationTakeover, gateVal) - - errs := Strategy.Validate(ctx, &tc.input) - if gateVal { - declarativeTakeoverErrs = errs - } else { - imperativeErrs = errs - } - // The errOutputMatcher is used to verify the output matches the expected errors in test cases. - errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() - if len(tc.expectedErrs) > 0 { - errOutputMatcher.Test(t, tc.expectedErrs, errs) - } else if len(errs) != 0 { - t.Errorf("expected no errors, but got: %v", errs) - } - } - // 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.Test(t, imperativeErrs, declarativeTakeoverErrs) - - apitesting.VerifyVersionedValidationEquivalence(t, &tc.input, nil) + apitesting.VerifyValidationEquivalence(t, ctx, &tc.input, Strategy.Validate, tc.expectedErrs) }) } } @@ -189,49 +158,7 @@ func TestValidateUpdateForDeclarative(t *testing.T) { t.Run(k, func(t *testing.T) { tc.old.ObjectMeta.ResourceVersion = "1" tc.update.ObjectMeta.ResourceVersion = "1" - var declarativeTakeoverErrs field.ErrorList - var imperativeErrs field.ErrorList - for _, gateVal := range []bool{true, false} { - // We only need to test both gate enabled and disabled together, because - // 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled. - // 2) the validation output, when only DeclarativeValidation is enabled, is the same as when both gates are disabled. - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidation, gateVal) - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidationTakeover, gateVal) - errs := Strategy.ValidateUpdate(ctx, &tc.update, &tc.old) - if gateVal { - declarativeTakeoverErrs = errs - } else { - imperativeErrs = errs - } - // The errOutputMatcher is used to verify the output matches the expected errors in test cases. - errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() - - if len(tc.expectedErrs) > 0 { - errOutputMatcher.Test(t, tc.expectedErrs, errs) - } else if len(errs) != 0 { - t.Errorf("expected no errors, but got: %v", errs) - } - } - // 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() - // TODO: remove this once RC's validation is fixed to not return duplicate errors. - dedupedImperativeErrs := field.ErrorList{} - for _, err := range imperativeErrs { - found := false - for _, existingErr := range dedupedImperativeErrs { - if equivalenceMatcher.Matches(existingErr, err) { - found = true - break - } - } - if !found { - dedupedImperativeErrs = append(dedupedImperativeErrs, err) - } - } - equivalenceMatcher.Test(t, dedupedImperativeErrs, declarativeTakeoverErrs) - - apitesting.VerifyVersionedValidationEquivalence(t, &tc.update, &tc.old) + apitesting.VerifyUpdateValidationEquivalence(t, ctx, &tc.update, &tc.old, Strategy.ValidateUpdate, tc.expectedErrs) }) } }