-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(ecs-patterns): allow passing a listener to ApplicationLoadBalancedFargateService pattern #34493
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
base: main
Are you sure you want to change the base?
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 fails with the following errors:
❌ Features must contain a change to an integration test file and the resulting snapshot.
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.
✅ A exemption request has been requested. Please wait for a maintainer's review.
Exemption Request - There are no integration tests for custom load balancers for this construct as far as I can see, so having one for listeners would be pointless. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -180,6 +181,14 @@ export interface ApplicationLoadBalancedServiceBaseProps { | |||
*/ | |||
readonly loadBalancer?: IApplicationLoadBalancer; | |||
|
|||
/** | |||
* The application load balancer's listener that traffic should be served over. | |||
* Only has an effect when specifying `loadBalancer`. |
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.
It would be great to understand the rationale for this condition.
Also, since the listener only takes effect when a specific value is provided, let's add some input validation in the constructor to ensure that the provided listener specifies loadBalancer
. This will prevent the scenario where users provide a listener which silently has no effect, and they have no visibility into why their setup does not work.
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.
Primarily because if you're specifying a custom listener you would also need to provide a custom ALB for it to work, as far as I could tell. If you try to put a custom listener with a non-custom load balancer, it should fail because the listener comes from a different ALB. If that's wrong I'm happy to walk that bit back, but I'm also happy to go about putting in the validation to ensure that's all correctly set
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.
Would this satisfy your request for an input validation rule?
if (props.listener && !props.loadBalancer) {
throw new ValidationError('If you specify listener, you must also specify loadBalancer.', this);
}
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.
Primarily because if you're specifying a custom listener you would also need to provide a custom ALB for it to work
If the listener is attached to the ALB anyway, would it be better to just use loadBalancer.listeners
to get the attached listener for the custom ALB provided?
This way there is no risk of the user providing a listener which is not attached to the provided 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.
Primarily because if you're specifying a custom listener you would also need to provide a custom ALB for it to work
If the listener is attached to the ALB anyway, would it be better to just use
loadBalancer.listeners
to get the attached listener for the custom ALB provided?This way there is no risk of the user providing a listener which is not attached to the provided load balancer.
I guess my concern there is what if you're providing a load balancer that has one listener it actually needs to use and one that's used for a different service or an HTTP->HTTPS redirect or similar.
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 attempting to write some CDK code that will put our load balancers into maintenance mode before rolling out a new image deployment, running migrations, etc. In order to do that we need to create our listeners ahead of time due to the design of our app, but because this pattern tries to configure one for you, we can't implement it cleanly.
I think it would help if you can elaborate on your use case - my understanding (which may be incorrect) is that you would like to decouple the listener creation from the pattern construct initialisation.
The example in the readme shows the listener being attached to the same load balancer which is provided to the pattern, which indicates that they are tied together - this was the basis for my suggestion. If this is not the case and the listener does not have to be attached to the same load balancer, then I think it would help to provide a couple of examples of actual intended use. We can then discuss what input validation is appropriate.
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.
That's fair but also doesn't address the issue of making sure it uses a SPECIFIC listener as opposed to, say, the first one it finds. That's my requirement on this, I want to be able to give it a specific listener on an LB that might have two or three
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.
Has my reply there helped explain what I'm trying to solve for here? If not I can provide some level of a basic example of the code I've got to back it up.
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.
Hi @hbjydev, apologies for the delayed response - I requested a team member to take a look at the PR and share their thoughts. Please take a look at this comment: #34493 (comment).
While this may not be a widely used feature, allowing users to pass an existing listener makes sense as there's no easy workaround without introducing a new prop. This addition would support several valid use cases:
However, we should add input validation to ensure the provided listener belongs to the provided load balancer to prevent potential runtime issues. This will help users catch configuration errors early and provide clear guidance. Consider adding some improvements before it can be merged:
|
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.
Requesting the changes suggested in #34493 (comment)
Hi @hbjydev, just checking if this change is still needed. If you have figured out another way to fulfil your requirements, I will close this PR. |
Hi, yeah, we ended up vendoring our changes temporarily. I'll look at merging in the feedback here in the next few days. |
Reason for this change
We're attempting to write some CDK code that will put our load balancers into maintenance mode before rolling out a new image deployment, running migrations, etc.
In order to do that we need to create our listeners ahead of time due to the design of our app, but because this pattern tries to configure one for you, we can't implement it cleanly.
This PR resolves that problem.
Description of changes
This adds a new property for the ALB Fargate Service pattern,
listener
, which is an optionalApplicationListener
, and does a ternary check to see if it's set. If it is, it uses that as the listener for the rest of the construct, and if not, it creates a new one, similarly to how this check is implemented for load balancers themselves.Describe any new or updated permissions being added
None
Description of how you validated changes
I've tested this file having extracted it out of CDK source code and modifying a 'fork' of it in our codebase, which I then ran and verified does what I need it to.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license