From 3075a9ae968bb995bc170b966a48e052d18c75c0 Mon Sep 17 00:00:00 2001 From: AxeZhan Date: Wed, 23 Oct 2024 17:28:27 +0800 Subject: [PATCH] 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. --- pkg/apis/core/validation/validation.go | 28 +++++++---- pkg/apis/resource/validation/validation.go | 4 +- .../validation_resourceclaim_test.go | 47 +++++++++++++++++++ .../validation_resourceslice_test.go | 46 ++++++++++++++++++ 4 files changed, 113 insertions(+), 12 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index dfebbf43dafc6..694896ee750c6 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -4477,7 +4477,7 @@ func validateWindows(spec *core.PodSpec, fldPath *field.Path) field.ErrorList { } // ValidateNodeSelectorRequirement tests that the specified NodeSelectorRequirement fields has valid data -func ValidateNodeSelectorRequirement(rq core.NodeSelectorRequirement, fldPath *field.Path) field.ErrorList { +func ValidateNodeSelectorRequirement(rq core.NodeSelectorRequirement, allowInvalidLabelValueInRequiredNodeAffinity bool, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} switch rq.Operator { case core.NodeSelectorOpIn, core.NodeSelectorOpNotIn: @@ -4496,9 +4496,15 @@ func ValidateNodeSelectorRequirement(rq core.NodeSelectorRequirement, fldPath *f default: allErrs = append(allErrs, field.Invalid(fldPath.Child("operator"), rq.Operator, "not a valid selector operator")) } - allErrs = append(allErrs, unversionedvalidation.ValidateLabelName(rq.Key, fldPath.Child("key"))...) - + if !allowInvalidLabelValueInRequiredNodeAffinity { + path := fldPath.Child("values") + for valueIndex, value := range rq.Values { + for _, msg := range validation.IsValidLabelValue(value) { + allErrs = append(allErrs, field.Invalid(path.Index(valueIndex), value, msg)) + } + } + } return allErrs } @@ -4534,11 +4540,11 @@ func ValidateNodeFieldSelectorRequirement(req core.NodeSelectorRequirement, fldP } // ValidateNodeSelectorTerm tests that the specified node selector term has valid data -func ValidateNodeSelectorTerm(term core.NodeSelectorTerm, fldPath *field.Path) field.ErrorList { +func ValidateNodeSelectorTerm(term core.NodeSelectorTerm, allowInvalidLabelValueInRequiredNodeAffinity bool, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for j, req := range term.MatchExpressions { - allErrs = append(allErrs, ValidateNodeSelectorRequirement(req, fldPath.Child("matchExpressions").Index(j))...) + allErrs = append(allErrs, ValidateNodeSelectorRequirement(req, allowInvalidLabelValueInRequiredNodeAffinity, fldPath.Child("matchExpressions").Index(j))...) } for j, req := range term.MatchFields { @@ -4549,7 +4555,7 @@ func ValidateNodeSelectorTerm(term core.NodeSelectorTerm, fldPath *field.Path) f } // ValidateNodeSelector tests that the specified nodeSelector fields has valid data -func ValidateNodeSelector(nodeSelector *core.NodeSelector, fldPath *field.Path) field.ErrorList { +func ValidateNodeSelector(nodeSelector *core.NodeSelector, allowInvalidLabelValueInRequiredNodeAffinity bool, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} termFldPath := fldPath.Child("nodeSelectorTerms") @@ -4558,7 +4564,7 @@ func ValidateNodeSelector(nodeSelector *core.NodeSelector, fldPath *field.Path) } for i, term := range nodeSelector.NodeSelectorTerms { - allErrs = append(allErrs, ValidateNodeSelectorTerm(term, termFldPath.Index(i))...) + allErrs = append(allErrs, ValidateNodeSelectorTerm(term, allowInvalidLabelValueInRequiredNodeAffinity, termFldPath.Index(i))...) } return allErrs @@ -4659,7 +4665,9 @@ func ValidatePreferredSchedulingTerms(terms []core.PreferredSchedulingTerm, fldP allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("weight"), term.Weight, "must be in the range 1-100")) } - allErrs = append(allErrs, ValidateNodeSelectorTerm(term.Preference, fldPath.Index(i).Child("preference"))...) + // we always allow invalid label-value for preferred affinity + // as they can success when cluster has only one node + allErrs = append(allErrs, ValidateNodeSelectorTerm(term.Preference, true, fldPath.Index(i).Child("preference"))...) } return allErrs } @@ -4729,7 +4737,7 @@ func validateNodeAffinity(na *core.NodeAffinity, fldPath *field.Path) field.Erro // allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingRequiredDuringExecution, fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...) // } if na.RequiredDuringSchedulingIgnoredDuringExecution != nil { - allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingIgnoredDuringExecution, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) + allErrs = append(allErrs, ValidateNodeSelector(na.RequiredDuringSchedulingIgnoredDuringExecution, true /* TODO: opts.AllowInvalidLabelValueInRequiredNodeAffinity */, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) } if len(na.PreferredDuringSchedulingIgnoredDuringExecution) > 0 { allErrs = append(allErrs, ValidatePreferredSchedulingTerms(na.PreferredDuringSchedulingIgnoredDuringExecution, fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...) @@ -7783,7 +7791,7 @@ func validateVolumeNodeAffinity(nodeAffinity *core.VolumeNodeAffinity, fldPath * } if nodeAffinity.Required != nil { - allErrs = append(allErrs, ValidateNodeSelector(nodeAffinity.Required, fldPath.Child("required"))...) + allErrs = append(allErrs, ValidateNodeSelector(nodeAffinity.Required, true /* TODO: opts.AllowInvalidLabelValueInRequiredNodeAffinity */, fldPath.Child("required"))...) } else { allErrs = append(allErrs, field.Required(fldPath.Child("required"), "must specify required node constraints")) } diff --git a/pkg/apis/resource/validation/validation.go b/pkg/apis/resource/validation/validation.go index 4f06c114d85fd..2f58721ddc106 100644 --- a/pkg/apis/resource/validation/validation.go +++ b/pkg/apis/resource/validation/validation.go @@ -332,7 +332,7 @@ func validateAllocationResult(allocation *resource.AllocationResult, fldPath *fi var allErrs field.ErrorList allErrs = append(allErrs, validateDeviceAllocationResult(allocation.Devices, fldPath.Child("devices"), requestNames, stored)...) if allocation.NodeSelector != nil { - allErrs = append(allErrs, corevalidation.ValidateNodeSelector(allocation.NodeSelector, fldPath.Child("nodeSelector"))...) + allErrs = append(allErrs, corevalidation.ValidateNodeSelector(allocation.NodeSelector, false, fldPath.Child("nodeSelector"))...) } return allErrs } @@ -490,7 +490,7 @@ func validateResourceSliceSpec(spec, oldSpec *resource.ResourceSliceSpec, fldPat } if spec.NodeSelector != nil { numNodeSelectionFields++ - allErrs = append(allErrs, corevalidation.ValidateNodeSelector(spec.NodeSelector, fldPath.Child("nodeSelector"))...) + allErrs = append(allErrs, corevalidation.ValidateNodeSelector(spec.NodeSelector, false, fldPath.Child("nodeSelector"))...) if len(spec.NodeSelector.NodeSelectorTerms) != 1 { // This additional constraint simplifies merging of different selectors // when devices are allocated from different slices. diff --git a/pkg/apis/resource/validation/validation_resourceclaim_test.go b/pkg/apis/resource/validation/validation_resourceclaim_test.go index ac202caf6b208..c48a5bd3079c6 100644 --- a/pkg/apis/resource/validation/validation_resourceclaim_test.go +++ b/pkg/apis/resource/validation/validation_resourceclaim_test.go @@ -1282,6 +1282,53 @@ func TestValidateClaimStatusUpdate(t *testing.T) { }, deviceStatusFeatureGate: false, }, + "invalid-update-invalid-label-value": { + wantFailures: field.ErrorList{ + 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])?')"), + }, + oldClaim: validClaim, + update: func(claim *resource.ResourceClaim) *resource.ResourceClaim { + claim = claim.DeepCopy() + claim.Status.Allocation = validAllocatedClaim.Status.Allocation.DeepCopy() + claim.Status.Allocation.NodeSelector = &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "foo", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + } + return claim + }, + }, + "valid-update-with-invalid-label-value": { + oldClaim: func() *resource.ResourceClaim { + claim := validAllocatedClaim.DeepCopy() + claim.Status.Allocation = validAllocatedClaim.Status.Allocation.DeepCopy() + claim.Status.Allocation.NodeSelector = &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "foo", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + } + return claim + }(), + update: func(claim *resource.ResourceClaim) *resource.ResourceClaim { + for i := 0; i < resource.ResourceClaimReservedForMaxSize; i++ { + claim.Status.ReservedFor = append(claim.Status.ReservedFor, + resource.ResourceClaimConsumerReference{ + Resource: "pods", + Name: fmt.Sprintf("foo-%d", i), + UID: types.UID(fmt.Sprintf("%d", i)), + }) + } + return claim + }, + }, } for name, scenario := range scenarios { diff --git a/pkg/apis/resource/validation/validation_resourceslice_test.go b/pkg/apis/resource/validation/validation_resourceslice_test.go index 7901a655acb4b..c4d57a2b09a12 100644 --- a/pkg/apis/resource/validation/validation_resourceslice_test.go +++ b/pkg/apis/resource/validation/validation_resourceslice_test.go @@ -430,6 +430,23 @@ func TestValidateResourceSlice(t *testing.T) { return slice }(), }, + "invalid-node-selecor-label-value": { + 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])?')")}, + slice: func() *resourceapi.ResourceSlice { + slice := testResourceSlice(goodName, goodName, goodName, 3) + slice.Spec.NodeName = "" + slice.Spec.NodeSelector = &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "foo", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + } + return slice + }(), + }, } for name, scenario := range scenarios { @@ -485,6 +502,35 @@ func TestValidateResourceSliceUpdate(t *testing.T) { return slice }, }, + "invalid-update-to-invalid-nodeselector-label-value": { + 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])?')")}, + oldResourceSlice: func() *resourceapi.ResourceSlice { + slice := validResourceSlice.DeepCopy() + slice.Spec.NodeName = "" + slice.Spec.NodeSelector = &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "foo", + Operator: core.NodeSelectorOpIn, + Values: []string{"bar"}, + }}, + }}, + } + return slice + }(), + update: func(slice *resourceapi.ResourceSlice) *resourceapi.ResourceSlice { + slice.Spec.NodeSelector = &core.NodeSelector{ + NodeSelectorTerms: []core.NodeSelectorTerm{{ + MatchExpressions: []core.NodeSelectorRequirement{{ + Key: "foo", + Operator: core.NodeSelectorOpIn, + Values: []string{"-1"}, + }}, + }}, + } + return slice + }, + }, } for name, scenario := range scenarios {