Fix bugs. Cloudformation updates are complex! (#4200)

* **Fix bug.** If a cloudformation stack is updated with a new
    parameter, that parameter should be honored. Several unit tests
    had bugs where they were not providing parameters required by the template.
* **Fix bug.** Do not update stack parameters until after deleting removed
    resources, so that any references to removed parameters can be resolved.
* **Fix bug.** Per the API, creation of a change set should not modify
    a stack. The `diff` method, called in the creation of a
    FakeChangeSet, was mutating the resource map which was problematic

moto/cloudformation/models.py

@@ -402,7 +402,7 @@ class FakeChangeSet(BaseModel):
 
     def diff(self):
         changes = []
-        resources_by_action = self.stack.resource_map.diff(
+        resources_by_action = self.stack.resource_map.build_change_set_actions(
             self.template_dict, self.parameters
         )
         for action, resources in resources_by_action.items():

moto/cloudformation/parsing.py

@@ -606,63 +606,55 @@ class ResourceMap(collections_abc.Mapping):
                     [self[resource].physical_resource_id], self.tags
                 )
 
-    def diff(self, template, parameters=None):
+    def build_resource_diff(self, other_template):
-        if parameters:
-            self.input_parameters = parameters
-        self.load_mapping()
-        self.load_parameters()
-        self.load_conditions()
 
-        old_template = self._resource_json_map
+        old = self._resource_json_map
-        new_template = template["Resources"]
+        new = other_template["Resources"]
+
+        resource_names_by_action = {"Add": {}, "Modify": {}, "Remove": {}}
 
         resource_names_by_action = {
-            "Add": set(new_template) - set(old_template),
+            "Add": set(new) - set(old),
             "Modify": set(
-                name
+                name for name in new if name in old and new[name] != old[name]
-                for name in new_template
-                if name in old_template and new_template[name] != old_template[name]
             ),
-            "Remove": set(old_template) - set(new_template),
+            "Remove": set(old) - set(new),
         }
+
+        return resource_names_by_action
+
+    def build_change_set_actions(self, template, parameters):
+
+        resource_names_by_action = self.build_resource_diff(template)
+
         resources_by_action = {"Add": {}, "Modify": {}, "Remove": {}}
 
         for resource_name in resource_names_by_action["Add"]:
             resources_by_action["Add"][resource_name] = {
                 "LogicalResourceId": resource_name,
-                "ResourceType": new_template[resource_name]["Type"],
+                "ResourceType": template["Resources"][resource_name]["Type"],
             }
 
         for resource_name in resource_names_by_action["Modify"]:
             resources_by_action["Modify"][resource_name] = {
                 "LogicalResourceId": resource_name,
-                "ResourceType": new_template[resource_name]["Type"],
+                "ResourceType": template["Resources"][resource_name]["Type"],
             }
 
         for resource_name in resource_names_by_action["Remove"]:
             resources_by_action["Remove"][resource_name] = {
                 "LogicalResourceId": resource_name,
-                "ResourceType": old_template[resource_name]["Type"],
+                "ResourceType": self._resource_json_map[resource_name]["Type"],
             }
 
         return resources_by_action
 
     def update(self, template, parameters=None):
-        resources_by_action = self.diff(template, parameters)
 
-        old_template = self._resource_json_map
+        resource_names_by_action = self.build_resource_diff(template)
-        new_template = template["Resources"]
-        self._resource_json_map = new_template
 
-        for resource_name, resource in resources_by_action["Add"].items():
+        for logical_name in resource_names_by_action["Remove"]:
-            resource_json = new_template[resource_name]
+            resource_json = self._resource_json_map[logical_name]
-            new_resource = parse_and_create_resource(
-                resource_name, resource_json, self, self._region_name
-            )
-            self._parsed_resources[resource_name] = new_resource
-
-        for logical_name, _ in resources_by_action["Remove"].items():
-            resource_json = old_template[logical_name]
             resource = self._parsed_resources[logical_name]
             # ToDo: Standardize this.
             if hasattr(resource, "physical_resource_id"):
@@ -676,10 +668,25 @@ class ResourceMap(collections_abc.Mapping):
             )
             self._parsed_resources.pop(logical_name)
 
+        self._template = template
+        if parameters:
+            self.input_parameters = parameters
+        self.load_mapping()
+        self.load_parameters()
+        self.load_conditions()
+
+        self._resource_json_map = template["Resources"]
+
+        for logical_name in resource_names_by_action["Add"]:
+
+            # call __getitem__ to initialize the resource
+            # TODO: usage of indexer to initalize the resource is questionable
+            _ = self[logical_name]
+
         tries = 1
-        while resources_by_action["Modify"] and tries < 5:
+        while resource_names_by_action["Modify"] and tries < 5:
-            for logical_name, _ in resources_by_action["Modify"].copy().items():
+            for logical_name in resource_names_by_action["Modify"].copy():
-                resource_json = new_template[logical_name]
+                resource_json = self._resource_json_map[logical_name]
                 try:
                     changed_resource = parse_and_update_resource(
                         logical_name, resource_json, self, self._region_name
@@ -690,7 +697,7 @@ class ResourceMap(collections_abc.Mapping):
                     last_exception = e
                 else:
                     self._parsed_resources[logical_name] = changed_resource
-                    del resources_by_action["Modify"][logical_name]
+                    resource_names_by_action["Modify"].remove(logical_name)
             tries += 1
         if tries == 5:
             raise last_exception

tests/test_cloudformation/test_cloudformation_stack_crud_boto3.py

@@ -101,6 +101,20 @@ Resources:
           Value: !Ref TagName
 """
 
+dummy_empty_template = {
+    "AWSTemplateFormatVersion": "2010-09-09",
+    "Parameters": {},
+    "Resources": {},
+}
+
+dummy_parametrized_template = {
+    "AWSTemplateFormatVersion": "2010-09-09",
+    "Parameters": {
+        "KeyName": {"Description": "A template parameter", "Type": "String"}
+    },
+    "Resources": {},
+}
+
 dummy_update_template = {
     "AWSTemplateFormatVersion": "2010-09-09",
     "Parameters": {
@@ -194,6 +208,8 @@ dummy_template_json = json.dumps(dummy_template)
 dummy_template_special_chars_in_description_json = json.dumps(
     dummy_template_special_chars_in_description
 )
+dummy_empty_template_json = json.dumps(dummy_empty_template)
+dummy_parametrized_template_json = json.dumps(dummy_parametrized_template)
 dummy_update_template_json = json.dumps(dummy_update_template)
 dummy_output_template_json = json.dumps(dummy_output_template)
 dummy_import_template_json = json.dumps(dummy_import_template)
@@ -620,6 +636,7 @@ def test_boto3_list_stack_set_operations():
 @mock_cloudformation
 def test_boto3_bad_list_stack_resources():
     cf_conn = boto3.client("cloudformation", region_name="us-east-1")
+
     with pytest.raises(ClientError):
         cf_conn.list_stack_resources(StackName="test_stack_set")
 
@@ -754,6 +771,17 @@ def test_boto3_create_stack():
     )
 
 
+@mock_cloudformation
+def test_boto3_create_stack_fail_missing_parameter():
+    cf_conn = boto3.client("cloudformation", region_name="us-east-1")
+
+    with pytest.raises(ClientError, match="Missing parameter KeyName"):
+
+        cf_conn.create_stack(
+            StackName="test_stack", TemplateBody=dummy_parametrized_template_json
+        )
+
+
 @mock_cloudformation
 def test_boto3_create_stack_s3_long_name():
     cf_conn = boto3.client("cloudformation", region_name="us-east-1")
@@ -918,6 +946,88 @@ def test_create_stack_from_s3_url():
     )
 
 
+@mock_cloudformation
+def test_boto3_update_stack_fail_missing_new_parameter():
+
+    name = "update_stack_fail_missing_new_parameter"
+
+    cf_conn = boto3.client("cloudformation", region_name="us-east-1")
+
+    cf_conn.create_stack(StackName=name, TemplateBody=dummy_empty_template_json)
+
+    with pytest.raises(ClientError, match="Missing parameter KeyName"):
+
+        cf_conn.update_stack(
+            StackName=name, TemplateBody=dummy_parametrized_template_json
+        )
+
+
+@mock_cloudformation
+def test_boto3_update_stack_deleted_resources_can_reference_deleted_parameters():
+
+    name = "update_stack_deleted_resources_can_reference_deleted_parameters"
+
+    cf_conn = boto3.client("cloudformation", region_name="us-east-1")
+
+    template_json = json.dumps(
+        {
+            "AWSTemplateFormatVersion": "2010-09-09",
+            "Parameters": {"TimeoutParameter": {"Default": 61, "Type": "String"}},
+            "Resources": {
+                "Queue": {
+                    "Type": "AWS::SQS::Queue",
+                    "Properties": {"VisibilityTimeout": {"Ref": "TimeoutParameter"}},
+                }
+            },
+        }
+    )
+
+    cf_conn.create_stack(StackName=name, TemplateBody=template_json)
+
+    response = cf_conn.describe_stack_resources(StackName=name)
+    len(response["StackResources"]).should.equal(1)
+
+    cf_conn.update_stack(StackName=name, TemplateBody=dummy_empty_template_json)
+
+    response = cf_conn.describe_stack_resources(StackName=name)
+    len(response["StackResources"]).should.equal(0)
+
+
+@mock_cloudformation
+def test_boto3_update_stack_deleted_resources_can_reference_deleted_resources():
+
+    name = "update_stack_deleted_resources_can_reference_deleted_resources"
+
+    cf_conn = boto3.client("cloudformation", region_name="us-east-1")
+
+    template_json = json.dumps(
+        {
+            "AWSTemplateFormatVersion": "2010-09-09",
+            "Parameters": {"TimeoutParameter": {"Default": 61, "Type": "String"}},
+            "Resources": {
+                "VPC": {
+                    "Type": "AWS::EC2::VPC",
+                    "Properties": {"CidrBlock": "10.0.0.0/16"},
+                },
+                "Subnet": {
+                    "Type": "AWS::EC2::Subnet",
+                    "Properties": {"VpcId": {"Ref": "VPC"}, "CidrBlock": "10.0.0.0/24"},
+                },
+            },
+        }
+    )
+
+    cf_conn.create_stack(StackName=name, TemplateBody=template_json)
+
+    response = cf_conn.describe_stack_resources(StackName=name)
+    len(response["StackResources"]).should.equal(2)
+
+    cf_conn.update_stack(StackName=name, TemplateBody=dummy_empty_template_json)
+
+    response = cf_conn.describe_stack_resources(StackName=name)
+    len(response["StackResources"]).should.equal(0)
+
+
 @mock_cloudformation
 def test_update_stack_with_previous_value():
     name = "update_stack_with_previous_value"
@@ -974,7 +1084,11 @@ def test_update_stack_from_s3_url():
         ClientMethod="get_object", Params={"Bucket": "foobar", "Key": "template-key"}
     )
 
-    cf_conn.update_stack(StackName="update_stack_from_url", TemplateURL=key_url)
+    cf_conn.update_stack(
+        StackName="update_stack_from_url",
+        TemplateURL=key_url,
+        Parameters=[{"ParameterKey": "KeyName", "ParameterValue": "value"}],
+    )
 
     cf_conn.get_template(StackName="update_stack_from_url")[
         "TemplateBody"
@@ -1067,7 +1181,9 @@ def test_describe_change_set():
         TemplateBody=dummy_update_template_json,
         ChangeSetName="NewChangeSet2",
         ChangeSetType="UPDATE",
+        Parameters=[{"ParameterKey": "KeyName", "ParameterValue": "value"}],
     )
+
     stack = cf_conn.describe_change_set(ChangeSetName="NewChangeSet2")
     stack["ChangeSetName"].should.equal("NewChangeSet2")
     stack["StackName"].should.equal("NewStack")
@@ -1336,6 +1452,7 @@ def test_describe_updated_stack():
         RoleARN="arn:aws:iam::{}:role/moto".format(ACCOUNT_ID),
         TemplateBody=dummy_update_template_json,
         Tags=[{"Key": "foo", "Value": "baz"}],
+        Parameters=[{"ParameterKey": "KeyName", "ParameterValue": "value"}],
     )
 
     stack = cf_conn.describe_stacks(StackName="test_stack")["Stacks"][0]
@@ -1405,7 +1522,10 @@ def test_stack_tags():
 def test_stack_events():
     cf = boto3.resource("cloudformation", region_name="us-east-1")
     stack = cf.create_stack(StackName="test_stack", TemplateBody=dummy_template_json)
-    stack.update(TemplateBody=dummy_update_template_json)
+    stack.update(
+        TemplateBody=dummy_update_template_json,
+        Parameters=[{"ParameterKey": "KeyName", "ParameterValue": "value"}],
+    )
     stack = cf.Stack(stack.stack_id)
     stack.delete()