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.

Sign up for GitHub

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(elasticloadbalancingv2): support Weighted Random algorithm and Automatic Target Weights for alb #30542

Merged
merged 17 commits into from
Jul 30, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0f2eab1
feat: support Weighted Randdom algorithm and Automatic Target Weights…
mazyu36 Jun 13, 2024
ded7d36
fix: update README
mazyu36 Jun 13, 2024
83c0406
Merge branch 'main' into elb-automatic-target-weights-29969
mazyu36 Jun 13, 2024
4f7a148
Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/applic…
mazyu36 Jun 14, 2024
4e27419
Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/applic…
mazyu36 Jun 14, 2024
14bb6a9
Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/applic…
mazyu36 Jun 14, 2024
9e6f50b
Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/liste…
mazyu36 Jun 14, 2024
1ef611b
Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/liste…
mazyu36 Jun 14, 2024
68ca394
Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/targe…
mazyu36 Jun 14, 2024
4bcf5b9
Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md
mazyu36 Jun 14, 2024
398b91d
fix: incorporate review comments
mazyu36 Jun 15, 2024
efb2342
chore update integ test name
mazyu36 Jul 2, 2024
5ba0fc0
Merge branch 'main' into elb-automatic-target-weights-29969
mazyu36 Jul 2, 2024
1ab04b2
Merge branch 'main' into elb-automatic-target-weights-29969
mazyu36 Jul 24, 2024
1a1a23f
update integ test
mazyu36 Jul 24, 2024
26c08ba
update README
mazyu36 Jul 30, 2024
e1733c1
Merge branch 'main' into elb-automatic-target-weights-29969
mazyu36 Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ const tg = new elbv2.ApplicationTargetGroup(this, 'TargetGroup', {
loadBalancingAlgorithmType: elbv2.TargetGroupLoadBalancingAlgorithmType.WEIGHTED_RANDOM,
enableAnomalyMitigation: true,
});
```

## Using Lambda Targets

Expand Down
4 changes: 1 addition & 3 deletions packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

@mazyu36 mazyu36 Jun 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

throw new Error('Anomaly mitigation is only available when `loadBalancingAlgorithmType` is `TargetGroupLoadBalancingAlgorithmType.WEIGHTED_RANDOM`.');
}
this.setAttribute('load_balancing.algorithm.anomaly_mitigation', props.enableAnomalyMitigation ? 'on' : 'off');
}
}

}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1861,7 +1861,7 @@ describe('tests', () => {
});
});

test('Can add targets with weight_random algorithm and anomaly mitigation disabled, () => {
test('Can add targets with weight_random algorithm and anomaly mitigation disabled', () => {
// GIVEN
const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/target-group.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ describe('tests', () => {
});
});

test('weight_random algorithm and anomaly mitigation is disabled, () => {
test('weight_random algorithm and anomaly mitigation is disabled', () => {
// GIVEN
const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
Expand Down