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-elasticloadbalancingv2): fix rule dependency #1170

Merged
merged 3 commits into from
Nov 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
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-ecs/lib/base/base-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ export abstract class BaseService extends cdk.Construct
containerPort: this.taskDefinition.defaultContainer!.containerPort,
});

this.resource.addDependency(targetGroup.listenerDependency());
this.resource.addDependency(targetGroup.loadBalancerDependency());

const targetType = this.taskDefinition.networkMode === NetworkMode.AwsVpc ? elbv2.TargetType.Ip : elbv2.TargetType.Instance;
return { targetType };
Expand All @@ -239,4 +239,4 @@ export abstract class BaseService extends cdk.Construct
/**
* The port range to open up for dynamic port mapping
*/
const EPHEMERAL_PORT_RANGE = new ec2.TcpPortRange(32768, 65535);
const EPHEMERAL_PORT_RANGE = new ec2.TcpPortRange(32768, 65535);
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -196,10 +196,10 @@ 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:
`targetGroup.loadBalancerDependency()` 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());
```
resourced.addDependency(targetGroup.loadBalancerDependency());
```
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export class ApplicationListenerRule extends cdk.Construct implements cdk.IDepen
targetGroupArn: targetGroup.targetGroupArn,
type: 'forward'
});
targetGroup.registerListener(this.listener);
targetGroup.registerListener(this.listener, this);
}

/**
Expand All @@ -142,4 +142,4 @@ export class ApplicationListenerRule extends cdk.Construct implements cdk.IDepen
}
return ret;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,14 @@ export class ApplicationTargetGroup extends BaseTargetGroup {
*
* Don't call this directly. It will be called by listeners.
*/
public registerListener(listener: IApplicationListener) {
public registerListener(listener: IApplicationListener, dependable?: cdk.IDependable) {
// Notify this listener of all connectables that we know about.
// Then remember for new connectables that might get added later.
for (const member of this.connectableMembers) {
listener.registerConnectable(member.connectable, member.portRange);
}
this.listeners.push(listener);
this.dependableListeners.push(listener);
this.loadBalancerAssociationDependencies.push(dependable || listener);
}
}

Expand Down Expand Up @@ -172,18 +172,18 @@ export interface IApplicationTargetGroup extends ITargetGroup {
*
* Don't call this directly. It will be called by listeners.
*/
registerListener(listener: IApplicationListener): void;
registerListener(listener: IApplicationListener, dependable?: cdk.IDependable): void;
}

/**
* An imported application target group
*/
class ImportedApplicationTargetGroup extends BaseImportedTargetGroup implements IApplicationTargetGroup {
public registerListener(_listener: IApplicationListener) {
public registerListener(_listener: IApplicationListener, _dependable?: cdk.IDependable) {
// Nothing to do, we know nothing of our members
}

public listenerDependency(): cdk.IDependable {
public loadBalancerDependency(): cdk.IDependable {
return new LazyDependable([]);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export class NetworkTargetGroup extends BaseTargetGroup {
* Don't call this directly. It will be called by listeners.
*/
public registerListener(listener: INetworkListener) {
this.dependableListeners.push(listener);
this.loadBalancerAssociationDependencies.push(listener);
}
}

Expand All @@ -96,7 +96,7 @@ class ImportedNetworkTargetGroup extends BaseImportedTargetGroup implements INet
// Nothing to do, we know nothing of our members
}

public listenerDependency(): cdk.IDependable {
public loadBalancerDependency(): cdk.IDependable {
return new LazyDependable([]);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,9 @@ export abstract class BaseTargetGroup extends cdk.Construct implements ITargetGr
protected readonly defaultPort: string;

/**
* List of listeners routing to this target group
* List of dependables that need to be depended on to ensure the TargetGroup is associated to a load balancer
*/
protected readonly dependableListeners = new Array<cdk.IDependable>();
protected readonly loadBalancerAssociationDependencies = new Array<cdk.IDependable>();

/**
* Attributes of this target group
Expand Down Expand Up @@ -249,10 +249,10 @@ export abstract class BaseTargetGroup extends cdk.Construct implements ITargetGr
}

/**
* Return an object to depend on the listeners added to this target group
* Return an object to depend on this TargetGroup being attached to a load balancer
*/
public listenerDependency(): cdk.IDependable {
return new LazyDependable(this.dependableListeners);
public loadBalancerDependency(): 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.

Should we just remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess. I didn't want to cause yet another BREAKING CHANGE. But sure.

return new LazyDependable(this.loadBalancerAssociationDependencies);
}

/**
Expand Down Expand Up @@ -297,7 +297,7 @@ export interface ITargetGroup {
/**
* Return an object to depend on the listeners added to this target group
*/
listenerDependency(): cdk.IDependable;
loadBalancerDependency(): cdk.IDependable;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ export = {

// WHEN
const resource = new cdk.Resource(stack, 'SomeResource', { type: 'Test::Resource' });
resource.addDependency(group.listenerDependency());
resource.addDependency(group.loadBalancerDependency());

loadBalancer.addListener('Listener', {
port: 80,
Expand All @@ -363,5 +363,40 @@ export = {
}, MatchStyle.SUPERSET);

test.done();
}
},

'Can add dependency on ListenerRule 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 group1 = new elbv2.ApplicationTargetGroup(stack, 'TargetGroup1', { vpc, port: 80 });
const group2 = new elbv2.ApplicationTargetGroup(stack, 'TargetGroup2', { vpc, port: 80 });
const listener = loadBalancer.addListener('Listener', {
port: 80,
defaultTargetGroups: [group1]
});

// WHEN
const resource = new cdk.Resource(stack, 'SomeResource', { type: 'Test::Resource' });
resource.addDependency(group2.loadBalancerDependency());

listener.addTargetGroups('SecondGroup', {
pathPattern: '/bla',
priority: 10,
targetGroups: [group2]
});

// THEN
expect(stack).toMatch({
Resources: {
SomeResource: {
Type: "Test::Resource",
DependsOn: ["LoadBalancerListenerSecondGroupRuleF5FDC196"]
}
}
}, MatchStyle.SUPERSET);

test.done();
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export = {
const resource = new cdk.Resource(stack, 'MyResource', {
type: 'SomeResource',
});
resource.addDependency(group.listenerDependency());
resource.addDependency(group.loadBalancerDependency());

// THEN
expect(stack).toMatch({
Expand Down