Skip to content

(acm): DomainName length check fails with too long prop due to string encoded tokens. #23565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rv2673 opened this issue Jan 4, 2023 · 2 comments · Fixed by #23567
Closed
Assignees
Labels
@aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p2

Comments

@rv2673
Copy link
Contributor

rv2673 commented Jan 4, 2023

Describe the bug

When creating Certificate where the domainName property contains domain with tokens, the length check fails because the string encoded token causes the length to exceed the maximum even if token resolved to something shorter.

Expected Behavior

Check is only done when string contains no unresolved tokens.

Offending line:

if (props.domainName.length > 64) {

Current Behavior

Synthesis failed because length is exceeded due to string encoded token.

Reproduction Steps

Create cdk stack
Add Certificate where the base domain is 54 characters.
Add domainName string where subdomain comes from Token (For example CfnParameter)
Try to synthesize stack.

Possible Solution

Perform check only when tokens are unresolved. Like cdk documentation suggests.

if (props.domainName.length > 64) {

Example from docs:

if (!Token.isUnresolved(name) && name.length > 10) {
  throw new Error(`Maximum length for name is 10 characters`);
}

In this context:

if (!Token.isUnresolved(props.domainName) && props.domainName.length > 64) {
  throw new Error('Domain name must be 64 characters or less');
}

Additional Information/Context

No response

CDK CLI Version

2.58.1

Framework Version

No response

Node.js Version

16.19.0

OS

Ubuntu

Language

Typescript

Language Version

No response

Other information

No response

@rv2673 rv2673 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 4, 2023
@github-actions github-actions bot added the @aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager label Jan 4, 2023
@rv2673 rv2673 changed the title (acm): DomainName length check fails with to long string due to string encoded tokens. (acm): DomainName length check fails with too long prop due to string encoded tokens. Jan 4, 2023
@peterwoodworth peterwoodworth added p2 effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. and removed needs-triage This issue or PR still needs to be triaged. labels Jan 4, 2023
@peterwoodworth
Copy link
Contributor

Thanks for the quick PR! much appreciated 🙂

mergify bot added a commit to rv2673/aws-cdk that referenced this issue Jan 4, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…traint
@mergify mergify bot closed this as completed in #23567 Jan 4, 2023
mergify bot pushed a commit that referenced this issue Jan 4, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
When creating Certificate where the domainName property contains tokens, the length check can fail. This is because the string encoded token can cause the length to exceed the maximum even if token will resolve to something shorter.

To solve this the property is checked for token as provided in the design guidelines:
https://github.com/aws/aws-cdk/blob/b14a3d1dc4c4508ad354d954b9480e9c2c64401c/docs/DESIGN_GUIDELINES.md#throwing-exceptions

fixes #23565

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? // No fix involves only synth time code.
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? // No changes to integration tests where made.

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants