Skip to content

Conversation

@nnsense
Copy link

@nnsense nnsense commented Apr 14, 2022

what

  • Add the option to deploy a single NGW instead of multiple across AZs

why

  • Save money during development phase.

@nnsense nnsense requested review from a team as code owners April 14, 2022 10:28
@jamengual
Copy link
Contributor

/test all

aknysh
aknysh previously requested changes Apr 15, 2022
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@nnsense thank you for the PR
Please see comments.

can you please run the following command

make init
make github/init
make readme

and commit the changes

@osterman
Copy link
Member

Why even bother running in multiple AZs if you only deploy a single NGW? If you lose the AZ with the NGW, all other AZs lose egress. If the concern is cost, then reduce the number of AZs, not the number of NGWs. It's a false sense of redundancy.

What's the counter argument?

@jamengual
Copy link
Contributor

Why even bother running in multiple AZs if you only deploy a single NGW? If you lose the AZ with the NGW, all other AZs lose egress. If the concern is cost, then reduce the number of AZs, not the number of NGWs. It's a false sense of redundancy.

What's the counter argument?

In accounts like audit, security, etc you might do more management than deploying services and on those, you might not even have critical services in them but you might need to have a way to connect to the internet but not highly available (like for sending reports, analytics etc)

In an account that you use for back end services connected through a TGW and you have multiple RDS clusters that are highly available but the egress is through the TGW and not the NGW you might not need that many NGWs

in companies with low budgets.

@mergify mergify bot dismissed aknysh’s stale review April 15, 2022 14:05

This Pull Request has been updated, so we're dismissing all reviews.

@nnsense
Copy link
Author

nnsense commented Apr 15, 2022

Hi @osterman , that is of course a good point, but in our case we didn't want to change the infrastructure, just pay for a single NGW while developing with a single server which we had the chance to run in HA for testing (still using a single NGW) and the reduce back to single. At the time we needed to save money, and I've been using this branch to deploy a single NGW

@jamengual
Copy link
Contributor

/test all

@jamengual
Copy link
Contributor

@nnsense can you address the comment?

@nnsense
Copy link
Author

nnsense commented Apr 16, 2022

Hi @jamengual - Sorry I'm a bit lost, what comment I need to address?

@jamengual
Copy link
Contributor

jamengual commented Apr 16, 2022 via email

@nnsense
Copy link
Author

nnsense commented Apr 19, 2022

Done, thanks I've missed that comment :)

@jamengual
Copy link
Contributor

/test all

@osterman
Copy link
Member

just pay for a single NGW while developing with a single server which we had the chance to run in HA for testing (still using a single NGW) and the reduce back to single. At the time we needed to save money, and I've been using this branch to deploy a single NGW

In that case, why run more than one subnet if running a single server?

@osterman
Copy link
Member

you might need to have a way to connect to the internet but not highly available (like for sending reports, analytics etc)

Yep, I don't disagree wit the use-case. But then why have multiple AZs instead of single AZ? A single AZ communicates that it's not HA.

@jamengual
Copy link
Contributor

you might need to have a way to connect to the internet but not highly available (like for sending reports, analytics etc)

Yep, I don't disagree wit the use-case. But then why have multiple AZs instead of single AZ? A single AZ communicates that it's not HA.

Erik, if this was an account that has only Aurora RDS clusters and the internet is not the primary ingress then you might not want to run multiple nat gateways, I know this is not what ClousPosse recommends but it is a very common practice and that is why this option is disabled by default.

jamengual
jamengual previously approved these changes Apr 20, 2022
@osterman
Copy link
Member

osterman commented Apr 20, 2022

Erik, if this was an account that has only Aurora RDS clusters and the internet is not the primary ingress then you might not want to run multiple nat gateways,

Okay, I can accept that.

default = true
}

variable "single_nat_gateway" {
Copy link
Member

Choose a reason for hiding this comment

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

IMO, we should change this to max_nat_gateways. There's some code stink about calling it single, when single isn't the primary concern. It's the number of nat gateways to reduce cost. Operating in 5 AZs, it might be desirable to have 2 NAT gateways instead.

@jamengual
Copy link
Contributor

@nnsense please look at the new comments

@mergify mergify bot dismissed jamengual’s stale review May 3, 2022 20:54

This Pull Request has been updated, so we're dismissing all reviews.

@jamengual
Copy link
Contributor

@nnsense can you address the rename change from Erik ( I know we asked for this before, sorry) so we can get this merged? thanks.

@nnsense
Copy link
Author

nnsense commented May 5, 2022

Hi @jamengual @osterman
Sorry for the delay, busy weeks :)
It's actually a good idea, basically we can set the number of NGWs or leave it to default which is == length(var.availability_zones) , I like it! I will change it over the weekend, have a nice one :)
Matt

Nuru added a commit that referenced this pull request May 12, 2022
@Nuru Nuru closed this in #159 May 15, 2022
@nnsense
Copy link
Author

nnsense commented May 17, 2022

@Nuru @jamengual
I've implemented the change, apparently too late. What does "Supersedes" means? Is the mentioned PR covering this change too? If it isnt should I open a new PR?
https://github.com/nnsense/terraform-aws-dynamic-subnets

@nnsense
Copy link
Author

nnsense commented May 18, 2022

@Nuru @jamengual @osterman
I've checked version 2, and I see that the max number of NGW has been implemented. One comment here to notify me would have been great, no one is here to waste time. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants