From 54599e505c7f1072ccf76cca3153bd11f69f3106 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 28 Aug 2019 17:26:48 +0200 Subject: [PATCH] fix(vpc): recognize Public subnets by Internet Gateway (#3784) The VPC importer used to look at `MapPublicIpOnLaunch` as a shortcut for determining whether a Subnet was public or private. The correct way to do it is to look at the route table and see if it includes an Internet Gateway. Fixes #3706. --- .../aws-cdk/lib/context-providers/vpcs.ts | 42 +++- .../test/context-providers/test.vpcs.ts | 181 ++++++++++++------ 2 files changed, 151 insertions(+), 72 deletions(-) diff --git a/packages/aws-cdk/lib/context-providers/vpcs.ts b/packages/aws-cdk/lib/context-providers/vpcs.ts index c29c57f15c34e..e065d89c3b636 100644 --- a/packages/aws-cdk/lib/context-providers/vpcs.ts +++ b/packages/aws-cdk/lib/context-providers/vpcs.ts @@ -47,9 +47,7 @@ export class VpcNetworkContextProviderPlugin implements ContextProviderPlugin { const listedSubnets = subnetsResponse.Subnets || []; const routeTablesResponse = await ec2.describeRouteTables(filters).promise(); - const listedRouteTables = routeTablesResponse.RouteTables || []; - - const mainRouteTable = listedRouteTables.find(table => table.Associations && !!table.Associations.find(assoc => assoc.Main)); + const routeTables = new RouteTables(routeTablesResponse.RouteTables || []); // Now comes our job to separate these subnets out into AZs and subnet groups (Public, Private, Isolated) // We have the following attributes to go on: @@ -64,19 +62,17 @@ export class VpcNetworkContextProviderPlugin implements ContextProviderPlugin { const subnets: Subnet[] = listedSubnets.map(subnet => { let type = getTag('aws-cdk:subnet-type', subnet.Tags); - if (type === undefined) { - type = subnet.MapPublicIpOnLaunch ? SubnetType.Public : SubnetType.Private; - } + if (type === undefined && subnet.MapPublicIpOnLaunch) { type = SubnetType.Public; } + if (type === undefined && routeTables.hasRouteToIgw(subnet.SubnetId)) { type = SubnetType.Public; } + if (type === undefined) { type = SubnetType.Private; } + if (!isValidSubnetType(type)) { // tslint:disable-next-line: max-line-length throw new Error(`Subnet ${subnet.SubnetArn} has invalid subnet type ${type} (must be ${SubnetType.Public}, ${SubnetType.Private} or ${SubnetType.Isolated})`); } const name = getTag('aws-cdk:subnet-name', subnet.Tags) || type; - const specificRouteTable = - listedRouteTables.find(table => table.Associations && !!table.Associations.find(assoc => assoc.SubnetId === subnet.SubnetId)); - const routeTableId = (specificRouteTable && specificRouteTable.RouteTableId) - || (mainRouteTable && mainRouteTable.RouteTableId); + const routeTableId = routeTables.routeTableIdForSubnetId(subnet.SubnetId); if (!routeTableId) { throw new Error(`Subnet ${subnet.SubnetArn} does not have an associated route table (and there is no "main" table)`); @@ -131,6 +127,32 @@ export class VpcNetworkContextProviderPlugin implements ContextProviderPlugin { } } +class RouteTables { + public readonly mainRouteTable?: AWS.EC2.RouteTable; + + constructor(private readonly tables: AWS.EC2.RouteTable[]) { + this.mainRouteTable = this.tables.find(table => !!table.Associations && table.Associations.some(assoc => !!assoc.Main)); + } + + public routeTableIdForSubnetId(subnetId: string | undefined): string | undefined { + const table = this.tableForSubnet(subnetId); + return (table && table.RouteTableId) || (this.mainRouteTable && this.mainRouteTable.RouteTableId); + } + + /** + * Whether the given subnet has a route to an IGW + */ + public hasRouteToIgw(subnetId: string | undefined): boolean { + const table = this.tableForSubnet(subnetId); + + return !!table && !!table.Routes && table.Routes.some(route => !!route.GatewayId && route.GatewayId.startsWith('igw-')); + } + + public tableForSubnet(subnetId: string | undefined) { + return this.tables.find(table => !!table.Associations && table.Associations.some(assoc => assoc.SubnetId === subnetId)); + } +} + /** * Return the value of a tag from a set of tags */ diff --git a/packages/aws-cdk/test/context-providers/test.vpcs.ts b/packages/aws-cdk/test/context-providers/test.vpcs.ts index ad1a12849ac6b..9883d71a7b10c 100644 --- a/packages/aws-cdk/test/context-providers/test.vpcs.ts +++ b/packages/aws-cdk/test/context-providers/test.vpcs.ts @@ -25,35 +25,17 @@ export = nodeunit.testCase({ const filter = { foo: 'bar' }; const provider = new VpcNetworkContextProviderPlugin(mockSDK); - AWS.mock('EC2', 'describeVpcs', (params: aws.EC2.DescribeVpcsRequest, cb: AwsCallback) => { - test.deepEqual(params.Filters, [{ Name: 'foo', Values: ['bar'] }]); - return cb(null, { Vpcs: [{ VpcId: 'vpc-1234567' }] }); - }); - AWS.mock('EC2', 'describeSubnets', (params: aws.EC2.DescribeSubnetsRequest, cb: AwsCallback) => { - test.deepEqual(params.Filters, [{ Name: 'vpc-id', Values: ['vpc-1234567'] }]); - return cb(null, { - Subnets: [ - { SubnetId: 'sub-123456', AvailabilityZone: 'bermuda-triangle-1337', MapPublicIpOnLaunch: true }, - { SubnetId: 'sub-789012', AvailabilityZone: 'bermuda-triangle-1337', MapPublicIpOnLaunch: false } - ] - }); - }); - AWS.mock('EC2', 'describeRouteTables', (params: aws.EC2.DescribeRouteTablesRequest, cb: AwsCallback) => { - test.deepEqual(params.Filters, [{ Name: 'vpc-id', Values: ['vpc-1234567'] }]); - return cb(null, { - RouteTables: [ - { Associations: [{ SubnetId: 'sub-123456' }], RouteTableId: 'rtb-123456', }, - { Associations: [{ SubnetId: 'sub-789012' }], RouteTableId: 'rtb-789012', } - ] - }); - }); - AWS.mock('EC2', 'describeVpnGateways', (params: aws.EC2.DescribeVpnGatewaysRequest, cb: AwsCallback) => { - test.deepEqual(params.Filters, [ - { Name: 'attachment.vpc-id', Values: [ 'vpc-1234567' ] }, - { Name: 'attachment.state', Values: [ 'attached' ] }, - { Name: 'state', Values: [ 'available' ] } - ]); - return cb(null, { VpnGateways: [{ VpnGatewayId: 'gw-abcdef' }] }); + mockVpcLookup(test, { + subnets: [ + { SubnetId: 'sub-123456', AvailabilityZone: 'bermuda-triangle-1337', MapPublicIpOnLaunch: true }, + { SubnetId: 'sub-789012', AvailabilityZone: 'bermuda-triangle-1337', MapPublicIpOnLaunch: false } + ], + routeTables: [ + { Associations: [{ SubnetId: 'sub-123456' }], RouteTableId: 'rtb-123456', }, + { Associations: [{ SubnetId: 'sub-789012' }], RouteTableId: 'rtb-789012', } + ], + vpnGateways: [{ VpnGatewayId: 'gw-abcdef' }] + }); // WHEN @@ -75,8 +57,8 @@ export = nodeunit.testCase({ vpnGatewayId: 'gw-abcdef' }); - test.done(); AWS.restore(); + test.done(); }, async 'throws when no such VPC is found'(test: nodeunit.Test) { @@ -97,8 +79,8 @@ export = nodeunit.testCase({ test.throws(() => { throw e; }, /Could not find any VPCs matching/); } - test.done(); AWS.restore(); + test.done(); }, async 'throws when multiple VPCs are found'(test: nodeunit.Test) { @@ -119,8 +101,8 @@ export = nodeunit.testCase({ test.throws(() => { throw e; }, /Found 2 VPCs matching/); } - test.done(); AWS.restore(); + test.done(); }, async 'uses the VPC main route table when a subnet has no specific association'(test: nodeunit.Test) { @@ -128,35 +110,16 @@ export = nodeunit.testCase({ const filter = { foo: 'bar' }; const provider = new VpcNetworkContextProviderPlugin(mockSDK); - AWS.mock('EC2', 'describeVpcs', (params: aws.EC2.DescribeVpcsRequest, cb: AwsCallback) => { - test.deepEqual(params.Filters, [{ Name: 'foo', Values: ['bar'] }]); - return cb(null, { Vpcs: [{ VpcId: 'vpc-1234567' }] }); - }); - AWS.mock('EC2', 'describeSubnets', (params: aws.EC2.DescribeSubnetsRequest, cb: AwsCallback) => { - test.deepEqual(params.Filters, [{ Name: 'vpc-id', Values: ['vpc-1234567'] }]); - return cb(null, { - Subnets: [ - { SubnetId: 'sub-123456', AvailabilityZone: 'bermuda-triangle-1337', MapPublicIpOnLaunch: true }, - { SubnetId: 'sub-789012', AvailabilityZone: 'bermuda-triangle-1337', MapPublicIpOnLaunch: false } - ] - }); - }); - AWS.mock('EC2', 'describeRouteTables', (params: aws.EC2.DescribeRouteTablesRequest, cb: AwsCallback) => { - test.deepEqual(params.Filters, [{ Name: 'vpc-id', Values: ['vpc-1234567'] }]); - return cb(null, { - RouteTables: [ - { Associations: [{ SubnetId: 'sub-123456' }], RouteTableId: 'rtb-123456', }, - { Associations: [{ Main: true }], RouteTableId: 'rtb-789012', } - ] - }); - }); - AWS.mock('EC2', 'describeVpnGateways', (params: aws.EC2.DescribeVpnGatewaysRequest, cb: AwsCallback) => { - test.deepEqual(params.Filters, [ - { Name: 'attachment.vpc-id', Values: [ 'vpc-1234567' ] }, - { Name: 'attachment.state', Values: [ 'attached' ] }, - { Name: 'state', Values: [ 'available' ] } - ]); - return cb(null, { VpnGateways: [{ VpnGatewayId: 'gw-abcdef' }] }); + mockVpcLookup(test, { + subnets: [ + { SubnetId: 'sub-123456', AvailabilityZone: 'bermuda-triangle-1337', MapPublicIpOnLaunch: true }, + { SubnetId: 'sub-789012', AvailabilityZone: 'bermuda-triangle-1337', MapPublicIpOnLaunch: false } + ], + routeTables: [ + { Associations: [{ SubnetId: 'sub-123456' }], RouteTableId: 'rtb-123456', }, + { Associations: [{ Main: true }], RouteTableId: 'rtb-789012', } + ], + vpnGateways: [{ VpnGatewayId: 'gw-abcdef' }] }); // WHEN @@ -180,5 +143,99 @@ export = nodeunit.testCase({ test.done(); AWS.restore(); - } + }, + + async 'Recognize public subnet by route table'(test: nodeunit.Test) { + // GIVEN + const filter = { foo: 'bar' }; + const provider = new VpcNetworkContextProviderPlugin(mockSDK); + + mockVpcLookup(test, { + subnets: [ + { SubnetId: 'sub-123456', AvailabilityZone: 'bermuda-triangle-1337', MapPublicIpOnLaunch: false }, + ], + routeTables: [ + { + Associations: [{ SubnetId: 'sub-123456' }], + RouteTableId: 'rtb-123456', + Routes: [ + { + DestinationCidrBlock: "10.0.2.0/26", + Origin: "CreateRoute", + State: "active", + VpcPeeringConnectionId: "pcx-xxxxxx" + }, + { + DestinationCidrBlock: "10.0.1.0/24", + GatewayId: "local", + Origin: "CreateRouteTable", + State: "active" + }, + { + DestinationCidrBlock: "0.0.0.0/0", + GatewayId: "igw-xxxxxx", + Origin: "CreateRoute", + State: "active" + } + ], + }, + ], + }); + + // WHEN + const result = await provider.getValue({ filter }); + + // THEN + test.deepEqual(result, { + vpcId: 'vpc-1234567', + availabilityZones: ['bermuda-triangle-1337'], + isolatedSubnetIds: undefined, + isolatedSubnetNames: undefined, + isolatedSubnetRouteTableIds: undefined, + privateSubnetIds: undefined, + privateSubnetNames: undefined, + privateSubnetRouteTableIds: undefined, + publicSubnetIds: ['sub-123456'], + publicSubnetNames: ['Public'], + publicSubnetRouteTableIds: ['rtb-123456'], + vpnGatewayId: undefined, + }); + + AWS.restore(); + test.done(); + }, }); + +interface VpcLookupOptions { + subnets: aws.EC2.Subnet[]; + routeTables: aws.EC2.RouteTable[]; + vpnGateways?: aws.EC2.VpnGateway[]; +} + +function mockVpcLookup(test: nodeunit.Test, options: VpcLookupOptions) { + const VpcId = 'vpc-1234567'; + + AWS.mock('EC2', 'describeVpcs', (params: aws.EC2.DescribeVpcsRequest, cb: AwsCallback) => { + test.deepEqual(params.Filters, [{ Name: 'foo', Values: ['bar'] }]); + return cb(null, { Vpcs: [{ VpcId }] }); + }); + + AWS.mock('EC2', 'describeSubnets', (params: aws.EC2.DescribeSubnetsRequest, cb: AwsCallback) => { + test.deepEqual(params.Filters, [{ Name: 'vpc-id', Values: [VpcId] }]); + return cb(null, { Subnets: options.subnets }); + }); + + AWS.mock('EC2', 'describeRouteTables', (params: aws.EC2.DescribeRouteTablesRequest, cb: AwsCallback) => { + test.deepEqual(params.Filters, [{ Name: 'vpc-id', Values: [VpcId] }]); + return cb(null, { RouteTables: options.routeTables }); + }); + + AWS.mock('EC2', 'describeVpnGateways', (params: aws.EC2.DescribeVpnGatewaysRequest, cb: AwsCallback) => { + test.deepEqual(params.Filters, [ + { Name: 'attachment.vpc-id', Values: [ VpcId ] }, + { Name: 'attachment.state', Values: [ 'attached' ] }, + { Name: 'state', Values: [ 'available' ] } + ]); + return cb(null, { VpnGateways: options.vpnGateways }); + }); +} \ No newline at end of file