From ffcf3ee6f8752b42134e62528226c6268b93dd33 Mon Sep 17 00:00:00 2001 From: Kensei Nakada Date: Fri, 31 Mar 2023 02:15:04 +0000 Subject: [PATCH 1/2] feature(pkg/api): warning for Pod with null labelSelector in PodAffinity and TopologySpread --- pkg/api/pod/warnings.go | 38 ++++++++++++ pkg/api/pod/warnings_test.go | 109 +++++++++++++++++++++++++++++++++-- 2 files changed, 141 insertions(+), 6 deletions(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index f40d5320520..a41b72f8b6d 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -112,6 +112,11 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta msg, )) } + + // warn if labelSelector is empty which is no-match. + if t.LabelSelector == nil { + warnings = append(warnings, fmt.Sprintf("%s: LabelSelector mustn't be empty; it will result in matching with no object", fieldPath.Child("spec", "topologySpreadConstraints").Index(i).Child("labelSelector"))) + } } // use of deprecated annotations @@ -254,5 +259,38 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta if podSpec.TerminationGracePeriodSeconds != nil && *podSpec.TerminationGracePeriodSeconds < 0 { warnings = append(warnings, fmt.Sprintf("%s: must be >= 0; negative values are invalid and will be treated as 1", fieldPath.Child("spec", "terminationGracePeriodSeconds"))) } + + if podSpec.Affinity != nil { + if affinity := podSpec.Affinity.PodAffinity; affinity != nil { + warnings = append(warnings, warningsForPodAffinityTerms(affinity.RequiredDuringSchedulingIgnoredDuringExecution, fieldPath.Child("spec", "affinity", "podAffinity", "requiredDuringSchedulingIgnoredDuringExecution"))...) + warnings = append(warnings, warningsForWeightedPodAffinityTerms(affinity.PreferredDuringSchedulingIgnoredDuringExecution, fieldPath.Child("spec", "affinity", "podAffinity", "preferredDuringSchedulingIgnoredDuringExecution"))...) + } + if affinity := podSpec.Affinity.PodAntiAffinity; affinity != nil { + warnings = append(warnings, warningsForPodAffinityTerms(affinity.RequiredDuringSchedulingIgnoredDuringExecution, fieldPath.Child("spec", "affinity", "podAntiAffinity", "requiredDuringSchedulingIgnoredDuringExecution"))...) + warnings = append(warnings, warningsForWeightedPodAffinityTerms(affinity.PreferredDuringSchedulingIgnoredDuringExecution, fieldPath.Child("spec", "affinity", "podAntiAffinity", "preferredDuringSchedulingIgnoredDuringExecution"))...) + } + } + + return warnings +} + +func warningsForPodAffinityTerms(terms []api.PodAffinityTerm, fieldPath *field.Path) []string { + var warnings []string + for i, t := range terms { + if t.LabelSelector == nil { + warnings = append(warnings, fmt.Sprintf("%s: LabelSelector mustn't be empty; it will result in matching with no object", fieldPath.Index(i).Child("labelSelector"))) + } + } + return warnings +} + +func warningsForWeightedPodAffinityTerms(terms []api.WeightedPodAffinityTerm, fieldPath *field.Path) []string { + var warnings []string + for i, t := range terms { + // warn if labelSelector is empty which is no-match. + if t.PodAffinityTerm.LabelSelector == nil { + warnings = append(warnings, fmt.Sprintf("%s: LabelSelector mustn't be empty; it will result in matching with no object", fieldPath.Index(i).Child("podAffinityTerm", "labelSelector"))) + } + } return warnings } diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index b1b814a4f12..38c38e39ed9 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -401,12 +401,30 @@ func TestWarnings(t *testing.T) { template: &api.PodTemplateSpec{ Spec: api.PodSpec{ TopologySpreadConstraints: []api.TopologySpreadConstraint{ - {TopologyKey: `foo`}, - {TopologyKey: `beta.kubernetes.io/arch`}, - {TopologyKey: `beta.kubernetes.io/os`}, - {TopologyKey: `failure-domain.beta.kubernetes.io/region`}, - {TopologyKey: `failure-domain.beta.kubernetes.io/zone`}, - {TopologyKey: `beta.kubernetes.io/instance-type`}, + { + TopologyKey: `foo`, + LabelSelector: &metav1.LabelSelector{}, + }, + { + TopologyKey: `beta.kubernetes.io/arch`, + LabelSelector: &metav1.LabelSelector{}, + }, + { + TopologyKey: `beta.kubernetes.io/os`, + LabelSelector: &metav1.LabelSelector{}, + }, + { + TopologyKey: `failure-domain.beta.kubernetes.io/region`, + LabelSelector: &metav1.LabelSelector{}, + }, + { + TopologyKey: `failure-domain.beta.kubernetes.io/zone`, + LabelSelector: &metav1.LabelSelector{}, + }, + { + TopologyKey: `beta.kubernetes.io/instance-type`, + LabelSelector: &metav1.LabelSelector{}, + }, }, }, }, @@ -502,6 +520,85 @@ func TestWarnings(t *testing.T) { `spec.terminationGracePeriodSeconds: must be >= 0; negative values are invalid and will be treated as 1`, }, }, + { + name: "empty LabelSelector in topologySpreadConstraints", + template: &api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + { + LabelSelector: &metav1.LabelSelector{}, + }, + { + LabelSelector: nil, + }, + }, + }, + }, + expected: []string{ + `spec.topologySpreadConstraints[1].labelSelector: LabelSelector mustn't be empty; it will result in matching with no object`, + }, + }, + { + name: "empty LabelSelector in PodAffinity", + template: &api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: api.PodSpec{ + Affinity: &api.Affinity{ + PodAffinity: &api.PodAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: []api.PodAffinityTerm{ + { + LabelSelector: &metav1.LabelSelector{}, + }, + { + LabelSelector: nil, + }, + }, + PreferredDuringSchedulingIgnoredDuringExecution: []api.WeightedPodAffinityTerm{ + { + PodAffinityTerm: api.PodAffinityTerm{ + LabelSelector: &metav1.LabelSelector{}, + }, + }, + { + PodAffinityTerm: api.PodAffinityTerm{ + LabelSelector: nil, + }, + }, + }, + }, + PodAntiAffinity: &api.PodAntiAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: []api.PodAffinityTerm{ + { + LabelSelector: &metav1.LabelSelector{}, + }, + { + LabelSelector: nil, + }, + }, + PreferredDuringSchedulingIgnoredDuringExecution: []api.WeightedPodAffinityTerm{ + { + PodAffinityTerm: api.PodAffinityTerm{ + LabelSelector: &metav1.LabelSelector{}, + }, + }, + { + PodAffinityTerm: api.PodAffinityTerm{ + LabelSelector: nil, + }, + }, + }, + }, + }, + }, + }, + expected: []string{ + `spec.affinity.podAffinity.requiredDuringSchedulingIgnoredDuringExecution[1].labelSelector: LabelSelector mustn't be empty; it will result in matching with no object`, + `spec.affinity.podAffinity.preferredDuringSchedulingIgnoredDuringExecution[1].podAffinityTerm.labelSelector: LabelSelector mustn't be empty; it will result in matching with no object`, + `spec.affinity.podAntiAffinity.requiredDuringSchedulingIgnoredDuringExecution[1].labelSelector: LabelSelector mustn't be empty; it will result in matching with no object`, + `spec.affinity.podAntiAffinity.preferredDuringSchedulingIgnoredDuringExecution[1].podAffinityTerm.labelSelector: LabelSelector mustn't be empty; it will result in matching with no object`, + }, + }, } for _, tc := range testcases { From e389d140ae6b941f23349849adfd88b571524b95 Mon Sep 17 00:00:00 2001 From: Kensei Nakada Date: Sat, 1 Apr 2023 10:34:57 +0000 Subject: [PATCH 2/2] fix as suggested --- pkg/api/pod/warnings.go | 6 +++--- pkg/api/pod/warnings_test.go | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index a41b72f8b6d..31a66b116be 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -115,7 +115,7 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta // warn if labelSelector is empty which is no-match. if t.LabelSelector == nil { - warnings = append(warnings, fmt.Sprintf("%s: LabelSelector mustn't be empty; it will result in matching with no object", fieldPath.Child("spec", "topologySpreadConstraints").Index(i).Child("labelSelector"))) + warnings = append(warnings, fmt.Sprintf("%s: a null labelSelector results in matching no pod", fieldPath.Child("spec", "topologySpreadConstraints").Index(i).Child("labelSelector"))) } } @@ -278,7 +278,7 @@ func warningsForPodAffinityTerms(terms []api.PodAffinityTerm, fieldPath *field.P var warnings []string for i, t := range terms { if t.LabelSelector == nil { - warnings = append(warnings, fmt.Sprintf("%s: LabelSelector mustn't be empty; it will result in matching with no object", fieldPath.Index(i).Child("labelSelector"))) + warnings = append(warnings, fmt.Sprintf("%s: a null labelSelector results in matching no pod", fieldPath.Index(i).Child("labelSelector"))) } } return warnings @@ -289,7 +289,7 @@ func warningsForWeightedPodAffinityTerms(terms []api.WeightedPodAffinityTerm, fi for i, t := range terms { // warn if labelSelector is empty which is no-match. if t.PodAffinityTerm.LabelSelector == nil { - warnings = append(warnings, fmt.Sprintf("%s: LabelSelector mustn't be empty; it will result in matching with no object", fieldPath.Index(i).Child("podAffinityTerm", "labelSelector"))) + warnings = append(warnings, fmt.Sprintf("%s: a null labelSelector results in matching no pod", fieldPath.Index(i).Child("podAffinityTerm", "labelSelector"))) } } return warnings diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index 38c38e39ed9..e849fb37bef 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -521,7 +521,7 @@ func TestWarnings(t *testing.T) { }, }, { - name: "empty LabelSelector in topologySpreadConstraints", + name: "null LabelSelector in topologySpreadConstraints", template: &api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{}, Spec: api.PodSpec{ @@ -536,11 +536,11 @@ func TestWarnings(t *testing.T) { }, }, expected: []string{ - `spec.topologySpreadConstraints[1].labelSelector: LabelSelector mustn't be empty; it will result in matching with no object`, + `spec.topologySpreadConstraints[1].labelSelector: a null labelSelector results in matching no pod`, }, }, { - name: "empty LabelSelector in PodAffinity", + name: "null LabelSelector in PodAffinity", template: &api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{}, Spec: api.PodSpec{ @@ -593,10 +593,10 @@ func TestWarnings(t *testing.T) { }, }, expected: []string{ - `spec.affinity.podAffinity.requiredDuringSchedulingIgnoredDuringExecution[1].labelSelector: LabelSelector mustn't be empty; it will result in matching with no object`, - `spec.affinity.podAffinity.preferredDuringSchedulingIgnoredDuringExecution[1].podAffinityTerm.labelSelector: LabelSelector mustn't be empty; it will result in matching with no object`, - `spec.affinity.podAntiAffinity.requiredDuringSchedulingIgnoredDuringExecution[1].labelSelector: LabelSelector mustn't be empty; it will result in matching with no object`, - `spec.affinity.podAntiAffinity.preferredDuringSchedulingIgnoredDuringExecution[1].podAffinityTerm.labelSelector: LabelSelector mustn't be empty; it will result in matching with no object`, + `spec.affinity.podAffinity.requiredDuringSchedulingIgnoredDuringExecution[1].labelSelector: a null labelSelector results in matching no pod`, + `spec.affinity.podAffinity.preferredDuringSchedulingIgnoredDuringExecution[1].podAffinityTerm.labelSelector: a null labelSelector results in matching no pod`, + `spec.affinity.podAntiAffinity.requiredDuringSchedulingIgnoredDuringExecution[1].labelSelector: a null labelSelector results in matching no pod`, + `spec.affinity.podAntiAffinity.preferredDuringSchedulingIgnoredDuringExecution[1].podAffinityTerm.labelSelector: a null labelSelector results in matching no pod`, }, }, }