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

Use NAT Gateway Service #213

Merged
merged 5 commits into from
Aug 23, 2023
Merged

Use NAT Gateway Service #213

merged 5 commits into from
Aug 23, 2023

Conversation

ranchodeluxe
Copy link
Contributor

@ranchodeluxe ranchodeluxe commented Aug 23, 2023

Background

https://github.com/NASA-IMPACT/veda-analytics/issues/86

Next Steps

  • deploy with a separate base_name and make sure the network is setup correctly. Maybe even manually provision a Lambda in it to do the public test that is failing on GHGC
  • test this change against the veda-shared-base/network/vpc in UAH
  • Then promote it to GHGC downstream fork and test there

anayeaye and others added 2 commits August 15, 2023 12:12
Release base infrastructure and dashboard postgres schema fixes
@ranchodeluxe
Copy link
Contributor Author

ranchodeluxe commented Aug 23, 2023

Question: will CDK do a force replacement here of the whole VPC or will do an in-place edit of the existing VPC? Hopefully the latter.

@slesaad
Copy link
Member

slesaad commented Aug 23, 2023

@ranchodeluxe looks pretty good to me, dunno the answer to the question tho. we'll know once we deploy? :D

@ividito
Copy link
Collaborator

ividito commented Aug 23, 2023

Question: will CDK do a force replacement here of the whole VPC or will do an in-place edit of the existing VPC? Hopefully the latter.

cdk diff makes me think that the VPC will change in-place, but the route tables will be recreated (which should be fine)

instance_type=aws_ec2.InstanceType("t3.nano"),
# NOTE: this line automatically creates a NAT Gateway for each AZ
# and binds the route table in the private subnet
subnet_type=aws_ec2.SubnetType.PRIVATE_WITH_NAT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get a new warning here:

[WARNING] aws-cdk-lib.aws_ec2.SubnetType#PRIVATE_WITH_NAT is deprecated.
  use `PRIVATE_WITH_EGRESS`
  This API will be removed in the next major release.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ividito I've been wondering about whether we should be switching to PRIVATE_WITH_EGRESS as well. Maybe we can see what changes this PR causes and test the subnet type change separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me make that change and redeploy on my test instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change committed

Copy link
Collaborator

@ividito ividito left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but I'm not 100% sure what the reasoning was for some of the code being removed (is there a reason we would ever need more than one NAT gateway?).

@ranchodeluxe
Copy link
Contributor Author

Mostly LGTM, but I'm not 100% sure what the reasoning was for some of the code being removed (is there a reason we would ever need more than one NAT gateway?).

Are you referring to the number of NAT(s) per AZ here? Or is this a general question about removal of EC2 Instances that function as a NAT and instead using the NAT Gateway?

@ranchodeluxe
Copy link
Contributor Author

Here is my deployed test VPC and test Lambda showing everything is gravy

Before we'd do this we kinda need to know if CDK is going to do a whole teardown here b/c that means the site will be offline and might need to redeploy all services. Let's talk about it at standup

@anayeaye
Copy link
Collaborator

Before the addition of the PRIVATE_WITH_EGRESS change I did some sanity checking:

  • I updated a non-customer facing shared vpc stack that already had a veda-backend instance deployed. The change was made in place (vpc endpoints/route tables were updated and the nat Instance was replaced by a nat gateway)
  • I confirmed basic functionality of the veda-backend instance running in the updated shared vpc after the update was completed

I'll pull the change and run these sanity checks again

@anayeaye
Copy link
Collaborator

No changes caused by changing the subnet type in the construct 👍

@ranchodeluxe
Copy link
Contributor Author

ranchodeluxe commented Aug 23, 2023

Mostly LGTM, but I'm not 100% sure what the reasoning was for some of the code being removed (is there a reason we would ever need more than one NAT gateway?).

Are you referring to the number of NAT(s) per AZ here? Or is this a general question about removal of EC2 Instances that function as a NAT and instead using the NAT Gateway?

I actually think @ividito has it right (of course he does!). I know everyone loves CDK here but there's too much magic. The default behavior here is that CDK magically creates a NAT Gateway for each AZ. I looked at my Terraform for all VPCs I use and we can easily just use one for both private subnets. But that means I have manually do all the plumbing. The reason they do that makes sense b/c they are thinking about fail over.

Thoughts @ividito or @anayeaye? Should I make that change?

@ividito
Copy link
Collaborator

ividito commented Aug 23, 2023

Are you referring to the number of NAT(s) per AZ here? Or is this a general question about removal of EC2 Instances that function as a NAT and instead using the NAT Gateway?

I was talking about removing the config parameter + related code: vpc_nat_gateways: Optional[int] = 1. I wasn't sure if this was still configurable behavior we need to support. The new way seems more appropriate, but I know we sometimes have unique networking requirements in different accounts.

@anayeaye
Copy link
Collaborator

anayeaye commented Aug 23, 2023

Sorry I didn't notice there was a still a discussion going when I approved. RE multiple nat gateway count--it is still configurable via aws-cdk and it looks like it determines the number of gateways per availability zone. I suppose leaving the configuration in place with default=1 gives us that option later?

If we're still on a roll, we could address this, too, I suppose

WARNING] aws-cdk-lib.aws_ec2.VpcProps#cidr is deprecated.
  Use ipAddresses instead
  This API will be removed in the next major release.

^I'm not requesting this as a condition for the PR, though

@ranchodeluxe
Copy link
Contributor Author

@ividito, @anayeaye: my most recent commits make everything groovy:

  1. I added back in nat_gateways=base_settings.vpc_nat_gateways, and it did what I wanted and make both private subnets point to the NAT Gateway (only one this time)

  2. I couldn't find the right incantation to fix this warning: [WARNING] aws-cdk-lib.aws_ec2.VpcProps#cidr is deprecated. Use ipAddresses instead but we can do that later

@ranchodeluxe ranchodeluxe merged commit d6601ed into develop Aug 23, 2023
3 checks passed
@anayeaye anayeaye deleted the feature/nat_gateway_service branch September 19, 2023 16:45
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.

None yet

4 participants