Repository: kubernetes-sigs/gcp-filestore-csi-driver
Pull request #572 (2023-08-03 22:40:30)

[release-1.3] Fix backup source comparison logic for single share instances
This is an automated cherry-pick of #447

/assign judemars

```release-note
Fix backup source comparison logic for single share instances
```


Patch:
diff --git a/pkg/cloud_provider/file/fake.go b/pkg/cloud_provider/file/fake.go
index 8cf82a86..d76457d5 100644
--- a/pkg/cloud_provider/file/fake.go
+++ b/pkg/cloud_provider/file/fake.go
@@ -164,8 +164,9 @@ func (manager *fakeServiceManager) CreateBackup(ctx context.Context, obj *Servic
 
 	backupSource := fmt.Sprintf("projects/%s/locations/%s/instances/%s", obj.Project, obj.Location, obj.Name)
 	if backupInfo, ok := manager.backups[backupUri]; ok {
-		if backupInfo.SourceVolumeHandle != backupSource {
-			return nil, fmt.Errorf("Mismatch in source volume handle for existing snapshot")
+		if backupInfo.SourceInstance != backupSource && backupInfo.SourceShare != obj.Volume.Name {
+			// TODO: format the right info
+			return nil, fmt.Errorf("Mismatch in source for existing snapshot %v", backupInfo)
 		}
 		return backupInfo.Backup, nil
 	}
@@ -179,8 +180,9 @@ func (manager *fakeServiceManager) CreateBackup(ctx context.Context, obj *Servic
 		CapacityGb:      defaultCapacityGb,
 	}
 	manager.backups[backupUri] = &BackupInfo{
-		Backup:             backupToCreate,
-		SourceVolumeHandle: backupSource,
+		Backup:         backupToCreate,
+		SourceInstance: backupSource,
+		SourceShare:    obj.Volume.Name,
 	}
 	return backupToCreate, nil
 }
diff --git a/pkg/cloud_provider/file/file.go b/pkg/cloud_provider/file/file.go
index 672e6490..f6ccc7d2 100644
--- a/pkg/cloud_provider/file/file.go
+++ b/pkg/cloud_provider/file/file.go
@@ -108,8 +108,9 @@ type Network struct {
 }
 
 type BackupInfo struct {
-	Backup             *filev1beta1.Backup
-	SourceVolumeHandle string
+	Backup         *filev1beta1.Backup
+	SourceInstance string
+	SourceShare    string
 }
 
 type Service interface {
@@ -496,8 +497,9 @@ func (manager *gcfsServiceManager) GetBackup(ctx context.Context, backupUri stri
 		return nil, err
 	}
 	return &BackupInfo{
-		Backup:             backup,
-		SourceVolumeHandle: backup.SourceInstance,
+		Backup:         backup,
+		SourceInstance: backup.SourceInstance,
+		SourceShare:    backup.SourceFileShare,
 	}, nil
 }
 
diff --git a/pkg/csi_driver/controller.go b/pkg/csi_driver/controller.go
index 11118df9..c11cace8 100644
--- a/pkg/csi_driver/controller.go
+++ b/pkg/csi_driver/controller.go
@@ -775,12 +775,12 @@ func (s *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSn
 			return nil, status.Error(codes.Internal, err.Error())
 		}
 	} else {
-		backupSourceCSIHandle, err := util.BackupVolumeSourceToCSIVolumeHandle(backupInfo.SourceVolumeHandle)
+		backupSourceCSIHandle, err := util.BackupVolumeSourceToCSIVolumeHandle(backupInfo.SourceInstance, backupInfo.SourceShare)
 		if err != nil {
-			return nil, status.Errorf(codes.Internal, "Cannot determine volume handle from back source %s", backupInfo.SourceVolumeHandle)
+			return nil, status.Errorf(codes.Internal, "Cannot determine volume handle from back source instance %s, share %s", backupInfo.SourceInstance, backupInfo.SourceShare)
 		}
 		if backupSourceCSIHandle != volumeID {
-			return nil, status.Errorf(codes.AlreadyExists, "Backup already exists with a different source volume %s, input source volume %s", backupInfo.SourceVolumeHandle, volumeID)
+			return nil, status.Errorf(codes.AlreadyExists, "Backup already exists with a different source volume %s, input source volume %s", backupSourceCSIHandle, volumeID)
 		}
 		// Check if backup is in the process of getting created.
 		if backupInfo.Backup.State == "CREATING" || backupInfo.Backup.State == "FINALIZING" {
diff --git a/pkg/util/util.go b/pkg/util/util.go
index 7ccea3f0..6a549324 100644
--- a/pkg/util/util.go
+++ b/pkg/util/util.go
@@ -198,12 +198,12 @@ func GetBackupLocation(params map[string]string) string {
 	return location
 }
 
-func BackupVolumeSourceToCSIVolumeHandle(backupVolumeSource string) (string, error) {
-	splitId := strings.Split(backupVolumeSource, "/")
+func BackupVolumeSourceToCSIVolumeHandle(sourceInstance, sourceShare string) (string, error) {
+	splitId := strings.Split(sourceInstance, "/")
 	if len(splitId) != volumeTotalElements {
-		return "", fmt.Errorf("Failed to get id components. Expected 'projects/{project}/location/{zone}/instances/{name}'. Got: %s", backupVolumeSource)
+		return "", fmt.Errorf("Failed to get id components. Expected 'projects/{project}/location/{zone}/instances/{name}'. Got: %s", sourceInstance)
 	}
-	return fmt.Sprintf("modeInstance/%s/%s/vol1", splitId[3], splitId[5]), nil
+	return fmt.Sprintf("modeInstance/%s/%s/%s", splitId[3], splitId[5], sourceShare), nil
 }
 
 // Multishare util functions.


Test patch:
diff --git a/pkg/csi_driver/controller_test.go b/pkg/csi_driver/controller_test.go
index 708e8c30..5c830e21 100644
--- a/pkg/csi_driver/controller_test.go
+++ b/pkg/csi_driver/controller_test.go
@@ -883,22 +883,99 @@ func ValidateExpectedError(t *testing.T, errResp <-chan error, operationUnblocke
 }
 
 func TestCreateSnapshot(t *testing.T) {
+	type BackupInfo struct {
+		s              *file.ServiceInstance
+		backupName     string
+		backupLocation string
+	}
+	backupName := "mybackup"
+	project := "test-project"
+	zone := "us-central1-c"
+	region := "us-central1"
+	instanceName := "myinstance"
+	shareName := "myshare"
 	cases := []struct {
-		name      string
-		req       *csi.CreateSnapshotRequest
-		expectErr bool
+		name          string
+		req           *csi.CreateSnapshotRequest
+		initialBackup *BackupInfo
+		expectErr     bool
 	}{
+		// Failure test cases
 		{
 			name: "Create snapshot request for multishare backed volumes",
 			req: &csi.CreateSnapshotRequest{
-				SourceVolumeId: "modeMultishare/mysc/myproject/location/instancename/sharename",
+				SourceVolumeId: "modeMultishare/mysc/test-project/location/myinstance/myshare",
 			},
 			expectErr: true,
 		},
+		{
+			name: "Existing backup found, with different volume Id (source zonal filestore instance), error expected",
+			req: &csi.CreateSnapshotRequest{
+				SourceVolumeId: "modeInstance/us-central1-c/myinstance1/myshare",
+				Name:           backupName,
+				Parameters: map[string]string{
+					util.VolumeSnapshotTypeKey: "backup",
+				},
+			},
+			initialBackup: &BackupInfo{
+				s: &file.ServiceInstance{
+					Project:  project,
+					Location: zone,
+					Name:     instanceName,
+					Volume: file.Volume{
+						Name: shareName,
+					},
+				},
+				backupName:     backupName,
+				backupLocation: region,
+			},
+			expectErr: true,
+		},
+		// Success test cases
+		{
+			name: "Existing backup found, with same source volume Id (source zonal filestore instance)",
+			req: &csi.CreateSnapshotRequest{
+				SourceVolumeId: "modeInstance/us-central1-c/myinstance/myshare",
+				Name:           backupName,
+				Parameters: map[string]string{
+					util.VolumeSnapshotTypeKey: "backup",
+				},
+			},
+			initialBackup: &BackupInfo{
+				s: &file.ServiceInstance{
+					Project:  project,
+					Location: zone,
+					Name:     instanceName,
+					Volume: file.Volume{
+						Name: shareName,
+					},
+				},
+				backupName:     backupName,
+				backupLocation: region,
+			},
+		},
 	}
 	for _, test := range cases {
-		cs := initTestController(t)
-		_, err := cs.CreateSnapshot(context.TODO(), test.req)
+		fileService, err := file.NewFakeService()
+		if err != nil {
+			t.Fatalf("failed to initialize GCFS service: %v", err)
+		}
+
+		cloudProvider, err := cloud.NewFakeCloud()
+		if err != nil {
+			t.Fatalf("Failed to get cloud provider: %v", err)
+		}
+		cs := newControllerServer(&controllerServerConfig{
+			driver:      initTestDriver(t),
+			fileService: fileService,
+			cloud:       cloudProvider,
+			volumeLocks: util.NewVolumeLocks(),
+		})
+
+		if test.initialBackup != nil {
+			fileService.CreateBackup(context.TODO(), test.initialBackup.s, test.initialBackup.backupName, test.initialBackup.backupLocation)
+		}
+		_, err = cs.CreateSnapshot(context.TODO(), test.req)
 		if !test.expectErr && err != nil {
 			t.Errorf("test %q failed: %v", test.name, err)
 		}