Skip to content

fix(core): Fn.select incorrectly short-circuits complex expressions#19680

Merged
mergify[bot] merged 2 commits intomasterfrom
huijbers/fn-select-too-smart
Apr 1, 2022
Merged

fix(core): Fn.select incorrectly short-circuits complex expressions#19680
mergify[bot] merged 2 commits intomasterfrom
huijbers/fn-select-too-smart

Conversation

@rix0rrr
Copy link
Copy Markdown
Contributor

@rix0rrr rix0rrr commented Apr 1, 2022 •
edited
Loading

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::NoValues 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

@rix0rrr
fix(core): Fn.select incorrectly short-circuits complex expressions
931a49a 
In raw 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.
@rix0rrr rix0rrr requested a review from a team April 1, 2022 10:00
@rix0rrr rix0rrr self-assigned this Apr 1, 2022
@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Apr 1, 2022

@github-actions github-actions bot added the p2 label Apr 1, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 1, 2022
@rix0rrr rix0rrr added p1 and removed p2 labels Apr 1, 2022
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Apr 1, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Apr 1, 2022

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: a69d5ab
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 7f26fad into master Apr 1, 2022
@mergify mergify bot deleted the huijbers/fn-select-too-smart branch April 1, 2022 19:58
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Apr 1, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This was referenced Apr 7, 2022
Merged
Merged
mergify bot added a commit that referenced this pull request Apr 7, 2022
@mergify
chore(release): 1.152.0 (#19781)
9487b39 
See [CHANGELOG](https://github.com/aws/aws-cdk/blob/bump/1.152.0/CHANGELOG.md)

For convenience, extracted the relevant CHANGELOG entry:

## [1.152.0](v1.151.0...v1.152.0) (2022-04-06)


### Features

* **cfnspec:** cloudformation spec v63.0.0 ([#19679](#19679)) ([dba96a9](dba96a9))
* **cfnspec:** cloudformation spec v65.0.0 ([#19745](#19745)) ([796fc64](796fc64))
* **cli:** add --build option ([#19663](#19663)) ([eb9b8e2](eb9b8e2)), closes [#19667](#19667)
* **cli:** preview of `cdk import` ([#17666](#17666)) ([4f12209](4f12209))
* **core:** throw error when stack name exceeds max length ([#19725](#19725)) ([1ffd45e](1ffd45e))
* **eks:** add k8s v1.22 ([#19756](#19756)) ([9a518c5](9a518c5))
* **opensearch:** Add latest Opensearch Version 1.2 ([#19749](#19749)) ([a2ac36e](a2ac36e))
* add new integration test runner ([#19754](#19754)) ([1b4d010](1b4d010))
* **eks:** alb-controller v2.4.1 ([#19653](#19653)) ([1ec08df](1ec08df))
* **lambda:** add support for ephemeral storage ([#19552](#19552)) ([f1d9b6a](f1d9b6a)), closes [#19605](#19605)
* **s3:** EventBridge bucket notifications ([#18614](#18614)) ([d8e602b](d8e602b)), closes [#18076](#18076)
* **synthetics:** new puppeteer 3.5 runtime ([#19673](#19673)) ([ce2b91b](ce2b91b)), closes [#19634](#19634)


### Bug Fixes

* **aws_applicationautoscaling:** Add missing members to PredefinedMetric enum ([#18978](#18978)) ([75a6fa7](75a6fa7)), closes [#18969](#18969)
* **cli:** apps with many resources scroll resource output offscreen ([#19742](#19742)) ([053d22c](053d22c)), closes [#19160](#19160)
* **cli:** support attributes of DynamoDB Tables for hotswapping ([#19620](#19620)) ([2321ece](2321ece)), closes [#19421](#19421)
* **cloudwatch:** automatic metric math label cannot be suppressed ([#17639](#17639)) ([7fa3bf2](7fa3bf2))
* **codedeploy:** add name validation for Application, Deployment Group and Deployment Configuration ([#19473](#19473)) ([9185042](9185042))
* **codedeploy:** the Service Principal is wrong in isolated regions ([#19729](#19729)) ([7e9a43d](7e9a43d)), closes [#19399](#19399)
* **core:** `Fn.select` incorrectly short-circuits complex expressions ([#19680](#19680)) ([7f26fad](7f26fad))
* **core:** detect and resolve stringified number tokens ([#19578](#19578)) ([7d9ab2a](7d9ab2a)), closes [#19546](#19546) [#19550](#19550)
* **core:** reduce CFN template indent size to save bytes ([#19656](#19656)) ([fd63ca3](fd63ca3))
* **ecs:** 'desiredCount' and 'ephemeralStorageGiB' cannot be tokens ([#19453](#19453)) ([c852239](c852239)), closes [#16648](#16648)
* **ecs:** remove unnecessary error when adding volume to external task definition ([#19774](#19774)) ([5446ded](5446ded)), closes [#19259](#19259)
* **iam:** policies aren't minimized as far as possible ([#19764](#19764)) ([876ed8a](876ed8a)), closes [#19751](#19751)
* **logs:** Faulty Resource Policy Generated ([#19640](#19640)) ([1fdf122](1fdf122)), closes [#17544](#17544)
mergify bot added a commit that referenced this pull request Apr 7, 2022
@mergify
chore(release): 2.20.0 (#19801)
738ef49 
See [CHANGELOG](https://github.com/aws/aws-cdk/blob/bump/2.20.0/CHANGELOG.md)

For convenience, extracted the relevant CHANGELOG entry:

## [2.20.0](v2.19.0...v2.20.0) (2022-04-07)


### Features

* **cfnspec:** cloudformation spec v63.0.0 ([#19679](#19679)) ([dba96a9](dba96a9))
* **cfnspec:** cloudformation spec v65.0.0 ([#19745](#19745)) ([796fc64](796fc64))
* **cli:** add --build option ([#19663](#19663)) ([eb9b8e2](eb9b8e2)), closes [#19667](#19667)
* **cli:** preview of `cdk import` ([#17666](#17666)) ([4f12209](4f12209))
* **core:** throw error when stack name exceeds max length ([#19725](#19725)) ([1ffd45e](1ffd45e))
* **eks:** add k8s v1.22 ([#19756](#19756)) ([9a518c5](9a518c5))
* **opensearch:** Add latest Opensearch Version 1.2 ([#19749](#19749)) ([a2ac36e](a2ac36e))
* add new integration test runner ([#19754](#19754)) ([1b4d010](1b4d010))
* **eks:** alb-controller v2.4.1 ([#19653](#19653)) ([1ec08df](1ec08df))
* **lambda:** add support for ephemeral storage ([#19552](#19552)) ([f1d9b6a](f1d9b6a)), closes [#19605](#19605)
* **s3:** EventBridge bucket notifications ([#18614](#18614)) ([d8e602b](d8e602b)), closes [#18076](#18076)


### Bug Fixes

* **aws_applicationautoscaling:** Add missing members to PredefinedMetric enum ([#18978](#18978)) ([75a6fa7](75a6fa7)), closes [#18969](#18969)
* **cli:** apps with many resources scroll resource output offscreen ([#19742](#19742)) ([053d22c](053d22c)), closes [#19160](#19160)
* **cli:** support attributes of DynamoDB Tables for hotswapping ([#19620](#19620)) ([2321ece](2321ece)), closes [#19421](#19421)
* **cloudwatch:** automatic metric math label cannot be suppressed ([#17639](#17639)) ([7fa3bf2](7fa3bf2))
* **codedeploy:** add name validation for Application, Deployment Group and Deployment Configuration ([#19473](#19473)) ([9185042](9185042))
* **codedeploy:** the Service Principal is wrong in isolated regions ([#19729](#19729)) ([7e9a43d](7e9a43d)), closes [#19399](#19399)
* **core:** `Fn.select` incorrectly short-circuits complex expressions ([#19680](#19680)) ([7f26fad](7f26fad))
* **core:** detect and resolve stringified number tokens ([#19578](#19578)) ([7d9ab2a](7d9ab2a)), closes [#19546](#19546) [#19550](#19550)
* **core:** reduce CFN template indent size to save bytes ([#19656](#19656)) ([fd63ca3](fd63ca3))
* **ecs:** 'desiredCount' and 'ephemeralStorageGiB' cannot be tokens ([#19453](#19453)) ([c852239](c852239)), closes [#16648](#16648)
* **ecs:** remove unnecessary error when adding volume to external task definition ([#19774](#19774)) ([5446ded](5446ded)), closes [#19259](#19259)
* **iam:** policies aren't minimized as far as possible ([#19764](#19764)) ([876ed8a](876ed8a)), closes [#19751](#19751)
* **logs:** Faulty Resource Policy Generated ([#19640](#19640)) ([1fdf122](1fdf122)), closes [#17544](#17544)
StevePotter pushed a commit to StevePotter/aws-cdk that referenced this pull request Apr 27, 2022
@rix0rrr
fix(core): Fn.select incorrectly short-circuits complex expressions (…
840877d 
…aws#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*
@asnaseer-resilient asnaseer-resilient mentioned this pull request Jul 15, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@skinny85 skinny85 skinny85 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rix0rrr rix0rrr

Labels

contribution/core This is a PR that came from AWS. p1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rix0rrr @aws-cdk-automation @skinny85