From e08bbf254c4af43ccc945d517ca19383d40513c2 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sat, 30 Nov 2024 15:09:47 -0800 Subject: [PATCH 1/8] Declaratively validate minimum value of RC.Spec.Replicas --- pkg/apis/core/v1/zz_generated.validations.go | 66 +++++++++++++++++++ pkg/apis/core/validation/validation.go | 2 +- .../src/k8s.io/api/core/v1/generated.proto | 1 + staging/src/k8s.io/api/core/v1/types.go | 1 + 4 files changed, 69 insertions(+), 1 deletion(-) diff --git a/pkg/apis/core/v1/zz_generated.validations.go b/pkg/apis/core/v1/zz_generated.validations.go index 73e28035dba..e5aaa2b3882 100644 --- a/pkg/apis/core/v1/zz_generated.validations.go +++ b/pkg/apis/core/v1/zz_generated.validations.go @@ -22,7 +22,15 @@ limitations under the License. package v1 import ( + context "context" + fmt "fmt" + + corev1 "k8s.io/api/core/v1" + operation "k8s.io/apimachinery/pkg/api/operation" + safe "k8s.io/apimachinery/pkg/api/safe" + validate "k8s.io/apimachinery/pkg/api/validate" runtime "k8s.io/apimachinery/pkg/runtime" + field "k8s.io/apimachinery/pkg/util/validation/field" ) func init() { localSchemeBuilder.Register(RegisterValidations) } @@ -30,5 +38,63 @@ 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 { + scheme.AddValidationFunc((*corev1.ReplicationController)(nil), func(ctx context.Context, op operation.Operation, obj, oldObj interface{}, subresources ...string) field.ErrorList { + if len(subresources) == 0 { + return Validate_ReplicationController(ctx, op, nil /* fldPath */, obj.(*corev1.ReplicationController), safe.Cast[*corev1.ReplicationController](oldObj)) + } + if len(subresources) == 1 && subresources[0] == "status" { + return nil // corev1.ReplicationControllerStatus has no validation + } + return field.ErrorList{field.InternalError(nil, fmt.Errorf("no validation found for %T, subresources: %v", obj, subresources))} + }) + scheme.AddValidationFunc((*corev1.ReplicationControllerList)(nil), func(ctx context.Context, op operation.Operation, obj, oldObj interface{}, subresources ...string) field.ErrorList { + if len(subresources) == 0 { + return Validate_ReplicationControllerList(ctx, op, nil /* fldPath */, obj.(*corev1.ReplicationControllerList), safe.Cast[*corev1.ReplicationControllerList](oldObj)) + } + return field.ErrorList{field.InternalError(nil, fmt.Errorf("no validation found for %T, subresources: %v", obj, subresources))} + }) return nil } + +func Validate_ReplicationController(ctx context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj *corev1.ReplicationController) (errs field.ErrorList) { + // field corev1.ReplicationController.TypeMeta has no validation + // field corev1.ReplicationController.ObjectMeta has no validation + + // field corev1.ReplicationController.Spec + errs = append(errs, + func(fldPath *field.Path, obj, oldObj *corev1.ReplicationControllerSpec) (errs field.ErrorList) { + errs = append(errs, Validate_ReplicationControllerSpec(ctx, op, fldPath, obj, oldObj)...) + return + }(fldPath.Child("spec"), &obj.Spec, safe.Field(oldObj, func(oldObj *corev1.ReplicationController) *corev1.ReplicationControllerSpec { return &oldObj.Spec }))...) + + // field corev1.ReplicationController.Status has no validation + return errs +} + +func Validate_ReplicationControllerList(ctx context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj *corev1.ReplicationControllerList) (errs field.ErrorList) { + // field corev1.ReplicationControllerList.TypeMeta has no validation + // field corev1.ReplicationControllerList.ListMeta has no validation + + // field corev1.ReplicationControllerList.Items + errs = append(errs, + func(fldPath *field.Path, obj, oldObj []corev1.ReplicationController) (errs field.ErrorList) { + errs = append(errs, validate.EachSliceVal(ctx, op, fldPath, obj, oldObj, nil, Validate_ReplicationController)...) + return + }(fldPath.Child("items"), obj.Items, safe.Field(oldObj, func(oldObj *corev1.ReplicationControllerList) []corev1.ReplicationController { return oldObj.Items }))...) + + return errs +} + +func Validate_ReplicationControllerSpec(ctx context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj *corev1.ReplicationControllerSpec) (errs field.ErrorList) { + // field corev1.ReplicationControllerSpec.Replicas + errs = append(errs, + func(fldPath *field.Path, obj, oldObj *int32) (errs field.ErrorList) { + errs = append(errs, validate.Minimum(ctx, op, fldPath, obj, oldObj, 0)...) + return + }(fldPath.Child("replicas"), obj.Replicas, safe.Field(oldObj, func(oldObj *corev1.ReplicationControllerSpec) *int32 { return oldObj.Replicas }))...) + + // field corev1.ReplicationControllerSpec.MinReadySeconds has no validation + // field corev1.ReplicationControllerSpec.Selector has no validation + // field corev1.ReplicationControllerSpec.Template has no validation + return errs +} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 298aa7b621d..8416ec8a074 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -6324,7 +6324,7 @@ func ValidateReplicationControllerSpec(spec, oldSpec *core.ReplicationController if spec.Replicas == nil { allErrs = append(allErrs, field.Required(fldPath.Child("replicas"), "")) } else { - allErrs = append(allErrs, ValidateNonnegativeField(int64(*spec.Replicas), fldPath.Child("replicas"))...) + allErrs = append(allErrs, ValidateNonnegativeField(int64(*spec.Replicas), fldPath.Child("replicas")).MarkCoveredByDeclarative()...) } allErrs = append(allErrs, ValidatePodTemplateSpecForRC(spec.Template, spec.Selector, fldPath.Child("template"), opts)...) return allErrs diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index a8ffbac586e..e1c4a711b4d 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -5085,6 +5085,7 @@ message ReplicationControllerSpec { // Defaults to 1. // More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller // +optional + // +k8s:minimum=0 optional int32 replicas = 1; // Minimum number of seconds for which a newly created pod should be ready diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index 6e6179e3283..1432d1f5441 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -5107,6 +5107,7 @@ type ReplicationControllerSpec struct { // Defaults to 1. // More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller // +optional + // +k8s:minimum=0 Replicas *int32 `json:"replicas,omitempty" protobuf:"varint,1,opt,name=replicas"` // Minimum number of seconds for which a newly created pod should be ready From 21b3da7e5aadf77e8fd17e467c86ff7e18f7d9ed Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 19 Dec 2024 09:03:52 -0800 Subject: [PATCH 2/8] Add declarative default for RC.Spec.Replicas Remove manual default: ``` $ git diff diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index e66de8bb432..1dd28dd35fb 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -60,10 +60,6 @@ func SetDefaults_ReplicationController(obj *v1.ReplicationController) { obj.Labels = labels } } - if obj.Spec.Replicas == nil { - obj.Spec.Replicas = new(int32) - *obj.Spec.Replicas = 1 - } } func SetDefaults_Volume(obj *v1.Volume) { if ptr.AllPtrFieldsNil(&obj.VolumeSource) { ``` The test fails: ``` $ go test ./pkg/apis/core/v1 | grep -v gate | grep -v SetEmulationVersion --- FAIL: TestSetDefaultReplicationControllerReplicas (0.00s) defaults_test.go:1608: expected: 1 replicas, got: 0 FAIL FAIL k8s.io/kubernetes/pkg/apis/core/v1 0.269s FAIL ``` Declare the default, update codegen and re-run the test: ``` $ git diff diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index 406ab56a002..7e5136fe9f6 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -5101,6 +5101,7 @@ type ReplicationControllerSpec struct { // Defaults to 1. // More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller // +optional + // +default=1 // +k8s:minimum=0 Replicas *int32 `json:"replicas,omitempty" protobuf:"varint,1,opt,name=replicas"` $ ./hack/update-codegen.sh default +++ [1219 08:58:43] Generating defaulter code for 102 targets $ git diff diff --git a/pkg/apis/core/v1/zz_generated.defaults.go b/pkg/apis/core/v1/zz_generated.defaults.go index 3b6eb4f0a93..567c49053aa 100644 --- a/pkg/apis/core/v1/zz_generated.defaults.go +++ b/pkg/apis/core/v1/zz_generated.defaults.go @@ -878,6 +878,10 @@ func SetObjectDefaults_PodTemplateList(in *corev1.PodTemplateList) { func SetObjectDefaults_ReplicationController(in *corev1.ReplicationController) { SetDefaults_ReplicationController(in) + if in.Spec.Replicas == nil { + var ptrVar1 int32 = 1 + in.Spec.Replicas = &ptrVar1 + } if in.Spec.Template != nil { SetDefaults_PodSpec(&in.Spec.Template.Spec) for i := range in.Spec.Template.Spec.Volumes { $ go test ./pkg/apis/core/v1 | grep -v gate | grep -v SetEmulationVersion ok k8s.io/kubernetes/pkg/apis/core/v1 (cached) ``` --- api/openapi-spec/v3/api__v1_openapi.json | 1 + pkg/apis/core/v1/defaults.go | 5 +---- pkg/apis/core/v1/zz_generated.defaults.go | 4 ++++ pkg/generated/openapi/zz_generated.openapi.go | 1 + staging/src/k8s.io/api/core/v1/generated.proto | 1 + staging/src/k8s.io/api/core/v1/types.go | 1 + .../client-go/applyconfigurations/internal/internal.go | 1 + 7 files changed, 10 insertions(+), 4 deletions(-) diff --git a/api/openapi-spec/v3/api__v1_openapi.json b/api/openapi-spec/v3/api__v1_openapi.json index 9302ec2b8b2..ead58a4f5d7 100644 --- a/api/openapi-spec/v3/api__v1_openapi.json +++ b/api/openapi-spec/v3/api__v1_openapi.json @@ -6528,6 +6528,7 @@ "type": "integer" }, "replicas": { + "default": 1, "description": "Replicas is the number of desired replicas. This is a pointer to distinguish between explicit zero and unspecified. Defaults to 1. More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller", "format": "int32", "type": "integer" diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index e66de8bb432..586e8013ab8 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -60,10 +60,7 @@ func SetDefaults_ReplicationController(obj *v1.ReplicationController) { obj.Labels = labels } } - if obj.Spec.Replicas == nil { - obj.Spec.Replicas = new(int32) - *obj.Spec.Replicas = 1 - } + // obj.Spec.Replicas is defaulted declaratively } func SetDefaults_Volume(obj *v1.Volume) { if ptr.AllPtrFieldsNil(&obj.VolumeSource) { diff --git a/pkg/apis/core/v1/zz_generated.defaults.go b/pkg/apis/core/v1/zz_generated.defaults.go index 3b6eb4f0a93..567c49053aa 100644 --- a/pkg/apis/core/v1/zz_generated.defaults.go +++ b/pkg/apis/core/v1/zz_generated.defaults.go @@ -878,6 +878,10 @@ func SetObjectDefaults_PodTemplateList(in *corev1.PodTemplateList) { func SetObjectDefaults_ReplicationController(in *corev1.ReplicationController) { SetDefaults_ReplicationController(in) + if in.Spec.Replicas == nil { + var ptrVar1 int32 = 1 + in.Spec.Replicas = &ptrVar1 + } if in.Spec.Template != nil { SetDefaults_PodSpec(&in.Spec.Template.Spec) for i := range in.Spec.Template.Spec.Volumes { diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go index 7d45d2d72a5..0b62252e7ef 100644 --- a/pkg/generated/openapi/zz_generated.openapi.go +++ b/pkg/generated/openapi/zz_generated.openapi.go @@ -30098,6 +30098,7 @@ func schema_k8sio_api_core_v1_ReplicationControllerSpec(ref common.ReferenceCall "replicas": { SchemaProps: spec.SchemaProps{ Description: "Replicas is the number of desired replicas. This is a pointer to distinguish between explicit zero and unspecified. Defaults to 1. More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller", + Default: 1, Type: []string{"integer"}, Format: "int32", }, diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index e1c4a711b4d..30353a0bd37 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -5085,6 +5085,7 @@ message ReplicationControllerSpec { // Defaults to 1. // More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller // +optional + // +default=1 // +k8s:minimum=0 optional int32 replicas = 1; diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index 1432d1f5441..0da3e37f7b5 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -5107,6 +5107,7 @@ type ReplicationControllerSpec struct { // Defaults to 1. // More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller // +optional + // +default=1 // +k8s:minimum=0 Replicas *int32 `json:"replicas,omitempty" protobuf:"varint,1,opt,name=replicas"` diff --git a/staging/src/k8s.io/client-go/applyconfigurations/internal/internal.go b/staging/src/k8s.io/client-go/applyconfigurations/internal/internal.go index 9cd097ca5fb..5fa1df9da4e 100644 --- a/staging/src/k8s.io/client-go/applyconfigurations/internal/internal.go +++ b/staging/src/k8s.io/client-go/applyconfigurations/internal/internal.go @@ -7649,6 +7649,7 @@ var schemaYAML = typed.YAMLObject(`types: - name: replicas type: scalar: numeric + default: 1 - name: selector type: map: From 0f4786536f7482559a517609f940e1f11f65da60 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 19 Dec 2024 10:34:42 -0800 Subject: [PATCH 3/8] Declaratively validate RC.Spec.Replicas optionality The existing test run both declarative and manual validation and it still passes. --- pkg/apis/core/v1/zz_generated.validations.go | 5 +++++ pkg/apis/core/validation/validation.go | 2 +- staging/src/k8s.io/api/core/v1/generated.proto | 1 + staging/src/k8s.io/api/core/v1/types.go | 1 + 4 files changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/apis/core/v1/zz_generated.validations.go b/pkg/apis/core/v1/zz_generated.validations.go index e5aaa2b3882..c5e54847792 100644 --- a/pkg/apis/core/v1/zz_generated.validations.go +++ b/pkg/apis/core/v1/zz_generated.validations.go @@ -89,6 +89,11 @@ func Validate_ReplicationControllerSpec(ctx context.Context, op operation.Operat // field corev1.ReplicationControllerSpec.Replicas errs = append(errs, func(fldPath *field.Path, obj, oldObj *int32) (errs field.ErrorList) { + // optional fields with default values are effectively required + if e := validate.RequiredPointer(ctx, op, fldPath, obj, oldObj); len(e) != 0 { + errs = append(errs, e...) + return // do not proceed + } errs = append(errs, validate.Minimum(ctx, op, fldPath, obj, oldObj, 0)...) return }(fldPath.Child("replicas"), obj.Replicas, safe.Field(oldObj, func(oldObj *corev1.ReplicationControllerSpec) *int32 { return oldObj.Replicas }))...) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 8416ec8a074..78abddc1522 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -6322,7 +6322,7 @@ func ValidateReplicationControllerSpec(spec, oldSpec *core.ReplicationController allErrs = append(allErrs, ValidateNonnegativeField(int64(spec.MinReadySeconds), fldPath.Child("minReadySeconds"))...) allErrs = append(allErrs, ValidateNonEmptySelector(spec.Selector, fldPath.Child("selector"))...) if spec.Replicas == nil { - allErrs = append(allErrs, field.Required(fldPath.Child("replicas"), "")) + allErrs = append(allErrs, field.Required(fldPath.Child("replicas"), "").MarkCoveredByDeclarative()) } else { allErrs = append(allErrs, ValidateNonnegativeField(int64(*spec.Replicas), fldPath.Child("replicas")).MarkCoveredByDeclarative()...) } diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index 30353a0bd37..4f5862d2886 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -5085,6 +5085,7 @@ message ReplicationControllerSpec { // Defaults to 1. // More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller // +optional + // +k8s:optional // +default=1 // +k8s:minimum=0 optional int32 replicas = 1; diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index 0da3e37f7b5..e4dc107e665 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -5107,6 +5107,7 @@ type ReplicationControllerSpec struct { // Defaults to 1. // More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller // +optional + // +k8s:optional // +default=1 // +k8s:minimum=0 Replicas *int32 `json:"replicas,omitempty" protobuf:"varint,1,opt,name=replicas"` From 20b69a54bff48d8ddb73a54387cf6d5f8659bdba Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Wed, 12 Mar 2025 19:06:25 -0400 Subject: [PATCH 4/8] Add declarative test cases for RC.Spec.Replicas --- .../declarative_validation_test.go | 67 ++++++++++++++++++- 1 file changed, 65 insertions(+), 2 deletions(-) diff --git a/pkg/registry/core/replicationcontroller/declarative_validation_test.go b/pkg/registry/core/replicationcontroller/declarative_validation_test.go index 048fddb779f..441b4d629a3 100644 --- a/pkg/registry/core/replicationcontroller/declarative_validation_test.go +++ b/pkg/registry/core/replicationcontroller/declarative_validation_test.go @@ -40,10 +40,34 @@ func TestDeclarativeValidateForDeclarative(t *testing.T) { input api.ReplicationController expectedErrs field.ErrorList }{ + // baseline "empty resource": { input: mkValidReplicationController(), }, - // TODO: Add test cases + // spec.replicas + "nil replicas": { + input: mkValidReplicationController(func(rc *api.ReplicationController) { + rc.Spec.Replicas = nil + }), + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("spec.replicas"), ""), + }, + }, + "0 replicas": { + input: mkValidReplicationController(setSpecReplicas(0)), + }, + "1 replicas": { + input: mkValidReplicationController(setSpecReplicas(1)), + }, + "positive replicas": { + input: mkValidReplicationController(setSpecReplicas(100)), + }, + "negative replicas": { + input: mkValidReplicationController(setSpecReplicas(-1)), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec.replicas"), nil, "").WithOrigin("minimum"), + }, + }, } for k, tc := range testCases { t.Run(k, func(t *testing.T) { @@ -90,7 +114,40 @@ func TestValidateUpdateForDeclarative(t *testing.T) { update api.ReplicationController expectedErrs field.ErrorList }{ - // TODO: Add test cases + // baseline + "baseline": { + old: mkValidReplicationController(), + update: mkValidReplicationController(), + }, + // spec.replicas + "nil replicas": { + old: mkValidReplicationController(), + update: mkValidReplicationController(func(rc *api.ReplicationController) { + rc.Spec.Replicas = nil + }), + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("spec.replicas"), ""), + }, + }, + "0 replicas": { + old: mkValidReplicationController(), + update: mkValidReplicationController(setSpecReplicas(0)), + }, + "1 replicas": { + old: mkValidReplicationController(), + update: mkValidReplicationController(setSpecReplicas(1)), + }, + "positive replicas": { + old: mkValidReplicationController(), + update: mkValidReplicationController(setSpecReplicas(100)), + }, + "negative replicas": { + old: mkValidReplicationController(), + update: mkValidReplicationController(setSpecReplicas(-1)), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec.replicas"), nil, "").WithOrigin("minimum"), + }, + }, } for k, tc := range testCases { t.Run(k, func(t *testing.T) { @@ -164,3 +221,9 @@ func mkValidReplicationController(tweaks ...func(rc *api.ReplicationController)) } return rc } + +func setSpecReplicas(val int32) func(rc *api.ReplicationController) { + return func(rc *api.ReplicationController) { + rc.Spec.Replicas = ptr.To(val) + } +} From 1059dbdee185ee585094c7e564ca60912ecab969 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 22 Dec 2024 23:10:29 -0800 Subject: [PATCH 5/8] Declaratively validate minimum value of RC.Spec.MinReadySeconds # Conflicts: # staging/src/k8s.io/api/core/v1/types.go --- pkg/apis/core/v1/zz_generated.validations.go | 8 +++++- pkg/apis/core/validation/validation.go | 2 +- pkg/apis/core/validation/validation_test.go | 28 +++++++++++++++++++ .../src/k8s.io/api/core/v1/generated.proto | 1 + staging/src/k8s.io/api/core/v1/types.go | 1 + 5 files changed, 38 insertions(+), 2 deletions(-) diff --git a/pkg/apis/core/v1/zz_generated.validations.go b/pkg/apis/core/v1/zz_generated.validations.go index c5e54847792..dcfcfd172e6 100644 --- a/pkg/apis/core/v1/zz_generated.validations.go +++ b/pkg/apis/core/v1/zz_generated.validations.go @@ -98,7 +98,13 @@ func Validate_ReplicationControllerSpec(ctx context.Context, op operation.Operat return }(fldPath.Child("replicas"), obj.Replicas, safe.Field(oldObj, func(oldObj *corev1.ReplicationControllerSpec) *int32 { return oldObj.Replicas }))...) - // field corev1.ReplicationControllerSpec.MinReadySeconds has no validation + // field corev1.ReplicationControllerSpec.MinReadySeconds + errs = append(errs, + func(fldPath *field.Path, obj, oldObj *int32) (errs field.ErrorList) { + errs = append(errs, validate.Minimum(ctx, op, fldPath, obj, oldObj, 0)...) + return + }(fldPath.Child("minReadySeconds"), &obj.MinReadySeconds, safe.Field(oldObj, func(oldObj *corev1.ReplicationControllerSpec) *int32 { return &oldObj.MinReadySeconds }))...) + // field corev1.ReplicationControllerSpec.Selector has no validation // field corev1.ReplicationControllerSpec.Template has no validation return errs diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 78abddc1522..b85c555896b 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -6319,7 +6319,7 @@ func ValidatePodTemplateSpecForRC(template *core.PodTemplateSpec, selectorMap ma // ValidateReplicationControllerSpec tests if required fields in the replication controller spec are set. func ValidateReplicationControllerSpec(spec, oldSpec *core.ReplicationControllerSpec, fldPath *field.Path, opts PodValidationOptions) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, ValidateNonnegativeField(int64(spec.MinReadySeconds), fldPath.Child("minReadySeconds"))...) + allErrs = append(allErrs, ValidateNonnegativeField(int64(spec.MinReadySeconds), fldPath.Child("minReadySeconds")).MarkCoveredByDeclarative()...) allErrs = append(allErrs, ValidateNonEmptySelector(spec.Selector, fldPath.Child("selector"))...) if spec.Replicas == nil { allErrs = append(allErrs, field.Required(fldPath.Child("replicas"), "").MarkCoveredByDeclarative()) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 705b0258d31..a6e3135e898 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -16755,6 +16755,16 @@ func TestValidateReplicationControllerUpdate(t *testing.T) { update: mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = ptr.To[int32](3) }), + }, { + old: mkValidReplicationController(func(rc *core.ReplicationController) {}), + update: mkValidReplicationController(func(rc *core.ReplicationController) { + rc.Spec.MinReadySeconds = 0 + }), + }, { + old: mkValidReplicationController(func(rc *core.ReplicationController) {}), + update: mkValidReplicationController(func(rc *core.ReplicationController) { + rc.Spec.MinReadySeconds = 3 + }), }, { old: mkValidReplicationController(func(rc *core.ReplicationController) {}), update: mkValidReplicationController(func(rc *core.ReplicationController) { @@ -16831,6 +16841,15 @@ func TestValidateReplicationControllerUpdate(t *testing.T) { field.Required(field.NewPath("spec.replicas"), ""), }, }, + "negative minReadySeconds": { + old: mkValidReplicationController(func(rc *core.ReplicationController) {}), + update: mkValidReplicationController(func(rc *core.ReplicationController) { + rc.Spec.MinReadySeconds = -1 + }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec.minReadySeconds"), nil, "").WithOrigin("minimum"), + }, + }, } for k, tc := range errorCases { t.Run(k, func(t *testing.T) { @@ -16872,6 +16891,9 @@ func TestValidateReplicationController(t *testing.T) { mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = ptr.To[int32](0) }), mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = ptr.To[int32](1) }), mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = ptr.To[int32](100) }), + mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.MinReadySeconds = 0 }), + mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.MinReadySeconds = 1 }), + mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.MinReadySeconds = 100 }), } for _, tc := range successCases { if errs := ValidateReplicationController(&tc, PodValidationOptions{}); len(errs) != 0 { @@ -16919,6 +16941,12 @@ func TestValidateReplicationController(t *testing.T) { field.Invalid(field.NewPath("spec.replicas"), nil, "").WithOrigin("minimum"), }, }, + "negative minReadySeconds": { + input: mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.MinReadySeconds = -1 }), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec.minReadySeconds"), nil, "").WithOrigin("minimum"), + }, + }, "nil replicas": { input: mkValidReplicationController(func(rc *core.ReplicationController) { rc.Spec.Replicas = nil }), expectedErrs: field.ErrorList{ diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index 4f5862d2886..95bd273f1e4 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -5094,6 +5094,7 @@ message ReplicationControllerSpec { // without any of its container crashing, for it to be considered available. // Defaults to 0 (pod will be considered available as soon as it is ready) // +optional + // +k8s:minimum=0 optional int32 minReadySeconds = 4; // Selector is a label query over pods that should match the Replicas count. diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index e4dc107e665..c33ddd6f9ee 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -5116,6 +5116,7 @@ type ReplicationControllerSpec struct { // without any of its container crashing, for it to be considered available. // Defaults to 0 (pod will be considered available as soon as it is ready) // +optional + // +k8s:minimum=0 MinReadySeconds int32 `json:"minReadySeconds,omitempty" protobuf:"varint,4,opt,name=minReadySeconds"` // Selector is a label query over pods that should match the Replicas count. From 1e336160681f129cb34d6fabfa23cdac50cc107b Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 22 Dec 2024 22:59:32 -0800 Subject: [PATCH 6/8] Add declarative default for RC.Spec.MinReadySeconds --- api/openapi-spec/v3/api__v1_openapi.json | 1 + pkg/generated/openapi/zz_generated.openapi.go | 1 + staging/src/k8s.io/api/core/v1/generated.proto | 1 + staging/src/k8s.io/api/core/v1/types.go | 1 + .../k8s.io/client-go/applyconfigurations/internal/internal.go | 1 + 5 files changed, 5 insertions(+) diff --git a/api/openapi-spec/v3/api__v1_openapi.json b/api/openapi-spec/v3/api__v1_openapi.json index ead58a4f5d7..75f3b90f9d6 100644 --- a/api/openapi-spec/v3/api__v1_openapi.json +++ b/api/openapi-spec/v3/api__v1_openapi.json @@ -6523,6 +6523,7 @@ "description": "ReplicationControllerSpec is the specification of a replication controller.", "properties": { "minReadySeconds": { + "default": 0, "description": "Minimum number of seconds for which a newly created pod should be ready without any of its container crashing, for it to be considered available. Defaults to 0 (pod will be considered available as soon as it is ready)", "format": "int32", "type": "integer" diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go index 0b62252e7ef..560ea0d573d 100644 --- a/pkg/generated/openapi/zz_generated.openapi.go +++ b/pkg/generated/openapi/zz_generated.openapi.go @@ -30106,6 +30106,7 @@ func schema_k8sio_api_core_v1_ReplicationControllerSpec(ref common.ReferenceCall "minReadySeconds": { SchemaProps: spec.SchemaProps{ Description: "Minimum number of seconds for which a newly created pod should be ready without any of its container crashing, for it to be considered available. Defaults to 0 (pod will be considered available as soon as it is ready)", + Default: 0, Type: []string{"integer"}, Format: "int32", }, diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index 95bd273f1e4..51dd639a703 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -5094,6 +5094,7 @@ message ReplicationControllerSpec { // without any of its container crashing, for it to be considered available. // Defaults to 0 (pod will be considered available as soon as it is ready) // +optional + // +default=0 // +k8s:minimum=0 optional int32 minReadySeconds = 4; diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index c33ddd6f9ee..778148962eb 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -5116,6 +5116,7 @@ type ReplicationControllerSpec struct { // without any of its container crashing, for it to be considered available. // Defaults to 0 (pod will be considered available as soon as it is ready) // +optional + // +default=0 // +k8s:minimum=0 MinReadySeconds int32 `json:"minReadySeconds,omitempty" protobuf:"varint,4,opt,name=minReadySeconds"` diff --git a/staging/src/k8s.io/client-go/applyconfigurations/internal/internal.go b/staging/src/k8s.io/client-go/applyconfigurations/internal/internal.go index 5fa1df9da4e..f2888de0007 100644 --- a/staging/src/k8s.io/client-go/applyconfigurations/internal/internal.go +++ b/staging/src/k8s.io/client-go/applyconfigurations/internal/internal.go @@ -7646,6 +7646,7 @@ var schemaYAML = typed.YAMLObject(`types: - name: minReadySeconds type: scalar: numeric + default: 0 - name: replicas type: scalar: numeric From 177193ed197ab482b8bcef968b491305542af06f Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Wed, 12 Mar 2025 20:45:28 -0400 Subject: [PATCH 7/8] Mark RC.Spec.MinReadySeconds as k8s:optional +k8s:optional should be used everywhere +optional is. This does not change the vailidation of the field, but it is a good practice, and code generator recognizes the fields tags and outputs this comment above the field validation as a result: "optional value-type fields with zero-value defaults are purely documentation". --- pkg/apis/core/v1/zz_generated.validations.go | 1 + staging/src/k8s.io/api/core/v1/generated.proto | 1 + staging/src/k8s.io/api/core/v1/types.go | 1 + 3 files changed, 3 insertions(+) diff --git a/pkg/apis/core/v1/zz_generated.validations.go b/pkg/apis/core/v1/zz_generated.validations.go index dcfcfd172e6..65030b5e10c 100644 --- a/pkg/apis/core/v1/zz_generated.validations.go +++ b/pkg/apis/core/v1/zz_generated.validations.go @@ -102,6 +102,7 @@ func Validate_ReplicationControllerSpec(ctx context.Context, op operation.Operat errs = append(errs, func(fldPath *field.Path, obj, oldObj *int32) (errs field.ErrorList) { errs = append(errs, validate.Minimum(ctx, op, fldPath, obj, oldObj, 0)...) + // optional value-type fields with zero-value defaults are purely documentation return }(fldPath.Child("minReadySeconds"), &obj.MinReadySeconds, safe.Field(oldObj, func(oldObj *corev1.ReplicationControllerSpec) *int32 { return &oldObj.MinReadySeconds }))...) diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index 51dd639a703..21b6d098f1f 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -5094,6 +5094,7 @@ message ReplicationControllerSpec { // without any of its container crashing, for it to be considered available. // Defaults to 0 (pod will be considered available as soon as it is ready) // +optional + // +k8s:optional // +default=0 // +k8s:minimum=0 optional int32 minReadySeconds = 4; diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index 778148962eb..d7ac4e93415 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -5116,6 +5116,7 @@ type ReplicationControllerSpec struct { // without any of its container crashing, for it to be considered available. // Defaults to 0 (pod will be considered available as soon as it is ready) // +optional + // +k8s:optional // +default=0 // +k8s:minimum=0 MinReadySeconds int32 `json:"minReadySeconds,omitempty" protobuf:"varint,4,opt,name=minReadySeconds"` From f7296b31f0da10ce4843c0816d373f2d89436485 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Wed, 12 Mar 2025 19:07:34 -0400 Subject: [PATCH 8/8] Add declarative test cases for RC.Spec.MinReadySeconds --- .../declarative_validation_test.go | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/pkg/registry/core/replicationcontroller/declarative_validation_test.go b/pkg/registry/core/replicationcontroller/declarative_validation_test.go index 441b4d629a3..099b0402f42 100644 --- a/pkg/registry/core/replicationcontroller/declarative_validation_test.go +++ b/pkg/registry/core/replicationcontroller/declarative_validation_test.go @@ -68,6 +68,22 @@ func TestDeclarativeValidateForDeclarative(t *testing.T) { field.Invalid(field.NewPath("spec.replicas"), nil, "").WithOrigin("minimum"), }, }, + // spec.minReadySeconds + "0 minReadySeconds": { + input: mkValidReplicationController(setSpecMinReadySeconds(0)), + }, + "1 minReadySeconds": { + input: mkValidReplicationController(setSpecMinReadySeconds(1)), + }, + "positive minReadySeconds": { + input: mkValidReplicationController(setSpecMinReadySeconds(100)), + }, + "negative minReadySeconds": { + input: mkValidReplicationController(setSpecMinReadySeconds(-1)), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec.minReadySeconds"), nil, "").WithOrigin("minimum"), + }, + }, } for k, tc := range testCases { t.Run(k, func(t *testing.T) { @@ -148,6 +164,26 @@ func TestValidateUpdateForDeclarative(t *testing.T) { field.Invalid(field.NewPath("spec.replicas"), nil, "").WithOrigin("minimum"), }, }, + // spec.minReadySeconds + "0 minReadySeconds": { + old: mkValidReplicationController(), + update: mkValidReplicationController(setSpecMinReadySeconds(0)), + }, + "1 minReadySeconds": { + old: mkValidReplicationController(), + update: mkValidReplicationController(setSpecMinReadySeconds(1)), + }, + "positive minReadySeconds": { + old: mkValidReplicationController(), + update: mkValidReplicationController(setSpecMinReadySeconds(3)), + }, + "negative minReadySeconds": { + old: mkValidReplicationController(), + update: mkValidReplicationController(setSpecMinReadySeconds(-1)), + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("spec.minReadySeconds"), nil, "").WithOrigin("minimum"), + }, + }, } for k, tc := range testCases { t.Run(k, func(t *testing.T) { @@ -227,3 +263,9 @@ func setSpecReplicas(val int32) func(rc *api.ReplicationController) { rc.Spec.Replicas = ptr.To(val) } } + +func setSpecMinReadySeconds(val int32) func(rc *api.ReplicationController) { + return func(rc *api.ReplicationController) { + rc.Spec.MinReadySeconds = val + } +}