diff --git a/pkg/api/testing/validation_test.go b/pkg/api/testing/validation_test.go index 868f1adf41e..89330835255 100644 --- a/pkg/api/testing/validation_test.go +++ b/pkg/api/testing/validation_test.go @@ -31,6 +31,7 @@ import ( func TestVersionedValidationByFuzzing(t *testing.T) { typesWithDeclarativeValidation := []schema.GroupVersion{ // Registered group versions for versioned validation fuzz testing: + {Group: "", Version: "v1"}, } for _, gv := range typesWithDeclarativeValidation { diff --git a/pkg/apis/core/v1/doc.go b/pkg/apis/core/v1/doc.go index af7c0a8492e..8790796620f 100644 --- a/pkg/apis/core/v1/doc.go +++ b/pkg/apis/core/v1/doc.go @@ -18,6 +18,8 @@ limitations under the License. // +k8s:conversion-gen-external-types=k8s.io/api/core/v1 // +k8s:defaulter-gen=TypeMeta // +k8s:defaulter-gen-input=k8s.io/api/core/v1 +// +k8s:validation-gen=TypeMeta +// +k8s:validation-gen-input=k8s.io/api/core/v1 // Package v1 is the v1 version of the API. package v1 diff --git a/pkg/apis/core/v1/zz_generated.validations.go b/pkg/apis/core/v1/zz_generated.validations.go new file mode 100644 index 00000000000..73e28035dba --- /dev/null +++ b/pkg/apis/core/v1/zz_generated.validations.go @@ -0,0 +1,34 @@ +//go:build !ignore_autogenerated +// +build !ignore_autogenerated + +/* +Copyright 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. +*/ + +// Code generated by validation-gen. DO NOT EDIT. + +package v1 + +import ( + runtime "k8s.io/apimachinery/pkg/runtime" +) + +func init() { localSchemeBuilder.Register(RegisterValidations) } + +// RegisterValidations adds validation functions to the given scheme. +// Public to allow building arbitrary schemes. +func RegisterValidations(scheme *runtime.Scheme) error { + return nil +} diff --git a/pkg/registry/core/replicationcontroller/declarative_validation_test.go b/pkg/registry/core/replicationcontroller/declarative_validation_test.go new file mode 100644 index 00000000000..4c06439d7f4 --- /dev/null +++ b/pkg/registry/core/replicationcontroller/declarative_validation_test.go @@ -0,0 +1,164 @@ +/* +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 replicationcontroller + +import ( + "testing" + + 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" +) + +func TestDeclarativeValidateForDeclarative(t *testing.T) { + ctx := genericapirequest.WithRequestInfo(genericapirequest.NewDefaultContext(), &genericapirequest.RequestInfo{ + APIGroup: "", + APIVersion: "v1", + }) + testCases := map[string]struct { + input api.ReplicationController + expectedErrs field.ErrorList + }{ + "empty resource": { + input: mkValidReplicationController(), + }, + // TODO: Add test cases + } + 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) + }) + } +} + +func TestValidateUpdateForDeclarative(t *testing.T) { + ctx := genericapirequest.WithRequestInfo(genericapirequest.NewDefaultContext(), &genericapirequest.RequestInfo{ + APIGroup: "", + APIVersion: "v1", + }) + testCases := map[string]struct { + old api.ReplicationController + update api.ReplicationController + expectedErrs field.ErrorList + }{ + // TODO: Add test cases + } + for k, tc := range testCases { + 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 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) + }) + } +} + +// Helper function for RC tests. +func mkValidReplicationController(tweaks ...func(rc *api.ReplicationController)) api.ReplicationController { + rc := api.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: api.ReplicationControllerSpec{ + Replicas: 1, + Selector: map[string]string{"a": "b"}, + Template: &api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"a": "b"}, + }, + Spec: podtest.MakePodSpec(), + }, + }, + } + for _, tweak := range tweaks { + tweak(&rc) + } + return rc +} diff --git a/pkg/registry/core/replicationcontroller/strategy.go b/pkg/registry/core/replicationcontroller/strategy.go index cdb32dd5f28..734b13d094e 100644 --- a/pkg/registry/core/replicationcontroller/strategy.go +++ b/pkg/registry/core/replicationcontroller/strategy.go @@ -37,11 +37,13 @@ import ( "k8s.io/apiserver/pkg/registry/rest" apistorage "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/api/pod" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/helper" corevalidation "k8s.io/kubernetes/pkg/apis/core/validation" + "k8s.io/kubernetes/pkg/features" "sigs.k8s.io/structured-merge-diff/v4/fieldpath" ) @@ -123,7 +125,27 @@ func (rcStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) func (rcStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { controller := obj.(*api.ReplicationController) opts := pod.GetValidationOptionsFromPodTemplate(controller.Spec.Template, nil) - return corevalidation.ValidateReplicationController(controller, opts) + + // Run imperative validation + allErrs := corevalidation.ValidateReplicationController(controller, opts) + + // If DeclarativeValidation feature gate is enabled, also run declarative validation + if utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidation) { + // Determine if takeover is enabled + takeover := utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidationTakeover) + + // Run declarative validation with panic recovery + declarativeErrs := rest.ValidateDeclarativelyWithRecovery(ctx, nil, legacyscheme.Scheme, controller, takeover) + + // Compare imperative and declarative errors and log + emit metric if there's a mismatch + rest.CompareDeclarativeErrorsAndEmitMismatches(ctx, allErrs, declarativeErrs, takeover) + + // Only apply declarative errors if takeover is enabled + if takeover { + allErrs = append(allErrs.RemoveCoveredByDeclarative(), declarativeErrs...) + } + } + return allErrs } // WarningsOnCreate returns warnings for the creation of the given object. @@ -153,6 +175,8 @@ func (rcStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) f newRc := obj.(*api.ReplicationController) opts := pod.GetValidationOptionsFromPodTemplate(newRc.Spec.Template, oldRc.Spec.Template) + // FIXME: Calling both validator functions here results in redundant calls to ValidateReplicationControllerSpec. + // This should be fixed to avoid the redundant calls, but carefully. validationErrorList := corevalidation.ValidateReplicationController(newRc, opts) updateErrorList := corevalidation.ValidateReplicationControllerUpdate(newRc, oldRc, opts) errs := append(validationErrorList, updateErrorList...) @@ -174,6 +198,23 @@ func (rcStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) f } } + // If DeclarativeValidation feature gate is enabled, also run declarative validation + if utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidation) { + // Determine if takeover is enabled + takeover := utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidationTakeover) + + // Run declarative update validation with panic recovery + declarativeErrs := rest.ValidateUpdateDeclarativelyWithRecovery(ctx, nil, legacyscheme.Scheme, newRc, oldRc, takeover) + + // Compare imperative and declarative errors and emit metric if there's a mismatch + rest.CompareDeclarativeErrorsAndEmitMismatches(ctx, errs, declarativeErrs, takeover) + + // Only apply declarative errors if takeover is enabled + if takeover { + errs = append(errs.RemoveCoveredByDeclarative(), declarativeErrs...) + } + } + return errs }