Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Tags passed into worker_groups_launch_template extend var.tags for the volumes #1397

Merged
merged 5 commits into from
Aug 31, 2021

Conversation

lisfo4ka
Copy link
Contributor

@lisfo4ka lisfo4ka commented May 26, 2021

PR o'clock

Description

This change brings an option to extend var.tags for the EBS volumes with tags that are passed into var.worker_groups_launch_template or var.workers_group_defaults.

This resolves #1396.

The main use case for this to separate resources costs between owners for shared cluster usage.

Since tagging of EC2 instances via var.worker_groups_launch_template or var.workers_group_defaults is already done, I assume it should be useful to tag volumes as well.

As a result of this change, tags from var.worker_groups_launch_template will override tags in var.workers_group_defaults if any and then will be merged with var.tags for EBS volumes of ASG created via launch templates.

Checklist

Olesia Ivanenko added 2 commits May 26, 2021 16:35
…e_at_launch=false

So if the propagation of the tag to Amazon EC2 instances launched via the ASG is disabled,
the volumes don't get tag as well
@barryib barryib self-assigned this May 27, 2021
Comment on lines +533 to +537
{
for tag in lookup(var.worker_groups_launch_template[count.index], "tags", local.workers_group_defaults["tags"]) :
tag["key"] => tag["value"]
if tag["key"] != "Name" && tag["propagate_at_launch"]
}
Copy link
Member

@barryib barryib May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please follow the instance tag specifications tag extension ? It would be nice to have the same behavior across all tags in launch template.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for such a quick response.
Unfortunately, there is a bit different approach for the instance tagging is already defined with this change #1092 and this improvement #1095
As a result, EC2 instances can get additional tags via the propagate_at_launch option in ASG. If I rewrite this in the same way as for volumes tagging via tag_specifications we'll lose the feature to tag instances created with launch configuration. Is it ok for us? If yes, I'm happy to do this.
Anyway, I can add this tags extension to the instances tag specification and it'll override the approach in ASG tagging, but it could lead to ambiguity in the future.
Please, share your thought regarding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@barryib, do you still want me to make a change for the instances tagging with overriding the existing approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @barryib . Sorry, maybe I misunderstood you. Do you mean that I should implement tagging of the volumes in the same way as tagging of the instances is already done?
Or should I modify the existing approach of the instances tagging in the same way as I've proposed to do for volumes?

We've really interested in the delivery of this feature, so could you advise, please, what should be done to get it merged?

@lisfo4ka
Copy link
Contributor Author

lisfo4ka commented Jun 9, 2021

Hi folks, please, let me know what should be done to bring this change to release?

@lisfo4ka
Copy link
Contributor Author

@barryib could you give any feedback, please? I'm open to any suggestion to give this change a life.

@lisfo4ka
Copy link
Contributor Author

lisfo4ka commented Aug 5, 2021

@barryib, our team looks forward to your response.

@antonbabenko antonbabenko merged commit 8e1d5c1 into terraform-aws-modules:master Aug 31, 2021
@antonbabenko
Copy link
Member

Hi @lisfo4ka !

Thanks for the PR!

I looked into tagging capabilities implemented across the module and I think this PR is on the right track.

v17.6.0 has been just released.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tags to volumes from worker_groups_launch_template["tags"]
3 participants