From 42abc2a73b939d1565d747b5f1247aa368da2275 Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Tue, 18 Feb 2025 11:05:41 +0100 Subject: [PATCH 1/3] Mark StatefulSet's .spec.serviceName optional The API reference doc (https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/stateful-set-v1/#StatefulSetSpec) mentions .spec.serviceName field is required, because it doesn't have optional tag, nor omitempty. In practice this field is optional, and can be empty. This change explicitly marks the field optional. Signed-off-by: Maciej Szulik --- pkg/apis/apps/types.go | 1 + staging/src/k8s.io/api/apps/v1/types.go | 1 + staging/src/k8s.io/api/apps/v1beta1/types.go | 1 + staging/src/k8s.io/api/apps/v1beta2/types.go | 1 + 4 files changed, 4 insertions(+) diff --git a/pkg/apis/apps/types.go b/pkg/apis/apps/types.go index 8d228de8e4b..a3b7786d462 100644 --- a/pkg/apis/apps/types.go +++ b/pkg/apis/apps/types.go @@ -198,6 +198,7 @@ type StatefulSetSpec struct { // the network identity of the set. Pods get DNS/hostnames that follow the // pattern: pod-specific-string.serviceName.default.svc.cluster.local // where "pod-specific-string" is managed by the StatefulSet controller. + // +optional ServiceName string // PodManagementPolicy controls how pods are created during initial scale up, diff --git a/staging/src/k8s.io/api/apps/v1/types.go b/staging/src/k8s.io/api/apps/v1/types.go index 0f4102907ca..7368e8ebb47 100644 --- a/staging/src/k8s.io/api/apps/v1/types.go +++ b/staging/src/k8s.io/api/apps/v1/types.go @@ -220,6 +220,7 @@ type StatefulSetSpec struct { // the network identity of the set. Pods get DNS/hostnames that follow the // pattern: pod-specific-string.serviceName.default.svc.cluster.local // where "pod-specific-string" is managed by the StatefulSet controller. + // +optional ServiceName string `json:"serviceName" protobuf:"bytes,5,opt,name=serviceName"` // podManagementPolicy controls how pods are created during initial scale up, diff --git a/staging/src/k8s.io/api/apps/v1beta1/types.go b/staging/src/k8s.io/api/apps/v1beta1/types.go index 85710264fe3..b97a050bfde 100644 --- a/staging/src/k8s.io/api/apps/v1beta1/types.go +++ b/staging/src/k8s.io/api/apps/v1beta1/types.go @@ -259,6 +259,7 @@ type StatefulSetSpec struct { // the network identity of the set. Pods get DNS/hostnames that follow the // pattern: pod-specific-string.serviceName.default.svc.cluster.local // where "pod-specific-string" is managed by the StatefulSet controller. + // +optional ServiceName string `json:"serviceName" protobuf:"bytes,5,opt,name=serviceName"` // podManagementPolicy controls how pods are created during initial scale up, diff --git a/staging/src/k8s.io/api/apps/v1beta2/types.go b/staging/src/k8s.io/api/apps/v1beta2/types.go index cc7d6582f50..2d88bfba534 100644 --- a/staging/src/k8s.io/api/apps/v1beta2/types.go +++ b/staging/src/k8s.io/api/apps/v1beta2/types.go @@ -269,6 +269,7 @@ type StatefulSetSpec struct { // the network identity of the set. Pods get DNS/hostnames that follow the // pattern: pod-specific-string.serviceName.default.svc.cluster.local // where "pod-specific-string" is managed by the StatefulSet controller. + // +optional ServiceName string `json:"serviceName" protobuf:"bytes,5,opt,name=serviceName"` // podManagementPolicy controls how pods are created during initial scale up, From 8eb74b96e33a89b9fbc312a05cc39c44a32cfd04 Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Tue, 18 Feb 2025 11:06:19 +0100 Subject: [PATCH 2/3] Add validation to StatefulSet's .spec.serviceName .spec.serviceName field is injected into pod.spec.subDomain which requires values to be valid DNS1123 label, but statefulset validation never validates the field, if specifired. This can cause the controller to fail creating pods. Signed-off-by: Maciej Szulik --- pkg/apis/apps/validation/validation.go | 22 +++++++++++-- pkg/apis/apps/validation/validation_test.go | 20 +++++++++++- .../statefulset/statefulset_test.go | 32 +++++++++++++++++++ 3 files changed, 70 insertions(+), 4 deletions(-) diff --git a/pkg/apis/apps/validation/validation.go b/pkg/apis/apps/validation/validation.go index 6640735dd8c..cff536d9ecf 100644 --- a/pkg/apis/apps/validation/validation.go +++ b/pkg/apis/apps/validation/validation.go @@ -34,6 +34,12 @@ import ( apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" ) +// StatefulSetValidationOptions is a struct that can be passed to ValidateStatefulSetSpec to record the validate options +type StatefulSetValidationOptions struct { + // Allow invalid DNS1123 ServiceName + AllowInvalidServiceName bool +} + // ValidateStatefulSetName can be used to check whether the given StatefulSet name is valid. // Prefix indicates this name will be used as part of generation, in which case // trailing dashes are allowed. @@ -89,7 +95,7 @@ func ValidatePersistentVolumeClaimRetentionPolicy(policy *apps.StatefulSetPersis } // ValidateStatefulSetSpec tests if required fields in the StatefulSet spec are set. -func ValidateStatefulSetSpec(spec *apps.StatefulSetSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions) field.ErrorList { +func ValidateStatefulSetSpec(spec *apps.StatefulSetSpec, fldPath *field.Path, opts apivalidation.PodValidationOptions, setOpts StatefulSetValidationOptions) field.ErrorList { allErrs := field.ErrorList{} switch spec.PodManagementPolicy { @@ -134,6 +140,10 @@ func ValidateStatefulSetSpec(spec *apps.StatefulSetSpec, fldPath *field.Path, op allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(replicaStartOrdinal), fldPath.Child("ordinals.start"))...) } + if !setOpts.AllowInvalidServiceName && len(spec.ServiceName) > 0 { + allErrs = append(allErrs, apivalidation.ValidateDNS1123Label(spec.ServiceName, fldPath.Child("serviceName"))...) + } + if spec.Selector == nil { allErrs = append(allErrs, field.Required(fldPath.Child("selector"), "")) } else { @@ -164,7 +174,10 @@ func ValidateStatefulSetSpec(spec *apps.StatefulSetSpec, fldPath *field.Path, op // ValidateStatefulSet validates a StatefulSet. func ValidateStatefulSet(statefulSet *apps.StatefulSet, opts apivalidation.PodValidationOptions) field.ErrorList { allErrs := apivalidation.ValidateObjectMeta(&statefulSet.ObjectMeta, true, ValidateStatefulSetName, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateStatefulSetSpec(&statefulSet.Spec, field.NewPath("spec"), opts)...) + setOpts := StatefulSetValidationOptions{ + AllowInvalidServiceName: false, // require valid serviceNames in new StatefulSets + } + allErrs = append(allErrs, ValidateStatefulSetSpec(&statefulSet.Spec, field.NewPath("spec"), opts, setOpts)...) return allErrs } @@ -177,7 +190,10 @@ func ValidateStatefulSetUpdate(statefulSet, oldStatefulSet *apps.StatefulSet, op // thing to do it delete such an instance, but if there is a finalizer, it // would need to pass update validation. Name can't change anyway. allErrs := apivalidation.ValidateObjectMetaUpdate(&statefulSet.ObjectMeta, &oldStatefulSet.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateStatefulSetSpec(&statefulSet.Spec, field.NewPath("spec"), opts)...) + setOpts := StatefulSetValidationOptions{ + AllowInvalidServiceName: true, // serviceName is immutable, tolerate existing invalid names on update + } + allErrs = append(allErrs, ValidateStatefulSetSpec(&statefulSet.Spec, field.NewPath("spec"), opts, setOpts)...) // statefulset updates aren't super common and general updates are likely to be touching spec, so we'll do this // deep copy right away. This avoids mutating our inputs diff --git a/pkg/apis/apps/validation/validation_test.go b/pkg/apis/apps/validation/validation_test.go index 96811ca57c5..42d4035f01f 100644 --- a/pkg/apis/apps/validation/validation_test.go +++ b/pkg/apis/apps/validation/validation_test.go @@ -195,6 +195,12 @@ func tweakPVCScalePolicy(t apps.PersistentVolumeClaimRetentionPolicyType) pvcPol } } +func tweakServiceName(name string) statefulSetTweak { + return func(ss *apps.StatefulSet) { + ss.Spec.ServiceName = name + } +} + func TestValidateStatefulSet(t *testing.T) { validLabels := map[string]string{"a": "b"} validPodTemplate := api.PodTemplate{ @@ -520,6 +526,14 @@ func TestValidateStatefulSet(t *testing.T) { errs: field.ErrorList{ field.Invalid(field.NewPath("spec", "ordinals.start"), nil, ""), }, + }, { + name: "invalid service name", + set: mkStatefulSet(&validPodTemplate, + tweakServiceName("Invalid.Name"), + ), + errs: field.ErrorList{ + field.Invalid(field.NewPath("spec", "serviceName"), "Invalid.Name", ""), + }, }, } @@ -595,7 +609,7 @@ func TestValidateStatefulSetMinReadySeconds(t *testing.T) { for tcName, tc := range testCases { t.Run(tcName, func(t *testing.T) { errs := ValidateStatefulSetSpec(tc.ss, field.NewPath("spec", "minReadySeconds"), - corevalidation.PodValidationOptions{}) + corevalidation.PodValidationOptions{}, StatefulSetValidationOptions{}) if tc.expectErr && len(errs) == 0 { t.Errorf("Unexpected success") } @@ -864,6 +878,10 @@ func TestValidateStatefulSetUpdate(t *testing.T) { name: "update existing instance with .spec.ordinals.start", old: mkStatefulSet(&validPodTemplate), update: mkStatefulSet(&validPodTemplate, tweakOrdinalsStart(3)), + }, { + name: "update with invalid .spec.serviceName", + old: mkStatefulSet(&validPodTemplate, tweakServiceName("Invalid.Name")), + update: mkStatefulSet(&validPodTemplate, tweakServiceName("Invalid.Name"), tweakReplicas(3)), }, } diff --git a/test/integration/statefulset/statefulset_test.go b/test/integration/statefulset/statefulset_test.go index a770e798180..a87bc0eae8b 100644 --- a/test/integration/statefulset/statefulset_test.go +++ b/test/integration/statefulset/statefulset_test.go @@ -768,3 +768,35 @@ func TestStatefulSetStartOrdinal(t *testing.T) { }) } } +func TestStatefulSetPodSubdomain(t *testing.T) { + tCtx, closeFn, rm, informers, c := scSetup(t) + defer closeFn() + ns := framework.CreateNamespaceOrDie(c, "test-pod-subdomain", t) + defer framework.DeleteNamespaceOrDie(c, ns, t) + cancel := runControllerAndInformers(tCtx, rm, informers) + defer cancel() + + // create a headless service + serviceName := "test-service" + service := newHeadlessService(ns.Name) + service.Name = serviceName + createHeadlessService(t, c, service) + + // create StatefulSet with the service name + sts := newSTS("sts", ns.Name, 3) + sts.Spec.ServiceName = serviceName + stss, _ := createSTSsPods(t, c, []*appsv1.StatefulSet{sts}, []*v1.Pod{}) + sts = stss[0] + waitSTSStable(t, c, sts) + + // get pods and verify subdomain + labelMap := labelMap() + podClient := c.CoreV1().Pods(ns.Name) + pods := getPods(t, podClient, labelMap) + + for _, pod := range pods.Items { + if pod.Spec.Subdomain != serviceName { + t.Errorf("Pod %s has incorrect subdomain: got %s, want %s", pod.Name, pod.Spec.Subdomain, serviceName) + } + } +} From d6e5d4f20d4f089a6a0ea1872fd3b709b3dfa218 Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Tue, 18 Feb 2025 11:31:10 +0100 Subject: [PATCH 3/3] make update Signed-off-by: Maciej Szulik --- api/openapi-spec/swagger.json | 3 +-- api/openapi-spec/v3/apis__apps__v1_openapi.json | 3 +-- pkg/generated/openapi/zz_generated.openapi.go | 6 +++--- staging/src/k8s.io/api/apps/v1/generated.proto | 1 + staging/src/k8s.io/api/apps/v1beta1/generated.proto | 1 + staging/src/k8s.io/api/apps/v1beta2/generated.proto | 1 + 6 files changed, 8 insertions(+), 7 deletions(-) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index b6315f4d5e4..9f518b5fac7 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -2826,8 +2826,7 @@ }, "required": [ "selector", - "template", - "serviceName" + "template" ], "type": "object" }, diff --git a/api/openapi-spec/v3/apis__apps__v1_openapi.json b/api/openapi-spec/v3/apis__apps__v1_openapi.json index f45eb000bb1..9c7e1c1875b 100644 --- a/api/openapi-spec/v3/apis__apps__v1_openapi.json +++ b/api/openapi-spec/v3/apis__apps__v1_openapi.json @@ -1171,8 +1171,7 @@ }, "required": [ "selector", - "template", - "serviceName" + "template" ], "type": "object" }, diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go index 765246a719f..caad5638a15 100644 --- a/pkg/generated/openapi/zz_generated.openapi.go +++ b/pkg/generated/openapi/zz_generated.openapi.go @@ -8244,7 +8244,7 @@ func schema_k8sio_api_apps_v1_StatefulSetSpec(ref common.ReferenceCallback) comm }, }, }, - Required: []string{"selector", "template", "serviceName"}, + Required: []string{"selector", "template"}, }, }, Dependencies: []string{ @@ -9396,7 +9396,7 @@ func schema_k8sio_api_apps_v1beta1_StatefulSetSpec(ref common.ReferenceCallback) }, }, }, - Required: []string{"template", "serviceName"}, + Required: []string{"template"}, }, }, Dependencies: []string{ @@ -11109,7 +11109,7 @@ func schema_k8sio_api_apps_v1beta2_StatefulSetSpec(ref common.ReferenceCallback) }, }, }, - Required: []string{"selector", "template", "serviceName"}, + Required: []string{"selector", "template"}, }, }, Dependencies: []string{ diff --git a/staging/src/k8s.io/api/apps/v1/generated.proto b/staging/src/k8s.io/api/apps/v1/generated.proto index 453035ca2e0..ac57a15d3e3 100644 --- a/staging/src/k8s.io/api/apps/v1/generated.proto +++ b/staging/src/k8s.io/api/apps/v1/generated.proto @@ -716,6 +716,7 @@ message StatefulSetSpec { // the network identity of the set. Pods get DNS/hostnames that follow the // pattern: pod-specific-string.serviceName.default.svc.cluster.local // where "pod-specific-string" is managed by the StatefulSet controller. + // +optional optional string serviceName = 5; // podManagementPolicy controls how pods are created during initial scale up, diff --git a/staging/src/k8s.io/api/apps/v1beta1/generated.proto b/staging/src/k8s.io/api/apps/v1beta1/generated.proto index 24abd5b8ba4..74b3ce54999 100644 --- a/staging/src/k8s.io/api/apps/v1beta1/generated.proto +++ b/staging/src/k8s.io/api/apps/v1beta1/generated.proto @@ -462,6 +462,7 @@ message StatefulSetSpec { // the network identity of the set. Pods get DNS/hostnames that follow the // pattern: pod-specific-string.serviceName.default.svc.cluster.local // where "pod-specific-string" is managed by the StatefulSet controller. + // +optional optional string serviceName = 5; // podManagementPolicy controls how pods are created during initial scale up, diff --git a/staging/src/k8s.io/api/apps/v1beta2/generated.proto b/staging/src/k8s.io/api/apps/v1beta2/generated.proto index 735eed7106e..b0d4e3f0894 100644 --- a/staging/src/k8s.io/api/apps/v1beta2/generated.proto +++ b/staging/src/k8s.io/api/apps/v1beta2/generated.proto @@ -761,6 +761,7 @@ message StatefulSetSpec { // the network identity of the set. Pods get DNS/hostnames that follow the // pattern: pod-specific-string.serviceName.default.svc.cluster.local // where "pod-specific-string" is managed by the StatefulSet controller. + // +optional optional string serviceName = 5; // podManagementPolicy controls how pods are created during initial scale up,