Ensure ModifyVolume returns InvalidArgument error code if VAC contain…

…s invalid parameter (#2103) * Fixing invalid VAC parameter on modification bug * Fixing invalid VAC parameter on modification bug * fix: sidecars.snapshotter.logLevel not being respect * Fixed e2e tests * Fixed e2e tests * fixed unit tests * fixed unit tests * moved helper logic to utils * Moved function to prefix into e2e_utils --------- Co-authored-by: zyue110026 <[email protected]>
kubernetes-sigs · Aug 6, 2024 · 40f2d17 · 40f2d17
1 parent aeb6a02
commit 40f2d17
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 36 deletions.
diff --git a/pkg/driver/controller_modify_volume.go b/pkg/driver/controller_modify_volume.go
@@ -200,6 +200,8 @@ func parseModifyVolumeParameters(params map[string]string) (*modifyVolumeRequest
 				options.modifyTagsOptions.TagsToAdd[st[0]] = st[1]
 			} else if strings.HasPrefix(key, ModificationDeleteTag) {
 				options.modifyTagsOptions.TagsToDelete = append(options.modifyTagsOptions.TagsToDelete, value)
+			} else {
+				return nil, status.Errorf(codes.InvalidArgument, "Invalid mutable parameter key: %s", key)
 			}
 		}
 	}

diff --git a/pkg/driver/controller_modify_volume_test.go b/pkg/driver/controller_modify_volume_test.go
@@ -37,6 +37,7 @@ const (
 	tagSpecificationWithNoEqual = "key1"
 	validTagDeletion            = "key2"
 	invalidTagSpecification     = "aws:test=TEST"
+	invalidParameter            = "invalid_parameter"
 )
 
 func TestParseModifyVolumeParameters(t *testing.T) {
@@ -125,6 +126,13 @@ func TestParseModifyVolumeParameters(t *testing.T) {
 			},
 			expectError: true,
 		},
+		{
+			name: "invalid VAC parameter",
+			params: map[string]string{
+				invalidParameter: "20",
+			},
+			expectError: true,
+		},
 	}
 
 	for _, tc := range testCases {

diff --git a/tests/e2e/modify_volume.go b/tests/e2e/modify_volume.go
@@ -37,72 +37,72 @@ var (
 	modifyVolumeTests = map[string]testsuites.ModifyVolumeTest{
 		"with a new iops annotation": {
 			CreateVolumeParameters: defaultModifyVolumeTestGp3CreateVolumeParameters,
-			ModifyVolumeAnnotations: map[string]string{
-				testsuites.AnnotationIops: "4000",
+			ModifyVolumeParameters: map[string]string{
+				testsuites.Iops: "4000",
 			},
 			ShouldResizeVolume:                    false,
 			ShouldTestInvalidModificationRecovery: false,
 		},
 		"with a new io2 volumeType annotation": {
 			CreateVolumeParameters: defaultModifyVolumeTestGp3CreateVolumeParameters,
-			ModifyVolumeAnnotations: map[string]string{
-				testsuites.AnnotationVolumeType: awscloud.VolumeTypeIO2,
-				testsuites.AnnotationIops:       testsuites.DefaultIopsIoVolumes, // As of aws-ebs-csi-driver v1.25.0, parameter iops must be re-specified when modifying volumeType io2 volumes.
+			ModifyVolumeParameters: map[string]string{
+				testsuites.VolumeType: awscloud.VolumeTypeIO2,
+				testsuites.Iops:       testsuites.DefaultIopsIoVolumes, // As of aws-ebs-csi-driver v1.25.0, parameter iops must be re-specified when modifying volumeType io2 volumes.
 			},
 			ShouldResizeVolume:                    false,
 			ShouldTestInvalidModificationRecovery: false,
 		},
 		"with a new throughput annotation": {
 			CreateVolumeParameters: defaultModifyVolumeTestGp3CreateVolumeParameters,
-			ModifyVolumeAnnotations: map[string]string{
-				testsuites.AnnotationThroughput: "150",
+			ModifyVolumeParameters: map[string]string{
+				testsuites.Throughput: "150",
 			},
 			ShouldResizeVolume:                    false,
 			ShouldTestInvalidModificationRecovery: false,
 		},
 		"with a new tag annotation": {
 			CreateVolumeParameters: defaultModifyVolumeTestGp3CreateVolumeParameters,
-			ModifyVolumeAnnotations: map[string]string{
-				testsuites.AnnotationsTagSpec: "key1=test1",
+			ModifyVolumeParameters: map[string]string{
+				testsuites.TagSpec: "key1=test1",
 			},
 			ShouldResizeVolume:                    false,
 			ShouldTestInvalidModificationRecovery: false,
 			ExternalResizerOnly:                   true,
 		},
 		"with new throughput, and iops annotations": {
 			CreateVolumeParameters: defaultModifyVolumeTestGp3CreateVolumeParameters,
-			ModifyVolumeAnnotations: map[string]string{
-				testsuites.AnnotationIops:       "4000",
-				testsuites.AnnotationThroughput: "150",
+			ModifyVolumeParameters: map[string]string{
+				testsuites.Iops:       "4000",
+				testsuites.Throughput: "150",
 			},
 			ShouldResizeVolume:                    false,
 			ShouldTestInvalidModificationRecovery: false,
 		},
 		"with new throughput, iops, and tag annotations": {
 			CreateVolumeParameters: defaultModifyVolumeTestGp3CreateVolumeParameters,
-			ModifyVolumeAnnotations: map[string]string{
-				testsuites.AnnotationIops:       "4000",
-				testsuites.AnnotationThroughput: "150",
-				testsuites.AnnotationsTagSpec:   "key2=test2",
+			ModifyVolumeParameters: map[string]string{
+				testsuites.Iops:       "4000",
+				testsuites.Throughput: "150",
+				testsuites.TagSpec:    "key2=test2",
 			},
 			ShouldResizeVolume:                    false,
 			ShouldTestInvalidModificationRecovery: false,
 			ExternalResizerOnly:                   true,
 		},
 		"with a larger size and new throughput and iops annotations": {
 			CreateVolumeParameters: defaultModifyVolumeTestGp3CreateVolumeParameters,
-			ModifyVolumeAnnotations: map[string]string{
-				testsuites.AnnotationIops:       "4000",
-				testsuites.AnnotationThroughput: "150",
+			ModifyVolumeParameters: map[string]string{
+				testsuites.Iops:       "4000",
+				testsuites.Throughput: "150",
 			},
 			ShouldResizeVolume:                    true,
 			ShouldTestInvalidModificationRecovery: false,
 		},
 		"with a larger size and new throughput and iops annotations after providing an invalid annotation": {
 			CreateVolumeParameters: defaultModifyVolumeTestGp3CreateVolumeParameters,
-			ModifyVolumeAnnotations: map[string]string{
-				testsuites.AnnotationIops:       "4000",
-				testsuites.AnnotationThroughput: "150",
+			ModifyVolumeParameters: map[string]string{
+				testsuites.Iops:       "4000",
+				testsuites.Throughput: "150",
 			},
 			ShouldResizeVolume:                    true,
 			ShouldTestInvalidModificationRecovery: true,
@@ -113,10 +113,10 @@ var (
 				ebscsidriver.FSTypeKey:     ebscsidriver.FSTypeExt4,
 				ebscsidriver.IopsKey:       testsuites.DefaultIopsIoVolumes,
 			},
-			ModifyVolumeAnnotations: map[string]string{
-				testsuites.AnnotationVolumeType: awscloud.VolumeTypeGP3,
-				testsuites.AnnotationIops:       "4000",
-				testsuites.AnnotationThroughput: "150",
+			ModifyVolumeParameters: map[string]string{
+				testsuites.VolumeType: awscloud.VolumeTypeGP3,
+				testsuites.Iops:       "4000",
+				testsuites.Throughput: "150",
 			},
 			ShouldResizeVolume:                    true,
 			ShouldTestInvalidModificationRecovery: false,

diff --git a/tests/e2e/testsuites/e2e_utils.go b/tests/e2e/testsuites/e2e_utils.go
@@ -39,11 +39,11 @@ const (
 	DefaultResizeTimout          = 1 * time.Minute
 	DefaultK8sApiPollingInterval = 5 * time.Second
 
-	AnnotationIops       = "ebs.csi.aws.com/iops"
-	AnnotationThroughput = "ebs.csi.aws.com/throughput"
-	AnnotationVolumeType = "ebs.csi.aws.com/volumeType"
-	AnnotationsTagSpec   = "ebs.csi.aws.com/tagSpecification"
-	AnnotationTagDel     = "ebs.csi.aws.com/tagDeletion"
+	Iops       = "iops"
+	Throughput = "throughput"
+	VolumeType = "type"
+	TagSpec    = "tagSpecification"
+	TagDel     = "tagDeletion"
 )
 
 var DefaultGeneratedVolumeMount = VolumeMountDetails{
@@ -173,3 +173,11 @@ func CreateVolumeDetails(createVolumeParameters map[string]string, volumeSize st
 
 	return &volume
 }
+
+func PrefixAnnotations(prefix string, parameters map[string]string) map[string]string {
+	result := make(map[string]string)
+	for key, value := range parameters {
+		result[prefix+key] = value
+	}
+	return result
+}
diff --git a/tests/e2e/testsuites/modify_volume_tester.go b/tests/e2e/testsuites/modify_volume_tester.go
@@ -31,15 +31,15 @@ import (
 // ModifyVolumeTest will provision pod with attached volume, and test that modifying its pvc will modify the associated pv.
 type ModifyVolumeTest struct {
 	CreateVolumeParameters                map[string]string
-	ModifyVolumeAnnotations               map[string]string
+	ModifyVolumeParameters                map[string]string
 	ShouldResizeVolume                    bool
 	ShouldTestInvalidModificationRecovery bool
 	ExternalResizerOnly                   bool
 }
 
 var (
 	invalidAnnotations = map[string]string{
-		AnnotationIops: "1",
+		Iops: "1",
 	}
 	volumeSize = "10Gi" // Different from driver.MinimumSizeForVolumeType to simplify iops, throughput, volumeType modification
 )
@@ -57,6 +57,8 @@ func (modifyVolumeTest *ModifyVolumeTest) Run(c clientset.Interface, ns *v1.Name
 	testVolume, _ := volumeDetails.SetupDynamicPersistentVolumeClaim(c, ns, ebsDriver)
 	defer testVolume.Cleanup()
 
+	parametersWithPrefix := PrefixAnnotations("ebs.csi.aws.com/", modifyVolumeTest.ModifyVolumeParameters)
+
 	By("deploying pod continuously writing to volume")
 	formatOptionMountPod := createPodWithVolume(c, ns, PodCmdContinuousWrite(DefaultMountPath), testVolume, volumeDetails)
 	defer formatOptionMountPod.Cleanup()
@@ -70,15 +72,15 @@ func (modifyVolumeTest *ModifyVolumeTest) Run(c clientset.Interface, ns *v1.Name
 	By("modifying the pvc")
 	modifyingPvc, _ := c.CoreV1().PersistentVolumeClaims(ns.Name).Get(context.TODO(), testVolume.persistentVolumeClaim.Name, metav1.GetOptions{})
 	if testType == VolumeModifierForK8s {
-		AnnotatePvc(modifyingPvc, modifyVolumeTest.ModifyVolumeAnnotations)
+		AnnotatePvc(modifyingPvc, parametersWithPrefix)
 	} else if testType == ExternalResizer {
 		vac, err := c.StorageV1alpha1().VolumeAttributesClasses().Create(context.Background(), &v1alpha1.VolumeAttributesClass{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      formatOptionMountPod.pod.Name,
 				Namespace: ns.Name,
 			},
 			DriverName: "ebs.csi.aws.com",
-			Parameters: modifyVolumeTest.ModifyVolumeAnnotations,
+			Parameters: modifyVolumeTest.ModifyVolumeParameters,
 		}, metav1.CreateOptions{})
 		framework.ExpectNoError(err)
 
@@ -100,7 +102,7 @@ func (modifyVolumeTest *ModifyVolumeTest) Run(c clientset.Interface, ns *v1.Name
 
 	By("wait for and confirm pv modification")
 	if testType == VolumeModifierForK8s {
-		err = WaitForPvToModify(c, ns, testVolume.persistentVolume.Name, modifyVolumeTest.ModifyVolumeAnnotations, DefaultModificationTimeout, DefaultK8sApiPollingInterval)
+		err = WaitForPvToModify(c, ns, testVolume.persistentVolume.Name, parametersWithPrefix, DefaultModificationTimeout, DefaultK8sApiPollingInterval)
 	} else if testType == ExternalResizer {
 		err = WaitForVacToApplyToPv(c, ns, testVolume.persistentVolume.Name, *modifyingPvc.Spec.VolumeAttributesClassName, DefaultModificationTimeout, DefaultK8sApiPollingInterval)
 	}