From 7d9ab2a783d1d3ae4508760149dee7ac263fdd44 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 1 Apr 2022 14:05:12 +0200 Subject: [PATCH] fix(core): detect and resolve stringified number tokens (#19578) Number tokens are encoded as a range of very large negative numbers (for example: -1.888154589709072e+289). When these are naively stringified, the `resolve()` method doesn't recognize and translate them anymore, and these numbers end up in the target template in a confusing way. However, recognizing them is actually not that hard and can be done using a regex. We can then do the token resolution appropriately, making it so that construct authors do not have to call `Tokenization.stringifyNumber()` anymore in order to support stringification of number values. Fixes #19546, closes #19550. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-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* --- .../test/batch/submit-job.test.ts | 27 ++++++++++ .../core/lib/private/cloudformation-lang.ts | 17 ++++--- .../@aws-cdk/core/lib/private/encoding.ts | 16 +++++- .../@aws-cdk/core/lib/private/token-map.ts | 6 ++- .../@aws-cdk/core/lib/string-fragments.ts | 6 ++- packages/@aws-cdk/core/test/tokens.test.ts | 50 ++++++++++++++----- 6 files changed, 96 insertions(+), 26 deletions(-) diff --git a/packages/@aws-cdk/aws-stepfunctions-tasks/test/batch/submit-job.test.ts b/packages/@aws-cdk/aws-stepfunctions-tasks/test/batch/submit-job.test.ts index 0885dcfb55930..aa9ccbdd8a0d8 100644 --- a/packages/@aws-cdk/aws-stepfunctions-tasks/test/batch/submit-job.test.ts +++ b/packages/@aws-cdk/aws-stepfunctions-tasks/test/batch/submit-job.test.ts @@ -169,6 +169,33 @@ test('supports tokens', () => { }); }); +test('container overrides are tokens', () => { + // WHEN + const task = new BatchSubmitJob(stack, 'Task', { + jobDefinitionArn: batchJobDefinition.jobDefinitionArn, + jobName: 'JobName', + jobQueueArn: batchJobQueue.jobQueueArn, + containerOverrides: { + memory: cdk.Size.mebibytes(sfn.JsonPath.numberAt('$.asdf')), + }, + }); + + // THEN + expect(stack.resolve(task.toStateJson())).toEqual({ + Type: 'Task', + Resource: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':states:::batch:submitJob.sync']] }, + End: true, + Parameters: { + JobDefinition: { Ref: 'JobDefinition24FFE3ED' }, + JobName: 'JobName', + JobQueue: { Ref: 'JobQueueEE3AD499' }, + ContainerOverrides: { + ResourceRequirements: [{ 'Type': 'MEMORY', 'Value.$': '$.asdf' }], + }, + }, + }); +}); + test('supports passing task input into payload', () => { // WHEN const task = new BatchSubmitJob(stack, 'Task', { diff --git a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts index 049ceb207f92f..82d2809255806 100644 --- a/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts +++ b/packages/@aws-cdk/core/lib/private/cloudformation-lang.ts @@ -43,8 +43,8 @@ export class CloudFormationLang { // Some case analysis to produce minimal expressions if (parts.length === 1) { return parts[0]; } - if (parts.length === 2 && typeof parts[0] === 'string' && typeof parts[1] === 'string') { - return parts[0] + parts[1]; + if (parts.length === 2 && isConcatable(parts[0]) && isConcatable(parts[1])) { + return `${parts[0]}${parts[1]}`; } // Otherwise return a Join intrinsic (already in the target document language to avoid taking @@ -323,8 +323,8 @@ export function minimalCloudFormationJoin(delimiter: string, values: any[]): any const el = values[i]; if (isSplicableFnJoinIntrinsic(el)) { values.splice(i, 1, ...el['Fn::Join'][1]); - } else if (i > 0 && isPlainString(values[i - 1]) && isPlainString(values[i])) { - values[i - 1] += delimiter + values[i]; + } else if (i > 0 && isConcatable(values[i - 1]) && isConcatable(values[i])) { + values[i - 1] = `${values[i-1]}${delimiter}${values[i]}`; values.splice(i, 1); } else { i += 1; @@ -333,10 +333,6 @@ export function minimalCloudFormationJoin(delimiter: string, values: any[]): any return values; - function isPlainString(obj: any): boolean { - return typeof obj === 'string' && !Token.isUnresolved(obj); - } - function isSplicableFnJoinIntrinsic(obj: any): boolean { if (!isIntrinsic(obj)) { return false; } if (Object.keys(obj)[0] !== 'Fn::Join') { return false; } @@ -351,6 +347,11 @@ export function minimalCloudFormationJoin(delimiter: string, values: any[]): any } } +function isConcatable(obj: any): boolean { + return ['string', 'number'].includes(typeof obj) && !Token.isUnresolved(obj); +} + + /** * Return whether the given value represents a CloudFormation intrinsic */ diff --git a/packages/@aws-cdk/core/lib/private/encoding.ts b/packages/@aws-cdk/core/lib/private/encoding.ts index fae5b1a41ec63..15be87cf42ee0 100644 --- a/packages/@aws-cdk/core/lib/private/encoding.ts +++ b/packages/@aws-cdk/core/lib/private/encoding.ts @@ -14,7 +14,10 @@ const QUOTED_BEGIN_STRING_TOKEN_MARKER = regexQuote(BEGIN_STRING_TOKEN_MARKER); const QUOTED_BEGIN_LIST_TOKEN_MARKER = regexQuote(BEGIN_LIST_TOKEN_MARKER); const QUOTED_END_TOKEN_MARKER = regexQuote(END_TOKEN_MARKER); -const STRING_TOKEN_REGEX = new RegExp(`${QUOTED_BEGIN_STRING_TOKEN_MARKER}([${VALID_KEY_CHARS}]+)${QUOTED_END_TOKEN_MARKER}`, 'g'); +// Sometimes the number of digits is different +export const STRINGIFIED_NUMBER_PATTERN = '-1\\.\\d{10,16}e\\+289'; + +const STRING_TOKEN_REGEX = new RegExp(`${QUOTED_BEGIN_STRING_TOKEN_MARKER}([${VALID_KEY_CHARS}]+)${QUOTED_END_TOKEN_MARKER}|(${STRINGIFIED_NUMBER_PATTERN})`, 'g'); const LIST_TOKEN_REGEX = new RegExp(`${QUOTED_BEGIN_LIST_TOKEN_MARKER}([${VALID_KEY_CHARS}]+)${QUOTED_END_TOKEN_MARKER}`, 'g'); /** @@ -52,7 +55,7 @@ export class TokenString { ret.addLiteral(this.str.substring(rest, m.index)); } - ret.addToken(lookup(m[1])); + ret.addToken(lookup(m[1] ?? m[2])); rest = this.re.lastIndex; m = this.re.exec(this.str); @@ -218,3 +221,12 @@ export function extractTokenDouble(encoded: number): number | undefined { return ints[0] + shl32(ints[1] & 0xFFFF); /* eslint-enable no-bitwise */ } + +const STRINGIFIED_NUMBER_REGEX = new RegExp(STRINGIFIED_NUMBER_PATTERN); + +/** + * Return whether the given string contains accidentally stringified number tokens + */ +export function stringContainsNumberTokens(x: string) { + return !!x.match(STRINGIFIED_NUMBER_REGEX); +} diff --git a/packages/@aws-cdk/core/lib/private/token-map.ts b/packages/@aws-cdk/core/lib/private/token-map.ts index 1a5b0e1f29547..ed2b6a59d0a4f 100644 --- a/packages/@aws-cdk/core/lib/private/token-map.ts +++ b/packages/@aws-cdk/core/lib/private/token-map.ts @@ -177,8 +177,12 @@ export class TokenMap { private registerNumberKey(token: IResolvable): number { const counter = this.tokenCounter++; + const dbl = createTokenDouble(counter); + // Register in the number map, as well as a string representation of that token + // in the string map. this.numberTokenMap.set(counter, token); - return createTokenDouble(counter); + this.stringTokenMap.set(`${dbl}`, token); + return dbl; } } diff --git a/packages/@aws-cdk/core/lib/string-fragments.ts b/packages/@aws-cdk/core/lib/string-fragments.ts index b92fd3628a28d..4fea67f333a2a 100644 --- a/packages/@aws-cdk/core/lib/string-fragments.ts +++ b/packages/@aws-cdk/core/lib/string-fragments.ts @@ -1,5 +1,5 @@ import { IFragmentConcatenator, IResolvable } from './resolvable'; -import { isResolvableObject } from './token'; +import { isResolvableObject, Token } from './token'; /** * Result of the split of a string with Tokens @@ -71,8 +71,10 @@ export class TokenizedStringFragments { const mapped = mapper.mapToken(f.token); if (isResolvableObject(mapped)) { ret.addToken(mapped); - } else { + } else if (Token.isUnresolved(mapped)) { ret.addIntrinsic(mapped); + } else { + ret.addLiteral(mapped); } break; case 'intrinsic': diff --git a/packages/@aws-cdk/core/test/tokens.test.ts b/packages/@aws-cdk/core/test/tokens.test.ts index 48e07c1fc720f..62cd683da6ac2 100644 --- a/packages/@aws-cdk/core/test/tokens.test.ts +++ b/packages/@aws-cdk/core/test/tokens.test.ts @@ -1,5 +1,5 @@ -import { Fn, isResolvableObject, Lazy, Stack, Token, Tokenization } from '../lib'; -import { createTokenDouble, extractTokenDouble } from '../lib/private/encoding'; +import { CfnResource, Fn, isResolvableObject, Lazy, Stack, Token, Tokenization } from '../lib'; +import { createTokenDouble, extractTokenDouble, stringContainsNumberTokens, STRINGIFIED_NUMBER_PATTERN } from '../lib/private/encoding'; import { Intrinsic } from '../lib/private/intrinsic'; import { findTokens } from '../lib/private/resolve'; import { IResolvable } from '../lib/resolvable'; @@ -482,15 +482,12 @@ describe('tokens', () => { expect(() => { resolve({ value: encoded[0] }); }).toThrow(/Found an encoded list/); - - }); }); describe('number encoding', () => { test('basic integer encoding works', () => { expect(16).toEqual(extractTokenDouble(createTokenDouble(16))); - }); test('arbitrary integers can be encoded, stringified, and recovered', () => { @@ -504,16 +501,12 @@ describe('tokens', () => { const decoded = extractTokenDouble(roundtripped); expect(decoded).toEqual(x); } - - }); test('arbitrary numbers are correctly detected as not being tokens', () => { expect(undefined).toEqual(extractTokenDouble(0)); expect(undefined).toEqual(extractTokenDouble(1243)); expect(undefined).toEqual(extractTokenDouble(4835e+532)); - - }); test('can number-encode and resolve Token objects', () => { @@ -528,8 +521,42 @@ describe('tokens', () => { // THEN const resolved = resolve({ value: encoded }); expect(resolved).toEqual({ value: 123 }); + }); + test('regex detects all stringifications of encoded tokens', () => { + expect(stringContainsNumberTokens(`${createTokenDouble(0)}`)).toBeTruthy(); + expect(stringContainsNumberTokens(`${createTokenDouble(Math.pow(2, 48) - 1)}`)).toBeTruthy(); // MAX_ENCODABLE_INTEGER + expect(stringContainsNumberTokens('1234')).toBeFalsy(); + }); + test('check that the first N encoded numbers can be detected', () => { + const re = new RegExp(STRINGIFIED_NUMBER_PATTERN); + // Ran this up to 1 million offline + for (let i = 0; i < 1000; i++) { + expect(`${createTokenDouble(i)}`).toMatch(re); + } + }); + + test('handle stringified number token', () => { + // GIVEN + const tok = `the answer is: ${Lazy.number({ produce: () => 86 })}`; + + // THEN + expect(resolve({ value: `${tok}` })).toEqual({ + value: 'the answer is: 86', + }); + }); + + test('handle stringified number reference', () => { + const stack = new Stack(); + const res = new CfnResource(stack, 'Resource', { type: 'My::Resource' }); + // GIVEN + const tok = `the answer is: ${Token.asNumber(res.ref)}`; + + // THEN + expect(resolve({ value: `${tok}` })).toEqual({ + value: { 'Fn::Join': ['', ['the answer is: ', { Ref: 'Resource' }]] }, + }); }); }); @@ -694,25 +721,21 @@ describe('tokens', () => { describe('stringifyNumber', () => { test('converts number to string', () => { expect(Tokenization.stringifyNumber(100)).toEqual('100'); - }); test('converts tokenized number to string', () => { expect(resolve(Tokenization.stringifyNumber({ resolve: () => 100, } as any))).toEqual('100'); - }); test('string remains the same', () => { expect(Tokenization.stringifyNumber('123' as any)).toEqual('123'); - }); test('Ref remains the same', () => { const val = { Ref: 'SomeLogicalId' }; expect(Tokenization.stringifyNumber(val as any)).toEqual(val); - }); test('lazy Ref remains the same', () => { @@ -791,3 +814,4 @@ function tokensThatResolveTo(value: any): Token[] { function resolve(x: any) { return new Stack().resolve(x); } +