chore(elbv2/ecs): clarify cross-stack ELB usage (aws#4667)

* chore(elbv2/ecs): clarify cross-stack ELB usage

Add a note to ELB's and ECS's READMEs to indicate that cross-stack
ELB usage will look different from the simple examples, and link
to a working example in the `aws-cdk-examples` repository.

Move `addTarget()` onto `I***TargetGroup` so that its usage can follow
common cross-construct usage patterns, and throw an appropriate error
when registration cannot work for imported target groups.

Add better validation/messaging around some mistakes I made while
putting together the example.

Fixes aws#4408.

* Update READMEs, restore parameter for backwards compatibility

* Fix code introduced in parallel

packages/@aws-cdk/aws-ecs/README.md

@@ -314,6 +314,21 @@ service.registerLoadBalancerTargets(
 );
 ```
 
+### Using a Load Balancer from a different Stack
+
+If you want to put your Load Balancer and the Service it is load balancing to in
+different stacks, you may not be able to use the convenience methods
+`loadBalancer.addListener()` and `listener.addTargets()`.
+
+The reason is that these methods will create resources in the same Stack as the
+object they're called on, which may lead to cyclic references between stacks.
+Instead, you will have to create an `ApplicationListener` in the service stack,
+or an empty `TargetGroup` in the load balancer stack that you attach your
+service to.
+
+See the [ecs/cross-stack-load-balancer example](https://github.com/aws-samples/aws-cdk-examples/tree/master/typescript/ecs/cross-stack-load-balancer/)
+for the alternatives.
+
 ### Include a classic load balancer
 `Services` can also be directly attached to a classic load balancer as targets:
 
@@ -445,7 +460,7 @@ Currently Supported Log Drivers:
 - journald
 - json-file
 - splunk
-- syslog  
+- syslog
 
 ### awslogs Log Driver
 

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

@@ -416,7 +416,7 @@ export class ContainerDefinition extends cdk.Construct {
     this.portMappings.push(...portMappings.map(pm => {
       if (this.taskDefinition.networkMode === NetworkMode.AWS_VPC || this.taskDefinition.networkMode === NetworkMode.HOST) {
         if (pm.containerPort !== pm.hostPort && pm.hostPort !== undefined) {
-          throw new Error(`Host port ${pm.hostPort} does not match container port ${pm.containerPort}.`);
+          throw new Error(`Host port (${pm.hostPort}) must be left out or equal to container port ${pm.containerPort} for network mode ${this.taskDefinition.networkMode}`);
         }
       }
 

packages/@aws-cdk/aws-elasticloadbalancingv2/README.md

@@ -208,6 +208,22 @@ listener.addTargets('AppFleet', {
 listener.connections.allowFrom(lb, ec2.Port.tcp(8088));
 ```
 
+### Using a Load Balancer from a different Stack
+
+If you want to put your Load Balancer and the Targets it is load balancing to in
+different stacks, you may not be able to use the convenience methods
+`loadBalancer.addListener()` and `listener.addTargets()`.
+
+The reason is that these methods will create resources in the same Stack as the
+object they're called on, which may lead to cyclic references between stacks.
+Instead, you will have to create an `ApplicationListener` in the target stack,
+or an empty `TargetGroup` in the load balancer stack that you attach your
+service to.
+
+For an example of the alternatives while load balancing to an ECS service, see the
+[ecs/cross-stack-load-balancer
+example](https://github.com/aws-samples/aws-cdk-examples/tree/master/typescript/ecs/cross-stack-load-balancer/).
+
 ### Protocol for Load Balancer Targets
 
 Constructs that want to be a load balancer target should implement

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts

@@ -3,7 +3,7 @@ import ec2 = require('@aws-cdk/aws-ec2');
 import { Construct, Duration, IConstruct } from '@aws-cdk/core';
 import {
   BaseTargetGroupProps, ITargetGroup, loadBalancerNameFromListenerArn, LoadBalancerTargetProps,
-  TargetGroupBase, TargetGroupImportProps
+  TargetGroupAttributes, TargetGroupBase, TargetGroupImportProps
 } from '../shared/base-target-group';
 import { ApplicationProtocol, Protocol, TargetType } from '../shared/enums';
 import { ImportedTargetGroupBase } from '../shared/imported';
@@ -70,8 +70,17 @@ export class ApplicationTargetGroup extends TargetGroupBase implements IApplicat
   /**
    * Import an existing target group
    */
+  public static fromTargetGroupAttributes(scope: Construct, id: string, attrs: TargetGroupAttributes): IApplicationTargetGroup {
+    return new ImportedApplicationTargetGroup(scope, id, attrs);
+  }
+
+  /**
+   * Import an existing target group
+   *
+   * @deprecated Use `fromTargetGroupAttributes` instead
+   */
   public static import(scope: Construct, id: string, props: TargetGroupImportProps): IApplicationTargetGroup {
-    return new ImportedApplicationTargetGroup(scope, id, props);
+    return ApplicationTargetGroup.fromTargetGroupAttributes(scope, id, props);
   }
 
   private readonly connectableMembers: ConnectableMember[];
@@ -352,6 +361,11 @@ export interface IApplicationTargetGroup extends ITargetGroup {
    * Don't call this directly. It will be called by load balancing targets.
    */
   registerConnectable(connectable: ec2.IConnectable, portRange?: ec2.Port): void;
+
+  /**
+   * Add a load balancing target to this target group
+   */
+  addTarget(...targets: IApplicationLoadBalancerTarget[]): void;
 }
 
 /**
@@ -366,6 +380,16 @@ class ImportedApplicationTargetGroup extends ImportedTargetGroupBase implements
   public registerConnectable(_connectable: ec2.IConnectable, _portRange?: ec2.Port | undefined): void {
     this.node.addWarning(`Cannot register connectable on imported target group -- security groups might need to be updated manually`);
   }
+
+  public addTarget(...targets: IApplicationLoadBalancerTarget[]) {
+    for (const target of targets) {
+      const result = target.attachToApplicationTargetGroup(this);
+
+      if (result.targetJson !== undefined) {
+        throw new Error('Cannot add a non-self registering target to an imported TargetGroup. Create a new TargetGroup instead.');
+      }
+    }
+  }
 }
 
 /**

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/nlb/network-target-group.ts

@@ -1,6 +1,6 @@
 import cdk = require('@aws-cdk/core');
 import { BaseTargetGroupProps, HealthCheck, ITargetGroup, loadBalancerNameFromListenerArn, LoadBalancerTargetProps,
-         TargetGroupBase, TargetGroupImportProps } from '../shared/base-target-group';
+         TargetGroupAttributes, TargetGroupBase, TargetGroupImportProps } from '../shared/base-target-group';
 import { Protocol } from '../shared/enums';
 import { ImportedTargetGroupBase } from '../shared/imported';
 import { INetworkListener } from './network-listener';
@@ -37,11 +37,20 @@ export interface NetworkTargetGroupProps extends BaseTargetGroupProps {
  * Define a Network Target Group
  */
 export class NetworkTargetGroup extends TargetGroupBase implements INetworkTargetGroup {
+  /**
+   * Import an existing target group
+   */
+  public static fromTargetGroupAttributes(scope: cdk.Construct, id: string, attrs: TargetGroupAttributes): INetworkTargetGroup {
+    return new ImportedNetworkTargetGroup(scope, id, attrs);
+  }
+
   /**
    * Import an existing listener
+   *
+   * @deprecated Use `fromTargetGroupAttributes` instead
    */
   public static import(scope: cdk.Construct, id: string, props: TargetGroupImportProps): INetworkTargetGroup {
-    return new ImportedNetworkTargetGroup(scope, id, props);
+    return NetworkTargetGroup.fromTargetGroupAttributes(scope, id, props);
   }
 
   private readonly listeners: INetworkListener[];
@@ -139,6 +148,11 @@ export interface INetworkTargetGroup extends ITargetGroup {
    * Don't call this directly. It will be called by listeners.
    */
   registerListener(listener: INetworkListener): void;
+
+  /**
+   * Add a load balancing target to this target group
+   */
+  addTarget(...targets: INetworkLoadBalancerTarget[]): void;
 }
 
 /**
@@ -148,6 +162,15 @@ class ImportedNetworkTargetGroup extends ImportedTargetGroupBase implements INet
   public registerListener(_listener: INetworkListener) {
     // Nothing to do, we know nothing of our members
   }
+
+  public addTarget(...targets: INetworkLoadBalancerTarget[]) {
+    for (const target of targets) {
+      const result = target.attachToNetworkTargetGroup(this);
+      if (result.targetJson !== undefined) {
+        throw new Error('Cannot add a non-self registering target to an imported TargetGroup. Create a new TargetGroup instead.');
+      }
+    }
+  }
 }
 
 /**

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/base-target-group.ts

@@ -301,7 +301,11 @@ export abstract class TargetGroupBase extends cdk.Construct implements ITargetGr
   protected validate(): string[]  {
     const ret = super.validate();
 
-    if (this.targetType !== undefined && this.targetType !== TargetType.LAMBDA && this.vpc === undefined) {
+    if (this.targetType === undefined && this.targetsJson.length === 0) {
+      this.node.addWarning(`When creating an empty TargetGroup, you should specify a 'targetType' (this warning may become an error in the future).`);
+    }
+
+    if (this.targetType !== TargetType.LAMBDA && this.vpc === undefined) {
       ret.push(`'vpc' is required for a non-Lambda TargetGroup`);
     }
 
@@ -312,23 +316,33 @@ export abstract class TargetGroupBase extends cdk.Construct implements ITargetGr
 /**
  * Properties to reference an existing target group
  */
-export interface TargetGroupImportProps {
+export interface TargetGroupAttributes {
   /**
    * ARN of the target group
    */
   readonly targetGroupArn: string;
 
   /**
    * Port target group is listening on
+   *
+   * @deprecated - This property is unused and the wrong type. No need to use it.
    */
-  readonly defaultPort: string;
+  readonly defaultPort?: string;
 
   /**
    * A Token representing the list of ARNs for the load balancer routing to this target group
    */
   readonly loadBalancerArns?: string;
 }
 
+/**
+ * Properties to reference an existing target group
+ *
+ * @deprecated Use TargetGroupAttributes instead
+ */
+export interface TargetGroupImportProps extends TargetGroupAttributes {
+}
+
 /**
  * A target group
  */

packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.target-group.ts

@@ -0,0 +1,56 @@
+import ec2 = require('@aws-cdk/aws-ec2');
+import cdk = require('@aws-cdk/core');
+import { Test } from 'nodeunit';
+import elbv2 = require('../../lib');
+import { InstanceTarget } from '../../lib';
+import { FakeSelfRegisteringTarget } from '../helpers';
+
+export = {
+  'Empty target Group without type still requires a VPC'(test: Test) {
+    // GIVEN
+    const app = new cdk.App();
+    const stack = new cdk.Stack(app, 'Stack');
+
+    // WHEN
+    new elbv2.ApplicationTargetGroup(stack, 'LB', {});
+
+    // THEN
+    test.throws(() => {
+      app.synth();
+    }, /'vpc' is required for a non-Lambda TargetGroup/);
+
+    test.done();
+  },
+
+  'Can add self-registering target to imported TargetGroup'(test: Test) {
+    // GIVEN
+    const app = new cdk.App();
+    const stack = new cdk.Stack(app, 'Stack');
+    const vpc = new ec2.Vpc(stack, 'Vpc');
+
+    // WHEN
+    const tg = elbv2.ApplicationTargetGroup.fromTargetGroupAttributes(stack, 'TG', {
+      targetGroupArn: 'arn'
+    });
+    tg.addTarget(new FakeSelfRegisteringTarget(stack, 'Target', vpc));
+
+    // THEN
+    test.done();
+  },
+
+  'Cannot add direct target to imported TargetGroup'(test: Test) {
+    // GIVEN
+    const app = new cdk.App();
+    const stack = new cdk.Stack(app, 'Stack');
+    const tg = elbv2.ApplicationTargetGroup.fromTargetGroupAttributes(stack, 'TG', {
+      targetGroupArn: 'arn'
+    });
+
+    // WHEN
+    test.throws(() => {
+      tg.addTarget(new InstanceTarget('i-1234'));
+    }, /Cannot add a non-self registering target to an imported TargetGroup/);
+
+    test.done();
+  }
+};

packages/@aws-cdk/aws-elasticloadbalancingv2/test/helpers.ts

@@ -23,4 +23,4 @@ export class FakeSelfRegisteringTarget extends cdk.Construct implements elbv2.IA
   public attachToNetworkTargetGroup(_targetGroup: elbv2.NetworkTargetGroup): elbv2.LoadBalancerTargetProps {
     return { targetType: elbv2.TargetType.INSTANCE };
   }
-}
+}

packages/@aws-cdk/aws-elasticloadbalancingv2/test/nlb/test.target-group.ts

@@ -1,4 +1,5 @@
 import { expect, haveResource } from '@aws-cdk/assert';
+import ec2 = require('@aws-cdk/aws-ec2');
 import cdk = require('@aws-cdk/core');
 import { Test } from 'nodeunit';
 import elbv2 = require('../../lib');
@@ -7,11 +8,13 @@ export = {
   'Enable proxy protocol v2 attribute for target group'(test: Test) {
     // GIVEN
     const stack = new cdk.Stack();
+    const vpc = new ec2.Vpc(stack, 'Vpc');
 
     // WHEN
     new elbv2.NetworkTargetGroup(stack, 'Group', {
-        port: 80,
-        proxyProtocolV2: true
+      vpc,
+      port: 80,
+      proxyProtocolV2: true
     });
 
     // THEN
@@ -30,11 +33,13 @@ export = {
   'Disable proxy protocol v2 for attribute target group'(test: Test) {
     // GIVEN
     const stack = new cdk.Stack();
+    const vpc = new ec2.Vpc(stack, 'Vpc');
 
     // WHEN
     new elbv2.NetworkTargetGroup(stack, 'Group', {
-        port: 80,
-        proxyProtocolV2: false
+      vpc,
+      port: 80,
+      proxyProtocolV2: false
     });
 
     // THEN