diff --git a/pkg/csi_driver/controller.go b/pkg/csi_driver/controller.go index 9b53858c9..2a271080c 100644 --- a/pkg/csi_driver/controller.go +++ b/pkg/csi_driver/controller.go @@ -292,14 +292,10 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu } // Add labels. - labels, err := extractLabels(param, s.config.driver.config.Name) + labels, err := extractLabels(param, s.config.extraVolumeLabels, s.config.driver.config.Name) if err != nil { return nil, file.StatusError(err) } - // Append extra lables from the command line option - for k, v := range s.config.extraVolumeLabels { - labels[k] = v - } newFiler.Labels = labels // Create the instance @@ -838,7 +834,7 @@ func getZoneFromSegment(seg map[string]string) (string, error) { return zone, nil } -func extractLabels(parameters map[string]string, driverName string) (map[string]string, error) { +func extractLabels(parameters, cliLabels map[string]string, driverName string) (map[string]string, error) { labels := make(map[string]string) scLables := make(map[string]string) for k, v := range parameters { @@ -859,12 +855,12 @@ func extractLabels(parameters map[string]string, driverName string) (map[string] } labels[tagKeyCreatedBy] = strings.ReplaceAll(driverName, ".", "_") - return mergeLabels(scLables, labels) + return mergeLabels(scLables, labels, cliLabels) } -func mergeLabels(scLabels map[string]string, metedataLabels map[string]string) (map[string]string, error) { +func mergeLabels(scLabels, metadataLabels, cliLabels map[string]string) (map[string]string, error) { result := make(map[string]string) - for k, v := range metedataLabels { + for k, v := range metadataLabels { result[k] = v } @@ -876,6 +872,14 @@ func mergeLabels(scLabels map[string]string, metedataLabels map[string]string) ( result[k] = v } + // add labels from command line with precedence given to + // metadata and storage class labels in same order. + for k, v := range cliLabels { + if _, ok := result[k]; !ok { + result[k] = v + } + } + return result, nil } @@ -948,7 +952,7 @@ func (s *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSn } else { // create new backup - labels, err := extractBackupLabels(req.GetParameters(), s.config.driver.config.Name, req.Name) + labels, err := extractBackupLabels(req.GetParameters(), s.config.extraVolumeLabels, s.config.driver.config.Name, req.Name) if err != nil { return nil, err } @@ -978,8 +982,8 @@ func (s *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSn } -func extractBackupLabels(parameters map[string]string, driverName string, snapshotName string) (map[string]string, error) { - labels, err := extractLabels(parameters, driverName) +func extractBackupLabels(parameters, cliLabels map[string]string, driverName string, snapshotName string) (map[string]string, error) { + labels, err := extractLabels(parameters, cliLabels, driverName) if err != nil { return nil, err } diff --git a/pkg/csi_driver/controller_test.go b/pkg/csi_driver/controller_test.go index 01b280af7..a9b7081fa 100644 --- a/pkg/csi_driver/controller_test.go +++ b/pkg/csi_driver/controller_test.go @@ -131,7 +131,10 @@ func TestCreateVolumeFromSnapshot(t *testing.T) { }, }, }, - Parameters: map[string]string{"tier": defaultTier}, + Parameters: map[string]string{ + "tier": defaultTier, + ParameterKeyLabels: "key1=value1", + }, VolumeCapabilities: volumeCapabilities, }, resp: &csi.CreateVolumeResponse{ @@ -343,6 +346,34 @@ func TestCreateVolumeFromSnapshot(t *testing.T) { SourceVolumeId: modeInstance + "/" + testRegion + "/" + instanceName + "/" + shareName, }, }, + { + name: "Parameters contain misconfigured labels(invalid KV separator(:) used)", + req: &csi.CreateVolumeRequest{ + Name: testCSIVolume, + Parameters: map[string]string{ + "tier": enterpriseTier, + ParameterKeyLabels: "key1:value1", + }, + }, + resp: nil, + expectedOptions: nil, + initialBackup: &BackupInfo{ + s: &file.ServiceInstance{ + Project: testProject, + Location: testRegion, + Name: instanceName, + Tier: enterpriseTier, + Volume: file.Volume{ + Name: shareName, + SizeBytes: testBytes, + }, + }, + backupName: backupName, + backupLocation: testRegion, + SourceVolumeId: modeInstance + "/" + testRegion + "/" + instanceName + "/" + shareName, + }, + expectErr: true, + }, } for _, test := range cases { @@ -359,9 +390,11 @@ func TestCreateVolumeFromSnapshot(t *testing.T) { SourceShare: test.initialBackup.s.Volume.Name, Name: test.initialBackup.backupName, SourceVolumeId: test.initialBackup.SourceVolumeId, - BackupURI: test.resp.Volume.ContentSource.GetSnapshot().SnapshotId, Labels: make(map[string]string), } + if test.resp != nil { + backupInfo.BackupURI = test.resp.Volume.ContentSource.GetSnapshot().SnapshotId + } cs.config.fileService.CreateBackup(context.TODO(), backupInfo) @@ -1616,6 +1649,30 @@ func TestCreateSnapshot(t *testing.T) { }, expectErr: true, }, + { + name: "Parameters contain misconfigured labels(invalid KV separator(:) used)", + req: &csi.CreateSnapshotRequest{ + SourceVolumeId: "modeInstance/us-central1/myinstance/myshare", + Name: backupName, + Parameters: map[string]string{ + util.VolumeSnapshotTypeKey: "backup", + ParameterKeyLabels: "key1:value1", + }, + }, + initialBackup: &BackupTestInfo{ + backup: &file.BackupInfo{ + Project: project, + Location: region, + SourceInstanceName: instanceName, + SourceShare: shareName, + Name: backupName, + BackupURI: defaultBackupUri, + SourceVolumeId: "modeInstance/us-central1/myinstance/myshare", + }, + state: "CREATING", + }, + expectErr: true, + }, // Success test cases { name: "No backup found", @@ -1717,6 +1774,30 @@ func TestCreateSnapshot(t *testing.T) { }, }, }, + { + // If the incorrect labels were added, labels processing will not happen for already + // existing backup resources. + name: "Existing backup found, in state READY. Labels will not be processed.", + req: &csi.CreateSnapshotRequest{ + SourceVolumeId: "modeInstance/us-central1-c/myinstance/myshare", + Name: backupName, + Parameters: map[string]string{ + util.VolumeSnapshotTypeKey: "backup", + ParameterKeyLabels: "key1:value1", + }, + }, + initialBackup: &BackupTestInfo{ + backup: &file.BackupInfo{ + Project: project, + Location: region, + SourceInstanceName: instanceName, + SourceShare: shareName, + Name: backupName, + BackupURI: defaultBackupUri, + SourceVolumeId: "modeInstance/us-central1-c/myinstance/myshare", + }, + }, + }, } for _, test := range cases { fileService, err := file.NewFakeService() @@ -2128,3 +2209,319 @@ func TestParsingNfsExportOptions(t *testing.T) { } } } + +func TestExtractLabels(t *testing.T) { + var ( + driverName = "test_driver" + pvcName = "test_pvc" + pvcNamespace = "test_pvc_namespace" + pvName = "test_pv" + parameterLabels = "key1=value1,key2=value2" + ) + + cases := []struct { + name string + parameters map[string]string + cliLabels map[string]string + expectLabels map[string]string + expectError string + }{ + { + name: "Success case", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + ParameterKeyLabels: parameterLabels, + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectLabels: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + "key4": "value4", + tagKeyCreatedForVolumeName: pvName, + tagKeyCreatedForClaimName: pvcName, + tagKeyCreatedForClaimNamespace: pvcNamespace, + tagKeyCreatedBy: driverName, + }, + }, + { + name: "Parsing labels in storageClass fails(invalid KV separator(:) used)", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + ParameterKeyLabels: "key1:value1,key2:value2", + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectLabels: nil, + expectError: `parameters contain invalid labels parameter: labels "key1:value1,key2:value2" are invalid, correct format: 'key1=value1,key2=value2'`, + }, + { + name: "storageClass labels contain reserved metadata label(kubernetes_io_created-for_pv_name)", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + ParameterKeyLabels: "key1=value1,key2=value2,kubernetes_io_created-for_pv_name=test", + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectLabels: nil, + expectError: `storage Class labels cannot contain metadata label key kubernetes_io_created-for_pv_name`, + }, + { + name: "storageClass labels parameter not present, only the CLI labels are defined", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + tagKeyCreatedForVolumeName: pvName, + tagKeyCreatedForClaimName: pvcName, + tagKeyCreatedForClaimNamespace: pvcNamespace, + tagKeyCreatedBy: driverName, + }, + }, + { + name: "CLI labels not defined, labels are defined only in the storageClass object", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + ParameterKeyLabels: parameterLabels, + }, + cliLabels: nil, + expectLabels: map[string]string{ + "key1": "value1", + "key2": "value2", + tagKeyCreatedForVolumeName: pvName, + tagKeyCreatedForClaimName: pvcName, + tagKeyCreatedForClaimNamespace: pvcNamespace, + tagKeyCreatedBy: driverName, + }, + }, + { + name: "CLI labels and storageClass labels parameter not defined", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + }, + cliLabels: nil, + expectLabels: map[string]string{ + tagKeyCreatedForVolumeName: pvName, + tagKeyCreatedForClaimName: pvcName, + tagKeyCreatedForClaimNamespace: pvcNamespace, + tagKeyCreatedBy: driverName, + }, + }, + { + name: "CLI labels and storageClass labels has duplicates", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + ParameterKeyLabels: parameterLabels, + }, + cliLabels: map[string]string{ + "key1": "value1", + "key2": "value202", + }, + expectLabels: map[string]string{ + "key1": "value1", + "key2": "value2", + tagKeyCreatedForVolumeName: pvName, + tagKeyCreatedForClaimName: pvcName, + tagKeyCreatedForClaimNamespace: pvcNamespace, + tagKeyCreatedBy: driverName, + }, + }, + } + for _, test := range cases { + labels, err := extractLabels(test.parameters, test.cliLabels, driverName) + if (err != nil || test.expectError != "") && err.Error() != test.expectError { + t.Errorf("extractLabels(): %s: got: %v, expectErr: %v", test.name, err, test.expectError) + } + if !reflect.DeepEqual(test.expectLabels, labels) { + t.Errorf("extractLabels(): %s: got: %v, want: %v", test.name, labels, test.expectLabels) + } + } +} + +func TestExtractBackupLabels(t *testing.T) { + var ( + driverName = "test_driver" + snapshotName = "test_snapshot" + pvcName = "test_pvc" + pvcNamespace = "test_pvc_namespace" + pvName = "test_pv" + parameterLabels = "key1=value1,key2=value2" + ) + + cases := []struct { + name string + parameters map[string]string + cliLabels map[string]string + expectLabels map[string]string + expectError string + }{ + { + name: "Success case", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + ParameterKeyLabels: parameterLabels, + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectLabels: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + "key4": "value4", + tagKeyCreatedForVolumeName: pvName, + tagKeyCreatedForClaimName: pvcName, + tagKeyCreatedForClaimNamespace: pvcNamespace, + tagKeyCreatedBy: driverName, + tagKeySnapshotName: snapshotName, + }, + }, + { + name: "Parsing labels in storageClass fails(invalid KV separator(:) used)", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + ParameterKeyLabels: "key1:value1,key2:value2", + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectLabels: nil, + expectError: `parameters contain invalid labels parameter: labels "key1:value1,key2:value2" are invalid, correct format: 'key1=value1,key2=value2'`, + }, + { + name: "storageClass labels contain reserved metadata label(kubernetes_io_created-for_pv_name)", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + ParameterKeyLabels: "key1=value1,key2=value2,kubernetes_io_created-for_pv_name=test", + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectLabels: nil, + expectError: `storage Class labels cannot contain metadata label key kubernetes_io_created-for_pv_name`, + }, + { + name: "storageClass labels parameter not present, only the CLI labels are defined", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + tagKeyCreatedForVolumeName: pvName, + tagKeyCreatedForClaimName: pvcName, + tagKeyCreatedForClaimNamespace: pvcNamespace, + tagKeyCreatedBy: driverName, + tagKeySnapshotName: snapshotName, + }, + }, + { + name: "CLI labels not defined, labels are defined only in the storageClass object", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + ParameterKeyLabels: parameterLabels, + }, + cliLabels: nil, + expectLabels: map[string]string{ + "key1": "value1", + "key2": "value2", + tagKeyCreatedForVolumeName: pvName, + tagKeyCreatedForClaimName: pvcName, + tagKeyCreatedForClaimNamespace: pvcNamespace, + tagKeyCreatedBy: driverName, + tagKeySnapshotName: snapshotName, + }, + }, + { + name: "CLI labels and storageClass labels parameter not defined", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + }, + cliLabels: nil, + expectLabels: map[string]string{ + tagKeyCreatedForVolumeName: pvName, + tagKeyCreatedForClaimName: pvcName, + tagKeyCreatedForClaimNamespace: pvcNamespace, + tagKeyCreatedBy: driverName, + tagKeySnapshotName: snapshotName, + }, + }, + { + name: "CLI labels and storageClass labels has duplicates", + parameters: map[string]string{ + ParameterKeyPVCName: pvcName, + ParameterKeyPVCNamespace: pvcNamespace, + ParameterKeyPVName: pvName, + ParameterKeyLabels: parameterLabels, + }, + cliLabels: map[string]string{ + "key1": "value1", + "key2": "value202", + }, + expectLabels: map[string]string{ + "key1": "value1", + "key2": "value2", + tagKeyCreatedForVolumeName: pvName, + tagKeyCreatedForClaimName: pvcName, + tagKeyCreatedForClaimNamespace: pvcNamespace, + tagKeyCreatedBy: driverName, + tagKeySnapshotName: snapshotName, + }, + }, + } + for _, test := range cases { + labels, err := extractBackupLabels(test.parameters, test.cliLabels, driverName, snapshotName) + if (err != nil || test.expectError != "") && err.Error() != test.expectError { + t.Errorf("extractBackupLabels(): %s: got: %v, expectErr: %v", test.name, err, test.expectError) + } + if !reflect.DeepEqual(test.expectLabels, labels) { + t.Errorf("extractBackupLabels(): %s: got: %v, want: %v", test.name, labels, test.expectLabels) + } + } +} diff --git a/pkg/csi_driver/multishare_controller.go b/pkg/csi_driver/multishare_controller.go index 2e95520d1..9968e33a4 100644 --- a/pkg/csi_driver/multishare_controller.go +++ b/pkg/csi_driver/multishare_controller.go @@ -68,6 +68,7 @@ type MultishareController struct { featureMaxSharePerInstance bool featureMultishareBackups bool featureNFSExportOptionsOnCreate bool + extraVolumeLabels map[string]string // Filestore instance description overrides descOverrideMaxSharesPerInstance string @@ -81,13 +82,14 @@ type MultishareController struct { func NewMultishareController(config *controllerServerConfig) *MultishareController { c := &MultishareController{ - driver: config.driver, - fileService: config.fileService, - cloud: config.cloud, - volumeLocks: config.volumeLocks, - ecfsDescription: config.ecfsDescription, - isRegional: config.isRegional, - clustername: config.clusterName, + driver: config.driver, + fileService: config.fileService, + cloud: config.cloud, + volumeLocks: config.volumeLocks, + ecfsDescription: config.ecfsDescription, + isRegional: config.isRegional, + clustername: config.clusterName, + extraVolumeLabels: config.extraVolumeLabels, } c.opsManager = NewMultishareOpsManager(config.cloud, c) if config.features != nil && config.features.FeatureMaxSharesPerInstance != nil { @@ -290,11 +292,12 @@ func (m *MultishareController) CreateSnapshot(ctx context.Context, req *csi.Crea BackupURI: backupURI, } - labels, err := extractBackupLabels(req.GetParameters(), m.driver.config.Name, req.Name) + labels, err := extractBackupLabels(req.GetParameters(), m.extraVolumeLabels, m.driver.config.Name, req.Name) if err != nil { return nil, err } backupInfo.Labels = labels + snapshot, err := m.createNewBackup(ctx, backupInfo) if err != nil { return nil, err @@ -607,7 +610,7 @@ func (m *MultishareController) generateNewMultishareInstance(instanceName string return nil, status.Errorf(codes.InvalidArgument, "failed to get region for regional cluster: %v", err.Error()) } } - labels, err := extractInstanceLabels(req.GetParameters(), m.driver.config.Name, m.clustername, location) + labels, err := extractInstanceLabels(req.GetParameters(), m.extraVolumeLabels, m.driver.config.Name, m.clustername, location) if err != nil { return nil, status.Errorf(codes.InvalidArgument, err.Error()) } @@ -708,7 +711,7 @@ func (m *MultishareController) pickRegion(top *csi.TopologyRequirement) (string, return region, nil } -func extractInstanceLabels(parameters map[string]string, driverName, clusterName, location string) (map[string]string, error) { +func extractInstanceLabels(parameters, cliLabels map[string]string, driverName, clusterName, location string) (map[string]string, error) { instanceLabels := make(map[string]string) userProvidedLabels := make(map[string]string) for k, v := range parameters { @@ -731,7 +734,7 @@ func extractInstanceLabels(parameters map[string]string, driverName, clusterName instanceLabels[tagKeyCreatedBy] = strings.ReplaceAll(driverName, ".", "_") instanceLabels[TagKeyClusterName] = clusterName instanceLabels[TagKeyClusterLocation] = location - finalInstanceLabels, err := mergeLabels(userProvidedLabels, instanceLabels) + finalInstanceLabels, err := mergeLabels(userProvidedLabels, instanceLabels, cliLabels) if err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) } diff --git a/pkg/csi_driver/multishare_controller_test.go b/pkg/csi_driver/multishare_controller_test.go index 7692bbd08..acfe00909 100644 --- a/pkg/csi_driver/multishare_controller_test.go +++ b/pkg/csi_driver/multishare_controller_test.go @@ -419,10 +419,15 @@ func TestGetShareRequestCapacity(t *testing.T) { } func TestExtractInstanceLabels(t *testing.T) { + var ( + parameterLabels = "key1=value1,key2=value2" + ) + tests := []struct { name string params map[string]string driver string + cliLabels map[string]string expectedLabel map[string]string expectErr bool }{ @@ -451,10 +456,108 @@ func TestExtractInstanceLabels(t *testing.T) { TagKeyClusterLocation: testLocation, }, }, + { + name: "Parsing labels in storageClass fails(invalid KV separator(:) used)", + driver: testDriverName, + params: map[string]string{ + ParamMultishareInstanceScLabel: "testsc", + ParameterKeyLabels: "key1:value1,key2:value2", + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectedLabel: nil, + expectErr: true, + }, + { + name: "storageClass labels contain reserved metadata label(storage_gke_io_created-by)", + driver: testDriverName, + params: map[string]string{ + ParamMultishareInstanceScLabel: "testsc", + ParameterKeyLabels: "key1=value1,key2=value2,storage_gke_io_created-by=test_filestore", + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectedLabel: nil, + expectErr: true, + }, + { + name: "storageClass labels parameter not present, only the CLI labels are defined", + driver: testDriverName, + params: map[string]string{ + ParamMultishareInstanceScLabel: "testsc", + }, + cliLabels: map[string]string{ + "key3": "value3", + "key4": "value4", + }, + expectedLabel: map[string]string{ + "key3": "value3", + "key4": "value4", + tagKeyCreatedBy: testDrivernameLabelValue, + util.ParamMultishareInstanceScLabelKey: "testsc", + TagKeyClusterName: testClusterName, + TagKeyClusterLocation: testLocation, + }, + }, + { + name: "CLI labels not defined, labels are defined only in storageClass object", + driver: testDriverName, + params: map[string]string{ + ParamMultishareInstanceScLabel: "testsc", + ParameterKeyLabels: parameterLabels, + }, + cliLabels: nil, + expectedLabel: map[string]string{ + "key1": "value1", + "key2": "value2", + tagKeyCreatedBy: testDrivernameLabelValue, + util.ParamMultishareInstanceScLabelKey: "testsc", + TagKeyClusterName: testClusterName, + TagKeyClusterLocation: testLocation, + }, + }, + { + name: "CLI labels and storageClass labels parameter not defined", + driver: testDriverName, + params: map[string]string{ + ParamMultishareInstanceScLabel: "testsc", + }, + cliLabels: nil, + expectedLabel: map[string]string{ + tagKeyCreatedBy: testDrivernameLabelValue, + util.ParamMultishareInstanceScLabelKey: "testsc", + TagKeyClusterName: testClusterName, + TagKeyClusterLocation: testLocation, + }, + }, + { + name: "CLI labels and storageClass labels has duplicates", + driver: testDriverName, + params: map[string]string{ + ParamMultishareInstanceScLabel: "testsc", + ParameterKeyLabels: parameterLabels, + }, + cliLabels: map[string]string{ + "key1": "value1", + "key2": "value202", + }, + expectedLabel: map[string]string{ + "key1": "value1", + "key2": "value2", + tagKeyCreatedBy: testDrivernameLabelValue, + util.ParamMultishareInstanceScLabelKey: "testsc", + TagKeyClusterName: testClusterName, + TagKeyClusterLocation: testLocation, + }, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - label, err := extractInstanceLabels(tc.params, tc.driver, testClusterName, testLocation) + label, err := extractInstanceLabels(tc.params, tc.cliLabels, tc.driver, testClusterName, testLocation) if tc.expectErr && err == nil { t.Error("expected error, got none") } @@ -2843,6 +2946,30 @@ func TestCreateMultishareSnapshot(t *testing.T) { }, expectErr: true, }, + { + name: "Parameters contain misconfigured labels(invalid KV separator(:) used)", + req: &csi.CreateSnapshotRequest{ + SourceVolumeId: "modeInstance/us-central1/myinstance/myshare", + Name: backupName, + Parameters: map[string]string{ + util.VolumeSnapshotTypeKey: "backup", + "labels": "key1:value1", + }, + }, + initialBackup: &BackupTestInfo{ + backup: &file.BackupInfo{ + Project: testProject, + Location: testRegion, + SourceInstanceName: testInstanceName1, + SourceShare: testShareName, + Name: backupName, + BackupURI: defaultBackupUri, + SourceVolumeId: modeMultishare + "/" + testRegion + "/" + testInstanceName1 + "/" + testShareName, + }, + state: "CREATING", + }, + expectErr: true, + }, //Success test cases { name: "No existing backup", @@ -2887,6 +3014,32 @@ func TestCreateMultishareSnapshot(t *testing.T) { state: "READY", }, }, + { + // If the incorrect labels were added, labels processing will not happen for already + // existing backup resources. + name: "Existing backup found, in state READY. Labels will not be processed.", + req: &csi.CreateSnapshotRequest{ + SourceVolumeId: defaultSourceVolumeID, + Name: backupName, + Parameters: map[string]string{ + util.VolumeSnapshotTypeKey: "backup", + "labels": "key1:value1", + }, + }, + features: features, + initialBackup: &BackupTestInfo{ + backup: &file.BackupInfo{ + Project: testProject, + Location: testRegion, + SourceInstanceName: testInstanceName1, + SourceShare: testShareName, + Name: backupName, + BackupURI: defaultBackupUri, + SourceVolumeId: modeMultishare + "/" + testRegion + "/" + testInstanceName1 + "/" + testShareName, + }, + state: "READY", + }, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/csi_driver/reconciler.go b/pkg/csi_driver/reconciler.go index dc9599ee1..71c068baa 100644 --- a/pkg/csi_driver/reconciler.go +++ b/pkg/csi_driver/reconciler.go @@ -485,7 +485,7 @@ func (recon *MultishareReconciler) generateNewMultishareInstance(instanceInfo *v } } - labels, err := extractInstanceLabels(params, recon.config.Name, recon.config.ClusterName, clusterLocation) + labels, err := extractInstanceLabels(params, recon.config.ExtraVolumeLabels, recon.config.Name, recon.config.ClusterName, clusterLocation) if err != nil { return nil, status.Errorf(codes.InvalidArgument, err.Error()) }