feat(route53): add cidrRoutingConfig property #34301

tttol · 2025-04-29T21:21:04Z

Issue # (if applicable)

N/A

Reason for this change

The L2 construct of Route 53 resource record set did not have a property cidrRoutingConfig. It was always necessary to set this prop via the L1 Construct when configuring IP-based routing.

Description of changes

added cidrRoutingConfig to RecordSetProps
fixed logic in RecordSet constructor

Describe any new or updated permissions being added

None

Description of how you validated changes

I added new unit test and integration test.

cidr-routing-config.test.ts
integ.cidr-routing-config.ts

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

packages/aws-cdk-lib/aws-route53/lib/cidr-routing-config.ts

badmintoncryer

Thank you for your quick response. I've added some comments and I'll approve this later.

badmintoncryer · 2025-05-06T10:01:04Z

packages/@aws-cdk-testing/framework-integ/test/aws-route53/test/integ.cidr-routing-config.ts

+new IntegTest(app, 'CidrRoutingConfigInteg', {
+  testCases: [stack],
+});
+app.synth();


You don't need this line.

badmintoncryer · 2025-05-06T10:02:42Z

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

+
+```ts
+declare const zone: route53.HostedZone;
+


Could you please add a example of default cidr config??

badmintoncryer · 2025-05-06T10:04:48Z

packages/aws-cdk-lib/aws-route53/lib/cidr-routing-config.ts

+    if (!Token.isUnresolved(props.collectionId) && !COLLECTION_ID_REGEX.test(props.collectionId)) {
+      throw new UnscopedValidationError(`collectionId(${props.collectionId}) is required and must be a valid UUID in the format: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx(8-4-4-4-12 digits)`);
+    }
+    if (!props.locationName || !LOCATION_NAME_REGEX.test(props.locationName)) {


Please take care of the case that location name is a token.

badmintoncryer · 2025-05-06T10:06:45Z

packages/aws-cdk-lib/aws-route53/lib/record-set.ts

+  /**
+   * The object that is specified in resource record set object when you are linking a resource record set to a CIDR location.
+   *
+   * A LocationName with an asterisk “*” can be used to create a default CIDR record. CollectionId is still required for default record.


This line is not needed now by adding default factory method.

I think this line is needed. There are 2 reasons:

The official documentation for AWS::Route53::RecordSet CidrRoutingConfig includes this explanation.

While the default method now correctly sets locationName to *, I think it would be helpful to explicitly write this requirement for default CIDR records. How do you think?

badmintoncryer

Thank you for addressing my comments. This is the final request.

badmintoncryer · 2025-05-07T00:44:17Z

packages/aws-cdk-lib/aws-route53/lib/cidr-routing-config.ts

+    if (!Token.isUnresolved(props.collectionId) && !COLLECTION_ID_REGEX.test(props.collectionId)) {
+      throw new UnscopedValidationError(`collectionId(${props.collectionId}) is required and must be a valid UUID in the format: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx(8-4-4-4-12 digits)`);
+    }
+    if (!Token.isUnresolved(props.locationName) && !props.locationName || !LOCATION_NAME_REGEX.test(props.locationName)) {


I think regex validation must be executed only when token is resolved.

Suggested change

if (!Token.isUnresolved(props.locationName) && !props.locationName || !LOCATION_NAME_REGEX.test(props.locationName)) {

if (!Token.isUnresolved(props.locationName) && (!props.locationName || !LOCATION_NAME_REGEX.test(props.locationName))) {

accepted your suggestion and updated unit test.
43a8858

badmintoncryer

Thanks!

scorbiere

Thanks for your contribution @tttol
I don't see any big blocker but I would like some clarifications (see my other comments). Thank you.

packages/aws-cdk-lib/aws-route53/lib/cidr-routing-config.ts

scorbiere · 2025-05-09T22:41:36Z

packages/aws-cdk-lib/aws-route53/lib/cidr-routing-config.ts

+  /**
+   * Creates a new instance of CidrRoutingConfig
+   */
+  public static new(props: CidrRoutingConfigProps): CidrRoutingConfig {


I wouldn't use new for that method name, especially since it is a commonly used keyword. I would much prefer something like create if we really want to use a static factory strategy.

Suggested change

public static new(props: CidrRoutingConfigProps): CidrRoutingConfig {

public static create(props: CidrRoutingConfigProps): CidrRoutingConfig {

scorbiere · 2025-05-09T22:54:46Z

packages/aws-cdk-lib/aws-route53/lib/cidr-routing-config.ts

+   */
+  readonly locationName: string;
+
+  private constructor(props: CidrRoutingConfigProps) {


I am curious why you are making this constructor private to enforce the utilization of the static factory methods?

I think private constructor has several benefits:

It centralizes instance creation logic ensuring that all objects are constructed in a consistent and controlled manner.

It allows for the implementation of specialized static methods, such as withDefaultLocationName

In addition, I followed the approach used in the GeoLocation class (a property of RecordSetOptions), which also uses a private constructor.

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-route53/lib/geo-location.ts#L72

private constructor(readonly continentCode: Continent | undefined, readonly countryCode: string | undefined, readonly subdivisionCode: string | undefined) {

scorbiere · 2025-05-09T22:56:06Z

packages/aws-cdk-lib/aws-route53/lib/cidr-routing-config.ts

+  /**
+   * The CIDR collection location name.
+   */
+  readonly locationName: string;


If you mark it as optional, you would be able to default it to '*', right?

Yes, we can default it to '*' if it's optional. I have updated locationName to be an optional property.

/** * The CIDR collection location name. */ readonly locationName?: string;

packages/aws-cdk-lib/aws-route53/lib/cidr-routing-config.ts

Co-authored-by: CORBIERE Sebastien <[email protected]>

Pull request has been modified.

scorbiere

Thanks for addressing all my comments and your contribution :)

mergify · 2025-05-12T16:21: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).

mergify · 2025-05-12T16:21:14Z

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 update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

Pull request has been modified.

aws-cdk-automation · 2025-05-12T16:52:23Z

AWS CodeBuild CI Report

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

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

mergify · 2025-05-12T16:52:46Z

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-05-12T16:53:07Z

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.

tttol added 6 commits April 28, 2025 17:19

add prop

7651ea7

add UT

681a4e7

fix ut

205317e

add integ test

13edc49

add integ test

ccadb76

update README

cb22653

aws-cdk-automation requested a review from a team April 29, 2025 21:21

github-actions bot added p2 valued-contributor [Pilot] contributed between 6-12 PRs to the CDK labels Apr 29, 2025

tttol added 2 commits May 1, 2025 06:49

fix error

7606499

update README

e956917

github-actions bot mentioned this pull request May 1, 2025

Monthly PR metrics report #34324

Closed

tttol added 5 commits May 1, 2025 19:40

fix lint error

49a37ea

fix typo

39df4fe

fix UT

6b1101a

fix UT

43f071f

fix README

c89aebf

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

badmintoncryer reviewed May 4, 2025

View reviewed changes

packages/aws-cdk-lib/aws-route53/lib/cidr-routing-config.ts Show resolved Hide resolved

add method for a default CIDR record

0cd648a

badmintoncryer suggested changes May 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 May 6, 2025

update for PR comments

84b1aaa

badmintoncryer suggested changes May 7, 2025

View reviewed changes

tttol added 2 commits May 7, 2025 11:59

fix for the PR comment

43a8858

typo

b503681

badmintoncryer approved these changes May 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 May 7, 2025

scorbiere previously requested changes May 9, 2025

View reviewed changes

scorbiere self-assigned this May 9, 2025

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

Update packages/aws-cdk-lib/aws-route53/lib/cidr-routing-config.ts

c4b51e5

Co-authored-by: CORBIERE Sebastien <[email protected]>

tttol added 2 commits May 11, 2025 06:49

fix for PR comments

a175c61

chore

fd37c6a

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

scorbiere previously approved these changes May 12, 2025

View reviewed changes

Merge branch 'main' into cidr-routing-config

1fe0fda

scorbiere approved these changes May 12, 2025

View reviewed changes

mergify bot merged commit 5cd82f5 into aws:main May 12, 2025
16 of 17 checks passed

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2025

	if (!Token.isUnresolved(props.locationName) && !props.locationName \|\| !LOCATION_NAME_REGEX.test(props.locationName)) {
	if (!Token.isUnresolved(props.locationName) && (!props.locationName \|\| !LOCATION_NAME_REGEX.test(props.locationName))) {

	public static new(props: CidrRoutingConfigProps): CidrRoutingConfig {
	public static create(props: CidrRoutingConfigProps): CidrRoutingConfig {

feat(route53): add cidrRoutingConfig property #34301

feat(route53): add cidrRoutingConfig property #34301

Uh oh!

Conversation

tttol commented Apr 29, 2025

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

Uh oh!

Uh oh!

badmintoncryer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

badmintoncryer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

badmintoncryer left a comment

Choose a reason for hiding this comment

Uh oh!

scorbiere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tttol May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tttol May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scorbiere left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented May 12, 2025

Uh oh!

mergify bot commented May 12, 2025

Uh oh!

aws-cdk-automation commented May 12, 2025

AWS CodeBuild CI Report

Uh oh!

mergify bot commented May 12, 2025

Uh oh!

Uh oh!

github-actions bot commented May 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tttol May 10, 2025 •

edited

Loading

tttol May 10, 2025 •

edited

Loading