fix(RecordSet): recordName with a token do not suffix with zone name

[matusfaro]

Closes #26572.

Context

RecordSet attempts to be smart about the input value of recordName. Typically, for a zone example.com, a FQDN is required for the recordName such as test.example.com.. However, RecordSet's determineFullyQualifiedDomainName method attempts to fix user input in case they forgot to add the zone name turning test into test.example.com. This works great in the usual cases, except for parameterized input such as CfnCondition or imported value from another stack. The determineFullyQualifiedDomainName thinks the input does not end in the zone name and tacks it on. When test.example.com. is passed in via CfnParameter, the resulting record becomes test.example.com.example.com.

Solution

If the recordName (a.k.a. providedName inside determineFullyQualifiedDomainName) is found to contain a token, leave the input as is and do not suffix it with zone name. This is because the input should have been a FQDN in the first place. So we assume the token includes the correct FQDN as required since we are not able to confirm ourselves.

---

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.

[rix0rrr]

What will happen to a customer currently successfully using a tokenized value as input to this construct (because they know the behavior and they're only passing the subdomain name) ?

[matusfaro]

@rix0rrr: What will happen to a customer currently successfully using a tokenized value as input

It's a fair point but hear me out:

• If we were to put this change behind a parameter flag, it will still not allow me to use constructs that use RecordSets internally but will not expose the new flag
• The official docs say recordName should be The domain name for this record.; Accepting subdomains (and via tokens) is a nice to have, but in my opinion should not trump using the construct in the intended way: using a domain via token.
• It is not possible to use aws-cdk-lib's ownaws-route53-patterns.HttpsRedirect construct with a token right now and I suspect there are more libraries that do this. This is because a single argument is used for both RecordSet as well as ViewerCertificate.fromAcmCertificate which cannot have a trailing dot and thus subverting the trailing dot workaround.
• I suspect the affected customers are close to zero and will require them to add the suffix themselves. Customers using CDK are typically not using stack params anyway as they can create conditions within CDK, so this is for a smaller subset of customers

The options I see are:

1. Stop suffixing for input that contains a token (This PR currently)
   -- Breaks functionality for existing customers, requires them to add the suffix themselves
2. Stop suffixing for input that contains a token, but adding a parameter suffixZoneName to force suffix to be appended regardless of input
   -- Breaks functionality for existing customers, requires them to only set a new flag
3. Adding a new parameter suppressSuffixZoneName to suppress the behavior of determining whether to conditionally suffix zone name
   -- Doesn't break any existing customers; still doesn't fix cases where RecordSet is within a construct

[matusfaro]

@rix0rrr Friendly ping. Which option do you prefer?

In summary, I would propose the PR as is; yes it will break existing customers, but:

• I assume minimal customers impacted; easy fix for them.
• The ones that are relying on it are already breaking the stated documentation
• AFAIK, there is no other way to fix this without breaking existing customers.

[aws-cdk-automation]

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[matusfaro]

Closing due to inactivity