Optional private subnet creation #127
Conversation
alexjurkiewicz
force-pushed
the
optional-private-public
branch
from
fd4c7da
to
ac4d191
Compare
mergify
bot
dismissed
Nuru’s stale review
This Pull Request has been updated, so we're dismissing all reviews.
|
Thanks for the feedback. Agreed on all points and updated. I'll add some tests if you agree with the design 👍 |
|
This pull request is now in conflict. Could you fix it @alexjurkiewicz? 🙏 |
Previously, var.context.enabled / var.enabled was ignored, and public subnets were always created.
alexjurkiewicz
force-pushed
the
optional-private-public
branch
from
0227cc6
to
315403e
Compare
|
ping 🙏 |
alexjurkiewicz
force-pushed
the
optional-private-public
branch
from
4790d78
to
f7ee59e
Compare
This lets users create only private (or only public) subnets. The CIDR range is still divided as per usual, so you can change your mind later with minimal disruption.
alexjurkiewicz
force-pushed
the
optional-private-public
branch
2 times, most recently
from
a2f546c
to
d13a86f
Compare
alexjurkiewicz
force-pushed
the
optional-private-public
branch
from
d13a86f
to
53c2dc5
Compare
|
This pull request is now in conflict. Could you fix it @alexjurkiewicz? 🙏 |
There was a problem hiding this comment.
I think this module stops being useful if you are not creating both public and private subnets. Almost all of the benefit of this module comes from setting up the communication and routing between the private and public subnets, and if you only create private subnets they are not useful because they will be completely isolated.
| @@ -136,3 +136,15 @@ variable "root_block_device_encrypted" { | |||
| default = true | |||
| description = "Whether to encrypt the root block device" | |||
| } | |||
|
|
|||
| variable "create_public_subnets" { | |||
There was a problem hiding this comment.
Cloud Posse convention is to use *_enabled for boolean flags
| variable "create_public_subnets" { | |
| variable "public_subnets_enabled" { |
| description = "Set to false to prevent the module from creating public subnets. Some of your CIDR block will still be reserved, so you can change this later without disruption." | ||
| } | ||
|
|
||
| variable "create_private_subnets" { |
There was a problem hiding this comment.
Cloud Posse convention is to use *_enabled for boolean flags
| variable "create_private_subnets" { | |
| variable "private_subnets_enabled" { |
| @@ -15,7 +15,7 @@ locals { | |||
| } | |||
|
|
|||
| resource "aws_eip" "default" { | |||
| count = local.enabled ? local.nat_gateway_eip_count : 0 | |||
| count = local.public_enabled ? local.nat_gateway_eip_count : 0 | |||
There was a problem hiding this comment.
This raises the question of whether or not we should even allow this module to create only private subnets, since they would have no means of egress.
In any case, this is wrong, because we only can and need to create a NAT gateway or instance if we have both public and private subnets.
There was a problem hiding this comment.
yes, NAT gets created in public subnets.
Without NAT, the private subnets can't communicate with anything (and many applications will not work b/c of that).
@alexjurkiewicz what's the use case for having only private (and completely isolated from everything) subnets?
Especially taking into account that the module will always still reserve space in the CIDR block for both subnet types.
Nuru
changed the title
|
@alexjurkiewicz
|
|
This pull request is now in conflict. Could you fix it @alexjurkiewicz? 🙏 |
|
I am not deploying our private subnets with this module any more. For the record, we created private subnets for data lake-related services which should not access the internet. |
what
var.create_public_subnetsandvar.create_private_subnetswhy