-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix(s3-deployment): list tokens in Source.jsonData are not resolved. #35169
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
Conversation
dd03826
to
a810de5
Compare
if (Array.isArray(obj)) { | ||
return obj.map(v => Source.escapeTokens(scope, v)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better approach might be to have escapeTokens
accept an array, and perform the operation for each. If there is a single object, then the array would contain a single object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be a misunderstanding, escapeTokens
already accepts an array.
*/ | ||
private static escapeTokens(scope: Construct, obj: any): any { | ||
if (Token.isUnresolved(obj)) { | ||
// Return tokens as numbers. This is a hack to prevent the JSON serializer to wrap this token as a string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better approach which removes the need for the hack entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only other approach I can think of is either:
- Create a custom JSON serializer
- Process the output of the JSON serialization such that the markers don't have string (
"..."
) escape.
Both methods are quite error-prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Process the output of the JSON serialization such that the markers don't have string ("...") escape.
Can you elaborate on why this is error prone? It would help to explain how these are different from the current approach, and why converting to a number helps.
If an approach is the right one but a little more complex, we'd still prefer that to a hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a "right" approach here. However this is by far the simplest. One possible issue with the point you mention is with YAML serialization. In YAML serialization, a string can be represented in either a quoted or unquoted fashion. We'd have to create logic to handle both cases when escaping tokens. It can also also mean that the tokens string itself could be escaped by the serializer, leading to malformed tokens such as $\{TOKEN[...]\}
. We want to avoid this escaping behavior altogether by putting a number instead of a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a custom JSON serializer
Probably much nicer, but what you have should work.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
if (Token.isUnresolved(obj)) { | ||
// Return tokens as numbers. This is a hack to prevent the JSON serializer to wrap this token as a string. | ||
// Stack.toJsonString should take care of escaping the object output for JSON. | ||
return Token.asNumber(Stack.of(scope).toJsonString(obj)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is doing what you think it is. Token.asNumber()
is just casting a value as number. If the value is a a number, it's returned unchanged otherwise it's wrapped as a number token. This is mainly used to convert CFN attributes (which are all strings) to "numbers" so they can be used in CDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional. It's a hack to prevent the JSON serializer (JSON.stringify
) or YAML serializer to wrap the token with quotes ("..."). I want the cast to it being a number even if it does not actually represent a number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. I'm just surprised this works.
if (Token.isUnresolved(obj)) { | ||
// Return tokens as numbers. This is a hack to prevent the JSON serializer to wrap this token as a string. | ||
// Stack.toJsonString should take care of escaping the object output for JSON. | ||
return Token.asNumber(Stack.of(scope).toJsonString(obj)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this and than it's fine.
return Token.asNumber(Stack.of(scope).toJsonString(obj)); | |
return Token.asString(Stack.of(scope).toJsonString(obj)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally want to return a number, not a string in this case. So that the JSON serializer won't wrap the string with quotes. The output of toJsonString will have its own quotes by default, or array if its a list token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance of conflicts with real numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To quote Amazon Q:
How Number Tokens are Generated by Token.asNumber()
When you call Token.asNumber(value)
, here's what happens:
1. Initial Check
If the value is already a number, it's returned as-is:
if (typeof value === 'number') { return value; }
2. Token Registration
For non-number values, the token is registered in the TokenMap
singleton:
return TokenMap.instance().registerNumber(Token.asAny(value));
3. Number Generation Process
The registerNumber
method generates a unique number through these steps:
a) Counter Increment:
- Uses an internal counter (
tokenCounter
) that starts at a random number (0-9) - This randomization prevents accidental dependencies on token values between runs
b) Special Double Encoding:
- Calls
createTokenDouble(counter)
to create a special IEEE 754 double - Uses a specific bit pattern in the top 16 bits to mark it as a token
- The marker pattern is
0xFBFF << 16
(can't be all 1s as that would create NaN)
c) Bit Manipulation:
const buf = new ArrayBuffer(8);
const ints = new Uint32Array(buf);
ints[0] = x & 0x0000FFFFFFFF; // Bottom 32 bits of counter
ints[1] = (shr32(x) & 0xFFFF) | DOUBLE_TOKEN_MARKER_BITS; // Top 16 bits + marker
return (new Float64Array(buf))[0];
4. Key Properties
- Unique: Each call generates a new unique number, even for the same token
- Reversible: The number can be decoded back to extract the original token
- Detectable: Special pattern allows detection of encoded tokens vs regular numbers
- JSON-safe: The encoded numbers survive JSON serialization/deserialization
- Range: Can encode up to 2^48-1 different token indices
5. Example
const myToken = Token.asNumber(someResolvable);
// Returns something like: -1.2345678901234e+289
// This special number encodes the token's unique identifier
The generated numbers look like very large negative scientific notation numbers (e.g., -1.1234567890123e+289
) but are actually specially crafted IEEE 754 doubles that encode the token's unique identifier in their bit representation.
Given this info, I think a conflict is very unlikely.
a810de5
to
7f7eec1
Compare
@Mergifyio update |
✅ Branch has been successfully updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No obvious concerns from core team.
If an approach is the right one but a little more complex, we'd still prefer that to a hack.
I very much agree with this. Given that it's a regression I will leave the risk/benefit analysis to y'all.
17288f5
to
2a52cb0
Compare
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue #35145
Closes #35145.
Reason for this change
The recent fix to Source.jsonData did not work for list tokens such as SSM string list parameters. There was a mistake with the fix that assumed
stack.toJsonString
handled those, when in fact the tokens were serialized byJSON.stringify
, breaking the token itself.Token.isUnresolved
does not check all object properties for tokens, so it considered the object resolved and passed it to the regular JSON serializer.Description of changes
I've rewritten the JSON and YAML serializer such that it will use
stack.toJsonString
whenever it encounteres tokens in any of the object's children. One exception this is if the key in an object is a token, the token is assumed to be a string (because it has to in JSON) and is passed directly.Describe any new or updated permissions being added
N/A.
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license