fix(ec2): Internet connectivity not established for private subnets #21495

laurelmay · 2022-08-08T05:11:51Z

Because private subnets rely on a NAT Gateway for internet connectivity,
it is important that the NAT Gateway have the necessary dependencies for
its own internet connectivity. Otherwise,
internetConnectivityEstablished on a private subnet may not be true
during stack creation and deletion. This is most notable for
CloudFormaton Custom Resources; however, it can result in other
dependency failures during stack deletion, especially if resources
within a private subnet take a long time to delete.

Ensuring that the NAT Gateway depends on its public subnet having
internet connectivity completes the chain of dependencies and ensures
that all resources will correctly have internet connectivity.

Because of the layers of abstraction around subnets and NAT gateways,
unit tests for this feature are challenging (because there isn't a clear
means to get the CloudFormaton Logical ID of the AWS::EC2::Route that
establishes the connectivity); however, NAT Gateways are included in
several integration tests so this dependency can be tested there.

Closes: #21348

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Because private subnets rely on a NAT Gateway for internet connectivity, it is important that the NAT Gateway have the necessary dependencies for its own internet connectivity. Otherwise, `internetConnectivityEstablished` on a private subnet may not be true during stack creation and deletion. This is most notable for CloudFormaton Custom Resources; however, it can result in other dependency failures during stack deletion, especially if resources within a private subnet take a long time to delete. Ensuring that the NAT Gateway depends on its public subnet having internet connectivity completes the chain of dependencies and ensures that all resources will correctly have internet connectivity. Because of the layers of abstraction around subnets and NAT gateways, unit tests for this feature are challenging (because there isn't a clear means to get the CloudFormaton Logical ID of the AWS::EC2::Route that establishes the connectivity); however, NAT Gateways are included in several integration tests so this dependency can be tested there.

gitpod-io · 2022-08-08T05:11:59Z

laurelmay · 2022-08-08T12:04:58Z

It looks like this ended up failing because of changes in snapshots for integration tests in other packages than @aws-cdk/aws-ec2. Unfortunately, there are several there whose pricing models I am unfamiliar with and I don't really want to spin up a bunch of resources I don't understand to fully integration test them, especially in light of #21496.

I'd be happy to find all the relevant integration tests and run them using --dry-run if that'd be acceptable. But if not, based on snapshot template contents the following packages would need to be yarn build+test && yarn integ --update-on-failed and I'd need support with that:

$ grep -l  "AWS::EC2::NatGateway"  **/*template*.json | cut -d'/' -f1-2 | uniq

@aws-cdk/aws-apigateway
@aws-cdk/aws-apigatewayv2-integrations
@aws-cdk/aws-appmesh
@aws-cdk/aws-apprunner
@aws-cdk/aws-autoscaling
@aws-cdk/aws-batch
@aws-cdk/aws-cloud9
@aws-cdk/aws-cloudfront-origins
@aws-cdk/aws-codebuild
@aws-cdk/aws-codedeploy
@aws-cdk/aws-codepipeline-actions
@aws-cdk/aws-docdb
@aws-cdk/aws-ec2
@aws-cdk/aws-ecs-patterns
@aws-cdk/aws-ecs
@aws-cdk/aws-efs
@aws-cdk/aws-eks-legacy
@aws-cdk/aws-eks
@aws-cdk/aws-elasticloadbalancing
@aws-cdk/aws-elasticloadbalancingv2-actions
@aws-cdk/aws-elasticloadbalancingv2-targets
@aws-cdk/aws-elasticloadbalancingv2
@aws-cdk/aws-elasticsearch
@aws-cdk/aws-events-targets
@aws-cdk/aws-fsx
@aws-cdk/aws-globalaccelerator-endpoints
@aws-cdk/aws-glue
@aws-cdk/aws-lambda-nodejs
@aws-cdk/aws-lambda
@aws-cdk/aws-msk
@aws-cdk/aws-neptune
@aws-cdk/aws-opensearchservice
@aws-cdk/aws-rds
@aws-cdk/aws-redshift
@aws-cdk/aws-route53resolver
@aws-cdk/aws-route53-targets
@aws-cdk/aws-route53
@aws-cdk/aws-s3-deployment
@aws-cdk/aws-servicediscovery
@aws-cdk/aws-stepfunctions-tasks
@aws-cdk/aws-synthetics
@aws-cdk-containers/ecs-service-extensions
@aws-cdk/pipelines

TheRealAmazonKendra · 2022-08-08T15:36:21Z

We don't want to just do a dry run because we won't see if the deployment of the new template actually succeeds. I'll go ahead and download this PR and run the tests and upload the changes. I'll then have you take a look and ensure it matches the output you'd expect based off the changes you made. In the meantime, I'll take an initial pass through your code changes this morning.

TheRealAmazonKendra · 2022-08-08T15:41:24Z

Oh, looks like this is just a one line change. I'll upload the tests. I see there's conversation going on in the issue so I'll let @corymhall or @rix0rrr do the actual review on this one (this is also very much not my area of expertise).

rix0rrr

This is clever, and you are very correct that this is a missing dependency.

But I think we need a couple dependencies more to make sure we get rid of all race conditions, no?

Function to internetConnectivityEstablished
internetConnectivityEstablished needs to include the RouteTableAssociation

Would you mind adding those as well, while you're in here anyway?

TheRealAmazonKendra · 2022-08-09T22:36:37Z

Crying on the inside for finally having finished updating all the integ tests

Just kidding. I'll keep these test updates stashed until the requested changes have been made and then I'll rerun the ones needed. @kylelaker please assign this to me and/or tag me when it's ready for integ test updates and I'll get them uploaded.

…ablished Internet connectivity is not truly available until the subnet route table has been properly associated. This needs to be around for the life of any resources that depend on internet connectivity.

Pull request has been modified.

This reverts commit 99bdea4.

laurelmay · 2022-08-10T03:19:28Z

Alright! I think I've made the requested changes. Subnet's internetConnectivityEstablished now depends on the Route Table Association. A NAT Gateway now depends on its subnet's internetConnectivityEstablished. And all Functions within a VPC now depend on their subnets' internetConnectivityEstablished.

I also reverted 99bdea4 which added (now incorrect) updated results for integration tests.

I think the list of packages that need to have integration tests run to have the outputs updated is:

@aws-cdk/aws-apigateway
@aws-cdk/aws-apigatewayv2-integrations
@aws-cdk/aws-appmesh
@aws-cdk/aws-apprunner
@aws-cdk/aws-autoscaling
@aws-cdk/aws-batch
@aws-cdk/aws-cloud9
@aws-cdk/aws-cloudfront-origins
@aws-cdk/aws-codebuild
@aws-cdk/aws-codedeploy
@aws-cdk/aws-codepipeline-actions
@aws-cdk/aws-docdb
@aws-cdk/aws-ec2
@aws-cdk/aws-ecs
@aws-cdk/aws-ecs-patterns
@aws-cdk/aws-efs
@aws-cdk/aws-eks
@aws-cdk/aws-eks-legacy
@aws-cdk/aws-elasticloadbalancing
@aws-cdk/aws-elasticloadbalancingv2
@aws-cdk/aws-elasticloadbalancingv2-actions
@aws-cdk/aws-elasticloadbalancingv2-targets
@aws-cdk/aws-elasticsearch
@aws-cdk/aws-events-targets
@aws-cdk/aws-fsx
@aws-cdk/aws-globalaccelerator-endpoints
@aws-cdk/aws-glue
@aws-cdk/aws-lambda
@aws-cdk/aws-lambda-nodejs
@aws-cdk/aws-lambda-python
@aws-cdk/aws-msk
@aws-cdk/aws-neptune
@aws-cdk/aws-opensearchservice
@aws-cdk/aws-rds
@aws-cdk/aws-redshift
@aws-cdk/aws-route53
@aws-cdk/aws-route53resolver
@aws-cdk/aws-route53-targets
@aws-cdk/aws-s3-deployment
@aws-cdk/aws-servicediscovery
@aws-cdk/aws-stepfunctions-tasks
@aws-cdk/aws-synthetics
@aws-cdk-containers/ecs-service-extensions
@aws-cdk/custom-resources
@aws-cdk/pipelines

That list is based off running yarn integ and ignoring failures that were caused by integ not being a command.

…et-dependency

TheRealAmazonKendra · 2022-08-10T03:37:35Z

@rix0rrr Can you review and make sure this looks right to you before I add the test updates? They're quite a lot so I don't want the real code changes to get lost in those.

rix0rrr

Thanks for the changes! One more thing?

rix0rrr · 2022-08-10T14:45:31Z

packages/@aws-cdk/aws-lambda/lib/function.ts

@@ -1101,6 +1101,8 @@ Environment variables can be marked for removal when used in Lambda@Edge by sett
      return undefined;
    }

+    this.node.addDependency(props.vpc.selectSubnets(props.vpcSubnets).internetConnectivityEstablished);


Can we call selectSubnets once, and use the result for both the subnetIds and the dependency?

In other words, add the dependnecy where selectSubnets is already being called?

This also gets rid of the destructing assignment to `subnetIds` to make it a little more clear what's going on when we're also getting public subnets and what exactly `internetConnectivityEstablished` means, since we're now using two properties and there are several subnet lists flying around.

Pull request has been modified.

laurelmay · 2022-08-10T17:20:26Z

@TheRealAmazonKendra It looks like I am not able to assign to you nor to directly request review; but I think this should be ready for the integration tests. Thank you so much for helping with those!

TheRealAmazonKendra · 2022-08-10T17:37:40Z

On it!

Pull request has been modified.

TheRealAmazonKendra · 2022-08-11T04:56:44Z

Good news/bad news is this: we have a lot of broken tests but I don't think they're broken from this change so I'm uploading them in batches as I verify that they're good. If they need a fix that is unrelated to this change, I'll PR that fix separately first so it doesn't get mixed in with these updates.

mergify · 2022-08-18T20:15:57Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra · 2022-08-19T17:14:38Z

@Mergifyio update

mergify · 2022-08-19T17:15:12Z

update

✅ Branch has been successfully updated

…kylelaker/aws-cdk into pr/21495

TheRealAmazonKendra

Finally!!!

mergify · 2022-08-22T05:55:26Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-08-22T06:35:28Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 3ba573b
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-08-22T06:35:43Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…ws#21495) Because private subnets rely on a NAT Gateway for internet connectivity, it is important that the NAT Gateway have the necessary dependencies for its own internet connectivity. Otherwise, `internetConnectivityEstablished` on a private subnet may not be true during stack creation and deletion. This is most notable for CloudFormaton Custom Resources; however, it can result in other dependency failures during stack deletion, especially if resources within a private subnet take a long time to delete. Ensuring that the NAT Gateway depends on its public subnet having internet connectivity completes the chain of dependencies and ensures that all resources will correctly have internet connectivity. Because of the layers of abstraction around subnets and NAT gateways, unit tests for this feature are challenging (because there isn't a clear means to get the CloudFormaton Logical ID of the AWS::EC2::Route that establishes the connectivity); however, NAT Gateways are included in several integration tests so this dependency can be tested there. Closes: aws#21348 ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

vleandersson · 2022-08-31T15:30:11Z

This is amazing! Nice to come to completed work when finding a bug 🥳

I'm not fully sure about CDKs publishing process, but do we know when this will be available in the npm cdk package @TheRealAmazonKendra ?

laurelmay · 2022-08-31T15:32:08Z

@vleandersson It looks like it was included in v2.39.0, according to the last "bug fix" bullet point

github-actions bot added the p2 label Aug 8, 2022

aws-cdk-automation requested a review from a team August 8, 2022 05:12

laurelmay mentioned this pull request Aug 8, 2022

(integ-runner): Runner builds only the old version of integration tests for metadata-only changes #21496

Closed

Add integration test results

99bdea4

laurelmay marked this pull request as ready for review August 8, 2022 12:05

rix0rrr previously requested changes Aug 9, 2022

View reviewed changes

laurelmay added 2 commits August 9, 2022 22:05

Add dependency on route table association for internetConnectivityEst…

92051b6

…ablished Internet connectivity is not truly available until the subnet route table has been properly associated. This needs to be around for the life of any resources that depend on internet connectivity.

Make VPC-attached Lambdas depend on internetConnectivityEstablished

20a3659

Revert "Add integration test results"

90b1a65

This reverts commit 99bdea4.

Merge branch 'main' of github.com:aws/aws-cdk into nat-gateway-intern…

00434d3

…et-dependency

rix0rrr previously requested changes Aug 10, 2022

View reviewed changes

rix0rrr previously approved these changes Aug 10, 2022

View reviewed changes

TheRealAmazonKendra self-assigned this Aug 10, 2022

update ec2 integ tests

ede3662

update synthetics integ tests

57281a5

mergify bot and others added 4 commits August 18, 2022 20:15

Merge branch 'main' into nat-gateway-internet-dependency

ea048a5

re-update ec2 integ tests

45b7518

re-update msk integ tests

8491598

update batch integ tests

0b54154

mergify bot and others added 13 commits August 19, 2022 17:15

Merge branch 'main' into nat-gateway-internet-dependency

0777197

Merge branch 'nat-gateway-internet-dependency' of https://github.com/…

428e096

…kylelaker/aws-cdk into pr/21495

update ecs integ tests

76dd4c8

update event-targets integ tests

b462ded

update ecs-service-extensions integ tests

2684446

update stepfunctions integ tests

e31eb4e

update codepipeline-actions integ tests

0e58001

update stepfunctions-tasks integ tests

09256a1

update ecs-patterns integ tests

d6e6d6b

update elasticloadbalancingv2-targets integ tests

4b5aea0

update rds integ tests

e4afcdf

update pipelines integ tests

bb9fa7f

remove unintended change to ecs-patterns test

ab4521a

TheRealAmazonKendra approved these changes Aug 22, 2022

View reviewed changes

Merge branch 'main' into nat-gateway-internet-dependency

3ba573b

mergify bot merged commit 5b1488d into aws:main Aug 22, 2022

laurelmay mentioned this pull request Aug 23, 2022

feat(custom-resource): allow AwsCustomResource to be placed in vpc #21357

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ec2): Internet connectivity not established for private subnets #21495

fix(ec2): Internet connectivity not established for private subnets #21495

laurelmay commented Aug 8, 2022 •

edited

Loading

gitpod-io bot commented Aug 8, 2022

laurelmay commented Aug 8, 2022

TheRealAmazonKendra commented Aug 8, 2022

TheRealAmazonKendra commented Aug 8, 2022

rix0rrr left a comment

TheRealAmazonKendra commented Aug 9, 2022

laurelmay commented Aug 10, 2022

TheRealAmazonKendra commented Aug 10, 2022

rix0rrr left a comment

rix0rrr Aug 10, 2022

laurelmay commented Aug 10, 2022

TheRealAmazonKendra commented Aug 10, 2022

TheRealAmazonKendra commented Aug 11, 2022

mergify bot commented Aug 18, 2022

TheRealAmazonKendra commented Aug 19, 2022

mergify bot commented Aug 19, 2022

TheRealAmazonKendra left a comment

mergify bot commented Aug 22, 2022

aws-cdk-automation commented Aug 22, 2022

mergify bot commented Aug 22, 2022

vleandersson commented Aug 31, 2022 •

edited

Loading

laurelmay commented Aug 31, 2022 •

edited

Loading

fix(ec2): Internet connectivity not established for private subnets #21495

fix(ec2): Internet connectivity not established for private subnets #21495

Conversation

laurelmay commented Aug 8, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Aug 8, 2022

laurelmay commented Aug 8, 2022

TheRealAmazonKendra commented Aug 8, 2022

TheRealAmazonKendra commented Aug 8, 2022

rix0rrr left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Aug 9, 2022

laurelmay commented Aug 10, 2022

TheRealAmazonKendra commented Aug 10, 2022

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Aug 10, 2022

Choose a reason for hiding this comment

laurelmay commented Aug 10, 2022

TheRealAmazonKendra commented Aug 10, 2022

TheRealAmazonKendra commented Aug 11, 2022

mergify bot commented Aug 18, 2022

✅ Branch has been successfully updated

TheRealAmazonKendra commented Aug 19, 2022

mergify bot commented Aug 19, 2022

✅ Branch has been successfully updated

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 22, 2022

aws-cdk-automation commented Aug 22, 2022

AWS CodeBuild CI Report

mergify bot commented Aug 22, 2022

vleandersson commented Aug 31, 2022 • edited Loading

laurelmay commented Aug 31, 2022 • edited Loading

laurelmay commented Aug 8, 2022 •

edited

Loading

vleandersson commented Aug 31, 2022 •

edited

Loading

laurelmay commented Aug 31, 2022 •

edited

Loading