fix TODO: moving driver name check in API validation

pkg/apis/core/validation/validation.go

@@ -63,6 +63,8 @@ const isInvalidQuotaResource string = `must be a standard resource for quota`
 const fieldImmutableErrorMsg string = apimachineryvalidation.FieldImmutableErrorMsg
 const isNotIntegerErrorMsg string = `must be an integer`
 const isNotPositiveErrorMsg string = `must be greater than zero`
+const csiDriverNameRexpErrMsg string = "must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character"
+const csiDriverNameRexpFmt string = `^[a-zA-Z0-9][-a-zA-Z0-9_.]{0,61}[a-zA-Z-0-9]$`
 
 var pdPartitionErrorMsg string = validation.InclusiveRangeError(1, 255)
 var fileModeErrorMsg string = "must be a number between 0 and 0777 (octal), both inclusive"
@@ -74,6 +76,8 @@ var iscsiInitiatorIqnRegex = regexp.MustCompile(`iqn\.\d{4}-\d{2}\.([[:alnum:]-.
 var iscsiInitiatorEuiRegex = regexp.MustCompile(`^eui.[[:alnum:]]{16}$`)
 var iscsiInitiatorNaaRegex = regexp.MustCompile(`^naa.[[:alnum:]]{32}$`)
 
+var csiDriverNameRexp = regexp.MustCompile(csiDriverNameRexpFmt)
+
 // ValidateHasLabel requires that metav1.ObjectMeta has a Label with key and expectedValue
 func ValidateHasLabel(meta metav1.ObjectMeta, fldPath *field.Path, key, expectedValue string) field.ErrorList {
 	allErrs := field.ErrorList{}
@@ -1433,6 +1437,14 @@ func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldP
 		allErrs = append(allErrs, field.Required(fldPath.Child("driver"), ""))
 	}
 
+	if len(csi.Driver) > 63 {
+		allErrs = append(allErrs, field.TooLong(fldPath.Child("driver"), csi.Driver, 63))
+	}
+
+	if !csiDriverNameRexp.MatchString(csi.Driver) {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("driver"), csi.Driver, validation.RegexError(csiDriverNameRexpErrMsg, csiDriverNameRexpFmt, "csi-hostpath")))
+	}
+
 	if len(csi.VolumeHandle) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath.Child("volumeHandle"), ""))
 	}

pkg/apis/core/validation/validation_test.go

@@ -1663,6 +1663,74 @@ func TestValidateCSIVolumeSource(t *testing.T) {
 			errtype:  field.ErrorTypeRequired,
 			errfield: "volumeHandle",
 		},
+		{
+			name: "driver name: ok no punctuations",
+			csi:  &core.CSIPersistentVolumeSource{Driver: "comgooglestoragecsigcepd", VolumeHandle: "test-123"},
+		},
+		{
+			name: "driver name: ok dot only",
+			csi:  &core.CSIPersistentVolumeSource{Driver: "io.kubernetes.storage.csi.flex", VolumeHandle: "test-123"},
+		},
+		{
+			name: "driver name: ok dash only",
+			csi:  &core.CSIPersistentVolumeSource{Driver: "io-kubernetes-storage-csi-flex", VolumeHandle: "test-123"},
+		},
+		{
+			name: "driver name: ok underscore only",
+			csi:  &core.CSIPersistentVolumeSource{Driver: "io_kubernetes_storage_csi_flex", VolumeHandle: "test-123"},
+		},
+		{
+			name: "driver name: ok dot underscores",
+			csi:  &core.CSIPersistentVolumeSource{Driver: "io.kubernetes.storage_csi.flex", VolumeHandle: "test-123"},
+		},
+		{
+			name: "driver name: ok beginnin with number",
+			csi:  &core.CSIPersistentVolumeSource{Driver: "2io.kubernetes.storage_csi.flex", VolumeHandle: "test-123"},
+		},
+		{
+			name: "driver name: ok ending with number",
+			csi:  &core.CSIPersistentVolumeSource{Driver: "io.kubernetes.storage_csi.flex2", VolumeHandle: "test-123"},
+		},
+		{
+			name: "driver name: ok dot dash underscores",
+			csi:  &core.CSIPersistentVolumeSource{Driver: "io.kubernetes-storage.csi_flex", VolumeHandle: "test-123"},
+		},
+		{
+			name:     "driver name: invalid length 0",
+			csi:      &core.CSIPersistentVolumeSource{Driver: "", VolumeHandle: "test-123"},
+			errtype:  field.ErrorTypeRequired,
+			errfield: "driver",
+		},
+		{
+			name:     "driver name: invalid length 1",
+			csi:      &core.CSIPersistentVolumeSource{Driver: "a", VolumeHandle: "test-123"},
+			errtype:  field.ErrorTypeInvalid,
+			errfield: "driver",
+		},
+		{
+			name:     "driver name: invalid length > 63",
+			csi:      &core.CSIPersistentVolumeSource{Driver: "comgooglestoragecsigcepdcomgooglestoragecsigcepdcomgooglestoragecsigcepdcomgooglestoragecsigcepd", VolumeHandle: "test-123"},
+			errtype:  field.ErrorTypeTooLong,
+			errfield: "driver",
+		},
+		{
+			name:     "driver name: invalid start char",
+			csi:      &core.CSIPersistentVolumeSource{Driver: "_comgooglestoragecsigcepd", VolumeHandle: "test-123"},
+			errtype:  field.ErrorTypeInvalid,
+			errfield: "driver",
+		},
+		{
+			name:     "driver name: invalid end char",
+			csi:      &core.CSIPersistentVolumeSource{Driver: "comgooglestoragecsigcepd/", VolumeHandle: "test-123"},
+			errtype:  field.ErrorTypeInvalid,
+			errfield: "driver",
+		},
+		{
+			name:     "driver name: invalid separators",
+			csi:      &core.CSIPersistentVolumeSource{Driver: "com/google/storage/csi~gcepd", VolumeHandle: "test-123"},
+			errtype:  field.ErrorTypeInvalid,
+			errfield: "driver",
+		},
 	}
 
 	err := utilfeature.DefaultFeatureGate.Set("CSIPersistentVolume=true")

pkg/volume/csi/csi_plugin.go

@@ -19,7 +19,6 @@ package csi
 import (
 	"errors"
 	"fmt"
-	"regexp"
 	"time"
 
 	csipb "github.com/container-storage-interface/spec/lib/go/csi"
@@ -47,8 +46,7 @@ const (
 
 var (
 	// csiVersion supported csi version
-	csiVersion     = &csipb.Version{Major: 0, Minor: 1, Patch: 0}
-	driverNameRexp = regexp.MustCompile(`^[A-Za-z]+(\.?-?_?[A-Za-z0-9-])+$`)
+	csiVersion = &csipb.Version{Major: 0, Minor: 1, Patch: 0}
 )
 
 type csiPlugin struct {
@@ -85,12 +83,6 @@ func (p *csiPlugin) GetVolumeName(spec *volume.Spec) (string, error) {
 		return "", err
 	}
 
-	//TODO (vladimirvivien) this validation should be done at the API validation check
-	if !isDriverNameValid(csi.Driver) {
-		glog.Error(log("plugin.GetVolumeName failed to create volume name: invalid csi driver name %s", csi.Driver))
-		return "", errors.New("invalid csi driver name")
-	}
-
 	// return driverName<separator>volumeHandle
 	return fmt.Sprintf("%s%s%s", csi.Driver, volNameSep, csi.VolumeHandle), nil
 }
@@ -114,13 +106,6 @@ func (p *csiPlugin) NewMounter(
 		return nil, err
 	}
 
-	// TODO (vladimirvivien) consider moving this check in API validation
-	// check Driver name to conform to CSI spec
-	if !isDriverNameValid(pvSource.Driver) {
-		glog.Error(log("driver name does not conform to CSI spec: %s", pvSource.Driver))
-		return nil, errors.New("driver name is invalid")
-	}
-
 	// before it is used in any paths such as socket etc
 	addr := fmt.Sprintf(csiAddrTemplate, pvSource.Driver)
 	glog.V(4).Infof(log("setting up mounter for [volume=%v,driver=%v]", pvSource.VolumeHandle, pvSource.Driver))
@@ -243,11 +228,3 @@ func getCSISourceFromSpec(spec *volume.Spec) (*api.CSIPersistentVolumeSource, er
 func log(msg string, parts ...interface{}) string {
 	return fmt.Sprintf(fmt.Sprintf("%s: %s", csiPluginName, msg), parts...)
 }
-
-// isDriverNameValid validates the driverName using CSI spec
-func isDriverNameValid(name string) bool {
-	if len(name) == 0 || len(name) > 63 {
-		return false
-	}
-	return driverNameRexp.MatchString(name)
-}

pkg/volume/csi/csi_plugin_test.go

@@ -243,36 +243,6 @@ func TestPluginNewUnmounter(t *testing.T) {
 
 }
 
-func TestValidateDriverName(t *testing.T) {
-	testCases := []struct {
-		name       string
-		driverName string
-		valid      bool
-	}{
-
-		{"ok no punctuations", "comgooglestoragecsigcepd", true},
-		{"ok dot only", "io.kubernetes.storage.csi.flex", true},
-		{"ok dash only", "io-kubernetes-storage-csi-flex", true},
-		{"ok underscore only", "io_kubernetes_storage_csi_flex", true},
-		{"ok dot underscores", "io.kubernetes.storage_csi.flex", true},
-		{"ok dot dash underscores", "io.kubernetes-storage.csi_flex", true},
-
-		{"invalid length 0", "", false},
-		{"invalid length > 63", "comgooglestoragecsigcepdcomgooglestoragecsigcepdcomgooglestoragecsigcepdcomgooglestoragecsigcepd", false},
-		{"invalid start char", "_comgooglestoragecsigcepd", false},
-		{"invalid end char", "comgooglestoragecsigcepd/", false},
-		{"invalid separators", "com/google/storage/csi~gcepd", false},
-	}
-
-	for _, tc := range testCases {
-		t.Logf("test case: %v", tc.name)
-		drValid := isDriverNameValid(tc.driverName)
-		if tc.valid != drValid {
-			t.Errorf("expecting driverName %s as valid=%t, but got valid=%t", tc.driverName, tc.valid, drValid)
-		}
-	}
-}
-
 func TestPluginNewAttacher(t *testing.T) {
 	plug, tmpDir := newTestPlugin(t)
 	defer os.RemoveAll(tmpDir)