-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(elasticloadbalancing): classic load balancer supports ec2 instances #24353
Conversation
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
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.
In addition to the following comments, this PR should also have
- a readme change advertising the new feature
- an integ test
packages/@aws-cdk/aws-elasticloadbalancing/lib/load-balancer.ts
Outdated
Show resolved
Hide resolved
|
||
// WHEN | ||
elb.addListener({ externalPort: 80, internalPort: 8080 }); | ||
elb.addTarget(new InstanceTarget(instance, connections)); |
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.
OK I see that attachToClassicLB
is tested via the addTarget
API since elb.addTarget
is implemented like this:
public addTarget(target: ILoadBalancerTarget) {
target.attachToClassicLB(this);
this.newTarget(target);
}
TO me, it seems like attachToClassicLB
should be the place where we implement the 'attaching' code and it's quite odd that it's empty...
elb.addListener({ externalPort: 80, internalPort: 8080 }); | ||
elb.addTarget(new InstanceTarget(instance, connections)); | ||
|
||
// THEN: at the very least it added a security group rule for the backend |
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.
Does this really demonstrate that an instance has been attached to a load balancer?
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.
Comment about security group should be updated to explain how the test works now.
* @param instance Instance to register to. | ||
* @param connections The network connections associated with this resource. | ||
*/ | ||
constructor(public readonly instance: Instance, public readonly connections: Connections) { |
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.
An Instance
already has Connections
, so you don't need to pass any additional ones.
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.
I think we might need port
as input though! How else are we going to know what port to forward to?
* If you register a target of this type, you are responsible for making | ||
* sure the load balancer's security group can connect to the instance. |
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.
I'd rather we take of this for the user.
const securityGroup = new SecurityGroup(stack, 'simple-instance-1-sg', | ||
{ | ||
vpc, | ||
allowAllOutbound: true, // will let your instance send outboud traffic | ||
securityGroupName: 'simple-instance-1-sg', | ||
}, | ||
); | ||
|
||
// lets use the security group to allow inbound traffic on specific ports | ||
securityGroup.addIngressRule( | ||
Peer.anyIpv4(), | ||
Port.tcp(22), | ||
'Allows SSH access from Internet', | ||
); | ||
|
||
securityGroup.addIngressRule( | ||
Peer.anyIpv4(), | ||
Port.tcp(80), | ||
'Allows HTTP access from Internet', | ||
); | ||
|
||
securityGroup.addIngressRule( | ||
Peer.anyIpv4(), | ||
Port.tcp(443), | ||
'Allows HTTPS access from Internet', | ||
); |
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.
This looks like it's testing things about SecurityGroup
, but we're not actually testing SecurityGroup here.
healthCheck: { | ||
interval: Duration.minutes(1), | ||
path: '/ping', | ||
protocol: LoadBalancingProtocol.HTTPS, | ||
port: 443, | ||
}, |
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.
We're also not testing health checks.
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.
This looks much much better! I have some minor comments thats more about cleaning up the code than anything, and then of course you still need to address the automated reviewer (need readme change) and the build failure (need to update test in autoscaling).
/** | ||
* Ec2 instance | ||
*/ | ||
readonly ec2Instance?: Instance |
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.
readonly ec2Instance?: Instance | |
readonly ec2Instance?: Instance; |
*/ | ||
export class InstanceTarget implements ILoadBalancerTarget { | ||
readonly connections: Connections; | ||
readonly ec2Instance?: Instance |
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.
readonly ec2Instance?: Instance | |
readonly ec2Instance?: Instance; |
/** | ||
* An object that has instance object. | ||
*/ |
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 IInstance
class also. Give another look at all your doc strings and see if they can be improved. For example,
/**
* Ec2 instance
*/
readonly ec2Instance?: Instance;
The docstring isn't really giving us more information than the property name.
packages/@aws-cdk/aws-elasticloadbalancing/lib/load-balancer.ts
Outdated
Show resolved
Hide resolved
this.connections = instance.connections; | ||
this.ec2Instance = instance; | ||
} | ||
public attachToClassicLB(_loadBalancer: LoadBalancer): void { |
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.
public attachToClassicLB(_loadBalancer: LoadBalancer): void { | |
public attachToClassicLB(loadBalancer: LoadBalancer): void { |
The _
is usually used to describe a property that has to be in the function definition, but not used in the function. Usually the case is when you are implementing an inherited function but some properties in the definition are not useful for your particular implementation. Here, you are using loadBalancer
, so there's no need to add _
as a prefix.
elb.addListener({ externalPort: 80, internalPort: 8080 }); | ||
elb.addTarget(new InstanceTarget(instance, connections)); | ||
|
||
// THEN: at the very least it added a security group rule for the backend |
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.
Comment about security group should be updated to explain how the test works now.
@@ -2,6 +2,7 @@ | |||
import * as ec2 from '@aws-cdk/aws-ec2'; | |||
import * as cdk from '@aws-cdk/core'; | |||
import * as elb from '../lib'; | |||
import { InstanceTarget } from '../lib'; |
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.
this import isn't necessary since the line above is import * as elb from '../lib';
. You should just use elb.InstanceTarget
instead and remove this line.
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.
There are a couple of small issues left that I'd like you to address, and I definitely think we don't need the IInstance
interface.
The solution you came up with is good! I have a solution I'd technically like to see a bit more, and I explained my reasoning in a comment below.
You don't have to follow my suggestion. You can read my argumentation, and be convinced or be unconvinced and keep your own solution. Both are fine by me.
@@ -2,6 +2,7 @@ | |||
import * as ec2 from '@aws-cdk/aws-ec2'; |
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.
Instead of modifying the existing integ test, can you add a new one instead?
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might work, but I'm not sure this not only works by accident. Usually we use Lazy
instead. It will also have an option to drop the list if it turns out to be empty.
Search the code base for Lazy.list
for an example.
/** | ||
* An object that has instance object. | ||
*/ | ||
interface IInstance { |
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.
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 IMaybeHasInstance
.
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 ILoadBalancerTarget
.
But don't do that yet, see below.
/** | ||
* Interface that is going to be implemented by constructs that you can load balance to | ||
*/ | ||
export interface ILoadBalancerTarget extends IConnectable { | ||
export interface ILoadBalancerTarget extends IConnectable, IInstance { |
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.
Looks like what you did is the following:
- Every
ILoadBalancerTaraget
maybe has an instance associated with it. - If it does, the
LoadBalancer
will pick it up and add it to theinstanceIds
array.
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:
- Some load balancer targets may need to add to the
instanceIds
array onLoadBalancer
.
As far as I can tell, we can do this in one of three ways:
ILoadBalancerTarget
s have a public member that theLoadBalancer
knows to query for, and do something with (the solution you came up with)- The
attachToClassicLb()
function returns a value that theLoadBalancer
knows what to do with (this is very similar to the first solution, but I like it a bit more because it's more explicitly part of the "attach to Load Balancer" protocol, and less of what it means to be a load balancer target -- I told you my objections were going to be very conceptual). OurattachToClassicLb
function doesn't return anything yet, but we can make it return some type of result value. This is an approach that we also take a bunch around the CDK. - Finally, the load balancer target can tell the
LoadBalancer
to add a value to itsinstanceIds
array, via some command.
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 ThreeInstancesTarget
: every target can add exactly 0 or 1 instances, no more.
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.
|
||
if (target.ec2Instance) { | ||
this.instanceIds.push(target.ec2Instance.instanceId); | ||
target.ec2Instance.addSecurityGroup(this.securityGroup); |
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.
I don't think we need to do addSecurityGroup
.
We need to do something like:
target.ec2Instance.connections.allowFrom(/* something here */);
And it would be better if that implementation was inside attachToClassicLB
, because that is ultimately the function that should do the work of "connecting an LB to an instance".
this.ec2Instance = instance; | ||
} | ||
public attachToClassicLB(_loadBalancer: LoadBalancer): void { | ||
_loadBalancer.addListener({ externalPort: this.port }); |
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.
Wait, doesn't this add a public listener? Is this correct?
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
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.
Almost there! Can we get rid of the port
parameter?
// 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: Lazy.list({ produce: () => this.instanceIds.length == 0 ? undefined : this.instanceIds }), |
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.
I think our linter will have you write triple equals (===
) instead of double equals (==
), does it not?
instances: Lazy.list({ produce: () => this.instanceIds.length == 0 ? undefined : this.instanceIds }), | |
instances: Lazy.list({ produce: () => this.instanceIds.length === 0 ? undefined : this.instanceIds }), |
This is a crazy JavaScript thing. Read about it here.
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.
@@ -398,6 +399,34 @@ export class LoadBalancer extends Resource implements IConnectable { | |||
Port.tcp(instancePort), | |||
`Port ${instancePort} LB to fleet`); | |||
} | |||
|
|||
/** | |||
* add instance to the load balancer. |
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.
* add instance to the load balancer. | |
* Add instance to the load balancer. | |
* |
/** | ||
* add instance to the load balancer. | ||
* @internal | ||
* @param instanceId |
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.
* @param instanceId |
Either describe the parameter, or remove this.
* @param port Override the default port for the target. | ||
*/ | ||
constructor(public readonly instance: Instance, public readonly port: number) { | ||
this.connections = instance.connections; | ||
} | ||
public attachToClassicLB(loadBalancer: LoadBalancer): void { | ||
loadBalancer._addInstanceId(this.instance.instanceId); | ||
this.connections.allowFrom(loadBalancer.connections, Port.tcp(this.port)); |
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.
I have one last question about the port
parameter. You are saying that if port
is given, it "overrides the default port".
But think about the following:
- The parameter is not optional. Normally, if you say that a value "overrides" a different value, you have an option to use the default as well. This parameter is not optional, so while you could say it "overrides" something, that is not really a useful statement: it is just going to be the port on the target.
- Second, the parameter is only used in the SecurityGroup (firewall rules). We're not using the port when we are configuring the load balancer itself. That port is configured elsewhere.
So that means we are in the following situation: the load balancer will forward to instance port A, while we open up the firewall on port B. It is now the user's responsibility to make sure that A == B
. If they make a mistake, then the load balancer will try to use a port that the security groups don't allow connections on, and the load balancing will not work.
This kind of redundancy, with opportunities for misconfiguration, is exactly what CDK tries to prevent!
So our ideal solution will be for the InstanceTarget
to make sure that the user doesn't have to specify the port number again. The load balancer already knows what port it's trying to connect to. We should reuse that.
packages/@aws-cdk/aws-elasticloadbalancing/lib/load-balancer.ts
Outdated
Show resolved
Hide resolved
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.
Oops! After reading the code some more, I'm starting to think that part of the code I told you should exist, already exists, so you didn't need to write it!
See the newInstancePort
function...
I'm sorry that I made you do unnecessary work. But I said when we talked "I don't know how every little bit of the code base works, I just know how the services work and what the end result needs to be", and that is literally true. If I had known this code already existed I would have told you to use it. I just know what code needed to exist to make everything work out correctly.
It's very likely your education prepared you for writing code, but not so much for reading it. Looking at what's already there is extremely important however, and something we'll need to do every day.
So, again: sorry I didn't tell you that what you wrote already existed. From now on, assume an implied "unless reality contracts Rico" with everything I say 🥲.
Reading code is an important skill and one that may be hard to pick up automatically. Would you like to practice this together some time?
|
||
### Adding Ec2 Instance as a target for the load balancer | ||
|
||
EC2 instaces can be added as the target for the load balancer via `addTarget()` method using `InstanceTarget` class by providing ec2 `instance` as a target. |
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.
EC2 instaces can be added as the target for the load balancer via `addTarget()` method using `InstanceTarget` class by providing ec2 `instance` as a target. | |
You can add an EC2 instance to the load balancer by calling using `new InstanceTarget` as the argument to `addTarget()`: |
instanceType: InstanceType.of(InstanceClass.BURSTABLE2, InstanceSize.MICRO), | ||
machineImage: new AmazonLinuxImage(), | ||
}); | ||
lb.addTarget(elb.InstanceTarget(instance)) |
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.
lb.addTarget(elb.InstanceTarget(instance)) | |
lb.addTarget(elb.InstanceTarget(instance)); |
@@ -318,6 +320,8 @@ export class LoadBalancer extends Resource implements IConnectable { | |||
// Keep track using array so user can get to them even if they were all supplied in the constructor | |||
this.listenerPorts.push(port); | |||
|
|||
// Allow connection to all instances to new listener. |
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.
I'm seeing this line of code a couple of lines above this line.
this.newInstancePort(instancePort);
It's named suspiciously like some of the code you just added. Could you have a look at that and see if that is maybe already doing what we need it to do?
@@ -248,23 +248,25 @@ export class LoadBalancer extends Resource implements IConnectable { | |||
private readonly elb: CfnLoadBalancer; | |||
private readonly securityGroup: SecurityGroup; | |||
private readonly listeners: CfnLoadBalancer.ListenersProperty[] = []; | |||
private readonly instances: IInstance[] = []; |
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.
This holds on to IInstances
, for the sole purpose of using them as IConnectable
s. This collection doesn't need to be as specific as it is. If we just need "anything that's IConnectable
" we can just write that. By writing IInstance
we are excluding other things that could be connectable.
But I'm now noticing that ILoadBalancerTarget extends IConnectable
, so we already have a list of IConnectable
s... it's this.targets
!
elb.addListener({ externalPort: 80, internalPort: 8080 }); | ||
elb.addTarget(new InstanceTarget(instance)); |
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.
Can we also do a test where these are reversed to make sure we still see the security group rules?
SecurityGroupIngress: [ | ||
{ | ||
CidrIp: '0.0.0.0/0', | ||
FromPort: 80, |
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.
80
is the externalPort
number up there. Don't we want to assert that we added a security group rule for the internalPort
?
Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>
Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>
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.
🍾🍾
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…ces (aws#24353) Introducing the `InstanceTarget` class: an EC2 instance that serves as the target for load balancing. This class allows to register an instance to a load balancer. `InstanceTarget ` takes an instance to register as the target for the load balancer. For example, ```ts const target = new elb.InstanceTarget(instance); elb.addTarget(target); ``` creates an InstanceTarget object with the specified instance and then adds it to the load balancer. > [CONTRIBUTING GUIDE]: https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md > [DESIGN GUIDELINES]: https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md Closes aws#23500. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Introducing the
InstanceTarget
class: an EC2 instance that serves as the target for load balancing. This class allows to register an instance to a load balancer.InstanceTarget
takes an instance to register as the target for the load balancer.For example,
creates an InstanceTarget object with the specified instance and then adds it to the load balancer.
Closes #23500.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license