fix(ec2): 'encoded list token' error using Vpc imported from deploy-t…

…ime lists (#12040)

Even though using `Vpc.fromVpcAttributes()` using deploy-time lists like
from `Fn.importValue()`s and `CfnParameters` was never really supposed
to work, it accidentally did.

The reason for that is:

```ts

// Encoded list token
const subnetIds = Token.asList(Fn.importValue('someValue'))
// [ '#{Token[1234]}' ]

// Gets parsed to a singleton `Subnet` list:
const subnets = subnetIds.map(s => Subnet.fromSubnetAttributes({ subnetId: s }));
// [ Subnet({ subnetId: '#{Token[1234]}' }) ]

// This 'subnetId' is illegal by itself, and if yould try to use it for,
// say, an ec2.Instance it would fail. However, if you treat this single
// subnet as a GROUP of subnets:
new CfnAutoScalingGroup({ subnetIds: subnets.map(s => s.subnetId) })
// [ '#{Token[1234]}' ]

// And this resolves back to:
resolve(cfnSubnetIds)
// SubnetIds: { Fn::ImportValue: 'someValue' }
```

--------

We introduced an additional check in #11899 to make sure that the
list-element token that represents an encoded list (`'#{Token[1234]}'`)
never occurs in a non-list context, because it's illegal there.

However, because:

* `Subnet.fromSubnetAttributes()` logs the subnetId as a *warning*
  to its own metadata (which will log a string like `"there's something
  wrong with '#{Token[1234]}' ..."`).
* All metadata is resolved just the same as the template expressions
  are.

The `resolve()` function encounters that orphaned list token in the
metadata and throws.

--------

The *proper* solution would be to handle unparseable list tokens
specially to never get into this situation, but doing that requires
introducing classes and new concepts that will be a large effort and
not be backwards compatible. Tracking in #4118.

Another possible solution is to stop resolving metadata. I don't
know if we usefully use this feature; I think we don't. However,
we have tests enforcing that it is being done, and I'm scared
to break something there.

The quick solution around this for now is to have
`Subnet.fromSubnetAttributes()` recognize when it's about to log
a problematic identifier to metadata, and don't do it.

Fixes #11945.

----

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

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');
 }

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

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();