feat(ecs-patterns): support NLB with TLS listener and target group#30611
feat(ecs-patterns): support NLB with TLS listener and target group#30611mergify[bot] merged 22 commits intoaws:mainfrom
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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.
agarwal-aashish
left a comment
There was a problem hiding this comment.
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 |
| listenerCertificate: new Certificate(stack, 'myCert', { | ||
| domainName, | ||
| validation, | ||
| }), |
There was a problem hiding this comment.
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.
Sounds good to me.
To confirm I understand your suggestion:
I can use
and then import a certificate with
There was a problem hiding this comment.
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.
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.
Thanks for the guidance!
I will take me some time to learn the details of self signed certificate and update the tests.
There was a problem hiding this comment.
Hi @moelasmar,
I tested my change with Amazon issued cert at the end.
And the update code works.
When I use the self signed cert, all AWS resources exception the NLB created successfully. The NLB failed with the following message in CloudFormation deployment events:
Resource handler returned message: "The certificate 'arn:aws:acm:us-east-1:128346755879:certificate/efe2138c-cf1d-481b-ba56-737b1ca1819f' must have a fully-qualified domain name, a supported signature, and a supported key size. (Service: ElasticLoadBalancingV2, Status Code: 400, Request ID: 49557e15-b32d-43f9-bb26-7ef309de21a7)" (RequestToken: 5c8f3642-cc69-6051-e9d7-a4b6fdc41064, HandlerErrorCode: InvalidRequest)
Pull request has been modified.
Pull request has been modified.
|
Hi moelasmar, I have cleaned up the integration test, please review. I notice there are other integration tests creating a test certificate inline. |
| * In order to test this you need prepare a certificate. | ||
| */ | ||
| const certArn = process.env.CDK_INTEG_CERT_ARN || process.env.CERT_ARN; | ||
| if (!certArn) throw new Error('For this test you must provide your own Certificate as an env var "CERT_ARN". See framework-integ/README.md for details.'); |
There was a problem hiding this comment.
I do not see the environment variable CERT_ARN mentioned in the framework-integ/README.md file. Can you update this file, and add some details on how to create this certificate.
There was a problem hiding this comment.
Thanks for the review!
I updated the README and attached a link to AWS doc.
If we need more details in README, I propose to do it in another PR.
So the ECS Fargate pattern fix can be published first.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
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
listenerCertificatetoNetworkLoadBalancedServiceBaseProps, default value isnonelistenerPortandtaskImageOptions.containerPortto 443, iflistenerCertificateis 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