Skip to content

Commit

Permalink
fix(core): Fn.select incorrectly short-circuits complex expressions (
Browse files Browse the repository at this point in the history
…#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*
  • Loading branch information
rix0rrr authored Apr 1, 2022
1 parent 097fe19 commit 7f26fad
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 2 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"Parameters": {
"DoIt": {
"Type": "String"
}
},
"Conditions": {
"MyCondition": {
"Fn::Equals": [{ "Ref": "DoIt" }, "Yes"]
}
},
"Resources": {
"Bucket": {
"Type": "AWS::S3::Bucket",
"Properties": {
"BucketName": { "Fn::Select": [0, [
{ "Fn::If": ["MyCondition", "doing-it", { "Ref": "AWS::NoValue" }] },
"not-doingit"
]]}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1081,6 +1081,14 @@ describe('CDK Include', () => {
loadTestFileToJsObject('properties-not-in-cfn-spec.json'),
);
});

test('roundtrip a fn-select with a fn-if/ref-novalue in it', () => {
includeTestTemplate(stack, 'fn-select-with-novalue.json');

Template.fromStack(stack).templateMatches(
loadTestFileToJsObject('fn-select-with-novalue.json'),
);
});
});

interface IncludeTestTemplateProps {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/core/lib/cfn-fn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export class Fn {
* @returns a token represented as a string
*/
public static select(index: number, array: string[]): string {
if (!Token.isUnresolved(array)) {
if (!Token.isUnresolved(index) && !Token.isUnresolved(array) && !array.some(Token.isUnresolved)) {
return array[index];
}

Expand Down
18 changes: 17 additions & 1 deletion packages/@aws-cdk/core/test/fn.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as fc from 'fast-check';
import * as _ from 'lodash';
import { App, CfnOutput, Fn, Stack, Token } from '../lib';
import { App, Aws, CfnOutput, Fn, Stack, Token } from '../lib';
import { Intrinsic } from '../lib/private/intrinsic';

function asyncTest(cb: () => Promise<void>): () => void {
Expand All @@ -27,8 +27,24 @@ describe('fn', () => {
describe('eager resolution for non-tokens', () => {
test('Fn.select', () => {
expect(Fn.select(2, ['hello', 'you', 'dude'])).toEqual('dude');
});

test('Fn.select does not short-circuit if there are tokens in the array', () => {
const stack = new Stack();

expect(stack.resolve(Fn.select(2, [
Fn.conditionIf('xyz', 'yep', Aws.NO_VALUE).toString(),
'you',
'dude',
]))).toEqual({
'Fn::Select': [2, [
{ 'Fn::If': ['xyz', 'yep', { Ref: 'AWS::NoValue' }] },
'you',
'dude',
]],
});
});

test('Fn.split', () => {
expect(Fn.split(':', 'hello:world:yeah')).toEqual(['hello', 'world', 'yeah']);

Expand Down

0 comments on commit 7f26fad

Please sign in to comment.