Apply extra volume tags to EBS snapshots #568

chrishenzie · 2020-09-24T19:03:00Z

Is this a bug fix or adding new feature?

Adding a new feature, see #566

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

This PR takes the --extra-volume-tags that are applied to provisioned EBS volumes in CreateVolume(), and also adds them provisioned snapshots via CreateSnapshot().

What testing is done?

Included a unit test that asserts the snapshotOptions received by cloud.CreateSnapshot() includes the supplied
extraVolumeTags
Included a unit test that asserts the snapshotOptions received by cloud.CreateSnapshot() includes the cluster ID if that driver option is enabled (See PR feedback)
Included a unit test that asserts the request object sent to the AWS API is correct
- This test caught a bug where we were passing a pointer to a loop variable which resulted in duplicate tags being passed to the API
Performed manual end to end testing in an AWS cluster, created a VolumeSnapshot resource and saw the provisioned snapshot have the additional tags applied to it

k8s-ci-robot · 2020-09-24T19:03:08Z

Welcome @chrishenzie!

It looks like this is your first PR to kubernetes-sigs/aws-ebs-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-ebs-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-09-24T19:03:08Z

Hi @chrishenzie. 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/test-infra repository.

coveralls · 2020-09-24T19:06:11Z

Pull Request Test Coverage Report for Build 1261

36 of 37 (97.3%) changed or added relevant lines in 4 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.1%) to 81.253%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/driver/validation.go	11	12	91.67%

Files with Coverage Reduction	New Missed Lines	%
pkg/driver/validation.go	1	94.59%

Totals
Change from base Build 1260:	0.1%
Covered Lines:	1595
Relevant Lines:	1963

💛 - Coveralls

bertinatto · 2020-09-25T07:23:57Z

/ok-to-test

msau42

generally lgtm
/assign @bertinatto @wongma7
especially regarding reusing the "extra-volume-tags" arg.

msau42 · 2020-09-28T20:43:58Z

pkg/driver/controller.go

@@ -440,6 +440,9 @@ func (d *controllerService) CreateSnapshot(ctx context.Context, req *csi.CreateS
 	opts := &cloud.SnapshotOptions{
 		Tags: map[string]string{cloud.SnapshotNameTagKey: snapshotName},
 	}
+	for k, v := range d.driverOptions.extraVolumeTags {


Do you also want to add cluster id tag, similar to how it's done for CreateVolume?

Added this and an additional unit test to cover this case.

bertinatto

Overal LGTM, just 2 nits.

I'd like to propose we mark the extra-volume-tags option as Deprecated and make it an alias to a new option with a more representative name. Later on we can remove extra-volume-tags and keep only the new one.

bertinatto · 2020-09-29T11:21:41Z

pkg/cloud/cloud.go

+	// Enforce ordering for unit tests.
+	sort.SliceStable(tags, func(i, j int) bool {
+		return *tags[i].Key < *tags[j].Key
+	})


Is this sorting really necessary? If so, can we potentially move it to the test function?

Removed in favor of a custom matcher for this AWS input type. Let me know if this is what you were thinking.

pkg/driver/controller.go

chrishenzie · 2020-09-30T20:25:06Z

@bertinatto One concern I have with this general --extra-tags approach is that it's a little less flexible for customers.

Suppose customers want different tags for snapshots than they do volumes. With this solution, they will need to combine their tags for snapshots and volumes into one string, which could potentially exceed the limit of the max # of tags.

Since this driver only provisions either EBS volumes or snapshots, it seems like another alternative would be to introduce --extra-snapshot-tags and use that for just snapshots. If users want the same tags applied to both, they can provide the same value for both flags.

pkg/driver/controller.go

bertinatto · 2020-10-01T09:03:11Z

cmd/options/controller_options.go

 	// ID of the kubernetes cluster. This is used only to create the same tags on volumes that
 	// in-tree volume volume plugin does.
 	KubernetesClusterID string
 }

 func (s *ControllerOptions) AddFlags(fs *flag.FlagSet) {
-	fs.Var(cliflag.NewMapStringString(&s.ExtraVolumeTags), "extra-volume-tags", "Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")
+	fs.Var(cliflag.NewMapStringString(&s.ExtraTags), "extra-tags", "Extra tags to attach to each dynamically provisioned resource. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")
+	fs.Var(cliflag.NewMapStringString(&s.ExtraTags), "extra-volume-tags", "DEPRECATED: Please use --extra-tags instead. Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")


If both flags are set, is it possible that the deprecated one will overwrite the other? Is there a way we can give extra-tags priority?

Perhaps we can:

Keep ExtraVolumeTags above

Keep the driver.WithExtraVolumeTags line in cmd/main.go

Keep the WithExtraVolumeTags in driver.go and change it to only set o.extraTags if it hasn't been set yet.

Does that makes sense?

The other changes LGTM.

Yeah that makes sense. Let me know if this captures it.

bertinatto · 2020-10-01T09:27:06Z

@bertinatto One concern I have with this general --extra-tags approach is that it's a little less flexible for customers.

Suppose customers want different tags for snapshots than they do volumes. With this solution, they will need to combine their tags for snapshots and volumes into one string, which could potentially exceed the limit of the max # of tags.

Since this driver only provisions either EBS volumes or snapshots, it seems like another alternative would be to introduce --extra-snapshot-tags and use that for just snapshots. If users want the same tags applied to both, they can provide the same value for both flags.

Sorry, I missed this comment. That's a valid point, but I'm not sure if we need this flexibility at the moment (I don't have any use cases in mind). To me it feels like using a single flag is OK at this point, but we can always split it if we come across a strong use case.

bertinatto

Just 2 nits regarding the deprecated flag. Other than that, I think we can merge this!

I'm not sure how many people out there is using this flag, so IMO it's worth to be very "loud" about it 👍

cmd/options/controller_options.go

pkg/driver/driver.go

chrishenzie · 2020-10-02T19:52:07Z

@bertinatto I've squashed these into one commit.

See here for the deprecation comment:

aws-ebs-csi-driver/cmd/options/controller_options.go

Line 32 in cfda969

// DEPRECATED: Use ExtraTags instead.

And here for the warning:

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

Lines 161 to 164 in cfda969

    
           if o.extraTags == nil && extraVolumeTags != nil { 
        
           	klog.Warning("DEPRECATION WARNING: --extra-volume-tags is deprecated, please use --extra-tags instead") 
        
           	o.extraTags = extraVolumeTags 
        
           }

I added an extra check for extraVolumeTags != nil so we don't trip this warning when a user doesn't supply --extra-volume-tags.

bertinatto

Thank you for working on this @chrishenzie!

/lgtm

bertinatto · 2020-10-05T05:29:09Z

pkg/driver/driver.go

 func WithExtraVolumeTags(extraVolumeTags map[string]string) func(*DriverOptions) {
 	return func(o *DriverOptions) {
-		o.extraVolumeTags = extraVolumeTags
+		if o.extraTags == nil && extraVolumeTags != nil {


Ideally we'd have 2 checks: 1 for printing the warning if extraVolumeTags != nil and the other to overwrite o.extraTags. But I think this is fine for now.

bertinatto · 2020-10-05T05:31:00Z

/approve

k8s-ci-robot · 2020-10-05T05:31:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, chrishenzie

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bertinatto]

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

bertinatto · 2020-10-05T06:10:06Z

/retest

bertinatto · 2020-10-05T07:47:40Z

/retest

bertinatto · 2020-10-05T08:20:22Z