fix(aws-elbv2): fix load balancer registration (#890)

This change includes the following fixes that came out while using
this library for ECS:

- Fixes a misconception of what TargetType was supposed to represent.
  Directly-registered/self-registering targets and registered-by-IP
  /by-instance-ID are two orthogonal axes that both need configuring.
  Even for self-registering targets, the TargetGroup must have been set
  up with the correct TargetType beforehand.

- The 'open' parameter said it defaulted to 'true' but it actually
  defaulted to 'false'. Make it actually default to 'true' now.

- Make it possible for Targets to depend on the Listener via the
  TargetGroup, even if the TargetGroup only gets attached to the
  Listener later on. This is necessary for targets that can only
  self-attach to a TargetGroup AFTER the TargetGroup has been attached
  to a LoadBalancer (via the Listener). Adding a CloudFormation
  dependency on the Listener makes everything deploy in the right
  order.

Author: rix0rrr

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

@@ -276,15 +276,15 @@ export class AutoScalingGroup extends cdk.Construct implements cdk.ITaggable, el
   public attachToApplicationTargetGroup(targetGroup: elbv2.ApplicationTargetGroup): elbv2.LoadBalancerTargetProps {
     this.targetGroupArns.push(targetGroup.targetGroupArn);
     targetGroup.registerConnectable(this);
-    return { targetType: elbv2.TargetType.SelfRegistering };
+    return { targetType: elbv2.TargetType.Instance };
   }
 
   /**
    * Attach to ELBv2 Application Target Group
    */
   public attachToNetworkTargetGroup(targetGroup: elbv2.NetworkTargetGroup): elbv2.LoadBalancerTargetProps {
     this.targetGroupArns.push(targetGroup.targetGroupArn);
-    return { targetType: elbv2.TargetType.SelfRegistering };
+    return { targetType: elbv2.TargetType.Instance };
   }
 
   /**

packages/@aws-cdk/aws-autoscaling/test/integ.asg-w-elbv2.expected.json

@@ -470,7 +470,7 @@
         "SecurityGroupIngress": [
           {
             "CidrIp": "0.0.0.0/0",
-            "Description": "Open to the world",
+            "Description": "Allow from anyone on port 80",
             "FromPort": 80,
             "IpProtocol": "tcp",
             "ToPort": 80
@@ -526,6 +526,7 @@
       "Properties": {
         "Port": 80,
         "Protocol": "HTTP",
+        "TargetType": "instance",
         "VpcId": {
           "Ref": "VPCB9E5F0B4"
         },
@@ -534,4 +535,4 @@
       }
     }
   }
-}
+}

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

@@ -179,20 +179,27 @@ load balancing target:
 public attachToApplicationTargetGroup(targetGroup: ApplicationTargetGroup): LoadBalancerTargetProps {
     targetGroup.registerConnectable(...);
     return {
-        targetType: TargetType.Instance | TargetType.Ip | TargetType.SelfRegistering,
+        targetType: TargetType.Instance | TargetType.Ip
         targetJson: { id: ..., port: ... },
     };
 }
 ```
-
-`targetType` should be one of `Instance` or `Ip` if the target can be directly
-added to the target group, or `SelfRegistering` if the target will register new
-instances with the load balancer at some later point.
-
-If the `targetType` is `Instance` or `Ip`, `targetJson` should contain the `id`
-of the target (either instance ID or IP address depending on the type) and
+`targetType` should be one of `Instance` or `Ip`. If the target can be
+directly added to the target group, `targetJson` should contain the `id` of
+the target (either instance ID or IP address depending on the type) and
 optionally a `port` or `availabilityZone` override.
 
 Application load balancer targets can call `registerConnectable()` on the
 target group to register themselves for addition to the load balancer's security
 group rules.
+
+If your load balancer target requires that the TargetGroup has been
+associated with a LoadBalancer before registration can happen (such as is the
+case for ECS Services for example), take a resource dependency on
+`targetGroup.listenerDependency()` as follows:
+
+```ts
+// Make sure that the listener has been created, and so the TargetGroup
+// has been associated with the LoadBalancer, before 'resource' is created.
+resourced.addDependency(targetGroup.listenerDependency());
+```

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

@@ -134,7 +134,7 @@ export class ApplicationListener extends BaseListener implements IApplicationLis
 
     (props.defaultTargetGroups || []).forEach(this.addDefaultTargetGroup.bind(this));
 
-    if (props.open) {
+    if (props.open !== false) {
       this.connections.allowDefaultPortFrom(new ec2.AnyIPv4(), `Allow from anyone on port ${port}`);
     }
   }
@@ -258,7 +258,7 @@ export class ApplicationListener extends BaseListener implements IApplicationLis
 /**
  * Properties to reference an existing listener
  */
-export interface IApplicationListener extends ec2.IConnectable {
+export interface IApplicationListener extends ec2.IConnectable, cdk.IDependable {
   /**
    * ARN of the listener
    */
@@ -319,6 +319,7 @@ export interface ApplicationListenerRefProps {
 }
 
 class ImportedApplicationListener extends cdk.Construct implements IApplicationListener {
+  public readonly dependencyElements: cdk.IDependable[] = [];
   public readonly connections: ec2.Connections;
 
   /**

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

@@ -3,7 +3,7 @@ import cdk = require('@aws-cdk/cdk');
 import { BaseTargetGroup, BaseTargetGroupProps, ITargetGroup, LoadBalancerTargetProps, TargetGroupRefProps } from '../shared/base-target-group';
 import { ApplicationProtocol } from '../shared/enums';
 import { BaseImportedTargetGroup } from '../shared/imported';
-import { determineProtocolAndPort } from '../shared/util';
+import { determineProtocolAndPort, LazyDependable } from '../shared/util';
 import { IApplicationListener } from './application-listener';
 
 /**
@@ -144,6 +144,7 @@ export class ApplicationTargetGroup extends BaseTargetGroup {
       listener.registerConnectable(member.connectable, member.portRange);
     }
     this.listeners.push(listener);
+    this.dependableListeners.push(listener);
   }
 }
 
@@ -181,6 +182,10 @@ class ImportedApplicationTargetGroup extends BaseImportedTargetGroup implements
   public registerListener(_listener: IApplicationListener) {
     // Nothing to do, we know nothing of our members
   }
+
+  public listenerDependency(): cdk.IDependable {
+    return new LazyDependable([]);
+  }
 }
 
 /**

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

@@ -113,7 +113,7 @@ export class NetworkListener extends BaseListener implements INetworkListener {
 /**
  * Properties to reference an existing listener
  */
-export interface INetworkListener {
+export interface INetworkListener extends cdk.IDependable {
   /**
    * ARN of the listener
    */
@@ -134,6 +134,8 @@ export interface NetworkListenerRefProps {
  * An imported Network Listener
  */
 class ImportedNetworkListener extends cdk.Construct implements INetworkListener {
+  public readonly dependencyElements: cdk.IDependable[] = [];
+
   /**
    * ARN of the listener
    */

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

@@ -2,6 +2,8 @@ import cdk = require('@aws-cdk/cdk');
 import { BaseTargetGroup, BaseTargetGroupProps, ITargetGroup, LoadBalancerTargetProps, TargetGroupRefProps } from '../shared/base-target-group';
 import { Protocol } from '../shared/enums';
 import { BaseImportedTargetGroup } from '../shared/imported';
+import { LazyDependable } from '../shared/util';
+import { INetworkListener } from './network-listener';
 
 /**
  * Properties for a new Network Target Group
@@ -62,6 +64,15 @@ export class NetworkTargetGroup extends BaseTargetGroup {
       this.addLoadBalancerTarget(result);
     }
   }
+
+  /**
+   * Register a listener that is load balancing to this target group.
+   *
+   * Don't call this directly. It will be called by listeners.
+   */
+  public registerListener(listener: INetworkListener) {
+    this.dependableListeners.push(listener);
+  }
 }
 
 /**
@@ -75,6 +86,9 @@ export interface INetworkTargetGroup extends ITargetGroup {
  * An imported network target group
  */
 class ImportedNetworkTargetGroup extends BaseImportedTargetGroup implements INetworkTargetGroup {
+  public listenerDependency(): cdk.IDependable {
+    return new LazyDependable([]);
+  }
 }
 
 /**

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

@@ -5,7 +5,8 @@ import { ITargetGroup } from './base-target-group';
 /**
  * Base class for listeners
  */
-export abstract class BaseListener extends cdk.Construct {
+export abstract class BaseListener extends cdk.Construct implements cdk.IDependable {
+  public readonly dependencyElements: cdk.IDependable[];
   public readonly listenerArn: string;
   private readonly defaultActions: any[] = [];
 
@@ -17,6 +18,7 @@ export abstract class BaseListener extends cdk.Construct {
       defaultActions: new cdk.Token(() => this.defaultActions),
     });
 
+    this.dependencyElements = [resource];
     this.listenerArn = resource.ref;
   }
 

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

@@ -3,7 +3,7 @@ import ec2 = require('@aws-cdk/aws-ec2');
 import cdk = require('@aws-cdk/cdk');
 import { cloudformation } from '../elasticloadbalancingv2.generated';
 import { Protocol, TargetType } from './enums';
-import { Attributes, renderAttributes } from './util';
+import { Attributes, LazyDependable, renderAttributes } from './util';
 
 /**
  * Basic properties of both Application and Network Target Groups
@@ -146,6 +146,11 @@ export abstract class BaseTargetGroup extends cdk.Construct implements ITargetGr
    */
   protected readonly defaultPort: string;
 
+  /**
+   * List of listeners routing to this target group
+   */
+  protected readonly dependableListeners = new Array<cdk.IDependable>();
+
   /**
    * Attributes of this target group
    */
@@ -242,19 +247,21 @@ export abstract class BaseTargetGroup extends cdk.Construct implements ITargetGr
     };
   }
 
+  /**
+   * Return an object to depend on the listeners added to this target group
+   */
+  public listenerDependency(): cdk.IDependable {
+    return new LazyDependable(this.dependableListeners);
+  }
+
   /**
    * Register the given load balancing target as part of this group
    */
   protected addLoadBalancerTarget(props: LoadBalancerTargetProps) {
-    if ((props.targetType === TargetType.SelfRegistering) !== (props.targetJson === undefined)) {
-      throw new Error('Load balancing target should specify targetJson if and only if TargetType is not SelfRegistering');
-    }
-    if (props.targetType !== TargetType.SelfRegistering) {
-      if (this.targetType !== undefined && this.targetType !== props.targetType) {
-        throw new Error(`Already have a of type '${this.targetType}', adding '${props.targetType}'; make all targets the same type.`);
-      }
-      this.targetType = props.targetType;
+    if (this.targetType !== undefined && this.targetType !== props.targetType) {
+      throw new Error(`Already have a of type '${this.targetType}', adding '${props.targetType}'; make all targets the same type.`);
     }
+    this.targetType = props.targetType;
 
     if (props.targetJson) {
       this.targetsJson.push(props.targetJson);
@@ -285,6 +292,11 @@ export interface ITargetGroup {
    * ARN of the target group
    */
   readonly targetGroupArn: string;
+
+  /**
+   * Return an object to depend on the listeners added to this target group
+   */
+  listenerDependency(): cdk.IDependable;
 }
 
 /**
@@ -298,6 +310,8 @@ export interface LoadBalancerTargetProps {
 
   /**
    * JSON representing the target's direct addition to the TargetGroup list
+   *
+   * May be omitted if the target is going to register itself later.
    */
   targetJson?: any;
 }

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/enums.ts

@@ -109,9 +109,4 @@ export enum TargetType {
    * Targets identified by IP address
    */
   Ip = 'ip',
-
-  /**
-   * A target that will register itself with the target group
-   */
-  SelfRegistering = 'self-registering',
 }

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/util.ts

@@ -1,3 +1,4 @@
+import cdk = require('@aws-cdk/cdk');
 import { ApplicationProtocol } from "./enums";
 
 export type Attributes = {[key: string]: string | undefined};
@@ -67,3 +68,15 @@ export function determineProtocolAndPort(protocol: ApplicationProtocol | undefin
 export function ifUndefined<T>(x: T | undefined, def: T) {
   return x !== undefined ? x : def;
 }
+
+/**
+ * Allow lazy evaluation of a list of dependables
+ */
+export class LazyDependable implements cdk.IDependable {
+  constructor(private readonly depList: cdk.IDependable[]) {
+  }
+
+  public get dependencyElements(): cdk.IDependable[] {
+    return this.depList;
+  }
+}

packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.listener.ts

@@ -1,4 +1,4 @@
-import { expect, haveResource } from '@aws-cdk/assert';
+import { expect, haveResource, MatchStyle } from '@aws-cdk/assert';
 import ec2 = require('@aws-cdk/aws-ec2');
 import cdk = require('@aws-cdk/cdk');
 import { Test } from 'nodeunit';
@@ -47,6 +47,33 @@ export = {
     test.done();
   },
 
+  'Listener default to open'(test: Test) {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const vpc = new ec2.VpcNetwork(stack, 'Stack');
+    const loadBalancer = new elbv2.ApplicationLoadBalancer(stack, 'LB', { vpc });
+
+    // WHEN
+    loadBalancer.addListener('MyListener', {
+      port: 80,
+      defaultTargetGroups: [new elbv2.ApplicationTargetGroup(stack, 'Group', { vpc, port: 80 })]
+    });
+
+    // THEN
+    expect(stack).to(haveResource('AWS::EC2::SecurityGroup', {
+      SecurityGroupIngress: [
+        {
+          CidrIp: "0.0.0.0/0",
+          FromPort: 80,
+          IpProtocol: "tcp",
+          ToPort: 80
+        }
+      ]
+    }));
+
+    test.done();
+  },
+
   'HTTPS listener requires certificate'(test: Test) {
     // GIVEN
     const stack = new cdk.Stack();
@@ -306,4 +333,33 @@ export = {
 
     test.done();
   },
+
+  'Can depend on eventual listener via TargetGroup'(test: Test) {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const vpc = new ec2.VpcNetwork(stack, 'VPC');
+    const loadBalancer = new elbv2.ApplicationLoadBalancer(stack, 'LoadBalancer', { vpc });
+    const group = new elbv2.ApplicationTargetGroup(stack, 'TargetGroup', { vpc, port: 80 });
+
+    // WHEN
+    const resource = new cdk.Resource(stack, 'SomeResource', { type: 'Test::Resource' });
+    resource.addDependency(group.listenerDependency());
+
+    loadBalancer.addListener('Listener', {
+      port: 80,
+      defaultTargetGroups: [group]
+    });
+
+    // THEN
+    expect(stack).toMatch({
+      Resources: {
+        SomeResource: {
+          Type: "Test::Resource",
+          DependsOn: ["LoadBalancerListenerE1A099B9"]
+        }
+      }
+    }, MatchStyle.SUPERSET);
+
+    test.done();
+  }
 };

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

@@ -17,10 +17,10 @@ export class FakeSelfRegisteringTarget extends cdk.Construct implements elbv2.IA
 
   public attachToApplicationTargetGroup(targetGroup: elbv2.ApplicationTargetGroup): elbv2.LoadBalancerTargetProps {
     targetGroup.registerConnectable(this);
-    return { targetType: elbv2.TargetType.SelfRegistering };
+    return { targetType: elbv2.TargetType.Instance };
   }
 
   public attachToNetworkTargetGroup(_targetGroup: elbv2.NetworkTargetGroup): elbv2.LoadBalancerTargetProps {
-    return { targetType: elbv2.TargetType.SelfRegistering };
+    return { targetType: elbv2.TargetType.Instance };
   }
 }