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

Add NAT gateway scope parameter to control the vpc/az deployment scope the CFN #97

Merged
merged 3 commits into from
Aug 1, 2024

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Jul 30, 2024

Closes https://github.com/elastio/elastio/issues/9476

I didn't remove the existing logic that deploys NAT gateways per AZ (maybe it will be useful in the future). Instead I added an input parameter to the stack to control the deployment scope. The default value is "vpc" scope, meaning there will be just one NAT gateway per VPC.


Another change. Added elastio:resource tags to all resources that support tags.

@Veetaha Veetaha requested a review from anelson July 31, 2024 17:40
Comment on lines +30 to +31
If set to 'az', only one NAT Gateway will be deployed per VPC/AZ. This
results in intra-AZ traffic, but increases the number of NAT gateways.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If set to 'az', only one NAT Gateway will be deployed per VPC/AZ. This
results in intra-AZ traffic, but increases the number of NAT gateways.
If set to 'az', only one NAT Gateway will be deployed per VPC/AZ. This
reduces cross-AZ traffic, but increases the number of NAT gateways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs are correct. az scope means there will be one NAT per AZ, and there will be intra-AZ traffic

Comment on lines +76 to +77
- Key: elastio:resource
Value: 'true'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it the right call to put our tag on these? That means aws-gc will delete the lambdas. That seems like it would not be expected behavior...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it will. We discussed that the Cloudwatch groups should have elastio:resource tag https://github.com/elastio/elastio/issues/6694#issuecomment-2261039044, for us to be able to read. I decided to add this tag for all resources anyway. If we do aws-gc we'll definitely want to start everything from scratch, likely including the NAT stack as well, because if we need to do aws-gc it's probably the case that something in the user's env deletes resources and it already broke both the NAT stack and Connector stack

@Veetaha Veetaha merged commit 1a22dbc into master Aug 1, 2024
5 checks passed
@Veetaha Veetaha deleted the feat/nat-deployment-at-az-level branch August 1, 2024 14:48
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.

2 participants