feat(elasticloadbalancing): classic load balancer supports ec2 instances #24353
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,5 @@ | ||||||
| import { | ||||||
| Connections, IConnectable, Instance, ISecurityGroup, IVpc, Peer, Port, | ||||||
| SecurityGroup, SelectedSubnets, SubnetSelection, SubnetType, | ||||||
| } from '@aws-cdk/aws-ec2'; | ||||||
| import { Duration, Lazy, Resource } from '@aws-cdk/core'; | ||||||
|
|
@@ -141,10 +141,20 @@ export interface HealthCheck { | |||||
| readonly timeout?: Duration; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * An object that has instance object. | ||||||
| */ | ||||||
| interface IInstance { | ||||||
|
There was a problem hiding this comment. The name doesn't quite cover what this is trying to express. It's not that something that implements this interface is an instance, it's that something that implements this interface maybe has an instance. Another name for it would have been Since we're not using the interface by itself anywhere (or at least I don't think so), I don't think we need to name it separately. It could be folded into But don't do that yet, see below. |
||||||
| /** | ||||||
| * Ec2 instance | ||||||
| */ | ||||||
| readonly ec2Instance?: Instance | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Interface that is going to be implemented by constructs that you can load balance to | ||||||
| */ | ||||||
| export interface ILoadBalancerTarget extends IConnectable, IInstance { | ||||||
|
There was a problem hiding this comment. Looks like what you did is the following:
It's a nice and pragmatic solution, and one that we follow in a lot of places in the CDK already! Having said that, I'm not suuuper fond of it, mostly for theoretical and aesthetic reasons that have no bearing on the practicality of it. But bear with me for a bit 😆. Ultimately, what we are trying to achieve is:
As far as I can tell, we can do this in one of three ways:
From those three solutions, I like the 3rd one the best. The reason for that is that is that Tell, Don't Ask tends to produce software that is more flexible and extensible. For example, if we had: class LoadBalancer {
/**
* @internal
*/
public _addInstanceId(instanceId: string) { this.instanceIds.push(instanceId); }
}
class InstanceTarrget {
public attachToClassicLb(lb) {
lb._addInstanceId(this.instance.instanceId);
}
}And we said "hey you know what I want to do? I want to have a single target that represents 3 instances", we could easily make that as follows: class ThreeInstancesTarget {
public attachToClassicLb(lb) {
lb._addInstanceId(this.instance1.instanceId);
lb._addInstanceId(this.instance2.instanceId);
lb._addInstanceId(this.instance3.instanceId);
}
}Whereas if we had the following protocol: interface ILoadBalancerTarget {
public instance?: IInstance;
}Then now there is no way anymore to implement the class Of course, we could predict this use case and type it as follows: interface ILoadBalancerTarget {
public instance?: IInstance[];
}And now every load balancer target can have 0, 1 or many instances. But the point of designing using "Tell, Don't Ask" is that you get flexibility for free without having to think too much about it. |
||||||
| /** | ||||||
| * Attach load-balanced target to a classic ELB | ||||||
| * @param loadBalancer [disable-awslint:ref-via-interface] The load balancer to attach the target to | ||||||
|
|
@@ -251,20 +261,21 @@ export class LoadBalancer extends Resource implements IConnectable { | |||||
|
|
||||||
| private readonly instancePorts: number[] = []; | ||||||
| private readonly targets: ILoadBalancerTarget[] = []; | ||||||
| private readonly instanceIds: string[] = []; | ||||||
|
|
||||||
| constructor(scope: Construct, id: string, props: LoadBalancerProps) { | ||||||
| super(scope, id); | ||||||
|
|
||||||
| this.securityGroup = new SecurityGroup(this, 'SecurityGroup', { vpc: props.vpc, allowAllOutbound: false }); | ||||||
| this.connections = new Connections({ securityGroups: [this.securityGroup] }); | ||||||
| // Depending on whether the ELB has public or internal IPs, pick the right backend subnets | ||||||
| const selectedSubnets: SelectedSubnets = loadBalancerSubnets(props); | ||||||
|
|
||||||
| this.elb = new CfnLoadBalancer(this, 'Resource', { | ||||||
| securityGroups: [this.securityGroup.securityGroupId], | ||||||
| subnets: selectedSubnets.subnetIds, | ||||||
| listeners: Lazy.any({ produce: () => this.listeners }), | ||||||
| instances: this.instanceIds, | ||||||
|
There was a problem hiding this comment. This might work, but I'm not sure this not only works by accident. Usually we use Search the code base for |
||||||
| scheme: props.internetFacing ? 'internet-facing' : 'internal', | ||||||
| healthCheck: props.healthCheck && healthCheckToJSON(props.healthCheck), | ||||||
| crossZone: props.crossZone ?? true, | ||||||
|
|
@@ -323,7 +334,10 @@ export class LoadBalancer extends Resource implements IConnectable { | |||||
|
|
||||||
| public addTarget(target: ILoadBalancerTarget) { | ||||||
| target.attachToClassicLB(this); | ||||||
| if (target.ec2Instance) { | ||||||
| this.instanceIds.push(target.ec2Instance.instanceId); | ||||||
| target.ec2Instance.addSecurityGroup(this.securityGroup); | ||||||
|
There was a problem hiding this comment. I don't think we need to do We need to do something like: target.ec2Instance.connections.allowFrom(/* something here */);And it would be better if that implementation was inside |
||||||
| } | ||||||
| this.newTarget(target); | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -400,6 +414,28 @@ export class LoadBalancer extends Resource implements IConnectable { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * An EC2 instance that is the target for load balancing | ||||||
| * | ||||||
rix0rrr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| */ | ||||||
| export class InstanceTarget implements ILoadBalancerTarget { | ||||||
| readonly connections: Connections; | ||||||
| readonly ec2Instance?: Instance | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| /** | ||||||
| * Create a new Instance target. | ||||||
| * | ||||||
| * @param instance Instance to register to. | ||||||
| * @param port Override the default port for the target. | ||||||
| */ | ||||||
| constructor(public readonly instance: Instance, public readonly port: number) { | ||||||
| this.connections = instance.connections; | ||||||
| this.ec2Instance = instance; | ||||||
| } | ||||||
| public attachToClassicLB(_loadBalancer: LoadBalancer): void { | ||||||
|
There was a problem hiding this comment.
Suggested change
The |
||||||
| _loadBalancer.addListener({ externalPort: this.port }); | ||||||
|
There was a problem hiding this comment. Wait, doesn't this add a public listener? Is this correct? |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Reference to a listener's port just created. | ||||||
| * | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| {"version":"29.0.0"} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| { | ||
| "version": "29.0.0", | ||
| "testCases": { | ||
| "integ.elb": { | ||
| "stacks": [ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be explaining the motivation behind having an
IInstanceclass also. Give another look at all your doc strings and see if they can be improved. For example,The docstring isn't really giving us more information than the property name.