From bd5ab532417133ce0a6735e51f36a7828925b12c Mon Sep 17 00:00:00 2001 From: Timothy Klopotoski Date: Thu, 19 Aug 2021 12:36:11 -0400 Subject: [PATCH] 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 | 2 +- moto/cloudformation/parsing.py | 73 ++++++----- .../test_cloudformation_stack_crud_boto3.py | 124 +++++++++++++++++- 3 files changed, 163 insertions(+), 36 deletions(-) diff --git a/moto/cloudformation/models.py b/moto/cloudformation/models.py index 61650513e..808ea6a7b 100644 --- a/moto/cloudformation/models.py +++ b/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(): diff --git a/moto/cloudformation/parsing.py b/moto/cloudformation/parsing.py index 46c6e154b..6228ade67 100644 --- a/moto/cloudformation/parsing.py +++ b/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): - if parameters: - self.input_parameters = parameters - self.load_mapping() - self.load_parameters() - self.load_conditions() + def build_resource_diff(self, other_template): - old_template = self._resource_json_map - new_template = template["Resources"] + old = self._resource_json_map + 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 - for name in new_template - if name in old_template and new_template[name] != old_template[name] + name for name in new if name in old and new[name] != old[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 - new_template = template["Resources"] - self._resource_json_map = new_template + resource_names_by_action = self.build_resource_diff(template) - for resource_name, resource in resources_by_action["Add"].items(): - resource_json = new_template[resource_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] + for logical_name in resource_names_by_action["Remove"]: + resource_json = self._resource_json_map[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: - for logical_name, _ in resources_by_action["Modify"].copy().items(): - resource_json = new_template[logical_name] + while resource_names_by_action["Modify"] and tries < 5: + for logical_name in resource_names_by_action["Modify"].copy(): + 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 diff --git a/tests/test_cloudformation/test_cloudformation_stack_crud_boto3.py b/tests/test_cloudformation/test_cloudformation_stack_crud_boto3.py index 9db754ca3..0ffb71fdc 100644 --- a/tests/test_cloudformation/test_cloudformation_stack_crud_boto3.py +++ b/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()