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.

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

Merged
merged 22 commits into from
Oct 14, 2024

Conversation

199911
Copy link
Contributor

@199911 199911 commented Jun 21, 2024

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

  • 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


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

@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Jun 21, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team June 21, 2024 08:03
@github-actions 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
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@199911 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
@aws-cdk-automation aws-cdk-automation dismissed their stale review June 21, 2024 08:10

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation 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
Copy link

@agarwal-aashish agarwal-aashish left a 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.

@199911
Copy link
Contributor Author

199911 commented Aug 5, 2024

Hi @agarwal-aashish

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

@199911
Copy link
Contributor Author

199911 commented Aug 15, 2024

@Mergifyio refresh

Copy link
Contributor

mergify bot commented Aug 15, 2024

refresh

✅ Pull request refreshed

Copy link
Contributor

@moelasmar moelasmar left a comment

Choose a reason for hiding this comment

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

left minor comment

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 8, 2024
Comment on lines 42 to 45
listenerCertificate: new Certificate(stack, 'myCert', {
domainName,
validation,
}),
Copy link
Contributor

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.

Copy link
Contributor Author

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

const certArn = process.env.CDK_INTEG_CERT_ARN || process.env.CERT_ARN;

and then import a certificate with
const certificate = Certificate.fromCertificateArn(testCase, 'Cert', certArn);

Copy link
Contributor

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.

Copy link
Contributor

@moelasmar moelasmar Sep 8, 2024

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

@mergify mergify bot dismissed moelasmar’s stale review September 10, 2024 22:02

Pull request has been modified.

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

@moelasmar moelasmar left a 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

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 11, 2024
@mergify mergify bot dismissed moelasmar’s stale review September 30, 2024 16:20

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 30, 2024
@199911 199911 requested a review from moelasmar October 1, 2024 12:18
@199911
Copy link
Contributor Author

199911 commented Oct 4, 2024

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.

* 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.');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@moelasmar moelasmar left a comment

Choose a reason for hiding this comment

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

it looks good to me

Copy link
Contributor

mergify bot commented Oct 14, 2024

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 aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 14, 2024
@aws-cdk-automation
Copy link
Collaborator

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 mergify bot merged commit f4f8abc into aws:main Oct 14, 2024
15 checks passed
Copy link
Contributor

mergify bot commented Oct 14, 2024

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

Copy link

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 github-actions bot locked as resolved and limited conversation to collaborators Oct 14, 2024
@199911 199911 deleted the issue-8517-pr branch October 14, 2024 07:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ecs-patterns] - HTTPS between NLB and fargate service when using NetworkLoadBalancedFargateService
5 participants