Skip to content

CFn: fix invalid behaviour for nested intrinsic calls #13146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 16, 2025
Merged

CFn: fix invalid behaviour for nested intrinsic calls #13146

merged 3 commits into from
Sep 16, 2025

Conversation

@simonrw
Copy link
Contributor

@simonrw simonrw commented Sep 16, 2025

Motivation

A previous PR #13136 broke template deployment in the Pro repsitory. The issue was that while visiting the template for CreateChangeSet in the transformer, we were not handling the case where the nested intrinsic function calls would not return None but would still return a semi-valid transformed value. In particular, the inner intrinsic would return the visited arguments (say ["Resource", "Attribute"] from a GetAtt call. The arguments to select may be something like:

Fn::Select: [4, [":", ["Resource", "Attribute"]]]
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^    the result of visiting `Fn::Join` arguments
                      ^^^^^^^^^^^^^^^^^^^^^^^^      the result of visiting `Fn::GetAtt` arguments
             ^                                      index still out of range

This problem is really highlighting that we need a way of traversing the template, but returning a KNOWN sentinel value representing an unknown value. Let's tackle this in a follow up.

Changes

  • Fix the Fn::Select argument handling
  • Add validated test that captures the behaviour that failed in the Pro pipeline
  • Update the transform visit...get_att to better match the other overrides
  • Update the validator visit...get_att to replicate the structure of the transform implementation
  • Update the ECR repo resource provider to assign a default repository name
    • Ensure the name is lowercase to match the regex that ECR expects: (?:[a-z0-9]+(?:[._-][a-z0-9]+)*/)*[a-z0-9]+(?:[._-][a-z0-9]+)*

@simonrw simonrw added this to the 4.9 milestone Sep 16, 2025
@simonrw simonrw added aws:cloudformation AWS CloudFormation semver: patch Non-breaking changes which can be included in patch releases review: merge when ready Signals to the reviewer that a PR can be merged if accepted docs: skip Pull request does not require documentation changes labels Sep 16, 2025
@github-actions
Copy link

github-actions bot commented Sep 16, 2025

Test Results - Preflight, Unit

22 145 tests  ±0   20 407 ✅ ±0   6m 24s ⏱️ -1s
     1 suites ±0    1 738 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 43c1fe6. ± Comparison against base commit f8e915e.

@github-actions
Copy link

github-actions bot commented Sep 16, 2025

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 7s ⏱️ ±0s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 43c1fe6. ± Comparison against base commit f8e915e.

@github-actions
Copy link

github-actions bot commented Sep 16, 2025

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   35m 42s ⏱️ - 1h 19m 18s
572 tests  - 4 097  457 ✅  - 3 881  115 💤  - 216  0 ❌ ±0 
574 runs   - 4 097  457 ✅  - 3 881  117 💤  - 216  0 ❌ ±0 

Results for commit 43c1fe6. ± Comparison against base commit f8e915e.

This pull request removes 4098 and adds 1 tests. Note that renamed tests count towards both. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.cloudformation.test_change_set_fn_select.TestChangeSetFnSelect ‑ test_nested_select_within_other_intrinsics

@github-actions
Copy link

github-actions bot commented Sep 16, 2025

Test Results (amd64) - Integration, Bootstrap

  5 files    5 suites   47m 9s ⏱️
596 tests 482 ✅ 114 💤 0 ❌
602 runs  482 ✅ 120 💤 0 ❌

Results for commit 43c1fe6.

@simonrw simonrw marked this pull request as ready for review September 16, 2025 11:23
@github-actions
Copy link

github-actions bot commented Sep 16, 2025

Test Results - Alternative Providers

571 tests   330 ✅  24m 11s ⏱️
  1 suites  241 💤
  1 files      0 ❌

Results for commit 43c1fe6.

@simonrw simonrw merged commit 7a1d430 into main Sep 16, 2025
60 checks passed
@simonrw simonrw deleted the cfn/v2/fix-nested-selects branch September 16, 2025 13:52
@simonrw simonrw mentioned this pull request Sep 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dfangl dfangl dfangl approved these changes

@pinzon pinzon Awaiting requested review from pinzon pinzon is a code owner

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

Assignees

No one assigned

Labels

aws:cloudformation AWS CloudFormation docs: skip Pull request does not require documentation changes review: merge when ready Signals to the reviewer that a PR can be merged if accepted semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Milestone

4.9

Development

Successfully merging this pull request may close these issues.

3 participants

@simonrw @dfangl