Skip to content

CFN: add validation in GetAtt for conditionally canceled resources #13213

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
Oct 2, 2025
Merged

CFN: add validation in GetAtt for conditionally canceled resources #13213

merged 3 commits into from
Oct 2, 2025

Conversation

@pinzon
Copy link
Member

@pinzon pinzon commented Sep 30, 2025

Motivation

This PR adds a validation that the resource should be deployed when retrieving attributes with Fn::GetAtt

Changes

  • Validate if there is a condition preventing the deployment of a resource before attempting to retrieve an attribute.

Testing

  • AWS validated test.

@pinzon pinzon added aws:cloudformation AWS CloudFormation semver: patch Non-breaking changes which can be included in patch releases docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes labels Sep 30, 2025
@github-actions
Copy link

github-actions bot commented Sep 30, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 281 tests  ±0   20 538 ✅ ±0   16m 38s ⏱️ +35s
     1 suites ±0    1 743 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 013242b. ± Comparison against base commit 77b0748.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 30, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

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

Results for commit 013242b. ± Comparison against base commit 77b0748.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 30, 2025 •
edited
Loading

Test Results - Alternative Providers

575 tests   - 775   330 ✅  - 371   26m 17s ⏱️ - 13m 9s
  1 suites  -   4   245 💤  - 404 
  1 files    -   4     0 ❌ ±  0 

Results for commit 013242b. ± Comparison against base commit 77b0748.

This pull request removes 775 tests. 
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_basic_operations_multiple_protocols[json]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_basic_operations_multiple_protocols[query]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_basic_operations_multiple_protocols[smithy-rpc-v2-cbor]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_exception_serializing_with_no_shape_in_spec[json]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_exception_serializing_with_no_shape_in_spec[query]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_exception_serializing_with_no_shape_in_spec[smithy-rpc-v2-cbor]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_multi_protocol_client_fixture[json]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_multi_protocol_client_fixture[query]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudWatchMultiProtocol ‑ test_multi_protocol_client_fixture[smithy-rpc-v2-cbor]
tests.aws.services.cloudwatch.test_cloudwatch.TestCloudwatch ‑ test_alarm_lambda_target
…

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 30, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files  ±0      5 suites  ±0   2h 38m 8s ⏱️ - 2m 10s
5 169 tests ±0  4 673 ✅ ±0  496 💤 ±0  0 ❌ ±0 
5 175 runs  ±0  4 673 ✅ ±0  502 💤 ±0  0 ❌ ±0 

Results for commit 013242b. ± Comparison against base commit 77b0748.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 30, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 59m 50s ⏱️ +2s
4 795 tests ±0  4 459 ✅ ±0  336 💤 ±0  0 ❌ ±0 
4 797 runs  ±0  4 459 ✅ ±0  338 💤 ±0  0 ❌ ±0 

Results for commit 013242b. ± Comparison against base commit 77b0748.

♻️ This comment has been updated with latest results.

@pinzon pinzon marked this pull request as ready for review September 30, 2025 21:51
@pinzon pinzon added this to the Playground milestone Sep 30, 2025
dominikschubert
dominikschubert approved these changes Oct 2, 2025
View reviewed changes
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this fixes an immediate issue, I'm proposing we move forward with the fix here. 👍
I'd like to circle back to this soon though to see if we could find a better solution for raising these exceptions. Intuitively it seems this might not be the ideal location in the engine to raise them, as we might need to duplicate the effort here. 🤔 I'm not familiar enough yet with the engine though, to give more useful advice on alternatives, so again, let's circle back on this

tests/aws/services/cloudformation/engine/test_conditions.py Outdated
Comment on lines 138 to 139
topic_name = f"test-topic-{short_uid()}"
ssm_param_name = f"test-param-{short_uid()}"
Copy link
Member

@dominikschubert dominikschubert Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Both of these names are not actually necessary for this test and removing them + the name parameters from the template would simplify the test. This in turn makes it easier to focus on the core concept you'd like to test, which is the reference to a resource with a condition evaluating to false.

@pinzon
Copy link
Member Author

pinzon commented Oct 2, 2025

I'd like to circle back to this soon though to see if we could find a better solution for raising these exceptions. Intuitively it seems this might not be the ideal location in the engine to raise them, as we might need to duplicate the effort here.

Yes. This is something that @simonrw and I have been discussing. We have been raising ValidationErrors in the Preprocessor class but we believe they should be part of Validator class. Hopefully we will find a solution soon.

@pinzon pinzon merged commit 478dbcf into main Oct 2, 2025
41 checks passed
@pinzon pinzon deleted the cfn/fix/conditional-get-att branch October 2, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dominikschubert dominikschubert dominikschubert approved these changes

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

Assignees

No one assigned

Labels

aws:cloudformation AWS CloudFormation docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Milestone

Playground

Development

Successfully merging this pull request may close these issues.

3 participants

@pinzon @dominikschubert