Add the ability to provide static EBS volume tags

[leakingtapan]

Is your feature request related to a problem? Please describe.

@leakingtapan this is our issue: we need to make sure all volumes dynamic provisioned are tagged with a cluster uuid so that we can safely delete the volume during cluster teardown. For that particular use case, we don't necessarily need to dynamically tag the volumes given some user input.

Describe the solution you'd like in detail
Add a config option (eg. cmd flag) to controller service, so that it'll always attach some tags to all the dynamically provisioned EBS volumes. The default can be empty so that it matches the current behavior.

Describe alternatives you've considered
We can either implement this as a standalone feature or let this feature depend on #180

/cc @jieyu

[jieyu]

@leakingtapan let us know how to proceed. Happy to submit PR for the feature.

[leakingtapan]

Since I see there are mutiple ways to implement this, could you create an issue with a high level design to begin with. Would happy to review it.

@jieyu

[jieyu]

@leakingtapan The proposal is to add a new cli command option --extra-volume-tags of type mapStringString. By default, it's an empty map which maps the the current behavior.

When doing dynamic provisioning (i.e., in CreateVolume), we always attach the extra volume tags when calling d.cloud.CreateDisk.

[leakingtapan]

@jieyu Several questions for this approach:

• What will happen if reserved tags are used eg. kubernetes.io/cluster or CSIVolumeName?
• How many key/value pairs are you looking for pass in? Since AWS tag has a limit of 50 tags per resource.
• What's the maximum length for key and value you need to tag?
• Are we looking for tagging volume only or snapshot as well? Given the problem you describe, it makes sense to tag snapshot as well for the sake of cluster tear down clean up.
• Do you only care about dynamic provisioned volume or pre-provisioned volume as well?

Back to your use case, do you only care about tagging volume (or snapshot) with cluster ID? Or you want volumes(or snapshots) be tagged with arbitrary static key/value pairs that are passed in through the --extra-volume-tags

[jieyu]

@leakingtapan thanks for the feedback! those are good questions. Will definitely address those and add validation.

Back to your use case, do you only care about tagging volume (or snapshot) with cluster ID? Or you want volumes(or snapshots) be tagged with arbitrary static key/value pairs that are passed in through the --extra-volume-tags

We care about more than just cluster ID. For instance, we deploy some kind of cloud cleaner to periodically cleanup resources that are expired unless you add a special tag expiration to prevent cloud cleaner from cleaning up the volume.

Are we looking for tagging volume only or snapshot as well?

Good question. Yes, allowing adding tags to snapshot will be nice! We could add another flag --extra-snapshot-tags.

[jieyu]

@leakingtapan does the above sound good to you? If yes, I can start working on it.

[leakingtapan]

Sounds good to me. Will be happy to review the code.

[leakingtapan]

BTW, are you looking for using this feature in the context of k8s or other CO?

[jieyu]

@leakingtapan currently, only in context of k8s

[leakingtapan]

@jieyu I spent some time on the design of arbitrary tag #351. I feel both solve your problem. Let me know which one fits better for you. I'm totally fine with your this proposal, just feel the other one might be more useful/general if you are just using k8s

[jieyu]

@leakingtapan thanks for the pointer and the design! I looked at the design. Unfortunately it does not solve our use case. We have two personas:

1. cluster admin, who sets up the cluster (e.g., storage class)
2. developers, who creates PVC.

The proposal in #351 requires developers to annotate PVC. This is undesirable in our use case because they might just forgot to annotate their PVCs. Since cluster admin manages the lifecycle of the cluster, they are responsible for cleaning up all AWS resources associated with the cluster. And they cannot rely on developers to be cooperative. We must provide a mechanism for cluster admin to properly cleanup EBS volumes irrespective of what the developers do.

[leakingtapan]

makes sense. Thx for clarification !

[leakingtapan]

/assign @jieyu

[infa-ssaurabh]

Where to add the cli arg: --extra-volume-tags
In my ebs-csi-controller deployment, aws-ebs-csi-driver doesn't have argument like --extra-volume-tags

flag provided but not defined: -extra-volume-tags
Usage of /bin/aws-ebs-csi-driver:
-add_dir_header
If true, adds the file directory to the header
-alsologtostderr
log to standard error as well as files
-endpoint string
CSI Endpoint (default "unix://tmp/csi.sock")
-log_backtrace_at value
when logging hits line file:N, emit a stack trace
-log_dir string
If non-empty, write log files in this directory
-log_file string
If non-empty, use this log file
-log_file_max_size uint
Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)
-logtostderr
log to standard error instead of files (default true)
-skip_headers
If true, avoid header prefixes in the log messages
-skip_log_headers
If true, avoid headers when opening log files
-stderrthreshold value
logs at or above this threshold go to stderr (default 2)
-v value
number for the log level verbosity
-version
Print the version and exit.
-vmodule value
comma-separated list of pattern=N settings for file-filtered logging

[leakingtapan]

What’s the driver version you are using?

[infa-ssaurabh]

aws-ebs-csi-driver:v0.4.0

[leakingtapan]

This feature is only available in master branch or latest image tag. We have yet to cut a release

[infa-ssaurabh]

I don't see that feature even with latest tag(i took it from dev), with alpha its completely different:
flag provided but not defined: -extra-volume-tags
Usage of /bin/aws-ebs-csi-driver:
-add_dir_header
If true, adds the file directory to the header
-alsologtostderr
log to standard error as well as files
-endpoint string
CSI Endpoint (default "unix://tmp/csi.sock")
-log_backtrace_at value
when logging hits line file:N, emit a stack trace
-log_dir string
If non-empty, write log files in this directory
-log_file string
If non-empty, use this log file
-log_file_max_size uint
Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)
-logtostderr
log to standard error instead of files (default true)
-skip_headers
If true, avoid header prefixes in the log messages
-skip_log_headers
If true, avoid headers when opening log files
-stderrthreshold value
logs at or above this threshold go to stderr (default 2)
-v value
number for the log level verbosity
-version
Print the version and exit.
-vmodule value
comma-separated list of pattern=N settings for file-filtered logging

[jieyu]

@infa-ssaurabh The feature is merged in master branch, if you build the controller from master, you should be able to see those flags. We've tested internally and worked. @leakingtapan when we can expect the next release?