diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index a6089c9b30..007a4e8e53 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -1443,6 +1443,23 @@ func validateStorageOSPersistentVolumeSource(storageos *core.StorageOSPersistent return allErrs } +func ValidateCSIDriverName(driverName string, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + if len(driverName) == 0 { + allErrs = append(allErrs, field.Required(fldPath, "")) + } + + if len(driverName) > 63 { + allErrs = append(allErrs, field.TooLong(fldPath, driverName, 63)) + } + + if !csiDriverNameRexp.MatchString(driverName) { + allErrs = append(allErrs, field.Invalid(fldPath, driverName, validation.RegexError(csiDriverNameRexpErrMsg, csiDriverNameRexpFmt, "csi-hostpath"))) + } + return allErrs +} + func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} @@ -1450,17 +1467,7 @@ func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldP allErrs = append(allErrs, field.Forbidden(fldPath, "CSIPersistentVolume disabled by feature-gate")) } - if len(csi.Driver) == 0 { - 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"))) - } + 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 index d7278b6908..48d9ecde47 100644 --- a/pkg/apis/storage/validation/validation.go +++ b/pkg/apis/storage/validation/validation.go @@ -133,7 +133,7 @@ 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"))...) @@ -141,6 +141,20 @@ func ValidateVolumeAttachment(volumeAttachment *storage.VolumeAttachment) field. 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 index ce3b09f1e8..c79abf465e 100644 --- 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 index 252a76c6f8..7cb2a4bb38 100644 --- 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 index bdf912be3c..6d37f695c4 100644 --- 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 index 4be7184232..e2ec6b4b70 100644 --- 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") + } + }) + } +}