fix(elasticloadbalancingv2-targets): add AlbListenerTarget for NLBs, deprecate AlbTarget due to ALB listener race conditions (#17208) by stm29 · Pull Request #30396 · aws/aws-cdk

stm29 · 2024-05-31T08:51:39Z

Issue # (if applicable)

Closes #17208 .

Description of changes

This PR replaces AlbTarget with AlbListenerTarget, allowing us to automatically add a dependency between the provided ALB listener and the associated NLB target group. Without such a dependency stacks may fail to deploy if the listener is not created when the target group attempts to be created.
Also, As described - (aws-elasticloadbalancingv2): Cannot use a single application load balancer as a target for the network load balancer #17208 (comment) , it will handle graceful deletion of resources.

Description of how you validated changes

Added UnitTests
Ran integrationTests as below,

$ yarn integ aws-elasticloadbalancingv2-targets/test/integ.alb-listner-target.js --update-on-failed

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

…Bs, deprecate AlbTarget due to ALB listener race conditions (aws#17208)

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.

stm29 · 2024-06-11T05:36:46Z

@paulhcsun , I am on my initial stages of my contribution, where should I reach out to get this reviewed by community?

mmuller88

This looks great

packages/aws-cdk-lib/aws-elasticloadbalancingv2-targets/lib/alb-target.ts

...ing/framework-integ/test/aws-elasticloadbalancingv2-targets/test/integ.alb-listner-target.ts

packages/aws-cdk-lib/aws-elasticloadbalancingv2-targets/lib/alb-target.ts

tmokmss

Thanks for working on this! I (as a community reviewer) made some comments on an alternative approach. I'm not super familiar with NLB + ALB target, so sorry if my comments are irrelevant (at least I read this doc 🙏).

packages/aws-cdk-lib/aws-elasticloadbalancingv2-targets/lib/alb-target.ts

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts

packages/aws-cdk-lib/aws-elasticloadbalancingv2-targets/lib/alb-target.ts

tmokmss · 2024-06-17T13:27:05Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2-targets/lib/alb-target.ts

+   * @param albListener The application load balancer listener to target.
+   */
+  constructor(private albListener: elbv2.ApplicationListener) {
+    super(albListener.loadBalancer.loadBalancerArn, albListener.port);


(memo) using albListener.port here seems a right thing to do. (previously it was not validated at all that a corresponding listener to the port exists)

the Application Load Balancer you choose from the list must have a listener on the same port as the target group you’re creating.
https://docs.aws.amazon.com/elasticloadbalancing/latest/network/application-load-balancer-target.html

…arget or AlbArnTarget based on usecase

TheRealAmazonKendra

I do love a good deprecation. Thanks for your contribution!

mergify · 2024-07-25T01:24:14Z

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

mergify · 2024-07-25T01:28:09Z

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

Pull request has been modified.

stm29 · 2024-07-25T06:33:31Z

@TheRealAmazonKendra , I Merged main - branch commits in this PR.
I did this because, it was alerting current branch is not in sync unable to Merge.

Can you re-review this?

stm29 · 2024-07-28T00:26:57Z

@TheRealAmazonKendra , would you mind just re-approving this?

And regarding merging the changes, will you be merging this PR?, why because I did all these changes because I got confused with mergify messages...

mergify · 2024-08-14T18:55:53Z

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

mergify · 2024-08-14T18:59:59Z

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

shikha372 · 2024-08-14T19:57:21Z

@Mergifyio update

mergify · 2024-08-14T19:57:37Z

update

❌ Mergify doesn't have permission to update

Details

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/github-merit-badger.yml without workflows permission

stm29 · 2024-08-14T19:58:56Z

@shikha372 seems there is permission issue, should I add some permission?

Pull request has been modified.

mergify · 2024-08-15T17:37:48Z

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-08-15T18:08:33Z

AWS CodeBuild CI Report

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

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

mergify · 2024-08-15T18:08:50Z

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-08-15T18:09:25Z

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.

fix(aws-elasticloadbalancingv2-targets): add AlbListenerTarget for NL…

c3b0a61

…Bs, deprecate AlbTarget due to ALB listener race conditions (aws#17208)

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

aws-cdk-automation requested a review from a team May 31, 2024 08:51

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels May 31, 2024

aws-cdk-automation previously requested changes May 31, 2024

View reviewed changes

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

stm29 mentioned this pull request May 31, 2024

(aws-elasticloadbalancingv2): Cannot use a single application load balancer as a target for the network load balancer #17208

Closed

This was referenced Jun 1, 2024

Monthly PR metrics report #30414

Closed

Weekly PR metrics report #30434

Closed

paulhcsun added p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. and removed p1 pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Jun 10, 2024

mmuller88 approved these changes Jun 12, 2024

View reviewed changes

msambol reviewed Jun 17, 2024

View reviewed changes

packages/aws-cdk-lib/aws-elasticloadbalancingv2-targets/lib/alb-target.ts Outdated Show resolved Hide resolved

msambol reviewed Jun 17, 2024

View reviewed changes

packages/aws-cdk-lib/aws-elasticloadbalancingv2-targets/lib/alb-target.ts Outdated Show resolved Hide resolved

msambol reviewed Jun 17, 2024

View reviewed changes

packages/aws-cdk-lib/aws-elasticloadbalancingv2-targets/lib/alb-target.ts Outdated Show resolved Hide resolved

msambol reviewed Jun 17, 2024

View reviewed changes

...ing/framework-integ/test/aws-elasticloadbalancingv2-targets/test/integ.alb-listner-target.ts Outdated Show resolved Hide resolved

msambol reviewed Jun 17, 2024

View reviewed changes

packages/aws-cdk-lib/aws-elasticloadbalancingv2-targets/lib/alb-target.ts Outdated Show resolved Hide resolved

tmokmss reviewed Jun 17, 2024

View reviewed changes

stm29 and others added 5 commits June 17, 2024 20:53

Merge branch 'aws:main' into fix-elbv2-target-17208

b2e861e

fixed comments

ae85df0

Modified README.md

43d9f48

Empty commit message to trigger AWS CodeBuild CI Report

1e7381d

Modified Comment message, as such it clearly says to use AlbListenerT…

e34b4ac

…arget or AlbArnTarget based on usecase

TheRealAmazonKendra previously approved these changes Jul 25, 2024

View reviewed changes

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

Merge branch 'main' into fix-elbv2-target-17208

f1067fe

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

shikha372 self-assigned this Aug 14, 2024

shikha372 previously approved these changes Aug 14, 2024

View reviewed changes

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

Merge branch 'main' into fix-elbv2-target-17208

e329a10

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

shikha372 approved these changes Aug 15, 2024

View reviewed changes

Merge branch 'main' into fix-elbv2-target-17208

02b4aa5

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

mergify bot merged commit 1fca1e5 into aws:main Aug 15, 2024

github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2024

stm29 deleted the fix-elbv2-target-17208 branch August 15, 2024 18:19

Conversation

stm29 commented May 31, 2024

Issue # (if applicable)

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

stm29 commented Jun 11, 2024

Uh oh!

mmuller88 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tmokmss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tmokmss Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jul 25, 2024

Uh oh!

mergify bot commented Jul 25, 2024

Uh oh!

stm29 commented Jul 25, 2024

Uh oh!

stm29 commented Jul 28, 2024

Uh oh!

mergify bot commented Aug 14, 2024

Uh oh!

mergify bot commented Aug 14, 2024

Uh oh!

shikha372 commented Aug 14, 2024

Uh oh!

mergify bot commented Aug 14, 2024

❌ Mergify doesn't have permission to update

Uh oh!

stm29 commented Aug 14, 2024

Uh oh!

mergify bot commented Aug 15, 2024

Uh oh!

aws-cdk-automation commented Aug 15, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 15, 2024

Uh oh!

github-actions bot commented Aug 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants