feat: Support default_tags in aws_autoscaling_group and launch_template #1968
Conversation
| @@ -238,7 +242,7 @@ resource "aws_launch_template" "this" { | |||
| for_each = toset(["instance", "volume", "network-interface"]) | |||
| content { | |||
| resource_type = tag_specifications.key | |||
| tags = merge(var.tags, { Name = var.name }, var.launch_template_tags) | |||
| tags = merge(local.launch_template_default_tags, { Name = var.name }, var.launch_template_tags) | |||
There was a problem hiding this comment.
why do we need to set this here? from my understanding, only autoscaling groups behave differently when it comes to default tags
There was a problem hiding this comment.
I double checked what I'd been seeing on Friday, and while the provider default tags do get automatically included in the they do not get included in the tag_specifications of the launch_template resource's tags,launch_template. Those instead act much like the ASG tag specifications. Adding the ability to include the default tags to the launch_template tag specifications ensures that any instances created from the launch_template will also have the default tags set on the instance itself.
Edit to fix the misleading resource tags comment. The launch_template resource has both a tags and tag_specifications input attribute. The default_tags do get added to the tags_all attribute of the state, however they do not get added to the tag_specifications that propagate down to the instance, network-interface, and volume.
There was a problem hiding this comment.
Would a different variable name make sense to make it clearer? Maybe tag_specification_use_default_tags or something similar?
There was a problem hiding this comment.
I found a relevant issue in the provider repo. hashicorp/terraform-provider-aws#19387
There was a problem hiding this comment.
I think for now we leave off the launch template changes - this matches what we have in the autoscaling module (which at some point will get folded into this module) and just add to the autoscaling group
There was a problem hiding this comment.
I'm very confused. The module as it exists right now is not able to correctly set provider default tags for resources created by neither an autoscaling group nor a launch template. Are you saying that only the autoscaling group use case should be fixed?
There was a problem hiding this comment.
- We cannot control the autoscaling groups created by the EKS managed node group service so there is no solution for default tags on that resource currently other than using the external autoscaling group tags resource (has to be external to the module Add Tags to ASG when using node groups #1558 (comment))
- We should mirror what we did in the autoscaling group module for the self-managed node group module https://github.com/terraform-aws-modules/terraform-aws-autoscaling/blob/3c928b8cfb2be46ff756be350fb26c33ee1a640a/main.tf#L10-L15 which is the same as what the Terraform docs specify. We only add the default tags to the autoscaling group resource
There was a problem hiding this comment.
I understand there are many issues related to default tags right now which is why I am cautious regarding what gets introduced. The Terraform docs only offer the caveat for autoscaling groups https://www.hashicorp.com/blog/default-tags-in-the-terraform-aws-provider and not launch templates and this is what we have mimicked in the autoscaling group module without issue. So for now, I think that is a safe path if we want to introduce that change
There was a problem hiding this comment.
Ok, unfortunately I need to maintain my company fork with the launch template changes, since that is the issue I needed to solve when I stumbled across the open issue about the asg. I'll try to get a moment to pull just the asg changes over to a fork on my personal github and submit a new PR.
| @@ -229,7 +233,7 @@ resource "aws_launch_template" "this" { | |||
| for_each = toset(["instance", "volume", "network-interface"]) | |||
| content { | |||
| resource_type = tag_specifications.key | |||
| tags = merge(var.tags, { Name = var.name }, var.launch_template_tags) | |||
| tags = merge(local.launch_template_default_tags, { Name = var.name }, var.launch_template_tags) | |||
There was a problem hiding this comment.
same comment as above - why set on launch template explicitly?
|
I'm pretty sure that the default tags do not get set automatically on the launch template, but I will double check. |
raizyr
force-pushed
the
master
branch
3 times, most recently
from
7f491c2
to
79b2975
Compare
|
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. |
Description
launch_template_use_default_tagsto EKS managed node group and self managed node group module definitions. When set to true, the default tags configured on the provider will be added to the launch_templatetag_specifications.asg_use_default_tagsto EKS self managed node group module definition. When set totrue, the default tags configured on the provider will be added to the autoscaling_group tags.var.eks_managed_node_groups or var.eks_managed_node_group_defaultsandvar.self_managed_node_groups or var.self_managed_node_group_defaultsand pass the result to the apprioriate node group module.Motivation and Context
Fixes #1916
Breaking Changes
No
How Has This Been Tested?
examples/*projects