From 8018ab7cd91b05682309621ff12cf4c617943a23 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Sat, 5 Nov 2022 18:04:04 +0100 Subject: [PATCH] api: fully validate PotentialNodes and SuitableNodes This is in response to review feedback. Checking for valid node names and the set property catches programming mistakes in the components that have write permission. --- pkg/apis/resource/validation/validation.go | 29 +++++++---------- .../validation_podscheduling_test.go | 32 +++++++++++++++++-- 2 files changed, 40 insertions(+), 21 deletions(-) diff --git a/pkg/apis/resource/validation/validation.go b/pkg/apis/resource/validation/validation.go index 9191bf5a348..fcbf8c55c18 100644 --- a/pkg/apis/resource/validation/validation.go +++ b/pkg/apis/resource/validation/validation.go @@ -239,15 +239,7 @@ func ValidatePodScheduling(resourceClaim *resource.PodScheduling) field.ErrorLis } func validatePodSchedulingSpec(spec *resource.PodSchedulingSpec, fldPath *field.Path) field.ErrorList { - var allErrs field.ErrorList - // Checking PotentialNodes for duplicates is intentionally not done. It - // could be fairly expensive and the only component which normally has - // permissions to set this field, kube-scheduler, is a trusted - // component. Also, if it gets this wrong because of a bug, then the - // effect is limited (same semantic). - if len(spec.PotentialNodes) > resource.PodSchedulingNodeListMaxSize { - allErrs = append(allErrs, field.TooLongMaxLength(fldPath.Child("potentialNodes"), nil, resource.PodSchedulingNodeListMaxSize)) - } + allErrs := validateSliceIsASet(spec.PotentialNodes, resource.PodSchedulingNodeListMaxSize, validateNodeName, fldPath.Child("potentialNodes")) return allErrs } @@ -283,15 +275,8 @@ func validatePodSchedulingClaims(claimStatuses []resource.ResourceClaimSchedulin return allErrs } -func validatePodSchedulingClaim(claim resource.ResourceClaimSchedulingStatus, fldPath *field.Path) field.ErrorList { - var allErrs field.ErrorList - // Checking UnsuitableNodes for duplicates is intentionally not done. It - // could be fairly expensive and if a resource driver gets this wrong, - // then it is only going to have a negative effect for the pods relying - // on this driver. - if len(claim.UnsuitableNodes) > resource.PodSchedulingNodeListMaxSize { - allErrs = append(allErrs, field.TooLongMaxLength(fldPath.Child("unsuitableNodes"), nil, resource.PodSchedulingNodeListMaxSize)) - } +func validatePodSchedulingClaim(status resource.ResourceClaimSchedulingStatus, fldPath *field.Path) field.ErrorList { + allErrs := validateSliceIsASet(status.UnsuitableNodes, resource.PodSchedulingNodeListMaxSize, validateNodeName, fldPath.Child("unsuitableNodes")) return allErrs } @@ -315,3 +300,11 @@ func ValidateClaimTemplateUpdate(template, oldTemplate *resource.ResourceClaimTe allErrs = append(allErrs, ValidateClaimTemplate(template)...) return allErrs } + +func validateNodeName(name string, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + for _, msg := range corevalidation.ValidateNodeName(name, false) { + allErrs = append(allErrs, field.Invalid(fldPath, name, msg)) + } + return allErrs +} diff --git a/pkg/apis/resource/validation/validation_podscheduling_test.go b/pkg/apis/resource/validation/validation_podscheduling_test.go index 138f70004bf..30193eb5a3a 100644 --- a/pkg/apis/resource/validation/validation_podscheduling_test.go +++ b/pkg/apis/resource/validation/validation_podscheduling_test.go @@ -194,6 +194,7 @@ func TestValidatePodScheduling(t *testing.T) { func TestValidatePodSchedulingUpdate(t *testing.T) { validScheduling := testPodScheduling("foo", "ns", resource.PodSchedulingSpec{}) + badName := "!@#$%^" scenarios := map[string]struct { oldScheduling *resource.PodScheduling @@ -220,8 +221,8 @@ func TestValidatePodSchedulingUpdate(t *testing.T) { return scheduling }, }, - "invalid-potential-nodes": { - wantFailures: field.ErrorList{field.TooLongMaxLength(field.NewPath("spec", "potentialNodes"), nil, resource.PodSchedulingNodeListMaxSize)}, + "invalid-potential-nodes-too-long": { + wantFailures: field.ErrorList{field.TooLongMaxLength(field.NewPath("spec", "potentialNodes"), 129, resource.PodSchedulingNodeListMaxSize)}, oldScheduling: validScheduling, update: func(scheduling *resource.PodScheduling) *resource.PodScheduling { for i := 0; i < resource.PodSchedulingNodeListMaxSize+1; i++ { @@ -230,6 +231,14 @@ func TestValidatePodSchedulingUpdate(t *testing.T) { return scheduling }, }, + "invalid-potential-nodes-name": { + wantFailures: field.ErrorList{field.Invalid(field.NewPath("spec", "potentialNodes").Index(0), badName, "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')")}, + oldScheduling: validScheduling, + update: func(scheduling *resource.PodScheduling) *resource.PodScheduling { + scheduling.Spec.PotentialNodes = append(scheduling.Spec.PotentialNodes, badName) + return scheduling + }, + }, } for name, scenario := range scenarios { @@ -243,6 +252,7 @@ func TestValidatePodSchedulingUpdate(t *testing.T) { func TestValidatePodSchedulingStatusUpdate(t *testing.T) { validScheduling := testPodScheduling("foo", "ns", resource.PodSchedulingSpec{}) + badName := "!@#$%^" scenarios := map[string]struct { oldScheduling *resource.PodScheduling @@ -283,7 +293,7 @@ func TestValidatePodSchedulingStatusUpdate(t *testing.T) { }, }, "invalid-too-long-claim-status": { - wantFailures: field.ErrorList{field.TooLongMaxLength(field.NewPath("status", "claims").Index(0).Child("unsuitableNodes"), nil, resource.PodSchedulingNodeListMaxSize)}, + wantFailures: field.ErrorList{field.TooLongMaxLength(field.NewPath("status", "claims").Index(0).Child("unsuitableNodes"), 129, resource.PodSchedulingNodeListMaxSize)}, oldScheduling: validScheduling, update: func(scheduling *resource.PodScheduling) *resource.PodScheduling { scheduling.Status.ResourceClaims = append(scheduling.Status.ResourceClaims, @@ -300,6 +310,22 @@ func TestValidatePodSchedulingStatusUpdate(t *testing.T) { return scheduling }, }, + "invalid-node-name": { + wantFailures: field.ErrorList{field.Invalid(field.NewPath("status", "claims").Index(0).Child("unsuitableNodes").Index(0), badName, "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')")}, + oldScheduling: validScheduling, + update: func(scheduling *resource.PodScheduling) *resource.PodScheduling { + scheduling.Status.ResourceClaims = append(scheduling.Status.ResourceClaims, + resource.ResourceClaimSchedulingStatus{ + Name: "my-claim", + }, + ) + scheduling.Status.ResourceClaims[0].UnsuitableNodes = append( + scheduling.Status.ResourceClaims[0].UnsuitableNodes, + badName, + ) + return scheduling + }, + }, } for name, scenario := range scenarios {