Skip to content

CFN: add more validations to intrinsic FnEquals #13217

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
pinzon merged 6 commits into from
Oct 10, 2025
Merged

CFN: add more validations to intrinsic FnEquals #13217

pinzon merged 6 commits into from
Oct 10, 2025
+63 −0

Conversation

@pinzon
Copy link
Member

@pinzon pinzon commented Oct 2, 2025 •
edited
Loading

Motivation

This PR adds a validation on the number of arguments the Fn::Equals receives. Some user are getting finding issues with the new CFn engine due to the lack of ValidationError messages.

Example:

!Equals [ !Ref Parameter]

Should be:

!Equals [!Ref Parameter, 1]

Changes

  • Trigger the visitation of conditions during the validation
  • Validate number of arguments of the Fn::Equals

Testing

  • AWS Validated test

@pinzon pinzon added this to the Playground milestone Oct 2, 2025
@pinzon pinzon force-pushed the cp/cfn/v2/validations branch from fad4290 to 8f1c4ef Compare October 7, 2025 18:04
@github-actions
Copy link

github-actions bot commented Oct 7, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 298 tests  ±0   20 555 ✅ ±0   14m 58s ⏱️ - 1m 14s
     1 suites ±0    1 743 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 38c6319. ± Comparison against base commit 658d6fb.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both. 
tests.unit.test_moto ‑ test_service_exception_translator_context_manager

tests.unit.aws.handlers.test_service.TestServiceExceptionSerializer ‑ test_moto_exception_is_parsed

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Oct 7, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

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

Results for commit 38c6319. ± Comparison against base commit 658d6fb.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Oct 7, 2025 •
edited
Loading

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   36m 12s ⏱️ - 1h 23m 16s
585 tests  - 4 228  471 ✅  - 4 005  114 💤  - 223  0 ❌ ±0 
587 runs   - 4 228  471 ✅  - 4 005  116 💤  - 223  0 ❌ ±0 

Results for commit 38c6319. ± Comparison against base commit 658d6fb.

This pull request removes 4228 tests. 
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]
…

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Oct 7, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

  5 files  ±    0    5 suites  ±0   48m 51s ⏱️ - 1h 49m 36s
609 tests  - 4 578  496 ✅  - 4 194  113 💤  - 384  0 ❌ ±0 
615 runs   - 4 578  496 ✅  - 4 194  119 💤  - 384  0 ❌ ±0 

Results for commit 38c6319. ± Comparison against base commit 658d6fb.

This pull request removes 4578 tests. 
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]
…

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Oct 7, 2025 •
edited
Loading

Test Results - Alternative Providers

584 tests   - 783   330 ✅  - 381   26m 4s ⏱️ - 13m 52s
  1 suites  -   4   254 💤  - 402 
  1 files    -   4     0 ❌ ±  0 

Results for commit 38c6319. ± Comparison against base commit 658d6fb.

This pull request removes 783 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.

@pinzon pinzon force-pushed the cp/cfn/v2/validations branch from 8f1c4ef to b5f6aea Compare October 8, 2025 18:02
@pinzon pinzon changed the title Cp/cfn/v2/validations cfn: add more validations to intrinsic functions Oct 8, 2025
@pinzon pinzon changed the title cfn: add more validations to intrinsic functions CFN: add more validations to intrinsic FnEquals Oct 8, 2025
@pinzon pinzon mentioned this pull request Oct 8, 2025
Merged
@pinzon pinzon added aws:cloudformation AWS CloudFormation semver: patch Non-breaking changes which can be included in patch releases aws:cloudformation:v2 Issues related to the V2 CloudFormation engine docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes labels Oct 8, 2025
@pinzon pinzon force-pushed the cp/cfn/v2/validations branch from ffeb0eb to 840ba70 Compare October 8, 2025 21:39
@pinzon pinzon marked this pull request as ready for review October 9, 2025 01:34
simonrw
simonrw approved these changes Oct 10, 2025
View reviewed changes
Copy link
Contributor

@simonrw simonrw left a comment

Choose a reason for hiding this comment

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

I think the test construction (with unused condition) is a little weird, however it's still valid behaviour that we now have parity with!

tests/aws/services/cloudformation/test_change_set_fn_split.snapshot.json Outdated
}
}
},
"tests/aws/services/cloudformation/test_change_set_fn_split.py::TestValidations::test_validate_args_len": {
Copy link
Contributor

@simonrw simonrw Oct 10, 2025

Choose a reason for hiding this comment

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

question: where did these snapshots come from? I guess they are left over from your split PR (#13244), can we remove them before merging?

Copy link
Member Author

@pinzon pinzon Oct 10, 2025

Choose a reason for hiding this comment

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

You're right. Nice catch 👍

tests/aws/services/cloudformation/engine/test_conditions.py
@skip_if_legacy_engine
def test_validate_equals_args_len(self, aws_client, snapshot):
template = {
"Conditions": {"ShouldDeploy": {"Fn::Equals": ["a"]}},
Copy link
Contributor

@simonrw simonrw Oct 10, 2025

Choose a reason for hiding this comment

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

TIL you can have a condition that's unused!

localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_visitor.py
# entities (parameters, mappings, conditions, etc.). Then compute the output fields; computing
# only the output fields would only result in the deployment logic of the referenced outputs
# being evaluated, hence enforce the visiting of all the resources first.
self.visit(node_template.conditions)
Copy link
Contributor

@simonrw simonrw Oct 10, 2025

Choose a reason for hiding this comment

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

I guess this is only needed because in your test you have an unused condition, but in general this should not be required since resources would refer to the conditions which would cause the visiting process to visit the conditions, right?

I'm glad we have this extra parity with explicitly visiting the conditions (perhaps we should explicitly visit all nodes?) but it seems like a more contrived change than it needs to be.

Copy link
Member Author

@pinzon pinzon Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'll leave this here for now. But I have an idea to discuss later.

@pinzon pinzon merged commit 69ed784 into main Oct 10, 2025
40 checks passed
@pinzon pinzon deleted the cp/cfn/v2/validations branch October 10, 2025 17:18
baermat pushed a commit that referenced this pull request Oct 13, 2025
@pinzon @baermat
CFN: add more validations to intrinsic FnEquals (#13217)
6955908
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simonrw simonrw simonrw approved these changes

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

Assignees

No one assigned

Labels

aws:cloudformation:v2 Issues related to the V2 CloudFormation engine 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 @simonrw