Expand launch config customizations #18
Conversation
There was a problem hiding this comment.
@mheffner thanks a lot for contributing to this module!
I have a bunch of naming suggestions to fit our naming practices, and also a question regarding associate_public_ip_address value.
Appreciate if you run terraform fmt against this configuration as well! 🙏
main.tf
Outdated
| @@ -302,6 +302,12 @@ resource "aws_launch_template" "default" { | |||
| lifecycle { | |||
| create_before_destroy = true | |||
| } | |||
|
|
|||
| metadata_options { | |||
| http_endpoint = "enabled" | |||
There was a problem hiding this comment.
Could you please add a variable to support both options?
main.tf
Outdated
| metadata_options { | ||
| http_endpoint = "enabled" | ||
| http_tokens = var.enable_imdsv2 ? "required" : "optional" | ||
| http_protocol_ipv6 = var.metadata_ipv6 ? "enabled" : "disabled" |
There was a problem hiding this comment.
| http_protocol_ipv6 = var.metadata_ipv6 ? "enabled" : "disabled" | |
| http_protocol_ipv6 = var.http_protocol_ipv6_enabled ? "enabled" : "disabled" |
main.tf
Outdated
|
|
||
| metadata_options { | ||
| http_endpoint = "enabled" | ||
| http_tokens = var.enable_imdsv2 ? "required" : "optional" |
There was a problem hiding this comment.
| http_tokens = var.enable_imdsv2 ? "required" : "optional" | |
| http_tokens = var.imdsv2_enabled ? "required" : "optional" |
variables.tf
Outdated
| default = true | ||
| } | ||
|
|
||
| variable "metadata_ipv6" { |
There was a problem hiding this comment.
| variable "metadata_ipv6" { | |
| variable "http_protocol_ipv6_enabled" { |
variables.tf
Outdated
| default = null | ||
| } | ||
|
|
||
| variable "enable_imdsv2" { |
There was a problem hiding this comment.
| variable "enable_imdsv2" { | |
| variable "imdsv2_enabled" { |
variables.tf
Outdated
| } | ||
|
|
||
| variable "enable_imdsv2" { | ||
| description = "Enable IMDSv2" |
There was a problem hiding this comment.
| description = "Enable IMDSv2" | |
| description = <<-EOT | |
| Whether or not the metadata service requires session tokens, | |
| also referred to as Instance Metadata Service Version 2 (IMDSv2). | |
| EOT |
main.tf
Outdated
| @@ -276,11 +276,11 @@ resource "aws_launch_template" "default" { | |||
| user_data = base64encode(var.user_data) | |||
|
|
|||
| monitoring { | |||
| enabled = true | |||
| enabled = var.detailed_monitoring | |||
There was a problem hiding this comment.
| enabled = var.detailed_monitoring | |
| enabled = var.monitoring_enabled |
variables.tf
Outdated
| @@ -62,6 +62,31 @@ variable "additional_security_group_ids" { | |||
| default = [] | |||
| } | |||
|
|
|||
| variable "detailed_monitoring" { | |||
There was a problem hiding this comment.
| variable "detailed_monitoring" { | |
| variable "monitoring_enabled" { |
| description = "Associate public IP address" | ||
| type = bool | ||
| # default should fall back to subnet setting | ||
| default = null |
There was a problem hiding this comment.
I'm not sure this would work, see the doc:
Boolean value, can be left unset.
Could you please elaborate on the default fall back to the subnet setting?
There was a problem hiding this comment.
So I believe it's Terraform v0.12+ allows setting null to represent unset. I used default = null here to keep the default as the unset option and not require a user to set this variable.
My understanding is that if this isn't set, it'll use the property of the subnet on whether to assign a public IP address. We have public and private subnets in our VPC, on our public subnets they have "Associate public IPv4 address" set to true. When I tried this change with null, the nodes came up with a public IP address. I haven't tried using it in our private subnets, but my guess is they would not get a public IP there.
There was a problem hiding this comment.
Thank you for clarifying! That makes sense.
Yeah, I see now that this attribute has type nullable.TypeNullableBool in the provider, and allows to set the attribute to null explicitly.
|
@gberenice sounds good and thanks for looking. I'll take a look at the changes. I could have sworn I setup Goland to run fmt, but I'll have to double check 😄 |
|
I pushed name changes to the variables and made sure fmt ran this time. I maintained "metadata_" prefixes for the variable options related to the metadata endpoint, since it felt like they could be confusing as top-level variables without it. |
what
why
references