Allow users to opt-out of changing instance name tags

[mbillow]

I was very surprised when my instance names changed after adding this. I much prefer their original names, so I added a way to opt-out of of them being change. 😄

[mbillow]

Hey @jimsheldon @hikerspath @spier 😄

Can one of you take a look at this? Let me know what you think.

[spier]

@mbillow thanks for putting in the effort to describe your proposal as a PR. That makes it a lot easier to understand what you have in mind here.

Could you elaborating a bit on why the changed instance name tags caught you by surprize?
Also curious what you use them for, and how the original names set by AWS by default are better for that use case than the ones set by this module?

Do you think that the current behavior of this module is so surprizing in this regard that we should really set the update_instance_name_tag flag to false by default? (not sure if that would qualify as a breaking change but we can worry about that later)

Thanks again for your PR and I am looking forward to the conversation here and where it leads.

[mbillow]

Could you elaborating a bit on why the changed instance name tags caught you by surprize?

I have two ASGs that deploy very different applications that both use this module. By default, the instance template defines the tag Name={name of app}. Each of these application has it's own subdomain (app1.localdomain and app2.localdomain) and I wanted the DNS records to be #instanceid.{name of app}.localdomain.

This works, but it changes the name of the instances from {name of app} to their instance id. This turns the listing of nodes from something like:

Name                 Instance ID
--------             --------------
application-one      i-12345abcdef
application-one      i-67890ghijkl
application-two      i-1a2b3c4d5e6
application-two      i-f7g8h9i0j1k

where everything is clearly distinguished by application name to:

Name                 Instance ID
--------             --------------
i-12345abcdef        i-12345abcdef
i-67890ghijkl        i-67890ghijkl
i-1a2b3c4d5e6        i-1a2b3c4d5e6
i-f7g8h9i0j1k        i-f7g8h9i0j1k

which is completely useless without playing around with what fields are displayed in the console/are queried for in the CLI.

A possible work around without changing code would be to do something like app1-#instanceid.app1.localdomain but that just seems redundant and messy.

Do you think that the current behavior of this module is so surprizing in this regard that we should really set the update_instance_name_tag flag to false by default? (not sure if that would qualify as a breaking change but we can worry about that later)

At a minimum it should be clearly stated that the current behavior is "expected." I couldn't find anywhere in the README that stated this and it took some digging in CloudTrail to figure out what was actually happening.

Is this worth a potentially breaking change? 🤷🏼‍♂️ Probably not, which is why I did this the way I did. Personally, I feel that documenting the current behavior and allowing a way to easily opt-out (this PR) is fine.

[spier]

This is fantastic extra context @mbillow!
Thanks a lot for this, I understand this much better now.

[jimsheldon]

Thanks for the contribution!

We are reviewing this and will respond as soon as possible

[mbillow]

@jimsheldon @spier Is it possible to get a rough ETA on this getting merged? I'd like to avoid having to switch my TF over to my fork, if possible.

[jimsheldon]

Hello @mbillow

I am reviewing this and wondering if it would be better to control this behavior through a tag on the ASG itself, rather than a global setting? With this approach, the lambda function would either always change the name tag or never change it. It might be better to let the ASG control that behavior?

If you look at #34 the approach there uses a unique asg:multihost_pattern tag to control what the lambda function does, could you do something similar here?

Thanks

[mbillow]

@jimsheldon I made the requested change and updated the docs to reflect the changes I made. This leaves the default (untagged) behavior the same but the following tag will stop it from changing the Name tag:

tag {
  key                 = "asg:update_instance_name"
  value               = "false"
  propagate_at_launch = true
}

Let me know if you want anything else here. 😄

[cydowens]

@mbillow Thanks for providing the above code snippet. Would you mind also contributing a test to our terratest? Here is the link to our current test file: https://github.com/meltwater/terraform-aws-asg-dns-handler/blob/master/terratest/test/asg_dns_handler_test.go

[cydowens]

@mbillow Hello, unfortunately the code that you submitted drone's build isn't passing. Would you be able to update your code and also submit a test with it? Let us know if you have any additional questions.