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

@@ -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
+	allErrs := validateSliceIsASet(spec.PotentialNodes, resource.PodSchedulingNodeListMaxSize, validateNodeName, fldPath.Child("potentialNodes"))
-	// 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))
-	}
 	return allErrs
 }
 
@@ -283,15 +275,8 @@ func validatePodSchedulingClaims(claimStatuses []resource.ResourceClaimSchedulin
 	return allErrs
 }
 
-func validatePodSchedulingClaim(claim resource.ResourceClaimSchedulingStatus, fldPath *field.Path) field.ErrorList {
+func validatePodSchedulingClaim(status resource.ResourceClaimSchedulingStatus, fldPath *field.Path) field.ErrorList {
-	var allErrs field.ErrorList
+	allErrs := validateSliceIsASet(status.UnsuitableNodes, resource.PodSchedulingNodeListMaxSize, validateNodeName, fldPath.Child("unsuitableNodes"))
-	// 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))
-	}
 	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
+}

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": {
+		"invalid-potential-nodes-too-long": {
-			wantFailures:  field.ErrorList{field.TooLongMaxLength(field.NewPath("spec", "potentialNodes"), nil, resource.PodSchedulingNodeListMaxSize)},
+			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 {