Repository: kubernetes-sigs/aws-ebs-csi-driver
Pull request #1548 (2023-03-28 14:31:17)

Fix VolumeSnapshotClass tagging
**Is this a bug fix or adding new feature?**
Bug fix for #1545 

**What is this PR about? / Why do we need it?**
When users set "Name" tag for a snapshot via `VolumeSnapshotClass.parameters`, the value of the Name tag is overwritten by [the value automatically populated by the Driver](https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/5d75bf4be309d3b554d3959a79cc8f05ed65d4d2/pkg/driver/controller.go#L632), which is unexpected behavior.
This PR fixes the bug by setting the tags from VolumeSnapshotClass.parameters _after_ the Driver automatically populates the Name tag.

**What testing is done?** 
Added unit test & 
1. Built and installed the driver that has the changes. 
2. Followed the repro steps in #1545 to create a snapshot with "Name" tag in ``VolumeSnapshotClass.parameters``.
3.  Confirmed the name of the snapshot and the tags of the snapshot in the EC2 console are the values from ``VolumeSnapshotClass.parameters``.


k8s-ci-robot: Hi @hanyuel. Thanks for your PR.

I'm waiting for a [kubernetes-sigs](https://github.com/orgs/kubernetes-sigs/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://git.k8s.io/community/community-membership.md#member) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Faws-ebs-csi-driver).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

rdpsin: /lgtm
k8s-ci-robot: New changes are detected. LGTM label has been removed.
ConnorJC3: /ok-to-test
rdpsin: /lgtm

Patch:
diff --git a/README.md b/README.md
index 9f9f8735..b3d8114d 100644
--- a/README.md
+++ b/README.md
@@ -15,12 +15,12 @@ The [Amazon Elastic Block Store](https://aws.amazon.com/ebs/) Container Storage
 
 ## Features
 * **Static Provisioning** - Associate an externally-created EBS volume with a [PersistentVolume](https://kubernetes.io/docs/concepts/storage/persistent-volumes/) (PV) for consumption within Kubernetes.
-* **Dynamic Provisioning** - Automatically create EBS volumes and associated [PersistentVolumes](https://kubernetes.io/docs/concepts/storage/persistent-volumes/) (PV) from [PersistentVolumeClaims](https://kubernetes.io/docs/concepts/storage/persistent-volumes/#dynamic)) (PVC). Parameters can be passed via a [StorageClass](https://kubernetes.io/docs/concepts/storage/storage-classes/#the-storageclass-resource) for fine-grained control over volume creation.
+* **Dynamic Provisioning** - Automatically create EBS volumes and associated [PersistentVolumes](https://kubernetes.io/docs/concepts/storage/persistent-volumes/) (PV) from [PersistentVolumeClaims](https://kubernetes.io/docs/concepts/storage/persistent-volumes/#PersistentVolumeClaim:~:text=PersistentVolumeClaim%20(PVC)) (PVC). Parameters can be passed via a [StorageClass](https://kubernetes.io/docs/concepts/storage/storage-classes/#the-storageclass-resource) for fine-grained control over volume creation.
 * **Mount Options** - Mount options could be specified in the [PersistentVolume](https://kubernetes.io/docs/concepts/storage/persistent-volumes/) (PV) resource to define how the volume should be mounted.
 * **NVMe Volumes** - Consume [NVMe](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/nvme-ebs-volumes.html) volumes from EC2 [Nitro instances](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-types.html#ec2-nitro-instances).
 * **Block Volumes** - Consume an EBS volume as a [raw block device](https://kubernetes-csi.github.io/docs/raw-block.html).
 * **Volume Snapshots** - Create and restore [snapshots](https://kubernetes.io/docs/concepts/storage/volume-snapshots/) taken from a volume in Kubernetes.
-* **Volume Resizing** - Expand the volume (both offline and online) by specifying a new size in the [PersistentVolumeClaim](https://kubernetes.io/docs/concepts/storage/persistent-volumes/#expanding-persistent-volumes-claims) (PVC).
+* **Volume Resizing** - Expand the volume size by specifying a new size in the [PersistentVolumeClaim](https://kubernetes.io/docs/concepts/storage/persistent-volumes/#PersistentVolumeClaim:~:text=PersistentVolumeClaim%20(PVC)) (PVC).
 
 ## Container Images
 
diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go
index f1bd3458..aa6093ee 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,
 	}


Test patch:
diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go
index db04d632..de957e92 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)
 				}