Skip to content

Commit e4c1f98

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#128932 from pohly/dra-node-selector-validation
DRA API: validate node selector labels
2 parents 35d098a + 3075a9a commit e4c1f98

File tree

4 files changed

+113
-12
lines changed

4 files changed

+113
-12
lines changed

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

+18-10
Original file line numberDiff line numberDiff line change
@@ -4477,7 +4477,7 @@ func validateWindows(spec *core.PodSpec, fldPath *field.Path) field.ErrorList {
44774477
}
44784478

44794479
// ValidateNodeSelectorRequirement tests that the specified NodeSelectorRequirement fields has valid data
4480-
func ValidateNodeSelectorRequirement(rq core.NodeSelectorRequirement, fldPath *field.Path) field.ErrorList {
4480+
func ValidateNodeSelectorRequirement(rq core.NodeSelectorRequirement, allowInvalidLabelValueInRequiredNodeAffinity bool, fldPath *field.Path) field.ErrorList {
44814481
allErrs := field.ErrorList{}
44824482
switch rq.Operator {
44834483
case core.NodeSelectorOpIn, core.NodeSelectorOpNotIn:
@@ -4496,9 +4496,15 @@ func ValidateNodeSelectorRequirement(rq core.NodeSelectorRequirement, fldPath *f
44964496
default:
44974497
allErrs = append(allErrs, field.Invalid(fldPath.Child("operator"), rq.Operator, "not a valid selector operator"))
44984498
}
4499-
45004499
allErrs = append(allErrs, unversionedvalidation.ValidateLabelName(rq.Key, fldPath.Child("key"))...)
4501-
4500+
if !allowInvalidLabelValueInRequiredNodeAffinity {
4501+
path := fldPath.Child("values")
4502+
for valueIndex, value := range rq.Values {
4503+
for _, msg := range validation.IsValidLabelValue(value) {
4504+
allErrs = append(allErrs, field.Invalid(path.Index(valueIndex), value, msg))
4505+
}
4506+
}
4507+
}
45024508
return allErrs
45034509
}
45044510

@@ -4534,11 +4540,11 @@ func ValidateNodeFieldSelectorRequirement(req core.NodeSelectorRequirement, fldP
45344540
}
45354541

45364542
// ValidateNodeSelectorTerm tests that the specified node selector term has valid data
4537-
func ValidateNodeSelectorTerm(term core.NodeSelectorTerm, fldPath *field.Path) field.ErrorList {
4543+
func ValidateNodeSelectorTerm(term core.NodeSelectorTerm, allowInvalidLabelValueInRequiredNodeAffinity bool, fldPath *field.Path) field.ErrorList {
45384544
allErrs := field.ErrorList{}
45394545

45404546
for j, req := range term.MatchExpressions {
4541-
allErrs = append(allErrs, ValidateNodeSelectorRequirement(req, fldPath.Child("matchExpressions").Index(j))...)
4547+
allErrs = append(allErrs, ValidateNodeSelectorRequirement(req, allowInvalidLabelValueInRequiredNodeAffinity, fldPath.Child("matchExpressions").Index(j))...)
45424548
}
45434549

45444550
for j, req := range term.MatchFields {
@@ -4549,7 +4555,7 @@ func ValidateNodeSelectorTerm(term core.NodeSelectorTerm, fldPath *field.Path) f
45494555
}
45504556

45514557
// ValidateNodeSelector tests that the specified nodeSelector fields has valid data
4552-
func ValidateNodeSelector(nodeSelector *core.NodeSelector, fldPath *field.Path) field.ErrorList {
4558+
func ValidateNodeSelector(nodeSelector *core.NodeSelector, allowInvalidLabelValueInRequiredNodeAffinity bool, fldPath *field.Path) field.ErrorList {
45534559
allErrs := field.ErrorList{}
45544560

45554561
termFldPath := fldPath.Child("nodeSelectorTerms")
@@ -4558,7 +4564,7 @@ func ValidateNodeSelector(nodeSelector *core.NodeSelector, fldPath *field.Path)
45584564
}
45594565

45604566
for i, term := range nodeSelector.NodeSelectorTerms {
4561-
allErrs = append(allErrs, ValidateNodeSelectorTerm(term, termFldPath.Index(i))...)
4567+
allErrs = append(allErrs, ValidateNodeSelectorTerm(term, allowInvalidLabelValueInRequiredNodeAffinity, termFldPath.Index(i))...)
45624568
}
45634569

45644570
return allErrs
@@ -4659,7 +4665,9 @@ func ValidatePreferredSchedulingTerms(terms []core.PreferredSchedulingTerm, fldP
46594665
allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("weight"), term.Weight, "must be in the range 1-100"))
46604666
}
46614667

4662-
allErrs = append(allErrs, ValidateNodeSelectorTerm(term.Preference, fldPath.Index(i).Child("preference"))...)
4668+
// we always allow invalid label-value for preferred affinity
4669+
// as they can success when cluster has only one node
4670+
allErrs = append(allErrs, ValidateNodeSelectorTerm(term.Preference, true, fldPath.Index(i).Child("preference"))...)
46634671
}
46644672
return allErrs
46654673
}
@@ -4729,7 +4737,7 @@ func validateNodeAffinity(na *core.NodeAffinity, fldPath *field.Path) field.Erro
47294737
// allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingRequiredDuringExecution, fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...)
47304738
// }
47314739
if na.RequiredDuringSchedulingIgnoredDuringExecution != nil {
4732-
allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingIgnoredDuringExecution, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...)
4740+
allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingIgnoredDuringExecution, true /* TODO: opts.AllowInvalidLabelValueInRequiredNodeAffinity */, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...)
47334741
}
47344742
if len(na.PreferredDuringSchedulingIgnoredDuringExecution) > 0 {
47354743
allErrs = append(allErrs, ValidatePreferredSchedulingTerms(na.PreferredDuringSchedulingIgnoredDuringExecution, fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...)
@@ -7783,7 +7791,7 @@ func validateVolumeNodeAffinity(nodeAffinity *core.VolumeNodeAffinity, fldPath *
77837791
}
77847792

77857793
if nodeAffinity.Required != nil {
7786-
allErrs = append(allErrs, ValidateNodeSelector(nodeAffinity.Required, fldPath.Child("required"))...)
7794+
allErrs = append(allErrs, ValidateNodeSelector(nodeAffinity.Required, true /* TODO: opts.AllowInvalidLabelValueInRequiredNodeAffinity */, fldPath.Child("required"))...)
77877795
} else {
77887796
allErrs = append(allErrs, field.Required(fldPath.Child("required"), "must specify required node constraints"))
77897797
}

‎pkg/apis/resource/validation/validation.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,7 @@ func validateAllocationResult(allocation *resource.AllocationResult, fldPath *fi
332332
var allErrs field.ErrorList
333333
allErrs = append(allErrs, validateDeviceAllocationResult(allocation.Devices, fldPath.Child("devices"), requestNames, stored)...)
334334
if allocation.NodeSelector != nil {
335-
allErrs = append(allErrs, corevalidation.ValidateNodeSelector(allocation.NodeSelector, fldPath.Child("nodeSelector"))...)
335+
allErrs = append(allErrs, corevalidation.ValidateNodeSelector(allocation.NodeSelector, false, fldPath.Child("nodeSelector"))...)
336336
}
337337
return allErrs
338338
}
@@ -490,7 +490,7 @@ func validateResourceSliceSpec(spec, oldSpec *resource.ResourceSliceSpec, fldPat
490490
}
491491
if spec.NodeSelector != nil {
492492
numNodeSelectionFields++
493-
allErrs = append(allErrs, corevalidation.ValidateNodeSelector(spec.NodeSelector, fldPath.Child("nodeSelector"))...)
493+
allErrs = append(allErrs, corevalidation.ValidateNodeSelector(spec.NodeSelector, false, fldPath.Child("nodeSelector"))...)
494494
if len(spec.NodeSelector.NodeSelectorTerms) != 1 {
495495
// This additional constraint simplifies merging of different selectors
496496
// when devices are allocated from different slices.

‎pkg/apis/resource/validation/validation_resourceclaim_test.go

+47
Original file line numberDiff line numberDiff line change
@@ -1282,6 +1282,53 @@ func TestValidateClaimStatusUpdate(t *testing.T) {
12821282
},
12831283
deviceStatusFeatureGate: false,
12841284
},
1285+
"invalid-update-invalid-label-value": {
1286+
wantFailures: field.ErrorList{
1287+
field.Invalid(field.NewPath("status", "allocation", "nodeSelector", "nodeSelectorTerms").Index(0).Child("matchExpressions").Index(0).Child("values").Index(0), "-1", "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])?')"),
1288+
},
1289+
oldClaim: validClaim,
1290+
update: func(claim *resource.ResourceClaim) *resource.ResourceClaim {
1291+
claim = claim.DeepCopy()
1292+
claim.Status.Allocation = validAllocatedClaim.Status.Allocation.DeepCopy()
1293+
claim.Status.Allocation.NodeSelector = &core.NodeSelector{
1294+
NodeSelectorTerms: []core.NodeSelectorTerm{{
1295+
MatchExpressions: []core.NodeSelectorRequirement{{
1296+
Key: "foo",
1297+
Operator: core.NodeSelectorOpIn,
1298+
Values: []string{"-1"},
1299+
}},
1300+
}},
1301+
}
1302+
return claim
1303+
},
1304+
},
1305+
"valid-update-with-invalid-label-value": {
1306+
oldClaim: func() *resource.ResourceClaim {
1307+
claim := validAllocatedClaim.DeepCopy()
1308+
claim.Status.Allocation = validAllocatedClaim.Status.Allocation.DeepCopy()
1309+
claim.Status.Allocation.NodeSelector = &core.NodeSelector{
1310+
NodeSelectorTerms: []core.NodeSelectorTerm{{
1311+
MatchExpressions: []core.NodeSelectorRequirement{{
1312+
Key: "foo",
1313+
Operator: core.NodeSelectorOpIn,
1314+
Values: []string{"-1"},
1315+
}},
1316+
}},
1317+
}
1318+
return claim
1319+
}(),
1320+
update: func(claim *resource.ResourceClaim) *resource.ResourceClaim {
1321+
for i := 0; i < resource.ResourceClaimReservedForMaxSize; i++ {
1322+
claim.Status.ReservedFor = append(claim.Status.ReservedFor,
1323+
resource.ResourceClaimConsumerReference{
1324+
Resource: "pods",
1325+
Name: fmt.Sprintf("foo-%d", i),
1326+
UID: types.UID(fmt.Sprintf("%d", i)),
1327+
})
1328+
}
1329+
return claim
1330+
},
1331+
},
12851332
}
12861333

12871334
for name, scenario := range scenarios {

‎pkg/apis/resource/validation/validation_resourceslice_test.go

+46
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,23 @@ func TestValidateResourceSlice(t *testing.T) {
430430
return slice
431431
}(),
432432
},
433+
"invalid-node-selecor-label-value": {
434+
wantFailures: field.ErrorList{field.Invalid(field.NewPath("spec", "nodeSelector", "nodeSelectorTerms").Index(0).Child("matchExpressions").Index(0).Child("values").Index(0), "-1", "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])?')")},
435+
slice: func() *resourceapi.ResourceSlice {
436+
slice := testResourceSlice(goodName, goodName, goodName, 3)
437+
slice.Spec.NodeName = ""
438+
slice.Spec.NodeSelector = &core.NodeSelector{
439+
NodeSelectorTerms: []core.NodeSelectorTerm{{
440+
MatchExpressions: []core.NodeSelectorRequirement{{
441+
Key: "foo",
442+
Operator: core.NodeSelectorOpIn,
443+
Values: []string{"-1"},
444+
}},
445+
}},
446+
}
447+
return slice
448+
}(),
449+
},
433450
}
434451

435452
for name, scenario := range scenarios {
@@ -485,6 +502,35 @@ func TestValidateResourceSliceUpdate(t *testing.T) {
485502
return slice
486503
},
487504
},
505+
"invalid-update-to-invalid-nodeselector-label-value": {
506+
wantFailures: field.ErrorList{field.Invalid(field.NewPath("spec", "nodeSelector", "nodeSelectorTerms").Index(0).Child("matchExpressions").Index(0).Child("values").Index(0), "-1", "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])?')")},
507+
oldResourceSlice: func() *resourceapi.ResourceSlice {
508+
slice := validResourceSlice.DeepCopy()
509+
slice.Spec.NodeName = ""
510+
slice.Spec.NodeSelector = &core.NodeSelector{
511+
NodeSelectorTerms: []core.NodeSelectorTerm{{
512+
MatchExpressions: []core.NodeSelectorRequirement{{
513+
Key: "foo",
514+
Operator: core.NodeSelectorOpIn,
515+
Values: []string{"bar"},
516+
}},
517+
}},
518+
}
519+
return slice
520+
}(),
521+
update: func(slice *resourceapi.ResourceSlice) *resourceapi.ResourceSlice {
522+
slice.Spec.NodeSelector = &core.NodeSelector{
523+
NodeSelectorTerms: []core.NodeSelectorTerm{{
524+
MatchExpressions: []core.NodeSelectorRequirement{{
525+
Key: "foo",
526+
Operator: core.NodeSelectorOpIn,
527+
Values: []string{"-1"},
528+
}},
529+
}},
530+
}
531+
return slice
532+
},
533+
},
488534
}
489535

490536
for name, scenario := range scenarios {

0 commit comments

Comments
 (0)