Skip to content

CFNv2: parameter resolution and presentation #12796

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
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

CFNv2: parameter resolution and presentation #12796

wants to merge 7 commits into from

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Jun 24, 2025 •
edited
Loading

Motivation

We currently don't support the resolution of SSM parameters in the template Parameters when performing template describing, only executing. This does not match AWS behaviour, which represents the resolved values as well as the given parameters.

Changes

  • Move SSM parameter resolution to the preprocessing step as it's performed during create_change_set, so we should do it before describing and executing
  • Add significant test for resolving SSM parameters
  • Remove skip from cdk bootstrap since it's no longer failing
  • Add structured type to parameters when describing so we can represent the original value (e.g. SSM parameter name) and resolved value (the SSM parameter value)

@simonrw simonrw added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Jun 24, 2025
@simonrw simonrw added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Jun 24, 2025
@simonrw simonrw marked this pull request as draft June 24, 2025 15:26
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 24, 2025 •
edited
Loading

Test Results - Preflight, Unit

21 727 tests  ±0   20 070 ✅ ±0   7m 13s ⏱️ +48s
     1 suites ±0    1 657 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 6c4cd17. ± Comparison against base commit 62f162f.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 24, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

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

Results for commit 6c4cd17. ± Comparison against base commit 62f162f.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 24, 2025 •
edited
Loading

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   21m 29s ⏱️ - 1h 22m 59s
887 tests  - 4 007  323 ✅  - 3 797  564 💤  - 210  0 ❌ ±0 
889 runs   - 4 007  323 ✅  - 3 797  566 💤  - 210  0 ❌ ±0 

Results for commit 6c4cd17. ± Comparison against base commit 62f162f.

This pull request removes 4008 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.v2.test_change_sets ‑ test_dynamic_ssm_parameter_lookup
This pull request removes 211 skipped tests and adds 1 skipped test. Note that renamed tests count towards both. 
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input4-FAILED]
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_deployed_infra_state
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_populate_data
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_user_clicks_are_stored
tests.aws.services.apigateway.test_apigateway_api.TestApiGatewayApiRestApi ‑ test_get_api_case_insensitive
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_put_integration_request_parameter_bool_type
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_api_gateway_authorizer_crud
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_api_gateway_http_integration_with_path_request_parameter
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_api_gateway_lambda_proxy_integration[/lambda/foo1]
…

tests.aws.services.cloudformation.v2.test_change_sets ‑ test_dynamic_ssm_parameter_lookup

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 24, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

  5 files    5 suites   34m 34s ⏱️
911 tests 348 ✅ 563 💤 0 ❌
917 runs  348 ✅ 569 💤 0 ❌

Results for commit 6c4cd17.

♻️ This comment has been updated with latest results.

@simonrw simonrw marked this pull request as ready for review June 24, 2025 22:54
@simonrw simonrw requested a review from MEPalma June 24, 2025 22:55
MEPalma
MEPalma reviewed Jun 26, 2025
View reviewed changes
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model.py
@@ -1382,7 +1394,7 @@ def _type_name_of(value: Maybe[Any]) -> str:

@staticmethod
def _is_terminal(value: Any) -> bool:
return type(value) in {int, float, bool, str, None, NothingType}
return type(value) in {int, float, bool, str, None, NothingType, ResolvedParameter}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand why we need this in the modeler?

MEPalma
MEPalma reviewed Jun 26, 2025
View reviewed changes
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py
@@ -304,6 +308,10 @@ def _resolve_reference(self, logical_id: str) -> PreprocEntityDelta:
node_parameter = self._get_node_parameter_if_exists(parameter_name=logical_id)
if isinstance(node_parameter, NodeParameter):
parameter_delta = self.visit(node_parameter)
if isinstance(parameter_delta.before, ResolvedParameter):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to first wrap the relevant values in a ResolvedParameter class and only evaluate them here?

MEPalma
MEPalma reviewed Jun 26, 2025
View reviewed changes
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py
region_name=self._change_set.region_name,
stack_parameter_value=after,
)
after = ResolvedParameter(value=after, resolved_value=resolved_value)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we not resolving the resolved parameter here (this visitor function), but later?

@simonrw simonrw added this to the Playground milestone Jun 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MEPalma MEPalma MEPalma left review comments

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

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

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
Playground
Development

Successfully merging this pull request may close these issues.
2 participants
@simonrw @MEPalma