Skip to content
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

aws-cdk/core: Tokenization.reverseCompleteString() Error updates / erroneous behavior in Stack.exportValue() #31035

Closed
IkeNefcy opened this issue Aug 6, 2024 · 3 comments
Assignees
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. p3 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@IkeNefcy
Copy link

IkeNefcy commented Aug 6, 2024

Describe the bug

Hey team, was deep diving the exportValue() function because of an error that it was throwing when trying to export route 53 properties / IDs.

The error throwing from Tokenization.reverseCompleteString() is the concatenation error. For clarity I think this error needs to be changed to include the possibility of this getting called from Stack.exportValue().

Expected Behavior

The error checking should be aware that exportValue could call for a token from a source that returns a token list, like route53, potentially being able to identify when a fragment list is due to a multi return from reverseString(), or an actual concatenation.
Basically the error currently, doesn't fit this particular situation, where the user is not in fact concatenating anything.

Current Behavior

Here is what I'm thinking; it seems like the purpose of the concatenation error is in case someone is calling one of these tokenization methods directly, and when parsing the token fragments from reverseString() it's assuming if you ended up with a list of fragments then you must of somehow gotten more than one value back from the splitstring, therefore some kind of concat happened. I think this is a side effect in this case of route53 using intrinsic sting lists. This summarizes pretty well https://docs.aws.amazon.com/cdk/v2/guide/tokens.html#tokens_list

So when calling reverseCompleteString(), route53 is simply returning a list of fragments due to it's natural behavior, but this isn't a user error. The error that you should not pass in a concatenation in this case makes no sense to the user since that's not what they did (see below code block), rather the return behavior for this particular cfn construct is causing the issue on it's own.

Reproduction Steps

create a hosted zone using CDK, then the following:

const test = myHostedZone.hostedZoneId; // or hostedZoneArn
this.exportValue(test);

Possible Solution

You can solve this simply by adding a "name" to the exportValue() function when called. Which brings up an interesting point. What is the purpose of using exportValue() without a name? it seems like a risk to have the build parse the CFN data just to add a name to the exported value. Potentially a simple fix would be to just remove this side of exportValue() all together and force the user to include a name when using this function.

Additionally it seems like in Tokenization.reverse() the option options.failConcat could maybe help with this situation, but Stack.exportValue() doesn't include this as part of the inputs, so there wouldn't be a way for a user to override in this fashion either.

Additional Information/Context

Overall seems like these functions just aren't fully meshing, it's hard to say what the best fix / root cause is, since it could very well be route53 in the end too.

CDK CLI Version

2.149.0

Framework Version

No response

Node.js Version

v18.20.2

OS

Mac Sonoma 14.5

Language

TypeScript

Language Version

^5.0.4

Other information

No response

@IkeNefcy IkeNefcy added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 6, 2024
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Aug 6, 2024
@ashishdhingra ashishdhingra self-assigned this Aug 6, 2024
@ashishdhingra ashishdhingra added needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Aug 6, 2024
@ashishdhingra
Copy link
Contributor

@IkeNefcy Good afternoon. Could you please share the exact CDK command and error that you are getting? Somehow, using cdk deploy on the below stack code works fine using CDK version 2.151.0:

const myHostedZone = new route53.HostedZone(this, 'myHostedZone', {
  zoneName: 'mynamespace.com'
});
this.exportValue(myHostedZone.hostedZoneId);

Thanks,
Ashish

@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p3 and removed needs-reproduction This issue needs reproduction. labels Aug 6, 2024
@IkeNefcy
Copy link
Author

IkeNefcy commented Aug 7, 2024

I just updated to the latest commit so I could test again. I'm now getting the error that matches instead of the concat error
Error: exportValue: either supply 'name' or make sure to export a resource attribute (like 'bucket.bucketName')

I just did this;

    const MyHostedZone = PublicHostedZone.fromPublicHostedZoneAttributes(this, 'MyHostedZone', {
      zoneName: props.MyZone,
      hostedZoneId: props.zoneID,
    });
    const test = MyHostedZone.hostedZoneId;
    this.exportValue(test);

I think this issue might have already been solved looks like, I just needed to update my version.
It looks like exportValue() isn't calling reverseCompleteString() anymore, instead there is a new function resolveExportedValue() with a separate check that is being called. Sorry for the churn will close this

@IkeNefcy IkeNefcy closed this as completed Aug 7, 2024
Copy link

github-actions bot commented Aug 7, 2024

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. p3 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

2 participants