Skip to content

CFn executor v2: provide previous payload correctly #12511

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.

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

Merged
merged 22 commits into from
Apr 16, 2025

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Apr 10, 2025

Motivation

There are a few limitations with the POC executor as it stands. It does not correctly propagate the previous state of the resource to the resource provider. This means that the resource provider is not aware of the previous state, even on update which is incorrect.

In addition the executor does not currently understand or propagate the template parameter values (as specified alongside the template) so the describer and executor cannot correctly determine if resources need to be updated because of parameter changes.

Unfortunately the scope of this PR expanded quite a bit to include provider cleanup (for v2) and simplification of the internal representation of stacks and change sets. This is because the old code was not designed to be used in a provider context and was very tightly coupled to the old executor. I have also added more robustness to the resource provider deploy process, since I was finding it difficult to debug deployment failures without it.

The bottom line is:

  • 6 change set capture tests (and one of the parametrized variants of another) are now passing
  • One additional test was added specifically fetching the resource value before and after update to ensure the resource was correctly updated
  • One test (tests/aws/services/cloudformation/api/test_changesets.py::TestUpdates::test_deleting_resource) fails with the old provider, but passes with the new 🎉

Changes

  • Added two new helpers for fetching and extracting the template potentially from a remote source
    • this splits apart two two functions out of the previous one which had confusing responsibilities
  • Rename some types to be more descriptive (e.g. StackMetadata => CreateChangeSetInput)
  • Stop using get_resource_type since it's not overridden in pro any more
  • Create new Stack and ChangeSet types
  • Add new fields in the store for the new type stacks and change sets
  • Implement _reduce_intrinsic_function_ref_value therefore supporting Ref lookups from resources
  • Simplify the executor constructor only taking the change set
  • Strip out most of the unneeded old code from the new v2 provider change set methods
  • Bring across provider methods that interact with stacks as they now need to use the stacks_v2 property in the store
  • Pick up previous properties from the last deploy from this store in the execute change set process so we pass the correct payload to the resource providers
  • Update skip marks for tests by either removing the skip marker or updating the failure reason if it's unrelated to deployment
  • Added some debug logging for deployment
  • Handle failure and exceptions during deployment
  • Add new test that tests the deployed resource and asserts it has been updated

@simonrw simonrw requested review from MEPalma and removed request for dominikschubert and pinzon April 10, 2025 10:29
@simonrw simonrw force-pushed the cfn/v2/send-previous-properties branch from e23ed84 to b279404 Compare April 10, 2025 10:32
@simonrw simonrw added the semver: patch Non-breaking changes which can be included in patch releases label Apr 10, 2025
@simonrw simonrw self-assigned this Apr 10, 2025
Copy link

github-actions bot commented Apr 10, 2025

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   20m 32s ⏱️ - 1h 31m 29s
474 tests  - 3 896  328 ✅  - 3 686  146 💤  - 210  0 ❌ ±0 
476 runs   - 3 896  328 ✅  - 3 686  148 💤  - 210  0 ❌ ±0 

Results for commit 67d8ad8. ± Comparison against base commit 2ec3574.

This pull request removes 3897 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.api.test_changesets.TestCaptureUpdateProcess ‑ test_execute_with_ref
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.api.test_changesets.TestCaptureUpdateProcess ‑ test_execute_with_ref

♻️ This comment has been updated with latest results.

@simonrw simonrw changed the title CFn executor v2: support updates for SSM parameters CFn executor v2: provide previous payload correctly Apr 10, 2025
Copy link

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   8m 33s ⏱️
488 tests 438 ✅  50 💤 0 ❌
976 runs  876 ✅ 100 💤 0 ❌

Results for commit 3a6780b.

@simonrw simonrw force-pushed the cfn/v2/send-previous-properties branch from 3a6780b to d072285 Compare April 11, 2025 22:23
@simonrw simonrw marked this pull request as draft April 11, 2025 22:50
@alexrashed alexrashed removed their request for review April 15, 2025 06:20
@simonrw simonrw force-pushed the cfn/v2/send-previous-properties branch from 05fbb4f to e32c9d6 Compare April 15, 2025 15:14
@simonrw simonrw marked this pull request as ready for review April 15, 2025 15:48
@simonrw simonrw requested review from dominikschubert and removed request for dfangl April 15, 2025 15:48
simonrw added 10 commits April 15, 2025 20:57
# Conflicts:
#	tests/aws/services/cloudformation/api/test_changesets.py

diff --git c/localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_executor.py i/localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_executor.py
index c341c76..503ae06 100644
--- c/localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_executor.py
+++ i/localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_executor.py
@@ -60,6 +60,8 @@ class ChangeSetModelExecutor(ChangeSetModelPreproc):
         return delta

     def _reduce_intrinsic_function_ref_value(self, preproc_value: Any) -> Any:
+        if preproc_value is None:
+            return None
         resource = self.resources.get(preproc_value.name)
         if resource is None:
             raise NotImplementedError(f"No resource '{preproc_value.name}' found")
diff --git c/localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py i/localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py
index bc3e6ce..5addbe4 100644
--- c/localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py
+++ i/localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py
@@ -173,20 +173,20 @@ class ChangeSetModelPreproc(ChangeSetModelVisitor):
                 return condition
         return None

-    def _resolve_reference(self, logica_id: str) -> PreprocEntityDelta:
-        node_condition = self._get_node_condition_if_exists(condition_name=logica_id)
+    def _resolve_reference(self, logical_id: str) -> PreprocEntityDelta:
+        node_condition = self._get_node_condition_if_exists(condition_name=logical_id)
         if isinstance(node_condition, NodeCondition):
             condition_delta = self.visit(node_condition)
             return condition_delta

-        node_parameter = self._get_node_parameter_if_exists(parameter_name=logica_id)
+        node_parameter = self._get_node_parameter_if_exists(parameter_name=logical_id)
         if isinstance(node_parameter, NodeParameter):
             parameter_delta = self.visit(node_parameter)
             return parameter_delta

         # TODO: check for KNOWN AFTER APPLY values for logical ids coming from intrinsic functions as arguments.
         node_resource = self._get_node_resource_for(
-            resource_name=logica_id, node_template=self._node_template
+            resource_name=logical_id, node_template=self._node_template
         )
         resource_delta = self.visit(node_resource)
         before = resource_delta.before
@@ -210,11 +210,11 @@ class ChangeSetModelPreproc(ChangeSetModelVisitor):
     ) -> PreprocEntityDelta:
         before = None
         if before_logical_id is not None:
-            before_delta = self._resolve_reference(logica_id=before_logical_id)
+            before_delta = self._resolve_reference(logical_id=before_logical_id)
             before = before_delta.before
         after = None
         if after_logical_id is not None:
-            after_delta = self._resolve_reference(logica_id=after_logical_id)
+            after_delta = self._resolve_reference(logical_id=after_logical_id)
             after = after_delta.after
         return PreprocEntityDelta(before=before, after=after)

@@ -335,7 +335,7 @@ class ChangeSetModelPreproc(ChangeSetModelVisitor):

         def _compute_delta_for_if_statement(args: list[Any]) -> PreprocEntityDelta:
             condition_name = args[0]
-            boolean_expression_delta = self._resolve_reference(logica_id=condition_name)
+            boolean_expression_delta = self._resolve_reference(logical_id=condition_name)
             return PreprocEntityDelta(
                 before=args[1] if boolean_expression_delta.before else args[2],
                 after=args[1] if boolean_expression_delta.after else args[2],
@@ -420,16 +420,20 @@ class ChangeSetModelPreproc(ChangeSetModelVisitor):
         before_logical_id = arguments_delta.before
         before = None
         if before_logical_id is not None:
-            before_delta = self._resolve_reference(logica_id=before_logical_id)
+            before_delta = self._resolve_reference(logical_id=before_logical_id)
             before_value = before_delta.before
             before = self._reduce_intrinsic_function_ref_value(before_value)

         after_logical_id = arguments_delta.after
         after = None
         if after_logical_id is not None:
-            after_delta = self._resolve_reference(logica_id=after_logical_id)
+            after_delta = self._resolve_reference(logical_id=after_logical_id)
             after_value = after_delta.after
-            after = self._reduce_intrinsic_function_ref_value(after_value)
+            # TODO: swap isinstance to be a structured type check
+            if isinstance(after_value, str):
+                after = after_value
+            else:
+                after = self._reduce_intrinsic_function_ref_value(after_value)

         return PreprocEntityDelta(before=before, after=after)

diff --git c/tests/aws/services/cloudformation/api/test_changesets.py i/tests/aws/services/cloudformation/api/test_changesets.py
index 6ad8608..a2f002f 100644
--- c/tests/aws/services/cloudformation/api/test_changesets.py
+++ i/tests/aws/services/cloudformation/api/test_changesets.py
@@ -105,7 +105,6 @@ class TestUpdates:
         res.destroy()

     @markers.aws.needs_fixing
-    @pytest.mark.skip(reason="WIP")
     def test_deleting_resource(self, aws_client: ServiceLevelClientFactory, deploy_cfn_template):
         parameter_name = "my-parameter"
         value1 = "foo"
@simonrw simonrw force-pushed the cfn/v2/send-previous-properties branch from e32c9d6 to 67d8ad8 Compare April 15, 2025 19:59
@simonrw simonrw merged commit 3936886 into master Apr 16, 2025
31 checks passed
@simonrw simonrw deleted the cfn/v2/send-previous-properties branch April 16, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant