From 3359b2d23da17e65531e9418e11e7f298efd1731 Mon Sep 17 00:00:00 2001 From: James Fleming Date: Tue, 11 Aug 2020 01:20:51 +0000 Subject: [PATCH 1/2] fix(ec2): subnet AZ evaluation for imported subnet --- packages/@aws-cdk/aws-ec2/lib/vpc.ts | 18 ++++---- packages/@aws-cdk/aws-ec2/test/vpc.test.ts | 48 ++++++++++++++-------- 2 files changed, 43 insertions(+), 23 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/lib/vpc.ts b/packages/@aws-cdk/aws-ec2/lib/vpc.ts index daaba4cd8a05f..882832d59adbf 100644 --- a/packages/@aws-cdk/aws-ec2/lib/vpc.ts +++ b/packages/@aws-cdk/aws-ec2/lib/vpc.ts @@ -1,5 +1,5 @@ import * as cxschema from '@aws-cdk/cloud-assembly-schema'; -import { ConcreteDependable, Construct, ContextProvider, DependableTrait, IConstruct, +import { ConcreteDependable, Construct, ContextProvider, DependableTrait, Fn, IConstruct, IDependable, IResource, Lazy, Resource, Stack, Tag, Token } from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; import { @@ -1884,7 +1884,7 @@ class ImportedSubnet extends Resource implements ISubnet, IPublicSubnet, IPrivat public readonly internetConnectivityEstablished: IDependable = new ConcreteDependable(); public readonly subnetId: string; public readonly routeTable: IRouteTable; - private readonly _availabilityZone?: string; + private readonly _availabilityZone: string; constructor(scope: Construct, id: string, attrs: SubnetAttributes) { super(scope, id); @@ -1897,8 +1897,16 @@ class ImportedSubnet extends Resource implements ISubnet, IPublicSubnet, IPrivat scope.node.addWarning(`No routeTableId was provided to the subnet ${ref}. Attempting to read its .routeTable.routeTableId will return null/undefined. (More info: https://github.com/aws/aws-cdk/pull/3171)`); } - this._availabilityZone = attrs.availabilityZone; + if(!attrs.availabilityZone) { + const ref = Token.isUnresolved(attrs.subnetId) + ? `at '${scope.node.path}/${id}'` + : `'${attrs.subnetId}'`; + // eslint-disable-next-line max-len + scope.node.addWarning(`No availabilityZone was provided to the subnet ${ref}. Attempting to read its availabilityZone will return a CFN reference to the AZ, not a string`); + } + this.subnetId = attrs.subnetId; + this._availabilityZone = attrs.availabilityZone ?? Fn.getAtt(this.subnetId, 'AvailabilityZone').toString(); this.routeTable = { // Forcing routeTableId to pretend non-null to maintain backwards-compatibility. See https://github.com/aws/aws-cdk/pull/3171 routeTableId: attrs.routeTableId!, @@ -1906,10 +1914,6 @@ class ImportedSubnet extends Resource implements ISubnet, IPublicSubnet, IPrivat } public get availabilityZone(): string { - if (!this._availabilityZone) { - // eslint-disable-next-line max-len - throw new Error("You cannot reference a Subnet's availability zone if it was not supplied. Add the availabilityZone when importing using Subnet.fromSubnetAttributes()"); - } return this._availabilityZone; } diff --git a/packages/@aws-cdk/aws-ec2/test/vpc.test.ts b/packages/@aws-cdk/aws-ec2/test/vpc.test.ts index 113a8eb7edd56..48703661d3ba2 100644 --- a/packages/@aws-cdk/aws-ec2/test/vpc.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/vpc.test.ts @@ -1,4 +1,5 @@ import { countResources, expect, haveResource, haveResourceLike, isSuperObject, MatchStyle } from '@aws-cdk/assert'; +import { Role, ServicePrincipal } from '@aws-cdk/aws-iam'; import { CfnOutput, Lazy, Stack, Tag } from '@aws-cdk/core'; import { nodeunitShim, Test } from 'nodeunit-shim'; import { AclCidr, AclTraffic, CfnSubnet, CfnVPC, DefaultInstanceTenancy, GenericLinuxImage, InstanceType, InterfaceVpcEndpoint, @@ -1261,30 +1262,30 @@ nodeunitShim({ test.done(); }, - 'Referencing AZ throws error when subnet created from subnetId'(test: Test) { + 'Referencing AZ returns CFN reference when subnet created from subnetId'(test: Test) { // GIVEN const stack = getTestStack(); // WHEN const subnet = Subnet.fromSubnetId(stack, 'subnet1', 'pub-1'); - - // THEN - // eslint-disable-next-line max-len - test.throws(() => subnet.availabilityZone, "You cannot reference a Subnet's availability zone if it was not supplied. Add the availabilityZone when importing using Subnet.fromSubnetAttributes()"); - test.done(); - }, - - 'Referencing AZ throws error when subnet created from attributes without az'(test: Test) { - // GIVEN - const stack = getTestStack(); - - // WHEN - const subnet = Subnet.fromSubnetAttributes(stack, 'subnet1', { subnetId: 'pub-1', availabilityZone: '' }); + // Doesn't matter if it's a role, just need something to resolve the subnet.availabilityZone + // token to a string we can test against. + new Role(stack, 'role', { + assumedBy: new ServicePrincipal('lambda.amazonaws.com'), + description: subnet.availabilityZone, + }); // THEN test.deepEqual(subnet.subnetId, 'pub-1'); - // eslint-disable-next-line max-len - test.throws(() => subnet.availabilityZone, "You cannot reference a Subnet's availability zone if it was not supplied. Add the availabilityZone when importing using Subnet.fromSubnetAttributes()"); + expect(stack).to(haveResource('AWS::IAM::Role', { + Description: { + 'Fn::GetAtt': [ + 'pub-1', + 'AvailabilityZone', + ], + }, + })); + test.done(); }, @@ -1365,6 +1366,21 @@ nodeunitShim({ test.done(); }, + 'SubnetSelection doesnt throw error when selecting imported subnets'(test: Test) { + // GIVEN + const stack = getTestStack(); + + // WHEN + const vpc = new Vpc(stack, 'VPC'); + + // THEN + test.doesNotThrow(() => vpc.selectSubnets({ + subnets: [ + Subnet.fromSubnetId(stack, 'Subnet', 'sub-1'), + ], + })); + test.done(); + }, }, }); From e7a0df1b2abdf99c49d54cfe4e276f1ac674b25c Mon Sep 17 00:00:00 2001 From: James Fleming Date: Thu, 13 Aug 2020 13:24:35 +0000 Subject: [PATCH 2/2] review feedback --- packages/@aws-cdk/aws-ec2/lib/vpc.ts | 20 +++++-------- packages/@aws-cdk/aws-ec2/test/vpc.test.ts | 34 ++++++++++------------ 2 files changed, 24 insertions(+), 30 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/lib/vpc.ts b/packages/@aws-cdk/aws-ec2/lib/vpc.ts index 882832d59adbf..55dbd5cb53533 100644 --- a/packages/@aws-cdk/aws-ec2/lib/vpc.ts +++ b/packages/@aws-cdk/aws-ec2/lib/vpc.ts @@ -1,5 +1,5 @@ import * as cxschema from '@aws-cdk/cloud-assembly-schema'; -import { ConcreteDependable, Construct, ContextProvider, DependableTrait, Fn, IConstruct, +import { ConcreteDependable, Construct, ContextProvider, DependableTrait, IConstruct, IDependable, IResource, Lazy, Resource, Stack, Tag, Token } from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; import { @@ -345,7 +345,7 @@ abstract class VpcBase extends Resource implements IVpc { return { subnetIds: subnets.map(s => s.subnetId), - availabilityZones: subnets.map(s => s.availabilityZone), + get availabilityZones(): string[] { return subnets.map(s => s.availabilityZone); }, internetConnectivityEstablished: tap(new CompositeDependable(), d => subnets.forEach(s => d.add(s.internetConnectivityEstablished))), subnets, hasPublic: subnets.some(s => pubs.has(s)), @@ -1884,7 +1884,7 @@ class ImportedSubnet extends Resource implements ISubnet, IPublicSubnet, IPrivat public readonly internetConnectivityEstablished: IDependable = new ConcreteDependable(); public readonly subnetId: string; public readonly routeTable: IRouteTable; - private readonly _availabilityZone: string; + private readonly _availabilityZone?: string; constructor(scope: Construct, id: string, attrs: SubnetAttributes) { super(scope, id); @@ -1897,16 +1897,8 @@ class ImportedSubnet extends Resource implements ISubnet, IPublicSubnet, IPrivat scope.node.addWarning(`No routeTableId was provided to the subnet ${ref}. Attempting to read its .routeTable.routeTableId will return null/undefined. (More info: https://github.com/aws/aws-cdk/pull/3171)`); } - if(!attrs.availabilityZone) { - const ref = Token.isUnresolved(attrs.subnetId) - ? `at '${scope.node.path}/${id}'` - : `'${attrs.subnetId}'`; - // eslint-disable-next-line max-len - scope.node.addWarning(`No availabilityZone was provided to the subnet ${ref}. Attempting to read its availabilityZone will return a CFN reference to the AZ, not a string`); - } - + this._availabilityZone = attrs.availabilityZone; this.subnetId = attrs.subnetId; - this._availabilityZone = attrs.availabilityZone ?? Fn.getAtt(this.subnetId, 'AvailabilityZone').toString(); this.routeTable = { // Forcing routeTableId to pretend non-null to maintain backwards-compatibility. See https://github.com/aws/aws-cdk/pull/3171 routeTableId: attrs.routeTableId!, @@ -1914,6 +1906,10 @@ class ImportedSubnet extends Resource implements ISubnet, IPublicSubnet, IPrivat } public get availabilityZone(): string { + if (!this._availabilityZone) { + // eslint-disable-next-line max-len + throw new Error("You cannot reference a Subnet's availability zone if it was not supplied. Add the availabilityZone when importing using Subnet.fromSubnetAttributes()"); + } return this._availabilityZone; } diff --git a/packages/@aws-cdk/aws-ec2/test/vpc.test.ts b/packages/@aws-cdk/aws-ec2/test/vpc.test.ts index 48703661d3ba2..08bade38865bc 100644 --- a/packages/@aws-cdk/aws-ec2/test/vpc.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/vpc.test.ts @@ -1,5 +1,4 @@ import { countResources, expect, haveResource, haveResourceLike, isSuperObject, MatchStyle } from '@aws-cdk/assert'; -import { Role, ServicePrincipal } from '@aws-cdk/aws-iam'; import { CfnOutput, Lazy, Stack, Tag } from '@aws-cdk/core'; import { nodeunitShim, Test } from 'nodeunit-shim'; import { AclCidr, AclTraffic, CfnSubnet, CfnVPC, DefaultInstanceTenancy, GenericLinuxImage, InstanceType, InterfaceVpcEndpoint, @@ -1262,30 +1261,30 @@ nodeunitShim({ test.done(); }, - 'Referencing AZ returns CFN reference when subnet created from subnetId'(test: Test) { + 'Referencing AZ throws error when subnet created from subnetId'(test: Test) { // GIVEN const stack = getTestStack(); // WHEN const subnet = Subnet.fromSubnetId(stack, 'subnet1', 'pub-1'); - // Doesn't matter if it's a role, just need something to resolve the subnet.availabilityZone - // token to a string we can test against. - new Role(stack, 'role', { - assumedBy: new ServicePrincipal('lambda.amazonaws.com'), - description: subnet.availabilityZone, - }); // THEN - test.deepEqual(subnet.subnetId, 'pub-1'); - expect(stack).to(haveResource('AWS::IAM::Role', { - Description: { - 'Fn::GetAtt': [ - 'pub-1', - 'AvailabilityZone', - ], - }, - })); + // eslint-disable-next-line max-len + test.throws(() => subnet.availabilityZone, "You cannot reference a Subnet's availability zone if it was not supplied. Add the availabilityZone when importing using Subnet.fromSubnetAttributes()"); + test.done(); + }, + 'Referencing AZ throws error when subnet created from attributes without az'(test: Test) { + // GIVEN + const stack = getTestStack(); + + // WHEN + const subnet = Subnet.fromSubnetAttributes(stack, 'subnet1', { subnetId: 'pub-1', availabilityZone: '' }); + + // THEN + test.deepEqual(subnet.subnetId, 'pub-1'); + // eslint-disable-next-line max-len + test.throws(() => subnet.availabilityZone, "You cannot reference a Subnet's availability zone if it was not supplied. Add the availabilityZone when importing using Subnet.fromSubnetAttributes()"); test.done(); }, @@ -1365,7 +1364,6 @@ nodeunitShim({ })); test.done(); }, - 'SubnetSelection doesnt throw error when selecting imported subnets'(test: Test) { // GIVEN const stack = getTestStack();