From 7f26fad5241756cdb6b15c9fb20995a96bba71f2 Mon Sep 17 00:00:00 2001
From: Rico Huijbers <rix0rrr@gmail.com>
Date: Fri, 1 Apr 2022 21:58:37 +0200
Subject: [PATCH] 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*
---
 .../fn-select-with-novalue.json               | 23 +++++++++++++++++++
 .../test/valid-templates.test.ts              |  8 +++++++
 packages/@aws-cdk/core/lib/cfn-fn.ts          |  2 +-
 packages/@aws-cdk/core/test/fn.test.ts        | 18 ++++++++++++++-
 4 files changed, 49 insertions(+), 2 deletions(-)
 create mode 100644 packages/@aws-cdk/cloudformation-include/test/test-templates/fn-select-with-novalue.json

diff --git a/packages/@aws-cdk/cloudformation-include/test/test-templates/fn-select-with-novalue.json b/packages/@aws-cdk/cloudformation-include/test/test-templates/fn-select-with-novalue.json
new file mode 100644
index 0000000000000..861387e330ee7
--- /dev/null
+++ b/packages/@aws-cdk/cloudformation-include/test/test-templates/fn-select-with-novalue.json
@@ -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"
+        ]]}
+      }
+    }
+  }
+}
diff --git a/packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts b/packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts
index 65cd7e981cc81..eec714ac5d7d6 100644
--- a/packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts
+++ b/packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts
@@ -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 {
diff --git a/packages/@aws-cdk/core/lib/cfn-fn.ts b/packages/@aws-cdk/core/lib/cfn-fn.ts
index 3ef1c265654bd..673784e0e2a5b 100644
--- a/packages/@aws-cdk/core/lib/cfn-fn.ts
+++ b/packages/@aws-cdk/core/lib/cfn-fn.ts
@@ -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];
     }
 
diff --git a/packages/@aws-cdk/core/test/fn.test.ts b/packages/@aws-cdk/core/test/fn.test.ts
index 221a7b6e811a1..343c3e0ea0422 100644
--- a/packages/@aws-cdk/core/test/fn.test.ts
+++ b/packages/@aws-cdk/core/test/fn.test.ts
@@ -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 {
@@ -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']);