feat(elasticloadbalancingv2): support Weighted Random algorithm and Automatic Target Weights for alb #30542

mazyu36 · 2024-06-13T11:16:13Z

Issue # (if applicable)

Closes #29969

Reason for this change

Application Load Balancer supports Weight Random routing Algorithm and Automatic Target Weights. (link)

But current L2 construct does not support it.

Description of changes

Add weighted_random algorithm to ENUM.
Add a flag for setting Automatic Target Weights anomaly mitigation.

Description of how you validated changes

Add unit tests and integ tests.

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

… for alb

lpizzinidev

Thanks 👍 Left comments for minor adjustments

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts

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

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts

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

packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/target-group.test.ts

packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md

…ation-target-group.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

…ation-listener.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

…ation-target-group.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

…ner.test.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

…t-group.test.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

mazyu36 · 2024-06-15T13:28:23Z

@lpizzinidev
Thank you for the review. I have incorporated your suggestions and made the modifications.

mazyu36 · 2024-06-15T13:30:18Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts

@@ -367,13 +367,11 @@ export class ApplicationTargetGroup extends TargetGroupBase implements IApplicat
      this.addTarget(...(props.targets || []));

      if (props.enableAnomalyMitigation !== undefined) {
-        if (props.enableAnomalyMitigation && isWeightedRandomAlgorithm) {
+        if (props.enableAnomalyMitigation && !isWeightedRandomAlgorithm) {


I want to throw an error when anomaly mitigation is enabled and the algorithm is not weighted_random, so I added a '!' before isWeightedRandomAlgorithm.

The codechanges make sense to me but the only thing I'm not so keen on is enforcing the restrictions on the properties:

weighted_random algorithm cannot be used in slow start mode

anomaly mitigation can only be enabled when the weighted_random algorithm is used

with error handling in the constructor. For this current case it works fine because we're not adding much but this can slowly grow over time into a wall of error handling checks as more properties with restrictions are added.

What I had in mind here was to instead create another version of this class that specifically has the weighted_random algorithm set by default and does not allow the user to set other conflicting properties to remove the need for these error checks.

i.e. A class like ApplicationTargetGroupWeightedRandomRouting or something where the anomaly mitigation can be enabled or not without the need for error handling and slow start mode is disabled and cannot be enabled.

Although part of me worries that this may be a bit confusing as a DX to use because we're not providing the option through ApplicationTargetGroup and I'm not sure how many other routing algorithm options there are that we may not have done this for. What do you think?

Thank you for your suggestion.

Personally, I like the current consolidated form of ApplicationTargetGroup.

While there are other routing algorithms such as Least outstanding requests, I think having only weighted random as a separate class would be confusing for users.

Additionally, I think that the current restriction is not complex enough to warrant a separate class.

There are common items with other routing algorithms such as stickiness.enabled, and it's easier to understand when they are grouped together.

For the reasons mentioned above, I personally think the current format is preferable.
However, as you've expressed concern, your suggestion might be better considering the possibility of increased constraints in the future.
But honestly, I'm not sure if they will increase...

Given the above, I'd appreciate your thoughts on this.

Thanks for sharing your thoughts.

After hearing your reasonings I agree that separating out a new class just for this algorithm is probably not necessary. As you mentioned, the restriction is not super complex so I'm okay to accept this change as is.

Thanks @mazyu36!

lpizzinidev

Thanks 👍

paulhcsun

Hi @mazyu36, thank you for your contribution! I just left one comment regarding the error checks in the constructor to enforce a couple of the restrictions of using the weighted_random algorithm. Let me know what you think!

paulhcsun · 2024-07-26T18:48:52Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md

@@ -458,6 +458,24 @@ const tg = new elbv2.ApplicationTargetGroup(this, 'TG', {
 });
 ```

+### Weighted random routing algorithms and automatic target weights for your Application Load Balancer


Can we also document the. restriction that the weighted_random algorithm cannot be used with slow start mode?

Thanks. I've updated docs.

paulhcsun · 2024-07-29T17:20:30Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts

@@ -367,13 +367,11 @@ export class ApplicationTargetGroup extends TargetGroupBase implements IApplicat
      this.addTarget(...(props.targets || []));

      if (props.enableAnomalyMitigation !== undefined) {
-        if (props.enableAnomalyMitigation && isWeightedRandomAlgorithm) {
+        if (props.enableAnomalyMitigation && !isWeightedRandomAlgorithm) {


The codechanges make sense to me but the only thing I'm not so keen on is enforcing the restrictions on the properties:

weighted_random algorithm cannot be used in slow start mode

anomaly mitigation can only be enabled when the weighted_random algorithm is used

with error handling in the constructor. For this current case it works fine because we're not adding much but this can slowly grow over time into a wall of error handling checks as more properties with restrictions are added.

What I had in mind here was to instead create another version of this class that specifically has the weighted_random algorithm set by default and does not allow the user to set other conflicting properties to remove the need for these error checks.

i.e. A class like ApplicationTargetGroupWeightedRandomRouting or something where the anomaly mitigation can be enabled or not without the need for error handling and slow start mode is disabled and cannot be enabled.

Although part of me worries that this may be a bit confusing as a DX to use because we're not providing the option through ApplicationTargetGroup and I'm not sure how many other routing algorithm options there are that we may not have done this for. What do you think?

Pull request has been modified.

aws-cdk-automation · 2024-07-30T04:16:03Z

AWS CodeBuild CI Report

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

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

mazyu36 · 2024-07-30T04:17:20Z

@paulhcsun
Thank you for the review !
I've addressed all your comments.

paulhcsun

Nice work @mazyu36!

And thanks for the review as always @lpizzinidev!

mergify · 2024-07-30T17:46: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).

github-actions · 2024-07-30T17:47:16Z

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.

feat: support Weighted Randdom algorithm and Automatic Target Weights…

0f2eab1

… for alb

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jun 13, 2024

aws-cdk-automation requested a review from a team June 13, 2024 11:16

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Jun 13, 2024

fix: update README

ded7d36

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 13, 2024

Merge branch 'main' into elb-automatic-target-weights-29969

83c0406

lpizzinidev suggested changes Jun 14, 2024

View reviewed changes

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

mazyu36 and others added 8 commits June 15, 2024 05:35

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/applic…

4f7a148

…ation-target-group.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/applic…

4e27419

…ation-listener.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/applic…

14bb6a9

…ation-target-group.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/liste…

9e6f50b

…ner.test.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/liste…

1ef611b

…ner.test.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/targe…

68ca394

…t-group.test.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md

4bcf5b9

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

fix: incorporate review comments

398b91d

mazyu36 force-pushed the elb-automatic-target-weights-29969 branch 2 times, most recently from 2ef710f to 398b91d Compare June 15, 2024 12:48

mazyu36 commented Jun 15, 2024

View reviewed changes

lpizzinidev approved these changes Jun 15, 2024

View reviewed changes

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

github-actions bot mentioned this pull request Jun 17, 2024

Weekly PR metrics report #30566

Closed

github-actions bot mentioned this pull request Jul 1, 2024

Monthly PR metrics report #30715

Closed

mazyu36 added 3 commits July 2, 2024 20:01

chore update integ test name

efb2342

Merge branch 'main' into elb-automatic-target-weights-29969

5ba0fc0

Merge branch 'main' into elb-automatic-target-weights-29969

1ab04b2

update integ test

1a1a23f

paulhcsun self-assigned this Jul 26, 2024

paulhcsun previously requested changes Jul 29, 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 29, 2024

update README

26c08ba

Merge branch 'main' into elb-automatic-target-weights-29969

e1733c1

mazyu36 requested a review from paulhcsun July 30, 2024 04:17

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

paulhcsun approved these changes Jul 30, 2024

View reviewed changes

mergify bot merged commit d0a609d into aws:main Jul 30, 2024
15 checks passed

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

github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2024

mazyu36 deleted the elb-automatic-target-weights-29969 branch July 30, 2024 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(elasticloadbalancingv2): support Weighted Random algorithm and Automatic Target Weights for alb #30542

feat(elasticloadbalancingv2): support Weighted Random algorithm and Automatic Target Weights for alb #30542

mazyu36 commented Jun 13, 2024

lpizzinidev left a comment

mazyu36 commented Jun 15, 2024

mazyu36 Jun 15, 2024 •

edited

Loading

paulhcsun Jul 29, 2024

mazyu36 Jul 30, 2024

paulhcsun Jul 30, 2024

lpizzinidev left a comment

paulhcsun left a comment

paulhcsun Jul 26, 2024

mazyu36 Jul 30, 2024

paulhcsun Jul 29, 2024

aws-cdk-automation commented Jul 30, 2024

mazyu36 commented Jul 30, 2024

paulhcsun left a comment

mergify bot commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

feat(elasticloadbalancingv2): support Weighted Random algorithm and Automatic Target Weights for alb #30542

feat(elasticloadbalancingv2): support Weighted Random algorithm and Automatic Target Weights for alb #30542

Conversation

mazyu36 commented Jun 13, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

lpizzinidev left a comment

Choose a reason for hiding this comment

mazyu36 commented Jun 15, 2024

mazyu36 Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

paulhcsun Jul 29, 2024

Choose a reason for hiding this comment

mazyu36 Jul 30, 2024

Choose a reason for hiding this comment

paulhcsun Jul 30, 2024

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

paulhcsun left a comment

Choose a reason for hiding this comment

paulhcsun Jul 26, 2024

Choose a reason for hiding this comment

mazyu36 Jul 30, 2024

Choose a reason for hiding this comment

paulhcsun Jul 29, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Jul 30, 2024

AWS CodeBuild CI Report

mazyu36 commented Jul 30, 2024

paulhcsun left a comment

Choose a reason for hiding this comment

mergify bot commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

mazyu36 Jun 15, 2024 •

edited

Loading