Feature/launch template #97

jcejohnson · 2018-10-04T14:58:13Z

The goal of this PR is to allow the module user to select either launch_configuration (traditional) or launch_template. Selecting launch_template should be backward compatible with the original implementation but with the ability to propagate tags to instance volumes. (With the caveat described below.)

As usual, I can only test this with our particular usecases. In that internal testing (one VPC per region, two vault clusters per VPC) I have validated that the resulting instances are identical to their launch_configuration counterparts and that the instance volumes have the expected tags.

I have not yet implemented a similar change to terraform-aws-consul. Doing so may result in changes to this PR if new edgecases are discovered there.

Caveat:
It only works when encrypted == "". This was reportedly addressed by AWS provider 1.34.0 but I'm still having issues. Ref: hashicorp/terraform-provider-aws#4553

…guration so that we can apply tags to instance volumes Syntax, typos

… cluster_extra_tags can't overwrite it.

…corp/terraform-provider-aws#4553 to be fixed & then do a cleaner implementation. To make this work, I have to do: terraform apply -target aws_iam_instance_profile.instance_profile -var-file inputs modules/vault-cluster/ terraform apply -var-file inputs modules/vault-cluster/

Tested with AWS provider 1.38.0 and root_volume_ebs_encryption="" Does not work for *my* usecase with any other value for root_volume_ebs_encryption

Etiene · 2018-10-04T15:34:37Z

modules/vault-cluster/variables.tf

+
+variable "asg_launch_mechanism" {
+  description = "Select between launch_config-driven or launch_template-driven autoscaling group."
+  default     = "launch_config"


On line 14 of main.tf the expected value is launch_configuration, not launch_config.

It's also worth adding here more clearly what are the expected values, or even maybe doing this in a different way, such as using booleans, to avoid confusion?

Another thing is that I think this is lacking an explanation of why you would choose one way over another. If not here, maybe a small note on the Readme?

Fixed, thanks. When testing I was always passing the value for asg_launch_mechanism and didn't realize I had a bad default.
What I really need for the choice is an enumeration. If I had a boolean for each option then we're setup for enabling both at the same time. If I do one boolean to select between the two (e.g. - use_launch_template) I feel like it would be contrived.
I've added a bit of explanation in the comment for asg_launch_mechanism. My reason for doing this in the first place is to set tags on instance volumes. I'm sure there are other reasons for choosing one over the other but I'm very myopic at the moment.

brikis98 · 2018-10-06T12:55:48Z

modules/vault-cluster/main.tf

  name_prefix = "${var.cluster_name}"

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

+  depends_on = ["aws_iam_instance_profile.instance_profile", "aws_launch_template.launch_template"]


Why these depends_on?

Should have been aws_launch_configuration.launch_configuration. Fixed.
I added these for self-documentation since there are two autoscaling groups.

brikis98 · 2018-10-06T12:59:45Z

modules/vault-cluster/main.tf

+  }"]
+}
+
+# An alternate autoscaling group that uses a launch_template


Hm, I wonder if we could get by with just one aws_autoscaling_group resource.

locals { launch_template = { launch_configuration = [] launch_template = [ { id = "${aws_launch_template.launch_template.id}" version = "${var.launch_template_version}" } ] } launch_configuration = { launch_configuration = "${aws_launch_configuration.launch_configuration.name}" launch_template = "" } } resource "aws_autoscaling_group" "autoscaling_group" { launch_template = "${local.launch_template[var.asg_launch_mechanism]}" launch_configuration = "${local.launch_configuration[var.asg_launch_mechanism]}" }

I tried but terraform immediately complained if I tried to declare both.

brikis98 · 2018-10-06T13:10:47Z

modules/vault-cluster/main.tf

+      )
+    }"
+  }
+  # Important note: whenever using a launch configuration with an auto scaling group, you must set


Is this true with a launch template too?

To be honest, I'm not entirely sure that it is (still?) true for launch_configuration. My code is uses the vault-cluster default (true) and I haven't made any special configuration for the other resources.

brikis98 · 2018-10-06T13:11:25Z

modules/vault-cluster/main.tf

+  tags = "${var.launch_template_tags}"
+  tag_specifications {
+    # Instance tags are already handled by the autoscaling group
+    resource_type = "volume"


I must admit that this is a lot of extra code to maintain just to be able to tag EBS volumes. Is there no other way to do it?

I agree. My hope is that others will find other reasons why launch_template is a better alternative for their usecases and can expand on what I've started here.

The reason for the long delay between my initial submission & issue and this PR was due to the aws provider issue mentioned elsewhere. In the meantime, I was able to find a workaround via user-data script which inspects the instance & applies its tags to the instance volume. Of course, that requires that the instance be able to query its own tags and apply tags to its instance.

So, yes, there is another way to do it but it's a hack. And, from what I gather, launch templates are the new hot thing and presumably the preferred option going forward.

brikis98 · 2018-10-06T13:11:35Z

modules/vault-cluster/main.tf

@@ -88,7 +198,7 @@ resource "aws_security_group" "lc_security_group" {
    create_before_destroy = true
  }

-  tags = "${merge(map("Name", var.cluster_name), var.security_group_tags)}"
+  tags = "${merge(var.security_group_tags, map("Name", var.cluster_name))}"


Why reverse the order of the tags?

I don't want var.security_group_tags to inadvertently change the value of the Name tag.

Hm, I'm tempted to say that's a feature, not a bug, as it allows the user full control over the Name tag in case the default isn't what they want...

brikis98 · 2018-10-06T13:11:48Z

modules/vault-cluster/main.tf

@@ -29,10 +66,11 @@ resource "aws_autoscaling_group" "autoscaling_group" {
  wait_for_capacity_timeout = "${var.wait_for_capacity_timeout}"

  tags = ["${concat(
+    var.cluster_extra_tags,


Why reverse the order of the tags?

I don't want var.cluster_extra_tagsto inadvertently change the value of the Name tag.

brikis98 · 2018-10-06T13:12:32Z

modules/vault-cluster/outputs.tf

-  value = "${aws_autoscaling_group.autoscaling_group.name}"
+  # This is safe because asg_launch_mechanism will only allow one of aws_autoscaling_group.autoscaling_group.*
+  # or aws_autoscaling_group.lt_autoscaling_group.* to be non-empty.
+  value = "${join("",concat(aws_autoscaling_group.autoscaling_group.*.name,aws_autoscaling_group.lt_autoscaling_group.*.name))}"


Our typical approach for this is: ${element(concat(x, y), 0)}.

Ah. Nice. Fixed.

brikis98 · 2018-10-06T13:13:24Z

modules/vault-cluster/variables.tf

+
+variable "asg_launch_mechanism" {
+  # Choices are launch_configuration and launch_template.
+  # launch_template gives you the opportunithy to declare volume encryption and


You can enable volume encryption with a launch config too: https://www.terraform.io/docs/providers/aws/r/launch_configuration.html#encrypted

Indeed it can. Comment updated.
(FWIW, that ability isn't currently exposed in this module)

We probably should expose the ability to encrypt EBS volumes in the module for both launch configs and launch templates...

jcejohnson · 2018-10-06T18:56:15Z

So, the upshot of it is this...

There's a lot of new code here to add the option to use launch_template instead of launch_configuration
Some (i.e. - lt_autoscaling_group) of the new code is redundant because aws_autoscaling_group cannot be given both a launch_template and a launch_configuration even if one is empty
Currently, the new vault-cluster features enabled by choosing launch_template are:
- volume encryption (possible with launch_configuration but not exposed by this module)
- specifying volume tags (not possible with launch_configuration but can be worked around via user-data)
launch_template offers more features than launch_configuration, whether or not those are applicable to vault-cluster remains to be seen
If this PR is accepted I will followup with a similar change to terraform-aws-consul to give it the ability to specify volume tags

brikis98 · 2018-10-07T12:37:54Z

One thought for you: is it worth waiting until Terraform 0.12 comes out to add this feature? It will support both dynamic blocks and null defaults, so we'd be able to do this with just one resource rather than several.

jcejohnson · 2018-10-07T14:44:10Z

One thought for you: is it worth waiting until Terraform 0.12 comes out to add this feature? It will support both dynamic blocks and null defaults, so we'd be able to do this with just one resource rather than several.

I think that is an excellent idea!

brikis98 · 2018-10-07T14:48:44Z

OK, great. Hopefully, 0.12 will be out before too long, and we can make this PR simpler and get it merged. Thanks!

ozbillwang · 2019-08-13T01:49:28Z

Any updates for this PR?

jcejohnson · 2019-08-18T20:53:33Z

No. I've been re-tasked to work on GCP adoption and haven't been able to get back to this. If there is no other interest it's probably best to just close this.

hashicorp-cla · 2020-06-08T02:32:32Z

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

0 out of 2 committers have signed the CLA.

jcejohnson
efx-jjohnson

_{Have you signed the CLA already but the status is still pending? Recheck it.}

jcejohnson and others added 5 commits August 24, 2018 10:52

First pass at using launch_template as an alternative to launch_confi…

ef23524

…guration so that we can apply tags to instance volumes Syntax, typos

WIP experiment but failed due to: hashicorp/terraform-provider-aws#4553

72aee03

Set the cluster name tag (etc) after cluster_extra_tags (etc) so that…

0ee25b9

… cluster_extra_tags can't overwrite it.

Working launch_template without any hacks.

af10a1f

Tested with AWS provider 1.38.0 and root_volume_ebs_encryption="" Does not work for *my* usecase with any other value for root_volume_ebs_encryption

jcejohnson mentioned this pull request Oct 4, 2018

Encrypted flag cannot be specified since device /dev/sda1 has a snapshot specified hashicorp/terraform-provider-aws#4553

Closed

Etiene reviewed Oct 4, 2018

View reviewed changes

Fix default vaule and explain the choices

cbd44e2

brikis98 reviewed Oct 6, 2018

View reviewed changes

jcejohnson added 3 commits October 6, 2018 14:22

Wrong dependency for launch_configuration based autoscaling group

ed5cebe

Cleaner approach for setting the cluster_size output value

bb7a7a6

launch_configuration can also request volume encryption

234f63b

eedwards-sk mentioned this pull request Nov 28, 2018

Feature Request: Support Launch-Templates for t2/t3 unlimited configuration hashicorp/terraform-aws-consul#103

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/launch template #97

Feature/launch template #97

jcejohnson commented Oct 4, 2018

Etiene Oct 4, 2018

jcejohnson Oct 4, 2018

brikis98 Oct 6, 2018

jcejohnson Oct 6, 2018

brikis98 Oct 6, 2018

jcejohnson Oct 6, 2018

brikis98 Oct 7, 2018

brikis98 Oct 6, 2018

jcejohnson Oct 6, 2018

brikis98 Oct 6, 2018

jcejohnson Oct 6, 2018

brikis98 Oct 6, 2018

jcejohnson Oct 6, 2018

brikis98 Oct 7, 2018

brikis98 Oct 6, 2018

jcejohnson Oct 6, 2018

brikis98 Oct 6, 2018

jcejohnson Oct 6, 2018

brikis98 Oct 6, 2018

jcejohnson Oct 6, 2018

brikis98 Oct 7, 2018

jcejohnson commented Oct 6, 2018

brikis98 commented Oct 7, 2018

jcejohnson commented Oct 7, 2018

brikis98 commented Oct 7, 2018

ozbillwang commented Aug 13, 2019

jcejohnson commented Aug 18, 2019

hashicorp-cla commented Jun 8, 2020 •

edited

Loading

Feature/launch template #97

Are you sure you want to change the base?

Feature/launch template #97

Conversation

jcejohnson commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcejohnson commented Oct 6, 2018

brikis98 commented Oct 7, 2018

jcejohnson commented Oct 7, 2018

brikis98 commented Oct 7, 2018

ozbillwang commented Aug 13, 2019

jcejohnson commented Aug 18, 2019

hashicorp-cla commented Jun 8, 2020 • edited Loading

hashicorp-cla commented Jun 8, 2020 •

edited

Loading