Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat (Issue #65: aws-alb-lambda construct) #231

Closed
wants to merge 4 commits into from
Closed

feat (Issue #65: aws-alb-lambda construct) #231

wants to merge 4 commits into from

Conversation

surukonda
Copy link
Contributor

@surukonda surukonda commented Jun 24, 2021 •
edited
Loading

Issue 65, if available:

Description of changes:
Architecture, Props & Properties of the construct submitted for review

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
  • Commit ID: f412ec3
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
  • Commit ID: 7091c6d
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

biffgaut
biffgaut reviewed Jun 28, 2021
View reviewed changes
Copy link
Contributor

@biffgaut biffgaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the basis for a pretty good construct. A couple issues around consistency with VPC handling, and a outstanding issue to resolve around how it can default to https - but might be pretty close.

source/patterns/@aws-solutions-constructs/aws-alb-lambda/README.md

new AlbToLambda(this, 'AlbToLambdaPattern', {
lambdaFunctionProps: {
runtime: lambda.Runtime.NODEJS_10_X,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit Pick - Nodejs 10 is no longer supported, should change to 12 or 14

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to NODEJS_14_X

source/patterns/@aws-solutions-constructs/aws-alb-lambda/README.md
![Architecture Diagram](architecture.png)

***
© Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2021

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected

source/patterns/@aws-solutions-constructs/aws-alb-lambda/README.md
|:-------------|:----------------|-----------------|
|existingLambdaObj?|[`lambda.Function`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.Function.html)|Existing instance of Lambda Function object, if this is set then the lambdaFunctionProps is ignored.|
|lambdaFunctionProps?|[`lambda.FunctionProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.FunctionProps.html)|User provided props to override the default props of the newly created Lambda function.|
|createDefaultListner?|`boolean`|Default value set to `true`. Creates a HTTP Listener on `port 80` and associates to the Application Load Balancer. If `certificates` property is specified, a HTTPS Listener on `port 443` is created and associated to Application Load Balancer.|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo - Listener

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, will update it.

source/patterns/@aws-solutions-constructs/aws-alb-lambda/README.md
|lambdaFunctionProps?|[`lambda.FunctionProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.FunctionProps.html)|User provided props to override the default props of the newly created Lambda function.|
|createDefaultListner?|`boolean`|Default value set to `true`. Creates a HTTP Listener on `port 80` and associates to the Application Load Balancer. If `certificates` property is specified, a HTTPS Listener on `port 443` is created and associated to Application Load Balancer.|
|certificates?|[`elb.IListenerCertificate[]`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-elasticloadbalancingv2.IListenerCertificate.html)| List of ACM cert ARNs. Used to create a HTTPS Listener on `port 443` if `createDefaultListner` is set to `true`|
|vpcProps?|[`ec2.VpcProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ec2.VpcProps.html)|User provided VPC props that are used to override the default provide used for creating a new VPC. A new VPC is created if user doesn't provide existing VPC details [`elb.applicationLoadBalancerProps.vpc`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-elasticloadbalancingv2.ApplicationLoadBalancerProps.html).|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow pattern of other constructs and attributes-

existingVpcObj
vpcProps

While some constructs have services that don't require a VPC have a deployVpc attribute, this construct needs a VPC so that attribute should not be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

point taken, elb.applicationLoadBalancerProps has a mandatory vpc parameter that user cannot skip. What should happen when existingVpcObj and elb.applicationLoadBalancerProps.vpc is passed to the construct? You are correct on the deployVpc parameter, user cannot deploy this construct without vpc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Partial<> on the template will make the vpc parameter optional (we're looking at replacing | any with Partial - see a separate issue). We want clients to be able to not specify the vpc and we will create one for them like other constructs. Currently if someone sends vpcProps and existingVpcObject to a construct we toss an error and stop, we'll want to add the same type of check here.

source/patterns/@aws-solutions-constructs/aws-alb-lambda/README.md
|createDefaultListner?|`boolean`|Default value set to `true`. Creates a HTTP Listener on `port 80` and associates to the Application Load Balancer. If `certificates` property is specified, a HTTPS Listener on `port 443` is created and associated to Application Load Balancer.|
|certificates?|[`elb.IListenerCertificate[]`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-elasticloadbalancingv2.IListenerCertificate.html)| List of ACM cert ARNs. Used to create a HTTPS Listener on `port 443` if `createDefaultListner` is set to `true`|
|vpcProps?|[`ec2.VpcProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ec2.VpcProps.html)|User provided VPC props that are used to override the default provide used for creating a new VPC. A new VPC is created if user doesn't provide existing VPC details [`elb.applicationLoadBalancerProps.vpc`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-elasticloadbalancingv2.ApplicationLoadBalancerProps.html).|
|applicationLoadBalancerProps?|[`elb.ApplicationLoadBalancerProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-elasticloadbalancingv2.ApplicationLoadBalancerProps.html)|Optional user-provided props to override the default props for the Application Load Balancer.|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it should accept an existingApplicationLoadBalancerObj attribute. There will probably be times in the future where this is connected to an upstream service, such as aws-apigateway-applicationloadbalancer or aws-certificatemanager-applicationloadbalancer.

source/patterns/@aws-solutions-constructs/aws-alb-lambda/README.md
|existingLambdaObj?|[`lambda.Function`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.Function.html)|Existing instance of Lambda Function object, if this is set then the lambdaFunctionProps is ignored.|
|lambdaFunctionProps?|[`lambda.FunctionProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.FunctionProps.html)|User provided props to override the default props of the newly created Lambda function.|
|createDefaultListner?|`boolean`|Default value set to `true`. Creates a HTTP Listener on `port 80` and associates to the Application Load Balancer. If `certificates` property is specified, a HTTPS Listener on `port 443` is created and associated to Application Load Balancer.|
|certificates?|[`elb.IListenerCertificate[]`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-elasticloadbalancingv2.IListenerCertificate.html)| List of ACM cert ARNs. Used to create a HTTPS Listener on `port 443` if `createDefaultListner` is set to `true`|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to need to figure out how to make it default to https. A) it's best practice, B) our internal tools will find unit tests on GitHub that use port 80 and flag them to be fixed.

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
  • Commit ID: f720183
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@biffgaut biffgaut requested a review from hayesry as a code owner October 13, 2021 18:41
@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: 25acb24
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v1
  • Commit ID: 25acb24
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@biffgaut
Copy link
Contributor

This was covered by PR #467

@biffgaut biffgaut closed this Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@biffgaut biffgaut biffgaut left review comments

@hnishar hnishar Awaiting requested review from hnishar

@hayesry hayesry Awaiting requested review from hayesry hayesry is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@surukonda @aws-solutions-constructs-team @biffgaut