Adding Proper Handling of VAC and Controller Tags On Volume Creation

[mdzraf]

What type of PR is this?

kind feature

What is this PR about? / Why do we need it?

This PR adds proper handling of volumeAttributesClass tags so that the tags specified in the VAC are templated, validated, and added to the volume on volume creation. It also handles the templating and validation of any --extra-tags on the controller on volume creation.

Resolves #2442
Resolves #2439

How was this change tested?

1. Ran make test

> make test 
Ran 76 of 92 Specs in 0.442 seconds
SUCCESS! -- 76 Passed | 0 Failed | 1 Pending | 15 Skipped

2. Used/deployed the following resource configurations and then ran aws ec2 describe-volumes --volume-ids vol-<ID fo provisioned volume> to make sure that the tags specified in the VAC were added and properly interpolated.

---
apiVersion: storage.k8s.io/v1beta1
kind: VolumeAttributesClass
metadata:
  name: io2-class
driverName: ebs.csi.aws.com
parameters:
  type: io2
  iops: "3000"
  tagSpecification_1: "pvcnamespaceTEST={{ .PVCNamespace }}"
  tagSpecification_2: "pvcnameTEST={{ .PVCName }}"
  tagSpecification_3: "pvnameTEST={{ .PVName }}"
  tagSpecification_4: "location=Seattle"
  tagSpecification_5: "cost-center="
  tagSpecification_6: 'backup={{ .PVCNamespace | contains "prod" }}'
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: ebs-claim
spec:
  volumeAttributesClassName: io2-class
  accessModes:
    - ReadWriteOnce
  storageClassName: ebs-sc
  resources:
    requests:
      storage: 4Gi
 ---
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: ebs-sc
provisioner: ebs.csi.aws.com
volumeBindingMode: WaitForFirstConsumer
---
apiVersion: v1
kind: Pod
metadata:
  name: app
spec:
  containers:
  - name: app
    image: public.ecr.aws/amazonlinux/amazonlinux
    command: ["/bin/sh"]
    args: ["-c", "while true; do echo $(date -u) >> /data/out.txt; sleep 5; done"]
    volumeMounts:
    - name: persistent-storage
      mountPath: /data
  volumes:
  - name: persistent-storage
    persistentVolumeClaim:
      claimName: ebs-claim

Result of describe call

{
            "Iops": 2000,
            "Tags": [
                {
                    "Key": "Name",
                    "Value": "ebs-csi-e2e.k8s.local-dynamic-pvc-cffd7dc2-3d86-49a2-b787-e64861fc1f7c"
                },
                {
                    "Key": "cost-center",
                    "Value": ""
                },
                {
                    "Key": "ebs.csi.aws.com/cluster",
                    "Value": "true"
                },
                {
                    "Key": "CSIVolumeName",
                    "Value": "pvc-cffd7dc2-3d86-49a2-b787-e64861fc1f7c"
                },
                {
                    "Key": "extrakey",
                    "Value": "extraval"
                },
                {
                    "Key": "backup",
                    "Value": "false"
                },
                {
                    "Key": "kubernetes.io/cluster/ebs-csi-e2e.k8s.local",
                    "Value": "owned"
                },
                {
                    "Key": "kubernetes.io/created-for/pvc/name",
                    "Value": "ebs-claim"
                },
                {
                    "Key": "KubernetesCluster",
                    "Value": "ebs-csi-e2e.k8s.local"
                },
                {
                    "Key": "location",
                    "Value": "Seattle"
                },
                {
                    "Key": "kubernetes.io/created-for/pvc/namespace",
                    "Value": "default"
                },
                {
                    "Key": "pvcnamespaceTEST",
                    "Value": "default"
                },
                {
                    "Key": "VolumeNamespace",
                    "Value": "default"
                },
                {
                    "Key": "pvcnameTEST",
                    "Value": "ebs-claim"
                },
                {
                    "Key": "pvnameTEST",
                    "Value": "pvc-cffd7dc2-3d86-49a2-b787-e64861fc1f7c"
                },
                {
                    "Key": "kubernetes.io/created-for/pv/name",
                    "Value": "pvc-cffd7dc2-3d86-49a2-b787-e64861fc1f7c"
                }
            ],
            "VolumeType": "io2"...

Does this PR introduce a user-facing change?

Added Proper Handling To Ensure Tags Added in a VAC Which a PVC Points To Get Added to the Provisioned Volume. Also Added Proper Handling To Ensure that extra-tags on the Controller Also Get Added to the Provisioned Volume.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign gnufied for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @mdzraf. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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 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.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[ElijahQuinones]

/ok-to-test

[AndrewSirenko]

I'm a bit worried at the amount code duplication we have for validating parameters that can be specified in either the SC versus mutable_parameters from VAC. Can we consolidate the logic?

Before this PR there are 2 separate functions that validate parameters shared between SC and VAC like IOPS:

• controller.go:

  aws-ebs-csi-driver/pkg/driver/controller.go

  Lines 147 to 152 in 6c1235d

  	case IopsKey:
  	parseIopsKey, parseIopsKeyErr := strconv.ParseInt(value, 10, 32)
  	if parseIopsKeyErr != nil {
  	return nil, status.Errorf(codes.InvalidArgument, "Could not parse invalid iops: %v", err)
  	}
  	iops = int32(parseIopsKey)

• controller_modify_volume.go:

  aws-ebs-csi-driver/pkg/driver/controller_modify_volume.go

  Lines 178 to 183 in 8814261

  	case ModificationKeyIOPS:
  	iops, err := strconv.ParseInt(value, 10, 32)
  	if err != nil {
  	return nil, status.Errorf(codes.InvalidArgument, "Could not parse IOPS: %q", value)
  	}
  	options.modifyDiskOptions.IOPS = int32(iops)

This PR adds a third place we copy paste same exact code. (Which means there are three separate places to forget about when dealing with new parameters)

---

One possible solution for the create path would be to overwrite SC parameters with values from VAC mutable_parameters BEFORE validating them. They share the same parameter key anyway, and can be validated within the existing loop + switch statement.

This is in line with CSI Spec:

Values specified in mutable_parameters MUST take precedence over the values from parameters.

Alternatively we can refactor the switch statement + for loop into a shared validate function used in Create and Modify paths.

[AndrewSirenko]

Is there a reason why VAC and 'extra' tags being evaluated and validated separately from SCTags? (But lines 402-409 are duplicate of 308-315?)

A smell here is that tags specified via extra-tags option would wind up in the unrelated struct modifyOptions.modifyTagOptions.TagsToAdd.

---

Have you considered instead:

1. Collating all validated scTags, extraTags, and vacTags into an array called tagsToEvaluate
2. Put them through template.Evaluate and validateExtraTags
3. Append them to list of volumeTags that get passed in to cloud.DiskOptions

That way all tags, no matter their source, go through a clear set of steps? Bonus points if these steps are located near each other so it's easy to see how all tags are processed.

[AndrewSirenko]

General comment: Should we add a unit test that ensures all tags get evaluated for string interpolation to prevent regressions? (SCTags, VACTags, and Extra tags can be tested in the same unit test)