feat(route53): ip-based routing#28833
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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.
130f681 to
660e616
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| * | ||
| * @default - Create a new CIDR Collection | ||
| */ | ||
| collection?: CfnCidrCollection; |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
Yes, that's fine. Though, it looks like the CidrCollection would just be the first two props here.
e4994a7 to
0cb632b
Compare
|
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. |
2 similar comments
|
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. |
|
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. |
61d6b14 to
4d86430
Compare
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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.
|
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
left a comment
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
Same comment as above. The peculiarities of how these four props interact should be enforced in the contract.
| * | ||
| * @default - Create a new CIDR Collection | ||
| */ | ||
| collection?: CfnCidrCollection; |
There was a problem hiding this comment.
Yes, that's fine. Though, it looks like the CidrCollection would just be the first two props here.
| 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 ( |
There was a problem hiding this comment.
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.
|
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. |
|
oh.. I've forgotten this. |
ff41bbf to
2e05009
Compare
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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. |
|
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. |
In this PR, I have implemented support for IP-based routing.
We can configure it from the
cidrRoutingConfigas shown in the following example:Question
As mentioned above, the existing
cidrCollectioncan be passed ascollectionargument.During this process, a new
Locationis added to the existingLocationsin thecidrCollection.This code works, but I am not confident about the implementation approach. I would greatly appreciate any candid feedback.
Closes #28801.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license