Add tagging support for AWS ASG

[radeksimko]

This is fixing #932
I'm aware of #1076 but the implementation is not as easy as @bobtfish think it is 😉

The autoscaling.CreateOrUpdateTags endpoint does not remove tags (as the name suggests), there's another endpoint autoscaling.DeleteTags for this. Therefore we need to do all the diffing and call correct endpoints with correct data when updating.

Updated tests + documentation attached.

I intentionally used tag (singular) as it IMO makes more sense -> it's similar to ELB's listener.

Here's an example:

resource "aws_autoscaling_group" "bar" {
  availability_zones = ["eu-west-1a"]
  name = "sample"
  max_size = 1
  min_size = 1

  launch_configuration = "${aws_launch_configuration.foo.name}"

  tag {
    key = "Name"
    value = "dadada"
    propagate_at_launch = false
  }

  tag {
    key = "Division"
    value = "finance"
    propagate_at_launch = true
  }
}

[radeksimko]

Regarding PropagateAtLaunch, it's a bit complicated, but here's a few of my thoughts:

• CloudFormation makes this required
• The parameter is optional by the API docs, default is false (although default is not actually documented anywhere)
• AWS CLI has the same behavior as API above
• I personally think that there will be more cases with PropagateAtLaunch=true than false

The current implementation is making it optional and defaulting to false (no propagation), but I'm open for any suggestions and ideas for changing it.

Shall we mimic the CF behavior and make it required then? Sometimes explicit is better than implicit I believe.

[bobtfish]

Your synopsis nails my take on it :)

I'm slightly pro explicit, but I don't have really strong feelings.

[mitchellh]

I think we should just move this out into a helper in the AWS provider. I don't see this as being generally useful enough to put onto the Set directly.

[mitchellh]

One comment from me but this looks really, really good and will be much appreciated.

As for your propagate behavior: I think this is correct. Thanks!

[radeksimko]

Modified as requested = propagate_at_launch is now required (documentation updated too), MapByKey moved into builtin/providers/aws/autoscaling_tags.go as SetToMapByKey.

[mitchellh]

Can you rename SetToMapByKey to be lowercase? Sorry for the nitpick but I don't think we should export that.

[mitchellh]

After that, LGTM

[radeksimko]

Do you want just the first character to be lowercase or the whole name?

i.e. setToMapByKey or settomapbykey?

[mitchellh]

Just the first character. This is how Go knows whether to export it publicly or not.

[radeksimko]

It's all fixed now. 😉

[chrisferry]

👍 This works perfectly!

[catsby]

Hey @radeksimko it looks like c4ec8cf introduced significant changes, does the original description of the PR still hold up? Perhaps it was just a significant refactoring, I didn't see the original.

Does this specific tag behavior apply to other resources, or is it just on ASGs?

[radeksimko]

does the original description of the PR still hold up? Perhaps it was just a significant refactoring, I didn't see the original.

I think it's all up to date, the logic was mostly taken from the existing EC2-tagging logic, but I couldn't directly reuse that logic, see reasons below.

Does this specific tag behavior apply to other resources, or is it just on ASGs?

@catsby Good question, complicated answer below.

EC2-related tagging

Existing

It's worth pointing out that the existing ./builtin/providers/aws/tags.go should really be called ec2_tags.go as it in fact uses the EC2 API only and deals with tags that are related to EC2 only which are currently following:

• aws_instance
• aws_internet_gateway
• aws_network_acl
• aws_route_table
• aws_security_group
• aws_subnet
• aws_vpc
• aws_vpc_peering_connection

Missing

Here's the full list of EC2 resources that can be tagged using the same API endpoint which means that we're missing tagging functionality in the EC2 world for these:

• AMI - probably unrelated to Terraform
• aws_customer_gateway - not supported by TF (yet)
• aws_dhcp_options - not supported by TF (yet)
• EBS Volume - tagging not implemented, most likely should go under aws_instance.block_device.tags
• aws_network_interface - currently auto-managed along with EC2 instances to assign public IP, not sure if this would be useful as a separate resource, maybe related to Support AWS Elastic Network Interfaces (ENI) #1149 ?
• aws_reserved_instances - not supported by TF (yet)
• aws_spot_instance_request - not sure how useful it could be for managing in TF
• Snapshot - probably unrelated to Terraform
• aws_vpn_gateway - not supported by TF (yet)
• aws_vpn_connection - not supported by TF (yet) - Adding CRUD support for Customer Gateway for AWS VPC. #838 Resources to manage AWS vpn connections, customer and virtual gateways #1011 AWS VPN Resources  #551

EC2-UNrelated tags

I think most of the resources having the simple tag structure as below 🔽

type Tag struct {
    Key   aws.StringValue `ec2:"Key" xml:"key"`
    Value aws.StringValue `ec2:"Value" xml:"value"`
}

may theoretically work with the existing tagging logic as long as we can abstract the EC2-specific API calls & data-types that we currently have in ./builtin/providers/aws/tags.go.

grep -i 'ec2' ./builtin/providers/aws/tags.go

./builtin/providers/aws/tags.go:    "github.com/hashicorp/aws-sdk-go/gen/ec2"
./builtin/providers/aws/tags.go:func setTags(conn *ec2.EC2, d *schema.ResourceData) error {
./builtin/providers/aws/tags.go:            err := conn.DeleteTags(&ec2.DeleteTagsRequest{
./builtin/providers/aws/tags.go:            err := conn.CreateTags(&ec2.CreateTagsRequest{
./builtin/providers/aws/tags.go:func diffTags(oldTags, newTags []ec2.Tag) ([]ec2.Tag, []ec2.Tag) {
./builtin/providers/aws/tags.go:    var remove []ec2.Tag
./builtin/providers/aws/tags.go:func tagsFromMap(m map[string]interface{}) []ec2.Tag {
./builtin/providers/aws/tags.go:    result := make([]ec2.Tag, 0, len(m))
./builtin/providers/aws/tags.go:        result = append(result, ec2.Tag{
./builtin/providers/aws/tags.go:func tagsToMap(ts []ec2.Tag) map[string]string {

Resources that would benefit from this as these have the same Tag structure but not part of the EC2 group:

• cloudformation - not supported by TF (yet)
• elasticbeanstalk - not supported by TF (yet)
• elb - tagging not implemented, but possible
• emr - not supported by TF (yet)
• kinesis - not supported by TF (yet)
• rds - tagging not implemented, but possible
• redshift - not supported by TF (yet)

EC2-UNrelated requiring extra care

All these will most likely require resource-specific code for handling tags

autoscaling

type Tag struct {
    Key               aws.StringValue  `query:"Key" xml:"Key"`
    PropagateAtLaunch aws.BooleanValue `query:"PropagateAtLaunch" xml:"PropagateAtLaunch"`
    ResourceID        aws.StringValue  `query:"ResourceId" xml:"ResourceId"`
    ResourceType      aws.StringValue  `query:"ResourceType" xml:"ResourceType"`
    Value             aws.StringValue  `query:"Value" xml:"Value"`
}

route53

type Tag struct {
    XMLName xml.Name

    Key   aws.StringValue `xml:"Key"`
    Value aws.StringValue `xml:"Value"`
}

s3

type Tag struct {
    XMLName xml.Name

    Key   aws.StringValue `xml:"Key"`
    Value aws.StringValue `xml:"Value"`
}

[radeksimko]

@catsby Do you want me to create another issue from that last comment with a checklist or something?

[jacobwgillespie]

Any updates on merging this? We could really use tagging on ASGs (in the meantime, building Terraform from the fork is working for us).

[catsby]

@radeksimko hey sorry for the silence here; I'm taking on Tagging now and tracking it internally, but your list is very similar so yeah, if you could make a separate issue with that last comment that would be great! We can track progress publicly that way.

I just merged #1289 adding tags for ELB, and I've got a PR open for RDS (#1292).

@jacobwgillespie re: merging this specific PR, I suspect it will happen this week, I need to take a closer look. I have plans for what @radeksimko mentioned in terms of consolidating the tagging files, right now I'm duplicating them to get things rolling but we'll merge them into something generic soon.

[phinze]

@radeksimko coupla notes on d.Set

• It returns an error, which generally we don't check, except in the case when the value we're setting is complex. So this "tag" case is one where I'd check the error return value.
• As of recently it got the ability to handle pointer dereferencing internally with proper nil checking, so can you remove the dereferencing from the other d.Set calls here?

👯

[radeksimko]

ok, I will have a look at that probably tomorrow.

[phinze]

Generally looking good! One comment for you.

[jacobwgillespie]

@catsby awesome, thanks! 👍

[catsby]

Closing in favor of #1319

[catsby]

I just merged #1319, a continuation of this PR, so this is effectively merged. Thanks everyone! Special thanks to @radeksimko for doing all the hard work 😄

[ghost]

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.