Skip to content

Commit

Permalink
fix(aws-ecs-patterns): only create an A record if LB is public (#6895)
Browse files Browse the repository at this point in the history 
* fix(aws-ecs-patterns): only create an A record if LB is public

Before this change, we unnecessarily create A record when the load balancer(s) is non public-facing. This commit fixes #6702.

* Added tests

* Moved these 2 lines back to where they were
  • Loading branch information
@hencrice
hencrice committed Mar 20, 2020
1 parent 8c6084c commit f31f4e1
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 29 deletions.
14 changes: 8 additions & 6 deletions packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts
View file
Expand Up @@ -351,13 +351,15 @@ export abstract class ApplicationLoadBalancedServiceBase extends cdk.Construct {
throw new Error('A Route53 hosted domain zone name is required to configure the specified domain name');
}

const record = new ARecord(this, "DNS", {
zone: props.domainZone,
recordName: props.domainName,
target: RecordTarget.fromAlias(new LoadBalancerTarget(loadBalancer)),
});
if (internetFacing) {
const record = new ARecord(this, "DNS", {
zone: props.domainZone,
recordName: props.domainName,
target: RecordTarget.fromAlias(new LoadBalancerTarget(loadBalancer)),
});

domainName = record.domainName;
domainName = record.domainName;
}
}

if (loadBalancer instanceof ApplicationLoadBalancer) {
Expand Down
28 changes: 15 additions & 13 deletions ...ges/@aws-cdk/aws-ecs-patterns/lib/base/application-multiple-target-groups-service-base.ts
View file
Expand Up @@ -365,7 +365,8 @@ export abstract class ApplicationMultipleTargetGroupsServiceBase extends Constru

if (props.loadBalancers) {
for (const lbProps of props.loadBalancers) {
const lb = this.createLoadBalancer(lbProps.name, lbProps.publicLoadBalancer);
const internetFacing = lbProps.publicLoadBalancer !== undefined ? lbProps.publicLoadBalancer : true;
const lb = this.createLoadBalancer(lbProps.name, internetFacing);
this.loadBalancers.push(lb);
const protocolType = new Set<ApplicationProtocol>();
for (const listenerProps of lbProps.listeners) {
Expand All @@ -384,7 +385,7 @@ export abstract class ApplicationMultipleTargetGroupsServiceBase extends Constru
});
this.listeners.push(listener);
}
const domainName = this.createDomainName(lb, lbProps.domainName, lbProps.domainZone);
const domainName = this.createDomainName(lb, internetFacing, lbProps.domainName, lbProps.domainZone);
new CfnOutput(this, `LoadBalancerDNS${lb.node.id}`, { value: lb.loadBalancerDnsName });
for (const protocol of protocolType) {
new CfnOutput(this, `ServiceURL${lb.node.id}${protocol.toLowerCase()}`, { value: protocol.toLowerCase() + '://' + domainName });
Expand All @@ -394,13 +395,13 @@ export abstract class ApplicationMultipleTargetGroupsServiceBase extends Constru
this.loadBalancer = this.loadBalancers[0];
this.listener = this.listeners[0];
} else {
this.loadBalancer = this.createLoadBalancer('LB');
this.loadBalancer = this.createLoadBalancer('LB', true);
const protocol = this.createListenerProtocol();
this.listener = this.configListener(protocol, {
listenerName: "PublicListener",
loadBalancer: this.loadBalancer,
});
const domainName = this.createDomainName(this.loadBalancer);
const domainName = this.createDomainName(this.loadBalancer, true);

new CfnOutput(this, 'LoadBalancerDNS', { value: this.loadBalancer.loadBalancerDnsName });
new CfnOutput(this, 'ServiceURL', { value: protocol.toLowerCase() + '://' + domainName });
Expand Down Expand Up @@ -514,8 +515,7 @@ export abstract class ApplicationMultipleTargetGroupsServiceBase extends Constru
}
}

private createLoadBalancer(name: string, publicLoadBalancer?: boolean): ApplicationLoadBalancer {
const internetFacing = publicLoadBalancer !== undefined ? publicLoadBalancer : true;
private createLoadBalancer(name: string, internetFacing: boolean): ApplicationLoadBalancer {
const lbProps = {
vpc: this.cluster.vpc,
internetFacing
Expand Down Expand Up @@ -551,20 +551,22 @@ export abstract class ApplicationMultipleTargetGroupsServiceBase extends Constru
});
}

private createDomainName(loadBalancer: ApplicationLoadBalancer, name?: string, zone?: IHostedZone): string {
private createDomainName(loadBalancer: ApplicationLoadBalancer, internetFacing: boolean, name?: string, zone?: IHostedZone): string {
let domainName = loadBalancer.loadBalancerDnsName;
if (typeof name !== 'undefined') {
if (typeof zone === 'undefined') {
throw new Error('A Route53 hosted domain zone name is required to configure the specified domain name');
}

const record = new ARecord(this, `DNS${loadBalancer.node.id}`, {
zone,
recordName: name,
target: RecordTarget.fromAlias(new LoadBalancerTarget(loadBalancer)),
});
if (internetFacing) {
const record = new ARecord(this, `DNS${loadBalancer.node.id}`, {
zone,
recordName: name,
target: RecordTarget.fromAlias(new LoadBalancerTarget(loadBalancer)),
});

domainName = record.domainName;
domainName = record.domainName;
}
}
return domainName;
}
Expand Down
12 changes: 7 additions & 5 deletions packages/@aws-cdk/aws-ecs-patterns/lib/base/network-load-balanced-service-base.ts
View file
Expand Up @@ -293,11 +293,13 @@ export abstract class NetworkLoadBalancedServiceBase extends cdk.Construct {
throw new Error('A Route53 hosted domain zone name is required to configure the specified domain name');
}

new ARecord(this, "DNS", {
zone: props.domainZone,
recordName: props.domainName,
target: RecordTarget.fromAlias(new LoadBalancerTarget(loadBalancer)),
});
if (internetFacing) {
new ARecord(this, "DNS", {
zone: props.domainZone,
recordName: props.domainName,
target: RecordTarget.fromAlias(new LoadBalancerTarget(loadBalancer)),
});
}
}

if (loadBalancer instanceof NetworkLoadBalancer) {
Expand Down
12 changes: 7 additions & 5 deletions packages/@aws-cdk/aws-ecs-patterns/lib/base/network-multiple-target-groups-service-base.ts
View file
Expand Up @@ -297,20 +297,23 @@ export abstract class NetworkMultipleTargetGroupsServiceBase extends Construct {

if (props.loadBalancers) {
for (const lbProps of props.loadBalancers) {
const lb = this.createLoadBalancer(lbProps.name, lbProps.publicLoadBalancer);
const internetFacing = lbProps.publicLoadBalancer !== undefined ? lbProps.publicLoadBalancer : true;
const lb = this.createLoadBalancer(lbProps.name, internetFacing);
this.loadBalancers.push(lb);
for (const listenerProps of lbProps.listeners) {
const listener = this.createListener(listenerProps.name, lb, listenerProps.port || 80);
this.listeners.push(listener);
}
this.createDomainName(lb, lbProps.domainName, lbProps.domainZone);
if (internetFacing) {
this.createDomainName(lb, lbProps.domainName, lbProps.domainZone);
}
new CfnOutput(this, `LoadBalancerDNS${lb.node.id}`, { value: lb.loadBalancerDnsName });
}
// set up default load balancer and listener.
this.loadBalancer = this.loadBalancers[0];
this.listener = this.listeners[0];
} else {
this.loadBalancer = this.createLoadBalancer('LB');
this.loadBalancer = this.createLoadBalancer('LB', true);
this.listener = this.createListener('PublicListener', this.loadBalancer, 80);
this.createDomainName(this.loadBalancer);

Expand Down Expand Up @@ -405,8 +408,7 @@ export abstract class NetworkMultipleTargetGroupsServiceBase extends Construct {
}
}

private createLoadBalancer(name: string, publicLoadBalancer?: boolean): NetworkLoadBalancer {
const internetFacing = publicLoadBalancer !== undefined ? publicLoadBalancer : true;
private createLoadBalancer(name: string, internetFacing: boolean): NetworkLoadBalancer {
const lbProps = {
vpc: this.cluster.vpc,
internetFacing
Expand Down
10 changes: 10 additions & 0 deletions packages/@aws-cdk/aws-ecs-patterns/test/ec2/test.l3s-v2.ts
View file
Expand Up @@ -235,6 +235,16 @@ export = {
}
}));

expect(stack).notTo(haveResourceLike('AWS::ServiceDiscovery::Service', {
DnsConfig: {
DnsRecords: [
{
Type: "A"
}
]
}
}));

test.done();
},

Expand Down
73 changes: 73 additions & 0 deletions packages/@aws-cdk/aws-ecs-patterns/test/ec2/test.l3s.ts
View file
Expand Up @@ -60,6 +60,44 @@ export = {
test.done();
},

'test ECS loadbalanced construct with private load balancer'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });
cluster.addCapacity('DefaultAutoScalingGroup', { instanceType: new ec2.InstanceType('t2.micro') });

// WHEN
new ecsPatterns.ApplicationLoadBalancedEc2Service(stack, 'Service', {
cluster,
memoryLimitMiB: 1024,
taskImageOptions: {
image: ecs.ContainerImage.fromRegistry('test'),
environment: {
TEST_ENVIRONMENT_VARIABLE1: "test environment variable 1 value",
TEST_ENVIRONMENT_VARIABLE2: "test environment variable 2 value"
}
},
desiredCount: 2,
publicLoadBalancer: false,
});

// THEN - stack contains a load balancer and a service
expect(stack).to(haveResource('AWS::ElasticLoadBalancingV2::LoadBalancer'));

expect(stack).notTo(haveResourceLike('AWS::ServiceDiscovery::Service', {
DnsConfig: {
DnsRecords: [
{
Type: "A"
}
]
}
}));

test.done();
},

'set vpc instead of cluster'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down Expand Up @@ -336,6 +374,41 @@ export = {
test.done();
},

'test Fargate loadbalanced construct with private load balancer'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

// WHEN
new ecsPatterns.ApplicationLoadBalancedFargateService(stack, 'Service', {
cluster,
taskImageOptions: {
image: ecs.ContainerImage.fromRegistry('test'),
environment: {
TEST_ENVIRONMENT_VARIABLE1: "test environment variable 1 value",
TEST_ENVIRONMENT_VARIABLE2: "test environment variable 2 value"
}
},
desiredCount: 2,
publicLoadBalancer: false,
});

// THEN - stack contains a load balancer and a service
expect(stack).to(haveResource('AWS::ElasticLoadBalancingV2::LoadBalancer'));
expect(stack).notTo(haveResourceLike('AWS::ServiceDiscovery::Service', {
DnsConfig: {
DnsRecords: [
{
Type: "A"
}
]
}
}));

test.done();
},

'test Fargate loadbalanced construct opting out of log driver creation'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down

0 comments on commit f31f4e1

Please sign in to comment.