Add MatchLabelKeys validation for PodTemplateSpec #130534

mochizuki875 · Mar 3, 2025

What type of PR is this?

/kind feature
/sig scheduling

What this PR does / why we need it:

As #130063, when an attempt is made to create an object(e.g. Deployment) containing a PodTemplateSpec with a specific key that exists in both PodAffinity's matchLabelKeys and labelSelector, the PodTemplateSpec validation will pass.
However, the Pods are not created due to the PodSpec validation.
I think it makes users confused, and the PodTemplateSpec validation should fail in this case.

In this PR, I've added MatchLabelKeys validation for PodTemplateSpec to validate such cases.

Which issue(s) this PR fixes:

Fixes #130063

Special notes for your reviewer:

I've added test cases for this PR to TestValidatePodTemplateSpecSeccomp(), which invokes ValidatePodTemplateSpec() for each test case containing PodTemplateSpec.
While the name of TestValidatePodTemplateSpecSeccomp() contains "seccomp", the invoked method ValidatePodTemplateSpec() used for more than just seccomp validation and should have a more general name.
Therefore I've submitted another PR.(#130155)

Does this PR introduce a user-facing change?

When an attempt is made to create an object containing a `PodTemplateSpec`(e.g. `Deployment`, `StatefulSet` etc), the validation will fail if a specific key exists in both `PodAffinity`'s `matchLabelKeys` and `labelSelector` or duplicate keys exist in `matchLabelKeys`.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · Mar 3, 2025

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 · Mar 3, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mochizuki875
Once this PR has been reviewed and has the lgtm label, please assign thockin for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/apis/OWNERS

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

mochizuki875 · Mar 3, 2025

/retest

mochizuki875 · Mar 4, 2025

/retest

haosdent

Maybe we could add some valid cases to cover the following scenarios:

A test where no conflicts exist.
A test with an empty label selector (or even a nil label selector, if allowed) along with non-empty matchLabelKeys to verify that no error is raised when there’s nothing to conflict with.
Include a test case where mismatchLabelKeys and matchLabelKeys are both provided but do not overlap.

haosdent · Mar 3, 2025

pkg/apis/core/validation/validation.go

-				// Here, we found the duplicate key in labelSelector and the key is specified in labelKeys.
-				// Meaning that the same key is specified in both labelSelector and matchLabelKeys/mismatchLabelKeys.
-				allErrs = append(allErrs, field.Invalid(fldPath.Index(i), key, "exists in both matchLabelKeys and labelSelector"))
+		if strings.Contains(fldPath.String(), "template.spec") { // PodTemplateSpec validation


Do we have a robust way to detect PodTemplateSpec, not sure if this check may match unexpected cases in the future.

I've reconsidered and changed to use opts.ResourceIsPod.

haosdent · Mar 4, 2025

pkg/apis/core/validation/validation_test.go

+		expectedErr: field.ErrorList{
+			field.Invalid(rootFld.Child("spec").Child("affinity").Child("podAffinity").Child("preferredDuringSchedulingIgnoredDuringExecution").Index(0).Child("podAffinityTerm").Child("matchLabelKeys").Index(0), "/key", "prefix part must be non-empty"),
+		},
+		spec: &core.PodTemplateSpec{


Do we have test cases to cover core.PodSpec{ scenario

} else { // PodSpec validation // labelKeysMap is keyed by label key and valued by the index of label key in labelKeys. labelKeysMap := map[string]int{} for i, key := range matchLabelKeys {

They have already been added as part of the existing test cases in TestValidatePod().

e.g.

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

Lines 11543 to 11575 in 9d9e1af

"invalid soft pod affinity, key exists in both matchLabelKeys and labelSelector": {

expectedError: "exists in both matchLabelKeys and labelSelector",

spec: *podtest.MakePod("123",

podtest.SetAffinity(&core.Affinity{

PodAffinity: &core.PodAffinity{

PreferredDuringSchedulingIgnoredDuringExecution: []core.WeightedPodAffinityTerm{

{

Weight: 10,

PodAffinityTerm: core.PodAffinityTerm{

LabelSelector: &metav1.LabelSelector{

MatchExpressions: []metav1.LabelSelectorRequirement{

// This one should be created from MatchLabelKeys.

{

Key: "key",

Operator: metav1.LabelSelectorOpIn,

Values: []string{"value1"},

},

{

Key: "key",

Operator: metav1.LabelSelectorOpNotIn,

Values: []string{"value2"},

},

},

},

TopologyKey: "k8s.io/zone",

MatchLabelKeys: []string{"key"},

},

},

},

},

}),

),

},

haosdent · Mar 4, 2025

pkg/apis/core/validation/validation_test.go

+			),
+		},
+	}, {
+		description: "invalid hard pod anti-affinity, key exists in both MatchLabelKeys and MismatchLabelKeys",


Do we need to test positive case, where mismatchLabelKeys and matchLabelKeys are both provided but do not overlap.

I've added positive test cases as successCases.

haosdent · Mar 4, 2025

pkg/apis/core/validation/validation_test.go

+							{
+								LabelSelector:  &metav1.LabelSelector{},
+								TopologyKey:    "k8s.io/zone",
+								MatchLabelKeys: []string{"key1", "key2", "key1"},


Do we have test cases that only a subset of the provided keys is problematic (for example, one key is duplicated or conflicts while others are valid) between MatchLabelKeys/LabelSelector/MismatchLabelKeys

I've fixed the additional test cases.
e.g.

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

Lines 23916 to 23937 in 6dcf62b

podtest.SetAffinity(&core.Affinity{

PodAffinity: &core.PodAffinity{

PreferredDuringSchedulingIgnoredDuringExecution: []core.WeightedPodAffinityTerm{

{

PodAffinityTerm: core.PodAffinityTerm{

LabelSelector: &metav1.LabelSelector{

MatchExpressions: []metav1.LabelSelectorRequirement{

{

Key: "key1",

Operator: metav1.LabelSelectorOpIn,

Values: []string{"value1"},

},

{

Key: "key2",

Operator: metav1.LabelSelectorOpIn,

Values: []string{"value2"},

},

},

},

TopologyKey: "k8s.io/zone",

MatchLabelKeys: []string{"key1", "key3"},

},

mochizuki875 · Mar 4, 2025

The origin information "labelKey" has been added to the error of ValidateLabelName() by #130388, and expectedErr of related test cases are required to be added .WithOrigin("labelKey") to pass the unit test.
Also, based on #130388, we might need to fix to replace error comparisons using assert.Equal() with error comparisons using fldtest.MatchErrors() like other test functions(e.g. TestValidateTopologySpreadConstraints()).

mochizuki875 · Mar 4, 2025

@haosdent
Thank you for your comments!
I'll address them later.

mochizuki875 · Mar 5, 2025

@haosdent
OK, I've addressed.

k8s-ci-robot · May 8, 2025

PR needs rebase.

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 added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 3, 2025

mochizuki875 changed the title ~~add MatchLabelKeys validation for PodTemplateSpec~~ Add MatchLabelKeys validation for PodTemplateSpec Mar 3, 2025

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Mar 3, 2025

github-project-automation bot added this to SIG Apps Mar 3, 2025

github-project-automation bot moved this to Needs Triage in SIG Apps Mar 3, 2025

k8s-ci-robot requested review from thockin and yujuhong March 3, 2025 07:45

mochizuki875 force-pushed the validate_podtemplate_matchlabelkeys branch from 889738e to ac8226b Compare March 3, 2025 07:49

haosdent reviewed Mar 4, 2025

View reviewed changes

add MatchLabelKeys validation of PodTemplateSpec

0ab306a

mochizuki875 force-pushed the validate_podtemplate_matchlabelkeys branch from ac8226b to 0ab306a Compare March 4, 2025 04:54

mochizuki875 mentioned this pull request Mar 4, 2025

Duplicate keys in MatchLabelKeys of PodSpec should be explicitly validated #130554

Open

fix1

6dcf62b

This was referenced Mar 6, 2025

WIP: Add validation for duplicate keys in MatchLabelKeys #130608

Closed

Add validation for duplicate keys in MatchLabelKeys #130614

Open

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2025

	"invalid soft pod affinity, key exists in both matchLabelKeys and labelSelector": {
	expectedError: "exists in both matchLabelKeys and labelSelector",
	spec: *podtest.MakePod("123",
	podtest.SetAffinity(&core.Affinity{
	PodAffinity: &core.PodAffinity{
	PreferredDuringSchedulingIgnoredDuringExecution: []core.WeightedPodAffinityTerm{
	{
	Weight: 10,
	PodAffinityTerm: core.PodAffinityTerm{
	LabelSelector: &metav1.LabelSelector{
	MatchExpressions: []metav1.LabelSelectorRequirement{
	// This one should be created from MatchLabelKeys.
	{
	Key: "key",
	Operator: metav1.LabelSelectorOpIn,
	Values: []string{"value1"},
	},
	{
	Key: "key",
	Operator: metav1.LabelSelectorOpNotIn,
	Values: []string{"value2"},
	},
	},
	},
	TopologyKey: "k8s.io/zone",
	MatchLabelKeys: []string{"key"},
	},
	},
	},
	},
	}),
	),
	},

Search code, repositories, users, issues, pull requests...

Add MatchLabelKeys validation for PodTemplateSpec #130534

Are you sure you want to change the base?

Add MatchLabelKeys validation for PodTemplateSpec #130534

Uh oh!

Conversation

mochizuki875 commented Mar 3, 2025

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?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Mar 3, 2025

Uh oh!

k8s-ci-robot commented Mar 3, 2025

Uh oh!

mochizuki875 commented Mar 3, 2025

Uh oh!

mochizuki875 commented Mar 4, 2025

Uh oh!

haosdent left a comment

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!

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!

mochizuki875 commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mochizuki875 commented Mar 4, 2025

Uh oh!

mochizuki875 commented Mar 5, 2025

Uh oh!

k8s-ci-robot commented May 8, 2025

Uh oh!

Uh oh!

mochizuki875 commented Mar 4, 2025 •

edited

Loading