diff --git a/packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts b/packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts index 737566c369b76..ae3be33401f71 100644 --- a/packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts +++ b/packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts @@ -1351,6 +1351,45 @@ test('Can set autoScalingGroupName', () => { })); }); +test('can use Vpc imported from unparseable list tokens', () => { + // GIVEN + const stack = new cdk.Stack(); + + const vpcId = cdk.Fn.importValue('myVpcId'); + const availabilityZones = cdk.Fn.split(',', cdk.Fn.importValue('myAvailabilityZones')); + const publicSubnetIds = cdk.Fn.split(',', cdk.Fn.importValue('myPublicSubnetIds')); + const privateSubnetIds = cdk.Fn.split(',', cdk.Fn.importValue('myPrivateSubnetIds')); + const isolatedSubnetIds = cdk.Fn.split(',', cdk.Fn.importValue('myIsolatedSubnetIds')); + + const vpc = ec2.Vpc.fromVpcAttributes(stack, 'importedVpc', { + vpcId, + availabilityZones, + publicSubnetIds, + privateSubnetIds, + isolatedSubnetIds, + }); + + // WHEN + new autoscaling.AutoScalingGroup(stack, 'ecs-ec2-asg', { + instanceType: new ec2.InstanceType('t2.micro'), + machineImage: new ec2.AmazonLinuxImage(), + minCapacity: 1, + maxCapacity: 1, + desiredCapacity: 1, + vpc, + allowAllOutbound: false, + associatePublicIpAddress: false, + vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE }, + }); + + // THEN + expect(stack).to(haveResourceLike('AWS::AutoScaling::AutoScalingGroup', { + VPCZoneIdentifier: { + 'Fn::Split': [',', { 'Fn::ImportValue': 'myPrivateSubnetIds' }], + }, + })); +}); + function mockSecurityGroup(stack: cdk.Stack) { return ec2.SecurityGroup.fromSecurityGroupId(stack, 'MySG', 'most-secure'); } diff --git a/packages/@aws-cdk/aws-ec2/lib/vpc.ts b/packages/@aws-cdk/aws-ec2/lib/vpc.ts index f6ce634b18a75..f642f131d5a70 100644 --- a/packages/@aws-cdk/aws-ec2/lib/vpc.ts +++ b/packages/@aws-cdk/aws-ec2/lib/vpc.ts @@ -1010,7 +1010,16 @@ export class Vpc extends VpcBase { ]; /** - * Import an exported VPC + * Import a VPC by supplying all attributes directly + * + * NOTE: using `fromVpcAttributes()` with deploy-time parameters (like a `Fn.importValue()` or + * `CfnParameter` to represent a list of subnet IDs) sometimes accidentally works. It happens + * to work for constructs that need a list of subnets (like `AutoScalingGroup` and `eks.Cluster`) + * but it does not work for constructs that need individual subnets (like + * `Instance`). See https://github.com/aws/aws-cdk/issues/4118 for more + * information. + * + * Prefer to use `Vpc.fromLookup()` instead. */ public static fromVpcAttributes(scope: Construct, id: string, attrs: VpcAttributes): IVpc { return new ImportedVpc(scope, id, attrs, false); @@ -1927,7 +1936,21 @@ class ImportedSubnet extends Resource implements ISubnet, IPublicSubnet, IPrivat super(scope, id); if (!attrs.routeTableId) { - const ref = Token.isUnresolved(attrs.subnetId) + // The following looks a little weird, but comes down to: + // + // * Is the subnetId itself unresolved ({ Ref: Subnet }); or + // * Was it the accidentally extracted first element of a list-encoded + // token? ({ Fn::ImportValue: Subnets } => ['#{Token[1234]}'] => + // '#{Token[1234]}' + // + // There's no other API to test for the second case than to the string back into + // a list and see if the combination is Unresolved. + // + // In both cases we can't output the subnetId literally into the metadata (because it'll + // be useless). In the 2nd case even, if we output it to metadata, the `resolve()` call + // that gets done on the metadata will even `throw`, because the '#{Token}' value will + // occur in an illegal position (not in a list context). + const ref = Token.isUnresolved(attrs.subnetId) || Token.isUnresolved([attrs.subnetId]) ? `at '${Node.of(scope).path}/${id}'` : `'${attrs.subnetId}'`; // eslint-disable-next-line max-len diff --git a/packages/@aws-cdk/aws-ec2/test/vpc.test.ts b/packages/@aws-cdk/aws-ec2/test/vpc.test.ts index 9ea01086411dd..0b9401f289119 100644 --- a/packages/@aws-cdk/aws-ec2/test/vpc.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/vpc.test.ts @@ -1,5 +1,5 @@ import { countResources, expect, haveResource, haveResourceLike, isSuperObject, MatchStyle } from '@aws-cdk/assert'; -import { CfnOutput, Lazy, Stack, Tags } from '@aws-cdk/core'; +import { CfnOutput, CfnResource, Fn, Lazy, Stack, Tags } from '@aws-cdk/core'; import { nodeunitShim, Test } from 'nodeunit-shim'; import { AclCidr, AclTraffic, BastionHostLinux, CfnSubnet, CfnVPC, SubnetFilter, DefaultInstanceTenancy, GenericLinuxImage, @@ -1227,6 +1227,36 @@ nodeunitShim({ test.done(); }, + 'fromVpcAttributes using imported refs'(test: Test) { + // GIVEN + const stack = getTestStack(); + + const vpcId = Fn.importValue('myVpcId'); + const availabilityZones = Fn.split(',', Fn.importValue('myAvailabilityZones')); + const publicSubnetIds = Fn.split(',', Fn.importValue('myPublicSubnetIds')); + + // WHEN + const vpc = Vpc.fromVpcAttributes(stack, 'VPC', { + vpcId, + availabilityZones, + publicSubnetIds, + }); + + new CfnResource(stack, 'Resource', { + type: 'Some::Resource', + properties: { + subnetIds: vpc.selectSubnets().subnetIds, + }, + }); + + // THEN - No exception + expect(stack).to(haveResource('Some::Resource', { + subnetIds: { 'Fn::Split': [',', { 'Fn::ImportValue': 'myPublicSubnetIds' }] }, + })); + + test.done(); + }, + 'select explicitly defined subnets'(test: Test) { // GIVEN const stack = getTestStack();