feat(elbv2): add support for Lambda targets #3348
Conversation
…into alb-lambda-target
…into alb-lambda-target
|
|
||
| super(scope, id, props, { | ||
| const targetType = props ? props.targetType : undefined; |
There was a problem hiding this comment.
Why introduce all these local variables?
There was a problem hiding this comment.
TBH I am not that proficient in TS and didn't really know what was idiomatic, since props had to be optional.
I tried to make it a little simpler now. Any advice on how to do it more idiomatic is greatly appreciated.
| @@ -111,3 +111,42 @@ export class IpTarget implements IApplicationLoadBalancerTarget, INetworkLoadBal | |||
| }; | |||
| } | |||
| } | |||
|
|
|||
| export class LambdaTarget implements IApplicationLoadBalancerTarget { | |||
There was a problem hiding this comment.
I'm wondering whether this class should go into a separate package, so we can set up dependencies properly so that...
There was a problem hiding this comment.
A separate package inside aws-elasticloadbalancingv2? Like inside lib/shared?
I thought about that, but then IpTarget and InstanceTarget were also here.
I like the idea and can get going on this in the next few days, if you think it is the way forward.
| * | ||
| * @param functionArn The Lambda Function ARN to load balance to | ||
| */ | ||
| constructor(private readonly functionArn: string) { |
There was a problem hiding this comment.
...this can be an IFunction.
| @@ -189,6 +190,16 @@ export class ApplicationListener extends BaseListener implements IApplicationLis | |||
| throw new Error('Can only call addTargets() when using a constructed Load Balancer; construct a new TargetGroup and use addTargetGroup'); | |||
| } | |||
|
|
|||
| function hasLambdaTargets(targets: IApplicationLoadBalancerTarget[]): boolean { | |||
| if (targets.length > 0) { | |||
| return targets.map(target => target instanceof LambdaTarget).reduceRight((previous, current) => previous || current); | |||
There was a problem hiding this comment.
Simpler would be:
const hasLambdaTargets = targets.some(target => target instanceof LambdaTarget);There was a problem hiding this comment.
Thanks for the tip. Didn't know that :)
| @@ -198,7 +209,8 @@ export class ApplicationListener extends BaseListener implements IApplicationLis | |||
| stickinessCookieDuration: props.stickinessCookieDuration, | |||
| targetGroupName: props.targetGroupName, | |||
| targets: props.targets, | |||
| vpc: this.loadBalancer.vpc, | |||
| targetType, | |||
There was a problem hiding this comment.
I believe targetType is autodeduced from calling addTarget(), no? So you shouldn't need to pass it here?
There was a problem hiding this comment.
Cool. Didn't see that. The tests are passing, so thanks 🙂
Well I guess the tests aren't passing after all, if I don't explicitly supply the targetGroup here, if it is Lambda.
|
Is there a current work-around to get ALB + Lambda working with CDK? |
Pull Request Checklist
|
rix0rrr
changed the title
|
Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged. |
|
Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged. |
|
Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged. |
Add a new package for ELBv2 targets called
@aws-cdk/aws-elasticloadbalancingv2-targets.In this package, add a
LambdaTargetwhich can be used to add Lambas as a backendfor ALBs.
IpTargetandInstanceTargethave been moved to the new package, but the originalshave been left in place to not break backwards compatibility (they have been marked
@deprecatedto encourage movement to the new classes).Fixes #1921.