From efb68ddf73b3bc328d7952bba03772c9eb6e10df Mon Sep 17 00:00:00 2001 From: linyouchong Date: Thu, 1 Feb 2018 15:55:02 +0800 Subject: [PATCH] fix TODO: moving driver name check in API validation --- pkg/apis/core/validation/validation.go | 12 ++++ pkg/apis/core/validation/validation_test.go | 68 +++++++++++++++++++++ pkg/volume/csi/csi_plugin.go | 25 +------- pkg/volume/csi/csi_plugin_test.go | 30 --------- 4 files changed, 81 insertions(+), 54 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 655d0bf2163..75ecd6862e6 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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"), "")) } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 25dd774ab86..dd237b561b1 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/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") diff --git a/pkg/volume/csi/csi_plugin.go b/pkg/volume/csi/csi_plugin.go index be40992df04..f600d11ab03 100644 --- a/pkg/volume/csi/csi_plugin.go +++ b/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 driverNamevolumeHandle 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) -} diff --git a/pkg/volume/csi/csi_plugin_test.go b/pkg/volume/csi/csi_plugin_test.go index 26d5f8c14de..60c5bd7661a 100644 --- a/pkg/volume/csi/csi_plugin_test.go +++ b/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)