Skip to content

Commit 3075a9a

Browse files
AxeZhanpohly
AxeZhan
authored and
pohly
committed
DRA API: validate node selector labels
Previously, ValidateNodeSelector did not check that labels are valid. Now it does for resource.k8s.io, regardless whether an object already was created with invalid labels in an earlier Kubernetes release. Theoretically this is a breaking change and could cause problems during an upgrade, but that is highly unlikely in practice. In contrast to node affinity, DRA does not ignore parse errors (= uses NewNodeSelector, not NewLazyErrorNodeSelector), so invalid labels would have been found instead of being silently ignored. Even if some object has invalid labels, this only affects an alpha -> beta upgrade which isn't guaranteed to work seamlessly.
1 parent cf480a3 commit 3075a9a

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)