fix(cloudformation): skip properties of conditional resources with false conditions#13664
Conversation
…lse conditions When deleting a CloudFormation stack, the v2 engine was incorrectly evaluating properties of conditional resources even when their condition evaluated to false (meaning the resource was never created). This caused deletion to fail with "Unresolved resource dependencies" errors when a conditional resource referenced another conditional resource via GetAtt. The fix checks whether a resource's condition is true BEFORE visiting its properties, preventing resolution of references to resources that may not exist. Fixes localstack#13609 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
sodre
requested review from
dominikschubert,
pinzon and
simonrw
as code owners
localstack-bot
left a comment
localstack-bot
left a comment
There was a problem hiding this comment.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
pinzon
left a comment
pinzon
left a comment
There was a problem hiding this comment.
Hi @sodre thank you for your interest in contributing to LocalStack and kudos for using AI to help you code. There are some issues with this PR:
-
test_delete_stack_with_conditional_getattpasses without your fixes. So either the test is unable to reproduce the issue or the issue has been already fixed. -
test_delete_stack_with_conditional_resource_referencing_conditionalwithout your changes tochange_set_model_preproc.py, the template is rejected even before deploying. It seems that our validation incorrectly rejects this template because is unable to handle the conditional definition correctly. Your changes fixes that. You could repurpose your test for that behavior. -
When submitting a PR, you need to make sure to test against AWS using
TEST_TARGET=AWS_CLOUD; AWS_DEFAULT_PROFILE=ls-sandbox; AWS_DEFAULT_REGION=us-east-1. This generates a validation file that tells the reviewer that your tests are AWS valid.
|
@pinzon, thank you for the feedback. I would not have been able to write the tests without the AI help in this case, given that localstack is a black box for me. I (and Claude) will work on making a smaller change set than the one proposed here. I will update the PR to address item 3 as well. |
…idation - Remove test_delete_stack_with_conditional_getatt (passed without fix) - Rename and repurpose test to verify template acceptance with conditions - Use snapshot matching instead of manual assertions - Delete unused conditional-getatt-delete.yaml template - Add AWS-validated snapshot and validation data Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
sodre
force-pushed
the
fix/cfn-v2-conditional-resource-deletion-13609
branch
from
eae4b84 to
317f124
Compare
alexrashed
added
semver: patch
pinzon
left a comment
pinzon
left a comment
There was a problem hiding this comment.
Thanks for the changes. Let's just remove the extra comments. In the meantime I'll approve the CI checks. 👍
tests/aws/templates/conditions/conditional-resource-getatt-delete.yaml
Outdated
Show resolved
Hide resolved
…ete.yaml Co-authored-by: Cristopher Pinzón <18080804+pinzon@users.noreply.github.com>
Co-authored-by: Cristopher Pinzón <18080804+pinzon@users.noreply.github.com>
Awesome! I've committed your changes. |
pinzon
left a comment
pinzon
left a comment
There was a problem hiding this comment.
Great PR. Thank you for addressing the comments 👍
|
Thank you @pinzon, I credit Claude, my co-author :) |
4 participants
Summary
Fn::Ifwrapped GetAtt and direct conditional resource referencesRoot Cause
In
visit_node_resource, properties were visited unconditionally before checking if the resource's condition was true. This caused!GetAttreferences inside conditional resources to be evaluated even when the resource was never created (condition=false), resulting in "Unresolved resource dependencies" errors during stack deletion.The Fix
Check whether a resource's condition evaluates to true before visiting its properties. This prevents resolution of references to other conditional resources that may not exist.
Test plan
test_delete_stack_with_conditional_getatt- tests!GetAttwrapped inFn::Iftest_delete_stack_with_conditional_resource_referencing_conditional- tests!GetAttinside a conditional resourceFixes #13609
🤖 Generated with Claude Code