From 233758100f621eeb8ea0d1db1938a88043477850 Mon Sep 17 00:00:00 2001 From: Arthur Boghossian Date: Fri, 11 Aug 2023 16:58:46 -0700 Subject: [PATCH 1/7] Allow "aws cloudformation package" to succeed when a Resource is not a key-value pair --- .../customizations/cloudformation/artifact_exporter.py | 5 ++++- .../cloudformation/test_artifact_exporter.py | 9 ++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 9bb150660c02..4064271dcc9d 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -232,7 +232,7 @@ def __init__(self, uploader): self.uploader = uploader def export(self, resource_id, resource_dict, parent_dir): - if resource_dict is None: + if resource_dict is None or not isinstance(resource_dict, dict): return property_value = jmespath.search(self.PROPERTY_NAME, resource_dict) @@ -661,6 +661,9 @@ def export(self): for resource_id, resource in self.template_dict["Resources"].items(): + if not isinstance(resource, dict): + continue + resource_type = resource.get("Type", None) resource_dict = resource.get("Properties", None) diff --git a/tests/unit/customizations/cloudformation/test_artifact_exporter.py b/tests/unit/customizations/cloudformation/test_artifact_exporter.py index 93df4297d660..c35a6031531c 100644 --- a/tests/unit/customizations/cloudformation/test_artifact_exporter.py +++ b/tests/unit/customizations/cloudformation/test_artifact_exporter.py @@ -986,7 +986,14 @@ def test_template_export(self, yaml_parse_mock): "Resource3": { "Type": "some-other-type", "Properties": properties - } + }, + "Fn::ForEach": [ + "Identifier", + ["Item1", "Item2"], + { + "Key${Identifier}": "Value" + } + ] } } From 307056bc3539cd95cb6316b4974ae598c4013baa Mon Sep 17 00:00:00 2001 From: Arthur Boghossian Date: Tue, 15 Aug 2023 14:52:36 -0700 Subject: [PATCH 2/7] Add warning message that local artifacts aren't supported in Resources that aren't defined as key-value pairs --- .../cloudformation/artifact_exporter.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 4064271dcc9d..f34c47c364b9 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -13,6 +13,7 @@ import logging import os +import sys import tempfile import zipfile import contextlib @@ -22,6 +23,7 @@ from botocore.utils import set_value_from_jmespath from awscli.compat import urlparse +from colorama import Fore, Style from contextlib import contextmanager from awscli.customizations.cloudformation import exceptions from awscli.customizations.cloudformation.yamlhelper import yaml_dump, \ @@ -232,7 +234,7 @@ def __init__(self, uploader): self.uploader = uploader def export(self, resource_id, resource_dict, parent_dir): - if resource_dict is None or not isinstance(resource_dict, dict): + if resource_dict is None: return property_value = jmespath.search(self.PROPERTY_NAME, resource_dict) @@ -659,9 +661,12 @@ def export(self): self.template_dict = self.export_global_artifacts(self.template_dict) + non_dict_resources = [] + for resource_id, resource in self.template_dict["Resources"].items(): if not isinstance(resource, dict): + non_dict_resources.append(resource_id) continue resource_type = resource.get("Type", None) @@ -675,4 +680,11 @@ def export(self): exporter = exporter_class(self.uploader) exporter.export(resource_id, resource_dict, self.template_dir) + if non_dict_resources: + msg = (Fore.YELLOW + + "WARNING: Ensure there are no local artifacts defined within Resources with the following logical " + f"ID's: {non_dict_resources}" + + Style.RESET_ALL) + sys.stdout.write(msg) + return self.template_dict From db8d728d370f13608034f92977e3e94616f0a5b2 Mon Sep 17 00:00:00 2001 From: Arthur Boghossian Date: Fri, 1 Sep 2023 16:43:06 -0700 Subject: [PATCH 3/7] Modify artifact_explorer to be able to handle the Fn::ForEach intrinsic function by recursively calling the export function on the fragment property of Fn::ForEach --- .../cloudformation/artifact_exporter.py | 41 +++-- .../cloudformation/test_artifact_exporter.py | 154 +++++++++++++++++- 2 files changed, 165 insertions(+), 30 deletions(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index f34c47c364b9..942dc32d04c5 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -13,7 +13,6 @@ import logging import os -import sys import tempfile import zipfile import contextlib @@ -23,7 +22,6 @@ from botocore.utils import set_value_from_jmespath from awscli.compat import urlparse -from colorama import Fore, Style from contextlib import contextmanager from awscli.customizations.cloudformation import exceptions from awscli.customizations.cloudformation.yamlhelper import yaml_dump, \ @@ -661,30 +659,29 @@ def export(self): self.template_dict = self.export_global_artifacts(self.template_dict) - non_dict_resources = [] + def export_resources(resource_dict): + for resource_id, resource in resource_dict.items(): - for resource_id, resource in self.template_dict["Resources"].items(): + if resource_id.startswith("Fn::ForEach"): + if not isinstance(resource, list) or len(resource) < 3: + raise ValueError("Fn::ForEach with name {0} has invalid format".format(resource_id)) + if isinstance(resource[2], dict): + export_resources(resource[2]) + continue - if not isinstance(resource, dict): - non_dict_resources.append(resource_id) - continue + resource_type = resource.get("Type", None) + if resource_type is None: + continue + resource_dict = resource.get("Properties", None) - resource_type = resource.get("Type", None) - resource_dict = resource.get("Properties", None) + for exporter_class in self.resources_to_export: + if exporter_class.RESOURCE_TYPE != resource_type: + continue - for exporter_class in self.resources_to_export: - if exporter_class.RESOURCE_TYPE != resource_type: - continue + # Export code resources + exporter = exporter_class(self.uploader) + exporter.export(resource_id, resource_dict, self.template_dir) - # Export code resources - exporter = exporter_class(self.uploader) - exporter.export(resource_id, resource_dict, self.template_dir) - - if non_dict_resources: - msg = (Fore.YELLOW + - "WARNING: Ensure there are no local artifacts defined within Resources with the following logical " - f"ID's: {non_dict_resources}" + - Style.RESET_ALL) - sys.stdout.write(msg) + export_resources(self.template_dict["Resources"]) return self.template_dict diff --git a/tests/unit/customizations/cloudformation/test_artifact_exporter.py b/tests/unit/customizations/cloudformation/test_artifact_exporter.py index c35a6031531c..861208cbe8ca 100644 --- a/tests/unit/customizations/cloudformation/test_artifact_exporter.py +++ b/tests/unit/customizations/cloudformation/test_artifact_exporter.py @@ -986,14 +986,7 @@ def test_template_export(self, yaml_parse_mock): "Resource3": { "Type": "some-other-type", "Properties": properties - }, - "Fn::ForEach": [ - "Identifier", - ["Item1", "Item2"], - { - "Key${Identifier}": "Value" - } - ] + } } } @@ -1023,6 +1016,151 @@ def test_template_export(self, yaml_parse_mock): resource_type2_instance.export.assert_called_once_with( "Resource2", mock.ANY, template_dir) + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") + def test_template_export_foreach_valid(self, yaml_parse_mock): + parent_dir = os.path.sep + template_dir = os.path.join(parent_dir, 'foo', 'bar') + template_path = os.path.join(template_dir, 'path') + template_str = self.example_yaml_template() + + resource_type1_class = mock.Mock() + resource_type1_class.RESOURCE_TYPE = "resource_type1" + resource_type1_instance = mock.Mock() + resource_type1_class.return_value = resource_type1_instance + resource_type2_class = mock.Mock() + resource_type2_class.RESOURCE_TYPE = "resource_type2" + resource_type2_instance = mock.Mock() + resource_type2_class.return_value = resource_type2_instance + + resources_to_export = [ + resource_type1_class, + resource_type2_class + ] + + properties = {"foo": "bar"} + template_dict = { + "Resources": { + "Resource1": { + "Type": "resource_type1", + "Properties": properties + }, + "Resource2": { + "Type": "resource_type2", + "Properties": properties + }, + "Resource3": { + "Type": "some-other-type", + "Properties": properties + }, + "Fn::ForEach::OuterLoopName": [ + "Identifier1", + ["4", "5"], + { + "Fn::ForEach::InnerLoopName": [ + "Identifier2", + ["6", "7"], + { + "Resource${Identifier1}${Identifier2}": { + "Type": "resource_type2", + "Properties": properties + } + } + ], + "Resource${Identifier1}": { + "Type": "resource_type1", + "Properties": properties + } + } + ] + } + } + + open_mock = mock.mock_open() + yaml_parse_mock.return_value = template_dict + + # Patch the file open method to return template string + with mock.patch( + "awscli.customizations.cloudformation.artifact_exporter.open", + open_mock(read_data=template_str)) as open_mock: + + template_exporter = Template( + template_path, parent_dir, self.s3_uploader_mock, + resources_to_export) + exported_template = template_exporter.export() + self.assertEqual(exported_template, template_dict) + + open_mock.assert_called_once_with( + make_abs_path(parent_dir, template_path), "r") + + self.assertEqual(1, yaml_parse_mock.call_count) + + resource_type1_class.assert_called_with(self.s3_uploader_mock) + resource_type1_instance.export.assert_called_with( + "Resource${Identifier1}", mock.ANY, template_dir) + resource_type2_class.assert_called_with(self.s3_uploader_mock) + resource_type2_instance.export.assert_called_with( + "Resource${Identifier1}${Identifier2}", mock.ANY, template_dir) + + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") + def test_template_export_foreach_invalid(self, yaml_parse_mock): + parent_dir = os.path.sep + template_dir = os.path.join(parent_dir, 'foo', 'bar') + template_path = os.path.join(template_dir, 'path') + template_str = self.example_yaml_template() + + resource_type1_class = mock.Mock() + resource_type1_class.RESOURCE_TYPE = "resource_type1" + resource_type1_instance = mock.Mock() + resource_type1_class.return_value = resource_type1_instance + resource_type2_class = mock.Mock() + resource_type2_class.RESOURCE_TYPE = "resource_type2" + resource_type2_instance = mock.Mock() + resource_type2_class.return_value = resource_type2_instance + + resources_to_export = [ + resource_type1_class, + resource_type2_class + ] + + properties = {"foo": "bar"} + template_dict = { + "Resources": { + "Resource1": { + "Type": "resource_type1", + "Properties": properties + }, + "Resource2": { + "Type": "resource_type2", + "Properties": properties + }, + "Resource3": { + "Type": "some-other-type", + "Properties": properties + }, + "Fn::ForEach::OuterLoopName": [ + "Identifier1", + { + "Resource${Identifier1}": { + } + } + ] + } + } + + open_mock = mock.mock_open() + yaml_parse_mock.return_value = template_dict + + # Patch the file open method to return template string + with mock.patch( + "awscli.customizations.cloudformation.artifact_exporter.open", + open_mock(read_data=template_str)) as open_mock: + template_exporter = Template( + template_path, parent_dir, self.s3_uploader_mock, + resources_to_export) + with self.assertRaises(ValueError): + template_exporter.export() + + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") def test_template_global_export(self, yaml_parse_mock): parent_dir = os.path.sep From 1e844958c14dff2713029279989d62475d0149bc Mon Sep 17 00:00:00 2001 From: Arthur Boghossian Date: Tue, 5 Sep 2023 15:59:08 -0700 Subject: [PATCH 4/7] Move export_resources method into the Template class --- .../cloudformation/artifact_exporter.py | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 942dc32d04c5..d8f370a6cb57 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -659,29 +659,29 @@ def export(self): self.template_dict = self.export_global_artifacts(self.template_dict) - def export_resources(resource_dict): - for resource_id, resource in resource_dict.items(): - - if resource_id.startswith("Fn::ForEach"): - if not isinstance(resource, list) or len(resource) < 3: - raise ValueError("Fn::ForEach with name {0} has invalid format".format(resource_id)) - if isinstance(resource[2], dict): - export_resources(resource[2]) - continue + self.export_resources(self.template_dict["Resources"]) - resource_type = resource.get("Type", None) - if resource_type is None: - continue - resource_dict = resource.get("Properties", None) + return self.template_dict - for exporter_class in self.resources_to_export: - if exporter_class.RESOURCE_TYPE != resource_type: - continue + def export_resources(self, resource_dict): + for resource_id, resource in resource_dict.items(): - # Export code resources - exporter = exporter_class(self.uploader) - exporter.export(resource_id, resource_dict, self.template_dir) + if resource_id.startswith("Fn::ForEach"): + if not isinstance(resource, list) or len(resource) < 3: + raise ValueError("Fn::ForEach with name {0} has invalid format".format(resource_id)) + if isinstance(resource[2], dict): + self.export_resources(resource[2]) + continue - export_resources(self.template_dict["Resources"]) + resource_type = resource.get("Type", None) + if resource_type is None: + continue + resource_dict = resource.get("Properties", None) - return self.template_dict + for exporter_class in self.resources_to_export: + if exporter_class.RESOURCE_TYPE != resource_type: + continue + + # Export code resources + exporter = exporter_class(self.uploader) + exporter.export(resource_id, resource_dict, self.template_dir) From 10ae676faf65111caa2a2fe53e0b2a70fbe37689 Mon Sep 17 00:00:00 2001 From: Arthur Boghossian Date: Thu, 2 Nov 2023 14:33:40 -0700 Subject: [PATCH 5/7] Create changelog entry, define and use exception, and modify test to ensure there aren't unaccounted behaviors during export --- ...hancement-cloudformationpackage-39279.json | 5 +++++ .../cloudformation/artifact_exporter.py | 6 ++---- .../cloudformation/exceptions.py | 4 ++++ .../cloudformation/test_artifact_exporter.py | 20 ++++++++++++++----- 4 files changed, 26 insertions(+), 9 deletions(-) create mode 100644 .changes/next-release/enhancement-cloudformationpackage-39279.json diff --git a/.changes/next-release/enhancement-cloudformationpackage-39279.json b/.changes/next-release/enhancement-cloudformationpackage-39279.json new file mode 100644 index 000000000000..329081a2ec00 --- /dev/null +++ b/.changes/next-release/enhancement-cloudformationpackage-39279.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "``cloudformation package``", + "description": "Add support for intrinsic Fn:ForEach (fixes `#8075 `__)" +} diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index d8f370a6cb57..319e5f70af49 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -666,16 +666,14 @@ def export(self): def export_resources(self, resource_dict): for resource_id, resource in resource_dict.items(): - if resource_id.startswith("Fn::ForEach"): + if resource_id.startswith("Fn::ForEach::"): if not isinstance(resource, list) or len(resource) < 3: - raise ValueError("Fn::ForEach with name {0} has invalid format".format(resource_id)) + raise exceptions.InvalidForEachIntrinsicFunctionError(resource_id=resource_id) if isinstance(resource[2], dict): self.export_resources(resource[2]) continue resource_type = resource.get("Type", None) - if resource_type is None: - continue resource_dict = resource.get("Properties", None) for exporter_class in self.resources_to_export: diff --git a/awscli/customizations/cloudformation/exceptions.py b/awscli/customizations/cloudformation/exceptions.py index a31cf25ea492..4cdded7d4a6e 100644 --- a/awscli/customizations/cloudformation/exceptions.py +++ b/awscli/customizations/cloudformation/exceptions.py @@ -53,3 +53,7 @@ class DeployBucketRequiredError(CloudFormationCommandError): "via an S3 Bucket. Please add the --s3-bucket parameter to your " "command. The local template will be copied to that S3 bucket and " "then deployed.") + + +class InvalidForEachIntrinsicFunctionError(CloudFormationCommandError): + fmt = 'The value of {resource_id} has an invalid "Fn::ForEach::" format: Must be a list of at least three entries' diff --git a/tests/unit/customizations/cloudformation/test_artifact_exporter.py b/tests/unit/customizations/cloudformation/test_artifact_exporter.py index 861208cbe8ca..1b071101cc7e 100644 --- a/tests/unit/customizations/cloudformation/test_artifact_exporter.py +++ b/tests/unit/customizations/cloudformation/test_artifact_exporter.py @@ -1095,11 +1095,21 @@ def test_template_export_foreach_valid(self, yaml_parse_mock): self.assertEqual(1, yaml_parse_mock.call_count) resource_type1_class.assert_called_with(self.s3_uploader_mock) - resource_type1_instance.export.assert_called_with( - "Resource${Identifier1}", mock.ANY, template_dir) + self.assertEqual( + resource_type1_instance.export.call_args_list, + [ + mock.call("Resource1", properties, template_dir), + mock.call("Resource${Identifier1}", properties, template_dir) + ] + ) resource_type2_class.assert_called_with(self.s3_uploader_mock) - resource_type2_instance.export.assert_called_with( - "Resource${Identifier1}${Identifier2}", mock.ANY, template_dir) + self.assertEqual( + resource_type2_instance.export.call_args_list, + [ + mock.call("Resource2", properties, template_dir), + mock.call("Resource${Identifier1}${Identifier2}", properties, template_dir) + ] + ) @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") def test_template_export_foreach_invalid(self, yaml_parse_mock): @@ -1157,7 +1167,7 @@ def test_template_export_foreach_invalid(self, yaml_parse_mock): template_exporter = Template( template_path, parent_dir, self.s3_uploader_mock, resources_to_export) - with self.assertRaises(ValueError): + with self.assertRaises(exceptions.InvalidForEachIntrinsicFunctionError): template_exporter.export() From 2a473b3d04e6e8059e202a41d225350e4ba18250 Mon Sep 17 00:00:00 2001 From: Arthur Boghossian Date: Fri, 17 Nov 2023 13:27:26 -0800 Subject: [PATCH 6/7] Modify Fn::ForEach syntax definition to be stricter --- awscli/customizations/cloudformation/artifact_exporter.py | 5 ++--- awscli/customizations/cloudformation/exceptions.py | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 319e5f70af49..81be7fbd1b71 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -667,10 +667,9 @@ def export_resources(self, resource_dict): for resource_id, resource in resource_dict.items(): if resource_id.startswith("Fn::ForEach::"): - if not isinstance(resource, list) or len(resource) < 3: + if not isinstance(resource, list) or len(resource) != 3 or not isinstance(resource[2], dict): raise exceptions.InvalidForEachIntrinsicFunctionError(resource_id=resource_id) - if isinstance(resource[2], dict): - self.export_resources(resource[2]) + self.export_resources(resource[2]) continue resource_type = resource.get("Type", None) diff --git a/awscli/customizations/cloudformation/exceptions.py b/awscli/customizations/cloudformation/exceptions.py index 4cdded7d4a6e..b2625cdd27f9 100644 --- a/awscli/customizations/cloudformation/exceptions.py +++ b/awscli/customizations/cloudformation/exceptions.py @@ -56,4 +56,4 @@ class DeployBucketRequiredError(CloudFormationCommandError): class InvalidForEachIntrinsicFunctionError(CloudFormationCommandError): - fmt = 'The value of {resource_id} has an invalid "Fn::ForEach::" format: Must be a list of at least three entries' + fmt = 'The value of {resource_id} has an invalid "Fn::ForEach::" format: Must be a list of three entries' From 8f3be23a516148c7cfc9f1c955972e6e4ae753f0 Mon Sep 17 00:00:00 2001 From: Arthur Boghossian Date: Mon, 11 Dec 2023 10:23:13 -0800 Subject: [PATCH 7/7] Remove dict type check on 3rd parameter of Fn::ForEach intrinsic function --- awscli/customizations/cloudformation/artifact_exporter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 81be7fbd1b71..64eb5a06e1a4 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -667,7 +667,7 @@ def export_resources(self, resource_dict): for resource_id, resource in resource_dict.items(): if resource_id.startswith("Fn::ForEach::"): - if not isinstance(resource, list) or len(resource) != 3 or not isinstance(resource[2], dict): + if not isinstance(resource, list) or len(resource) != 3: raise exceptions.InvalidForEachIntrinsicFunctionError(resource_id=resource_id) self.export_resources(resource[2]) continue