Merge pull request #409 from jacobwolfaws/master

Handle empty and misconfigured extraTags in controller

Author: k8s-ci-robot

pkg/driver/controller.go

@@ -249,8 +249,12 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 		tagArray = strings.Split(optionsTags, ",")
 	}
 
-	if val, ok := volumeParams[volumeParamsExtraTags]; ok {
+	if val, ok := volumeParams[volumeParamsExtraTags]; ok && len(val) > 0 {
 		extraTags := strings.Split(val, ",")
+		err := validateExtraTags(extraTags)
+		if err != nil {
+			return nil, status.Error(codes.InvalidArgument, err.Error())
+		}
 		tagArray = append(tagArray, extraTags...)
 	}
 	fsOptions.ExtraTags = tagArray
@@ -474,3 +478,13 @@ func newCreateVolumeResponse(fs *cloud.FileSystem) *csi.CreateVolumeResponse {
 		},
 	}
 }
+
+func validateExtraTags(tags []string) error {
+	for _, tag := range tags {
+		tagSplit := strings.Split(tag, "=")
+		if len(tagSplit) != 2 {
+			return fmt.Errorf("invalid extraTag %s was provided", tag)
+		}
+	}
+	return nil
+}

pkg/driver/controller_test.go

@@ -56,7 +56,9 @@ func TestCreateVolume(t *testing.T) {
 				Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER,
 			},
 		}
-		extraTags = "key1=value1,key2=value2"
+		extraTags        = "key1=value1,key2=value2"
+		emptyExtraTags   = ""
+		invalidExtraTags = "key1=value1,value2"
 	)
 	testCases := []struct {
 		name     string
@@ -374,6 +376,111 @@ func TestCreateVolume(t *testing.T) {
 				mockCtl.Finish()
 			},
 		},
+		{
+			name: "success: empty extraTags",
+			testFunc: func(t *testing.T) {
+				mockCtl := gomock.NewController(t)
+				mockCloud := mocks.NewMockCloud(mockCtl)
+
+				driver := controllerService{
+					cloud:         mockCloud,
+					inFlight:      internal.NewInFlight(),
+					driverOptions: &DriverOptions{},
+				}
+
+				req := &csi.CreateVolumeRequest{
+					Name: volumeName,
+					VolumeCapabilities: []*csi.VolumeCapability{
+						stdVolCap,
+					},
+					Parameters: map[string]string{
+						volumeParamsSubnetId:         subnetId,
+						volumeParamsSecurityGroupIds: securityGroupIds,
+						volumeParamsExtraTags:        emptyExtraTags,
+					},
+				}
+
+				ctx := context.Background()
+				fs := &cloud.FileSystem{
+					FileSystemId: fileSystemId,
+					CapacityGiB:  volumeSizeGiB,
+					DnsName:      dnsName,
+					MountName:    mountName,
+				}
+				mockCloud.EXPECT().CreateFileSystem(gomock.Eq(ctx), gomock.Eq(volumeName), gomock.Any()).Return(fs, nil)
+				mockCloud.EXPECT().WaitForFileSystemAvailable(gomock.Eq(ctx), gomock.Eq(fileSystemId)).Return(nil)
+
+				resp, err := driver.CreateVolume(ctx, req)
+				if err != nil {
+					t.Fatalf("CreateVolume is failed: %v", err)
+				}
+
+				if resp.Volume == nil {
+					t.Fatal("resp.Volume is nil")
+				}
+
+				if resp.Volume.VolumeId != fileSystemId {
+					t.Fatalf("VolumeId mismatches. actual: %v expected: %v", resp.Volume.VolumeId, fileSystemId)
+				}
+
+				if resp.Volume.CapacityBytes == 0 {
+					t.Fatalf("resp.Volume.CapacityGiB is zero")
+				}
+
+				dnsname, exists := resp.Volume.VolumeContext[volumeContextDnsName]
+				if !exists {
+					t.Fatal("dnsname is missing")
+				}
+
+				if dnsname != dnsName {
+					t.Fatalf("dnsname mismatches. actual: %v expected: %v", dnsname, dnsName)
+				}
+
+				mountname, exists := resp.Volume.VolumeContext[volumeContextMountName]
+				if !exists {
+					t.Fatal("mountname is missing")
+				}
+
+				if mountname != mountName {
+					t.Fatalf("mountname mismatches. actual: %v expected: %v", mountname, mountName)
+				}
+
+				mockCtl.Finish()
+			},
+		},
+		{
+			name: "fail: invalid extraTags",
+			testFunc: func(t *testing.T) {
+				mockCtl := gomock.NewController(t)
+				mockCloud := mocks.NewMockCloud(mockCtl)
+
+				driver := controllerService{
+					cloud:         mockCloud,
+					inFlight:      internal.NewInFlight(),
+					driverOptions: &DriverOptions{},
+				}
+
+				req := &csi.CreateVolumeRequest{
+					Name: volumeName,
+					VolumeCapabilities: []*csi.VolumeCapability{
+						stdVolCap,
+					},
+					Parameters: map[string]string{
+						volumeParamsSubnetId:         subnetId,
+						volumeParamsSecurityGroupIds: securityGroupIds,
+						volumeParamsExtraTags:        invalidExtraTags,
+					},
+				}
+
+				ctx := context.Background()
+				_, err := driver.CreateVolume(ctx, req)
+				if err == nil {
+					t.Fatal("CreateVolume is not failed")
+				}
+
+				mockCtl.Finish()
+			},
+		},
 		{
 			name: "fail: volume name missing",
 			testFunc: func(t *testing.T) {