Skip to content

Commit 3bec245

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#128212 from AxeZhan/selecor_validation
Add validation for NodeSelectorRequirement's values
2 parents e8615e2 + ae11c7d commit 3bec245

File tree

10 files changed

+366
-13
lines changed

10 files changed

+366
-13
lines changed

‎pkg/api/node/util.go‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"fmt"
2121

2222
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
23+
"k8s.io/apimachinery/pkg/util/validation"
2324
"k8s.io/apimachinery/pkg/util/validation/field"
2425
api "k8s.io/kubernetes/pkg/apis/core"
2526
"k8s.io/kubernetes/pkg/apis/node"
@@ -91,7 +92,7 @@ func GetWarningsForNodeSelector(nodeSelector *metav1.LabelSelector, fieldPath *f
9192
}
9293

9394
// GetWarningsForNodeSelectorTerm checks match expressions of node selector term
94-
func GetWarningsForNodeSelectorTerm(nodeSelectorTerm api.NodeSelectorTerm, fieldPath *field.Path) []string {
95+
func GetWarningsForNodeSelectorTerm(nodeSelectorTerm api.NodeSelectorTerm, checkLabelValue bool, fieldPath *field.Path) []string {
9596
var warnings []string
9697
// use of deprecated node labels in matchLabelExpressions
9798
for i, expression := range nodeSelectorTerm.MatchExpressions {
@@ -106,6 +107,19 @@ func GetWarningsForNodeSelectorTerm(nodeSelectorTerm api.NodeSelectorTerm, field
106107
),
107108
)
108109
}
110+
if checkLabelValue {
111+
for index, value := range expression.Values {
112+
for _, msg := range validation.IsValidLabelValue(value) {
113+
warnings = append(warnings,
114+
fmt.Sprintf(
115+
"%s: %s is invalid, %s",
116+
fieldPath.Child("matchExpressions").Index(i).Child("values").Index(index),
117+
value,
118+
msg,
119+
))
120+
}
121+
}
122+
}
109123
}
110124
return warnings
111125
}

‎pkg/api/persistentvolume/util.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ func warningsForPersistentVolumeSpecAndMeta(fieldPath *field.Path, pvSpec *api.P
7676
termFldPath := fieldPath.Child("spec", "nodeAffinity", "required", "nodeSelectorTerms")
7777
// use of deprecated node labels in node affinity
7878
for i, term := range pvSpec.NodeAffinity.Required.NodeSelectorTerms {
79-
warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term, termFldPath.Index(i))...)
79+
warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term, false, termFldPath.Index(i))...)
8080
}
8181
}
8282
// If we are on deprecated volume plugin

‎pkg/api/pod/util.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,7 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po
385385
AllowNonLocalProjectedTokenPath: false,
386386
AllowPodLifecycleSleepActionZeroValue: utilfeature.DefaultFeatureGate.Enabled(features.PodLifecycleSleepActionAllowZero),
387387
PodLevelResourcesEnabled: utilfeature.DefaultFeatureGate.Enabled(features.PodLevelResources),
388+
AllowInvalidLabelValueInRequiredNodeAffinity: false,
388389
}
389390

390391
// If old spec uses relaxed validation or enabled the RelaxedEnvironmentVariableValidation feature gate,
@@ -399,6 +400,7 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po
399400
opts.AllowIndivisibleHugePagesValues = usesIndivisibleHugePagesValues(oldPodSpec)
400401

401402
opts.AllowInvalidLabelValueInSelector = hasInvalidLabelValueInAffinitySelector(oldPodSpec)
403+
opts.AllowInvalidLabelValueInRequiredNodeAffinity = hasInvalidLabelValueInRequiredNodeAffinity(oldPodSpec)
402404
// if old spec has invalid labelSelector in topologySpreadConstraint, we must allow it
403405
opts.AllowInvalidTopologySpreadConstraintLabelSelector = hasInvalidTopologySpreadConstraintLabelSelector(oldPodSpec)
404406
// if old spec has an invalid projected token volume path, we must allow it
@@ -1271,6 +1273,16 @@ func IsRestartableInitContainer(initContainer *api.Container) bool {
12711273
return *initContainer.RestartPolicy == api.ContainerRestartPolicyAlways
12721274
}
12731275

1276+
func hasInvalidLabelValueInRequiredNodeAffinity(spec *api.PodSpec) bool {
1277+
if spec == nil ||
1278+
spec.Affinity == nil ||
1279+
spec.Affinity.NodeAffinity == nil ||
1280+
spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil {
1281+
return false
1282+
}
1283+
return helper.HasInvalidLabelValueInNodeSelectorTerms(spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms)
1284+
}
1285+
12741286
func MarkPodProposedForResize(oldPod, newPod *api.Pod) {
12751287
if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) {
12761288
// Update is invalid: ignore changes and let validation handle it

‎pkg/api/pod/util_test.go‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4204,3 +4204,77 @@ func TestValidateAllowSidecarResizePolicy(t *testing.T) {
42044204
})
42054205
}
42064206
}
4207+
4208+
func TestValidateInvalidLabelValueInNodeSelectorOption(t *testing.T) {
4209+
testCases := []struct {
4210+
name string
4211+
oldPodSpec *api.PodSpec
4212+
wantOption bool
4213+
}{
4214+
{
4215+
name: "Create",
4216+
wantOption: false,
4217+
},
4218+
{
4219+
name: "UpdateInvalidLabelSelector",
4220+
oldPodSpec: &api.PodSpec{
4221+
Affinity: &api.Affinity{
4222+
NodeAffinity: &api.NodeAffinity{
4223+
RequiredDuringSchedulingIgnoredDuringExecution: &api.NodeSelector{
4224+
NodeSelectorTerms: []api.NodeSelectorTerm{{
4225+
MatchExpressions: []api.NodeSelectorRequirement{{
4226+
Key: "foo",
4227+
Operator: api.NodeSelectorOpIn,
4228+
Values: []string{"-1"},
4229+
}},
4230+
}},
4231+
},
4232+
},
4233+
},
4234+
},
4235+
wantOption: true,
4236+
},
4237+
{
4238+
name: "UpdateValidLabelSelector",
4239+
oldPodSpec: &api.PodSpec{
4240+
Affinity: &api.Affinity{
4241+
NodeAffinity: &api.NodeAffinity{
4242+
RequiredDuringSchedulingIgnoredDuringExecution: &api.NodeSelector{
4243+
NodeSelectorTerms: []api.NodeSelectorTerm{{
4244+
MatchExpressions: []api.NodeSelectorRequirement{{
4245+
Key: "foo",
4246+
Operator: api.NodeSelectorOpIn,
4247+
Values: []string{"bar"},
4248+
}},
4249+
}},
4250+
},
4251+
},
4252+
},
4253+
},
4254+
wantOption: false,
4255+
},
4256+
{
4257+
name: "UpdateEmptyLabelSelector",
4258+
oldPodSpec: &api.PodSpec{
4259+
Affinity: &api.Affinity{
4260+
NodeAffinity: &api.NodeAffinity{
4261+
RequiredDuringSchedulingIgnoredDuringExecution: &api.NodeSelector{
4262+
NodeSelectorTerms: []api.NodeSelectorTerm{},
4263+
},
4264+
},
4265+
},
4266+
},
4267+
wantOption: false,
4268+
},
4269+
}
4270+
4271+
for _, tc := range testCases {
4272+
t.Run(tc.name, func(t *testing.T) {
4273+
// Pod meta doesn't impact the outcome.
4274+
gotOptions := GetValidationOptionsFromPodSpecAndMeta(&api.PodSpec{}, tc.oldPodSpec, nil, nil)
4275+
if tc.wantOption != gotOptions.AllowInvalidLabelValueInRequiredNodeAffinity {
4276+
t.Errorf("Got AllowInvalidLabelValueInRequiredNodeAffinity=%t, want %t", gotOptions.AllowInvalidLabelValueInRequiredNodeAffinity, tc.wantOption)
4277+
}
4278+
})
4279+
}
4280+
}

‎pkg/api/pod/warnings.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,12 +96,12 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta
9696
if n.RequiredDuringSchedulingIgnoredDuringExecution != nil {
9797
termFldPath := fieldPath.Child("spec", "affinity", "nodeAffinity", "requiredDuringSchedulingIgnoredDuringExecution", "nodeSelectorTerms")
9898
for i, term := range n.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms {
99-
warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term, termFldPath.Index(i))...)
99+
warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term, false, termFldPath.Index(i))...)
100100
}
101101
}
102102
preferredFldPath := fieldPath.Child("spec", "affinity", "nodeAffinity", "preferredDuringSchedulingIgnoredDuringExecution")
103103
for i, term := range n.PreferredDuringSchedulingIgnoredDuringExecution {
104-
warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term.Preference, preferredFldPath.Index(i).Child("preference"))...)
104+
warnings = append(warnings, nodeapi.GetWarningsForNodeSelectorTerm(term.Preference, true, preferredFldPath.Index(i).Child("preference"))...)
105105
}
106106
}
107107
for i, t := range podSpec.TopologySpreadConstraints {

‎pkg/api/pod/warnings_test.go‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1711,6 +1711,62 @@ func TestWarnings(t *testing.T) {
17111711
`spec.containers[0].ports[1]: duplicate port name "test" with spec.initContainers[0].ports[0], services and probes that select ports by name will use spec.initContainers[0].ports[0]`,
17121712
},
17131713
},
1714+
{
1715+
name: "creating pod with invalid value in nodeaffinity",
1716+
template: &api.PodTemplateSpec{Spec: api.PodSpec{
1717+
Affinity: &api.Affinity{NodeAffinity: &api.NodeAffinity{
1718+
PreferredDuringSchedulingIgnoredDuringExecution: []api.PreferredSchedulingTerm{{
1719+
Weight: 10,
1720+
Preference: api.NodeSelectorTerm{
1721+
MatchExpressions: []api.NodeSelectorRequirement{{
1722+
Key: "foo",
1723+
Operator: api.NodeSelectorOpIn,
1724+
Values: []string{"-1"},
1725+
}},
1726+
},
1727+
}},
1728+
}},
1729+
}},
1730+
expected: []string{
1731+
`spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution[0].preference.matchExpressions[0].values[0]: -1 is invalid, a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')`,
1732+
},
1733+
},
1734+
{
1735+
name: "updating pod with invalid value in nodeaffinity",
1736+
template: &api.PodTemplateSpec{Spec: api.PodSpec{
1737+
Affinity: &api.Affinity{NodeAffinity: &api.NodeAffinity{
1738+
PreferredDuringSchedulingIgnoredDuringExecution: []api.PreferredSchedulingTerm{{
1739+
Weight: 10,
1740+
Preference: api.NodeSelectorTerm{
1741+
MatchExpressions: []api.NodeSelectorRequirement{{
1742+
Key: "foo",
1743+
Operator: api.NodeSelectorOpIn,
1744+
Values: []string{"-1"},
1745+
}},
1746+
},
1747+
}},
1748+
}},
1749+
SchedulingGates: []api.PodSchedulingGate{{Name: "foo"}},
1750+
}},
1751+
oldTemplate: &api.PodTemplateSpec{Spec: api.PodSpec{
1752+
Affinity: &api.Affinity{NodeAffinity: &api.NodeAffinity{
1753+
PreferredDuringSchedulingIgnoredDuringExecution: []api.PreferredSchedulingTerm{{
1754+
Weight: 10,
1755+
Preference: api.NodeSelectorTerm{
1756+
MatchExpressions: []api.NodeSelectorRequirement{{
1757+
Key: "foo",
1758+
Operator: api.NodeSelectorOpIn,
1759+
Values: []string{"bar"},
1760+
}},
1761+
},
1762+
}},
1763+
}},
1764+
SchedulingGates: []api.PodSchedulingGate{{Name: "foo"}},
1765+
}},
1766+
expected: []string{
1767+
`spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution[0].preference.matchExpressions[0].values[0]: -1 is invalid, a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')`,
1768+
},
1769+
},
17141770
}
17151771

17161772
for _, tc := range testcases {

‎pkg/apis/core/helper/helpers.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -500,3 +500,18 @@ func validFirstDigit(str string) bool {
500500
}
501501
return str[0] == '-' || (str[0] == '0' && str == "0") || (str[0] >= '1' && str[0] <= '9')
502502
}
503+
504+
// HasInvalidLabelValueInNodeSelectorTerms checks if there's an invalid label value
505+
// in one NodeSelectorTerm's MatchExpression values
506+
func HasInvalidLabelValueInNodeSelectorTerms(terms []core.NodeSelectorTerm) bool {
507+
for _, term := range terms {
508+
for _, expression := range term.MatchExpressions {
509+
for _, value := range expression.Values {
510+
if len(validation.IsValidLabelValue(value)) > 0 {
511+
return true
512+
}
513+
}
514+
}
515+
}
516+
return false
517+
}

‎pkg/apis/core/helper/helpers_test.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,3 +369,48 @@ func TestIsServiceIPSet(t *testing.T) {
369369
})
370370
}
371371
}
372+
373+
func TestHasInvalidLabelValueInNodeSelectorTerms(t *testing.T) {
374+
testCases := []struct {
375+
name string
376+
terms []core.NodeSelectorTerm
377+
expect bool
378+
}{
379+
{
380+
name: "valid values",
381+
terms: []core.NodeSelectorTerm{{
382+
MatchExpressions: []core.NodeSelectorRequirement{{
383+
Key: "foo",
384+
Operator: core.NodeSelectorOpIn,
385+
Values: []string{"far"},
386+
}},
387+
}},
388+
expect: false,
389+
},
390+
{
391+
name: "empty terms",
392+
terms: []core.NodeSelectorTerm{},
393+
expect: false,
394+
},
395+
{
396+
name: "invalid label value",
397+
terms: []core.NodeSelectorTerm{{
398+
MatchExpressions: []core.NodeSelectorRequirement{{
399+
Key: "foo",
400+
Operator: core.NodeSelectorOpIn,
401+
Values: []string{"-1"},
402+
}},
403+
}},
404+
expect: true,
405+
},
406+
}
407+
408+
for _, tc := range testCases {
409+
t.Run(tc.name, func(t *testing.T) {
410+
got := HasInvalidLabelValueInNodeSelectorTerms(tc.terms)
411+
if got != tc.expect {
412+
t.Errorf("exepct %v, got %v", tc.expect, got)
413+
}
414+
})
415+
}
416+
}

‎pkg/apis/core/validation/validation.go‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1777,6 +1777,8 @@ var allowedTemplateObjectMetaFields = map[string]bool{
17771777
type PersistentVolumeSpecValidationOptions struct {
17781778
// Allow users to modify the class of volume attributes
17791779
EnableVolumeAttributesClass bool
1780+
// Allow invalid label-value in RequiredNodeSelector
1781+
AllowInvalidLabelValueInRequiredNodeAffinity bool
17801782
}
17811783

17821784
// ValidatePersistentVolumeName checks that a name is appropriate for a
@@ -1798,11 +1800,17 @@ var supportedVolumeModes = sets.New(core.PersistentVolumeBlock, core.PersistentV
17981800

17991801
func ValidationOptionsForPersistentVolume(pv, oldPv *core.PersistentVolume) PersistentVolumeSpecValidationOptions {
18001802
opts := PersistentVolumeSpecValidationOptions{
1801-
EnableVolumeAttributesClass: utilfeature.DefaultMutableFeatureGate.Enabled(features.VolumeAttributesClass),
1803+
EnableVolumeAttributesClass: utilfeature.DefaultMutableFeatureGate.Enabled(features.VolumeAttributesClass),
1804+
AllowInvalidLabelValueInRequiredNodeAffinity: false,
18021805
}
18031806
if oldPv != nil && oldPv.Spec.VolumeAttributesClassName != nil {
18041807
opts.EnableVolumeAttributesClass = true
18051808
}
1809+
if oldPv != nil && oldPv.Spec.NodeAffinity != nil &&
1810+
oldPv.Spec.NodeAffinity.Required != nil {
1811+
terms := oldPv.Spec.NodeAffinity.Required.NodeSelectorTerms
1812+
opts.AllowInvalidLabelValueInRequiredNodeAffinity = helper.HasInvalidLabelValueInNodeSelectorTerms(terms)
1813+
}
18061814
return opts
18071815
}
18081816

@@ -1874,7 +1882,7 @@ func ValidatePersistentVolumeSpec(pvSpec *core.PersistentVolumeSpec, pvName stri
18741882
if validateInlinePersistentVolumeSpec {
18751883
allErrs = append(allErrs, field.Forbidden(fldPath.Child("nodeAffinity"), "may not be specified in the context of inline volumes"))
18761884
} else {
1877-
nodeAffinitySpecified, errs = validateVolumeNodeAffinity(pvSpec.NodeAffinity, fldPath.Child("nodeAffinity"))
1885+
nodeAffinitySpecified, errs = validateVolumeNodeAffinity(pvSpec.NodeAffinity, opts, fldPath.Child("nodeAffinity"))
18781886
allErrs = append(allErrs, errs...)
18791887
}
18801888
}
@@ -3865,7 +3873,7 @@ func validateAffinity(affinity *core.Affinity, opts PodValidationOptions, fldPat
38653873

38663874
if affinity != nil {
38673875
if affinity.NodeAffinity != nil {
3868-
allErrs = append(allErrs, validateNodeAffinity(affinity.NodeAffinity, fldPath.Child("nodeAffinity"))...)
3876+
allErrs = append(allErrs, validateNodeAffinity(affinity.NodeAffinity, opts, fldPath.Child("nodeAffinity"))...)
38693877
}
38703878
if affinity.PodAffinity != nil {
38713879
allErrs = append(allErrs, validatePodAffinity(affinity.PodAffinity, opts.AllowInvalidLabelValueInSelector, fldPath.Child("podAffinity"))...)
@@ -4053,6 +4061,8 @@ type PodValidationOptions struct {
40534061
PodLevelResourcesEnabled bool
40544062
// Allow sidecar containers resize policy for backward compatibility
40554063
AllowSidecarResizePolicy bool
4064+
// Allow invalid label-value in RequiredNodeSelector
4065+
AllowInvalidLabelValueInRequiredNodeAffinity bool
40564066
}
40574067

40584068
// validatePodMetadataAndSpec tests if required fields in the pod.metadata and pod.spec are set,
@@ -4730,14 +4740,14 @@ func validatePodAntiAffinity(podAntiAffinity *core.PodAntiAffinity, allowInvalid
47304740
}
47314741

47324742
// validateNodeAffinity tests that the specified nodeAffinity fields have valid data
4733-
func validateNodeAffinity(na *core.NodeAffinity, fldPath *field.Path) field.ErrorList {
4743+
func validateNodeAffinity(na *core.NodeAffinity, opts PodValidationOptions, fldPath *field.Path) field.ErrorList {
47344744
allErrs := field.ErrorList{}
47354745
// TODO: Uncomment the next three lines once RequiredDuringSchedulingRequiredDuringExecution is implemented.
47364746
// if na.RequiredDuringSchedulingRequiredDuringExecution != nil {
47374747
// allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingRequiredDuringExecution, fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...)
47384748
// }
47394749
if na.RequiredDuringSchedulingIgnoredDuringExecution != nil {
4740-
allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingIgnoredDuringExecution, true /* TODO: opts.AllowInvalidLabelValueInRequiredNodeAffinity */, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...)
4750+
allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingIgnoredDuringExecution, opts.AllowInvalidLabelValueInRequiredNodeAffinity, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...)
47414751
}
47424752
if len(na.PreferredDuringSchedulingIgnoredDuringExecution) > 0 {
47434753
allErrs = append(allErrs, ValidatePreferredSchedulingTerms(na.PreferredDuringSchedulingIgnoredDuringExecution, fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...)
@@ -7783,15 +7793,15 @@ func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field.
77837793
// returns:
77847794
// - true if volumeNodeAffinity is set
77857795
// - errorList if there are validation errors
7786-
func validateVolumeNodeAffinity(nodeAffinity *core.VolumeNodeAffinity, fldPath *field.Path) (bool, field.ErrorList) {
7796+
func validateVolumeNodeAffinity(nodeAffinity *core.VolumeNodeAffinity, opts PersistentVolumeSpecValidationOptions, fldPath *field.Path) (bool, field.ErrorList) {
77877797
allErrs := field.ErrorList{}
77887798

77897799
if nodeAffinity == nil {
77907800
return false, allErrs
77917801
}
77927802

77937803
if nodeAffinity.Required != nil {
7794-
allErrs = append(allErrs, ValidateNodeSelector(nodeAffinity.Required, true /* TODO: opts.AllowInvalidLabelValueInRequiredNodeAffinity */, fldPath.Child("required"))...)
7804+
allErrs = append(allErrs, ValidateNodeSelector(nodeAffinity.Required, opts.AllowInvalidLabelValueInRequiredNodeAffinity, fldPath.Child("required"))...)
77957805
} else {
77967806
allErrs = append(allErrs, field.Required(fldPath.Child("required"), "must specify required node constraints"))
77977807
}

0 commit comments

Comments
 (0)