fix(route53): determineFullyQualifiedDomainName output ends with two periods when hostedzone.zonename is token by tejasmr · Pull Request #20435 · aws/aws-cdk

tejasmr · 2022-05-20T08:28:59Z

Added forceRemovePeriodProp prop which can forcefully remove the period if desired by the user fixes #20389

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

…olved token

peterwoodworth

Thanks for the PR @tezz-io! There are a few things we need to address 🙂

peterwoodworth · 2022-05-23T21:07:25Z

packages/@aws-cdk/aws-route53/lib/util.ts

 *      </ul>
 */
-export function determineFullyQualifiedDomainName(providedName: string, hostedZone: IHostedZone): string {
+export function determineFullyQualifiedDomainName(providedName: string, hostedZone: IHostedZone, forceRemovePeriodProp?: boolean): string {


I'm not a fan of the name forceRemovePeriodProp - I used that as an example. forceRemovePeriod is fine, but honestly I'm not great at coming up with names. If you have any other suggestions, feel free. Something descriptive like removeDomainNamePeriod is much better in my opinion.

peterwoodworth · 2022-05-23T21:11:58Z

packages/@aws-cdk/aws-route53/test/util.test.ts

+      value: 'domain.com.',
+      exportName: 'zoneNameExample',
+    });
+


Remove this empty line here, we don't want any empty lines unless they're between the given / when / then clauses

peterwoodworth · 2022-05-23T21:14:27Z

packages/@aws-cdk/aws-route53/test/util.test.ts

+
+    // WHEN
+    const providedName = 'test';
+    new cdk.CfnOutput(stack, 'zoneNameExample', {


You shouldn't actually need to create a CfnOutput for the importValue to reference for this test to succeed

peterwoodworth · 2022-05-23T21:21:26Z

packages/@aws-cdk/aws-route53/lib/util.ts

 *      </ul>
 */
-export function determineFullyQualifiedDomainName(providedName: string, hostedZone: IHostedZone): string {
+export function determineFullyQualifiedDomainName(providedName: string, hostedZone: IHostedZone, forceRemovePeriodProp?: boolean): string {


This function looks great, but this functionality isn't exposed to the user, nor is it being used anywhere where we call this function in the code. So right now, this still isn't doing anything.

This function is called here

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

Line 226 in 692a0d0

name: determineFullyQualifiedDomainName(props.recordName || props.zone.zoneName, props.zone),

It will get used whenever a RecordSet or any of the constructs that extend it are created. So, this option needs to be present for all of those constructs.
We can easily accomplish this by adding the prop to RecordSetOptions

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

Line 135 in 692a0d0

export interface RecordSetOptions {

…eFQDN call

gitpod-io · 2022-05-24T05:58:32Z

Pull request has been modified.

tejasmr · 2022-05-24T08:27:32Z

Hi @peterwoodworth, I fixed all of the issues, but removeDomainNamePeriod was renamed to removeDomainNamePeriodEnd because removeDomainNamePeriod ended with the word period, awslint thought it was of type Duration instead of boolean. I tried with @type annotation but it didn't help. So I renamed the prop to removeDomainNamePeriodEnd. If there is a better name, I will be happy to rename to that. Another alternative was removeDomainNameDot but that didn't seem good.

pahud · 2022-05-24T13:54:45Z

packages/@aws-cdk/aws-route53/lib/util.ts

 *      </ul>
 */
-export function determineFullyQualifiedDomainName(providedName: string, hostedZone: IHostedZone): string {
+export function determineFullyQualifiedDomainName(providedName: string, hostedZone: IHostedZone, removeDomainNamePeriodEnd?: boolean): string {


This function is interesting.

Is there any scenario the removeDomainNamePeriodEnd should be false with the imported HostedZone?

If we know the imported hostedZone.zoneName is not a FQDN, e.g., "github.com". If it is ensured that it will be FQDN then with just !Token.isUnresolved(hostedZoneName) condition is enough.

pahud · 2022-05-24T13:56:07Z

Hi @neilkuan @robertd @misterjoshua

Any comments on this PR? Would love to hear more feedbacks.

TheRealAmazonKendra

Looks like our branch change over to v2 caused some branch conflicts. Could you please resolve these?

…-io/route53

Pull request has been modified.

aws-cdk-automation · 2022-07-14T07:44:12Z

AWS CodeBuild CI Report

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

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

peterwoodworth · 2022-07-20T18:29:24Z

@TheRealAmazonKendra the issues have been resolved, this is ready for full review. What do you think about this solution?

TheRealAmazonKendra · 2022-07-21T01:02:20Z

@comcalvi and I have been discussing this one and I don't think this is the right solution. We also noted that this is an issue that we have in various contexts so we're looking into a generic larger scale fix.

Because `route53.determineFullyQualifiedDomainName()` doesn't correctly handles tokens we get the zone name twice here. Another way to solve this would be to fix `determineFullyQualifiedDomainName()`. There is already a discussion there aws#20435 for the hosted zone name part, not the record name. Closes aws#21306

jogold · 2022-07-25T15:32:31Z

There's also an issue when the providedName is a token (see #21306 and #21318) because the implementation of determineFullyQualifiedDomainName() checks if it .endsWith('.') which doesn't make sense for a token.

Would be great if this could also be fixed in this PR => fix(route53): incorrect handling of tokens in determineFullyQualifiedDomainName()

TheRealAmazonKendra · 2022-07-27T21:18:52Z

Given that this is not the direction we want to take with this, I'm going to go ahead and close this PR. We can continue the conversation of alternative fixes in the issue. This may require us to make a breaking change and put this under a feature flag but it seems that the impacts of this bug are pretty wide reaching so doing so would likely be worth it to do so.

Because `route53.determineFullyQualifiedDomainName()` doesn't correctly handles tokens we get the zone name twice here. Another way to solve this would be to fix `determineFullyQualifiedDomainName()`. There is already a discussion there #20435 for the hosted zone name part, not the record name. Closes #21306 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

tejasmr added 2 commits May 20, 2022 08:20

Added prop which can forcefully remove the period if desired by the user

1b55666

Added unit test for determineFQDN for hostedZone.zoneName is an unres…

128821a

…olved token

aws-cdk-automation requested a review from a team May 20, 2022 08:29

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels May 20, 2022

peterwoodworth previously requested changes May 23, 2022

View reviewed changes

tejasmr added 3 commits May 24, 2022 05:50

Changed forceRemovePeriodProp to removeDomainNamePeriod

b21ec65

Removed unnecessary newline

e52a1ac

Added removeDomainNamePeriod to RecordSetOptions and updated determin…

73542e4

…eFQDN call

tejasmr added 2 commits May 24, 2022 06:33

Fixed formatting and changed prop name to removeDomainNamePeriodEnd

24c1bad

Removed CfnOutput zoneNameExample

eecf77b

Removed type annotation for removeDomainNamePeriodEnd prop

71e604b

tejasmr requested a review from peterwoodworth May 24, 2022 10:58

pahud reviewed May 24, 2022

View reviewed changes

TheRealAmazonKendra changed the base branch from v1-main to main June 2, 2022 09:15

TheRealAmazonKendra previously requested changes Jul 14, 2022

View reviewed changes

Merge branch 'master' of https://github.com/tezz-io/aws-cdk into tezz…

e1fc71f

…-io/route53

Removed trailing spaces

ff7d2a9

tejasmr requested a review from TheRealAmazonKendra July 14, 2022 07:44

jogold mentioned this pull request Jul 25, 2022

fix(ses): incorrect DKIM records for EmailIdentity #21318

Merged

4 tasks

TheRealAmazonKendra closed this Jul 27, 2022

Conversation

tejasmr commented May 20, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

peterwoodworth left a comment

Choose a reason for hiding this comment

Uh oh!

peterwoodworth May 23, 2022

Choose a reason for hiding this comment

Uh oh!

peterwoodworth May 23, 2022

Choose a reason for hiding this comment

Uh oh!

peterwoodworth May 23, 2022

Choose a reason for hiding this comment

Uh oh!

peterwoodworth May 23, 2022

Choose a reason for hiding this comment

Uh oh!

gitpod-io bot commented May 24, 2022

Uh oh!

tejasmr commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pahud May 24, 2022

Choose a reason for hiding this comment

Uh oh!

tejasmr May 24, 2022 • 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!

pahud commented May 24, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Jul 14, 2022

AWS CodeBuild CI Report

Uh oh!

peterwoodworth commented Jul 20, 2022

Uh oh!

TheRealAmazonKendra commented Jul 21, 2022

Uh oh!

jogold commented Jul 25, 2022

Uh oh!

TheRealAmazonKendra commented Jul 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tejasmr commented May 24, 2022 •

edited

Loading

tejasmr May 24, 2022 •

edited

Loading