This repository was archived by the owner on Mar 23, 2026. It is now read-only.

CFn: implement DeletionPolicy and UpdateReplacePolicy#13535

Merged

simonrw merged 6 commits intomainfrom

cfn/handle-policies

Feb 5, 2026

Contributor

simonrw commented Dec 17, 2025

Motivation

A user has reported that setting policies is not only not implemented by our engigne, but actually incorrectly raises an error.

Changes

Don't raise error when modeling the policy fields
Skip deletion if UpdateReplacePolicy=Retain and a resource is updated
Skip deletion if DeletionPolicy=Retain and a resource is deleted

Related to

Resolves ENG-200

simonrw added semver: patch docs: skip notes: skip labels

github-actions bot commented Dec 17, 2025 •

edited

Loading

Test Results - Preflight, Unit

23 114 tests ±0 21 255 ✅ ±0 6m 25s ⏱️ +7s
1 suites ±0 1 859 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 988559e. ± Comparison against base commit cf9a6fc.

♻️ This comment has been updated with latest results.

github-actions bot commented Dec 17, 2025 •

edited

Loading

LocalStack Community integration with Pro

2 files ±0 2 suites ±0 1h 58m 6s ⏱️ - 1m 6s
5 196 tests +4 4 845 ✅ +4 351 💤 ±0 0 ❌ ±0
5 198 runs +4 4 845 ✅ +4 353 💤 ±0 0 ❌ ±0

Results for commit 988559e. ± Comparison against base commit cf9a6fc.

♻️ This comment has been updated with latest results.

simonrw force-pushed the cfn/handle-policies branch from 2e205bc to 7c41c83 Compare

December 17, 2025 10:21

github-actions bot commented Dec 17, 2025 •

edited

Loading

Test Results (amd64) - Acceptance

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

Results for commit 988559e. ± Comparison against base commit cf9a6fc.

♻️ This comment has been updated with latest results.

github-actions bot commented Dec 17, 2025 •

edited

Loading

Test Results - Alternative Providers

590 tests 323 ✅ 17m 54s ⏱️
1 suites 267 💤
1 files 0 ❌

Results for commit 988559e.

♻️ This comment has been updated with latest results.

github-actions bot commented Dec 17, 2025 •

edited

Loading

Test Results (amd64) - Integration, Bootstrap

5 files 5 suites 2h 39m 46s ⏱️
5 617 tests 5 096 ✅ 521 💤 0 ❌
5 623 runs 5 096 ✅ 527 💤 0 ❌

Results for commit 988559e.

♻️ This comment has been updated with latest results.

simonrw force-pushed the cfn/handle-policies branch from 7c41c83 to f53ac96 Compare

December 18, 2025 20:26

simonrw marked this pull request as ready for review

December 18, 2025 22:20

simonrw requested review from dominikschubert and pinzon as code owners

December 18, 2025 22:20

pinzon suggested changes

View reviewed changes

Member

pinzon left a comment

Great PR.
The only blocking issues are the lack of cleanup of the retained parameters and the commented out code, feel free to ignore my suggestion on the snapshot skip if you deem it unimportant.

tests/aws/services/cloudformation/engine/test_policies.py Outdated

Comment on lines +36 to +37

		# TODO: provider parity issue with error message
		# snapshot.match("error", exc_info.value)

Member

pinzon Dec 22, 2025

Can't we add a snapshot skip so we can keep the snapshot?

tests/aws/services/cloudformation/engine/test_policies.py Outdated


		stack.destroy()

		value = aws_client.ssm.get_parameter(Name=parameter_name)["Parameter"]["Value"]

Member

pinzon Dec 22, 2025

We should add a cleanup for the parameter

Contributor Author

simonrw Jan 23, 2026

Great catch, thank you!

tests/aws/services/cloudformation/engine/test_policies.py Outdated

Comment on lines +119 to +120

		with pytest.raises(ClientError):
		aws_client.ssm.get_parameter(Name=parameter_name)["Parameter"]["Value"]

Member

pinzon Dec 22, 2025

Let's also add the snapshot here

tests/aws/services/cloudformation/engine/test_policies.py Outdated

+                      )
+                      # check the previous parameter was not deleted
+                      aws_client.ssm.get_parameter(Name=parameter_name)["Parameter"]["Value"]

Member

pinzon Dec 22, 2025

Here we should also add a cleanup for the retained parameter

tests/aws/services/cloudformation/engine/test_policies.py Outdated

Member

pinzon Dec 22, 2025 •

edited

Loading

We should consider splitting this file in two files: test_deletion_policy.py and test_update_replace_policy.py.
So in the future we can cleanly expand to test the other policy configurations.

simonrw modified the milestones: Playground, 4.13

simonrw force-pushed the cfn/handle-policies branch from f53ac96 to e78199a Compare

January 23, 2026 12:05

simonrw requested a review from pinzon

January 23, 2026 12:05

Contributor Author

simonrw commented Jan 23, 2026

Thanks for the review @pinzon I've hopefully addressed your (very valid!) comments. Please take another look

pinzon suggested changes

View reviewed changes

Member

pinzon left a comment

Pretty good PR. The blocking issues are the comments in the tests and the templates.

tests/aws/services/cloudformation/engine/test_deletion_policy.py

+              @markers.aws.validated
+              @markers.snapshot.skip_snapshot_verify(
+                  paths=[

Member

pinzon Jan 23, 2026 •

edited

Loading

Change: we should transform the PhysicalResourceId

Contributor Author

simonrw Feb 2, 2026

Except this is a parameter that's not output by LocalStack. It's not just that the value is different.

Member

pinzon Feb 3, 2026

Still the snapshot won't match even if we run the test against AWS again.

tests/aws/services/cloudformation/engine/test_deletion_policy.py

+              @markers.snapshot.skip_snapshot_verify(
+                  paths=[
+                      # our message is different. The AWS message does not seem to include the parameter

Member

pinzon Jan 23, 2026 •

edited

Loading

Nit: I don't think this comment is necessary. Adding 'message' to the skips already tells us there is something to fix.

Contributor Author

simonrw Feb 2, 2026

It's more about the reason why this is skipped, which is that we output a value extra to AWS.

tests/aws/services/cloudformation/engine/test_update_policy.py

+              @markers.aws.validated
+              @markers.snapshot.skip_snapshot_verify(
+                  paths=[

Member

pinzon Jan 23, 2026 •

edited

Loading

Change: Same here. we should use a Regex transform even if the stack events are not in parity.

tests/aws/templates/deletion_policy.yaml

+              Resources:
+                MyParameter:
+                  Type: AWS::SSM::Parameter
+                  DeletionPolicy: !If [IsPermanentEnvironment, Retain, Delete]

Member

pinzon Jan 23, 2026 •

edited

Loading

Change: It hides a bit what we're actually testing.

Contributor Author

simonrw Feb 2, 2026

What is the alternative? AWS::NoValue? I don't think that's much clearer. Or do you mean creating more templates?

Member

pinzon Feb 3, 2026

My idea here was to have a different test for deletion policy behavior and support for intrinsic functions. But @nik-localstack recently closed his pr about the later. So let's ignore my comment here.

localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py

Comment on lines +1291 to +1303

+                      deletion_policy_before = Nothing
+                      deletion_policy_after = Nothing
+                      if not is_nothing(node_resource.deletion_policy):
+                          deletion_policy_delta = self.visit(node_resource.deletion_policy)
+                          deletion_policy_before = deletion_policy_delta.before
+                          deletion_policy_after = deletion_policy_delta.after
+                      update_replace_policy_before = Nothing
+                      update_replace_policy_after = Nothing
+                      if not is_nothing(node_resource.update_replace_policy):
+                          update_replace_policy_delta = self.visit(node_resource.update_replace_policy)
+                          update_replace_policy_before = update_replace_policy_delta.before
+                          update_replace_policy_after = update_replace_policy_delta.after

Member

pinzon Jan 23, 2026

Nit: IMO, extracting the delta values here is unnecessary, you could just get the deltas here, and use the .before and .after when needed.

Contributor Author

simonrw Feb 2, 2026

Unfortunately if the node_resource.*_policy is nothing then we get the 'NothingType' object has no attribute 'after' error.

localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_executor.py Show resolved Hide resolved

simonrw modified the milestones: 4.13, Playground

pinzon mentioned this pull request

CloudFormation: Support intrinsic functions in DeletionPolicy and UpdateReplacePolicy #13679

Closed

simonrw force-pushed the cfn/handle-policies branch from e78199a to fd6db3a Compare

February 2, 2026 22:54

simonrw requested a review from pinzon

February 2, 2026 23:06

pinzon approved these changes

View reviewed changes

simonrw added 6 commits

February 5, 2026 22:19


          CFn: implement DeletionPolicy and UpdateReplacePolicy

af9d3bf

# Motivation

A user has reported that setting policies is not only not implemented by our engigne, but actually incorrectly raises an error.

# Changes

* Don't raise error when modeling the policy fields
* Skip deletion if `UpdateReplacePolicy=Retain` and a resource is updated
* Skip deletion if `DeletionPolicy=Retain` and a resource is deleted


# Related to

Resolves ENG-200


          Implement update replace policy


          Handle default deletion/update policy values

b90b878


          Snapshot error message

e03ecdd


          Cleanup retained ssm parameter

78161c7


          Restructure tests

988559e

simonrw force-pushed the cfn/handle-policies branch from fd6db3a to 988559e Compare

February 5, 2026 22:21

simonrw merged commit d32e212 into main

42 checks passed

simonrw deleted the cfn/handle-policies branch

February 5, 2026 23:58

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

docs: skip notes: skip semver: patch