feat(route53): ip-based routing #28833

badmintoncryer · 2024-01-23T20:44:09Z

In this PR, I have implemented support for IP-based routing.
We can configure it from the cidrRoutingConfig as shown in the following example:

declare const myZone: route53.HostedZone;

const record1 = new route53.ARecord(this, 'ARecordIpBased1', {
  zone: myZone,
  recordName: 'test',
  target: route53.RecordTarget.fromIpAddresses('1.2.3.4'),
  cidrRoutingConfig: {
    cidrList: ['192.168.1.0/24', '192.168.16.0/20'],
    locationName: 'TokyoServer',
    collectionName: 'myCollection',
  },
});

// You can also add a new Location to an existing CidrCollection
// Ensure all locations for the same record set name and type are part of the same CIDR collection to guarantee consistent routing.
const record2 = new route53.ARecord(this, 'ARecordIpBased2', {
  zone: myZone,
  recordName: 'test',
  target: route53.RecordTarget.fromIpAddresses('2.3.4.5'),
  cidrRoutingConfig: {
    cidrList: ['192.168.2.0/24', '192.168.48.0/20'],
    locationName: 'LondonServer',
    collection: record1.cidrCollection,
  },
});

// To define a zero bit CIDR block (0.0.0.0/0 or ::/0), use the default ("*") location.
const record3 = new route53.ARecord(this, 'ARecordIpBased3', {
  zone: myZone,
  recordName: 'default',
  target: route53.RecordTarget.fromIpAddresses('1.2.3.4'),
  cidrRoutingConfig: {
    locationName: '*',
  },
});

Question

As mentioned above, the existing cidrCollection can be passed as collection argument.
During this process, a new Location is added to the existing Locations in the cidrCollection.
This code works, but I am not confident about the implementation approach. I would greatly appreciate any candid feedback.

if (cidrRoutingConfig.collection) {
      this._cidrCollection = cidrRoutingConfig.collection;
      const currentLocations = this._cidrCollection.locations ?? [];
      const locationsAsArray = Array.isArray(currentLocations) ? currentLocations : [currentLocations];
      this._cidrCollection.addPropertyOverride('Locations', [...locationsAsArray.map((location) => {
        // Since the location is either CfnCidrCollection.LocationProperty or IResolvable,
        // use a type guard function to ascertain its exact type.
        if ('cidrList' in location && 'locationName' in location) {
          return {
            CidrList: location.cidrList,
            LocationName: location.locationName,
          };
        }
        return location;
      }), {
        CidrList: cidrList,
        LocationName: locationName,
      }]);
    }

Closes #28801.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

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.

badmintoncryer · 2024-01-28T14:10:26Z

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

+   *
+   * @default - Create a new CIDR Collection
+   */
+  collection?: CfnCidrCollection;


I understand that it is not preferable to accept L1 resources as arguments. However, there is no L2 construct for CidrCollection, I am using CfnCidrCollection as an argument.

I really wanted to accept an ICidrCollection as an argument. (Is it okay to create an L2 of CidrCollection in this PR?)

Yes, that's fine. Though, it looks like the CidrCollection would just be the first two props here.

aws-cdk-automation · 2024-02-08T00:08:45Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2024-02-09T00:08:25Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2024-02-10T00:08:26Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

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.

aws-cdk-automation · 2024-04-01T00:09:19Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

TheRealAmazonKendra

Thank you for your contribution. I see that you've been making progressive changes in this package and we very much appreciate that work. I do see, however, that a lot of props have been added that interact with each other. I think I'd like to know what else you plan to add so that we can work on an overall contract for all those features. Just adding additional optional props that don't always work together is a pattern we avoid now because it isn't a very good user experience.

TheRealAmazonKendra · 2024-04-16T23:37:41Z

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

+  cidrList?: string[];
+  /**
+   * The name of the location.
+   *
+   * When '*' is specified, it is treated as the default location.
+   * In this case, the cidrList cannot be specified.
+   */
+  locationName: string;


We should't have two props that are reliant on the value of the other to work or not work. Since these two seem to be tightly coupled, we want the contract to enforce that.

TheRealAmazonKendra · 2024-04-16T23:38:16Z

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

+  collectionName?: string;
+  /**
+   * Existing Cidr Collection.
+   *
+   * Use this to add a new Location to an existing Cidr Collection.
+   * Note that for IP-based routing, all resource record sets for the same record set name and type must reference the same CIDR collection.
+   *
+   * @see https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/routing-policy-ipbased.html
+   *
+   * @default - Create a new CIDR Collection
+   */
+  collection?: CfnCidrCollection;


Same comment as above. The peculiarities of how these four props interact should be enforced in the contract.

TheRealAmazonKendra · 2024-04-16T23:42:37Z

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

+   *
+   * @default - Create a new CIDR Collection
+   */
+  collection?: CfnCidrCollection;


Yes, that's fine. Though, it looks like the CidrCollection would just be the first two props here.

TheRealAmazonKendra · 2024-04-16T23:50:14Z

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

@@ -302,7 +354,14 @@ export class RecordSet extends Resource implements IRecordSet {
    if (props.setIdentifier && (props.setIdentifier.length < 1 || props.setIdentifier.length > 128)) {
      throw new Error(`setIdentifier must be between 1 and 128 characters long, got: ${props.setIdentifier.length}`);
    }
-    if (props.setIdentifier && props.weight === undefined && !props.geoLocation && !props.region && !props.multiValueAnswer) {
+    if (


We have a problem with the contract here already. I see that a lot of these were newly added. We might need to deprecate a bunch of these and restructure the props here. We shouldn't have so many mutually exclusive options.

aws-cdk-automation · 2024-05-08T00:07:31Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

badmintoncryer · 2024-05-08T00:43:35Z

oh.. I've forgotten this.

Pull request has been modified.

aws-cdk-automation · 2024-05-14T00:23:30Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2024-06-05T00:08:44Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2024-06-12T00:09:46Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

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

aws-cdk-automation requested a review from a team January 23, 2024 20:44

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Jan 23, 2024

aws-cdk-automation previously requested changes Jan 23, 2024

View reviewed changes

badmintoncryer force-pushed the 28801-ipBasedRouting branch 3 times, most recently from 130f681 to 660e616 Compare January 26, 2024 15:37

badmintoncryer marked this pull request as ready for review January 26, 2024 17:47

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

badmintoncryer commented Jan 28, 2024

View reviewed changes

badmintoncryer force-pushed the 28801-ipBasedRouting branch from e4994a7 to 0cb632b Compare January 29, 2024 16:41

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

Monthly PR metrics report - Jan 2024 #28952

Closed

badmintoncryer force-pushed the 28801-ipBasedRouting branch from 61d6b14 to 4d86430 Compare February 10, 2024 06:58

aws-cdk-automation previously requested changes Feb 10, 2024

View reviewed changes

badmintoncryer changed the title ~~feat(route53): IP-based routing~~ feat(route53): ip-based routing Feb 10, 2024

badmintoncryer mentioned this pull request Feb 12, 2024

support routing policy setting for ARecord class #4391

Open

TheRealAmazonKendra previously requested changes Apr 16, 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 Apr 16, 2024

badmintoncryer marked this pull request as draft May 10, 2024 03:12

feat: define ip-based routing props

4eccd56

badmintoncryer added 9 commits May 10, 2024 12:18

temp: existing collection

d004e5b

fix

a722124

refactor and add test

2a86dd1

feat: existing collection

d5fd1bd

test: unit test

c623f43

fix: unused lines

a670df4

test: integ test

203b86e

docs: update readme

56f120f

refactor

2e05009

badmintoncryer force-pushed the 28801-ipBasedRouting branch from ff41bbf to 2e05009 Compare May 10, 2024 03:20

badmintoncryer added 6 commits May 11, 2024 23:23

feat: add RoutingConfiguration class

1e14c41

refactoring

d023d97

refactoring

fb569f2

refactoring

c834bcf

fix: unit test

4284304

refacotring

b6df5b2

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jun 12, 2024

aws-cdk-automation closed this Jun 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(route53): ip-based routing #28833

feat(route53): ip-based routing #28833

badmintoncryer commented Jan 23, 2024 •

edited

Loading

aws-cdk-automation left a comment

badmintoncryer Jan 28, 2024 •

edited

Loading

TheRealAmazonKendra Apr 16, 2024

aws-cdk-automation commented Feb 8, 2024

aws-cdk-automation commented Feb 9, 2024

aws-cdk-automation commented Feb 10, 2024

aws-cdk-automation left a comment

aws-cdk-automation commented Apr 1, 2024

TheRealAmazonKendra left a comment

TheRealAmazonKendra Apr 16, 2024

TheRealAmazonKendra Apr 16, 2024

TheRealAmazonKendra Apr 16, 2024

TheRealAmazonKendra Apr 16, 2024

aws-cdk-automation commented May 8, 2024

badmintoncryer commented May 8, 2024

aws-cdk-automation commented May 14, 2024

aws-cdk-automation commented Jun 5, 2024

aws-cdk-automation commented Jun 12, 2024

feat(route53): ip-based routing #28833

feat(route53): ip-based routing #28833

Conversation

badmintoncryer commented Jan 23, 2024 • edited Loading

Question

aws-cdk-automation left a comment

Choose a reason for hiding this comment

badmintoncryer Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra Apr 16, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 8, 2024

aws-cdk-automation commented Feb 9, 2024

aws-cdk-automation commented Feb 10, 2024

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 1, 2024

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Apr 16, 2024

Choose a reason for hiding this comment

TheRealAmazonKendra Apr 16, 2024

Choose a reason for hiding this comment

TheRealAmazonKendra Apr 16, 2024

Choose a reason for hiding this comment

TheRealAmazonKendra Apr 16, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented May 8, 2024

badmintoncryer commented May 8, 2024

aws-cdk-automation commented May 14, 2024

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 5, 2024

aws-cdk-automation commented Jun 12, 2024

badmintoncryer commented Jan 23, 2024 •

edited

Loading

badmintoncryer Jan 28, 2024 •

edited

Loading