From ba2fb4a393d1338a8d0c66177e82297b62d45e9e Mon Sep 17 00:00:00 2001
From: Hanyue Liang <hanliang@amazon.com>
Date: Mon, 27 Mar 2023 21:54:16 +0000
Subject: [PATCH] Fix VolumeSnapshotClass tagging

Fix the bug of the tags in VolumeSnapshotClass.parameters being overwritten
by the tags automatically populated by the CSI Driver, such as the "Name"
tag that holds the snapshot name shown in the AWS EC2 console.
---
 pkg/driver/controller.go      |  9 +++---
 pkg/driver/controller_test.go | 58 +++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go
index f1bd3458e5..aa6093eef9 100644
--- a/pkg/driver/controller.go
+++ b/pkg/driver/controller.go
@@ -622,10 +622,6 @@ func (d *controllerService) CreateSnapshot(ctx context.Context, req *csi.CreateS
 		return nil, status.Errorf(codes.InvalidArgument, "Invalid tag value: %v", err)
 	}
 
-	for k, v := range addTags {
-		snapshotTags[k] = v
-	}
-
 	if d.driverOptions.kubernetesClusterID != "" {
 		resourceLifecycleTag := ResourceLifecycleTagPrefix + d.driverOptions.kubernetesClusterID
 		snapshotTags[resourceLifecycleTag] = ResourceLifecycleOwned
@@ -634,6 +630,11 @@ func (d *controllerService) CreateSnapshot(ctx context.Context, req *csi.CreateS
 	for k, v := range d.driverOptions.extraTags {
 		snapshotTags[k] = v
 	}
+
+	for k, v := range addTags {
+		snapshotTags[k] = v
+	}
+
 	opts := &cloud.SnapshotOptions{
 		Tags: snapshotTags,
 	}
diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go
index db04d63210..de957e9258 100644
--- a/pkg/driver/controller_test.go
+++ b/pkg/driver/controller_test.go
@@ -2398,6 +2398,64 @@ func TestCreateSnapshot(t *testing.T) {
 					t.Fatalf("Unexpected error: %v", err)
 				}
 
+				if snap := resp.GetSnapshot(); snap == nil {
+					t.Fatalf("Expected snapshot %v, got nil", expSnapshot)
+				}
+			},
+		},
+		{
+			name: "success with VolumeSnapshotClass with Name tag and cluster id",
+			testFunc: func(t *testing.T) {
+				const (
+					snapshotName = "test-snapshot"
+					nameTagValue = "test-name-tag-value"
+					clusterId    = "test-cluster-id"
+				)
+
+				req := &csi.CreateSnapshotRequest{
+					Name: snapshotName,
+					Parameters: map[string]string{
+						"tagSpecification_1": NameTag + "=" + nameTagValue,
+					},
+					SourceVolumeId: "vol-test",
+				}
+				expSnapshot := &csi.Snapshot{
+					ReadyToUse: true,
+				}
+
+				ctx := context.Background()
+				mockSnapshot := &cloud.Snapshot{
+					SnapshotID:     fmt.Sprintf("snapshot-%d", rand.New(rand.NewSource(time.Now().UnixNano())).Uint64()),
+					SourceVolumeID: req.SourceVolumeId,
+					Size:           1,
+					CreationTime:   time.Now(),
+				}
+				mockCtl := gomock.NewController(t)
+				defer mockCtl.Finish()
+
+				snapshotOptions := &cloud.SnapshotOptions{
+					Tags: map[string]string{
+						cloud.SnapshotNameTagKey:               snapshotName,
+						cloud.AwsEbsDriverTagKey:               isManagedByDriver,
+						NameTag:                                nameTagValue,
+						ResourceLifecycleTagPrefix + clusterId: ResourceLifecycleOwned,
+					},
+				}
+
+				mockCloud := cloud.NewMockCloud(mockCtl)
+				mockCloud.EXPECT().CreateSnapshot(gomock.Eq(ctx), gomock.Eq(req.SourceVolumeId), gomock.Eq(snapshotOptions)).Return(mockSnapshot, nil)
+				mockCloud.EXPECT().GetSnapshotByName(gomock.Eq(ctx), gomock.Eq(req.GetName())).Return(nil, cloud.ErrNotFound)
+
+				awsDriver := controllerService{
+					cloud:         mockCloud,
+					inFlight:      internal.NewInFlight(),
+					driverOptions: &DriverOptions{kubernetesClusterID: clusterId},
+				}
+				resp, err := awsDriver.CreateSnapshot(context.Background(), req)
+				if err != nil {
+					t.Fatalf("Unexpected error: %v", err)
+				}
+
 				if snap := resp.GetSnapshot(); snap == nil {
 					t.Fatalf("Expected snapshot %v, got nil", expSnapshot)
 				}