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

[laurelmay]

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:

• [X] 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

[gitpod-io[bot]]

[laurelmay]

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]

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]

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).

[TheRealAmazonKendra]

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.

[laurelmay]

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 99bdea48c002ee4e4c9ac6605ac3cc4fee6e4331 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.

[TheRealAmazonKendra]

@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.

[laurelmay]

@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]

On it!

[TheRealAmazonKendra]

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.

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[mergify[bot]]

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]

AWS CodeBuild CI Report

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

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