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
Open

Feature/launch template #97

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ef23524
First pass at using launch_template as an alternative to launch_confi…
jcejohnson Aug 24, 2018
72aee03
WIP experiment but failed due to: https://github.com/terraform-provid…
efx-jjohnson Aug 24, 2018
0ee25b9
Set the cluster name tag (etc) after cluster_extra_tags (etc) so that…
efx-jjohnson Aug 28, 2018
055132e
It's not pretty but it works. Though I think we should wait for https…
efx-jjohnson Aug 28, 2018
af10a1f
Working launch_template without any hacks.
efx-jjohnson Oct 1, 2018
cbd44e2
Fix default vaule and explain the choices
efx-jjohnson Oct 4, 2018
ed5cebe
Wrong dependency for launch_configuration based autoscaling group
jcejohnson Oct 6, 2018
bb7a7a6
Cleaner approach for setting the cluster_size output value
jcejohnson Oct 6, 2018
234f63b
launch_configuration can also request volume encryption
jcejohnson Oct 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 118 additions & 6 deletions modules/vault-cluster/main.tf
View file
Open in desktop
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
View file
Open in desktop
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)}"
}
}
34 changes: 34 additions & 0 deletions modules/vault-cluster/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,37 @@ 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" {
# Choices are launch_configuration and launch_template.
# launch_template gives you the opportunithy to declare volume encryption and
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

# set tags on instance volumes.
#
description = "Select between launch_config-driven or launch_template-driven autoscaling group."

default = "launch_configuration"
}

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 = {}
}