feat(cloudfront): add origin group selection criteria to L2 Distribution and L2 OriginGroup #32740

jamiepmullan · 2025-01-04T21:50:55Z

Issue # (if applicable)

Closes #.

Reason for this change

add origin selection criteria to L2 Distribution & L2 OriginGroup
add unit + integration tests

Description of changes

Add Selection Criteria to Distribution L2 construct. CFN docs can be found here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-distribution-origingroup.html#cfn-cloudfront-distribution-origingroup-selectioncriteria

Description of how you validated changes

Added unit + integration tests for this new feature - by default it can be undefined, so shouldn't impact other configurations.

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

add integration test

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.

codecov · 2025-01-04T22:05:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.78%. Comparing base (78bcd09) to head (52a38e3).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32740   +/-   ##
=======================================
  Coverage   80.78%   80.78%           
=======================================
  Files         232      232           
  Lines       14111    14111           
  Branches     2453     2453           
=======================================
  Hits        11400    11400           
  Misses       2431     2431           
  Partials      280      280

Flag	Coverage Δ
suite.unit	`80.78% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`79.51% <ø> (ø)`
packages/aws-cdk-lib/core	`82.17% <ø> (ø)`

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

… better with existing interfaces

lpizzinidev

Thanks 👍 Left some comments for adjustments

lpizzinidev · 2025-01-06T16:16:11Z

packages/aws-cdk-lib/aws-cloudfront/lib/origin.ts

+ *
+ * @default - none


Suggested change

*

* @default - none

enum declarations don't need a @default

my bad! removed

lpizzinidev · 2025-01-06T16:16:57Z

packages/aws-cdk-lib/aws-cloudfront/lib/origin.ts

+  DEFAULT='default',
+  /**
+   * Selection based on media quality.
+   */
+  MEDIA_QUALITY_BASED='media-quality-based',


Suggested change

DEFAULT='default',

/**

* Selection based on media quality.

*/

MEDIA_QUALITY_BASED='media-quality-based',

DEFAULT = 'default',

/**

* Selection based on media quality.

*/

MEDIA_QUALITY_BASED = 'media-quality-based',

Formatting.

lpizzinidev · 2025-01-06T16:18:31Z

packages/aws-cdk-lib/aws-cloudfront/lib/origin.ts

+   * The failover selection for the Distribution.
+   *
+   * @default - nothing is returned


Suggested change

* The failover selection for the Distribution.

*

* @default - nothing is returned

* The selection criteria for the origin group.

*

* @see https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/high_availability_origin_failover.html#concept_origin_groups.creating

*

* @default - OriginSelectionCriteria.DEFAULT

I assume CFN will enforce default if not specified by CDK?
Can you please align the documentation in the other variable declarations as well?

Yeah - if it is undefined, then it'll default to default - even if we pass undefined, do you want the docs to reflect the "default" setting?

lpizzinidev · 2025-01-06T16:24:34Z

packages/aws-cdk-lib/aws-cloudfront/test/selection-criteria-origin.test.ts

I think these test cases should be added to the existing distribution.test.ts file.
Also, the Ensure default is undefined case seems redundant.

I've moved them to distribution.test.ts - i removed that test, but have added in a test where "default" is explicitly set.

lpizzinidev

Thanks 👍 Please update the documentation for the variable in origin-group.ts as well

lpizzinidev · 2025-01-07T16:17:56Z

packages/aws-cdk-lib/aws-cloudfront-origins/lib/origin-group.ts

+  /**
+   * The selection criteria for the origin group.
+   *
+   * @default - nothing is returned


Suggested change

* @default - nothing is returned

* @see https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/high_availability_origin_failover.html#concept_origin_groups.creating

*

* @default - OriginSelectionCriteria.DEFAULT

done! thanks!

jamiepmullan · 2025-01-21T19:50:06Z

Hey, is there anything else I need to complete to get a maintainer review?

gracelu0

Thanks for contributing @jamiepmullan . Overall this looks good to me, just a couple of minor comments on the tests/formatting. Once addressed I'm happy to approve.

gracelu0 · 2025-01-22T23:04:25Z

packages/aws-cdk-lib/aws-cloudfront/test/distribution.test.ts

@@ -1434,3 +1437,93 @@ describe('attachWebAclId', () => {
  });

 });
+
+test('Selection criteria does set Media Quality Based failover for Origin Group', () => {


Could we move these tests to origin-group.test.ts instead? I was going to ask to add unit tests for origin group but I think it would be redundant with these.

packages/aws-cdk-lib/aws-cloudfront/lib/origin.ts

gracelu0 · 2025-01-22T23:21:06Z

...ng/framework-integ/test/aws-cloudfront-origins/test/integ.origin-group-selection-criteria.ts

+  },
+});
+
+new IntegTest(app, 'origin-group-selection-criteria', {


Could we add an assertion here to call getDistributionConfig and check the value of SelectionCriteria is as expected? An example integ test you can use for reference: https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/test/aws-cloudfront-origins/test/integ.s3-origin-oac.ts#L25

I have tried to do this but SelectionCriteria isn't returned at the moment - I wonder if the aws-sdk needs updating? Apologies, not deep dived this part of CDK yet

@gracelu0 - I've taken another look, and dived into the logs in CloudWatch - but the JSON body returned on the call doesn't include the SelectionCriteria - any advice on trying to get it? Am I missing something?

Hmm yea I didn't see it returned when using AWS CLI to call get-distribution-config but after upgrading the CLI version to the latest SelectionCriteria was included in the response. Let me check for the integration test framework, if there's no quick workaround then I'm good to approve without the assertion as I verified the test deploys and correctly configures the setting.

gracelu0 · 2025-01-22T23:27:43Z

packages/aws-cdk-lib/aws-cloudfront/lib/distribution.ts

+        this.addOriginGroup(originGroupId,
+          originBindConfig.failoverConfig.statusCodes,
+          originId, failoverOriginId,
+          originBindConfig.selectionCriteria);


Suggested change

this.addOriginGroup(originGroupId,

originBindConfig.failoverConfig.statusCodes,

originId, failoverOriginId,

originBindConfig.selectionCriteria);

this.addOriginGroup(

originGroupId,

originBindConfig.failoverConfig.statusCodes,

originId,

failoverOriginId,

originBindConfig.selectionCriteria

);

gracelu0 · 2025-01-22T23:29:13Z

packages/aws-cdk-lib/aws-cloudfront/lib/distribution.ts

+  private addOriginGroup(originGroupId: string,
+    statusCodes: number[] | undefined,
+    originId: string, failoverOriginId: string,
+    selectionCriteria: OriginSelectionCriteria | undefined): void {


Suggested change

private addOriginGroup(originGroupId: string,

statusCodes: number[] | undefined,

originId: string, failoverOriginId: string,

selectionCriteria: OriginSelectionCriteria | undefined): void {

private addOriginGroup(

originGroupId: string,

statusCodes: number[] | undefined,

originId: string,

failoverOriginId: string,

selectionCriteria: OriginSelectionCriteria | undefined

): void {

Pull request has been modified.

gracelu0

Thank you for contributing!

mergify · 2025-01-28T21:36:31Z

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 · 2025-01-28T21:36:35Z

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

gracelu0 · 2025-01-28T21:41:01Z

@Mergifyio update

mergify · 2025-01-28T21:41:09Z

update

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
@jamiepmullan needs to authorize modification on its head branch.

mergify · 2025-01-28T21:41:49Z

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.

mergify · 2025-01-28T22:17:38Z

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.

mergify · 2025-01-28T23:10:05Z

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 · 2025-01-28T23:40:50Z

AWS CodeBuild CI Report

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

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

mergify · 2025-01-28T23:40: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).

github-actions · 2025-01-28T23:41: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.

add origin selection criteria to L2 Distribution

e8d0f65

add integration test

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Jan 4, 2025

aws-cdk-automation requested a review from a team January 4, 2025 21:51

github-actions bot added the p2 label Jan 4, 2025

aws-cdk-automation previously requested changes Jan 4, 2025

View reviewed changes

jamiepmullan marked this pull request as draft January 4, 2025 22:05

Add readme

e8f669a

jamiepmullan marked this pull request as ready for review January 4, 2025 22:43

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

rename OriginGroupSelectionCriteria to OriginSelectionCriteria to fit…

6fb2474

… better with existing interfaces

jamiepmullan changed the title ~~feat(cloudfront): add origin group selection criteria to L2 Distribution~~ feat(cloudfront): add origin group selection criteria to L2 Distribution and L2 OriginGroup Jan 5, 2025

lpizzinidev suggested changes Jan 6, 2025

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 Jan 6, 2025

jamiepmullan added 2 commits January 6, 2025 17:21

fixes based on review feedback

7ec5343

Merge branch 'main' into feat/origin-group-selection-criteria

7b81e82

jamiepmullan requested a review from lpizzinidev January 6, 2025 18:50

lpizzinidev approved these changes Jan 7, 2025

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 7, 2025

jamiepmullan added 6 commits January 7, 2025 16:23

added docs

3c90fa5

Merge branch 'main' into feat/origin-group-selection-criteria

739b6f9

Merge branch 'main' into feat/origin-group-selection-criteria

b8ebf3e

Merge branch 'main' into feat/origin-group-selection-criteria

6cc21f0

Merge branch 'main' into feat/origin-group-selection-criteria

27d1493

Merge branch 'main' into feat/origin-group-selection-criteria

d3d1a7f

Merge branch 'main' into feat/origin-group-selection-criteria

ac6dd34

gracelu0 self-assigned this Jan 22, 2025

gracelu0 previously requested changes Jan 22, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 22, 2025

Merge branch 'main' into feat/origin-group-selection-criteria

0c8fe0d

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 23, 2025

jamiepmullan added 4 commits January 23, 2025 20:38

fixes

c884704

Merge branch 'main' into feat/origin-group-selection-criteria

8af9874

Merge branch 'main' into feat/origin-group-selection-criteria

6206a10

Merge branch 'main' into feat/origin-group-selection-criteria

d8ec4f3

gracelu0 previously approved these changes Jan 28, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 28, 2025

Merge branch 'main' into feat/origin-group-selection-criteria

5821523

gracelu0 previously approved these changes Jan 28, 2025

View reviewed changes

Merge branch 'main' into feat/origin-group-selection-criteria

6f4337c

gracelu0 approved these changes Jan 28, 2025

View reviewed changes

Merge branch 'main' into feat/origin-group-selection-criteria

52a38e3

mergify bot merged commit 1b35c4e into aws:main Jan 28, 2025
20 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2025

-   * @default - nothing is returned
+   * @see https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/high_availability_origin_failover.html#concept_origin_groups.creating
+   *
+   * @default - OriginSelectionCriteria.DEFAULT

feat(cloudfront): add origin group selection criteria to L2 Distribution and L2 OriginGroup #32740

feat(cloudfront): add origin group selection criteria to L2 Distribution and L2 OriginGroup #32740

Conversation

jamiepmullan commented Jan 4, 2025 • edited Loading

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

codecov bot commented Jan 4, 2025 • edited Loading

Codecov Report

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiepmullan commented Jan 21, 2025

gracelu0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gracelu0 left a comment

Choose a reason for hiding this comment

mergify bot commented Jan 28, 2025

mergify bot commented Jan 28, 2025

gracelu0 commented Jan 28, 2025

mergify bot commented Jan 28, 2025

❌ Pull request can't be updated with latest base branch changes

mergify bot commented Jan 28, 2025

mergify bot commented Jan 28, 2025

mergify bot commented Jan 28, 2025

aws-cdk-automation commented Jan 28, 2025

AWS CodeBuild CI Report

mergify bot commented Jan 28, 2025

github-actions bot commented Jan 28, 2025

jamiepmullan commented Jan 4, 2025 •

edited

Loading

codecov bot commented Jan 4, 2025 •

edited

Loading