Skip to content

Commit 9acdca6

Browse files
authored
Merge pull request kubernetes#130724 from jpbetz/replication-controller-to-declarative
Enable Declarative Validation for ReplicationController
2 parents 7f818e9 + ed08387 commit 9acdca6

File tree

5 files changed

+243
-1
lines changed

5 files changed

+243
-1
lines changed

pkg/api/testing/validation_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import (
3131
func TestVersionedValidationByFuzzing(t *testing.T) {
3232
typesWithDeclarativeValidation := []schema.GroupVersion{
3333
// Registered group versions for versioned validation fuzz testing:
34+
{Group: "", Version: "v1"},
3435
}
3536

3637
for _, gv := range typesWithDeclarativeValidation {

pkg/apis/core/v1/doc.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ limitations under the License.
1818
// +k8s:conversion-gen-external-types=k8s.io/api/core/v1
1919
// +k8s:defaulter-gen=TypeMeta
2020
// +k8s:defaulter-gen-input=k8s.io/api/core/v1
21+
// +k8s:validation-gen=TypeMeta
22+
// +k8s:validation-gen-input=k8s.io/api/core/v1
2123

2224
// Package v1 is the v1 version of the API.
2325
package v1

pkg/apis/core/v1/zz_generated.validations.go

Lines changed: 34 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package replicationcontroller
18+
19+
import (
20+
"testing"
21+
22+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
23+
"k8s.io/apimachinery/pkg/util/validation/field"
24+
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
25+
utilfeature "k8s.io/apiserver/pkg/util/feature"
26+
featuregatetesting "k8s.io/component-base/featuregate/testing"
27+
podtest "k8s.io/kubernetes/pkg/api/pod/testing"
28+
apitesting "k8s.io/kubernetes/pkg/api/testing"
29+
api "k8s.io/kubernetes/pkg/apis/core"
30+
"k8s.io/kubernetes/pkg/features"
31+
)
32+
33+
func TestDeclarativeValidateForDeclarative(t *testing.T) {
34+
ctx := genericapirequest.WithRequestInfo(genericapirequest.NewDefaultContext(), &genericapirequest.RequestInfo{
35+
APIGroup: "",
36+
APIVersion: "v1",
37+
})
38+
testCases := map[string]struct {
39+
input api.ReplicationController
40+
expectedErrs field.ErrorList
41+
}{
42+
"empty resource": {
43+
input: mkValidReplicationController(),
44+
},
45+
// TODO: Add test cases
46+
}
47+
for k, tc := range testCases {
48+
t.Run(k, func(t *testing.T) {
49+
var declarativeTakeoverErrs field.ErrorList
50+
var imperativeErrs field.ErrorList
51+
for _, gateVal := range []bool{true, false} {
52+
// We only need to test both gate enabled and disabled together, because
53+
// 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled.
54+
// 2) the validation output, when only DeclarativeValidation is enabled, is the same as when both gates are disabled.
55+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidation, gateVal)
56+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidationTakeover, gateVal)
57+
58+
errs := Strategy.Validate(ctx, &tc.input)
59+
if gateVal {
60+
declarativeTakeoverErrs = errs
61+
} else {
62+
imperativeErrs = errs
63+
}
64+
// The errOutputMatcher is used to verify the output matches the expected errors in test cases.
65+
errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin()
66+
if len(tc.expectedErrs) > 0 {
67+
errOutputMatcher.Test(t, tc.expectedErrs, errs)
68+
} else if len(errs) != 0 {
69+
t.Errorf("expected no errors, but got: %v", errs)
70+
}
71+
}
72+
// The equivalenceMatcher is used to verify the output errors from hand-written imperative validation
73+
// are equivalent to the output errors when DeclarativeValidationTakeover is enabled.
74+
equivalenceMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin()
75+
equivalenceMatcher.Test(t, imperativeErrs, declarativeTakeoverErrs)
76+
77+
apitesting.VerifyVersionedValidationEquivalence(t, &tc.input, nil)
78+
})
79+
}
80+
}
81+
82+
func TestValidateUpdateForDeclarative(t *testing.T) {
83+
ctx := genericapirequest.WithRequestInfo(genericapirequest.NewDefaultContext(), &genericapirequest.RequestInfo{
84+
APIGroup: "",
85+
APIVersion: "v1",
86+
})
87+
testCases := map[string]struct {
88+
old api.ReplicationController
89+
update api.ReplicationController
90+
expectedErrs field.ErrorList
91+
}{
92+
// TODO: Add test cases
93+
}
94+
for k, tc := range testCases {
95+
t.Run(k, func(t *testing.T) {
96+
tc.old.ObjectMeta.ResourceVersion = "1"
97+
tc.update.ObjectMeta.ResourceVersion = "1"
98+
var declarativeTakeoverErrs field.ErrorList
99+
var imperativeErrs field.ErrorList
100+
for _, gateVal := range []bool{true, false} {
101+
// We only need to test both gate enabled and disabled together, because
102+
// 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled.
103+
// 2) the validation output, when only DeclarativeValidation is enabled, is the same as when both gates are disabled.
104+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidation, gateVal)
105+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidationTakeover, gateVal)
106+
errs := Strategy.ValidateUpdate(ctx, &tc.update, &tc.old)
107+
if gateVal {
108+
declarativeTakeoverErrs = errs
109+
} else {
110+
imperativeErrs = errs
111+
}
112+
// The errOutputMatcher is used to verify the output matches the expected errors in test cases.
113+
errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin()
114+
115+
if len(tc.expectedErrs) > 0 {
116+
errOutputMatcher.Test(t, tc.expectedErrs, errs)
117+
} else if len(errs) != 0 {
118+
t.Errorf("expected no errors, but got: %v", errs)
119+
}
120+
}
121+
// The equivalenceMatcher is used to verify the output errors from hand-written imperative validation
122+
// are equivalent to the output errors when DeclarativeValidationTakeover is enabled.
123+
equivalenceMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin()
124+
// TODO: remove this once ErrorMatcher has been extended to handle this form of deduplication.
125+
dedupedImperativeErrs := field.ErrorList{}
126+
for _, err := range imperativeErrs {
127+
found := false
128+
for _, existingErr := range dedupedImperativeErrs {
129+
if equivalenceMatcher.Matches(existingErr, err) {
130+
found = true
131+
break
132+
}
133+
}
134+
if !found {
135+
dedupedImperativeErrs = append(dedupedImperativeErrs, err)
136+
}
137+
}
138+
equivalenceMatcher.Test(t, dedupedImperativeErrs, declarativeTakeoverErrs)
139+
140+
apitesting.VerifyVersionedValidationEquivalence(t, &tc.update, &tc.old)
141+
})
142+
}
143+
}
144+
145+
// Helper function for RC tests.
146+
func mkValidReplicationController(tweaks ...func(rc *api.ReplicationController)) api.ReplicationController {
147+
rc := api.ReplicationController{
148+
ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
149+
Spec: api.ReplicationControllerSpec{
150+
Replicas: 1,
151+
Selector: map[string]string{"a": "b"},
152+
Template: &api.PodTemplateSpec{
153+
ObjectMeta: metav1.ObjectMeta{
154+
Labels: map[string]string{"a": "b"},
155+
},
156+
Spec: podtest.MakePodSpec(),
157+
},
158+
},
159+
}
160+
for _, tweak := range tweaks {
161+
tweak(&rc)
162+
}
163+
return rc
164+
}

pkg/registry/core/replicationcontroller/strategy.go

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,13 @@ import (
3737
"k8s.io/apiserver/pkg/registry/rest"
3838
apistorage "k8s.io/apiserver/pkg/storage"
3939
"k8s.io/apiserver/pkg/storage/names"
40+
utilfeature "k8s.io/apiserver/pkg/util/feature"
4041
"k8s.io/kubernetes/pkg/api/legacyscheme"
4142
"k8s.io/kubernetes/pkg/api/pod"
4243
api "k8s.io/kubernetes/pkg/apis/core"
4344
"k8s.io/kubernetes/pkg/apis/core/helper"
4445
corevalidation "k8s.io/kubernetes/pkg/apis/core/validation"
46+
"k8s.io/kubernetes/pkg/features"
4547
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
4648
)
4749

@@ -123,7 +125,27 @@ func (rcStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object)
123125
func (rcStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
124126
controller := obj.(*api.ReplicationController)
125127
opts := pod.GetValidationOptionsFromPodTemplate(controller.Spec.Template, nil)
126-
return corevalidation.ValidateReplicationController(controller, opts)
128+
129+
// Run imperative validation
130+
allErrs := corevalidation.ValidateReplicationController(controller, opts)
131+
132+
// If DeclarativeValidation feature gate is enabled, also run declarative validation
133+
if utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidation) {
134+
// Determine if takeover is enabled
135+
takeover := utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidationTakeover)
136+
137+
// Run declarative validation with panic recovery
138+
declarativeErrs := rest.ValidateDeclarativelyWithRecovery(ctx, nil, legacyscheme.Scheme, controller, takeover)
139+
140+
// Compare imperative and declarative errors and log + emit metric if there's a mismatch
141+
rest.CompareDeclarativeErrorsAndEmitMismatches(ctx, allErrs, declarativeErrs, takeover)
142+
143+
// Only apply declarative errors if takeover is enabled
144+
if takeover {
145+
allErrs = append(allErrs.RemoveCoveredByDeclarative(), declarativeErrs...)
146+
}
147+
}
148+
return allErrs
127149
}
128150

129151
// 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
153175
newRc := obj.(*api.ReplicationController)
154176

155177
opts := pod.GetValidationOptionsFromPodTemplate(newRc.Spec.Template, oldRc.Spec.Template)
178+
// FIXME: Calling both validator functions here results in redundant calls to ValidateReplicationControllerSpec.
179+
// This should be fixed to avoid the redundant calls, but carefully.
156180
validationErrorList := corevalidation.ValidateReplicationController(newRc, opts)
157181
updateErrorList := corevalidation.ValidateReplicationControllerUpdate(newRc, oldRc, opts)
158182
errs := append(validationErrorList, updateErrorList...)
@@ -174,6 +198,23 @@ func (rcStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) f
174198
}
175199
}
176200

201+
// If DeclarativeValidation feature gate is enabled, also run declarative validation
202+
if utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidation) {
203+
// Determine if takeover is enabled
204+
takeover := utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidationTakeover)
205+
206+
// Run declarative update validation with panic recovery
207+
declarativeErrs := rest.ValidateUpdateDeclarativelyWithRecovery(ctx, nil, legacyscheme.Scheme, newRc, oldRc, takeover)
208+
209+
// Compare imperative and declarative errors and emit metric if there's a mismatch
210+
rest.CompareDeclarativeErrorsAndEmitMismatches(ctx, errs, declarativeErrs, takeover)
211+
212+
// Only apply declarative errors if takeover is enabled
213+
if takeover {
214+
errs = append(errs.RemoveCoveredByDeclarative(), declarativeErrs...)
215+
}
216+
}
217+
177218
return errs
178219
}
179220

0 commit comments

Comments
 (0)