Skip to content
This repository has been archived by the owner on Jan 25, 2023. It is now read-only.

Feature/launch template #97

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
124 changes: 118 additions & 6 deletions modules/vault-cluster/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,47 @@ terraform {
# ---------------------------------------------------------------------------------------------------------------------

resource "aws_autoscaling_group" "autoscaling_group" {
count = "${var.asg_launch_mechanism == "launch_configuration" ? 1 : 0}"
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"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why these depends_on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


availability_zones = ["${var.availability_zones}"]
vpc_zone_identifier = ["${var.subnet_ids}"]

# Use a fixed-size cluster
min_size = "${var.cluster_size}"
max_size = "${var.cluster_size}"
desired_capacity = "${var.cluster_size}"
termination_policies = ["${var.termination_policies}"]

health_check_type = "${var.health_check_type}"
health_check_grace_period = "${var.health_check_grace_period}"
wait_for_capacity_timeout = "${var.wait_for_capacity_timeout}"

tags = ["${concat(
var.cluster_extra_tags,
list(
map("key", var.cluster_tag_key, "value", var.cluster_name, "propagate_at_launch", true)
)
)
}"]
}

# An alternate autoscaling group that uses a launch_template
Copy link
Collaborator

Choose a reason for hiding this comment

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

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]}"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Bummer

resource "aws_autoscaling_group" "lt_autoscaling_group" {
count = "${var.asg_launch_mechanism == "launch_template" ? 1 : 0}"
name_prefix = "${var.cluster_name}"

launch_template {
id = "${aws_launch_template.launch_template.id}"
version = "${var.launch_template_version}"
}

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

availability_zones = ["${var.availability_zones}"]
vpc_zone_identifier = ["${var.subnet_ids}"]

Expand All @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why reverse the order of the tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

list(
map("key", var.cluster_tag_key, "value", var.cluster_name, "propagate_at_launch", true)
),
var.cluster_extra_tags)
)
)
}"]
}

Expand All @@ -41,6 +79,7 @@ resource "aws_autoscaling_group" "autoscaling_group" {
# ---------------------------------------------------------------------------------------------------------------------

resource "aws_launch_configuration" "launch_configuration" {
count = "${var.asg_launch_mechanism == "launch_configuration" ? 1 : 0}"
name_prefix = "${var.cluster_name}-"
image_id = "${var.ami_id}"
instance_type = "${var.instance_type}"
Expand Down Expand Up @@ -72,6 +111,77 @@ resource "aws_launch_configuration" "launch_configuration" {
}
}

# ---------------------------------------------------------------------------------------------------------------------
# CREATE LAUNCH TEMPLATE TO DEFINE WHAT RUNS ON EACH INSTANCE IN THE ASG
# ---------------------------------------------------------------------------------------------------------------------

data "aws_ami" "ami" {
filter {
name = "image-id"
values = ["${var.ami_id}"]
}
}

resource "aws_launch_template" "launch_template" {
count = "${var.asg_launch_mechanism == "launch_template" ? 1 : 0}"
name_prefix = "${var.cluster_name}-"
image_id = "${var.ami_id}"
instance_type = "${var.instance_type}"
user_data = "${base64encode(var.user_data)}"

depends_on = ["aws_iam_instance_profile.instance_profile"]

iam_instance_profile {
name = "${aws_iam_instance_profile.instance_profile.name}"
}

key_name = "${var.ssh_key_name}"

# Don't use vpc_security_group_ids when network_interfaces includes security_groups.
# vpc_security_group_ids = ["${concat(list(aws_security_group.lc_security_group.id), var.additional_security_group_ids)}"]

placement {
tenancy = "${var.tenancy}"
}
network_interfaces {
associate_public_ip_address = "${var.associate_public_ip_address}"
delete_on_termination = true
security_groups = ["${concat(list(aws_security_group.lc_security_group.id), var.additional_security_group_ids)}"]
}
ebs_optimized = "${var.root_volume_ebs_optimized}"
block_device_mappings {
device_name = "${data.aws_ami.ami.root_device_name}"

ebs {
encrypted = "${var.root_volume_ebs_encryption}"
volume_type = "${var.root_volume_type}"
volume_size = "${var.root_volume_size}"
delete_on_termination = "${var.root_volume_delete_on_termination}"
}
}
tags = "${var.launch_template_tags}"
tag_specifications {
# Instance tags are already handled by the autoscaling group
resource_type = "volume"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


tags = "${merge(
var.volume_extra_tags,
map("key", var.cluster_tag_key, "value", var.cluster_name)
)
}"
}
# Important note: whenever using a launch configuration with an auto scaling group, you must set
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this true with a launch template too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

# create_before_destroy = true. However, as soon as you set create_before_destroy = true in one resource, you must
# also set it in every resource that it depends on, or you'll get an error about cyclic dependencies (especially when
# removing resources). For more info, see:
#
# https://www.terraform.io/docs/providers/aws/r/launch_configuration.html
# https://terraform.io/docs/configuration/resources.html
lifecycle {
create_before_destroy = true
}
}

# ---------------------------------------------------------------------------------------------------------------------
# CREATE A SECURITY GROUP TO CONTROL WHAT REQUESTS CAN GO IN AND OUT OF EACH EC2 INSTANCE
# ---------------------------------------------------------------------------------------------------------------------
Expand All @@ -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))}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why reverse the order of the tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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...

}

resource "aws_security_group_rule" "allow_ssh_inbound_from_cidr_blocks" {
Expand Down Expand Up @@ -188,8 +298,9 @@ resource "aws_s3_bucket" "vault_storage" {
force_destroy = "${var.force_destroy_s3_bucket}"

tags = "${merge(
map("Description", "Used for secret storage with Vault. DO NOT DELETE this Bucket unless you know what you are doing."),
var.s3_bucket_tags)
var.s3_bucket_tags,
map("Description", "Used for secret storage with Vault. DO NOT DELETE this Bucket unless you know what you are doing.")
)
}"
}

Expand All @@ -201,7 +312,8 @@ resource "aws_iam_role_policy" "vault_s3" {
}

data "aws_iam_policy_document" "vault_s3" {
count = "${var.enable_s3_backend ? 1 : 0}"
count = "${var.enable_s3_backend ? 1 : 0}"

statement {
effect = "Allow"
actions = ["s3:*"]
Expand Down
14 changes: 7 additions & 7 deletions modules/vault-cluster/outputs.tf
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
output "asg_name" {
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))}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Nice. Fixed.

}

output "cluster_tag_key" {
Expand All @@ -11,11 +13,9 @@ output "cluster_tag_value" {
}

output "cluster_size" {
value = "${aws_autoscaling_group.autoscaling_group.desired_capacity}"
}

output "launch_config_name" {
value = "${aws_launch_configuration.launch_configuration.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.*.desired_capacity,aws_autoscaling_group.lt_autoscaling_group.*.desired_capacity))}"
}

output "iam_role_arn" {
Expand All @@ -32,4 +32,4 @@ output "security_group_id" {

output "s3_bucket_arn" {
value = "${join(",", aws_s3_bucket.vault_storage.*.arn)}"
}
}
29 changes: 29 additions & 0 deletions modules/vault-cluster/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,32 @@ variable "force_destroy_s3_bucket" {
description = "If 'configure_s3_backend' is enabled and you set this to true, when you run terraform destroy, this tells Terraform to delete all the objects in the S3 bucket used for backend storage. You should NOT set this to true in production or you risk losing all your data! This property is only here so automated tests of this module can clean up after themselves. Only used if 'enable_s3_backend' is set to true."
default = false
}

# Launch Template Extensions

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

variable "launch_template_tags" {
description = "A list of tags to add to the launch template."
type = "map"
default = {}
}

variable "root_volume_ebs_encryption" {
description = "If true, the launched EC2 instance's root volume will be encrypted."
default = ""
}

variable "launch_template_version" {
default = "Launch template verison to be used by the autoscaling group."
default = "$Latest"
}

variable "volume_extra_tags" {
description = "A list of additional tags to add to each Instance's volumes in the ASG. Only applicable when use_launch_template is true."
type = "map"
default = {}
}