From 9a7e2291d1ff00584b0078870dab1f04fbf03592 Mon Sep 17 00:00:00 2001 From: yongruilin Date: Fri, 19 Sep 2025 19:58:26 +0000 Subject: [PATCH] refactor: Remove Validate(Update)Declaratively and improve error handling --- .../validation/declarative_validation_test.go | 11 ++-- .../apiserver/pkg/registry/rest/validate.go | 37 ++--------- .../pkg/registry/rest/validate_test.go | 65 +++---------------- 3 files changed, 21 insertions(+), 92 deletions(-) diff --git a/pkg/apis/autoscaling/validation/declarative_validation_test.go b/pkg/apis/autoscaling/validation/declarative_validation_test.go index 056e10ecb70..2c17bc0827b 100644 --- a/pkg/apis/autoscaling/validation/declarative_validation_test.go +++ b/pkg/apis/autoscaling/validation/declarative_validation_test.go @@ -20,9 +20,9 @@ import ( "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" - "k8s.io/apiserver/pkg/registry/rest" "k8s.io/kubernetes/pkg/api/legacyscheme" apitesting "k8s.io/kubernetes/pkg/api/testing" "k8s.io/kubernetes/pkg/apis/autoscaling" @@ -30,9 +30,11 @@ import ( // TestScaleDeclarativeValidation verifies that the validation rules that are applied uniformly to the Scale API. func TestScaleDeclarativeValidation(t *testing.T) { + apiGroup := "autoscaling" + apiVersion := "v1" ctx := genericapirequest.WithRequestInfo(genericapirequest.NewDefaultContext(), &genericapirequest.RequestInfo{ - APIGroup: "autoscaling", - APIVersion: "v1", + APIGroup: apiGroup, + APIVersion: apiVersion, Subresource: "scale", }) @@ -61,7 +63,8 @@ func TestScaleDeclarativeValidation(t *testing.T) { // All resources that have a scale subresource are expected to test Scale validation against any handwritten // validation code defined on that resource. tester := field.ErrorMatcher{}.ByType().ByField().ByOrigin() - tester.Test(t, rest.ValidateDeclaratively(ctx, legacyscheme.Scheme, &tc.input), tc.expectedErrs) + obj, _ := legacyscheme.Scheme.ConvertToVersion(&tc.input, schema.GroupVersion{Group: apiGroup, Version: apiVersion}) + tester.Test(t, tc.expectedErrs, legacyscheme.Scheme.Validate(ctx, nil, obj, "scale")) apitesting.VerifyVersionedValidationEquivalence(t, &tc.input, nil, "scale") }) 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 a59ee37d584..1a0dd38490b 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go @@ -90,28 +90,7 @@ type validationConfigOption struct { validationIdentifier string } -// ValidateDeclaratively validates obj against declarative validation tags -// defined in its Go type. 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. -// -// Returns a field.ErrorList containing any validation errors. An internal error -// is included if requestInfo is missing from the context or if version -// conversion fails. -func ValidateDeclaratively(ctx context.Context, scheme *runtime.Scheme, obj runtime.Object, configOpts ...ValidationConfig) field.ErrorList { - cfg := &validationConfigOption{opType: operation.Create} - for _, o := range configOpts { - o(cfg) - } - - return panicSafeValidateFunc(validateDeclaratively, cfg.takeover, cfg.validationIdentifier)(ctx, scheme, obj, nil, cfg) -} - -// ValidateUpdateDeclaratively validates obj and oldObj against declarative +// validateDeclaratively validates obj and oldObj against declarative // validation tags defined in its Go type. It uses the API version extracted from // ctx and the provided scheme for validation. // @@ -123,14 +102,6 @@ func ValidateDeclaratively(ctx context.Context, scheme *runtime.Scheme, obj runt // Returns a field.ErrorList containing any validation errors. An internal error // is included if requestInfo is missing from the context or if version // conversion fails. -func ValidateUpdateDeclaratively(ctx context.Context, scheme *runtime.Scheme, obj, oldObj runtime.Object, configOpts ...ValidationConfig) field.ErrorList { - cfg := &validationConfigOption{opType: operation.Update} - for _, o := range configOpts { - o(cfg) - } - return panicSafeValidateFunc(validateDeclaratively, cfg.takeover, cfg.validationIdentifier)(ctx, scheme, obj, oldObj, cfg) -} - func validateDeclaratively(ctx context.Context, scheme *runtime.Scheme, obj, oldObj runtime.Object, o *validationConfigOption) field.ErrorList { // Find versionedGroupVersion, which identifies the API version to use for declarative validation. versionedGroupVersion, subresources, err := requestInfo(ctx, o.subresourceGVKMapper) @@ -187,9 +158,9 @@ func parseSubresourcePath(subresourcePath string) ([]string, error) { return parts, nil } -// CompareDeclarativeErrorsAndEmitMismatches checks for mismatches between imperative and declarative validation +// 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) { logger := klog.FromContext(ctx) mismatchDetails := gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs, takeover) for _, detail := range mismatchDetails { @@ -394,7 +365,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) 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 27a8f3f38d2..6de4d2f3e09 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 @@ -162,12 +162,16 @@ func TestValidateDeclaratively(t *testing.T) { Subresource: tc.subresource, }) t.Run(tc.name, func(t *testing.T) { - var results field.ErrorList - if tc.oldObject == nil { - results = ValidateDeclaratively(ctx, scheme, tc.object, WithOptions(tc.options)) - } else { - results = ValidateUpdateDeclaratively(ctx, scheme, tc.object, tc.oldObject, WithOptions(tc.options)) + + cfg := &validationConfigOption{ + options: tc.options, } + if tc.oldObject == nil { + cfg.opType = operation.Create + } else { + cfg.opType = operation.Update + } + results := panicSafeValidateFunc(validateDeclaratively, cfg.takeover, cfg.validationIdentifier)(ctx, scheme, tc.object, tc.oldObject, cfg) matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() matcher.Test(t, tc.expected, results) }) @@ -425,7 +429,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") klog.Flush() logOutput := buf.String() @@ -632,55 +636,6 @@ func TestWithRecoverUpdate(t *testing.T) { } } -func TestValidateDeclarativelyWithRecovery(t *testing.T) { - ctx := context.Background() - scheme := runtime.NewScheme() - var options []string - obj := &runtime.Unknown{} - - // Simple test for the ValidateDeclarativelyWithRecovery function - t.Run("with takeover disabled", func(t *testing.T) { - errs := ValidateDeclaratively(ctx, scheme, obj, WithOptions(options), WithTakeover(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 := ValidateDeclaratively(ctx, scheme, obj, WithOptions(options), WithTakeover(true)) - if errs == nil { - // This is expected to error since the request info is missioptionsng - t.Errorf("Expected errors but got nil") - } - }) -} - -func TestValidateUpdateDeclarativelyWithRecovery(t *testing.T) { - ctx := context.Background() - scheme := runtime.NewScheme() - var options []string - obj := &runtime.Unknown{} - oldObj := &runtime.Unknown{} - - // Simple test for the ValidateUpdateDeclarativelyWithRecovery function - t.Run("with takeover disabled", func(t *testing.T) { - errs := ValidateUpdateDeclaratively(ctx, scheme, obj, oldObj, WithOptions(options), WithTakeover(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 := ValidateUpdateDeclaratively(ctx, scheme, obj, oldObj, WithOptions(options), WithTakeover(true)) - if errs == nil { - // This is expected to error since the request info is missing - t.Errorf("Expected errors but got nil") - } - }) -} - func TestRecordDuplicateValidationErrors(t *testing.T) { ctx := context.Background()