Merge pull request #98753 from Jiawei0227/length

Relax csiNodeIDMaxLength to longer limit

pkg/apis/storage/validation/validation.go

@@ -45,8 +45,14 @@ const (
 	maxVolumeErrorMessageSize     = 1024
 
 	csiNodeIDMaxLength       = 128
+	csiNodeIDLongerMaxLength = 192
 )
 
+// CSINodeValidationOptions contains the validation options for validating CSINode
+type CSINodeValidationOptions struct {
+	AllowLongNodeID bool
+}
+
 // ValidateStorageClass validates a StorageClass.
 func ValidateStorageClass(storageClass *storage.StorageClass) field.ErrorList {
 	allErrs := apivalidation.ValidateObjectMeta(&storageClass.ObjectMeta, false, apivalidation.ValidateClassName, field.NewPath("metadata"))
@@ -291,15 +297,15 @@ func validateAllowedTopologies(topologies []api.TopologySelectorTerm, fldPath *f
 }
 
 // ValidateCSINode validates a CSINode.
-func ValidateCSINode(csiNode *storage.CSINode) field.ErrorList {
+func ValidateCSINode(csiNode *storage.CSINode, validationOpts CSINodeValidationOptions) field.ErrorList {
 	allErrs := apivalidation.ValidateObjectMeta(&csiNode.ObjectMeta, false, apivalidation.ValidateNodeName, field.NewPath("metadata"))
-	allErrs = append(allErrs, validateCSINodeSpec(&csiNode.Spec, field.NewPath("spec"))...)
+	allErrs = append(allErrs, validateCSINodeSpec(&csiNode.Spec, field.NewPath("spec"), validationOpts)...)
 	return allErrs
 }
 
 // ValidateCSINodeUpdate validates a CSINode.
-func ValidateCSINodeUpdate(new, old *storage.CSINode) field.ErrorList {
+func ValidateCSINodeUpdate(new, old *storage.CSINode, validationOpts CSINodeValidationOptions) field.ErrorList {
-	allErrs := ValidateCSINode(new)
+	allErrs := ValidateCSINode(new, validationOpts)
 
 	// Validate modifying fields inside an existing CSINodeDriver entry is not allowed
 	for _, oldDriver := range old.Spec.Drivers {
@@ -317,38 +323,47 @@ func ValidateCSINodeUpdate(new, old *storage.CSINode) field.ErrorList {
 
 // ValidateCSINodeSpec tests that the specified CSINodeSpec has valid data.
 func validateCSINodeSpec(
-	spec *storage.CSINodeSpec, fldPath *field.Path) field.ErrorList {
+	spec *storage.CSINodeSpec, fldPath *field.Path, validationOpts CSINodeValidationOptions) field.ErrorList {
 	allErrs := field.ErrorList{}
-	allErrs = append(allErrs, validateCSINodeDrivers(spec.Drivers, fldPath.Child("drivers"))...)
+	allErrs = append(allErrs, validateCSINodeDrivers(spec.Drivers, fldPath.Child("drivers"), validationOpts)...)
 	return allErrs
 }
 
 // ValidateCSINodeDrivers tests that the specified CSINodeDrivers have valid data.
-func validateCSINodeDrivers(drivers []storage.CSINodeDriver, fldPath *field.Path) field.ErrorList {
+func validateCSINodeDrivers(drivers []storage.CSINodeDriver, fldPath *field.Path, validationOpts CSINodeValidationOptions) field.ErrorList {
 	allErrs := field.ErrorList{}
 	driverNamesInSpecs := make(sets.String)
 	for i, driver := range drivers {
 		idxPath := fldPath.Index(i)
-		allErrs = append(allErrs, validateCSINodeDriver(driver, driverNamesInSpecs, idxPath)...)
+		allErrs = append(allErrs, validateCSINodeDriver(driver, driverNamesInSpecs, idxPath, validationOpts)...)
 	}
 
 	return allErrs
 }
 
 // validateCSINodeDriverNodeID tests if Name in CSINodeDriver is a valid node id.
-func validateCSINodeDriverNodeID(nodeID string, fldPath *field.Path) field.ErrorList {
+func validateCSINodeDriverNodeID(nodeID string, fldPath *field.Path, validationOpts CSINodeValidationOptions) field.ErrorList {
 	allErrs := field.ErrorList{}
 
 	// nodeID is always required
 	if len(nodeID) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath, nodeID))
 	}
-	if len(nodeID) > csiNodeIDMaxLength {
+	maxLength := csiNodeIDMaxLength
+	if validationOpts.AllowLongNodeID {
+		maxLength = csiNodeIDLongerMaxLength
+	}
+	if len(nodeID) > maxLength {
 		allErrs = append(allErrs, field.Invalid(fldPath, nodeID, fmt.Sprintf("must be %d characters or less", csiNodeIDMaxLength)))
 	}
 	return allErrs
 }
 
+// CSINodeLongerID will check if the nodeID is longer than csiNodeIDMaxLength
+func CSINodeLongerID(nodeID string) bool {
+	return len(nodeID) > csiNodeIDMaxLength
+}
+
 // validateCSINodeDriverAllocatable tests if Allocatable in CSINodeDriver has valid volume limits.
 func validateCSINodeDriverAllocatable(a *storage.VolumeNodeResources, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
@@ -362,11 +377,12 @@ func validateCSINodeDriverAllocatable(a *storage.VolumeNodeResources, fldPath *f
 }
 
 // validateCSINodeDriver tests if CSINodeDriver has valid entries
-func validateCSINodeDriver(driver storage.CSINodeDriver, driverNamesInSpecs sets.String, fldPath *field.Path) field.ErrorList {
+func validateCSINodeDriver(driver storage.CSINodeDriver, driverNamesInSpecs sets.String, fldPath *field.Path,
+	validationOpts CSINodeValidationOptions) field.ErrorList {
 	allErrs := field.ErrorList{}
 
 	allErrs = append(allErrs, apivalidation.ValidateCSIDriverName(driver.Name, fldPath.Child("name"))...)
-	allErrs = append(allErrs, validateCSINodeDriverNodeID(driver.NodeID, fldPath.Child("nodeID"))...)
+	allErrs = append(allErrs, validateCSINodeDriverNodeID(driver.NodeID, fldPath.Child("nodeID"), validationOpts)...)
 	allErrs = append(allErrs, validateCSINodeDriverAllocatable(driver.Allocatable, fldPath.Child("allocatable"))...)
 
 	// check for duplicate entries for the same driver in specs

pkg/apis/storage/validation/validation_test.go

@@ -46,6 +46,12 @@ var (
 			},
 		},
 	}
+	longerIDValidateOption = CSINodeValidationOptions{
+		AllowLongNodeID: true,
+	}
+	shorterIDValidationOption = CSINodeValidationOptions{
+		AllowLongNodeID: false,
+	}
 )
 
 func TestValidateStorageClass(t *testing.T) {
@@ -1026,8 +1032,9 @@ func TestValidateAllowedTopologies(t *testing.T) {
 func TestCSINodeValidation(t *testing.T) {
 	driverName := "driver-name"
 	driverName2 := "1io.kubernetes-storage-2-csi-driver3"
-	longName := "my-a-b-c-d-c-f-g-h-i-j-k-l-m-n-o-p-q-r-s-t-u-v-w-x-y-z-ABCDEFGHIJKLMNOPQRSTUVWXYZ-driver"
+	longName := "my-a-b-c-d-c-f-g-h-i-j-k-l-m-n-o-p-q-r-s-t-u-v-w-x-y-z-ABCDEFGHIJKLMNOPQRSTUVWXYZ-driver" // 88 chars
 	nodeID := "nodeA"
+	longID := longName + longName // 176 chars
 	successCases := []storage.CSINode{
 		{
 			// driver name: dot only
@@ -1217,10 +1224,29 @@ func TestCSINodeValidation(t *testing.T) {
 	}
 
 	for _, csiNode := range successCases {
-		if errs := ValidateCSINode(&csiNode); len(errs) != 0 {
+		if errs := ValidateCSINode(&csiNode, shorterIDValidationOption); len(errs) != 0 {
 			t.Errorf("expected success: %v", errs)
 		}
 	}
+
+	nodeIDCase := storage.CSINode{
+		// node ID length > 128 but < 192
+		ObjectMeta: metav1.ObjectMeta{Name: "foo7"},
+		Spec: storage.CSINodeSpec{
+			Drivers: []storage.CSINodeDriver{
+				{
+					Name:         driverName,
+					NodeID:       longID,
+					TopologyKeys: []string{"company.com/zone1", "company.com/zone2"},
+				},
+			},
+		},
+	}
+
+	if errs := ValidateCSINode(&nodeIDCase, longerIDValidateOption); len(errs) != 0 {
+		t.Errorf("expected success: %v", errs)
+	}
+
 	errorCases := []storage.CSINode{
 		{
 			// Empty driver name
@@ -1414,10 +1440,11 @@ func TestCSINodeValidation(t *testing.T) {
 				},
 			},
 		},
+		nodeIDCase,
 	}
 
 	for _, csiNode := range errorCases {
-		if errs := ValidateCSINode(&csiNode); len(errs) == 0 {
+		if errs := ValidateCSINode(&csiNode, shorterIDValidationOption); len(errs) == 0 {
 			t.Errorf("Expected failure for test: %v", csiNode)
 		}
 	}
@@ -1528,7 +1555,7 @@ func TestCSINodeUpdateValidation(t *testing.T) {
 	}
 
 	for _, csiNode := range successCases {
-		if errs := ValidateCSINodeUpdate(&csiNode, &old); len(errs) != 0 {
+		if errs := ValidateCSINodeUpdate(&csiNode, &old, shorterIDValidationOption); len(errs) != 0 {
 			t.Errorf("expected success: %+v", errs)
 		}
 	}
@@ -1651,7 +1678,7 @@ func TestCSINodeUpdateValidation(t *testing.T) {
 	}
 
 	for _, csiNode := range errorCases {
-		if errs := ValidateCSINodeUpdate(&csiNode, &old); len(errs) == 0 {
+		if errs := ValidateCSINodeUpdate(&csiNode, &old, shorterIDValidationOption); len(errs) == 0 {
 			t.Errorf("Expected failure for test: %+v", csiNode)
 		}
 	}

pkg/controller/volume/persistentvolume/pv_controller_base.go

@@ -366,7 +366,7 @@ func (ctrl *PersistentVolumeController) updateVolumeMigrationAnnotations(volume
 
 // updateMigrationAnnotations takes an Annotations map and checks for a
 // provisioner name using the provisionerKey. It will then add a
-// "volume.beta.kubernetes.io/migrated-to" annotation if migration with the CSI
+// "pv.kubernetes.io/migrated-to" annotation if migration with the CSI
 // driver name for that provisioner is "on" based on feature flags, it will also
 // remove the annotation is migration is "off" for that provisioner in rollback
 // scenarios. Returns true if the annotations map was modified and false otherwise.

pkg/registry/storage/csinode/strategy.go

@@ -48,8 +48,13 @@ func (csiNodeStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object)
 func (csiNodeStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
 	csiNode := obj.(*storage.CSINode)
 
-	errs := validation.ValidateCSINode(csiNode)
+	// in 1.21, on create, set AllowLongNodeID=false
-	errs = append(errs, validation.ValidateCSINode(csiNode)...)
+	// in 1.22, on create, set AllowLongNodeID=true
+	validateOptions := validation.CSINodeValidationOptions{
+		AllowLongNodeID: false,
+	}
+
+	errs := validation.ValidateCSINode(csiNode, validateOptions)
 
 	return errs
 }
@@ -69,8 +74,20 @@ func (csiNodeStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Ob
 func (csiNodeStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
 	newCSINodeObj := obj.(*storage.CSINode)
 	oldCSINodeObj := old.(*storage.CSINode)
-	errorList := validation.ValidateCSINode(newCSINodeObj)
+	// in 1.21 on update, set AllowLongNodeID to true only if the old object already has a long node ID
-	return append(errorList, validation.ValidateCSINodeUpdate(newCSINodeObj, oldCSINodeObj)...)
+	// in 1.22 on update, set AllowLongNodeID=true
+	allowLongNodeID := false
+	for _, nodeDriver := range oldCSINodeObj.Spec.Drivers {
+		if validation.CSINodeLongerID(nodeDriver.NodeID) {
+			allowLongNodeID = true
+		}
+	}
+	validateOptions := validation.CSINodeValidationOptions{
+		AllowLongNodeID: allowLongNodeID,
+	}
+
+	errorList := validation.ValidateCSINode(newCSINodeObj, validateOptions)
+	return append(errorList, validation.ValidateCSINodeUpdate(newCSINodeObj, oldCSINodeObj, validateOptions)...)
 }
 
 func (csiNodeStrategy) AllowUnconditionalUpdate() bool {