fix(aws-elasticloadbalancingv2-targets): add AlbListenerTarget for NLBs, deprecate AlbTarget due to ALB listener race conditions #17248
Conversation
…Bs, deprecate AlbTarget due to ALB listener race conditions
|
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| */ | ||
| private readonly certificateArns: string[]; | ||
| public readonly protocol: ApplicationProtocol; |
There was a problem hiding this comment.
It's not clear why protocol was exposed; simply for the tests? The tests are already checking this via the CloudFormation checks (Protocol: 'HTTPS',).
| @@ -25,6 +25,7 @@ describe('tests', () => { | |||
| expect(stack).toHaveResource('AWS::ElasticLoadBalancingV2::Listener', { | |||
| Protocol: 'HTTPS', | |||
| }); | |||
| expect(listener.protocol).toBe(elbv2.ApplicationProtocol.HTTPS); | |||
There was a problem hiding this comment.
This feels a bit redundant, given the above expect.
| * @param alb The application load balancer to load balance to | ||
| * @param port The port on which the target is listening | ||
| * | ||
| * @deprecated Use AlbListenerTarget instead. This target does not automatically |
There was a problem hiding this comment.
No need to deprecate both the class and constructor; the latter will effectively deprecate the former. Can you move this more descriptive deprecation message to the top class-level?
| @@ -23,7 +24,7 @@ export class AlbArnTarget implements elbv2.INetworkLoadBalancerTarget { | |||
| return this.attach(targetGroup); | |||
| } | |||
|
|
|||
| private attach(_targetGroup: elbv2.ITargetGroup): elbv2.LoadBalancerTargetProps { | |||
| protected attach(_targetGroup: elbv2.ITargetGroup): elbv2.LoadBalancerTargetProps { | |||
There was a problem hiding this comment.
~ (Nit) - An alternative would be to make this public _attach and mark it as @internal. That would prevent it showing up in the docs and (potentially) confusing people who think they should be using it directly.
njlynch
added
the
closing-soon
github-actions
bot
added
closed-for-staleness
This PR replaces
AlbTargetwithAlbListenerTarget, allowing us to automatically add a dependency between the provided ALB listener and the associated NLB target group. Without such a dependency stacks may fail to deploy if the listener is not created when the target group attempts to be created.AlbTargetcould have logic added to it to add dependencies, this would not work if the provided ALB doesn't have any items in itslistenerproperty (e.g.new ApplicationListener(...)instead ofalb.addListener(...)). I felt deprecating is the safer and clearer path here. Alternatively warnings could be added for such a scenario?Closes #17208
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license