Repository: getmoto/moto (python)

Issue:
CloudFormation Stack parameters don't update when UsePreviousValue is set to False
Moto is not updating CloudFormation Stack parameters via `CloudFormation.Client.update_stack(**kwargs)` when `UsePreviousValue=False` on the parameter.

The AWS documentation states the following. Testing with CloudFormation itself proves that setting `UsePreviousValue=False` on the parameter will then take the provided value in `ParameterValue`.

> UsePreviousValue (boolean) –
> 
> During a stack update, use the existing parameter value that the stack is using for a given parameter key. If you specify true, do not specify a parameter value.

I updated the test `test_update_stack_with_previous_value` to verify I would see the same behaviour within moto itself.

```diff
--- a/tests/test_cloudformation/test_cloudformation_stack_crud_boto3.py
+++ b/tests/test_cloudformation/test_cloudformation_stack_crud_boto3.py
@@ -1377,7 +1377,7 @@ def test_update_stack_with_previous_value():
         UsePreviousTemplate=True,
         Parameters=[
             {"ParameterKey": "TagName", "UsePreviousValue": True},
-            {"ParameterKey": "TagDescription", "ParameterValue": "not bar"},
+            {"ParameterKey": "TagDescription", "ParameterValue": "not bar", "UsePreviousValue": False},
         ],
     )
     stack = cf.describe_stacks(StackName=name)["Stacks"][0]
```

```text
========================================================================================================================================================= FAILURES ==========================================================================================================================================================
___________________________________________________________________________________________________________________________________________ test_update_stack_with_previous_value ___________________________________________________________________________________________________________________________________________

    @mock_aws
    def test_update_stack_with_previous_value():
        name = "update_stack_with_previous_value"
        cf = boto3.client("cloudformation", region_name=REGION_NAME)
        cf.create_stack(
            StackName=name,
            TemplateBody=dummy_template_yaml_with_ref,
            Parameters=[
                {"ParameterKey": "TagName", "ParameterValue": "foo"},
                {"ParameterKey": "TagDescription", "ParameterValue": "bar"},
            ],
        )
        cf.update_stack(
            StackName=name,
            UsePreviousTemplate=True,
            Parameters=[
                {"ParameterKey": "TagName", "UsePreviousValue": True},
                {"ParameterKey": "TagDescription", "ParameterValue": "not bar", "UsePreviousValue": False},
            ],
        )
        stack = cf.describe_stacks(StackName=name)["Stacks"][0]
        tag_name = [
            x["ParameterValue"]
            for x in stack["Parameters"]
            if x["ParameterKey"] == "TagName"
        ][0]
        tag_desc = [
            x["ParameterValue"]
            for x in stack["Parameters"]
            if x["ParameterKey"] == "TagDescription"
        ][0]
        assert tag_name == "foo"
>       assert tag_desc == "not bar"
E       AssertionError: assert 'bar' == 'not bar'
E         - not bar
E         + bar
```


I ran this against the latest commit of moto at time of writing, c8136bf22.

Pull request:
CloudFormation: update_stack() - Improve UsePreviousValue behaviour
Fixes #7855 

CloudFormation: update_stack() now persists the new parameters provided
CloudFormation: update_stack() now understands UsePreviousValue=False
CloudFormation: update_stack() now throws an exception when using UsePreviousValue=True and a new parameter value
CloudFormation: update_stack() is now able to update resources where only the parameters have changed

CloudFormation: The `AWS::S3::Bucket` resource will now create/update Tags when provided
CloudFormation: The `AWS::S3::Bucket` resource is no longer recreated for every update

Patch:
diff --git a/moto/cloudformation/models.py b/moto/cloudformation/models.py
index f9f8a2c2d..6d5c6bee5 100644
--- a/moto/cloudformation/models.py
+++ b/moto/cloudformation/models.py
@@ -536,6 +536,8 @@ class FakeStack(CloudFormationModel):
         self._add_stack_event("UPDATE_COMPLETE")
         self.status = "UPDATE_COMPLETE"
         self.role_arn = role_arn
+        if parameters:
+            self.parameters = parameters
         # only overwrite tags if passed
         if tags is not None:
             self.tags = tags
@@ -790,7 +792,7 @@ class CloudFormationBackend(BaseBackend):
                     instance.parameters[parameter["parameter_key"]],
                 )
                 for parameter in incoming_params
-                if "use_previous_value" in parameter
+                if parameter.get("use_previous_value") == "true"
             ]
         )
         parameters.update(previous)
diff --git a/moto/cloudformation/parsing.py b/moto/cloudformation/parsing.py
index 0749748ef..d0e5e3706 100644
--- a/moto/cloudformation/parsing.py
+++ b/moto/cloudformation/parsing.py
@@ -125,10 +125,9 @@ class LazyDict(Dict[str, Any]):
 
 def clean_json(resource_json: Any, resources_map: "ResourceMap") -> Any:
     """
-    Cleanup the a resource dict. For now, this just means replacing any Ref node
-    with the corresponding physical_resource_id.
-
-    Eventually, this is where we would add things like function parsing (fn::)
+    Cleanup a resource dict. This includes:
+     - replacing any Ref node
+     - Parsing functions: FindInMap, GetAtt, If, Join, Split, Select, Sub, ImportValue, GetAZs, ToJsonString
     """
     if isinstance(resource_json, dict):
         if "Ref" in resource_json:
@@ -498,6 +497,20 @@ def parse_output(
     return output
 
 
+def get_references_from_template(template: Dict[str, Any]) -> List[str]:
+    references = []
+    if isinstance(template, dict):
+        for key in template:
+            if key == "Ref":
+                references.append(template[key])
+            else:
+                references.extend(get_references_from_template(template[key]))
+    if isinstance(template, list):
+        for item in template:
+            references.extend(get_references_from_template(item))
+    return references
+
+
 class ResourceMap(collections_abc.Mapping):  # type: ignore[type-arg]
     """
     This is a lazy loading map for resources. This allows us to create resources
@@ -759,7 +772,11 @@ class ResourceMap(collections_abc.Mapping):  # type: ignore[type-arg]
                 all_resources_ready = False
         return all_resources_ready
 
-    def build_resource_diff(self, other_template: Dict[str, Any]) -> Dict[str, Any]:
+    def build_resource_diff(
+        self,
+        other_template: Dict[str, Any],
+        parameters: Optional[Dict[str, Any]] = None,
+    ) -> Dict[str, Any]:
         old = self._resource_json_map
         new = other_template["Resources"]
 
@@ -768,6 +785,18 @@ class ResourceMap(collections_abc.Mapping):  # type: ignore[type-arg]
             "Modify": [name for name in new if name in old and new[name] != old[name]],
             "Remove": [name for name in old if name not in new],
         }
+        # Modify resources when only parameters are different
+        if parameters:
+            changed_parameters = [
+                p for p in parameters if self.input_parameters.get(p) != parameters[p]
+            ]
+            for name in new:
+                # Only check for modified parameters if the template is exactly the same
+                # If the template is different, it will already be part of the Modify-collection
+                if name in old and new[name] == old[name]:
+                    template_parameters = get_references_from_template(new[name])
+                    if any(c for c in changed_parameters if c in template_parameters):
+                        resource_names_by_action["Modify"].append(name)
 
         return resource_names_by_action
 
@@ -805,7 +834,7 @@ class ResourceMap(collections_abc.Mapping):  # type: ignore[type-arg]
     def update(
         self, template: Dict[str, Any], parameters: Optional[Dict[str, Any]] = None
     ) -> None:
-        resource_names_by_action = self.build_resource_diff(template)
+        resource_names_by_action = self.build_resource_diff(template, parameters)
 
         for logical_name in resource_names_by_action["Remove"]:
             resource_json = self._resource_json_map[logical_name]
diff --git a/moto/cloudformation/responses.py b/moto/cloudformation/responses.py
index 66983ba65..c1a81441c 100644
--- a/moto/cloudformation/responses.py
+++ b/moto/cloudformation/responses.py
@@ -367,6 +367,13 @@ class CloudFormationResponse(BaseResponse):
             stack_body = self._get_stack_from_s3_url(template_url)
 
         incoming_params = self._get_list_prefix("Parameters.member")
+        for param in incoming_params:
+            if param.get("use_previous_value") == "true" and param.get(
+                "parameter_value"
+            ):
+                raise ValidationError(
+                    message=f"Invalid input for parameter key {param['parameter_key']}. Cannot specify usePreviousValue as true and non empty value for a parameter"
+                )
         # boto3 is supposed to let you clear the tags by passing an empty value, but the request body doesn't
         # end up containing anything we can use to differentiate between passing an empty value versus not
         # passing anything. so until that changes, moto won't be able to clear tags, only update them.
diff --git a/moto/s3/cloud_formation.py b/moto/s3/cloud_formation.py
index e295623e4..8f5735c3e 100644
--- a/moto/s3/cloud_formation.py
+++ b/moto/s3/cloud_formation.py
@@ -1,5 +1,5 @@
 from collections import OrderedDict
-from typing import Any, Dict, List
+from typing import Any, Dict
 
 
 def cfn_to_api_encryption(
@@ -21,13 +21,3 @@ def cfn_to_api_encryption(
     return OrderedDict(
         {"@xmlns": "http://s3.amazonaws.com/doc/2006-03-01/", "Rule": rule}
     )
-
-
-def is_replacement_update(properties: List[str]) -> bool:
-    properties_requiring_replacement_update = ["BucketName", "ObjectLockEnabled"]
-    return any(
-        [
-            property_requiring_replacement in properties
-            for property_requiring_replacement in properties_requiring_replacement_update
-        ]
-    )
diff --git a/moto/s3/models.py b/moto/s3/models.py
index 0256da5f8..155261282 100644
--- a/moto/s3/models.py
+++ b/moto/s3/models.py
@@ -71,7 +71,7 @@ from ..settings import (
     s3_allow_crossdomain_access,
 )
 from . import notifications
-from .cloud_formation import cfn_to_api_encryption, is_replacement_update
+from .cloud_formation import cfn_to_api_encryption
 from .select_object_content import parse_query
 from .utils import (
     ARCHIVE_STORAGE_CLASSES,
@@ -1478,6 +1478,9 @@ class FakeBucket(CloudFormationModel):
             backend.put_bucket_encryption(
                 bucket_name=resource_name, encryption=bucket_encryption
             )
+        if "Tags" in properties:
+            tags = TaggingService().flatten_tag_list(properties["Tags"])
+            backend.put_bucket_tagging(bucket_name=resource_name, tags=tags)  # type: ignore
 
         return bucket
 
@@ -1491,8 +1494,15 @@ class FakeBucket(CloudFormationModel):
         region_name: str,
     ) -> "FakeBucket":
         properties = cloudformation_json["Properties"]
+        backend = s3_backends[account_id][get_partition(region_name)]
+        # BucketName is not mandatory - a random name will be autogenerated if not supplied
+        # Only if we specify the BucketName *and* it has changed should we replace the bucket
+        requires_replacement = (
+            "BucketName" in properties
+            and properties["BucketName"] != original_resource.name
+        )
 
-        if is_replacement_update(properties):
+        if requires_replacement:
             resource_name_property = cls.cloudformation_name_type()
             if resource_name_property not in properties:
                 properties[resource_name_property] = new_resource_name
@@ -1513,11 +1523,15 @@ class FakeBucket(CloudFormationModel):
                 bucket_encryption = cfn_to_api_encryption(
                     properties["BucketEncryption"]
                 )
-                s3_backends[account_id][
-                    get_partition(region_name)
-                ].put_bucket_encryption(
+                backend.put_bucket_encryption(
                     bucket_name=original_resource.name, encryption=bucket_encryption
                 )
+            if "Tags" in properties:
+                tags = TaggingService().flatten_tag_list(properties["Tags"])
+                backend.put_bucket_tagging(
+                    bucket_name=properties["BucketName"],
+                    tags=tags,  # type: ignore
+                )
             return original_resource
 
     @classmethod


Test patch:
diff --git a/tests/test_cloudformation/test_cloudformation_stack_crud_boto3.py b/tests/test_cloudformation/test_cloudformation_stack_crud_boto3.py
index e5c8ff28e..cc9fa776e 100644
--- a/tests/test_cloudformation/test_cloudformation_stack_crud_boto3.py
+++ b/tests/test_cloudformation/test_cloudformation_stack_crud_boto3.py
@@ -2,6 +2,7 @@ import copy
 import json
 import os
 import sys
+import uuid
 from collections import OrderedDict
 from datetime import datetime, timedelta, timezone
 from unittest import SkipTest
@@ -15,6 +16,7 @@ from moto.cloudformation import cloudformation_backends
 from moto.core import DEFAULT_ACCOUNT_ID as ACCOUNT_ID
 from moto.utilities.distutils_version import LooseVersion
 from tests import EXAMPLE_AMI_ID
+from tests.test_cloudformation import cloudformation_aws_verified
 
 TEST_STACK_NAME = "test_stack"
 REGION_NAME = "us-east-1"
@@ -202,9 +204,19 @@ dummy_empty_template = {
 dummy_parametrized_template = {
     "AWSTemplateFormatVersion": "2010-09-09",
     "Parameters": {
-        "KeyName": {"Description": "A template parameter", "Type": "String"}
+        "BucketName": {"Type": "String"},
+        "KeyName": {"Description": "A template parameter", "Type": "String"},
+        "KeyDesc": {"Description": "Param 2", "Type": "String"},
+    },
+    "Resources": {
+        "Bucket": {
+            "Type": "AWS::S3::Bucket",
+            "Properties": {
+                "BucketName": {"Ref": "BucketName"},
+                "Tags": [{"Key": {"Ref": "KeyName"}, "Value": {"Ref": "KeyDesc"}}],
+            },
+        }
     },
-    "Resources": {},
 }
 
 dummy_update_template = {
@@ -971,7 +983,7 @@ def test_describe_stack_set_by_id():
 def test_create_stack_fail_missing_parameter():
     cf = boto3.client("cloudformation", region_name=REGION_NAME)
 
-    with pytest.raises(ClientError, match="Missing parameter KeyName"):
+    with pytest.raises(ClientError, match="Missing parameter BucketName"):
         cf.create_stack(StackName="ts", TemplateBody=dummy_parametrized_template_json)
 
 
@@ -1258,7 +1270,7 @@ def test_update_stack_fail_missing_new_parameter():
 
     cf.create_stack(StackName=name, TemplateBody=dummy_empty_template_json)
 
-    with pytest.raises(ClientError, match="Missing parameter KeyName"):
+    with pytest.raises(ClientError, match="Missing parameter BucketName"):
         cf.update_stack(StackName=name, TemplateBody=dummy_parametrized_template_json)
 
 
@@ -1360,39 +1372,111 @@ def test_update_stack_deleted_resources_can_reference_deleted_resources():
     assert len(response["StackResources"]) == 0
 
 
-@mock_aws
+@pytest.mark.aws_verified
+@cloudformation_aws_verified()
 def test_update_stack_with_previous_value():
-    name = "update_stack_with_previous_value"
+    name = f"update-stack-with-previous-value-{str(uuid.uuid4())[0:6]}"
+    bucket_name = str(uuid.uuid4())
     cf = boto3.client("cloudformation", region_name=REGION_NAME)
+    s3 = boto3.client("s3", REGION_NAME)
     cf.create_stack(
         StackName=name,
-        TemplateBody=dummy_template_yaml_with_ref,
+        TemplateBody=dummy_parametrized_template_json,
         Parameters=[
-            {"ParameterKey": "TagName", "ParameterValue": "foo"},
-            {"ParameterKey": "TagDescription", "ParameterValue": "bar"},
-        ],
-    )
-    cf.update_stack(
-        StackName=name,
-        UsePreviousTemplate=True,
-        Parameters=[
-            {"ParameterKey": "TagName", "UsePreviousValue": True},
-            {"ParameterKey": "TagDescription", "ParameterValue": "not bar"},
+            {"ParameterKey": "BucketName", "ParameterValue": bucket_name},
+            {"ParameterKey": "KeyName", "ParameterValue": "foo"},
+            {"ParameterKey": "KeyDesc", "ParameterValue": "bar"},
         ],
     )
+    waiter = cf.get_waiter("stack_create_complete")
+    waiter.wait(StackName=name)
+
+    # Verify that correct parameters are used
     stack = cf.describe_stacks(StackName=name)["Stacks"][0]
-    tag_name = [
-        x["ParameterValue"]
-        for x in stack["Parameters"]
-        if x["ParameterKey"] == "TagName"
-    ][0]
-    tag_desc = [
-        x["ParameterValue"]
-        for x in stack["Parameters"]
-        if x["ParameterKey"] == "TagDescription"
-    ][0]
-    assert tag_name == "foo"
-    assert tag_desc == "not bar"
+    assert {"ParameterKey": "KeyName", "ParameterValue": "foo"} in stack["Parameters"]
+    assert {"ParameterKey": "KeyDesc", "ParameterValue": "bar"} in stack["Parameters"]
+
+    # Verify that correct parameters are applied
+    tags = s3.get_bucket_tagging(Bucket=bucket_name)["TagSet"]
+    assert {"Key": "foo", "Value": "bar"} in tags
+
+    try:
+        cf.update_stack(
+            StackName=name,
+            UsePreviousTemplate=True,
+            Parameters=[
+                {"ParameterKey": "BucketName", "UsePreviousValue": True},
+                {"ParameterKey": "KeyName", "UsePreviousValue": True},
+                {"ParameterKey": "KeyDesc", "ParameterValue": "not bar"},
+            ],
+        )
+        waiter = cf.get_waiter("stack_update_complete")
+        waiter.wait(StackName=name)
+
+        # Verify that correct parameters are used
+        params = cf.describe_stacks(StackName=name)["Stacks"][0]["Parameters"]
+        assert {"ParameterKey": "KeyName", "ParameterValue": "foo"} in params
+        assert {"ParameterKey": "KeyDesc", "ParameterValue": "not bar"} in params
+
+        # Verify that correct parameters are applied
+        tags = s3.get_bucket_tagging(Bucket=bucket_name)["TagSet"]
+        assert {"Key": "foo", "Value": "not bar"} in tags
+
+        # Update Stack - Can't specify ParameterValue and UsePreviousValue
+        with pytest.raises(ClientError) as exc:
+            cf.update_stack(
+                StackName=name,
+                UsePreviousTemplate=True,
+                Parameters=[
+                    {"ParameterKey": "BucketName", "UsePreviousValue": True},
+                    {"ParameterKey": "KeyName", "ParameterValue": "foo2"},
+                    {
+                        "ParameterKey": "KeyDesc",
+                        "UsePreviousValue": True,
+                        "ParameterValue": "n/a",
+                    },
+                ],
+            )
+        err = exc.value.response["Error"]
+        assert err["Code"] == "ValidationError"
+        assert (
+            err["Message"]
+            == "Invalid input for parameter key KeyDesc. Cannot specify usePreviousValue as true and non empty value for a parameter"
+        )
+
+        # UsePreviousValue=True can be used if value is empty
+        # UsePreviousValue=False just means - use the new value
+        cf.update_stack(
+            StackName=name,
+            UsePreviousTemplate=True,
+            Parameters=[
+                {"ParameterKey": "BucketName", "UsePreviousValue": True},
+                {
+                    "ParameterKey": "KeyName",
+                    "ParameterValue": "foo2",
+                    "UsePreviousValue": False,
+                },
+                {
+                    "ParameterKey": "KeyDesc",
+                    "UsePreviousValue": True,
+                    "ParameterValue": "",
+                },
+            ],
+        )
+        waiter = cf.get_waiter("stack_update_complete")
+        waiter.wait(StackName=name)
+
+        # Verify that correct parameters are used
+        params = cf.describe_stacks(StackName=name)["Stacks"][0]["Parameters"]
+        assert {"ParameterKey": "KeyName", "ParameterValue": "foo2"} in params
+        assert {"ParameterKey": "KeyDesc", "ParameterValue": "not bar"} in params
+
+        # Verify that correct parameters are applied
+        tags = s3.get_bucket_tagging(Bucket=bucket_name)["TagSet"]
+        assert {"Key": "foo2", "Value": "not bar"} in tags
+
+    finally:
+        cf.delete_stack(StackName=name)
 
 
 @mock_aws
diff --git a/tests/test_cloudformation/test_utilities.py b/tests/test_cloudformation/test_utilities.py
new file mode 100644
index 000000000..e64c65739
--- /dev/null
+++ b/tests/test_cloudformation/test_utilities.py
@@ -0,0 +1,32 @@
+from moto.cloudformation.parsing import get_references_from_template
+
+
+class TestParametersFromResource:
+    def test_no_properties(self):
+        template = {"Type": "AWS::S3::Bucket"}
+        assert get_references_from_template(template) == []
+
+    def test_properties_without_param(self):
+        template = {"Type": "AWS::S3::Bucket", "Properties": {"BucketName": "mybucket"}}
+        assert get_references_from_template(template) == []
+
+    def test_properties_with_param(self):
+        template = {
+            "Type": "AWS::S3::Bucket",
+            "Properties": {
+                "BucketName": {"Ref": "BucketName"},
+                "Tags": [{"Key": {"Ref": "KeyName"}, "Value": {"Ref": "KeyDesc"}}],
+            },
+        }
+        assert get_references_from_template(template) == [
+            "BucketName",
+            "KeyName",
+            "KeyDesc",
+        ]
+
+    def test_nested_properties_with_param(self):
+        template = {
+            "Type": "AWS::S3::Bucket",
+            "Properties": {"L1": {"L2": {"Ref": "Name"}}},
+        }
+        assert get_references_from_template(template) == ["Name"]