Merge pull request #130724 from jpbetz/replication-controller-to-declarative

Enable Declarative Validation for ReplicationController

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 {

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

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
+}

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
+}

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
 }