Skip to content

Commit 7315d0a

Browse files
thockinthockin
committed
Change internal-version RC.Spec.Replicas to a ptr
This is needed to make declaratve validation clean. Past me thought this was clever (pointer versioned, non-pointer internal) but it is just confusing.
1 parent 68899f8 commit 7315d0a

File tree

13 files changed

+89
-46
lines changed

13 files changed

+89
-46
lines changed

‎pkg/apis/core/fuzzer/fuzzer.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,6 @@ import (
2121
"strconv"
2222
"time"
2323

24-
"sigs.k8s.io/randfill"
25-
2624
v1 "k8s.io/api/core/v1"
2725
"k8s.io/apimachinery/pkg/api/resource"
2826
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -32,6 +30,7 @@ import (
3230
"k8s.io/apimachinery/pkg/util/intstr"
3331
"k8s.io/kubernetes/pkg/apis/core"
3432
"k8s.io/utils/ptr"
33+
"sigs.k8s.io/randfill"
3534
)
3635

3736
// Funcs returns the fuzzer functions for the core group.
@@ -119,6 +118,12 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} {
119118
func(j *core.ReplicationControllerSpec, c randfill.Continue) {
120119
c.FillNoCustom(j) // fuzz self without calling this function again
121120
//j.TemplateRef = nil // this is required for round trip
121+
122+
// match defaulting
123+
if j.Replicas == nil {
124+
replicas := int32(0)
125+
j.Replicas = &replicas
126+
}
122127
},
123128
func(j *core.List, c randfill.Continue) {
124129
c.FillNoCustom(j) // fuzz self without calling this function again

‎pkg/apis/core/types.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4332,7 +4332,7 @@ type PodTemplateList struct {
43324332
// a TemplateRef or a Template set.
43334333
type ReplicationControllerSpec struct {
43344334
// Replicas is the number of desired replicas.
4335-
Replicas int32
4335+
Replicas *int32
43364336

43374337
// Minimum number of seconds for which a newly created pod should be ready
43384338
// without any of its container crashing, for it to be considered available.

‎pkg/apis/core/v1/conversion.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,9 @@ func Convert_apps_ReplicaSetStatus_To_v1_ReplicationControllerStatus(in *apps.Re
199199
}
200200

201201
func Convert_core_ReplicationControllerSpec_To_v1_ReplicationControllerSpec(in *core.ReplicationControllerSpec, out *v1.ReplicationControllerSpec, s conversion.Scope) error {
202-
out.Replicas = &in.Replicas
202+
if err := autoConvert_core_ReplicationControllerSpec_To_v1_ReplicationControllerSpec(in, out, s); err != nil {
203+
return err
204+
}
203205
out.MinReadySeconds = in.MinReadySeconds
204206
out.Selector = in.Selector
205207
if in.Template != nil {
@@ -214,8 +216,8 @@ func Convert_core_ReplicationControllerSpec_To_v1_ReplicationControllerSpec(in *
214216
}
215217

216218
func Convert_v1_ReplicationControllerSpec_To_core_ReplicationControllerSpec(in *v1.ReplicationControllerSpec, out *core.ReplicationControllerSpec, s conversion.Scope) error {
217-
if in.Replicas != nil {
218-
out.Replicas = *in.Replicas
219+
if err := autoConvert_v1_ReplicationControllerSpec_To_core_ReplicationControllerSpec(in, out, s); err != nil {
220+
return err
219221
}
220222
out.MinReadySeconds = in.MinReadySeconds
221223
out.Selector = in.Selector

‎pkg/apis/core/v1/zz_generated.conversion.go‎

Lines changed: 2 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/apis/core/validation/validation.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6291,7 +6291,7 @@ func ValidateNonEmptySelector(selectorMap map[string]string, fldPath *field.Path
62916291
}
62926292

62936293
// Validates the given template and ensures that it is in accordance with the desired selector and replicas.
6294-
func ValidatePodTemplateSpecForRC(template *core.PodTemplateSpec, selectorMap map[string]string, replicas int32, fldPath *field.Path, opts PodValidationOptions) field.ErrorList {
6294+
func ValidatePodTemplateSpecForRC(template *core.PodTemplateSpec, selectorMap map[string]string, fldPath *field.Path, opts PodValidationOptions) field.ErrorList {
62956295
allErrs := field.ErrorList{}
62966296
if template == nil {
62976297
allErrs = append(allErrs, field.Required(fldPath, ""))
@@ -6321,8 +6321,12 @@ func ValidateReplicationControllerSpec(spec, oldSpec *core.ReplicationController
63216321
allErrs := field.ErrorList{}
63226322
allErrs = append(allErrs, ValidateNonnegativeField(int64(spec.MinReadySeconds), fldPath.Child("minReadySeconds"))...)
63236323
allErrs = append(allErrs, ValidateNonEmptySelector(spec.Selector, fldPath.Child("selector"))...)
6324-
allErrs = append(allErrs, ValidateNonnegativeField(int64(spec.Replicas), fldPath.Child("replicas"))...)
6325-
allErrs = append(allErrs, ValidatePodTemplateSpecForRC(spec.Template, spec.Selector, spec.Replicas, fldPath.Child("template"), opts)...)
6324+
if spec.Replicas == nil {
6325+
allErrs = append(allErrs, field.Required(fldPath.Child("replicas"), ""))
6326+
} else {
6327+
allErrs = append(allErrs, ValidateNonnegativeField(int64(*spec.Replicas), fldPath.Child("replicas"))...)
6328+
}
6329+
allErrs = append(allErrs, ValidatePodTemplateSpecForRC(spec.Template, spec.Selector, fldPath.Child("template"), opts)...)
63266330
return allErrs
63276331
}
63286332

‎pkg/apis/core/validation/validation_test.go‎

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16670,7 +16670,7 @@ func TestValidateReplicationControllerStatusUpdate(t *testing.T) {
1667016670
update: core.ReplicationController{
1667116671
ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
1667216672
Spec: core.ReplicationControllerSpec{
16673-
Replicas: 3,
16673+
Replicas: ptr.To[int32](3),
1667416674
Selector: validSelector,
1667516675
Template: &validPodTemplate.Template,
1667616676
},
@@ -16702,7 +16702,7 @@ func TestValidateReplicationControllerStatusUpdate(t *testing.T) {
1670216702
update: core.ReplicationController{
1670316703
ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
1670416704
Spec: core.ReplicationControllerSpec{
16705-
Replicas: 2,
16705+
Replicas: ptr.To[int32](2),
1670616706
Selector: validSelector,
1670716707
Template: &validPodTemplate.Template,
1670816708
},
@@ -16725,7 +16725,7 @@ func mkValidReplicationController(tweaks ...func(rc *core.ReplicationController)
1672516725
rc := core.ReplicationController{
1672616726
ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
1672716727
Spec: core.ReplicationControllerSpec{
16728-
Replicas: 1,
16728+
Replicas: ptr.To[int32](1),
1672916729
Selector: map[string]string{"a": "b"},
1673016730
Template: &core.PodTemplateSpec{
1673116731
ObjectMeta: metav1.ObjectMeta{
@@ -16748,17 +16748,17 @@ func TestValidateReplicationControllerUpdate(t *testing.T) {
1674816748
}{{
1674916749
old: mkValidReplicationController(func(rc *core.ReplicationController) {}),
1675016750
update: mkValidReplicationController(func(rc *core.ReplicationController) {
16751-
rc.Spec.Replicas = 0
16751+
rc.Spec.Replicas = ptr.To[int32](0)
1675216752
}),
1675316753
}, {
1675416754
old: mkValidReplicationController(func(rc *core.ReplicationController) {}),
1675516755
update: mkValidReplicationController(func(rc *core.ReplicationController) {
16756-
rc.Spec.Replicas = 3
16756+
rc.Spec.Replicas = ptr.To[int32](3)
1675716757
}),
1675816758
}, {
1675916759
old: mkValidReplicationController(func(rc *core.ReplicationController) {}),
1676016760
update: mkValidReplicationController(func(rc *core.ReplicationController) {
16761-
rc.Spec.Replicas = 2
16761+
rc.Spec.Replicas = ptr.To[int32](2)
1676216762
rc.Spec.Template.Spec = podtest.MakePodSpec(
1676316763
podtest.SetVolumes(
1676416764
core.Volume{
@@ -16816,12 +16816,21 @@ func TestValidateReplicationControllerUpdate(t *testing.T) {
1681616816
"negative replicas": {
1681716817
old: mkValidReplicationController(func(rc *core.ReplicationController) {}),
1681816818
update: mkValidReplicationController(func(rc *core.ReplicationController) {
16819-
rc.Spec.Replicas = -1
16819+
rc.Spec.Replicas = ptr.To[int32](-1)
1682016820
}),
1682116821
expectedErrs: field.ErrorList{
1682216822
field.Invalid(field.NewPath("spec.replicas"), nil, "").WithOrigin("minimum"),
1682316823
},
1682416824
},
16825+
"nil replicas": {
16826+
old: mkValidReplicationController(func(rc *core.ReplicationController) {}),
16827+
update: mkValidReplicationController(func(rc *core.ReplicationController) {
16828+
rc.Spec.Replicas = nil
16829+
}),
16830+
expectedErrs: field.ErrorList{
16831+
field.Required(field.NewPath("spec.replicas"), ""),
16832+
},
16833+
},
1682516834
}
1682616835
for k, tc := range errorCases {
1682716836
t.Run(k, func(t *testing.T) {
@@ -16839,7 +16848,7 @@ func TestValidateReplicationController(t *testing.T) {
1683916848
mkValidReplicationController(func(rc *core.ReplicationController) {}),
1684016849
mkValidReplicationController(func(rc *core.ReplicationController) { rc.Name = "abc-123" }),
1684116850
mkValidReplicationController(func(rc *core.ReplicationController) {
16842-
rc.Spec.Replicas = 2
16851+
rc.Spec.Replicas = ptr.To[int32](2)
1684316852
rc.Spec.Template.Spec = podtest.MakePodSpec(
1684416853
podtest.SetVolumes(
1684516854
core.Volume{
@@ -16860,9 +16869,9 @@ func TestValidateReplicationController(t *testing.T) {
1686016869
}))),
1686116870
)
1686216871
}),
16863-
mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = 0 }),
16864-
mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = 1 }),
16865-
mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = 100 }),
16872+
mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = ptr.To[int32](0) }),
16873+
mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = ptr.To[int32](1) }),
16874+
mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = ptr.To[int32](100) }),
1686616875
}
1686716876
for _, tc := range successCases {
1686816877
if errs := ValidateReplicationController(&tc, PodValidationOptions{}); len(errs) != 0 {
@@ -16905,11 +16914,17 @@ func TestValidateReplicationController(t *testing.T) {
1690516914
},
1690616915
},
1690716916
"negative replicas": {
16908-
input: mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = -1 }),
16917+
input: mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = ptr.To[int32](-1) }),
1690916918
expectedErrs: field.ErrorList{
1691016919
field.Invalid(field.NewPath("spec.replicas"), nil, "").WithOrigin("minimum"),
1691116920
},
1691216921
},
16922+
"nil replicas": {
16923+
input: mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = nil }),
16924+
expectedErrs: field.ErrorList{
16925+
field.Required(field.NewPath("spec.replicas"), ""),
16926+
},
16927+
},
1691316928
"invalid label": {
1691416929
input: mkValidReplicationController(func(rc *core.ReplicationController) {
1691516930
rc.Labels = map[string]string{

‎pkg/apis/core/zz_generated.deepcopy.go‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/printers/internalversion/printers.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import (
5151
"k8s.io/apimachinery/pkg/util/duration"
5252
"k8s.io/apimachinery/pkg/util/sets"
5353
"k8s.io/client-go/util/certificate/csr"
54+
"k8s.io/utils/ptr"
5455

5556
podutil "k8s.io/kubernetes/pkg/api/pod"
5657
podutilv1 "k8s.io/kubernetes/pkg/api/v1/pod"
@@ -1123,7 +1124,7 @@ func printReplicationController(obj *api.ReplicationController, options printers
11231124
Object: runtime.RawExtension{Object: obj},
11241125
}
11251126

1126-
desiredReplicas := obj.Spec.Replicas
1127+
desiredReplicas := ptr.Deref(obj.Spec.Replicas, 0)
11271128
currentReplicas := obj.Status.Replicas
11281129
readyReplicas := obj.Status.ReadyReplicas
11291130

‎pkg/printers/internalversion/printers_test.go‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
"time"
2626

2727
"github.com/google/go-cmp/cmp"
28-
2928
apiv1 "k8s.io/api/core/v1"
3029
"k8s.io/apimachinery/pkg/api/resource"
3130
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -4559,18 +4558,21 @@ func TestPrintCertificateSigningRequest(t *testing.T) {
45594558

45604559
func TestPrintReplicationController(t *testing.T) {
45614560
tests := []struct {
4561+
name string
45624562
rc api.ReplicationController
45634563
options printers.GenerateOptions
45644564
expected []metav1.TableRow
45654565
}{
45664566
// Basic print replication controller without replicas or status.
45674567
{
4568+
name: "basic",
45684569
rc: api.ReplicationController{
45694570
ObjectMeta: metav1.ObjectMeta{
45704571
Name: "rc1",
45714572
Namespace: "test-namespace",
45724573
},
45734574
Spec: api.ReplicationControllerSpec{
4575+
Replicas: ptr.To[int32](0),
45744576
Selector: map[string]string{"a": "b"},
45754577
Template: &api.PodTemplateSpec{
45764578
ObjectMeta: metav1.ObjectMeta{
@@ -4597,13 +4599,14 @@ func TestPrintReplicationController(t *testing.T) {
45974599
},
45984600
// Basic print replication controller with replicas; does not print containers or labels
45994601
{
4602+
name: "basic with replicas",
46004603
rc: api.ReplicationController{
46014604
ObjectMeta: metav1.ObjectMeta{
46024605
Name: "rc1",
46034606
Namespace: "test-namespace",
46044607
},
46054608
Spec: api.ReplicationControllerSpec{
4606-
Replicas: 5,
4609+
Replicas: ptr.To[int32](5),
46074610
Selector: map[string]string{"a": "b"},
46084611
Template: &api.PodTemplateSpec{
46094612
ObjectMeta: metav1.ObjectMeta{
@@ -4634,12 +4637,13 @@ func TestPrintReplicationController(t *testing.T) {
46344637
},
46354638
// Generate options: Wide; print containers and labels.
46364639
{
4640+
name: "wide",
46374641
rc: api.ReplicationController{
46384642
ObjectMeta: metav1.ObjectMeta{
46394643
Name: "rc1",
46404644
},
46414645
Spec: api.ReplicationControllerSpec{
4642-
Replicas: 5,
4646+
Replicas: ptr.To[int32](5),
46434647
Selector: map[string]string{"a": "b"},
46444648
Template: &api.PodTemplateSpec{
46454649
ObjectMeta: metav1.ObjectMeta{
@@ -4670,12 +4674,16 @@ func TestPrintReplicationController(t *testing.T) {
46704674
},
46714675
{
46724676
// make sure Bookmark event will not lead a panic
4677+
name: "no panic",
46734678
rc: api.ReplicationController{
46744679
ObjectMeta: metav1.ObjectMeta{
46754680
Annotations: map[string]string{
46764681
metav1.InitialEventsAnnotationKey: "true",
46774682
},
46784683
},
4684+
Spec: api.ReplicationControllerSpec{
4685+
Replicas: ptr.To[int32](0),
4686+
},
46794687
},
46804688
options: printers.GenerateOptions{Wide: true},
46814689
// Columns: Name, Desired, Current, Ready, Age, Containers, Images, Selector

‎pkg/registry/core/replicationcontroller/declarative_validation_test.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
apitesting "k8s.io/kubernetes/pkg/api/testing"
2929
api "k8s.io/kubernetes/pkg/apis/core"
3030
"k8s.io/kubernetes/pkg/features"
31+
"k8s.io/utils/ptr"
3132
)
3233

3334
func TestDeclarativeValidateForDeclarative(t *testing.T) {
@@ -121,7 +122,7 @@ func TestValidateUpdateForDeclarative(t *testing.T) {
121122
// The equivalenceMatcher is used to verify the output errors from hand-written imperative validation
122123
// are equivalent to the output errors when DeclarativeValidationTakeover is enabled.
123124
equivalenceMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin()
124-
// TODO: remove this once ErrorMatcher has been extended to handle this form of deduplication.
125+
// TODO: remove this once RC's validation is fixed to not return duplicate errors.
125126
dedupedImperativeErrs := field.ErrorList{}
126127
for _, err := range imperativeErrs {
127128
found := false
@@ -142,12 +143,13 @@ func TestValidateUpdateForDeclarative(t *testing.T) {
142143
}
143144
}
144145

145-
// Helper function for RC tests.
146+
// mkValidReplicationController produces a ReplicationController which passes
147+
// validation with no tweaks.
146148
func mkValidReplicationController(tweaks ...func(rc *api.ReplicationController)) api.ReplicationController {
147149
rc := api.ReplicationController{
148150
ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
149151
Spec: api.ReplicationControllerSpec{
150-
Replicas: 1,
152+
Replicas: ptr.To[int32](1),
151153
Selector: map[string]string{"a": "b"},
152154
Template: &api.PodTemplateSpec{
153155
ObjectMeta: metav1.ObjectMeta{

0 commit comments

Comments
 (0)