feat(ecs-patterns): support NLB with TLS listener and target group #30611

199911 · 2024-06-21T08:03:00Z

Issue # (if applicable)

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

Add listenerCertificate to NetworkLoadBalancedServiceBaseProps, default value is none
Change the default value of listenerPort and taskImageOptions.containerPort to 443, if listenerCertificate is provided.

Description of how you validated changes

Added both unit test and integration test

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

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

The code looks good and unit tests should cover all the points. Moreover it is backward compatible so should not break any clients.

199911 · 2024-08-05T16:32:29Z

Hi @agarwal-aashish

Thanks for the review and approval!
Will the bot merge CR automatically?
Are there any pending actions on my side?

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).

… provided

199911 · 2024-08-15T10:22:28Z

@Mergifyio refresh

mergify · 2024-08-15T10:22:35Z

refresh

✅ Pull request refreshed

moelasmar

left minor comment

moelasmar · 2024-09-08T18:33:48Z

...ramework-integ/test/aws-ecs-patterns/test/ec2/integ.tls-network-load-balanced-ecs-service.ts

+  listenerCertificate: new Certificate(stack, 'myCert', {
+    domainName,
+    validation,
+  }),


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.

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);

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.

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

Thanks for the guidance!

I will take me some time to learn the details of self signed certificate and update the tests.

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.

moelasmar

thanks @199911. I will update the PR status to request changes till you update the integration testing

Pull request has been modified.

199911 · 2024-10-04T12:20:39Z

Hi moelasmar,

I have cleaned up the integration test, please review.

I notice there are other integration tests creating a test certificate inline.
I can help clean up those test after this PR merged.

moelasmar · 2024-10-10T21:06:15Z

...ramework-integ/test/aws-ecs-patterns/test/ec2/integ.tls-network-load-balanced-ecs-service.ts

+ * 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.');


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.

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.

moelasmar

it looks good to me

mergify · 2024-10-14T01:47:00Z

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-cdk-automation · 2024-10-14T02:17:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 153b7f7
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2024-10-14T02:17:54Z

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).

github-actions · 2024-10-14T02:18:13Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Jun 21, 2024

aws-cdk-automation requested a review from a team June 21, 2024 08:03

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Jun 21, 2024

aws-cdk-automation previously requested changes Jun 21, 2024

View reviewed changes

199911 changed the title ~~feat(ecs-patterns): NLB with TLS listener and target group~~ feat(ecs-patterns): support NLB with TLS listener and target group Jun 21, 2024

199911 requested a review from aws-cdk-automation June 21, 2024 08:09

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 21, 2024

This was referenced Jun 24, 2024

Weekly PR metrics report #30637

Closed

Monthly PR metrics report #30715

Closed

199911 mentioned this pull request Jul 13, 2024

Weekly PR metrics report #30783

Closed

agarwal-aashish approved these changes Aug 2, 2024

View reviewed changes

199911 added 9 commits August 15, 2024 02:37

feat: Add props for NLB TLS certificate

d83e8ee

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).

test: Add unit test for listenerCertificate

bae669f

test: Automatically set listener and target group port to 443 if cert…

dd71d68

… provided

docs: Update Readme

343b55d

test: Add unit test for NetworkLoadBalancedEc2Service

d7c577f

docs: Update Readme

13b8cfb

test: Add integration test for nlb with ecs

5d9b9e0

test: Add integration test for nlb with fargate

97f742b

fixup! test: Add integration test for nlb with ecs

0d6ec3f

199911 force-pushed the issue-8517-pr branch from 1d0d331 to 0d6ec3f Compare August 15, 2024 00:37

199911 added 3 commits August 15, 2024 10:36

fixup! test: Add integration test for nlb with ecs

d3a847f

fixup! test: Add integration test for nlb with fargate

1046b3e

Merge branch 'main' into issue-8517-pr

4cb910d

moelasmar previously requested changes Sep 8, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 8, 2024

moelasmar reviewed Sep 8, 2024

View reviewed changes

Merge branch 'main' into issue-8517-pr

36736a1

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 10, 2024

moelasmar previously requested changes Sep 11, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 11, 2024

199911 added 2 commits September 30, 2024 16:53

Speed up integration test by importing cert from arn

e3fed8a

Update fargate snapshot

eb57c44

199911 added 2 commits September 30, 2024 18:23

Merge branch 'main' into issue-8517-pr

19977fe

Update ec2 snapshot

65769ae

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 30, 2024

199911 requested a review from moelasmar October 1, 2024 12:18

moelasmar reviewed Oct 10, 2024

View reviewed changes

199911 added 3 commits October 11, 2024 11:35

Add certificate set up to framework-integ/README.md

129043c

Merge branch 'main' into issue-8517-pr

aab64ef

Minor fix on README

303f5b1

moelasmar approved these changes Oct 14, 2024

View reviewed changes

Merge branch 'main' into issue-8517-pr

153b7f7

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 14, 2024

mergify bot merged commit f4f8abc into aws:main Oct 14, 2024
15 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Oct 14, 2024

199911 deleted the issue-8517-pr branch October 14, 2024 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecs-patterns): support NLB with TLS listener and target group #30611

feat(ecs-patterns): support NLB with TLS listener and target group #30611

199911 commented Jun 21, 2024

aws-cdk-automation left a comment

agarwal-aashish left a comment

199911 commented Aug 5, 2024

199911 commented Aug 15, 2024

mergify bot commented Aug 15, 2024

moelasmar left a comment •

edited

Loading

moelasmar Sep 8, 2024

199911 Sep 8, 2024

moelasmar Sep 8, 2024

moelasmar Sep 8, 2024 •

edited

Loading

199911 Sep 8, 2024

199911 Oct 1, 2024

moelasmar left a comment

199911 commented Oct 4, 2024 •

edited

Loading

moelasmar Oct 10, 2024

199911 Oct 11, 2024

moelasmar left a comment

mergify bot commented Oct 14, 2024

aws-cdk-automation commented Oct 14, 2024

mergify bot commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

feat(ecs-patterns): support NLB with TLS listener and target group #30611

feat(ecs-patterns): support NLB with TLS listener and target group #30611

Conversation

199911 commented Jun 21, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

agarwal-aashish left a comment

Choose a reason for hiding this comment

199911 commented Aug 5, 2024

199911 commented Aug 15, 2024

mergify bot commented Aug 15, 2024

✅ Pull request refreshed

moelasmar left a comment • edited Loading

Choose a reason for hiding this comment

moelasmar Sep 8, 2024

Choose a reason for hiding this comment

199911 Sep 8, 2024

Choose a reason for hiding this comment

moelasmar Sep 8, 2024

Choose a reason for hiding this comment

moelasmar Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

199911 Sep 8, 2024

Choose a reason for hiding this comment

199911 Oct 1, 2024

Choose a reason for hiding this comment

moelasmar left a comment

Choose a reason for hiding this comment

199911 commented Oct 4, 2024 • edited Loading

moelasmar Oct 10, 2024

Choose a reason for hiding this comment

199911 Oct 11, 2024

Choose a reason for hiding this comment

moelasmar left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 14, 2024

aws-cdk-automation commented Oct 14, 2024

AWS CodeBuild CI Report

mergify bot commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

moelasmar left a comment •

edited

Loading

moelasmar Sep 8, 2024 •

edited

Loading

199911 commented Oct 4, 2024 •

edited

Loading