feat(aws-alb-lambda): New Construct - aws-alb-lambda #467
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
source/patterns/@aws-solutions-constructs/aws-alb-lambda/README.md
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-alb-lambda/README.md
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-alb-lambda/README.md
Outdated
Show resolved
Hide resolved
| | **Name** | **Type** | **Description** | | ||
| |:-------------|:----------------|-----------------| | ||
| | vpc | [ec2.IVpc](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ec2.IVpc.html) | The VPC used by the construct (whether created by the construct or providedb by the client) | | ||
| | loadBalancer | [elasticloadbalancingv2.ApplicationLoadBalancer](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-elasticloadbalancingv2.ApplicationLoadBalancer.html) | The Load Balancer used by the construct (whether created by the construct or providedb by the client) | |
There was a problem hiding this comment.
provided typos still there
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
biffgaut
changed the title
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Update from Main
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
source/patterns/@aws-solutions-constructs/aws-route53-alb/README.md
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-route53-alb/README.md
Outdated
Show resolved
Hide resolved
| super(scope, id); | ||
| defaults.CheckProps(props); | ||
|
|
||
| if (props.listenerProps?.certificateArns) { |
There was a problem hiding this comment.
I think it should be fine, but could you make sure this (and the whole construct) will compile with v2 build patterns script?
There was a problem hiding this comment.
Doesn't the build process confirm this every time it does a build on push? If not, I don't know how to check this.
| } | ||
|
|
||
| if ( | ||
| (props.existingLoadBalancerObj && (props.existingLoadBalancerObj.listeners.length === 0) || !props.existingLoadBalancerObj) |
There was a problem hiding this comment.
Do we have test cases to check for all combinations of this and the rest of the if conditions?
There was a problem hiding this comment.
Before we figured out the test coverage issue I attempted to visually inspect the file and attempted to write a use case that caught every if statement (every error). When we figured out the coverage, I covered every line of the file except for a line that is unreachable and is really more of an assert statement than an error check.
| ); | ||
| } | ||
|
|
||
| if ( props.existingLoadBalancerObj ) { |
There was a problem hiding this comment.
Is the warning message accurate? It's checking existingLoadBalancerObj and warning about the props.publicApi?
There was a problem hiding this comment.
Yes - publicApi defines whether the entire construct is being created as a public or private API, which affects the VPC, ALB and (I think) the Lambda function. If the customer is bringing their own existing ALB to the party, then it has to have been created in a manner (public or private) that will work in the construct. If it doesn't match, weird things will occur that may be hard to diagnose - this warning is there so give them a little guidance as to what may be going wrong if they end up in that situation.
| }); | ||
| } | ||
|
|
||
| this.loadBalancer = defaults.ObtainAlb( |
There was a problem hiding this comment.
Any reason why are we changing the terminology from buildblah to Obtainblah? Is there a difference between the two?
There was a problem hiding this comment.
Yes - because the routine doesn't always build an ALB. It looks at everything provided, decides if it needs to build a new ALB or use an existing ALB.
|
|
||
| if (this.loadBalancer.listeners.length === 0) { | ||
| // This is a new listener, we need to create it along with the default target | ||
| const newTargetGroup = defaults.CreateLambdaTargetGroup(this, 'first-tg', this.lambdaFunction, props.targetProps); |
There was a problem hiding this comment.
Does it matter if first-tg is inconsistent with tg${this.loadBalancer.listeners.length} line#224?
There was a problem hiding this comment.
It should make no functional difference, line #224 used to say something like second-tg until I realized that was going to create a problem for three or more target groups so I changed it. Now that you've made me aware of this it offends me stylistically, so I will change it. Thanks.
There was a problem hiding this comment.
That also alerted me to a bug - the length of the listener array will give me a number that is 1 too low for what I wanted (it gives me tg0, tg1, etc. instead of tg1, tg2). Changing that while I'm in there.
| let listener: elb.ApplicationListener; | ||
|
|
||
| if (listeners.length === 1 ) { | ||
| listener = listeners[0]; |
There was a problem hiding this comment.
Do we need to validate the .protocol === "HTTPS" or has it been validated before?
There was a problem hiding this comment.
We're good - if there's only 1 we return it whether it is HTTPS or HTTP (clients can explicitly direct us to make an HTTP API). It's only in there are 2 listeners that we want to be sure we return the HTTPS listener, assuming that the other is an HTTP listener that automatically redirects to HTTPS. That's how we create them according to best practice. I guess it's possible that someone could create an ALB on their own with 2 listeners and redirect the HTTPS listener to HTTP and supply it as an existing ALB - that would be such a bad practice that I didn't worry about writing code to support it (why be an enabler? :-)
| @@ -56,14 +56,14 @@ export interface Route53ToAlbProps { | |||
| * | |||
| * @default - true | |||
| */ | |||
| readonly logAccessLogs?: boolean, | |||
There was a problem hiding this comment.
Jamming up different things into one PR? :)
There was a problem hiding this comment.
Yeah...I guess I am. The name I used in that construct collides with other names in this construct, so I had to use a different name in this construct and changed the old one to keep them in sync. Although I could make an argument that the change in Route53ToAlbProps was driven by this Issue - it would come out kind of lawyerly, but it could be made.
There was a problem hiding this comment.
FWIW - aws-route53-alb hasn't been pushed to npm/etc. yet, so this won't break any existing customer code.
… into Issue65 Merge from github
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #, if available:
Closes #65
Description of changes:
First release of aws-alb-lambda
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.