Creates a single private route table when single_nat_gateway is true#83
Conversation
antonbabenko
left a comment
antonbabenko
left a comment
There was a problem hiding this comment.
Hi @lorengordon ! Welcome back! Overall these changes make it hard to understand (see comment at the bottom of it), as well as maintain and debug. I propose to close this PR even though we are creating duplicate resources (which are free, btw). This module is already a bit too complicated at some places (and there is still no IPv6 and few other features, which will make this code to grow even more... looking forward, hehe).
What was your main reason for this?
|
|
||
| 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}" |
There was a problem hiding this comment.
max_subnet_length gets longest of all 4 subnet types, while NAT should only be applicable to private_subnets
There was a problem hiding this comment.
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.
|
|
||
| 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}" |
There was a problem hiding this comment.
This should take into account number of azs
There was a problem hiding this comment.
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.
|
|
||
| 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))}" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I suppose I could also just create a local named single_private_route_table and set it to var.single_nat_gateway...
|
|
||
| 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}" |
|
I guess I'd disagree that this is more complicated. In some ways, I think it is more clear than the current code. The use of a well-named local helps make it quite clear to me what is happening. And centralizing that logic in the local also makes it easier to maintain. Have been using and maintaining this code in a fork for four months, periodically merging from upstream/master, and it's been quite solid and easy to follow. |
|
It took some time to review and agree with this PR mentally. 👍 Thank you! |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
2 participants
Fixes #82