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

vpc: PRIVATE_WITH_EGRESS not getting applied with route to TransitGateway #25626

Closed
bruecktech opened this issue May 17, 2023 · 3 comments · Fixed by #25958
Closed

vpc: PRIVATE_WITH_EGRESS not getting applied with route to TransitGateway #25626

bruecktech opened this issue May 17, 2023 · 3 comments · Fixed by #25958
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@bruecktech
Copy link
Contributor

Describe the bug

When I do Vpc.fromLookup in a VPC that has subnets with a default route to a TransitGateway the subnet is detected as PRIVATE_ISOLATED instead of PRIVATE_WITH_EGRESS

This PR that introduced it talks about TransitGateway too but it doesn't seem to get applied.

Expected Behavior

Detect subnet with a default route to a TransitGateway as PRIVATE_WITH_EGRESS

Current Behavior

Subnets with a default route to a TransitGateway are detected as PRIVATE_ISOLATED

Reproduction Steps

Use Vpc.fromLookup in a VPC that has subnets with a default route to a TransitGateway

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.66

Framework Version

No response

Node.js Version

16.16

OS

MacOS

Language

Typescript

Language Version

No response

Other information

No response

@bruecktech bruecktech added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 17, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label May 17, 2023
@peterwoodworth
Copy link
Contributor

peterwoodworth commented May 18, 2023

You're right, I'm finding the same behavior.

When we set the type of imported subnets, we are first checking that the subnet has the type explicitly declared with a tag. If it doesn't, then we will cross reference with the describeRouteTable response to check if the subnet is associated with a route table.

const subnets: Subnet[] = listedSubnets.map(subnet => {
let type = getTag('aws-cdk:subnet-type', subnet.Tags);
if (type === undefined && subnet.MapPublicIpOnLaunch) { type = SubnetType.Public; }
if (type === undefined && routeTables.hasRouteToIgw(subnet.SubnetId)) { type = SubnetType.Public; }
if (type === undefined && routeTables.hasRouteToNatGateway(subnet.SubnetId)) { type = SubnetType.Private; }
if (type === undefined) { type = SubnetType.Isolated; }

This doesn't account for transit gateway route tables, which are separate. And unfortunately, it doesn't appear we can fetch subnet information from this call. Maybe there's another way we could detect this?

@peterwoodworth peterwoodworth added p1 effort/medium Medium work item – several days of effort needs-review and removed needs-triage This issue or PR still needs to be triaged. labels May 18, 2023
@bruecktech
Copy link
Contributor Author

From what I understand we don't actually need to look at TransitGateway route tables, but just incorporate routes to TransitGatewayIds in the regular route tables.

This is how the route looks like in the describe-route-table CLI command

{
    "DestinationCidrBlock": "0.0.0.0/0",
    "TransitGatewayId": "tgw-02261c6cd4750fc3c",
    "Origin": "CreateRoute",
    "State": "active"
},

So I think what's missing is something like this before line 77

if (type === undefined && routeTables.hasRouteToTransitGateway(subnet.SubnetId)) { type = SubnetType.Private; }

whereas hasRouteToTransitGateway could be implemented as

  public hasRouteToTransitGateway(subnetId: string | undefined): boolean {
    const table = this.tableForSubnet(subnetId) || this.mainRouteTable;

    return !!table && !!table.Routes && table.Routes.some(route => !!route.TransitGatewayId && route.DestinationCidrBlock === '0.0.0.0/0');
  }

@mergify mergify bot closed this as completed in #25958 Jun 15, 2023
mergify bot pushed a commit that referenced this issue Jun 15, 2023
When I do Vpc.fromLookup in a VPC that has subnets with a default route to a TransitGateway the subnet is detected as `PRIVATE_ISOLATED` instead of `PRIVATE_WITH_EGRESS`
This PR adds the detection of subnets with TGW routes as `PRIVATE_WITH_EGRESS` instead of `PRIVATE_ISOLATED`.

This is potentially a breaking change depending on what is the expected behaviour. To me it seemed rather missed accidentally given that this [previous PR](#21699) mentions that `PRIVATE_WITH_EGRESS` was introduced also for Transit Gateways.

Closes #25626

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants