Tighten up validation of VolumeAttachment fields

kubernetes · Nov 14, 2018 · 8cfce0a · 8cfce0a
1 parent 498cd61
commit 8cfce0a
Show file tree

Hide file tree

Showing 6 changed files with 211 additions and 21 deletions.
diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go
@@ -1443,25 +1443,32 @@ func validateStorageOSPersistentVolumeSource(storageos *core.StorageOSPersistent
 	return allErrs
 }
 
-func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldPath *field.Path) field.ErrorList {
+func ValidateCSIDriverName(driverName string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 
-	if !utilfeature.DefaultFeatureGate.Enabled(features.CSIPersistentVolume) {
-		allErrs = append(allErrs, field.Forbidden(fldPath, "CSIPersistentVolume disabled by feature-gate"))
+	if len(driverName) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath, ""))
 	}
 
-	if len(csi.Driver) == 0 {
-		allErrs = append(allErrs, field.Required(fldPath.Child("driver"), ""))
+	if len(driverName) > 63 {
+		allErrs = append(allErrs, field.TooLong(fldPath, driverName, 63))
 	}
 
-	if len(csi.Driver) > 63 {
-		allErrs = append(allErrs, field.TooLong(fldPath.Child("driver"), csi.Driver, 63))
+	if !csiDriverNameRexp.MatchString(driverName) {
+		allErrs = append(allErrs, field.Invalid(fldPath, driverName, validation.RegexError(csiDriverNameRexpErrMsg, csiDriverNameRexpFmt, "csi-hostpath")))
 	}
+	return allErrs
+}
 
-	if !csiDriverNameRexp.MatchString(csi.Driver) {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("driver"), csi.Driver, validation.RegexError(csiDriverNameRexpErrMsg, csiDriverNameRexpFmt, "csi-hostpath")))
+func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if !utilfeature.DefaultFeatureGate.Enabled(features.CSIPersistentVolume) {
+		allErrs = append(allErrs, field.Forbidden(fldPath, "CSIPersistentVolume disabled by feature-gate"))
 	}
 
+	allErrs = append(allErrs, ValidateCSIDriverName(csi.Driver, fldPath.Child("driver"))...)
+
 	if len(csi.VolumeHandle) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath.Child("volumeHandle"), ""))
 	}

diff --git a/pkg/apis/storage/validation/validation.go b/pkg/apis/storage/validation/validation.go
@@ -133,14 +133,28 @@ func validateAllowVolumeExpansion(allowExpand *bool, fldPath *field.Path) field.
 	return allErrs
 }
 
-// ValidateVolumeAttachment validates a VolumeAttachment.
+// ValidateVolumeAttachment validates a VolumeAttachment. This function is common for v1 and v1beta1 objects,
 func ValidateVolumeAttachment(volumeAttachment *storage.VolumeAttachment) field.ErrorList {
 	allErrs := apivalidation.ValidateObjectMeta(&volumeAttachment.ObjectMeta, false, apivalidation.ValidateClassName, field.NewPath("metadata"))
 	allErrs = append(allErrs, validateVolumeAttachmentSpec(&volumeAttachment.Spec, field.NewPath("spec"))...)
 	allErrs = append(allErrs, validateVolumeAttachmentStatus(&volumeAttachment.Status, field.NewPath("status"))...)
 	return allErrs
 }
 
+// ValidateVolumeAttachmentV1 validates a v1/VolumeAttachment. It contains only extra checks missing in
+// ValidateVolumeAttachment.
+func ValidateVolumeAttachmentV1(volumeAttachment *storage.VolumeAttachment) field.ErrorList {
+	allErrs := apivalidation.ValidateCSIDriverName(volumeAttachment.Spec.Attacher, field.NewPath("spec.attacher"))
+
+	if volumeAttachment.Spec.Source.PersistentVolumeName != nil {
+		pvName := *volumeAttachment.Spec.Source.PersistentVolumeName
+		for _, msg := range apivalidation.ValidatePersistentVolumeName(pvName, false) {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec.source.persistentVolumeName"), pvName, msg))
+		}
+	}
+	return allErrs
+}
+
 // ValidateVolumeAttachmentSpec tests that the specified VolumeAttachmentSpec
 // has valid data.
 func validateVolumeAttachmentSpec(

diff --git a/pkg/apis/storage/validation/validation_test.go b/pkg/apis/storage/validation/validation_test.go
@@ -224,14 +224,9 @@ func TestVolumeAttachmentValidation(t *testing.T) {
 			Spec: storage.VolumeAttachmentSpec{
 				Attacher: "",
 				NodeName: "mynode",
-			},
-		},
-		{
-			// Invalid attacher name
-			ObjectMeta: metav1.ObjectMeta{Name: "foo"},
-			Spec: storage.VolumeAttachmentSpec{
-				Attacher: "invalid!@#$%^&*()",
-				NodeName: "mynode",
+				Source: storage.VolumeAttachmentSource{
+					PersistentVolumeName: &volumeName,
+				},
 			},
 		},
 		{
@@ -240,6 +235,9 @@ func TestVolumeAttachmentValidation(t *testing.T) {
 			Spec: storage.VolumeAttachmentSpec{
 				Attacher: "myattacher",
 				NodeName: "",
+				Source: storage.VolumeAttachmentSource{
+					PersistentVolumeName: &volumeName,
+				},
 			},
 		},
 		{
@@ -378,7 +376,7 @@ func TestVolumeAttachmentUpdateValidation(t *testing.T) {
 
 	for _, volumeAttachment := range successCases {
 		if errs := ValidateVolumeAttachmentUpdate(&volumeAttachment, &old); len(errs) != 0 {
-			t.Errorf("expected success: %v", errs)
+			t.Errorf("expected success: %+v", errs)
 		}
 	}
 
@@ -445,7 +443,61 @@ func TestVolumeAttachmentUpdateValidation(t *testing.T) {
 
 	for _, volumeAttachment := range errorCases {
 		if errs := ValidateVolumeAttachmentUpdate(&volumeAttachment, &old); len(errs) == 0 {
-			t.Errorf("Expected failure for test: %v", volumeAttachment)
+			t.Errorf("Expected failure for test: %+v", volumeAttachment)
+		}
+	}
+}
+
+func TestVolumeAttachmentValidationV1(t *testing.T) {
+	volumeName := "pv-name"
+	invalidVolumeName := "-invalid-@#$%^&*()-"
+	successCases := []storage.VolumeAttachment{
+		{
+			ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+			Spec: storage.VolumeAttachmentSpec{
+				Attacher: "myattacher",
+				Source: storage.VolumeAttachmentSource{
+					PersistentVolumeName: &volumeName,
+				},
+				NodeName: "mynode",
+			},
+		},
+	}
+
+	for _, volumeAttachment := range successCases {
+		if errs := ValidateVolumeAttachmentV1(&volumeAttachment); len(errs) != 0 {
+			t.Errorf("expected success: %+v", errs)
+		}
+	}
+
+	errorCases := []storage.VolumeAttachment{
+		{
+			// Invalid attacher name
+			ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+			Spec: storage.VolumeAttachmentSpec{
+				Attacher: "invalid-@#$%^&*()",
+				NodeName: "mynode",
+				Source: storage.VolumeAttachmentSource{
+					PersistentVolumeName: &volumeName,
+				},
+			},
+		},
+		{
+			// Invalid PV name
+			ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+			Spec: storage.VolumeAttachmentSpec{
+				Attacher: "myattacher",
+				NodeName: "mynode",
+				Source: storage.VolumeAttachmentSource{
+					PersistentVolumeName: &invalidVolumeName,
+				},
+			},
+		},
+	}
+
+	for _, volumeAttachment := range errorCases {
+		if errs := ValidateVolumeAttachmentV1(&volumeAttachment); len(errs) == 0 {
+			t.Errorf("Expected failure for test: %+v", volumeAttachment)
 		}
 	}
 }

diff --git a/pkg/registry/storage/volumeattachment/BUILD b/pkg/registry/storage/volumeattachment/BUILD
@@ -30,6 +30,7 @@ go_test(
         "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
     ],
 )

diff --git a/pkg/registry/storage/volumeattachment/strategy.go b/pkg/registry/storage/volumeattachment/strategy.go
@@ -63,7 +63,23 @@ func (volumeAttachmentStrategy) PrepareForCreate(ctx context.Context, obj runtim
 
 func (volumeAttachmentStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
 	volumeAttachment := obj.(*storage.VolumeAttachment)
-	return validation.ValidateVolumeAttachment(volumeAttachment)
+
+	errs := validation.ValidateVolumeAttachment(volumeAttachment)
+
+	var groupVersion schema.GroupVersion
+
+	if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
+		groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+	}
+
+	switch groupVersion {
+	case storageapiv1beta1.SchemeGroupVersion:
+		// no extra validation
+	default:
+		// tighten up validation of newly created v1 attachments
+		errs = append(errs, validation.ValidateVolumeAttachmentV1(volumeAttachment)...)
+	}
+	return errs
 }
 
 // Canonicalize normalizes the object after validation.

diff --git a/pkg/registry/storage/volumeattachment/strategy_test.go b/pkg/registry/storage/volumeattachment/strategy_test.go
@@ -22,6 +22,7 @@ import (
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/kubernetes/pkg/apis/storage"
 )
@@ -195,3 +196,102 @@ func TestBetaAndV1StatusCreate(t *testing.T) {
 		}
 	}
 }
+
+func TestVolumeAttachmentValidation(t *testing.T) {
+	invalidPVName := "invalid-!@#$%^&*()"
+	validPVName := "valid-volume-name"
+	tests := []struct {
+		name             string
+		volumeAttachment *storage.VolumeAttachment
+		expectBetaError  bool
+		expectV1Error    bool
+	}{
+		{
+			"valid attachment",
+			getValidVolumeAttachment("foo"),
+			false,
+			false,
+		},
+		{
+			"invalid PV name",
+			&storage.VolumeAttachment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: storage.VolumeAttachmentSpec{
+					Attacher: "valid-attacher",
+					Source: storage.VolumeAttachmentSource{
+						PersistentVolumeName: &invalidPVName,
+					},
+					NodeName: "valid-node",
+				},
+			},
+			false,
+			true,
+		},
+		{
+			"invalid attacher name",
+			&storage.VolumeAttachment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: storage.VolumeAttachmentSpec{
+					Attacher: "invalid!@#$%^&*()",
+					Source: storage.VolumeAttachmentSource{
+						PersistentVolumeName: &validPVName,
+					},
+					NodeName: "valid-node",
+				},
+			},
+			false,
+			true,
+		},
+		{
+			"invalid volume attachment",
+			&storage.VolumeAttachment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: storage.VolumeAttachmentSpec{
+					Attacher: "invalid!@#$%^&*()",
+					Source: storage.VolumeAttachmentSource{
+						PersistentVolumeName: nil,
+					},
+					NodeName: "valid-node",
+				},
+			},
+			true,
+			true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+
+			testValidation := func(va *storage.VolumeAttachment, apiVersion string) field.ErrorList {
+				ctx := genericapirequest.WithRequestInfo(genericapirequest.NewContext(), &genericapirequest.RequestInfo{
+					APIGroup:   "storage.k8s.io",
+					APIVersion: apiVersion,
+					Resource:   "volumeattachments",
+				})
+				return Strategy.Validate(ctx, va)
+			}
+
+			v1Err := testValidation(test.volumeAttachment, "v1")
+			if len(v1Err) > 0 && !test.expectV1Error {
+				t.Errorf("Validation of v1 object failed: %+v", v1Err)
+			}
+			if len(v1Err) == 0 && test.expectV1Error {
+				t.Errorf("Validation of v1 object unexpectedly succeeded")
+			}
+
+			betaErr := testValidation(test.volumeAttachment, "v1beta1")
+			if len(betaErr) > 0 && !test.expectBetaError {
+				t.Errorf("Validation of v1beta1 object failed: %+v", betaErr)
+			}
+			if len(betaErr) == 0 && test.expectBetaError {
+				t.Errorf("Validation of v1beta1 object unexpectedly succeeded")
+			}
+		})
+	}
+}