Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creates a single private route table when single_nat_gateway is true #83

Merged
Changes from all commits
Commits
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
23 changes: 12 additions & 11 deletions main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ terraform {

locals {
max_subnet_length = "${max(length(var.private_subnets), length(var.elasticache_subnets), length(var.database_subnets), length(var.redshift_subnets))}"
nat_gateway_count = "${var.single_nat_gateway ? 1 : local.max_subnet_length}"
Copy link
Member

Choose a reason for hiding this comment

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

max_subnet_length gets longest of all 4 subnet types, while NAT should only be applicable to private_subnets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can see, that's not how it works currently. All the private-type subnets, including the "extras" (database, redshift, elasticache), are associated to the "private" route tables. This means they all route through the NAT Gateway. The number of NAT Gateways therefore needs to account for all the private-type subnets, which is what max_subnet_length does.

}

######
Expand Down Expand Up @@ -80,11 +81,11 @@ resource "aws_route" "public_internet_gateway" {
# There are so many routing tables as the largest amount of subnets of each type (really?)
#################
resource "aws_route_table" "private" {
count = "${var.create_vpc && local.max_subnet_length > 0 ? local.max_subnet_length : 0}"
count = "${var.create_vpc && local.max_subnet_length > 0 ? local.nat_gateway_count : 0}"

vpc_id = "${aws_vpc.this.id}"

tags = "${merge(var.tags, var.private_route_table_tags, map("Name", format("%s-private-%s", var.name, element(var.azs, count.index))))}"
tags = "${merge(var.tags, var.private_route_table_tags, map("Name", (var.single_nat_gateway ? "${var.name}-private" : format("%s-private-%s", var.name, element(var.azs, count.index)))))}"

lifecycle {
# When attaching VPN gateways it is common to define aws_vpn_gateway_route_propagation
Expand Down Expand Up @@ -203,15 +204,15 @@ locals {
}

resource "aws_eip" "nat" {
count = "${var.create_vpc && (var.enable_nat_gateway && !var.reuse_nat_ips) ? (var.single_nat_gateway ? 1 : length(var.azs)) : 0}"
count = "${var.create_vpc && (var.enable_nat_gateway && !var.reuse_nat_ips) ? local.nat_gateway_count : 0}"
Copy link
Member

Choose a reason for hiding this comment

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

This should take into account number of azs

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 think that logic is incorrect. You can pass in a list of AZs, and pass in fewer private subnets. You end up with more NAT Gateways than would be used.

This "fixes" that logic, so you end up with a number of NAT Gateways that matches max_subnet_length, or just one if using single_nat_gateway.

Happy to move that to another PR if you like though.


vpc = true

tags = "${merge(var.tags, map("Name", format("%s-%s", var.name, element(var.azs, (var.single_nat_gateway ? 0 : count.index)))))}"
}

resource "aws_nat_gateway" "this" {
count = "${var.create_vpc && var.enable_nat_gateway ? (var.single_nat_gateway ? 1 : length(var.azs)) : 0}"
count = "${var.create_vpc && var.enable_nat_gateway ? local.nat_gateway_count : 0}"
Copy link
Member

Choose a reason for hiding this comment

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

Same here


allocation_id = "${element(local.nat_gateway_ips, (var.single_nat_gateway ? 0 : count.index))}"
subnet_id = "${element(aws_subnet.public.*.id, (var.single_nat_gateway ? 0 : count.index))}"
Expand All @@ -222,7 +223,7 @@ resource "aws_nat_gateway" "this" {
}

resource "aws_route" "private_nat_gateway" {
count = "${var.create_vpc && var.enable_nat_gateway ? length(var.private_subnets) : 0}"
count = "${var.create_vpc && var.enable_nat_gateway ? local.nat_gateway_count : 0}"

route_table_id = "${element(aws_route_table.private.*.id, count.index)}"
destination_cidr_block = "0.0.0.0/0"
Expand All @@ -246,7 +247,7 @@ resource "aws_vpc_endpoint" "s3" {
}

resource "aws_vpc_endpoint_route_table_association" "private_s3" {
count = "${var.create_vpc && var.enable_s3_endpoint ? length(var.private_subnets) : 0}"
count = "${var.create_vpc && var.enable_s3_endpoint ? local.nat_gateway_count : 0}"

vpc_endpoint_id = "${aws_vpc_endpoint.s3.id}"
route_table_id = "${element(aws_route_table.private.*.id, count.index)}"
Expand Down Expand Up @@ -276,7 +277,7 @@ resource "aws_vpc_endpoint" "dynamodb" {
}

resource "aws_vpc_endpoint_route_table_association" "private_dynamodb" {
count = "${var.create_vpc && var.enable_dynamodb_endpoint ? length(var.private_subnets) : 0}"
count = "${var.create_vpc && var.enable_dynamodb_endpoint ? local.nat_gateway_count : 0}"

vpc_endpoint_id = "${aws_vpc_endpoint.dynamodb.id}"
route_table_id = "${element(aws_route_table.private.*.id, count.index)}"
Expand All @@ -296,28 +297,28 @@ resource "aws_route_table_association" "private" {
count = "${var.create_vpc && length(var.private_subnets) > 0 ? length(var.private_subnets) : 0}"

subnet_id = "${element(aws_subnet.private.*.id, count.index)}"
route_table_id = "${element(aws_route_table.private.*.id, count.index)}"
route_table_id = "${element(aws_route_table.private.*.id, (var.single_nat_gateway ? 0 : count.index))}"
}

resource "aws_route_table_association" "database" {
count = "${var.create_vpc && length(var.database_subnets) > 0 ? length(var.database_subnets) : 0}"

subnet_id = "${element(aws_subnet.database.*.id, count.index)}"
route_table_id = "${element(aws_route_table.private.*.id, count.index)}"
route_table_id = "${element(aws_route_table.private.*.id, (var.single_nat_gateway ? 0 : count.index))}"
Copy link
Member

Choose a reason for hiding this comment

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

It does not feel right that aws_route_table_association related to database has anything to do with the value of var.single_nat_gateway even if it is right in this particular piece of code.

Copy link
Contributor Author

@lorengordon lorengordon Feb 19, 2018

Choose a reason for hiding this comment

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

I debated creating a new variable, since yes I am overloading var.single_nat_gateway, but in the end I didn't see it adding value. And this approach simplified some of the logic so I could re-use the local.

Either way I guess. var.single_private_route_table?

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 suppose I could also just create a local named single_private_route_table and set it to var.single_nat_gateway...

}

resource "aws_route_table_association" "redshift" {
count = "${var.create_vpc && length(var.redshift_subnets) > 0 ? length(var.redshift_subnets) : 0}"

subnet_id = "${element(aws_subnet.redshift.*.id, count.index)}"
route_table_id = "${element(aws_route_table.private.*.id, count.index)}"
route_table_id = "${element(aws_route_table.private.*.id, (var.single_nat_gateway ? 0 : count.index))}"
}

resource "aws_route_table_association" "elasticache" {
count = "${var.create_vpc && length(var.elasticache_subnets) > 0 ? length(var.elasticache_subnets) : 0}"

subnet_id = "${element(aws_subnet.elasticache.*.id, count.index)}"
route_table_id = "${element(aws_route_table.private.*.id, count.index)}"
route_table_id = "${element(aws_route_table.private.*.id, (var.single_nat_gateway ? 0 : count.index))}"
}

resource "aws_route_table_association" "public" {
Expand Down