Adding Feature To Modify Tags on Existing Volumes Through VolumeAttri…

…butesClass (#2082)

docs/example-iam-policy.json

@@ -25,15 +25,7 @@
       "Resource": [
         "arn:aws:ec2:*:*:volume/*",
         "arn:aws:ec2:*:*:snapshot/*"
-      ],
-      "Condition": {
-        "StringEquals": {
-          "ec2:CreateAction": [
-            "CreateVolume",
-            "CreateSnapshot"
-          ]
-        }
-      }
+      ]
     },
     {
       "Effect": "Allow",

docs/tagging.md

@@ -96,6 +96,36 @@ backup=true
 billingID=ABCDEF
 ```
 
+# Adding, Modifying, and Deleting Tags Of Existing Volumes
+The AWS EBS CSI Driver supports the modifying of tags of existing volumes through `VolumeAttributesClass.parameters` the examples below show the syntax for addition, modification, and deletion of tags within the `VolumeAttributesClass.parameters`. For a walkthrough on how to apply these modifications to a volume follow the [walkthrough for Volume Modification via VolumeAttributeClass](../examples/kubernetes/modify-volume)
+
+**Syntax for Adding or Modifying a Tag**
+
+If a key has the prefix `tagSpecification`, the CSI driver will treat the value as a key-value pair to be added to the existing volume. If there is already an existing tag with the specified key, the CSI driver will overwrite the value of that tag with the new value specified. 
+```
+apiVersion: storage.k8s.io/v1alpha1
+kind: VolumeAttributesClass
+metadata:
+  name: io2-class
+driverName: ebs.csi.aws.com
+parameters:
+  tagSpecification_1: "location=Seattle"
+  tagSpecification_2: "cost-center=" // If the value is left blank, tag is created with an empty value
+```
+**Syntax for Deleting a Tag**
+
+If a key has the prefix `tagDeletion`, the CSI driver will treat the value as a tag key, and the existing tag with that key will be removed from the volume.
+```
+apiVersion: storage.k8s.io/v1alpha1
+kind: VolumeAttributesClass
+metadata:
+  name: io2-class
+driverName: ebs.csi.aws.com
+parameters:
+  tagDeletion_1: "location" // Deletes tag with key "location"
+  tagDeletion_2: "cost-center"
+```
+
 # Snapshot Tagging
 The AWS EBS CSI Driver supports tagging snapshots through `VolumeSnapshotClass.parameters`, similarly to StorageClass tagging.
 

examples/kubernetes/modify-volume/README.md

@@ -35,21 +35,25 @@ This example will only work on a cluster with the `VolumeAttributesClass` featur
     Mon Feb 26 22:28:39 UTC 2024
     ...
     ```
+4. Deploy the `VolumeAttributesClass`
+    ```sh
+    $ kubectl apply -f manifests/volumeattributesclass.yaml
+    ```
 
-4. Simultaneously, deploy the `VolumeAttributesClass` and edit the `PersistentVolumeClaim` to point to this class
+5. Edit the `PersistentVolumeClaim` to point to this class
     ```sh
     $ kubectl patch pvc ebs-claim --patch '{"spec": {"volumeAttributesClassName": "io2-class"}}'
     persistentvolumeclaim/ebs-claim patched
     ```
 
-5. Wait for the `VolumeAttributesClass` to apply to the volume
+6. Wait for the `VolumeAttributesClass` to apply to the volume
     ```sh
     $ kubectl get pvc ebs-claim
     NAME        STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   VOLUMEATTRIBUTESCLASS   AGE
     ebs-claim   Bound    pvc-076b2d14-b643-47d4-a2ce-fbf9cd36572b   100Gi      RWO            ebs-sc         io2-class               5m54s
     ```
 
-6. (Optional) Delete example resources
+7. (Optional) Delete example resources
     ```sh
     $ kubectl delete -f manifests 
     storageclass.storage.k8s.io "ebs-sc" deleted

examples/kubernetes/modify-volume/manifests/volumeattributesclass.yaml

@@ -21,3 +21,5 @@ driverName: ebs.csi.aws.com
 parameters:
   type: io2
   iops: "10000"
+  tagSpecification_1: "location=Seattle"
+  tagSpecification_2: "cost-center="

hack/e2e/kops/patch-cluster.yaml

@@ -49,15 +49,7 @@ spec:
           "Resource": [
             "arn:aws:ec2:*:*:volume/*",
             "arn:aws:ec2:*:*:snapshot/*"
-          ],
-          "Condition": {
-            "StringEquals": {
-              "ec2:CreateAction": [
-                "CreateVolume",
-                "CreateSnapshot"
-              ]
-            }
-          }
+          ]
         },
         {
           "Effect": "Allow",

pkg/cloud/cloud.go

@@ -229,6 +229,12 @@ type ModifyDiskOptions struct {
 	Throughput int32
 }
 
+// ModifyTagsOptions represents parameter to modify the tags of an existing EBS volume
+type ModifyTagsOptions struct {
+	TagsToAdd    map[string]string
+	TagsToDelete []string
+}
+
 // Snapshot represents an EBS volume snapshot
 type Snapshot struct {
 	SnapshotID     string
@@ -746,6 +752,42 @@ func (c *cloud) batchDescribeVolumesModifications(request *ec2.DescribeVolumesMo
 	return r.Result, nil
 }
 
+// ModifyTags adds, updates, and deletes tags for the specified EBS volume.
+func (c *cloud) ModifyTags(ctx context.Context, volumeID string, tagOptions ModifyTagsOptions) error {
+	if len(tagOptions.TagsToDelete) > 0 {
+		deleteTagsInput := &ec2.DeleteTagsInput{
+			Resources: []string{volumeID},
+			Tags:      make([]types.Tag, 0, len(tagOptions.TagsToDelete)),
+		}
+		for _, tagKey := range tagOptions.TagsToDelete {
+			deleteTagsInput.Tags = append(deleteTagsInput.Tags, types.Tag{Key: aws.String(tagKey)})
+		}
+		_, deleteErr := c.ec2.DeleteTags(ctx, deleteTagsInput)
+		if deleteErr != nil {
+			klog.ErrorS(deleteErr, "failed to delete tags", "volumeID", volumeID)
+			return deleteErr
+		}
+	}
+	if len(tagOptions.TagsToAdd) > 0 {
+		createTagsInput := &ec2.CreateTagsInput{
+			Resources: []string{volumeID},
+			Tags:      make([]types.Tag, 0, len(tagOptions.TagsToAdd)),
+		}
+		for k, v := range tagOptions.TagsToAdd {
+			createTagsInput.Tags = append(createTagsInput.Tags, types.Tag{
+				Key:   aws.String(k),
+				Value: aws.String(v),
+			})
+		}
+		_, addErr := c.ec2.CreateTags(ctx, createTagsInput)
+		if addErr != nil {
+			klog.ErrorS(addErr, "failed to create tags", "volumeID", volumeID)
+			return addErr
+		}
+	}
+	return nil
+}
+
 // ResizeOrModifyDisk resizes an EBS volume in GiB increments, rounding up to the next possible allocatable unit, and/or modifies an EBS
 // volume with the parameters in ModifyDiskOptions.
 // The resizing operation is performed only when newSizeBytes != 0.

pkg/cloud/cloud_test.go

@@ -2509,6 +2509,118 @@ func TestResizeOrModifyDisk(t *testing.T) {
 	}
 }
 
+func TestModifyTags(t *testing.T) {
+	validTagsToAddInput := map[string]string{
+		"key1": "value1",
+		"key2": "value2",
+		"key3": "",
+	}
+
+	validTagsToDeleteInput := []string{
+		"key1",
+		"key2",
+	}
+
+	emptyTagsToAddInput := map[string]string{}
+	emptyTagsToDeleteInput := []string{}
+
+	testCases := []struct {
+		name              string
+		volumeID          string
+		negativeCase      bool
+		modifyTagsOptions ModifyTagsOptions
+		expErr            error
+	}{
+		{
+			name:     "success normal tag addition",
+			volumeID: "mod-tag-test-name",
+			modifyTagsOptions: ModifyTagsOptions{
+				TagsToAdd: validTagsToAddInput,
+			},
+			expErr: nil,
+		},
+		{
+			name:     "success normal tag deletion",
+			volumeID: "mod-tag-test-name",
+			modifyTagsOptions: ModifyTagsOptions{
+				TagsToDelete: validTagsToDeleteInput,
+			},
+			expErr: nil,
+		},
+		{
+			name:     "success normal tag addition and tag deletion",
+			volumeID: "mod-tag-test-name",
+			modifyTagsOptions: ModifyTagsOptions{
+				TagsToAdd:    validTagsToAddInput,
+				TagsToDelete: validTagsToDeleteInput,
+			},
+			expErr: nil,
+		},
+		{
+			name:         "fail: EC2 API generic error TagsToAdd",
+			volumeID:     "mod-tag-test-name",
+			negativeCase: true,
+			expErr:       fmt.Errorf("Generic EC2 API error"),
+			modifyTagsOptions: ModifyTagsOptions{
+				TagsToAdd:    validTagsToAddInput,
+				TagsToDelete: emptyTagsToDeleteInput,
+			},
+		},
+		{
+			name:         "fail: EC2 API generic error TagsToDelete",
+			volumeID:     "mod-tag-test-name",
+			negativeCase: true,
+			expErr:       fmt.Errorf("Generic EC2 API error"),
+			modifyTagsOptions: ModifyTagsOptions{
+				TagsToAdd:    emptyTagsToAddInput,
+				TagsToDelete: validTagsToDeleteInput,
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			mockEC2 := NewMockEC2API(mockCtrl)
+			c := newCloud(mockEC2)
+
+			ctx := context.Background()
+
+			if len(tc.modifyTagsOptions.TagsToAdd) > 0 {
+				if tc.negativeCase {
+					mockEC2.EXPECT().CreateTags(gomock.Any(), gomock.Any()).Return(nil, tc.expErr).Times(1)
+				} else {
+					mockEC2.EXPECT().CreateTags(gomock.Any(), gomock.Any()).Return(&ec2.CreateTagsOutput{}, tc.expErr).Times(1)
+				}
+			}
+			if len(tc.modifyTagsOptions.TagsToDelete) > 0 {
+				if tc.negativeCase {
+					mockEC2.EXPECT().DeleteTags(gomock.Any(), gomock.Any()).Return(nil, tc.expErr).Times(1)
+				} else {
+					mockEC2.EXPECT().DeleteTags(gomock.Any(), gomock.Any()).Return(&ec2.DeleteTagsOutput{}, tc.expErr).Times(1)
+				}
+			}
+
+			err := c.ModifyTags(ctx, tc.volumeID, tc.modifyTagsOptions)
+			if err != nil {
+				if tc.expErr == nil {
+					t.Fatalf("ModifyTags() failed: expected no error, got: %v", err)
+				} else {
+					if !strings.Contains(err.Error(), tc.expErr.Error()) {
+						t.Fatalf("ModifyTags() failed: expected error %v, got: %v", tc.expErr, err)
+					}
+				}
+			} else {
+				if tc.expErr != nil {
+					t.Fatal("ModifyTags() failed: expected error, got nothing")
+				}
+			}
+
+			mockCtrl.Finish()
+		})
+	}
+}
+
 func TestGetSnapshotByName(t *testing.T) {
 	testCases := []struct {
 		name            string

pkg/cloud/ec2_interface.go

@@ -35,5 +35,6 @@ type EC2API interface {
 	DescribeVolumesModifications(ctx context.Context, params *ec2.DescribeVolumesModificationsInput, optFns ...func(*ec2.Options)) (*ec2.DescribeVolumesModificationsOutput, error)
 	DescribeTags(ctx context.Context, params *ec2.DescribeTagsInput, optFns ...func(*ec2.Options)) (*ec2.DescribeTagsOutput, error)
 	CreateTags(ctx context.Context, params *ec2.CreateTagsInput, optFns ...func(*ec2.Options)) (*ec2.CreateTagsOutput, error)
+	DeleteTags(ctx context.Context, params *ec2.DeleteTagsInput, optFns ...func(*ec2.Options)) (*ec2.DeleteTagsOutput, error)
 	EnableFastSnapshotRestores(ctx context.Context, params *ec2.EnableFastSnapshotRestoresInput, optFns ...func(*ec2.Options)) (*ec2.EnableFastSnapshotRestoresOutput, error)
 }

pkg/cloud/interface.go

@@ -26,6 +26,7 @@ type Cloud interface {
 	DeleteDisk(ctx context.Context, volumeID string) (success bool, err error)
 	AttachDisk(ctx context.Context, volumeID string, nodeID string) (devicePath string, err error)
 	DetachDisk(ctx context.Context, volumeID string, nodeID string) (err error)
+	ModifyTags(ctx context.Context, volumeID string, tagOptions ModifyTagsOptions) (err error)
 	ResizeOrModifyDisk(ctx context.Context, volumeID string, newSizeBytes int64, options *ModifyDiskOptions) (newSize int32, err error)
 	WaitForAttachmentState(ctx context.Context, volumeID, expectedState string, expectedInstance string, expectedDevice string, alreadyAssigned bool) (*types.VolumeAttachment, error)
 	GetDiskByName(ctx context.Context, name string, capacityBytes int64) (disk *Disk, err error)

pkg/driver/controller.go

@@ -217,14 +217,14 @@ func (d *ControllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 
 	// "Values specified in mutable_parameters MUST take precedence over the values from parameters."
 	// https://github.com/container-storage-interface/spec/blob/master/spec.md#createvolume
-	if modifyOptions.VolumeType != "" {
-		volumeType = modifyOptions.VolumeType
+	if modifyOptions.modifyDiskOptions.VolumeType != "" {
+		volumeType = modifyOptions.modifyDiskOptions.VolumeType
 	}
-	if modifyOptions.IOPS != 0 {
-		iops = modifyOptions.IOPS
+	if modifyOptions.modifyDiskOptions.IOPS != 0 {
+		iops = modifyOptions.modifyDiskOptions.IOPS
 	}
-	if modifyOptions.Throughput != 0 {
-		throughput = modifyOptions.Throughput
+	if modifyOptions.modifyDiskOptions.Throughput != 0 {
+		throughput = modifyOptions.modifyDiskOptions.Throughput
 	}
 
 	responseCtx := map[string]string{}
@@ -592,7 +592,8 @@ func (d *ControllerService) ControllerModifyVolume(ctx context.Context, req *csi
 	}
 
 	_, err = d.modifyVolumeCoalescer.Coalesce(volumeID, modifyVolumeRequest{
-		modifyDiskOptions: *options,
+		modifyDiskOptions: options.modifyDiskOptions,
+		modifyTagsOptions: options.modifyTagsOptions,
 	})
 	if err != nil {
 		return nil, err