Skip to content

Commit 7f26fad

Browse files
authored
fix(core): Fn.select incorrectly short-circuits complex expressions (#19680)
In CloudFormation, it is possible to do the following: ``` 'Fn::Select': - 0 - - { 'Fn::If': ['Cond1', 'Value1', { Ref: 'AWS::NoValue' } } - { 'Fn::If': ['Cond2', 'Value2', { Ref: 'AWS::NoValue' } } - { 'Fn::If': ['Cond3', 'Value3', { Ref: 'AWS::NoValue' } } ``` Because the `AWS::NoValue`s will disappear from the array, this will evaluate to the first condition that is true. CDK is unlikely to generate expressions like this, but people may have written this in CloudFormation templates. The eager short-circuiting behavior of `Fn.select` was breaking the roundtrippability of this template's condition cascade through `cloudformation-include`, by unconditionally picking out the first element from the array. We can't get rid of the short-circuiting completely (as bunch of templates and tests may already depend on it), but we can catch this happening and guard against it, by not short-circuiting if we can't look into all values. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 097fe19 commit 7f26fad

File tree

4 files changed

+49
-2
lines changed

4 files changed

+49
-2
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
{
2+
"Parameters": {
3+
"DoIt": {
4+
"Type": "String"
5+
}
6+
},
7+
"Conditions": {
8+
"MyCondition": {
9+
"Fn::Equals": [{ "Ref": "DoIt" }, "Yes"]
10+
}
11+
},
12+
"Resources": {
13+
"Bucket": {
14+
"Type": "AWS::S3::Bucket",
15+
"Properties": {
16+
"BucketName": { "Fn::Select": [0, [
17+
{ "Fn::If": ["MyCondition", "doing-it", { "Ref": "AWS::NoValue" }] },
18+
"not-doingit"
19+
]]}
20+
}
21+
}
22+
}
23+
}

packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts

+8
Original file line numberDiff line numberDiff line change
@@ -1081,6 +1081,14 @@ describe('CDK Include', () => {
10811081
loadTestFileToJsObject('properties-not-in-cfn-spec.json'),
10821082
);
10831083
});
1084+
1085+
test('roundtrip a fn-select with a fn-if/ref-novalue in it', () => {
1086+
includeTestTemplate(stack, 'fn-select-with-novalue.json');
1087+
1088+
Template.fromStack(stack).templateMatches(
1089+
loadTestFileToJsObject('fn-select-with-novalue.json'),
1090+
);
1091+
});
10841092
});
10851093

10861094
interface IncludeTestTemplateProps {

packages/@aws-cdk/core/lib/cfn-fn.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ export class Fn {
127127
* @returns a token represented as a string
128128
*/
129129
public static select(index: number, array: string[]): string {
130-
if (!Token.isUnresolved(array)) {
130+
if (!Token.isUnresolved(index) && !Token.isUnresolved(array) && !array.some(Token.isUnresolved)) {
131131
return array[index];
132132
}
133133

packages/@aws-cdk/core/test/fn.test.ts

+17-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import * as fc from 'fast-check';
22
import * as _ from 'lodash';
3-
import { App, CfnOutput, Fn, Stack, Token } from '../lib';
3+
import { App, Aws, CfnOutput, Fn, Stack, Token } from '../lib';
44
import { Intrinsic } from '../lib/private/intrinsic';
55

66
function asyncTest(cb: () => Promise<void>): () => void {
@@ -27,8 +27,24 @@ describe('fn', () => {
2727
describe('eager resolution for non-tokens', () => {
2828
test('Fn.select', () => {
2929
expect(Fn.select(2, ['hello', 'you', 'dude'])).toEqual('dude');
30+
});
31+
32+
test('Fn.select does not short-circuit if there are tokens in the array', () => {
33+
const stack = new Stack();
3034

35+
expect(stack.resolve(Fn.select(2, [
36+
Fn.conditionIf('xyz', 'yep', Aws.NO_VALUE).toString(),
37+
'you',
38+
'dude',
39+
]))).toEqual({
40+
'Fn::Select': [2, [
41+
{ 'Fn::If': ['xyz', 'yep', { Ref: 'AWS::NoValue' }] },
42+
'you',
43+
'dude',
44+
]],
45+
});
3146
});
47+
3248
test('Fn.split', () => {
3349
expect(Fn.split(':', 'hello:world:yeah')).toEqual(['hello', 'world', 'yeah']);
3450

0 commit comments

Comments
 (0)