fix pod template spec validation missing in sts #131790

chengjoey · 2025-05-15T11:07:52Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

add pod spec validation for create sts

Which issue(s) this PR fixes:

Fixes #131761 #112609

Special notes for your reviewer:

Does this PR introduce a user-facing change?

add podSpec validation for create StatefulSet

k8s-ci-robot · 2025-05-15T11:08:02Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2025-05-15T11:08:03Z

Hi @chengjoey. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

pacoxu · 2025-05-16T09:24:51Z

/ok-to-test
/cc @adrianmoisey

adrianmoisey · 2025-05-16T10:21:19Z

The failed tests seems to need fixing

chengjoey · 2025-05-17T06:33:17Z

/test pull-kubernetes-unit

chengjoey · 2025-05-17T07:06:06Z

The failed tests seems to need fixing

done

liggitt · 2025-06-17T14:34:19Z

pkg/apis/apps/validation/validation.go

-		// volume mounts in the containers.
-		// allErrs = append(allErrs, apivalidation.ValidatePodTemplateSpec(template, fldPath)...)
+		// TODO: Add validation for PodSpec containers volumeMounts if volumeClaimTemplates are defined
+		allErrs = append(allErrs, apivalidation.ValidatePodTemplateSpec(template, fldPath, opts)...)


Tightening validation here can only be done conditionally... we have to tolerate updates to existing statefulset objects that wouldn't pass this validation

I think that looks like:

adding an SkipValidatePodTemplateSpec bool option to StatefulSetValidationOptions

plumbing that from ValidateStatefulSetSpec down into this function

calling this line if and only if setOpts.SkipValidatePodTemplateSpec is false

in ValidateStatefulSet (for create), set SkipValidatePodTemplateSpec to false. in ValidateStatefulSetUpdate (for update), set SkipValidatePodTemplateSpec to true if and only if oldStatefulSet fails ValidatePodTemplateSpec

done

I got it, we should only validate the creation of sts, and tolerate the existing.

liggitt · 2025-06-17T14:35:29Z

pkg/apis/core/validation/validation.go

@@ -2928,6 +2928,9 @@ func ValidateVolumeMounts(mounts []core.VolumeMount, voldevices map[string]strin
 		if len(mnt.Name) == 0 {
 			allErrs = append(allErrs, field.Required(idxPath.Child("name"), ""))
 		}
+		if opts.AllowVolumeNotFoundInVolumeMounts && !IsMatchedVolume(mnt.Name, volumes) {


is there a way to avoid skipping this validation by having statefulset validation modify a temporary copy of the pod template spec being validated in the same way the controller will to inject the volumes that will be constructed?

sort of like

kubernetes/pkg/controller/statefulset/stateful_set_utils.go

Lines 411 to 435 in ced19ff

// updateStorage updates pod's Volumes to conform with the PersistentVolumeClaim of set's templates. If pod has

// conflicting local Volumes these are replaced with Volumes that conform to the set's templates.

func updateStorage(set *apps.StatefulSet, pod *v1.Pod) {

currentVolumes := pod.Spec.Volumes

claims := getPersistentVolumeClaims(set, pod)

newVolumes := make([]v1.Volume, 0, len(claims))

for name, claim := range claims {

newVolumes = append(newVolumes, v1.Volume{

Name: name,

VolumeSource: v1.VolumeSource{

PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{

ClaimName: claim.Name,

// TODO: Use source definition to set this value when we have one.

ReadOnly: false,

},

},

})

}

for i := range currentVolumes {

if _, ok := claims[currentVolumes[i].Name]; !ok {

newVolumes = append(newVolumes, currentVolumes[i])

}

}

pod.Spec.Volumes = newVolumes

}

chengjoey · 2025-06-19T14:25:02Z

/test pull-kubernetes-unit

chengjoey · 2025-06-19T14:58:42Z

/test pull-kubernetes-unit

liggitt · 2025-06-19T16:38:43Z

pkg/apis/core/validation/validation.go

+	// Skip validating pod template spec, specifically for StatefulSet.
+	SkipValidatePodTemplateSpec bool


move this into the StatefulSetValidationOptions type, and plumb that options instance into ValidatePodTemplateSpecForStatefulSet where it is needed

liggitt · 2025-06-19T16:48:30Z

pkg/apis/apps/validation/validation.go

+	spec.Template.Spec.Volumes = newVolumes
+}
+
+func getPersistentVolumeClaims(spec *apps.StatefulSetSpec) map[string]api.PersistentVolumeClaim {


I think we just need to return []string with the claim names to add the volumes to the pod spec like we need to

liggitt · 2025-06-19T16:51:03Z

pkg/apis/apps/validation/validation.go

@@ -193,6 +238,9 @@ func ValidateStatefulSetUpdate(statefulSet, oldStatefulSet *apps.StatefulSet, op
 	setOpts := StatefulSetValidationOptions{
 		AllowInvalidServiceName: true, // serviceName is immutable, tolerate existing invalid names on update
 	}
+	// In order to tolerate the existing sts, we choose to skip the validation error of podTemplateSpec.
+	// TODO: pod template spec validation for statefulset update
+	opts.SkipValidatePodTemplateSpec = true


this should look more like if len(ValidateStatefulSetSpec(&oldStatefulSet.Spec, nil, opts, setOpts)) > 0 { opts.SkipValidatePodTemplateSpec = true } so we only relax validation on update for things which already fail spec validation

liggitt · 2025-06-19T16:57:35Z

pkg/apis/apps/validation/validation.go

@@ -158,6 +201,8 @@ func ValidateStatefulSetSpec(spec *apps.StatefulSetSpec, fldPath *field.Path, op
 	if err != nil {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), spec.Selector, ""))
 	} else {
+		// update the template's volumes to include the volumes from the volumeClaimTemplates
+		updateStorageForStatefulSet(spec)


don't mutate this in place ... if we need to add in volumes for validation purposes to match what will get auto-added by the controller, do that by assigning to a new variable, deep-copying, and appending the fewest new volumes we need, something like this:

templateToValidate := &spec.Template if volumesToAddForTemplates(spec); len(volumesToAddForTemplates) > 0 { templateToValidate = templateToValidate.DeepCopy() templateToValidate.Spec.Volumes = append(templateToValidate.Spec.Volumes, ...) } allErrs = append(allErrs, ValidatePodTemplateSpecForStatefulSet(templateToValidate, selector, fldPath.Child("template"), opts)...)

done, and add some test cases for update

liggitt · 2025-06-24T13:56:05Z

pkg/apis/apps/validation/validation.go

+		podSpecErrs := apivalidation.ValidatePodTemplateSpec(template, fldPath, opts)
+		if len(podSpecErrs) > 0 && !setOpts.SkipValidatePodTemplateSpec {
+			allErrs = append(allErrs, podSpecErrs...)


check the boolean first to avoid doing work we'll throw away

Suggested change

podSpecErrs := apivalidation.ValidatePodTemplateSpec(template, fldPath, opts)

if len(podSpecErrs) > 0 && !setOpts.SkipValidatePodTemplateSpec {

allErrs = append(allErrs, podSpecErrs...)

if !setOpts.SkipValidatePodTemplateSpec {

allErrs = append(allErrs, apivalidation.ValidatePodTemplateSpec(template, fldPath, opts)...)

pkg/apis/apps/validation/validation.go

Signed-off-by: joey <[email protected]>

liggitt · 2025-07-01T14:20:46Z

/retest

liggitt · 2025-07-01T14:22:41Z

/lgtm
/approve

Thanks for the improvement here and the tests!

k8s-ci-robot · 2025-07-01T14:22:48Z

LGTM label has been added.

Git tree hash: ce713cfd8cb38a474496c4e9c6dcd3052cd5b447

k8s-ci-robot · 2025-07-01T14:22:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chengjoey, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/registry/apps/OWNERS~~ [liggitt]
~~test/integration/etcd/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 15, 2025

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 15, 2025

github-project-automation bot added this to SIG Apps May 15, 2025

github-project-automation bot moved this to Needs Triage in SIG Apps May 15, 2025

k8s-ci-robot requested review from deads2k and tallclair May 15, 2025 11:08

k8s-ci-robot requested a review from adrianmoisey May 16, 2025 09:24

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 16, 2025

chengjoey force-pushed the fix/sts-validation branch from 5989a2b to 5287322 Compare May 16, 2025 15:47

chengjoey force-pushed the fix/sts-validation branch from 5287322 to 6a79033 Compare May 17, 2025 00:01

liggitt added this to @liggitt Jun 16, 2025

liggitt mentioned this pull request Jun 16, 2025

StatefulSet allows invalid pod specs that break pod creation (e.g. null or empty nodeSelectorTerms) #112609

Open

liggitt reviewed Jun 17, 2025

View reviewed changes

chengjoey force-pushed the fix/sts-validation branch from 6a79033 to d230082 Compare June 19, 2025 13:58

liggitt reviewed Jun 19, 2025

View reviewed changes

chengjoey force-pushed the fix/sts-validation branch from d230082 to bc33879 Compare June 20, 2025 07:22

liggitt reviewed Jun 24, 2025

View reviewed changes

chengjoey force-pushed the fix/sts-validation branch from bc33879 to eeecf20 Compare June 25, 2025 02:42

liggitt reviewed Jun 26, 2025

View reviewed changes

pkg/apis/apps/validation/validation.go Outdated Show resolved Hide resolved

pkg/apis/apps/validation/validation.go Outdated Show resolved Hide resolved

fix pod template spec validation missing in sts

dfd34a5

Signed-off-by: joey <[email protected]>

chengjoey force-pushed the fix/sts-validation branch from eeecf20 to dfd34a5 Compare June 27, 2025 08:31

k8s-ci-robot assigned liggitt Jul 1, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2025

k8s-ci-robot merged commit a9841ca into kubernetes:master Jul 1, 2025
13 checks passed

k8s-ci-robot added this to the v1.34 milestone Jul 1, 2025

github-project-automation bot moved this to Done in @liggitt Jul 1, 2025

github-project-automation bot moved this from Needs Triage to Done in SIG Apps Jul 1, 2025

	// updateStorage updates pod's Volumes to conform with the PersistentVolumeClaim of set's templates. If pod has
	// conflicting local Volumes these are replaced with Volumes that conform to the set's templates.
	func updateStorage(set apps.StatefulSet, pod v1.Pod) {
	currentVolumes := pod.Spec.Volumes
	claims := getPersistentVolumeClaims(set, pod)
	newVolumes := make([]v1.Volume, 0, len(claims))
	for name, claim := range claims {
	newVolumes = append(newVolumes, v1.Volume{
	Name: name,
	VolumeSource: v1.VolumeSource{
	PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{
	ClaimName: claim.Name,
	// TODO: Use source definition to set this value when we have one.
	ReadOnly: false,
	},
	},
	})
	}
	for i := range currentVolumes {
	if _, ok := claims[currentVolumes[i].Name]; !ok {
	newVolumes = append(newVolumes, currentVolumes[i])
	}
	}
	pod.Spec.Volumes = newVolumes
	}

		// Skip validating pod template spec, specifically for StatefulSet.
		SkipValidatePodTemplateSpec bool

fix pod template spec validation missing in sts #131790

fix pod template spec validation missing in sts #131790

Conversation

chengjoey commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

k8s-ci-robot commented May 15, 2025

Uh oh!

k8s-ci-robot commented May 15, 2025

Uh oh!

pacoxu commented May 16, 2025

Uh oh!

adrianmoisey commented May 16, 2025

Uh oh!

chengjoey commented May 17, 2025

Uh oh!

chengjoey commented May 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chengjoey commented Jun 19, 2025

Uh oh!

chengjoey commented Jun 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liggitt commented Jul 1, 2025

Uh oh!

liggitt commented Jul 1, 2025

Uh oh!

k8s-ci-robot commented Jul 1, 2025

Uh oh!

k8s-ci-robot commented Jul 1, 2025

Uh oh!

Uh oh!

Uh oh!

chengjoey commented May 15, 2025 •

edited

Loading