Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Oct 11, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
}

/**
Expand Down
12 changes: 4 additions & 8 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -179,18 +179,14 @@ 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}`);
}
}
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -319,6 +319,7 @@ export interface ApplicationListenerRefProps {
}

class ImportedApplicationListener extends cdk.Construct implements IApplicationListener {
public readonly dependencyElements: cdk.IDependable[] = [];
public readonly connections: ec2.Connections;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -144,6 +144,7 @@ export class ApplicationTargetGroup extends BaseTargetGroup {
listener.registerConnectable(member.connectable, member.portRange);
}
this.listeners.push(listener);
this.dependableListeners.push(listener);
}
}

Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mention listenerDependency in README

return new LazyDependable([]);
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}

/**
Expand All @@ -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([]);
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];

Expand All @@ -17,6 +18,7 @@ export abstract class BaseListener extends cdk.Construct {
defaultActions: new cdk.Token(() => this.defaultActions),
});

this.dependencyElements = [resource];
this.listenerArn = resource.ref;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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',
}
13 changes: 13 additions & 0 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/util.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import cdk = require('@aws-cdk/cdk');
import { ApplicationProtocol } from "./enums";

export type Attributes = {[key: string]: string | undefined};
Expand Down Expand Up @@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -47,6 +47,33 @@ export = {
test.done();
},

'Listener default to open'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double spaces in test name

// 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();
Expand Down Expand Up @@ -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();
}
};
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,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
Expand Down Expand Up @@ -427,4 +427,4 @@
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,4 @@ listener.addTargets('ConditionalTarget', {
targets: [new elbv2.IpTarget('10.0.1.2')]
});

listener.connections.allowDefaultPortFromAnyIpv4('Open to the world');

app.run();