-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(ecs-patterns): support NLB with TLS listener and target group #30611
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 has failed. See the aws-cdk-automation comment below for failure reasons. 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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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 code looks good and unit tests should cover all the points. Moreover it is backward compatible so should not break any clients.
Thanks for the review and approval! |
NLB listener only allow 1 cert. The listener's protocol will become TLS if cert configured. And the target group protocol is same as listener by default (TLS).
@Mergifyio refresh |
✅ Pull request refreshed |
@xazhao I notice you just merge and release a new version. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
* | ||
* @default - none | ||
*/ | ||
readonly listenerCertificate?: IListenerCertificate; |
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.
should it be an array of IListenerCertificate to match the type of the listener certificates property
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 @moelasmar,
Thanks for your review and appreciate your attention on details!
Both network and application load balancer require exact one certificate even HTTPS protocol is used. The construct expect zero or one certificate.
I designed the interface to take an optional IListenerCertificate
is to avoid user confusion when they passed multiple certificates into construct.
Please let me know you option.
Many thanks,
Lok
ALB doc: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_elasticloadbalancingv2.ApplicationListener.html#certificates
NLB doc: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_elasticloadbalancingv2.NetworkListener.html#certificates
You must provide exactly one certificate if the listener protocol is HTTPS or TLS.
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.
left minor comment
listenerCertificate: new Certificate(stack, 'myCert', { | ||
domainName, | ||
validation, | ||
}), |
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.
since creating the certificate is not important for this testing, could you please modify the integration for EC2, and Fargate to accept an imported certificate, as the certificate creation step takes a very long time, and slow down this testing.
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.
Sounds good to me.
To confirm I understand your suggestion:
I can use
aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-apigateway/test/integ.domain-name.ts
Line 13 in ab73e53
const certArn = process.env.CDK_INTEG_CERT_ARN || process.env.CERT_ARN; |
and then import a certificate with
aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-apigateway/test/integ.domain-name.ts
Line 83 in ab73e53
const certificate = Certificate.fromCertificateArn(testCase, 'Cert', certArn); |
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.
and for the certificate creation itself, I believe you can create a self signed certificate and then import it, I believe this will be an easier solution.
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.
you can follow the following steps to create a self signed certificate:
$ openssl genrsa -out my-private.key 2048
$ openssl req -new -key my-private.key -out my-csr.csr
$ openssl x509 -req -days 365 -in my-csr.csr -signkey my-private.key -out my-certificate.crt
Then you can follow this link to import it.
Also, please add these steps to the test case as comment
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.
Thanks for the guidance!
I will take me some time to learn the details of self signed certificate and update the tests.
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
thanks @199911. I will update the PR status to request changes till you update the integration testing
Issue # (if applicable)
Closes #8517
Reason for this change
NLB support TLS protocol in listener and target group.
This changes provide a feature parity in ECS patterns, allowing customer to enhance security with encrypted traffic between NLB and services
Description of changes
listenerCertificate
toNetworkLoadBalancedServiceBaseProps
, default value isnone
listenerPort
andtaskImageOptions.containerPort
to 443, iflistenerCertificate
is provided.Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license