From 0438bdcbc9d767e906c1aba5c84c3a62f21c373f Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Thu, 21 Nov 2024 16:20:56 -0800 Subject: [PATCH 01/26] Initial commit with TODOs for implementing modules Includes a basic test case for unit testing. --- .gitignore | 1 + .../cloudformation/artifact_exporter.py | 10 ++ .../customizations/cloudformation/modules.py | 109 ++++++++++++++++++ .../cloudformation/modules/basic-expect.yaml | 9 ++ .../cloudformation/modules/basic-module.yaml | 9 ++ .../modules/basic-template.yaml | 11 ++ 6 files changed, 149 insertions(+) create mode 100644 awscli/customizations/cloudformation/modules.py create mode 100644 tests/unit/customizations/cloudformation/modules/basic-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/basic-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/basic-template.yaml diff --git a/.gitignore b/.gitignore index 8bb22129f381..26fc540918a1 100644 --- a/.gitignore +++ b/.gitignore @@ -47,4 +47,5 @@ doc/source/tutorial/services.rst # Pyenv .python-version +.env diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 7fba8dddbba3..c6b5bae7da10 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -30,6 +30,7 @@ LOG = logging.getLogger(__name__) +MODULES = "Modules" def is_path_value_valid(path): return isinstance(path, str) @@ -651,6 +652,15 @@ def export(self): :return: The template with references to artifacts that have been exported to s3. """ + + # Process modules + # TODO + if MODULES in self.template_dict: + for module in self.template_dict[MODULES]: + self.template_dict = process_module(self.template_dict, module) + + # TODO - Remove the Modules section from the template + self.template_dict = self.export_metadata(self.template_dict) if "Resources" not in self.template_dict: diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py new file mode 100644 index 000000000000..e86a808b97a4 --- /dev/null +++ b/awscli/customizations/cloudformation/modules.py @@ -0,0 +1,109 @@ +# Copyright 2012-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"). You +# may not use this file except in compliance with the License. A copy of +# the License is located at +# +# http://aws.amazon.com/apache2.0/ +# +# or in the "license" file accompanying this file. This file is +# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF +# ANY KIND, either express or implied. See the License for the specific +# language governing permissions and limitations under the License. + +RESOURCES = "Resources" +METADATA = "Metadata" +OVERRIDES = "Overrides" +DEPENDSON = "DependsOn" +PROPERTIES = "Properties" +CREATIONPOLICY = "CreationPolicy" +UPDATEPOLICY = "UpdatePolicy" +DELETIONPOLICY = "DeletionPolicy" +UPDATEREPLACEPOLICY = "UpdateReplacePolicy" +CONDITION = "Condition" +DEFAULT = "Default" + + +class Module: + """ + Process client-side modules. + + See tests/unit/customizations/cloudformation/modules for examples of what + the Modules section of a template looks like. + + A module is itself basically a CloudFormation template, with a Parameters + section and Resources that are injected into the parent template. The + Properties defined in the Modules section correspond to the Parameters in + the module. These modules operate in a similar way to registry modules. + + The name of the module in the Modules section is used as a prefix to + logical ids that are defined in the module. + + In addition to the parent setting Properties, all attributes of the module + can be overridden with Overrides, which require the consumer to know how + the module is structured. This "escape hatch" is considered a first class + citizen in the design, to avoid excessive Parameter definitions to cover + every possible use case. + + Module Parameters (set by Properties in the parent) are handled with + Refs, Subs, and GetAtts in the module. These are handled in a way that + fixes references to match module prefixes, fully resolving values + that are actually strings and leaving others to be resolved at deploy time. + + Modules can contain other modules, with no limit to the levels of nesting. + """ + + def __init__(self, template, name, source, props, overrides): + "Initialize the module with values from the parent template" + + # The parent template dictionary + self.template = template + + # The name of the module, which is used as a logical id prefix + self.name = name + + # The location of the source for the module, a URI string + self.source = source + + # The Properties from the parent template + self.props = props + + # The Overrides from the parent template + self.overrides = overrides + + # Resources defined in the module + self.resources = {} + + # Parameters defined in the module + self.params = {} + + def process(self): + """ + Read the module source and return a dictionary that looks like a + template, with keys such as 'Resources' that have the processed + elements to be injected into the parent. + """ + retval = {} + retval[RESOURCES] = {} + + # TODO - Read the module file + + # TODO - Parse the text as if it were a template + + # TODO - Validate Overrides to make sure the resource exists + + # TODO - For each resource in the module: + + # TODO - For each property (and property-like attribute), + # replace the value if it appears in parent overrides. + # (Properties, CreationPolicy, Metadata, UpdatePolicy) + + # TODO - Replace scalar attributes with overrides + # (DeletionPolicy, UpdateReplacePolicy, Condition) + + # TODO - Replace DependsOn with overrides + + # TODO - Resolve refs, subs, and getatts + # (Process module Parameters and parent Properties) + + return retval diff --git a/tests/unit/customizations/cloudformation/modules/basic-expect.yaml b/tests/unit/customizations/cloudformation/modules/basic-expect.yaml new file mode 100644 index 000000000000..aa3975ab80ca --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/basic-expect.yaml @@ -0,0 +1,9 @@ +Resources: + ContentBucket: + Type: AWS::S3::Bucket + Properties: + BucketName: foo + OverrideMe: def + OtherResource: + Type: AWS::S3::Bucket + diff --git a/tests/unit/customizations/cloudformation/modules/basic-module.yaml b/tests/unit/customizations/cloudformation/modules/basic-module.yaml new file mode 100644 index 000000000000..98ab25d7b4f5 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/basic-module.yaml @@ -0,0 +1,9 @@ +Parameters: + Name: + Type: String +Resources: + Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: !Ref Name + OverrideMe: abc diff --git a/tests/unit/customizations/cloudformation/modules/basic-template.yaml b/tests/unit/customizations/cloudformation/modules/basic-template.yaml new file mode 100644 index 000000000000..23ce33d44ed5 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/basic-template.yaml @@ -0,0 +1,11 @@ +Modules: + Content: + Source: ./basic-module.yaml + Properties: + Name: foo + Overrides: + OverrideMe: def +Resources: + OtherResource: + Type: AWS::S3::Bucket + From 84cb38524e7880e44900121fc1f33deeea944016 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 4 Dec 2024 11:15:47 -0800 Subject: [PATCH 02/26] Unit test configured --- .../cloudformation/artifact_exporter.py | 17 +++-- .../cloudformation/exceptions.py | 6 ++ .../customizations/cloudformation/modules.py | 66 +++++++++++++++---- .../modules/basic-template.yaml | 1 - .../cloudformation/test_modules.py | 29 ++++++++ 5 files changed, 101 insertions(+), 18 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/test_modules.py diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index c6b5bae7da10..0e5cdb7802b5 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -25,6 +25,7 @@ from awscli.customizations.cloudformation import exceptions from awscli.customizations.cloudformation.yamlhelper import yaml_dump, \ yaml_parse +from awscli.customizations.cloudformation import modules import jmespath @@ -592,7 +593,6 @@ def __init__(self, template_path, parent_dir, uploader, raise ValueError("parent_dir parameter must be " "an absolute path to a folder {0}" .format(parent_dir)) - abs_template_path = make_abs_path(parent_dir, template_path) template_dir = os.path.dirname(abs_template_path) @@ -654,12 +654,19 @@ def export(self): """ # Process modules - # TODO if MODULES in self.template_dict: - for module in self.template_dict[MODULES]: - self.template_dict = process_module(self.template_dict, module) + # Process each Module node separately + for module_name, module_config in self.template_dict[MODULES].items(): + module_config[modules.NAME] = module_name + # Fix the source path + relative_path = module_config[modules.SOURCE] + module_config[modules.SOURCE] = f"{self.template_dir}/{relative_path}" + module = modules.Module(self.template_dict, module_config) + # Insert the content from the module and replace the dict + self.template_dict = module.process() - # TODO - Remove the Modules section from the template + # Remove the Modules section from the template + del self.template_dict[MODULES] self.template_dict = self.export_metadata(self.template_dict) diff --git a/awscli/customizations/cloudformation/exceptions.py b/awscli/customizations/cloudformation/exceptions.py index b2625cdd27f9..4beec17f6503 100644 --- a/awscli/customizations/cloudformation/exceptions.py +++ b/awscli/customizations/cloudformation/exceptions.py @@ -57,3 +57,9 @@ class DeployBucketRequiredError(CloudFormationCommandError): class InvalidForEachIntrinsicFunctionError(CloudFormationCommandError): fmt = 'The value of {resource_id} has an invalid "Fn::ForEach::" format: Must be a list of three entries' + +class InvalidModulePathError(CloudFormationCommandError): + fmt = 'The value of {source} is not a valid path to a local file' + +class InvalidModuleError(CloudFormationCommandError): + fmt = 'Invalid module: {msg}' diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index e86a808b97a4..4bc94bccc380 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -11,6 +11,10 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +from awscli.customizations.cloudformation import exceptions +from awscli.customizations.cloudformation import yamlhelper +import os + RESOURCES = "Resources" METADATA = "Metadata" OVERRIDES = "Overrides" @@ -22,6 +26,18 @@ UPDATEREPLACEPOLICY = "UpdateReplacePolicy" CONDITION = "Condition" DEFAULT = "Default" +NAME = "Name" +SOURCE = "Source" + + +def read_source(source): + "Read the source file and return the content as a string" + + if not isinstance(source, str) or not os.path.isfile(source): + raise exceptions.InvalidModulePathError(source=source) + + with open(source, "r") as s: + return s.read() class Module: @@ -53,23 +69,28 @@ class Module: Modules can contain other modules, with no limit to the levels of nesting. """ - def __init__(self, template, name, source, props, overrides): - "Initialize the module with values from the parent template" + def __init__(self, template, module_config): + """ + Initialize the module with values from the parent template + + :param template The parent template dictionary + :param module_config The configuration from the parent Modules section + """ # The parent template dictionary self.template = template # The name of the module, which is used as a logical id prefix - self.name = name + self.name = module_config[NAME] # The location of the source for the module, a URI string - self.source = source + self.source = module_config[SOURCE] # The Properties from the parent template - self.props = props + self.props = module_config[PROPERTIES] # The Overrides from the parent template - self.overrides = overrides + self.overrides = module_config[OVERRIDES] # Resources defined in the module self.resources = {} @@ -77,22 +98,43 @@ def __init__(self, template, name, source, props, overrides): # Parameters defined in the module self.params = {} + def __str__(self): + "Print out a string with module details for logs" + return ( + f"module name: {self.name}, " + + f"source: {self.source}, props: {self.props}" + ) + def process(self): """ Read the module source and return a dictionary that looks like a template, with keys such as 'Resources' that have the processed elements to be injected into the parent. + + :return: The modified parent template dictionary """ - retval = {} - retval[RESOURCES] = {} - # TODO - Read the module file + content = read_source(self.source) - # TODO - Parse the text as if it were a template + module_dict = yamlhelper.yaml_parse(content) + if RESOURCES not in module_dict: + msg = "Modules must have a Resources section" + raise exceptions.InvalidModuleError(msg=msg) - # TODO - Validate Overrides to make sure the resource exists + self.validate_overrides() # TODO - For each resource in the module: + for logical_id, resource in module_dict[RESOURCES].items(): + self.process_resource(logical_id, resource) + + return self.template + + def validate_overrides(self): + "Make sure resources referenced by overrides actually exist" + pass # TODO + + def process_resource(self, logical_id, resource): + "Process a single resource" # TODO - For each property (and property-like attribute), # replace the value if it appears in parent overrides. @@ -106,4 +148,4 @@ def process(self): # TODO - Resolve refs, subs, and getatts # (Process module Parameters and parent Properties) - return retval + self.template[RESOURCES]["Prefix?" + logical_id] = resource diff --git a/tests/unit/customizations/cloudformation/modules/basic-template.yaml b/tests/unit/customizations/cloudformation/modules/basic-template.yaml index 23ce33d44ed5..d63c346b64ec 100644 --- a/tests/unit/customizations/cloudformation/modules/basic-template.yaml +++ b/tests/unit/customizations/cloudformation/modules/basic-template.yaml @@ -8,4 +8,3 @@ Modules: Resources: OtherResource: Type: AWS::S3::Bucket - diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py new file mode 100644 index 000000000000..6aaf860c7929 --- /dev/null +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -0,0 +1,29 @@ +from awscli.testutils import unittest +from awscli.customizations.cloudformation import yamlhelper +from awscli.customizations.cloudformation import modules + +MODULES = "Modules" + + +class TestPackageModules(unittest.TestCase): + + def setUp(self): + pass + + def test_main(self): + base = "tests/unit/customizations/cloudformation/modules" + t = modules.read_source(f"{base}/basic-template.yaml") + td = yamlhelper.yaml_parse(t) + # m = modules.read_source(f"{base}/basic-module.yaml") + e = modules.read_source(f"{base}/basic-expect.yaml") + + for module_name, module_config in td[MODULES].items(): + module_config[modules.NAME] = module_name + relative_path = module_config[modules.SOURCE] + module_config[modules.SOURCE] = f"{base}/{relative_path}" + module = modules.Module(td, module_config) + td2 = module.process() + t2 = yamlhelper.yaml_dump(td2) + self.assertEqual(e, t2) + + # self.assertEqual(1, 0) From 9eac23a48565fa890974d4d31d967e37d28833cc Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 4 Dec 2024 15:22:24 -0800 Subject: [PATCH 03/26] Start resolving refs --- .../customizations/cloudformation/modules.py | 201 ++++++++++++++++-- .../modules/basic-template.yaml | 4 +- .../cloudformation/test_modules.py | 11 +- 3 files changed, 196 insertions(+), 20 deletions(-) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 4bc94bccc380..2b7b8226a44e 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -28,6 +28,10 @@ DEFAULT = "Default" NAME = "Name" SOURCE = "Source" +REF = "Ref" +SUB = "Fn::Sub" +GETATT = "Fn::GetAtt" +PARAMETERS = "Parameters" def read_source(source): @@ -40,6 +44,56 @@ def read_source(source): return s.read() +def merge_props(original, overrides): + """ + Merge props merges dicts, replacing values in the original with overrides. + This function is recursive and can act on lists and scalars. + + :return A new value with the overridden properties + + Original: + + A: + B: foo + C: + D: bar + + Override: + + A: + B: baz + C: + E: qux + + Result: + + A: + B: baz + C: + D: bar + E: qux + """ + original_type = type(original) + override_type = type(overrides) + if override_type is not dict and override_type is not list: + return overrides + + if original_type is not override_type: + return overrides + + if original_type is dict: + retval = original.copy() + for k in original: + if k in overrides: + retval[k] = merge_props(retval[k], overrides[k]) + for k in overrides: + if k not in original: + retval[k] = overrides[k] + return retval + else: + return original + overrides + + class Module: """ Process client-side modules. @@ -87,10 +141,14 @@ def __init__(self, template, module_config): self.source = module_config[SOURCE] # The Properties from the parent template - self.props = module_config[PROPERTIES] + self.props = {} + if PROPERTIES in module_config: + self.props = module_config[PROPERTIES] # The Overrides from the parent template - self.overrides = module_config[OVERRIDES] + self.overrides = {} + if OVERRIDES in module_config: + self.overrides = module_config[OVERRIDES] # Resources defined in the module self.resources = {} @@ -107,9 +165,7 @@ def __str__(self): def process(self): """ - Read the module source and return a dictionary that looks like a - template, with keys such as 'Resources' that have the processed - elements to be injected into the parent. + Read the module source process it. :return: The modified parent template dictionary """ @@ -120,11 +176,14 @@ def process(self): if RESOURCES not in module_dict: msg = "Modules must have a Resources section" raise exceptions.InvalidModuleError(msg=msg) + self.resources = module_dict[RESOURCES] + + if PARAMETERS in module_dict: + self.params = module_dict[PARAMETERS] self.validate_overrides() - # TODO - For each resource in the module: - for logical_id, resource in module_dict[RESOURCES].items(): + for logical_id, resource in self.resources.items(): self.process_resource(logical_id, resource) return self.template @@ -136,16 +195,126 @@ def validate_overrides(self): def process_resource(self, logical_id, resource): "Process a single resource" - # TODO - For each property (and property-like attribute), - # replace the value if it appears in parent overrides. - # (Properties, CreationPolicy, Metadata, UpdatePolicy) + # For each property (and property-like attribute), + # replace the value if it appears in parent overrides. + attrs = [ + PROPERTIES, + CREATIONPOLICY, + METADATA, + UPDATEPOLICY, + DELETIONPOLICY, + CONDITION, + UPDATEREPLACEPOLICY, + DEPENDSON, + ] + for a in attrs: + self.process_overrides(logical_id, resource, a) + + # Resolve refs, subs, and getatts + # (Process module Parameters and parent Properties) + self.resolve(logical_id, resource) + + self.template[RESOURCES][self.name + logical_id] = resource + + def process_overrides(self, logical_id, resource, attr_name): + """ + Replace overridden values in a property-like attribute of a resource. + + (Properties, Metadata, CreationPolicy, and UpdatePolicy) + + Overrides are a way to customize modules without needing a Parameter. + + Example template.yaml: + + Modules: + Foo: + Source: ./module.yaml + Overrides: + Bar: + Properties: + Name: bbb - # TODO - Replace scalar attributes with overrides - # (DeletionPolicy, UpdateReplacePolicy, Condition) + Example module.yaml: - # TODO - Replace DependsOn with overrides + Resources: + Bar: + Type: A::B::C + Properties: + Name: aaa + + Output yaml: + + Resources: + Bar: + Type: A::B::C + Properties: + Name: bbb + """ + + if logical_id not in self.overrides: + return + + resource_overrides = self.overrides[logical_id] + if attr_name not in resource_overrides: + return + + # Might be overriding something that's not in the module at all, + # like a Bucket with no Properties + if attr_name not in resource: + if attr_name in resource_overrides: + resource[attr_name] = resource_overrides[attr_name] + else: + return + + original = resource[attr_name] + overrides = resource_overrides[attr_name] + resource[attr_name] = merge_props(original, overrides) + + def resolve(self, k, v): + """ + Resolve Refs, Subs, and GetAtts recursively. + + :param k The name of the node + :param v The value of the node + + """ + if k == REF: + self.resolve_ref(v) + elif k == SUB: + self.resolve_sub(v) + elif k == GETATT: + self.resolve_getatt(v) + else: + if type(v) is dict: + for k2, v2 in v.items(): + self.resolve(k2, v2) + elif type(v) is list: + for v2 in v: + if type(v2) is dict: + for k3, v3 in v2.items(): + self.resolve(k3, v3) + + def resolve_ref(v): + """ + Look for the Ref in the parent template Properties if it matches + a module Parameter name. If it's not there, use the default if + there is one. If not, raise an error. + + If there is no matching Parameter, look for a resource with that + name in this module and fix the logical id so it has the prefix. + + Otherwise just leave it be and assume the module author is + expecting the parent template to have that Reference. + """ + if type(v) is not str: + msg = f"Ref should be a string: {v}" + raise exceptions.InvalidModuleError(msg=msg) + # TODO - # TODO - Resolve refs, subs, and getatts - # (Process module Parameters and parent Properties) + def resolve_sub(v): + pass + # TODO - self.template[RESOURCES]["Prefix?" + logical_id] = resource + def resolve_getatt(v): + pass + # TODO diff --git a/tests/unit/customizations/cloudformation/modules/basic-template.yaml b/tests/unit/customizations/cloudformation/modules/basic-template.yaml index d63c346b64ec..712eb2a8a87b 100644 --- a/tests/unit/customizations/cloudformation/modules/basic-template.yaml +++ b/tests/unit/customizations/cloudformation/modules/basic-template.yaml @@ -4,7 +4,9 @@ Modules: Properties: Name: foo Overrides: - OverrideMe: def + Bucket: + Properties: + OverrideMe: def Resources: OtherResource: Type: AWS::S3::Bucket diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 6aaf860c7929..745983004651 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -10,11 +10,18 @@ class TestPackageModules(unittest.TestCase): def setUp(self): pass + def test_merge_props(self): + original = {"b": "c", "d": {"e": "f", "i": [1, 2, 3]}} + overrides = {"b": "cc", "d": {"e": "ff", "g": "h", "i": [4, 5]}} + expect = {"b": "cc", "d": {"e": "ff", "g": "h", "i": [1, 2, 3, 4, 5]}} + merged = modules.merge_props(original, overrides) + self.assertEqual(merged, expect) + # TODO: More complex examples (especially merging Policies) + def test_main(self): base = "tests/unit/customizations/cloudformation/modules" t = modules.read_source(f"{base}/basic-template.yaml") td = yamlhelper.yaml_parse(t) - # m = modules.read_source(f"{base}/basic-module.yaml") e = modules.read_source(f"{base}/basic-expect.yaml") for module_name, module_config in td[MODULES].items(): @@ -25,5 +32,3 @@ def test_main(self): td2 = module.process() t2 = yamlhelper.yaml_dump(td2) self.assertEqual(e, t2) - - # self.assertEqual(1, 0) From 0b795263d749080f5342c4503701f66eb8db4e92 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Thu, 5 Dec 2024 10:23:46 -0800 Subject: [PATCH 04/26] Resolve Refs --- .../customizations/cloudformation/modules.py | 105 ++++++++++++++---- 1 file changed, 86 insertions(+), 19 deletions(-) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 2b7b8226a44e..71326ae7661e 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -14,6 +14,7 @@ from awscli.customizations.cloudformation import exceptions from awscli.customizations.cloudformation import yamlhelper import os +from collections import OrderedDict RESOURCES = "Resources" METADATA = "Metadata" @@ -34,6 +35,10 @@ PARAMETERS = "Parameters" +def isdict(v): + return type(v) is dict or isinstance(v, OrderedDict) + + def read_source(source): "Read the source file and return the content as a string" @@ -46,8 +51,8 @@ def read_source(source): def merge_props(original, overrides): """ - Merge props merges dicts, replacing values in the original with overrides. - This function is recursive and can act on lists and scalars. + This function merges dicts, replacing values in the original with + overrides. This function is recursive and can act on lists and scalars. :return A new value with the overridden properties @@ -75,13 +80,13 @@ def merge_props(original, overrides): """ original_type = type(original) override_type = type(overrides) - if override_type is not dict and override_type is not list: + if not isdict(overrides) and override_type is not list: return overrides if original_type is not override_type: return overrides - if original_type is dict: + if isdict(original): retval = original.copy() for k in original: if k in overrides: @@ -133,6 +138,9 @@ def __init__(self, template, module_config): # The parent template dictionary self.template = template + if RESOURCES not in self.template: + # The parent might only have Modules + self.template[RESOURCES] = {} # The name of the module, which is used as a logical id prefix self.name = module_config[NAME] @@ -156,6 +164,9 @@ def __init__(self, template, module_config): # Parameters defined in the module self.params = {} + # TODO: What about Conditions, Mappings, Outputs? + # Is there a use case for importing those into the parent? + def __str__(self): "Print out a string with module details for logs" return ( @@ -181,6 +192,8 @@ def process(self): if PARAMETERS in module_dict: self.params = module_dict[PARAMETERS] + # TODO: Recurse on nested modules + self.validate_overrides() for logical_id, resource in self.resources.items(): @@ -212,7 +225,9 @@ def process_resource(self, logical_id, resource): # Resolve refs, subs, and getatts # (Process module Parameters and parent Properties) - self.resolve(logical_id, resource) + container = {} + container[RESOURCES] = self.resources + self.resolve(logical_id, resource, container, RESOURCES) self.template[RESOURCES][self.name + logical_id] = resource @@ -270,31 +285,55 @@ def process_overrides(self, logical_id, resource, attr_name): overrides = resource_overrides[attr_name] resource[attr_name] = merge_props(original, overrides) - def resolve(self, k, v): + def resolve(self, k, v, d, n): """ Resolve Refs, Subs, and GetAtts recursively. :param k The name of the node :param v The value of the node + :param d The dict that is the parent of the dict that holds k, v + :param n The name of the dict that holds k, v + + Example + + Resources: + Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: !Ref Name + + In the above example, + k = !Ref, v = Name, d = Properties{}, n = BucketName + + So we can set d[n] = resolved_value (which replaces {k,v}) + In the prior iteration, + k = BucketName, v = {!Ref, Name}, d = Bucket{}, n = Properties """ + + print(f"resolve k={k}, v={v}, d={d}, n={n}") + if k == REF: - self.resolve_ref(v) + self.resolve_ref(k, v, d, n) elif k == SUB: - self.resolve_sub(v) + self.resolve_sub(k, v, d, n) elif k == GETATT: - self.resolve_getatt(v) + self.resolve_getatt(k, v, d, n) else: - if type(v) is dict: - for k2, v2 in v.items(): - self.resolve(k2, v2) + if isdict(v): + vc = v.copy() + for k2, v2 in vc.items(): + self.resolve(k2, v2, d[n], k) elif type(v) is list: for v2 in v: - if type(v2) is dict: - for k3, v3 in v2.items(): - self.resolve(k3, v3) + if isdict(v2): + v2c = v2.copy() + for k3, v3 in v2c.items(): + self.resolve(k3, v3, d[n], k) + else: + print(f"{v}: type(v) is {type(v)}") - def resolve_ref(v): + def resolve_ref(self, k, v, d, n): """ Look for the Ref in the parent template Properties if it matches a module Parameter name. If it's not there, use the default if @@ -309,12 +348,40 @@ def resolve_ref(v): if type(v) is not str: msg = f"Ref should be a string: {v}" raise exceptions.InvalidModuleError(msg=msg) - # TODO - def resolve_sub(v): + if not isdict(d): + # TODO: This is a bug, shouldn't happen + msg = f"{k}: expected {d} to be a dict" + raise exceptions.InvalidModuleError(msg=msg) + + if v in self.props: + if v not in self.params: + # The parent tried to set a property that doesn't exist + # in the Parameters section of this module + msg = f"{v} not found in module Parameters: {self.source}" + raise exceptions.InvalidModuleException(msg=msg) + prop = self.props[v] + print(f"About to set {n} to {prop}, d is {d}") + d[n] = prop + elif v in self.params: + param = self.params[v] + if DEFAULT in param: + # Use the default value of the Parameter + d[n] = param[DEFAULT] + else: + msg = f"{v} does not have a Default and is not a Property" + raise exceptions.InvalidModuleError(msg=msg) + else: + for k in self.resources: + if v == k: + # Simply rename local references to include the module name + d[n] = self.name + v + break + + def resolve_sub(self, k, v, d, n): pass # TODO - def resolve_getatt(v): + def resolve_getatt(self, k, v, d, n): pass # TODO From 6f24f03e9045815ac127984aa6efc71001fa7469 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Thu, 5 Dec 2024 14:09:11 -0800 Subject: [PATCH 05/26] parse_sub --- .../customizations/cloudformation/modules.py | 10 +- .../cloudformation/parse_sub.py | 102 ++++++++++++++++++ .../cloudformation/test_modules.py | 45 ++++++++ 3 files changed, 156 insertions(+), 1 deletion(-) create mode 100644 awscli/customizations/cloudformation/parse_sub.py diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 71326ae7661e..9133e9f707c0 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -379,8 +379,16 @@ def resolve_ref(self, k, v, d, n): break def resolve_sub(self, k, v, d, n): + """ + Parse the Sub string and break it into tokens. + + If we can fully resolve it, we can replace it with a string. + + Use the same logic as with resolve_ref. + """ pass - # TODO + + # TODO def resolve_getatt(self, k, v, d, n): pass diff --git a/awscli/customizations/cloudformation/parse_sub.py b/awscli/customizations/cloudformation/parse_sub.py new file mode 100644 index 000000000000..a6afb592eb03 --- /dev/null +++ b/awscli/customizations/cloudformation/parse_sub.py @@ -0,0 +1,102 @@ +import re + +DATA = ' ' # Any other character +DOLLAR = '$' +OPEN = '{' +CLOSE = '}' +BANG = '!' + +class WordType: + STR = 0 # A literal string fragment + REF = 1 # ${ParamOrResourceName} + AWS = 2 # ${AWS::X} + GETATT = 3 # ${X.Y} + +class State: + READSTR = 0 + READVAR = 1 + MAYBE = 2 + READLIT = 3 + + + +class SubWord: + def __init__(self, word_type, word): + self.T = word_type + self.W = word # Does not include the ${} if it's not a STR + +def parse_sub(sub_str, leave_bang=False): + words = [] + state = State.READSTR + buf = '' + i = -1 + last = '' + for char in sub_str: + i += 1 + if char == DOLLAR: + if state != State.READVAR: + state = State.MAYBE + else: + # This is a literal $ inside a variable: "${AB$C}" + # TODO: Should that be an error? Is it valid? + buf += char + elif char == OPEN: + if state == State.MAYBE: + # Peek to see if we're about to start a LITERAL ! + if len(sub_str) > i+1 and sub_str[i+1] == BANG: + # Treat this as part of the string, not a var + buf += "${" + state = State.READLIT + else: + state = State.READVAR + # We're about to start reading a variable. + # Append the last word in the buffer if it's not empty + if buf: + words.append(SubWord(WordType.STR, buf)) + buf = '' + else: + buf += char + elif char == CLOSE: + if state == State.READVAR: + # Figure out what type it is + if buf.startswith("AWS::"): + word_type = WordType.AWS + elif '.' in buf: + word_type = WordType.GETATT + else: + word_type = WordType.REF + buf = buf.replace("AWS::", "", 1) + words.append(SubWord(word_type, buf)) + buf = '' + state = State.READSTR + else: + buf += char + elif char == BANG: + # ${!LITERAL} becomes ${LITERAL} + if state == State.READLIT: + # Don't write the ! to the string + state = State.READSTR + if leave_bang: + # Unless we actually want it + buf += char + else: + # This is a ! somewhere not related to a LITERAL + buf += char + else: + if state == State.MAYBE: + buf += last # Put the $ back on the buffer + state = State.READSTR + buf += char + + last = char + + if buf: + words.append(SubWord(WordType.STR, buf)) + + # Handle malformed strings, like "ABC${XYZ" + if state != State.READSTR: + # Ended the string in the middle of a variable? + raise ValueError("invalid string, unclosed variable") + + return words + diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 745983004651..36f2ba887634 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -1,6 +1,8 @@ from awscli.testutils import unittest from awscli.customizations.cloudformation import yamlhelper from awscli.customizations.cloudformation import modules +from awscli.customizations.cloudformation.parse_sub import SubWord, WordType +from awscli.customizations.cloudformation.parse_sub import parse_sub MODULES = "Modules" @@ -10,6 +12,49 @@ class TestPackageModules(unittest.TestCase): def setUp(self): pass + def test_parse_sub(self): + cases = { + "ABC": [SubWord(WordType.STR, "ABC")], + "ABC-${XYZ}-123": [ + SubWord(WordType.STR, "ABC-"), + SubWord(WordType.REF, "XYZ"), + SubWord(WordType.STR, "-123"), + ], + "ABC-${!Literal}-1": [SubWord(WordType.STR, "ABC-${Literal}-1")], + "${ABC}": [SubWord(WordType.REF, "ABC")], + "${ABC.XYZ}": [SubWord(WordType.GETATT, "ABC.XYZ")], + "ABC${AWS::AccountId}XYZ": [ + SubWord(WordType.STR, "ABC"), + SubWord(WordType.AWS, "AccountId"), + SubWord(WordType.STR, "XYZ"), + ], + "BAZ${ABC$XYZ}FOO$BAR": [ + SubWord(WordType.STR, "BAZ"), + SubWord(WordType.REF, "ABC$XYZ"), + SubWord(WordType.STR, "FOO$BAR"), + ], + } + + for sub, expect in cases.items(): + words = parse_sub(sub, False) + self.assertEqual( + len(expect), + len(words), + f'"{sub}": words len is {len(words)}, expected {len(expect)}', + ) + for i, w in enumerate(expect): + self.assertEqual( + words[i].T, w.T, f'"{sub}": got {words[i]}, expected {w}' + ) + self.assertEqual( + words[i].W, w.W, f'"{sub}": got {words[i]}, expected {w}' + ) + + # Invalid strings should fail + sub = "${AAA" + with self.assertRaises(Exception, msg=f'"{sub}": should have failed'): + parse_sub(sub, False) + def test_merge_props(self): original = {"b": "c", "d": {"e": "f", "i": [1, 2, 3]}} overrides = {"b": "cc", "d": {"e": "ff", "g": "h", "i": [4, 5]}} From 86a5d29c5339d141fc04686d65d5bad4204d3e9d Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Thu, 5 Dec 2024 16:06:10 -0800 Subject: [PATCH 06/26] Sub strings --- .../customizations/cloudformation/modules.py | 58 ++++++++++++++++--- .../cloudformation/parse_sub.py | 3 + .../cloudformation/modules/basic-expect.yaml | 1 + .../cloudformation/modules/basic-module.yaml | 1 + 4 files changed, 55 insertions(+), 8 deletions(-) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 9133e9f707c0..1dc63363e456 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -13,6 +13,9 @@ from awscli.customizations.cloudformation import exceptions from awscli.customizations.cloudformation import yamlhelper + +from awscli.customizations.cloudformation.parse_sub import WordType +from awscli.customizations.cloudformation.parse_sub import parse_sub import os from collections import OrderedDict @@ -354,20 +357,23 @@ def resolve_ref(self, k, v, d, n): msg = f"{k}: expected {d} to be a dict" raise exceptions.InvalidModuleError(msg=msg) + found = self.find_ref(v) + if found is not None: + d[n] = found + + def find_ref(self, v): if v in self.props: if v not in self.params: # The parent tried to set a property that doesn't exist # in the Parameters section of this module msg = f"{v} not found in module Parameters: {self.source}" raise exceptions.InvalidModuleException(msg=msg) - prop = self.props[v] - print(f"About to set {n} to {prop}, d is {d}") - d[n] = prop + return self.props[v] elif v in self.params: param = self.params[v] if DEFAULT in param: # Use the default value of the Parameter - d[n] = param[DEFAULT] + return param[DEFAULT] else: msg = f"{v} does not have a Default and is not a Property" raise exceptions.InvalidModuleError(msg=msg) @@ -375,8 +381,9 @@ def resolve_ref(self, k, v, d, n): for k in self.resources: if v == k: # Simply rename local references to include the module name - d[n] = self.name + v + return self.name + v break + return None def resolve_sub(self, k, v, d, n): """ @@ -386,9 +393,44 @@ def resolve_sub(self, k, v, d, n): Use the same logic as with resolve_ref. """ - pass - - # TODO + words = parse_sub(v, True) + sub = "" + need_sub = False + for word in words: + print(f"resolve_sub word {word}") + if word.T == WordType.STR: + sub += word.W + elif word.T == WordType.AWS: + sub += "${AWS::" + word.W + "}" + need_sub = True + elif word.T == WordType.REF: + resolved = f"${word.W}" + found = self.find_ref(word.W) + print("found", found) + if found is not None: + if type(found) is str: + resolved = found + else: + need_sub = True + if REF in found: + resolved = "${" + found[REF] + "}" + # TODO: else what is this? + sub += resolved + elif word.T == WordType.GETATT: + need_sub = True + tokens = word.W.split() + if len(tokens) != 2: + msg = "GetAtt {word.W} has unexpected number of tokens" + raise exceptions.InvalidModuleError(msg=msg) + if tokens[0] in self.resources: + tokens[0] = self.name + tokens[0] + + print("need_sub", need_sub, "d", d, "n", n, "sub", sub) + + if need_sub: + d[n] = {SUB: sub} + else: + d[n] = sub def resolve_getatt(self, k, v, d, n): pass diff --git a/awscli/customizations/cloudformation/parse_sub.py b/awscli/customizations/cloudformation/parse_sub.py index a6afb592eb03..7970c77d8e0b 100644 --- a/awscli/customizations/cloudformation/parse_sub.py +++ b/awscli/customizations/cloudformation/parse_sub.py @@ -24,6 +24,9 @@ class SubWord: def __init__(self, word_type, word): self.T = word_type self.W = word # Does not include the ${} if it's not a STR + + def __str__(self): + return f"{self.T} {self.W}" def parse_sub(sub_str, leave_bang=False): words = [] diff --git a/tests/unit/customizations/cloudformation/modules/basic-expect.yaml b/tests/unit/customizations/cloudformation/modules/basic-expect.yaml index aa3975ab80ca..9c42d4f40f67 100644 --- a/tests/unit/customizations/cloudformation/modules/basic-expect.yaml +++ b/tests/unit/customizations/cloudformation/modules/basic-expect.yaml @@ -4,6 +4,7 @@ Resources: Properties: BucketName: foo OverrideMe: def + SubName: foo OtherResource: Type: AWS::S3::Bucket diff --git a/tests/unit/customizations/cloudformation/modules/basic-module.yaml b/tests/unit/customizations/cloudformation/modules/basic-module.yaml index 98ab25d7b4f5..00aec040f391 100644 --- a/tests/unit/customizations/cloudformation/modules/basic-module.yaml +++ b/tests/unit/customizations/cloudformation/modules/basic-module.yaml @@ -7,3 +7,4 @@ Resources: Properties: BucketName: !Ref Name OverrideMe: abc + SubName: !Sub ${Name} From 7b72e29cbf43005be2097df22e09f33c2987c32a Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Fri, 6 Dec 2024 09:27:07 -0800 Subject: [PATCH 07/26] Basic tests pass --- .../customizations/cloudformation/modules.py | 20 +++++++++---------- .../cloudformation/modules/basic-expect.yaml | 10 ++++++++-- .../cloudformation/modules/basic-module.yaml | 4 ++++ .../cloudformation/test_modules.py | 8 +++++--- 4 files changed, 27 insertions(+), 15 deletions(-) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 1dc63363e456..a95d1f49360e 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -314,8 +314,6 @@ def resolve(self, k, v, d, n): k = BucketName, v = {!Ref, Name}, d = Bucket{}, n = Properties """ - print(f"resolve k={k}, v={v}, d={d}, n={n}") - if k == REF: self.resolve_ref(k, v, d, n) elif k == SUB: @@ -333,8 +331,6 @@ def resolve(self, k, v, d, n): v2c = v2.copy() for k3, v3 in v2c.items(): self.resolve(k3, v3, d[n], k) - else: - print(f"{v}: type(v) is {type(v)}") def resolve_ref(self, k, v, d, n): """ @@ -397,7 +393,6 @@ def resolve_sub(self, k, v, d, n): sub = "" need_sub = False for word in words: - print(f"resolve_sub word {word}") if word.T == WordType.STR: sub += word.W elif word.T == WordType.AWS: @@ -406,7 +401,6 @@ def resolve_sub(self, k, v, d, n): elif word.T == WordType.REF: resolved = f"${word.W}" found = self.find_ref(word.W) - print("found", found) if found is not None: if type(found) is str: resolved = found @@ -425,13 +419,19 @@ def resolve_sub(self, k, v, d, n): if tokens[0] in self.resources: tokens[0] = self.name + tokens[0] - print("need_sub", need_sub, "d", d, "n", n, "sub", sub) - if need_sub: d[n] = {SUB: sub} else: d[n] = sub def resolve_getatt(self, k, v, d, n): - pass - # TODO + """ + Resolve a GetAtt. All we do here is add the prefix. + + !GetAtt Foo.Bar becomes !GetAtt ModuleNameFoo.Bar + """ + if type(v) is not list: + msg = f"GetAtt {v} is not a list" + raise exceptions.InvalidModuleError(msg=msg) + logical_id = self.name + v[0] + d[n] = {k: [logical_id, v[1]]} diff --git a/tests/unit/customizations/cloudformation/modules/basic-expect.yaml b/tests/unit/customizations/cloudformation/modules/basic-expect.yaml index 9c42d4f40f67..1010e68cf134 100644 --- a/tests/unit/customizations/cloudformation/modules/basic-expect.yaml +++ b/tests/unit/customizations/cloudformation/modules/basic-expect.yaml @@ -1,10 +1,16 @@ Resources: + OtherResource: + Type: AWS::S3::Bucket ContentBucket: Type: AWS::S3::Bucket Properties: BucketName: foo OverrideMe: def SubName: foo - OtherResource: + ContentBucket2: Type: AWS::S3::Bucket - + Properties: + BucketName: + Fn::GetAtt: + - ContentBucket + - Something diff --git a/tests/unit/customizations/cloudformation/modules/basic-module.yaml b/tests/unit/customizations/cloudformation/modules/basic-module.yaml index 00aec040f391..cbb5d4bbd90b 100644 --- a/tests/unit/customizations/cloudformation/modules/basic-module.yaml +++ b/tests/unit/customizations/cloudformation/modules/basic-module.yaml @@ -8,3 +8,7 @@ Resources: BucketName: !Ref Name OverrideMe: abc SubName: !Sub ${Name} + Bucket2: + Type: AWS::S3::Bucket + Properties: + BucketName: !GetAtt Bucket.Something diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 36f2ba887634..a7163d177992 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -74,6 +74,8 @@ def test_main(self): relative_path = module_config[modules.SOURCE] module_config[modules.SOURCE] = f"{base}/{relative_path}" module = modules.Module(td, module_config) - td2 = module.process() - t2 = yamlhelper.yaml_dump(td2) - self.assertEqual(e, t2) + td = module.process() + + del td[MODULES] + processed = yamlhelper.yaml_dump(td) + self.assertEqual(e, processed) From df507cde2850122df717ef0dbcfffb0c219be53d Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Fri, 6 Dec 2024 11:00:52 -0800 Subject: [PATCH 08/26] Fixed pylint issues --- .../customizations/cloudformation/modules.py | 83 ++++++++++++------- .../cloudformation/parse_sub.py | 38 +++++++-- .../cloudformation/test_modules.py | 17 +++- 3 files changed, 94 insertions(+), 44 deletions(-) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index a95d1f49360e..0db40162951a 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -11,13 +11,18 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +"This file implements local module support for the package command" + +# pylint: disable=fixme + +import os +from collections import OrderedDict + from awscli.customizations.cloudformation import exceptions from awscli.customizations.cloudformation import yamlhelper from awscli.customizations.cloudformation.parse_sub import WordType from awscli.customizations.cloudformation.parse_sub import parse_sub -import os -from collections import OrderedDict RESOURCES = "Resources" METADATA = "Metadata" @@ -39,7 +44,8 @@ def isdict(v): - return type(v) is dict or isinstance(v, OrderedDict) + "Returns True if the type is a dict or OrderedDict" + return isinstance(v, (dict, OrderedDict)) def read_source(source): @@ -48,7 +54,7 @@ def read_source(source): if not isinstance(source, str) or not os.path.isfile(source): raise exceptions.InvalidModulePathError(source=source) - with open(source, "r") as s: + with open(source, "r", encoding="utf-8") as s: return s.read() @@ -98,8 +104,8 @@ def merge_props(original, overrides): if k not in original: retval[k] = overrides[k] return retval - else: - return original + overrides + + return original + overrides class Module: @@ -206,7 +212,10 @@ def process(self): def validate_overrides(self): "Make sure resources referenced by overrides actually exist" - pass # TODO + for logical_id in self.overrides: + if logical_id not in self.resources: + msg = f"Override {logical_id} not found in {self.source}" + raise exceptions.InvalidModuleError(msg=msg) def process_resource(self, logical_id, resource): "Process a single resource" @@ -325,7 +334,7 @@ def resolve(self, k, v, d, n): vc = v.copy() for k2, v2 in vc.items(): self.resolve(k2, v2, d[n], k) - elif type(v) is list: + elif isinstance(v, list): for v2 in v: if isdict(v2): v2c = v2.copy() @@ -344,7 +353,7 @@ def resolve_ref(self, k, v, d, n): Otherwise just leave it be and assume the module author is expecting the parent template to have that Reference. """ - if type(v) is not str: + if not isinstance(v, str): msg = f"Ref should be a string: {v}" raise exceptions.InvalidModuleError(msg=msg) @@ -358,29 +367,39 @@ def resolve_ref(self, k, v, d, n): d[n] = found def find_ref(self, v): + """ + Find a Ref. + + A Ref might be to a module Parameter with a matching parent + template Property, or a Parameter Default. It could also + be a reference to another resource in this module. + + :return The referenced element or None + """ if v in self.props: if v not in self.params: # The parent tried to set a property that doesn't exist # in the Parameters section of this module msg = f"{v} not found in module Parameters: {self.source}" - raise exceptions.InvalidModuleException(msg=msg) + raise exceptions.InvalidModuleError(msg=msg) return self.props[v] - elif v in self.params: + + if v in self.params: param = self.params[v] if DEFAULT in param: # Use the default value of the Parameter return param[DEFAULT] - else: - msg = f"{v} does not have a Default and is not a Property" - raise exceptions.InvalidModuleError(msg=msg) - else: - for k in self.resources: - if v == k: - # Simply rename local references to include the module name - return self.name + v - break + msg = f"{v} does not have a Default and is not a Property" + raise exceptions.InvalidModuleError(msg=msg) + + for k in self.resources: + if v == k: + # Simply rename local references to include the module name + return self.name + v + return None + # pylint: disable=too-many-branches,unused-argument def resolve_sub(self, k, v, d, n): """ Parse the Sub string and break it into tokens. @@ -393,16 +412,16 @@ def resolve_sub(self, k, v, d, n): sub = "" need_sub = False for word in words: - if word.T == WordType.STR: - sub += word.W - elif word.T == WordType.AWS: - sub += "${AWS::" + word.W + "}" + if word.t == WordType.STR: + sub += word.w + elif word.t == WordType.AWS: + sub += "${AWS::" + word.w + "}" need_sub = True - elif word.T == WordType.REF: - resolved = f"${word.W}" - found = self.find_ref(word.W) + elif word.t == WordType.REF: + resolved = f"${word.w}" + found = self.find_ref(word.w) if found is not None: - if type(found) is str: + if isinstance(found, str): resolved = found else: need_sub = True @@ -410,11 +429,11 @@ def resolve_sub(self, k, v, d, n): resolved = "${" + found[REF] + "}" # TODO: else what is this? sub += resolved - elif word.T == WordType.GETATT: + elif word.t == WordType.GETATT: need_sub = True - tokens = word.W.split() + tokens = word.w.split() if len(tokens) != 2: - msg = "GetAtt {word.W} has unexpected number of tokens" + msg = "GetAtt {word.w} has unexpected number of tokens" raise exceptions.InvalidModuleError(msg=msg) if tokens[0] in self.resources: tokens[0] = self.name + tokens[0] @@ -430,7 +449,7 @@ def resolve_getatt(self, k, v, d, n): !GetAtt Foo.Bar becomes !GetAtt ModuleNameFoo.Bar """ - if type(v) is not list: + if not isinstance(v, list): msg = f"GetAtt {v} is not a list" raise exceptions.InvalidModuleError(msg=msg) logical_id = self.name + v[0] diff --git a/awscli/customizations/cloudformation/parse_sub.py b/awscli/customizations/cloudformation/parse_sub.py index 7970c77d8e0b..c05eb393adea 100644 --- a/awscli/customizations/cloudformation/parse_sub.py +++ b/awscli/customizations/cloudformation/parse_sub.py @@ -1,4 +1,18 @@ -import re +""" +This module parses CloudFormation Sub strings. + +For example: + + !Sub abc-${AWS::Region}-def-${Foo} + +The string is broken down into "words" that are one of four types: + + String: A literal string component + Ref: A reference to another resource or paramter like ${Foo} + AWS: An AWS pseudo-parameter like ${AWS::Region} + GetAtt: A reference to an attribute like ${Foo.Bar} +""" +#pylint: disable=too-few-public-methods DATA = ' ' # Any other character DOLLAR = '$' @@ -7,28 +21,36 @@ BANG = '!' class WordType: + "Word type enumeration" STR = 0 # A literal string fragment REF = 1 # ${ParamOrResourceName} AWS = 2 # ${AWS::X} GETATT = 3 # ${X.Y} class State: + "State machine enumeration" READSTR = 0 READVAR = 1 MAYBE = 2 READLIT = 3 - - class SubWord: + "A single word with a type and the word itself" def __init__(self, word_type, word): - self.T = word_type - self.W = word # Does not include the ${} if it's not a STR - + self.t = word_type + self.w = word # Does not include the ${} if it's not a STR + def __str__(self): - return f"{self.T} {self.W}" + return f"{self.t} {self.w}" +#pylint: disable=too-many-branches,too-many-statements def parse_sub(sub_str, leave_bang=False): + """ + Parse a Sub string + + :param leave_bang If this is True, leave the ! in literals + :return list of words + """ words = [] state = State.READSTR buf = '' @@ -41,7 +63,6 @@ def parse_sub(sub_str, leave_bang=False): state = State.MAYBE else: # This is a literal $ inside a variable: "${AB$C}" - # TODO: Should that be an error? Is it valid? buf += char elif char == OPEN: if state == State.MAYBE: @@ -102,4 +123,3 @@ def parse_sub(sub_str, leave_bang=False): raise ValueError("invalid string, unclosed variable") return words - diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index a7163d177992..6304e4e5ec29 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -1,3 +1,7 @@ +"Tests for module support in the package command" + +# pylint: disable=fixme + from awscli.testutils import unittest from awscli.customizations.cloudformation import yamlhelper from awscli.customizations.cloudformation import modules @@ -8,11 +12,13 @@ class TestPackageModules(unittest.TestCase): + "Module tests" def setUp(self): - pass + "Initialize the tests" def test_parse_sub(self): + "Test the parse_sub function" cases = { "ABC": [SubWord(WordType.STR, "ABC")], "ABC-${XYZ}-123": [ @@ -44,10 +50,10 @@ def test_parse_sub(self): ) for i, w in enumerate(expect): self.assertEqual( - words[i].T, w.T, f'"{sub}": got {words[i]}, expected {w}' + words[i].t, w.t, f'"{sub}": got {words[i]}, expected {w}' ) self.assertEqual( - words[i].W, w.W, f'"{sub}": got {words[i]}, expected {w}' + words[i].w, w.w, f'"{sub}": got {words[i]}, expected {w}' ) # Invalid strings should fail @@ -56,6 +62,8 @@ def test_parse_sub(self): parse_sub(sub, False) def test_merge_props(self): + "Test the merge_props function" + original = {"b": "c", "d": {"e": "f", "i": [1, 2, 3]}} overrides = {"b": "cc", "d": {"e": "ff", "g": "h", "i": [4, 5]}} expect = {"b": "cc", "d": {"e": "ff", "g": "h", "i": [1, 2, 3, 4, 5]}} @@ -64,6 +72,9 @@ def test_merge_props(self): # TODO: More complex examples (especially merging Policies) def test_main(self): + "Run tests on sample templates that include local modules" + + # TODO: Port tests over from Rain base = "tests/unit/customizations/cloudformation/modules" t = modules.read_source(f"{base}/basic-template.yaml") td = yamlhelper.yaml_parse(t) From 7808f629a07b26b49b797e919a111e54343d2959 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Fri, 6 Dec 2024 11:40:05 -0800 Subject: [PATCH 09/26] Fix path to unit tests --- tests/unit/customizations/cloudformation/test_modules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 6304e4e5ec29..ea593168efbb 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -75,7 +75,7 @@ def test_main(self): "Run tests on sample templates that include local modules" # TODO: Port tests over from Rain - base = "tests/unit/customizations/cloudformation/modules" + base = "unit/customizations/cloudformation/modules" t = modules.read_source(f"{base}/basic-template.yaml") td = yamlhelper.yaml_parse(t) e = modules.read_source(f"{base}/basic-expect.yaml") From e2131d259f917f071bc9f403171cc8a57e7a8f4d Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Mon, 9 Dec 2024 10:30:23 -0800 Subject: [PATCH 10/26] Add the ability to specify a module as a Resource --- .../cloudformation/artifact_exporter.py | 27 ++++++++-- .../customizations/cloudformation/modules.py | 10 +++- .../cloudformation/modules/type-expect.yaml | 16 ++++++ .../cloudformation/modules/type-module.yaml | 14 +++++ .../cloudformation/modules/type-template.yaml | 12 +++++ .../cloudformation/test_modules.py | 53 ++++++++++++++----- 6 files changed, 115 insertions(+), 17 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/type-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/type-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/type-template.yaml diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 0e5cdb7802b5..08bfdf3824c9 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -32,6 +32,9 @@ LOG = logging.getLogger(__name__) MODULES = "Modules" +RESOURCES = "Resources" +TYPE = "Type" +LOCAL_MODULE = "LocalModule" def is_path_value_valid(path): return isinstance(path, str) @@ -662,7 +665,6 @@ def export(self): relative_path = module_config[modules.SOURCE] module_config[modules.SOURCE] = f"{self.template_dir}/{relative_path}" module = modules.Module(self.template_dict, module_config) - # Insert the content from the module and replace the dict self.template_dict = module.process() # Remove the Modules section from the template @@ -670,12 +672,31 @@ def export(self): self.template_dict = self.export_metadata(self.template_dict) - if "Resources" not in self.template_dict: + if RESOURCES not in self.template_dict: return self.template_dict + # Process modules that are specified as Resources, not in Modules + for k, v in self.template_dict[RESOURCES].copy().items(): + if TYPE in v and v[TYPE] == LOCAL_MODULE: + module_config = {} + module_config[modules.NAME] = k + if modules.SOURCE not in v: + msg = f"{k} missing {modules.SOURCE}" + raise exceptions.InvalidModulePathError(msg=msg) + relative_path = v[modules.SOURCE] + module_config[modules.SOURCE] = f"{self.template_dir}/{relative_path}" + if modules.PROPERTIES in v: + module_config[modules.PROPERTIES] = v[modules.PROPERTIES] + if modules.OVERRIDES in v: + module_config[modules.OVERRIDES] = v[modules.OVERRIDES] + module = modules.Module(self.template_dict, module_config) + self.template_dict = module.process() + del self.template_dict[RESOURCES][k] + + self.template_dict = self.export_global_artifacts(self.template_dict) - self.export_resources(self.template_dict["Resources"]) + self.export_resources(self.template_dict[RESOURCES]) return self.template_dict diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 0db40162951a..76a4a177df80 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -115,13 +115,21 @@ class Module: See tests/unit/customizations/cloudformation/modules for examples of what the Modules section of a template looks like. + Modules can be referenced in a new Modules section in the templates, + or they can be referenced as Resources with the Type LocalModule. + Modules have a Source attribute pointing to a local file, + a Properties attribute that corresponds to Parameters in the modules, + and an Overrides attribute that can override module output. + A module is itself basically a CloudFormation template, with a Parameters section and Resources that are injected into the parent template. The Properties defined in the Modules section correspond to the Parameters in the module. These modules operate in a similar way to registry modules. The name of the module in the Modules section is used as a prefix to - logical ids that are defined in the module. + logical ids that are defined in the module. Or if the module is + referenced in the Type attribute of a Resource, the logical id of the + resource is used as the prefix. In addition to the parent setting Properties, all attributes of the module can be overridden with Overrides, which require the consumer to know how diff --git a/tests/unit/customizations/cloudformation/modules/type-expect.yaml b/tests/unit/customizations/cloudformation/modules/type-expect.yaml new file mode 100644 index 000000000000..1010e68cf134 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/type-expect.yaml @@ -0,0 +1,16 @@ +Resources: + OtherResource: + Type: AWS::S3::Bucket + ContentBucket: + Type: AWS::S3::Bucket + Properties: + BucketName: foo + OverrideMe: def + SubName: foo + ContentBucket2: + Type: AWS::S3::Bucket + Properties: + BucketName: + Fn::GetAtt: + - ContentBucket + - Something diff --git a/tests/unit/customizations/cloudformation/modules/type-module.yaml b/tests/unit/customizations/cloudformation/modules/type-module.yaml new file mode 100644 index 000000000000..cbb5d4bbd90b --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/type-module.yaml @@ -0,0 +1,14 @@ +Parameters: + Name: + Type: String +Resources: + Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: !Ref Name + OverrideMe: abc + SubName: !Sub ${Name} + Bucket2: + Type: AWS::S3::Bucket + Properties: + BucketName: !GetAtt Bucket.Something diff --git a/tests/unit/customizations/cloudformation/modules/type-template.yaml b/tests/unit/customizations/cloudformation/modules/type-template.yaml new file mode 100644 index 000000000000..e868af732938 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/type-template.yaml @@ -0,0 +1,12 @@ +Resources: + Content: + Type: LocalModule + Source: ./type-module.yaml + Properties: + Name: foo + Overrides: + Bucket: + Properties: + OverrideMe: def + OtherResource: + Type: AWS::S3::Bucket diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index ea593168efbb..9b973daa042e 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -9,6 +9,9 @@ from awscli.customizations.cloudformation.parse_sub import parse_sub MODULES = "Modules" +RESOURCES = "Resources" +TYPE = "Type" +LOCAL_MODULE = "LocalModule" class TestPackageModules(unittest.TestCase): @@ -75,18 +78,42 @@ def test_main(self): "Run tests on sample templates that include local modules" # TODO: Port tests over from Rain - base = "unit/customizations/cloudformation/modules" - t = modules.read_source(f"{base}/basic-template.yaml") - td = yamlhelper.yaml_parse(t) - e = modules.read_source(f"{base}/basic-expect.yaml") - - for module_name, module_config in td[MODULES].items(): - module_config[modules.NAME] = module_name - relative_path = module_config[modules.SOURCE] - module_config[modules.SOURCE] = f"{base}/{relative_path}" - module = modules.Module(td, module_config) - td = module.process() - - del td[MODULES] + + # The tests are in the modules directory. + # Each test has 3 files: + # test-template.yaml, test-module.yaml, and test-expect.yaml + tests = ["basic", "type"] + for test in tests: + base = "unit/customizations/cloudformation/modules" + t = modules.read_source(f"{base}/{test}-template.yaml") + td = yamlhelper.yaml_parse(t) + e = modules.read_source(f"{base}/{test}-expect.yaml") + + # Modules section + if MODULES in td: + for module_name, module_config in td[MODULES].items(): + module_config[modules.NAME] = module_name + relative_path = module_config[modules.SOURCE] + module_config[modules.SOURCE] = f"{base}/{relative_path}" + module = modules.Module(td, module_config) + td = module.process() + del td[MODULES] + + # Resources with Type LocalModule + for k, v in td[RESOURCES].copy().items(): + if TYPE in v and v[TYPE] == LOCAL_MODULE: + module_config = {} + module_config[modules.NAME] = k + relative_path = v[modules.SOURCE] + module_config[modules.SOURCE] = f"{base}/{relative_path}" + props = modules.PROPERTIES + if props in v: + module_config[props] = v[props] + if modules.OVERRIDES in v: + module_config[modules.OVERRIDES] = v[modules.OVERRIDES] + module = modules.Module(td, module_config) + td = module.process() + del td[RESOURCES][k] + processed = yamlhelper.yaml_dump(td) self.assertEqual(e, processed) From 49fb3921958dab93cd2b7d4a705075b253e5f3f4 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Mon, 9 Dec 2024 14:48:01 -0800 Subject: [PATCH 11/26] Fix issues with nested Subs and lists --- .../cloudformation/artifact_exporter.py | 67 +++++++++++-------- .../customizations/cloudformation/modules.py | 32 ++++----- .../cloudformation/modules/sub-expect.yaml | 21 ++++++ .../cloudformation/modules/sub-module.yaml | 23 +++++++ .../cloudformation/modules/sub-template.yaml | 11 +++ .../cloudformation/test_modules.py | 2 +- 6 files changed, 112 insertions(+), 44 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/sub-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/sub-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/sub-template.yaml diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 08bfdf3824c9..f69e7c2e464e 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -14,6 +14,7 @@ import logging import os import tempfile +import traceback import zipfile import contextlib import uuid @@ -657,18 +658,23 @@ def export(self): """ # Process modules - if MODULES in self.template_dict: - # Process each Module node separately - for module_name, module_config in self.template_dict[MODULES].items(): - module_config[modules.NAME] = module_name - # Fix the source path - relative_path = module_config[modules.SOURCE] - module_config[modules.SOURCE] = f"{self.template_dir}/{relative_path}" - module = modules.Module(self.template_dict, module_config) - self.template_dict = module.process() - - # Remove the Modules section from the template - del self.template_dict[MODULES] + try: + if MODULES in self.template_dict: + # Process each Module node separately + for module_name, module_config in self.template_dict[MODULES].items(): + module_config[modules.NAME] = module_name + # Fix the source path + relative_path = module_config[modules.SOURCE] + module_config[modules.SOURCE] = f"{self.template_dir}/{relative_path}" + module = modules.Module(self.template_dict, module_config) + self.template_dict = module.process() + + # Remove the Modules section from the template + del self.template_dict[MODULES] + except Exception as e: + traceback.print_exc() + msg=f"Failed to process Modules section: {e}" + raise exceptions.InvalidModuleError(msg=msg) self.template_dict = self.export_metadata(self.template_dict) @@ -676,22 +682,27 @@ def export(self): return self.template_dict # Process modules that are specified as Resources, not in Modules - for k, v in self.template_dict[RESOURCES].copy().items(): - if TYPE in v and v[TYPE] == LOCAL_MODULE: - module_config = {} - module_config[modules.NAME] = k - if modules.SOURCE not in v: - msg = f"{k} missing {modules.SOURCE}" - raise exceptions.InvalidModulePathError(msg=msg) - relative_path = v[modules.SOURCE] - module_config[modules.SOURCE] = f"{self.template_dir}/{relative_path}" - if modules.PROPERTIES in v: - module_config[modules.PROPERTIES] = v[modules.PROPERTIES] - if modules.OVERRIDES in v: - module_config[modules.OVERRIDES] = v[modules.OVERRIDES] - module = modules.Module(self.template_dict, module_config) - self.template_dict = module.process() - del self.template_dict[RESOURCES][k] + try: + for k, v in self.template_dict[RESOURCES].copy().items(): + if TYPE in v and v[TYPE] == LOCAL_MODULE: + module_config = {} + module_config[modules.NAME] = k + if modules.SOURCE not in v: + msg = f"{k} missing {modules.SOURCE}" + raise exceptions.InvalidModulePathError(msg=msg) + relative_path = v[modules.SOURCE] + module_config[modules.SOURCE] = f"{self.template_dir}/{relative_path}" + if modules.PROPERTIES in v: + module_config[modules.PROPERTIES] = v[modules.PROPERTIES] + if modules.OVERRIDES in v: + module_config[modules.OVERRIDES] = v[modules.OVERRIDES] + module = modules.Module(self.template_dict, module_config) + self.template_dict = module.process() + del self.template_dict[RESOURCES][k] + except Exception as e: + traceback.print_exc() + msg=f"Failed to process modules in Resources: {e}" + raise exceptions.InvalidModuleError(msg=msg) self.template_dict = self.export_global_artifacts(self.template_dict) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 76a4a177df80..87554dfe6a57 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -331,25 +331,29 @@ def resolve(self, k, v, d, n): k = BucketName, v = {!Ref, Name}, d = Bucket{}, n = Properties """ + # print(f"k: {k}, v: {v}, d: {d}, n: {n}") + if k == REF: - self.resolve_ref(k, v, d, n) + self.resolve_ref(v, d, n) elif k == SUB: - self.resolve_sub(k, v, d, n) + self.resolve_sub(v, d, n) elif k == GETATT: - self.resolve_getatt(k, v, d, n) + self.resolve_getatt(v, d, n) else: if isdict(v): vc = v.copy() for k2, v2 in vc.items(): self.resolve(k2, v2, d[n], k) elif isinstance(v, list): + idx = -1 for v2 in v: + idx = idx + 1 if isdict(v2): v2c = v2.copy() for k3, v3 in v2c.items(): - self.resolve(k3, v3, d[n], k) + self.resolve(k3, v3, v, idx) - def resolve_ref(self, k, v, d, n): + def resolve_ref(self, v, d, n): """ Look for the Ref in the parent template Properties if it matches a module Parameter name. If it's not there, use the default if @@ -365,11 +369,6 @@ def resolve_ref(self, k, v, d, n): msg = f"Ref should be a string: {v}" raise exceptions.InvalidModuleError(msg=msg) - if not isdict(d): - # TODO: This is a bug, shouldn't happen - msg = f"{k}: expected {d} to be a dict" - raise exceptions.InvalidModuleError(msg=msg) - found = self.find_ref(v) if found is not None: d[n] = found @@ -384,6 +383,7 @@ def find_ref(self, v): :return The referenced element or None """ + # print(f"find_ref {v}, props: {self.props}") if v in self.props: if v not in self.params: # The parent tried to set a property that doesn't exist @@ -403,12 +403,12 @@ def find_ref(self, v): for k in self.resources: if v == k: # Simply rename local references to include the module name - return self.name + v + return {REF: self.name + v} return None # pylint: disable=too-many-branches,unused-argument - def resolve_sub(self, k, v, d, n): + def resolve_sub(self, v, d, n): """ Parse the Sub string and break it into tokens. @@ -416,6 +416,7 @@ def resolve_sub(self, k, v, d, n): Use the same logic as with resolve_ref. """ + # print(f"resolve_sub v: {v}, d: {d}, n: {n}") words = parse_sub(v, True) sub = "" need_sub = False @@ -435,7 +436,8 @@ def resolve_sub(self, k, v, d, n): need_sub = True if REF in found: resolved = "${" + found[REF] + "}" - # TODO: else what is this? + elif SUB in found: + resolved = found[SUB] sub += resolved elif word.t == WordType.GETATT: need_sub = True @@ -451,7 +453,7 @@ def resolve_sub(self, k, v, d, n): else: d[n] = sub - def resolve_getatt(self, k, v, d, n): + def resolve_getatt(self, v, d, n): """ Resolve a GetAtt. All we do here is add the prefix. @@ -461,4 +463,4 @@ def resolve_getatt(self, k, v, d, n): msg = f"GetAtt {v} is not a list" raise exceptions.InvalidModuleError(msg=msg) logical_id = self.name + v[0] - d[n] = {k: [logical_id, v[1]]} + d[n] = {GETATT: [logical_id, v[1]]} diff --git a/tests/unit/customizations/cloudformation/modules/sub-expect.yaml b/tests/unit/customizations/cloudformation/modules/sub-expect.yaml new file mode 100644 index 000000000000..79cb95080ccc --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/sub-expect.yaml @@ -0,0 +1,21 @@ +Resources: + MyBucket1: + Type: AWS::S3::Bucket + Properties: + BucketName: foo + X: + Fn::Sub: ${Foo} + Y: + - Fn::Sub: noparent0-${Foo} + - Fn::Sub: noparent1-${Foo} + Z: + - Fn::Sub: ${Foo} + - Ref: MyBucket2 + - ZZ: + ZZZ: + ZZZZ: + Fn::Sub: ${Foo} + MyBucket2: + Type: AWS::S3::Bucket + Properties: + BucketName: bar diff --git a/tests/unit/customizations/cloudformation/modules/sub-module.yaml b/tests/unit/customizations/cloudformation/modules/sub-module.yaml new file mode 100644 index 000000000000..8d4937f4aefc --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/sub-module.yaml @@ -0,0 +1,23 @@ +Parameters: + Name: + Type: String + SubName: + Type: String +Resources: + Bucket1: + Type: AWS::S3::Bucket + Properties: + BucketName: !Sub ${Name} + X: !Sub "${SubName}" + Y: + - !Sub noparent0-${SubName} + - !Sub noparent1-${SubName} + Z: + - !Ref SubName + - !Ref Bucket2 + - ZZ: + ZZZ: + ZZZZ: !Sub "${SubName}" + Bucket2: + Type: AWS::S3::Bucket + diff --git a/tests/unit/customizations/cloudformation/modules/sub-template.yaml b/tests/unit/customizations/cloudformation/modules/sub-template.yaml new file mode 100644 index 000000000000..feeb263bfcad --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/sub-template.yaml @@ -0,0 +1,11 @@ +Resources: + My: + Type: LocalModule + Source: ./sub-module.yaml + Properties: + Name: foo + SubName: !Sub ${Foo} + Overrides: + Bucket2: + Properties: + BucketName: bar diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 9b973daa042e..61bdc9e1523f 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -82,7 +82,7 @@ def test_main(self): # The tests are in the modules directory. # Each test has 3 files: # test-template.yaml, test-module.yaml, and test-expect.yaml - tests = ["basic", "type"] + tests = ["basic", "type", "sub"] for test in tests: base = "unit/customizations/cloudformation/modules" t = modules.read_source(f"{base}/{test}-template.yaml") From ec32ecfa76ab6cea3d9fcce6e64e404b70c7da69 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Mon, 9 Dec 2024 15:59:49 -0800 Subject: [PATCH 12/26] Recursive modules --- .../cloudformation/artifact_exporter.py | 36 ++-------- .../customizations/cloudformation/modules.py | 71 +++++++++++++++++-- .../modules/modinmod-expect.yaml | 14 ++++ .../modules/modinmod-module.yaml | 11 +++ .../modules/modinmod-submodule.yaml | 12 ++++ .../modules/modinmod-template.yaml | 11 +++ .../cloudformation/test_modules.py | 2 +- 7 files changed, 120 insertions(+), 37 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/modinmod-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/modinmod-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/modinmod-submodule.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/modinmod-template.yaml diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index f69e7c2e464e..76c42852d1c9 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -34,8 +34,6 @@ MODULES = "Modules" RESOURCES = "Resources" -TYPE = "Type" -LOCAL_MODULE = "LocalModule" def is_path_value_valid(path): return isinstance(path, str) @@ -659,18 +657,9 @@ def export(self): # Process modules try: - if MODULES in self.template_dict: - # Process each Module node separately - for module_name, module_config in self.template_dict[MODULES].items(): - module_config[modules.NAME] = module_name - # Fix the source path - relative_path = module_config[modules.SOURCE] - module_config[modules.SOURCE] = f"{self.template_dir}/{relative_path}" - module = modules.Module(self.template_dict, module_config) - self.template_dict = module.process() - - # Remove the Modules section from the template - del self.template_dict[MODULES] + self.template_dict = modules.process_module_section( + self.template_dict, + self.template_dir) except Exception as e: traceback.print_exc() msg=f"Failed to process Modules section: {e}" @@ -683,22 +672,9 @@ def export(self): # Process modules that are specified as Resources, not in Modules try: - for k, v in self.template_dict[RESOURCES].copy().items(): - if TYPE in v and v[TYPE] == LOCAL_MODULE: - module_config = {} - module_config[modules.NAME] = k - if modules.SOURCE not in v: - msg = f"{k} missing {modules.SOURCE}" - raise exceptions.InvalidModulePathError(msg=msg) - relative_path = v[modules.SOURCE] - module_config[modules.SOURCE] = f"{self.template_dir}/{relative_path}" - if modules.PROPERTIES in v: - module_config[modules.PROPERTIES] = v[modules.PROPERTIES] - if modules.OVERRIDES in v: - module_config[modules.OVERRIDES] = v[modules.OVERRIDES] - module = modules.Module(self.template_dict, module_config) - self.template_dict = module.process() - del self.template_dict[RESOURCES][k] + self.template_dict = modules.process_resources_section( + self.template_dict, + self.template_dir) except Exception as e: traceback.print_exc() msg=f"Failed to process modules in Resources: {e}" diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 87554dfe6a57..d70e894039e7 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -16,6 +16,7 @@ # pylint: disable=fixme import os +import traceback from collections import OrderedDict from awscli.customizations.cloudformation import exceptions @@ -41,6 +42,51 @@ SUB = "Fn::Sub" GETATT = "Fn::GetAtt" PARAMETERS = "Parameters" +MODULES = "Modules" +TYPE = "Type" +LOCAL_MODULE = "LocalModule" + + +def process_module_section(template, base_path): + "Recursively process the Modules section of a template" + if MODULES in template: + + # Process each Module node separately + for module_name, module_config in template[MODULES].items(): + module_config[NAME] = module_name + # Fix the source path + relative_path = module_config[SOURCE] + module_config[SOURCE] = os.path.join(base_path, relative_path) + module_config[SOURCE] = os.path.normpath(module_config[SOURCE]) + module = Module(template, module_config) + template = module.process() + + # Remove the Modules section from the template + del template[MODULES] + + return template + + +def process_resources_section(template, base_path): + "Recursively process the Resources section of the template" + for k, v in template[RESOURCES].copy().items(): + if TYPE in v and v[TYPE] == LOCAL_MODULE: + module_config = {} + module_config[NAME] = k + if SOURCE not in v: + msg = f"{k} missing {SOURCE}" + raise exceptions.InvalidModulePathError(msg=msg) + relative_path = v[SOURCE] + module_config[SOURCE] = os.path.join(base_path, relative_path) + module_config[SOURCE] = os.path.normpath(module_config[SOURCE]) + if PROPERTIES in v: + module_config[PROPERTIES] = v[PROPERTIES] + if OVERRIDES in v: + module_config[OVERRIDES] = v[OVERRIDES] + module = Module(template, module_config) + template = module.process() + del template[RESOURCES][k] + return template def isdict(v): @@ -202,14 +248,26 @@ def process(self): module_dict = yamlhelper.yaml_parse(content) if RESOURCES not in module_dict: - msg = "Modules must have a Resources section" - raise exceptions.InvalidModuleError(msg=msg) - self.resources = module_dict[RESOURCES] + # The module may only have sub modules in the Modules section + self.resources = {} + else: + self.resources = module_dict[RESOURCES] if PARAMETERS in module_dict: self.params = module_dict[PARAMETERS] - # TODO: Recurse on nested modules + # Recurse on nested modules + base_path = os.path.dirname(self.source) + section = "" + try: + section = MODULES + module_dict = process_module_section(module_dict, base_path) + section = RESOURCES + module_dict = process_resources_section(module_dict, base_path) + except Exception as e: + traceback.print_exc() + msg = f"Failed to process {section} section: {e}" + raise exceptions.InvalidModuleError(msg=msg) self.validate_overrides() @@ -427,13 +485,14 @@ def resolve_sub(self, v, d, n): sub += "${AWS::" + word.w + "}" need_sub = True elif word.t == WordType.REF: - resolved = f"${word.w}" + resolved = "${" + word.w + "}" + need_sub = True found = self.find_ref(word.w) if found is not None: if isinstance(found, str): + need_sub = False resolved = found else: - need_sub = True if REF in found: resolved = "${" + found[REF] + "}" elif SUB in found: diff --git a/tests/unit/customizations/cloudformation/modules/modinmod-expect.yaml b/tests/unit/customizations/cloudformation/modules/modinmod-expect.yaml new file mode 100644 index 000000000000..561947b750a2 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/modinmod-expect.yaml @@ -0,0 +1,14 @@ +Parameters: + ParentVal: + Type: String + AppName: + Type: String +Resources: + MySubBucket: + Type: AWS::S3::Bucket + Properties: + BucketName: mod-in-mod-bucket + XName: + Fn::Sub: ${ParentVal}-abc + YName: + Fn::Sub: ${AppName}-xyz diff --git a/tests/unit/customizations/cloudformation/modules/modinmod-module.yaml b/tests/unit/customizations/cloudformation/modules/modinmod-module.yaml new file mode 100644 index 000000000000..208307bd37f5 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/modinmod-module.yaml @@ -0,0 +1,11 @@ +Parameters: + AppName: + Type: String +Resources: + Sub: + Type: LocalModule + Source: "./modinmod-submodule.yaml" + Properties: + X: !Sub ${ParentVal}-abc + Y: !Sub ${AppName}-xyz + diff --git a/tests/unit/customizations/cloudformation/modules/modinmod-submodule.yaml b/tests/unit/customizations/cloudformation/modules/modinmod-submodule.yaml new file mode 100644 index 000000000000..745292ee0711 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/modinmod-submodule.yaml @@ -0,0 +1,12 @@ +Parameters: + X: + Type: String + Y: + Type: String +Resources: + Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: mod-in-mod-bucket + XName: !Ref X + YName: !Ref Y diff --git a/tests/unit/customizations/cloudformation/modules/modinmod-template.yaml b/tests/unit/customizations/cloudformation/modules/modinmod-template.yaml new file mode 100644 index 000000000000..99cd480b512d --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/modinmod-template.yaml @@ -0,0 +1,11 @@ +Parameters: + ParentVal: + Type: String + AppName: + Type: String +Resources: + My: + Type: LocalModule + Source: ./modinmod-module.yaml + Properties: + AppName: !Ref AppName diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 61bdc9e1523f..51969358220e 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -82,7 +82,7 @@ def test_main(self): # The tests are in the modules directory. # Each test has 3 files: # test-template.yaml, test-module.yaml, and test-expect.yaml - tests = ["basic", "type", "sub"] + tests = ["basic", "type", "sub", "modinmod"] for test in tests: base = "unit/customizations/cloudformation/modules" t = modules.read_source(f"{base}/{test}-template.yaml") From 160be7c453d0c3491681a8648eecc16efff97fee Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Thu, 12 Dec 2024 16:22:23 -0800 Subject: [PATCH 13/26] Implement module outputs --- .../customizations/cloudformation/modules.py | 137 +++++++++++++++++- .../cloudformation/modules/output-expect.yaml | 17 +++ .../cloudformation/modules/output-module.yaml | 12 ++ .../modules/output-template.yaml | 12 ++ .../cloudformation/test_modules.py | 2 +- 5 files changed, 173 insertions(+), 7 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/output-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/output-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/output-template.yaml diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index d70e894039e7..e57ffcc98f1c 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -13,7 +13,7 @@ "This file implements local module support for the package command" -# pylint: disable=fixme +# pylint: disable=fixme,too-many-instance-attributes import os import traceback @@ -45,6 +45,7 @@ MODULES = "Modules" TYPE = "Type" LOCAL_MODULE = "LocalModule" +OUTPUTS = "Outputs" def process_module_section(template, base_path): @@ -227,8 +228,8 @@ def __init__(self, template, module_config): # Parameters defined in the module self.params = {} - # TODO: What about Conditions, Mappings, Outputs? - # Is there a use case for importing those into the parent? + # Outputs defined in the module + self.outputs = {} def __str__(self): "Print out a string with module details for logs" @@ -256,6 +257,9 @@ def process(self): if PARAMETERS in module_dict: self.params = module_dict[PARAMETERS] + if OUTPUTS in module_dict: + self.outputs = module_dict[OUTPUTS] + # Recurse on nested modules base_path = os.path.dirname(self.source) section = "" @@ -274,8 +278,126 @@ def process(self): for logical_id, resource in self.resources.items(): self.process_resource(logical_id, resource) + self.process_outputs() + return self.template + def process_outputs(self): + """ + Fix parent template output references. + + In the parent you can !GetAtt ModuleName.OutputName + This will be converted to !GetAtt ModuleName + OutputValue + + Recurse over all sections in the parent template looking for + GetAtts and Subs that reference a module output value. + """ + sections = [RESOURCES, OUTPUTS] # TODO: Any others? + for section in sections: + if section not in self.template: + continue + for k, v in self.template[section].items(): + self.resolve_outputs(k, v, self.template, section) + + def resolve_outputs(self, k, v, d, n): + """ + Recursively resolve GetAtts and Subs that reference module outputs. + + :param name The name of the output + :param output The output dict + :param k The name of the node + :param v The value of the node + :param d The dict that holds the parent of k + :param n The name of the node that holds k + + If a reference is found, this function sets the value of d[n] + """ + if k == SUB: + self.resolve_output_sub(v, d, n) + elif k == GETATT: + self.resolve_output_getatt(v, d, n) + else: + if isdict(v): + for k2, v2 in v.copy().items(): + self.resolve_outputs(k2, v2, d[n], k) + elif isinstance(v, list): + idx = -1 + for v2 in v: + idx = idx + 1 + if isdict(v2): + for k3, v3 in v2.copy().items(): + self.resolve_outputs(k3, v3, v, idx) + + def resolve_output_sub(self, v, d, n): + "Resolve a Sub that refers to a module output" + words = parse_sub(v, True) + sub = "" + for word in words: + if word.t == WordType.STR: + sub += word.w + elif word.t == WordType.AWS: + sub += "${AWS::" + word.w + "}" + elif word.t == WordType.REF: + # A reference to an output has to be a getatt + resolved = "${" + word.w + "}" + sub += resolved + elif word.t == WordType.GETATT: + resolved = "${" + word.w + "}" + tokens = word.w.split(".") + if len(tokens) != 2: + msg = f"GetAtt {word.w} has unexpected number of tokens" + raise exceptions.InvalidModuleError(msg=msg) + # !Sub ${Content.BucketArn} -> !Sub ${ContentBucket.Arn} + if tokens[0] == self.name and tokens[1] in self.outputs: + output = self.outputs[tokens[1]] + if GETATT in output: + getatt = output[GETATT] + resolved = "${" + self.name + ".".join(getatt) + "}" + elif SUB in output: + resolved = "${" + self.name + output[SUB] + "}" + sub += resolved + + d[n] = {SUB: sub} + + def resolve_output_getatt(self, v, d, n): + "Resolve a GetAtt that refers to a module output" + if not isinstance(v, list) or len(v) < 2: + msg = f"GetAtt {v} invalid" + raise exceptions.InvalidModuleError(msg=msg) + if v[0] == self.name and v[1] in self.outputs: + output = self.outputs[v[1]] + if GETATT in output: + getatt = output[GETATT] + d[n] = {GETATT: [self.name + getatt[0], getatt[1]]} + elif SUB in output: + # Parse the Sub in the module output + words = parse_sub(output[SUB], True) + sub = "" + for word in words: + if word.t == WordType.STR: + sub += word.w + elif word.t == WordType.AWS: + sub += "${AWS::" + word.w + "}" + elif word.t == WordType.REF: + # This is a ref to a param or resource + # TODO: If it's a ref to a param...? is this allowed? + # If it's a resource, concatenante the name + resolved = "${" + word.w + "}" + if word.w in self.resources: + resolved = "${" + self.name + word.w + "}" + sub += resolved + elif word.t == WordType.GETATT: + resolved = "${" + word.w + "}" + tokens = word.w.split(".") + if len(tokens) != 2: + msg = f"GetAtt {word.w} unexpected length" + raise exceptions.InvalidModuleError(msg=msg) + if tokens[0] in self.resources: + resolved = "${" + self.name + word.w + "}" + sub += resolved + + d[n] = {SUB: sub} + def validate_overrides(self): "Make sure resources referenced by overrides actually exist" for logical_id in self.overrides: @@ -500,12 +622,15 @@ def resolve_sub(self, v, d, n): sub += resolved elif word.t == WordType.GETATT: need_sub = True - tokens = word.w.split() - if len(tokens) != 2: - msg = "GetAtt {word.w} has unexpected number of tokens" + resolved = "${" + word.w + "}" + tokens = word.w.split(".") + if len(tokens) < 2: + msg = f"GetAtt {word.w} has unexpected number of tokens" raise exceptions.InvalidModuleError(msg=msg) if tokens[0] in self.resources: tokens[0] = self.name + tokens[0] + resolved = "${" + tokens[0] + "." + tokens[1] + "}" + sub += resolved if need_sub: d[n] = {SUB: sub} diff --git a/tests/unit/customizations/cloudformation/modules/output-expect.yaml b/tests/unit/customizations/cloudformation/modules/output-expect.yaml new file mode 100644 index 000000000000..a4ebb8fe8ea5 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/output-expect.yaml @@ -0,0 +1,17 @@ +Outputs: + ExampleOutput: + Value: + Fn::GetAtt: + - ContentBucket + - Arn + ExampleSub: + Value: + Fn::Sub: ${ContentBucket.Arn} + ExampleGetSub: + Value: + Fn::Sub: ${ContentBucket.Arn} +Resources: + ContentBucket: + Type: AWS::S3::Bucket + Properties: + BucketName: foo diff --git a/tests/unit/customizations/cloudformation/modules/output-module.yaml b/tests/unit/customizations/cloudformation/modules/output-module.yaml new file mode 100644 index 000000000000..f685ca154d84 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/output-module.yaml @@ -0,0 +1,12 @@ +Parameters: + Name: + Type: String +Resources: + Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: !Ref Name +Outputs: + BucketArn: !GetAtt Bucket.Arn + BucketArnSub: !Sub ${Bucket.Arn} + diff --git a/tests/unit/customizations/cloudformation/modules/output-template.yaml b/tests/unit/customizations/cloudformation/modules/output-template.yaml new file mode 100644 index 000000000000..872d3423597b --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/output-template.yaml @@ -0,0 +1,12 @@ +Modules: + Content: + Source: ./output-module.yaml + Properties: + Name: foo +Outputs: + ExampleOutput: + Value: !GetAtt Content.BucketArn + ExampleSub: + Value: !Sub ${Content.BucketArn} + ExampleGetSub: + Value: !GetAtt Content.BucketArnSub diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 51969358220e..bbd610d3b8b1 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -82,7 +82,7 @@ def test_main(self): # The tests are in the modules directory. # Each test has 3 files: # test-template.yaml, test-module.yaml, and test-expect.yaml - tests = ["basic", "type", "sub", "modinmod"] + tests = ["basic", "type", "sub", "modinmod", "output"] for test in tests: base = "unit/customizations/cloudformation/modules" t = modules.read_source(f"{base}/{test}-template.yaml") From e5177201cfe0dc6c58ebad40b9612b11f094cd2a Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Fri, 13 Dec 2024 09:34:46 -0800 Subject: [PATCH 14/26] Handle spaces in parse_sub --- awscli/customizations/cloudformation/modules.py | 2 +- awscli/customizations/cloudformation/parse_sub.py | 5 +++++ tests/unit/customizations/cloudformation/test_modules.py | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index e57ffcc98f1c..fd39cc47e809 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -287,7 +287,7 @@ def process_outputs(self): Fix parent template output references. In the parent you can !GetAtt ModuleName.OutputName - This will be converted to !GetAtt ModuleName + OutputValue + This will be converted so that it's correct in the packaged template. Recurse over all sections in the parent template looking for GetAtts and Subs that reference a module output value. diff --git a/awscli/customizations/cloudformation/parse_sub.py b/awscli/customizations/cloudformation/parse_sub.py index c05eb393adea..8b5ef8a6c78d 100644 --- a/awscli/customizations/cloudformation/parse_sub.py +++ b/awscli/customizations/cloudformation/parse_sub.py @@ -19,6 +19,7 @@ OPEN = '{' CLOSE = '}' BANG = '!' +SPACE = ' ' class WordType: "Word type enumeration" @@ -106,6 +107,10 @@ def parse_sub(sub_str, leave_bang=False): else: # This is a ! somewhere not related to a LITERAL buf += char + elif char == SPACE: + # Ignore spaces around Refs. ${ ABC } == ${ABC} + if state != State.READVAR: + buf += char else: if state == State.MAYBE: buf += last # Put the $ back on the buffer diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index bbd610d3b8b1..903ebf794ae8 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -42,6 +42,9 @@ def test_parse_sub(self): SubWord(WordType.REF, "ABC$XYZ"), SubWord(WordType.STR, "FOO$BAR"), ], + "${ ABC }": [SubWord(WordType.REF, "ABC")], + "${ ABC }": [SubWord(WordType.REF, "ABC")], + " ABC ": [SubWord(WordType.STR, " ABC ")], } for sub, expect in cases.items(): From e8e6d96fd8f12554fe34c77cd97b4fbb2a02abcb Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Fri, 13 Dec 2024 09:37:29 -0800 Subject: [PATCH 15/26] Enum --- awscli/customizations/cloudformation/parse_sub.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/awscli/customizations/cloudformation/parse_sub.py b/awscli/customizations/cloudformation/parse_sub.py index 8b5ef8a6c78d..28d6b9ed3516 100644 --- a/awscli/customizations/cloudformation/parse_sub.py +++ b/awscli/customizations/cloudformation/parse_sub.py @@ -14,6 +14,8 @@ """ #pylint: disable=too-few-public-methods +from enum import Enum + DATA = ' ' # Any other character DOLLAR = '$' OPEN = '{' @@ -21,14 +23,14 @@ BANG = '!' SPACE = ' ' -class WordType: +class WordType(Enum): "Word type enumeration" STR = 0 # A literal string fragment REF = 1 # ${ParamOrResourceName} AWS = 2 # ${AWS::X} GETATT = 3 # ${X.Y} -class State: +class State(Enum): "State machine enumeration" READSTR = 0 READVAR = 1 From 5b0ee496a250a66c9335c3f66c738d2ac51c7f72 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Fri, 13 Dec 2024 10:11:09 -0800 Subject: [PATCH 16/26] Code review fixes --- .../cloudformation/artifact_exporter.py | 5 ++--- awscli/customizations/cloudformation/modules.py | 14 ++++++++++---- awscli/customizations/cloudformation/parse_sub.py | 4 +--- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 76c42852d1c9..7c6b7693db50 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -14,7 +14,6 @@ import logging import os import tempfile -import traceback import zipfile import contextlib import uuid @@ -661,8 +660,8 @@ def export(self): self.template_dict, self.template_dir) except Exception as e: - traceback.print_exc() msg=f"Failed to process Modules section: {e}" + LOG.exception(msg) raise exceptions.InvalidModuleError(msg=msg) self.template_dict = self.export_metadata(self.template_dict) @@ -676,8 +675,8 @@ def export(self): self.template_dict, self.template_dir) except Exception as e: - traceback.print_exc() msg=f"Failed to process modules in Resources: {e}" + LOG.exception(msg) raise exceptions.InvalidModuleError(msg=msg) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index fd39cc47e809..9d07881fc65c 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -15,8 +15,8 @@ # pylint: disable=fixme,too-many-instance-attributes +import logging import os -import traceback from collections import OrderedDict from awscli.customizations.cloudformation import exceptions @@ -25,6 +25,8 @@ from awscli.customizations.cloudformation.parse_sub import WordType from awscli.customizations.cloudformation.parse_sub import parse_sub +LOG = logging.getLogger(__name__) + RESOURCES = "Resources" METADATA = "Metadata" OVERRIDES = "Overrides" @@ -265,12 +267,12 @@ def process(self): section = "" try: section = MODULES - module_dict = process_module_section(module_dict, base_path) + process_module_section(module_dict, base_path) section = RESOURCES - module_dict = process_resources_section(module_dict, base_path) + process_resources_section(module_dict, base_path) except Exception as e: - traceback.print_exc() msg = f"Failed to process {section} section: {e}" + LOG.exception(msg) raise exceptions.InvalidModuleError(msg=msg) self.validate_overrides() @@ -359,6 +361,7 @@ def resolve_output_sub(self, v, d, n): d[n] = {SUB: sub} + # pylint:disable=too-many-branches def resolve_output_getatt(self, v, d, n): "Resolve a GetAtt that refers to a module output" if not isinstance(v, list) or len(v) < 2: @@ -368,6 +371,9 @@ def resolve_output_getatt(self, v, d, n): output = self.outputs[v[1]] if GETATT in output: getatt = output[GETATT] + if len(getatt) < 2: + msg = f"GetAtt {getatt} in Output {v[1]} is invalid" + raise exceptions.InvalidModuleError(msg=msg) d[n] = {GETATT: [self.name + getatt[0], getatt[1]]} elif SUB in output: # Parse the Sub in the module output diff --git a/awscli/customizations/cloudformation/parse_sub.py b/awscli/customizations/cloudformation/parse_sub.py index 28d6b9ed3516..f096d70b4812 100644 --- a/awscli/customizations/cloudformation/parse_sub.py +++ b/awscli/customizations/cloudformation/parse_sub.py @@ -57,10 +57,8 @@ def parse_sub(sub_str, leave_bang=False): words = [] state = State.READSTR buf = '' - i = -1 last = '' - for char in sub_str: - i += 1 + for i, char in enumerate(sub_str): if char == DOLLAR: if state != State.READVAR: state = State.MAYBE From 4eef2c45f627701a6811b535580fb29d0c98fbaa Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Fri, 13 Dec 2024 10:31:24 -0800 Subject: [PATCH 17/26] Code review fixes --- awscli/customizations/cloudformation/modules.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 9d07881fc65c..ceba972a2ce1 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -54,6 +54,10 @@ def process_module_section(template, base_path): "Recursively process the Modules section of a template" if MODULES in template: + if not isdict(template[MODULES]): + msg = "Modules section is invalid" + raise exceptions.InvalidModuleError(msg=msg) + # Process each Module node separately for module_name, module_config in template[MODULES].items(): module_config[NAME] = module_name @@ -192,6 +196,9 @@ class Module: that are actually strings and leaving others to be resolved at deploy time. Modules can contain other modules, with no limit to the levels of nesting. + + Modules can define Outputs, which are key-value pairs that can be + referenced by the parent. """ def __init__(self, template, module_config): @@ -346,7 +353,7 @@ def resolve_output_sub(self, v, d, n): elif word.t == WordType.GETATT: resolved = "${" + word.w + "}" tokens = word.w.split(".") - if len(tokens) != 2: + if len(tokens) < 2: msg = f"GetAtt {word.w} has unexpected number of tokens" raise exceptions.InvalidModuleError(msg=msg) # !Sub ${Content.BucketArn} -> !Sub ${ContentBucket.Arn} @@ -395,7 +402,7 @@ def resolve_output_getatt(self, v, d, n): elif word.t == WordType.GETATT: resolved = "${" + word.w + "}" tokens = word.w.split(".") - if len(tokens) != 2: + if len(tokens) < 2: msg = f"GetAtt {word.w} unexpected length" raise exceptions.InvalidModuleError(msg=msg) if tokens[0] in self.resources: @@ -432,6 +439,7 @@ def process_resource(self, logical_id, resource): # Resolve refs, subs, and getatts # (Process module Parameters and parent Properties) container = {} + # We need the container for the first iteration of the recursion container[RESOURCES] = self.resources self.resolve(logical_id, resource, container, RESOURCES) From 72659f5e01de5d4cd809f3367edde6e737df3712 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Mon, 16 Dec 2024 11:07:27 -0800 Subject: [PATCH 18/26] Fix merging lists --- .../cloudformation/artifact_exporter.py | 7 ++++-- .../customizations/cloudformation/modules.py | 23 ++++++++++++++----- .../cloudformation/modules/policy-expect.yaml | 16 +++++++++++++ .../cloudformation/modules/policy-module.yaml | 19 +++++++++++++++ .../modules/policy-template.yaml | 13 +++++++++++ .../cloudformation/test_modules.py | 2 +- 6 files changed, 71 insertions(+), 9 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/policy-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/policy-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/policy-template.yaml diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 7c6b7693db50..aa143e389028 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -595,6 +595,7 @@ def __init__(self, template_path, parent_dir, uploader, "an absolute path to a folder {0}" .format(parent_dir)) abs_template_path = make_abs_path(parent_dir, template_path) + self.module_parent_path = abs_template_path template_dir = os.path.dirname(abs_template_path) with open(abs_template_path, "r") as handle: @@ -658,7 +659,8 @@ def export(self): try: self.template_dict = modules.process_module_section( self.template_dict, - self.template_dir) + self.template_dir, + self.module_parent_path) except Exception as e: msg=f"Failed to process Modules section: {e}" LOG.exception(msg) @@ -673,7 +675,8 @@ def export(self): try: self.template_dict = modules.process_resources_section( self.template_dict, - self.template_dir) + self.template_dir, + self.module_parent_path) except Exception as e: msg=f"Failed to process modules in Resources: {e}" LOG.exception(msg) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index ceba972a2ce1..455ea062d70c 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -50,10 +50,9 @@ OUTPUTS = "Outputs" -def process_module_section(template, base_path): +def process_module_section(template, base_path, parent_path): "Recursively process the Modules section of a template" if MODULES in template: - if not isdict(template[MODULES]): msg = "Modules section is invalid" raise exceptions.InvalidModuleError(msg=msg) @@ -65,6 +64,9 @@ def process_module_section(template, base_path): relative_path = module_config[SOURCE] module_config[SOURCE] = os.path.join(base_path, relative_path) module_config[SOURCE] = os.path.normpath(module_config[SOURCE]) + if module_config[SOURCE] == parent_path: + msg = f"Module refers to itself: {parent_path}" + raise exceptions.InvalidModuleError(msg=msg) module = Module(template, module_config) template = module.process() @@ -74,7 +76,7 @@ def process_module_section(template, base_path): return template -def process_resources_section(template, base_path): +def process_resources_section(template, base_path, parent_path): "Recursively process the Resources section of the template" for k, v in template[RESOURCES].copy().items(): if TYPE in v and v[TYPE] == LOCAL_MODULE: @@ -86,6 +88,9 @@ def process_resources_section(template, base_path): relative_path = v[SOURCE] module_config[SOURCE] = os.path.join(base_path, relative_path) module_config[SOURCE] = os.path.normpath(module_config[SOURCE]) + if module_config[SOURCE] == parent_path: + msg = f"Module refers to itself: {parent_path}" + raise exceptions.InvalidModuleError(msg=msg) if PROPERTIES in v: module_config[PROPERTIES] = v[PROPERTIES] if OVERRIDES in v: @@ -158,7 +163,13 @@ def merge_props(original, overrides): retval[k] = overrides[k] return retval - return original + overrides + # original and overrides are lists + new_list = [] + for item in original: + new_list.append(item) + for item in overrides: + new_list.append(item) + return new_list class Module: @@ -274,9 +285,9 @@ def process(self): section = "" try: section = MODULES - process_module_section(module_dict, base_path) + process_module_section(module_dict, base_path, self.source) section = RESOURCES - process_resources_section(module_dict, base_path) + process_resources_section(module_dict, base_path, self.source) except Exception as e: msg = f"Failed to process {section} section: {e}" LOG.exception(msg) diff --git a/tests/unit/customizations/cloudformation/modules/policy-expect.yaml b/tests/unit/customizations/cloudformation/modules/policy-expect.yaml new file mode 100644 index 000000000000..8d524c1fb4ce --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/policy-expect.yaml @@ -0,0 +1,16 @@ +Resources: + AccessPolicy: + Type: AWS::IAM::RolePolicy + Properties: + PolicyDocument: + Statement: + - Effect: ALLOW + Action: s3:List* + Resource: + - arn:aws:s3:::foo + - arn:aws:s3:::foo/* + - Effect: DENY + Action: s3:Put* + Resource: arn:aws:s3:::bar + PolicyName: my-policy + RoleName: foo diff --git a/tests/unit/customizations/cloudformation/modules/policy-module.yaml b/tests/unit/customizations/cloudformation/modules/policy-module.yaml new file mode 100644 index 000000000000..7de41d792584 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/policy-module.yaml @@ -0,0 +1,19 @@ +Parameters: + Role: + Type: String + BucketName: + Default: foo +Resources: + Policy: + Type: AWS::IAM::RolePolicy + Properties: + PolicyDocument: + Statement: + - Effect: ALLOW + Action: s3:List* + Resource: + - !Sub arn:aws:s3:::${BucketName} + - !Sub arn:aws:s3:::${BucketName}/* + PolicyName: my-policy + RoleName: !Ref Role + diff --git a/tests/unit/customizations/cloudformation/modules/policy-template.yaml b/tests/unit/customizations/cloudformation/modules/policy-template.yaml new file mode 100644 index 000000000000..9f587f7bda9d --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/policy-template.yaml @@ -0,0 +1,13 @@ +Modules: + Access: + Source: ./policy-module.yaml + Properties: + Role: foo + Overrides: + Policy: + Properties: + PolicyDocument: + Statement: + - Effect: DENY + Action: s3:Put* + Resource: arn:aws:s3:::bar diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 903ebf794ae8..78bc3e697555 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -85,7 +85,7 @@ def test_main(self): # The tests are in the modules directory. # Each test has 3 files: # test-template.yaml, test-module.yaml, and test-expect.yaml - tests = ["basic", "type", "sub", "modinmod", "output"] + tests = ["basic", "type", "sub", "modinmod", "output", "policy"] for test in tests: base = "unit/customizations/cloudformation/modules" t = modules.read_source(f"{base}/{test}-template.yaml") From 5cb126cc0f29c94b88fd91eaa1b1c3c17f8fb888 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Tue, 17 Dec 2024 18:30:10 -0800 Subject: [PATCH 19/26] Vpc module example with a map --- .../cloudformation/artifact_exporter.py | 3 +- .../customizations/cloudformation/modules.py | 150 +++++++---- .../cloudformation/modules/subnet-module.yaml | 59 +++++ .../cloudformation/modules/vpc-expect.yaml | 236 ++++++++++++++++++ .../cloudformation/modules/vpc-module.yaml | 37 +++ .../cloudformation/modules/vpc-template.yaml | 9 + .../cloudformation/test_modules.py | 2 +- 7 files changed, 440 insertions(+), 56 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/subnet-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/vpc-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/vpc-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/vpc-template.yaml diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index aa143e389028..e9137efbf31d 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -676,7 +676,8 @@ def export(self): self.template_dict = modules.process_resources_section( self.template_dict, self.template_dir, - self.module_parent_path) + self.module_parent_path, + None) except Exception as e: msg=f"Failed to process modules in Resources: {e}" LOG.exception(msg) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 455ea062d70c..2a0248404a06 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -15,6 +15,7 @@ # pylint: disable=fixme,too-many-instance-attributes +import copy import logging import os from collections import OrderedDict @@ -48,6 +49,9 @@ TYPE = "Type" LOCAL_MODULE = "LocalModule" OUTPUTS = "Outputs" +MAP = "Map" +MAP_PLACEHOLDER = "$MapValue" +INDEX_PLACEHOLDER = "$MapIndex" def process_module_section(template, base_path, parent_path): @@ -76,26 +80,64 @@ def process_module_section(template, base_path, parent_path): return template -def process_resources_section(template, base_path, parent_path): +def make_module(template, name, config, base_path, parent_path): + "Create an instance of a module based on a template and the module config" + module_config = {} + module_config[NAME] = name + if SOURCE not in config: + msg = f"{name} missing {SOURCE}" + raise exceptions.InvalidModulePathError(msg=msg) + relative_path = config[SOURCE] + module_config[SOURCE] = os.path.join(base_path, relative_path) + module_config[SOURCE] = os.path.normpath(module_config[SOURCE]) + if module_config[SOURCE] == parent_path: + msg = f"Module refers to itself: {parent_path}" + raise exceptions.InvalidModuleError(msg=msg) + if PROPERTIES in config: + module_config[PROPERTIES] = config[PROPERTIES] + if OVERRIDES in config: + module_config[OVERRIDES] = config[OVERRIDES] + return Module(template, module_config) + + +# pylint: disable=too-many-locals,too-many-nested-blocks +def process_resources_section(template, base_path, parent_path, parent_module): "Recursively process the Resources section of the template" for k, v in template[RESOURCES].copy().items(): if TYPE in v and v[TYPE] == LOCAL_MODULE: - module_config = {} - module_config[NAME] = k - if SOURCE not in v: - msg = f"{k} missing {SOURCE}" - raise exceptions.InvalidModulePathError(msg=msg) - relative_path = v[SOURCE] - module_config[SOURCE] = os.path.join(base_path, relative_path) - module_config[SOURCE] = os.path.normpath(module_config[SOURCE]) - if module_config[SOURCE] == parent_path: - msg = f"Module refers to itself: {parent_path}" - raise exceptions.InvalidModuleError(msg=msg) - if PROPERTIES in v: - module_config[PROPERTIES] = v[PROPERTIES] - if OVERRIDES in v: - module_config[OVERRIDES] = v[OVERRIDES] - module = Module(template, module_config) + # First, pre-process local modules that are looping over a list + if MAP in v: + # Expect Map to be a CSV or ref to a CSV + m = v[MAP] + if isdict(m) and REF in m: + if parent_module is None: + msg = "Map is only valid in a module" + raise exceptions.InvalidModuleError(msg=msg) + # TODO: We should be able to fake up a parent module + m = parent_module.find_ref(m[REF]) + if m is None: + msg = f"{k} has an invalid Map Ref" + raise exceptions.InvalidModuleError(msg=msg) + tokens = m.split(",") # TODO - use an actual csv parser? + for i, token in enumerate(tokens): + # Make a new resource + logical_id = f"{k}{i}" + resource = copy.deepcopy(v) + del resource[MAP] + # Replace $Map and $Index placeholders + for prop, val in resource[PROPERTIES].copy().items(): + if val == MAP_PLACEHOLDER: + resource[PROPERTIES][prop] = token + if val == INDEX_PLACEHOLDER: + resource[PROPERTIES][prop] = f"{i}" + template[RESOURCES][logical_id] = resource + + del template[RESOURCES][k] + + # Start over after pre-processing maps + for k, v in template[RESOURCES].copy().items(): + if TYPE in v and v[TYPE] == LOCAL_MODULE: + module = make_module(template, k, v, base_path, parent_path) template = module.process() del template[RESOURCES][k] return template @@ -281,13 +323,13 @@ def process(self): self.outputs = module_dict[OUTPUTS] # Recurse on nested modules - base_path = os.path.dirname(self.source) + bp = os.path.dirname(self.source) section = "" try: section = MODULES - process_module_section(module_dict, base_path, self.source) + process_module_section(module_dict, bp, self.source) section = RESOURCES - process_resources_section(module_dict, base_path, self.source) + process_resources_section(module_dict, bp, self.source, self) except Exception as e: msg = f"Failed to process {section} section: {e}" LOG.exception(msg) @@ -578,40 +620,6 @@ def resolve_ref(self, v, d, n): if found is not None: d[n] = found - def find_ref(self, v): - """ - Find a Ref. - - A Ref might be to a module Parameter with a matching parent - template Property, or a Parameter Default. It could also - be a reference to another resource in this module. - - :return The referenced element or None - """ - # print(f"find_ref {v}, props: {self.props}") - if v in self.props: - if v not in self.params: - # The parent tried to set a property that doesn't exist - # in the Parameters section of this module - msg = f"{v} not found in module Parameters: {self.source}" - raise exceptions.InvalidModuleError(msg=msg) - return self.props[v] - - if v in self.params: - param = self.params[v] - if DEFAULT in param: - # Use the default value of the Parameter - return param[DEFAULT] - msg = f"{v} does not have a Default and is not a Property" - raise exceptions.InvalidModuleError(msg=msg) - - for k in self.resources: - if v == k: - # Simply rename local references to include the module name - return {REF: self.name + v} - - return None - # pylint: disable=too-many-branches,unused-argument def resolve_sub(self, v, d, n): """ @@ -673,3 +681,37 @@ def resolve_getatt(self, v, d, n): raise exceptions.InvalidModuleError(msg=msg) logical_id = self.name + v[0] d[n] = {GETATT: [logical_id, v[1]]} + + def find_ref(self, v): + """ + Find a Ref. + + A Ref might be to a module Parameter with a matching parent + template Property, or a Parameter Default. It could also + be a reference to another resource in this module. + + :return The referenced element or None + """ + # print(f"find_ref {v}, props: {self.props}, params: {self.params}") + if v in self.props: + if v not in self.params: + # The parent tried to set a property that doesn't exist + # in the Parameters section of this module + msg = f"{v} not found in module Parameters: {self.source}" + raise exceptions.InvalidModuleError(msg=msg) + return self.props[v] + + if v in self.params: + param = self.params[v] + if DEFAULT in param: + # Use the default value of the Parameter + return param[DEFAULT] + msg = f"{v} does not have a Default and is not a Property" + raise exceptions.InvalidModuleError(msg=msg) + + for k in self.resources: + if v == k: + # Simply rename local references to include the module name + return {REF: self.name + v} + + return None diff --git a/tests/unit/customizations/cloudformation/modules/subnet-module.yaml b/tests/unit/customizations/cloudformation/modules/subnet-module.yaml new file mode 100644 index 000000000000..2291bd6b0c7e --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/subnet-module.yaml @@ -0,0 +1,59 @@ +Parameters: + + AZSelection: + Type: Number + + SubnetCidrBlock: + Type: String + +Resources: + + Subnet: + Type: AWS::EC2::Subnet + Metadata: + guard: + SuppressedRules: + - SUBNET_AUTO_ASSIGN_PUBLIC_IP_DISABLED + Properties: + AvailabilityZone: !Select [!Ref AZSelection, Fn::GetAZs: !Ref "AWS::Region"] + CidrBlock: !Ref SubnetCidrBlock + MapIpOnLaunch: true + VpcId: !Ref VPC + + RouteTable: + Type: AWS::EC2::RouteTable + Properties: + VpcId: !Ref VPC + + RouteTableAssociation: + Type: AWS::EC2::SubnetRouteTableAssociation + Properties: + RouteTableId: !Ref SubnetRouteTable + SubnetId: !Ref Subnet + + DefaultRoute: + Type: AWS::EC2::Route + Metadata: + guard: + SuppressedRules: + - NO_UNRESTRICTED_ROUTE_TO_IGW + Properties: + DestinationCidrBlock: 0.0.0.0/0 + GatewayId: !Ref InternetGateway + RouteTableId: !Ref SubnetRouteTable + DependsOn: VPCGW + + EIP: + Type: AWS::EC2::EIP + Properties: + Domain: vpc + + NATGateway: + Type: AWS::EC2::NatGateway + Properties: + AllocationId: !GetAtt SubnetEIP.AllocationId + SubnetId: !Ref Subnet + DependsOn: + - SubnetDefaultRoute + - SubnetRouteTableAssociation + diff --git a/tests/unit/customizations/cloudformation/modules/vpc-expect.yaml b/tests/unit/customizations/cloudformation/modules/vpc-expect.yaml new file mode 100644 index 000000000000..0d43ced5bd2b --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/vpc-expect.yaml @@ -0,0 +1,236 @@ +Resources: + NetworkVPC: + Type: AWS::EC2::VPC + Properties: + CidrBlock: 10.0.0.0/16 + EnableDnsHostnames: true + EnableDnsSupport: true + InstanceTenancy: default + NetworkPublicSubnet0Subnet: + Type: AWS::EC2::Subnet + Metadata: + guard: + SuppressedRules: + - SUBNET_AUTO_ASSIGN_PUBLIC_IP_DISABLED + Properties: + AvailabilityZone: + Fn::Select: + - '0' + - Fn::GetAZs: + Ref: AWS::Region + CidrBlock: 10.0.128.0/18 + MapIpOnLaunch: true + VpcId: + Ref: NetworkVPC + NetworkPublicSubnet0RouteTable: + Type: AWS::EC2::RouteTable + Properties: + VpcId: + Ref: NetworkVPC + NetworkPublicSubnet0RouteTableAssociation: + Type: AWS::EC2::SubnetRouteTableAssociation + Properties: + RouteTableId: + Ref: SubnetRouteTable + SubnetId: + Ref: NetworkPublicSubnet0Subnet + NetworkPublicSubnet0DefaultRoute: + Type: AWS::EC2::Route + Metadata: + guard: + SuppressedRules: + - NO_UNRESTRICTED_ROUTE_TO_IGW + Properties: + DestinationCidrBlock: 0.0.0.0/0 + GatewayId: + Ref: InternetGateway + RouteTableId: + Ref: SubnetRouteTable + DependsOn: VPCGW + NetworkPublicSubnet0EIP: + Type: AWS::EC2::EIP + Properties: + Domain: vpc + NetworkPublicSubnet0NATGateway: + Type: AWS::EC2::NatGateway + Properties: + AllocationId: + Fn::GetAtt: + - NetworkPublicSubnet0SubnetEIP + - AllocationId + SubnetId: + Ref: NetworkPublicSubnet0Subnet + DependsOn: + - SubnetDefaultRoute + - SubnetRouteTableAssociation + NetworkPublicSubnet1Subnet: + Type: AWS::EC2::Subnet + Metadata: + guard: + SuppressedRules: + - SUBNET_AUTO_ASSIGN_PUBLIC_IP_DISABLED + Properties: + AvailabilityZone: + Fn::Select: + - '1' + - Fn::GetAZs: + Ref: AWS::Region + CidrBlock: 10.0.192.0/18 + MapIpOnLaunch: true + VpcId: + Ref: NetworkVPC + NetworkPublicSubnet1RouteTable: + Type: AWS::EC2::RouteTable + Properties: + VpcId: + Ref: NetworkVPC + NetworkPublicSubnet1RouteTableAssociation: + Type: AWS::EC2::SubnetRouteTableAssociation + Properties: + RouteTableId: + Ref: SubnetRouteTable + SubnetId: + Ref: NetworkPublicSubnet1Subnet + NetworkPublicSubnet1DefaultRoute: + Type: AWS::EC2::Route + Metadata: + guard: + SuppressedRules: + - NO_UNRESTRICTED_ROUTE_TO_IGW + Properties: + DestinationCidrBlock: 0.0.0.0/0 + GatewayId: + Ref: InternetGateway + RouteTableId: + Ref: SubnetRouteTable + DependsOn: VPCGW + NetworkPublicSubnet1EIP: + Type: AWS::EC2::EIP + Properties: + Domain: vpc + NetworkPublicSubnet1NATGateway: + Type: AWS::EC2::NatGateway + Properties: + AllocationId: + Fn::GetAtt: + - NetworkPublicSubnet1SubnetEIP + - AllocationId + SubnetId: + Ref: NetworkPublicSubnet1Subnet + DependsOn: + - SubnetDefaultRoute + - SubnetRouteTableAssociation + NetworkPrivateSubnet0Subnet: + Type: AWS::EC2::Subnet + Metadata: + guard: + SuppressedRules: + - SUBNET_AUTO_ASSIGN_PUBLIC_IP_DISABLED + Properties: + AvailabilityZone: + Fn::Select: + - '0' + - Fn::GetAZs: + Ref: AWS::Region + CidrBlock: 10.0.0.0/18 + MapIpOnLaunch: true + VpcId: + Ref: NetworkVPC + NetworkPrivateSubnet0RouteTable: + Type: AWS::EC2::RouteTable + Properties: + VpcId: + Ref: NetworkVPC + NetworkPrivateSubnet0RouteTableAssociation: + Type: AWS::EC2::SubnetRouteTableAssociation + Properties: + RouteTableId: + Ref: SubnetRouteTable + SubnetId: + Ref: NetworkPrivateSubnet0Subnet + NetworkPrivateSubnet0DefaultRoute: + Type: AWS::EC2::Route + Metadata: + guard: + SuppressedRules: + - NO_UNRESTRICTED_ROUTE_TO_IGW + Properties: + DestinationCidrBlock: 0.0.0.0/0 + GatewayId: + Ref: InternetGateway + RouteTableId: + Ref: SubnetRouteTable + DependsOn: VPCGW + NetworkPrivateSubnet0EIP: + Type: AWS::EC2::EIP + Properties: + Domain: vpc + NetworkPrivateSubnet0NATGateway: + Type: AWS::EC2::NatGateway + Properties: + AllocationId: + Fn::GetAtt: + - NetworkPrivateSubnet0SubnetEIP + - AllocationId + SubnetId: + Ref: NetworkPrivateSubnet0Subnet + DependsOn: + - SubnetDefaultRoute + - SubnetRouteTableAssociation + NetworkPrivateSubnet1Subnet: + Type: AWS::EC2::Subnet + Metadata: + guard: + SuppressedRules: + - SUBNET_AUTO_ASSIGN_PUBLIC_IP_DISABLED + Properties: + AvailabilityZone: + Fn::Select: + - '1' + - Fn::GetAZs: + Ref: AWS::Region + CidrBlock: 10.0.64.0/18 + MapIpOnLaunch: true + VpcId: + Ref: NetworkVPC + NetworkPrivateSubnet1RouteTable: + Type: AWS::EC2::RouteTable + Properties: + VpcId: + Ref: NetworkVPC + NetworkPrivateSubnet1RouteTableAssociation: + Type: AWS::EC2::SubnetRouteTableAssociation + Properties: + RouteTableId: + Ref: SubnetRouteTable + SubnetId: + Ref: NetworkPrivateSubnet1Subnet + NetworkPrivateSubnet1DefaultRoute: + Type: AWS::EC2::Route + Metadata: + guard: + SuppressedRules: + - NO_UNRESTRICTED_ROUTE_TO_IGW + Properties: + DestinationCidrBlock: 0.0.0.0/0 + GatewayId: + Ref: InternetGateway + RouteTableId: + Ref: SubnetRouteTable + DependsOn: VPCGW + NetworkPrivateSubnet1EIP: + Type: AWS::EC2::EIP + Properties: + Domain: vpc + NetworkPrivateSubnet1NATGateway: + Type: AWS::EC2::NatGateway + Properties: + AllocationId: + Fn::GetAtt: + - NetworkPrivateSubnet1SubnetEIP + - AllocationId + SubnetId: + Ref: NetworkPrivateSubnet1Subnet + DependsOn: + - SubnetDefaultRoute + - SubnetRouteTableAssociation diff --git a/tests/unit/customizations/cloudformation/modules/vpc-module.yaml b/tests/unit/customizations/cloudformation/modules/vpc-module.yaml new file mode 100644 index 000000000000..a33c4e29aada --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/vpc-module.yaml @@ -0,0 +1,37 @@ +Parameters: + + CidrBlock: + Type: String + + PrivateCidrBlocks: + Type: CommaDelimitedList + + PublicCidrBlocks: + Type: CommaDelimitedList + +Resources: + + VPC: + Type: AWS::EC2::VPC + Properties: + CidrBlock: !Ref CidrBlock + EnableDnsHostnames: true + EnableDnsSupport: true + InstanceTenancy: default + + PublicSubnet: + Type: LocalModule + Map: !Ref PublicCidrBlocks + Source: ./subnet-module.yaml + Properties: + SubnetCidrBlock: $MapValue + AZSelection: $MapIndex + + PrivateSubnet: + Type: LocalModule + Map: !Ref PrivateCidrBlocks + Source: ./subnet-module.yaml + Properties: + SubnetCidrBlock: $MapValue + AZSelection: $MapIndex + diff --git a/tests/unit/customizations/cloudformation/modules/vpc-template.yaml b/tests/unit/customizations/cloudformation/modules/vpc-template.yaml new file mode 100644 index 000000000000..92e512b57d67 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/vpc-template.yaml @@ -0,0 +1,9 @@ +Resources: + + Network: + Type: LocalModule + Source: ./vpc-module.yaml + Properties: + CidrBlock: 10.0.0.0/16 + PrivateCidrBlocks: 10.0.0.0/18,10.0.64.0/18 + PublicCidrBlocks: 10.0.128.0/18,10.0.192.0/18 diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 78bc3e697555..f0ac75b11ffb 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -85,7 +85,7 @@ def test_main(self): # The tests are in the modules directory. # Each test has 3 files: # test-template.yaml, test-module.yaml, and test-expect.yaml - tests = ["basic", "type", "sub", "modinmod", "output", "policy"] + tests = ["basic", "type", "sub", "modinmod", "output", "policy", "vpc"] for test in tests: base = "unit/customizations/cloudformation/modules" t = modules.read_source(f"{base}/{test}-template.yaml") From 3ac0577e8366849cf10b35ab160f2d04725e8e77 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 18 Dec 2024 13:28:30 -0800 Subject: [PATCH 20/26] Use Map in the parent template --- .../customizations/cloudformation/modules.py | 62 ++++++++++++++----- .../cloudformation/modules/map-expect.yaml | 17 +++++ .../cloudformation/modules/map-module.yaml | 9 +++ .../cloudformation/modules/map-template.yaml | 12 ++++ .../cloudformation/test_modules.py | 35 ++++------- 5 files changed, 95 insertions(+), 40 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/map-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/map-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/map-template.yaml diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 2a0248404a06..7e1cf64e5d15 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -100,9 +100,38 @@ def make_module(template, name, config, base_path, parent_path): return Module(template, module_config) +def map_placeholders(i, token, val): + "Replace $MapValue and $MapIndex" + if SUB in val: + sub = val[SUB] + r = sub.replace(MAP_PLACEHOLDER, token) + r = r.replace(INDEX_PLACEHOLDER, f"{i}") + words = parse_sub(r) + need_sub = False + for word in words: + if word.t != WordType.STR: + need_sub = True + break + if need_sub: + return {SUB: r} + return r + r = val.replace(MAP_PLACEHOLDER, token) + r = r.replace(INDEX_PLACEHOLDER, f"{i}") + return r + + # pylint: disable=too-many-locals,too-many-nested-blocks def process_resources_section(template, base_path, parent_path, parent_module): "Recursively process the Resources section of the template" + if parent_module is None: + # Make a fake Module instance to handle find_ref for Maps + # The only valid way to do this at the template level + # is to specify a default for a Parameter, since we need to + # resolve the actual value client-side + parent_module = Module(template, {NAME: "", SOURCE: ""}) + if PARAMETERS in template: + parent_module.params = template[PARAMETERS] + for k, v in template[RESOURCES].copy().items(): if TYPE in v and v[TYPE] == LOCAL_MODULE: # First, pre-process local modules that are looping over a list @@ -113,7 +142,6 @@ def process_resources_section(template, base_path, parent_path, parent_module): if parent_module is None: msg = "Map is only valid in a module" raise exceptions.InvalidModuleError(msg=msg) - # TODO: We should be able to fake up a parent module m = parent_module.find_ref(m[REF]) if m is None: msg = f"{k} has an invalid Map Ref" @@ -126,10 +154,9 @@ def process_resources_section(template, base_path, parent_path, parent_module): del resource[MAP] # Replace $Map and $Index placeholders for prop, val in resource[PROPERTIES].copy().items(): - if val == MAP_PLACEHOLDER: - resource[PROPERTIES][prop] = token - if val == INDEX_PLACEHOLDER: - resource[PROPERTIES][prop] = f"{i}" + resource[PROPERTIES][prop] = map_placeholders( + i, token, val + ) template[RESOURCES][logical_id] = resource del template[RESOURCES][k] @@ -682,7 +709,7 @@ def resolve_getatt(self, v, d, n): logical_id = self.name + v[0] d[n] = {GETATT: [logical_id, v[1]]} - def find_ref(self, v): + def find_ref(self, name): """ Find a Ref. @@ -690,28 +717,29 @@ def find_ref(self, v): template Property, or a Parameter Default. It could also be a reference to another resource in this module. + :param name The name to search for :return The referenced element or None """ - # print(f"find_ref {v}, props: {self.props}, params: {self.params}") - if v in self.props: - if v not in self.params: + # print(f"find_ref {name}, props: {self.props}, params: {self.params}") + if name in self.props: + if name not in self.params: # The parent tried to set a property that doesn't exist # in the Parameters section of this module - msg = f"{v} not found in module Parameters: {self.source}" + msg = f"{name} not found in module Parameters: {self.source}" raise exceptions.InvalidModuleError(msg=msg) - return self.props[v] + return self.props[name] - if v in self.params: - param = self.params[v] + if name in self.params: + param = self.params[name] if DEFAULT in param: # Use the default value of the Parameter return param[DEFAULT] - msg = f"{v} does not have a Default and is not a Property" + msg = f"{name} does not have a Default and is not a Property" raise exceptions.InvalidModuleError(msg=msg) - for k in self.resources: - if v == k: + for logical_id in self.resources: + if name == logical_id: # Simply rename local references to include the module name - return {REF: self.name + v} + return {REF: self.name + logical_id} return None diff --git a/tests/unit/customizations/cloudformation/modules/map-expect.yaml b/tests/unit/customizations/cloudformation/modules/map-expect.yaml new file mode 100644 index 000000000000..69b789b9a716 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/map-expect.yaml @@ -0,0 +1,17 @@ +Parameters: + List: + Type: CommaDelimitedList + Default: A,B,C +Resources: + Content0Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: my-bucket-A + Content1Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: my-bucket-B + Content2Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: my-bucket-C diff --git a/tests/unit/customizations/cloudformation/modules/map-module.yaml b/tests/unit/customizations/cloudformation/modules/map-module.yaml new file mode 100644 index 000000000000..e96d93d1733e --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/map-module.yaml @@ -0,0 +1,9 @@ +Parameters: + Name: + Type: String + +Resources: + Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: !Ref Name diff --git a/tests/unit/customizations/cloudformation/modules/map-template.yaml b/tests/unit/customizations/cloudformation/modules/map-template.yaml new file mode 100644 index 000000000000..ba6a67dcd30d --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/map-template.yaml @@ -0,0 +1,12 @@ +Parameters: + List: + Type: CommaDelimitedList + Default: A,B,C + +Resources: + Content: + Type: LocalModule + Source: ./map-module.yaml + Map: !Ref List + Properties: + Name: !Sub my-bucket-$MapValue diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index f0ac75b11ffb..9eee5a7195ca 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -85,7 +85,16 @@ def test_main(self): # The tests are in the modules directory. # Each test has 3 files: # test-template.yaml, test-module.yaml, and test-expect.yaml - tests = ["basic", "type", "sub", "modinmod", "output", "policy", "vpc"] + tests = [ + "basic", + "type", + "sub", + "modinmod", + "output", + "policy", + "vpc", + "map", + ] for test in tests: base = "unit/customizations/cloudformation/modules" t = modules.read_source(f"{base}/{test}-template.yaml") @@ -93,30 +102,10 @@ def test_main(self): e = modules.read_source(f"{base}/{test}-expect.yaml") # Modules section - if MODULES in td: - for module_name, module_config in td[MODULES].items(): - module_config[modules.NAME] = module_name - relative_path = module_config[modules.SOURCE] - module_config[modules.SOURCE] = f"{base}/{relative_path}" - module = modules.Module(td, module_config) - td = module.process() - del td[MODULES] + td = modules.process_module_section(td, base, t) # Resources with Type LocalModule - for k, v in td[RESOURCES].copy().items(): - if TYPE in v and v[TYPE] == LOCAL_MODULE: - module_config = {} - module_config[modules.NAME] = k - relative_path = v[modules.SOURCE] - module_config[modules.SOURCE] = f"{base}/{relative_path}" - props = modules.PROPERTIES - if props in v: - module_config[props] = v[props] - if modules.OVERRIDES in v: - module_config[modules.OVERRIDES] = v[modules.OVERRIDES] - module = modules.Module(td, module_config) - td = module.process() - del td[RESOURCES][k] + td = modules.process_resources_section(td, base, t, None) processed = yamlhelper.yaml_dump(td) self.assertEqual(e, processed) From e0cfcfc058bfea3e978ea9cc9bbceb46bf2d39c3 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 18 Dec 2024 13:46:25 -0800 Subject: [PATCH 21/26] Moving docs to a readme --- .../customizations/cloudformation/modules.py | 70 ++----------------- .../cloudformation/modules_readme.md | 42 +++++++++++ 2 files changed, 46 insertions(+), 66 deletions(-) create mode 100644 awscli/customizations/cloudformation/modules_readme.md diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 7e1cf64e5d15..e43e59c92557 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -62,16 +62,8 @@ def process_module_section(template, base_path, parent_path): raise exceptions.InvalidModuleError(msg=msg) # Process each Module node separately - for module_name, module_config in template[MODULES].items(): - module_config[NAME] = module_name - # Fix the source path - relative_path = module_config[SOURCE] - module_config[SOURCE] = os.path.join(base_path, relative_path) - module_config[SOURCE] = os.path.normpath(module_config[SOURCE]) - if module_config[SOURCE] == parent_path: - msg = f"Module refers to itself: {parent_path}" - raise exceptions.InvalidModuleError(msg=msg) - module = Module(template, module_config) + for k, v in template[MODULES].items(): + module = make_module(template, k, v, base_path, parent_path) template = module.process() # Remove the Modules section from the template @@ -189,30 +181,10 @@ def merge_props(original, overrides): """ This function merges dicts, replacing values in the original with overrides. This function is recursive and can act on lists and scalars. + See the unit tests for example merges. + See tests/unit/customizations/cloudformation/modules/policy-*.yaml :return A new value with the overridden properties - - Original: - - A: - B: foo - C: - D: bar - - Override: - - A: - B: baz - C: - E: qux - - Result: - - A: - B: baz - C: - D: bar - E: qux """ original_type = type(original) override_type = type(overrides) @@ -245,40 +217,6 @@ class Module: """ Process client-side modules. - See tests/unit/customizations/cloudformation/modules for examples of what - the Modules section of a template looks like. - - Modules can be referenced in a new Modules section in the templates, - or they can be referenced as Resources with the Type LocalModule. - Modules have a Source attribute pointing to a local file, - a Properties attribute that corresponds to Parameters in the modules, - and an Overrides attribute that can override module output. - - A module is itself basically a CloudFormation template, with a Parameters - section and Resources that are injected into the parent template. The - Properties defined in the Modules section correspond to the Parameters in - the module. These modules operate in a similar way to registry modules. - - The name of the module in the Modules section is used as a prefix to - logical ids that are defined in the module. Or if the module is - referenced in the Type attribute of a Resource, the logical id of the - resource is used as the prefix. - - In addition to the parent setting Properties, all attributes of the module - can be overridden with Overrides, which require the consumer to know how - the module is structured. This "escape hatch" is considered a first class - citizen in the design, to avoid excessive Parameter definitions to cover - every possible use case. - - Module Parameters (set by Properties in the parent) are handled with - Refs, Subs, and GetAtts in the module. These are handled in a way that - fixes references to match module prefixes, fully resolving values - that are actually strings and leaving others to be resolved at deploy time. - - Modules can contain other modules, with no limit to the levels of nesting. - - Modules can define Outputs, which are key-value pairs that can be - referenced by the parent. """ def __init__(self, template, module_config): diff --git a/awscli/customizations/cloudformation/modules_readme.md b/awscli/customizations/cloudformation/modules_readme.md new file mode 100644 index 000000000000..4a74dd855ca2 --- /dev/null +++ b/awscli/customizations/cloudformation/modules_readme.md @@ -0,0 +1,42 @@ +# Local CloudFormation Modules + +See tests/unit/customizations/cloudformation/modules for examples of what the +Modules section of a template looks like. + +Modules can be referenced in a new Modules section in the template, or they can +be referenced as Resources with the Type LocalModule. Modules have a Source +attribute pointing to a local file, a Properties attribute that corresponds to +Parameters in the modules, and an Overrides attribute that can override module +output. + +A module is itself basically a CloudFormation template, with a Parameters +section and Resources that are injected into the parent template. The +Properties defined in the Modules section correspond to the Parameters in the +module. These modules operate in a similar way to registry modules. + +The name of the module in the Modules section is used as a prefix to logical +ids that are defined in the module. Or if the module is referenced in the Type +attribute of a Resource, the logical id of the resource is used as the prefix. + +In addition to the parent setting Properties, all attributes of the module can +be overridden with Overrides, which require the consumer to know how the module +is structured. This "escape hatch" is considered a first class citizen in the +design, to avoid excessive Parameter definitions to cover every possible use +case. + +Module Parameters (set by Properties in the parent) are handled with Refs, +Subs, and GetAtts in the module. These are handled in a way that fixes +references to match module prefixes, fully resolving values that are actually +strings and leaving others to be resolved at deploy time. + +Modules can contain other modules, with no limit to the levels of nesting. + +Modules can define Outputs, which are key-value pairs that can be referenced by +the parent. + +When using modules, you can use a comma-delimited list to create a number of +similar resources. This is simpler than using `Fn::ForEach` but has the +limitation of requiring the list to be resolved at build time. +See tests/unit/customizations/cloudformation/modules/vpc-module.yaml. + + From c6851c9029ca6f0c5e1396208717388c9be0da3a Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 18 Dec 2024 13:56:42 -0800 Subject: [PATCH 22/26] updating readme --- .../cloudformation/modules_readme.md | 68 ++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/awscli/customizations/cloudformation/modules_readme.md b/awscli/customizations/cloudformation/modules_readme.md index 4a74dd855ca2..e1d8b002d514 100644 --- a/awscli/customizations/cloudformation/modules_readme.md +++ b/awscli/customizations/cloudformation/modules_readme.md @@ -9,6 +9,35 @@ attribute pointing to a local file, a Properties attribute that corresponds to Parameters in the modules, and an Overrides attribute that can override module output. +The `Modules` section. + +```yaml +Modules: + Content: + Source: ./module.yaml + Properties: + Name: foo + Overrides: + Bucket: + Properties: + OverrideMe: def +``` + +A module configured as a `Resource`. + +```yaml +Resources: + Content: + Type: LocalModule + Source: ./module.yaml + Properties: + Name: foo + Overrides: + Bucket: + Properties: + OverrideMe: def +``` + A module is itself basically a CloudFormation template, with a Parameters section and Resources that are injected into the parent template. The Properties defined in the Modules section correspond to the Parameters in the @@ -22,14 +51,15 @@ In addition to the parent setting Properties, all attributes of the module can be overridden with Overrides, which require the consumer to know how the module is structured. This "escape hatch" is considered a first class citizen in the design, to avoid excessive Parameter definitions to cover every possible use -case. +case. One caveat is that using Overrides is less stable, since the module +author might change logical ids. Using module Outputs can mitigate this. Module Parameters (set by Properties in the parent) are handled with Refs, Subs, and GetAtts in the module. These are handled in a way that fixes references to match module prefixes, fully resolving values that are actually strings and leaving others to be resolved at deploy time. -Modules can contain other modules, with no limit to the levels of nesting. +Modules can contain other modules, with no enforced limit to the levels of nesting. Modules can define Outputs, which are key-value pairs that can be referenced by the parent. @@ -39,4 +69,38 @@ similar resources. This is simpler than using `Fn::ForEach` but has the limitation of requiring the list to be resolved at build time. See tests/unit/customizations/cloudformation/modules/vpc-module.yaml. +An example of a Map is defining subnets in a VPC. + +```yaml +Parameters: + CidrBlock: + Type: String + PrivateCidrBlocks: + Type: CommaDelimitedList + PublicCidrBlocks: + Type: CommaDelimitedList +Resources: + VPC: + Type: AWS::EC2::VPC + Properties: + CidrBlock: !Ref CidrBlock + EnableDnsHostnames: true + EnableDnsSupport: true + InstanceTenancy: default + PublicSubnet: + Type: LocalModule + Map: !Ref PublicCidrBlocks + Source: ./subnet-module.yaml + Properties: + SubnetCidrBlock: $MapValue + AZSelection: $MapIndex + PrivateSubnet: + Type: LocalModule + Map: !Ref PrivateCidrBlocks + Source: ./subnet-module.yaml + Properties: + SubnetCidrBlock: $MapValue + AZSelection: $MapIndex +``` + From dd16375e1ff7e2d65dcc3601710a9238e66d315d Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 18 Dec 2024 14:38:59 -0800 Subject: [PATCH 23/26] Handle extra dots in getatts --- awscli/customizations/cloudformation/modules.py | 6 +++--- tests/unit/customizations/cloudformation/test_modules.py | 3 --- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index e43e59c92557..9c377459e114 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -370,7 +370,7 @@ def resolve_output_sub(self, v, d, n): sub += resolved elif word.t == WordType.GETATT: resolved = "${" + word.w + "}" - tokens = word.w.split(".") + tokens = word.w.split(".", 1) if len(tokens) < 2: msg = f"GetAtt {word.w} has unexpected number of tokens" raise exceptions.InvalidModuleError(msg=msg) @@ -419,7 +419,7 @@ def resolve_output_getatt(self, v, d, n): sub += resolved elif word.t == WordType.GETATT: resolved = "${" + word.w + "}" - tokens = word.w.split(".") + tokens = word.w.split(".", 1) if len(tokens) < 2: msg = f"GetAtt {word.w} unexpected length" raise exceptions.InvalidModuleError(msg=msg) @@ -621,7 +621,7 @@ def resolve_sub(self, v, d, n): elif word.t == WordType.GETATT: need_sub = True resolved = "${" + word.w + "}" - tokens = word.w.split(".") + tokens = word.w.split(".", 1) if len(tokens) < 2: msg = f"GetAtt {word.w} has unexpected number of tokens" raise exceptions.InvalidModuleError(msg=msg) diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 9eee5a7195ca..0fa4fa12d880 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -75,13 +75,10 @@ def test_merge_props(self): expect = {"b": "cc", "d": {"e": "ff", "g": "h", "i": [1, 2, 3, 4, 5]}} merged = modules.merge_props(original, overrides) self.assertEqual(merged, expect) - # TODO: More complex examples (especially merging Policies) def test_main(self): "Run tests on sample templates that include local modules" - # TODO: Port tests over from Rain - # The tests are in the modules directory. # Each test has 3 files: # test-template.yaml, test-module.yaml, and test-expect.yaml From e1195768c6a78e35357d5b8b205a8d6ed6b0f9a8 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 18 Dec 2024 14:51:22 -0800 Subject: [PATCH 24/26] Fix manifest error --- .../customizations/cloudformation/modules.py | 108 +++++++++++++++++- .../cloudformation/modules_readme.md | 106 ----------------- 2 files changed, 107 insertions(+), 107 deletions(-) delete mode 100644 awscli/customizations/cloudformation/modules_readme.md diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 9c377459e114..fceb5780c870 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -11,7 +11,113 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. -"This file implements local module support for the package command" +""" +This file implements local module support for the package command + +See tests/unit/customizations/cloudformation/modules for examples of what the +Modules section of a template looks like. + +Modules can be referenced in a new Modules section in the template, or they can +be referenced as Resources with the Type LocalModule. Modules have a Source +attribute pointing to a local file, a Properties attribute that corresponds to +Parameters in the modules, and an Overrides attribute that can override module +output. + +The `Modules` section. + +```yaml +Modules: + Content: + Source: ./module.yaml + Properties: + Name: foo + Overrides: + Bucket: + Properties: + OverrideMe: def +``` + +A module configured as a `Resource`. + +```yaml +Resources: + Content: + Type: LocalModule + Source: ./module.yaml + Properties: + Name: foo + Overrides: + Bucket: + Properties: + OverrideMe: def +``` + +A module is itself basically a CloudFormation template, with a Parameters +section and Resources that are injected into the parent template. The +Properties defined in the Modules section correspond to the Parameters in the +module. These modules operate in a similar way to registry modules. + +The name of the module in the Modules section is used as a prefix to logical +ids that are defined in the module. Or if the module is referenced in the Type +attribute of a Resource, the logical id of the resource is used as the prefix. + +In addition to the parent setting Properties, all attributes of the module can +be overridden with Overrides, which require the consumer to know how the module +is structured. This "escape hatch" is considered a first class citizen in the +design, to avoid excessive Parameter definitions to cover every possible use +case. One caveat is that using Overrides is less stable, since the module +author might change logical ids. Using module Outputs can mitigate this. + +Module Parameters (set by Properties in the parent) are handled with Refs, +Subs, and GetAtts in the module. These are handled in a way that fixes +references to match module prefixes, fully resolving values that are actually +strings and leaving others to be resolved at deploy time. + +Modules can contain other modules, with no enforced limit to the levels of nesting. + +Modules can define Outputs, which are key-value pairs that can be referenced by +the parent. + +When using modules, you can use a comma-delimited list to create a number of +similar resources. This is simpler than using `Fn::ForEach` but has the +limitation of requiring the list to be resolved at build time. +See tests/unit/customizations/cloudformation/modules/vpc-module.yaml. + +An example of a Map is defining subnets in a VPC. + +```yaml +Parameters: + CidrBlock: + Type: String + PrivateCidrBlocks: + Type: CommaDelimitedList + PublicCidrBlocks: + Type: CommaDelimitedList +Resources: + VPC: + Type: AWS::EC2::VPC + Properties: + CidrBlock: !Ref CidrBlock + EnableDnsHostnames: true + EnableDnsSupport: true + InstanceTenancy: default + PublicSubnet: + Type: LocalModule + Map: !Ref PublicCidrBlocks + Source: ./subnet-module.yaml + Properties: + SubnetCidrBlock: $MapValue + AZSelection: $MapIndex + PrivateSubnet: + Type: LocalModule + Map: !Ref PrivateCidrBlocks + Source: ./subnet-module.yaml + Properties: + SubnetCidrBlock: $MapValue + AZSelection: $MapIndex +``` + +""" # pylint: disable=fixme,too-many-instance-attributes diff --git a/awscli/customizations/cloudformation/modules_readme.md b/awscli/customizations/cloudformation/modules_readme.md deleted file mode 100644 index e1d8b002d514..000000000000 --- a/awscli/customizations/cloudformation/modules_readme.md +++ /dev/null @@ -1,106 +0,0 @@ -# Local CloudFormation Modules - -See tests/unit/customizations/cloudformation/modules for examples of what the -Modules section of a template looks like. - -Modules can be referenced in a new Modules section in the template, or they can -be referenced as Resources with the Type LocalModule. Modules have a Source -attribute pointing to a local file, a Properties attribute that corresponds to -Parameters in the modules, and an Overrides attribute that can override module -output. - -The `Modules` section. - -```yaml -Modules: - Content: - Source: ./module.yaml - Properties: - Name: foo - Overrides: - Bucket: - Properties: - OverrideMe: def -``` - -A module configured as a `Resource`. - -```yaml -Resources: - Content: - Type: LocalModule - Source: ./module.yaml - Properties: - Name: foo - Overrides: - Bucket: - Properties: - OverrideMe: def -``` - -A module is itself basically a CloudFormation template, with a Parameters -section and Resources that are injected into the parent template. The -Properties defined in the Modules section correspond to the Parameters in the -module. These modules operate in a similar way to registry modules. - -The name of the module in the Modules section is used as a prefix to logical -ids that are defined in the module. Or if the module is referenced in the Type -attribute of a Resource, the logical id of the resource is used as the prefix. - -In addition to the parent setting Properties, all attributes of the module can -be overridden with Overrides, which require the consumer to know how the module -is structured. This "escape hatch" is considered a first class citizen in the -design, to avoid excessive Parameter definitions to cover every possible use -case. One caveat is that using Overrides is less stable, since the module -author might change logical ids. Using module Outputs can mitigate this. - -Module Parameters (set by Properties in the parent) are handled with Refs, -Subs, and GetAtts in the module. These are handled in a way that fixes -references to match module prefixes, fully resolving values that are actually -strings and leaving others to be resolved at deploy time. - -Modules can contain other modules, with no enforced limit to the levels of nesting. - -Modules can define Outputs, which are key-value pairs that can be referenced by -the parent. - -When using modules, you can use a comma-delimited list to create a number of -similar resources. This is simpler than using `Fn::ForEach` but has the -limitation of requiring the list to be resolved at build time. -See tests/unit/customizations/cloudformation/modules/vpc-module.yaml. - -An example of a Map is defining subnets in a VPC. - -```yaml -Parameters: - CidrBlock: - Type: String - PrivateCidrBlocks: - Type: CommaDelimitedList - PublicCidrBlocks: - Type: CommaDelimitedList -Resources: - VPC: - Type: AWS::EC2::VPC - Properties: - CidrBlock: !Ref CidrBlock - EnableDnsHostnames: true - EnableDnsSupport: true - InstanceTenancy: default - PublicSubnet: - Type: LocalModule - Map: !Ref PublicCidrBlocks - Source: ./subnet-module.yaml - Properties: - SubnetCidrBlock: $MapValue - AZSelection: $MapIndex - PrivateSubnet: - Type: LocalModule - Map: !Ref PrivateCidrBlocks - Source: ./subnet-module.yaml - Properties: - SubnetCidrBlock: $MapValue - AZSelection: $MapIndex -``` - - From 39ac9a7f43b3d227e539f507d2332d101faf1d41 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Thu, 19 Dec 2024 10:18:30 -0800 Subject: [PATCH 25/26] Add basic conditional support --- .../cloudformation/module_conditions.py | 63 +++++++++++++++++++ .../customizations/cloudformation/modules.py | 30 +++++++-- .../modules/conditional-expect.yaml | 46 ++++++++++++++ .../modules/conditional-module.yaml | 52 +++++++++++++++ .../modules/conditional-template.yaml | 13 ++++ .../cloudformation/test_modules.py | 1 + 6 files changed, 201 insertions(+), 4 deletions(-) create mode 100644 awscli/customizations/cloudformation/module_conditions.py create mode 100644 tests/unit/customizations/cloudformation/modules/conditional-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/conditional-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/conditional-template.yaml diff --git a/awscli/customizations/cloudformation/module_conditions.py b/awscli/customizations/cloudformation/module_conditions.py new file mode 100644 index 000000000000..1f712da85902 --- /dev/null +++ b/awscli/customizations/cloudformation/module_conditions.py @@ -0,0 +1,63 @@ +# Copyright 2012-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"). You +# may not use this file except in compliance with the License. A copy of +# the License is located at +# +# http://aws.amazon.com/apache2.0/ +# +# or in the "license" file accompanying this file. This file is +# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF +# ANY KIND, either express or implied. See the License for the specific +# language governing permissions and limitations under the License. + +# pylint: disable=fixme + +""" +Parse the Conditions section in a module + +This section is not emitted into the output. +We have to be able to fully resolve it locally. +""" + +from awscli.customizations.cloudformation import exceptions + +AND = "Fn::And" +EQUALS = "Fn::Equals" +IF = "Fn::If" +NOT = "Fn::Not" +OR = "Fn::Or" +REF = "Ref" + + +def parse_conditions(d, find_ref): + """Parse conditions and return a map of name:boolean""" + retval = {} + + for k, v in d.items(): + retval[k] = istrue(v, find_ref) + + return retval + + +def istrue(v, find_ref): + "Recursive function to evaluate a Condition" + if EQUALS in v: + eq = v[EQUALS] + if len(eq) == 2: + val0 = eq[0] + val1 = eq[1] + if REF in eq[0]: + val0 = find_ref(eq[0][REF]) + if REF in eq[1]: + val1 = find_ref(eq[1][REF]) + return val0 == val1 + elif NOT in v: + if not isinstance(v[NOT], list): + msg = "Not expression should be a list with 1 element: {v[NOT]}" + raise exceptions.InvalidModuleError(msg=msg) + return not istrue(v[NOT][0], find_ref) + + # TODO - Implement the other intrisics + + return False diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index fceb5780c870..01e8ad9df96e 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -73,15 +73,16 @@ references to match module prefixes, fully resolving values that are actually strings and leaving others to be resolved at deploy time. -Modules can contain other modules, with no enforced limit to the levels of nesting. +Modules can contain other modules, with no enforced limit to the levels of +nesting. Modules can define Outputs, which are key-value pairs that can be referenced by the parent. When using modules, you can use a comma-delimited list to create a number of similar resources. This is simpler than using `Fn::ForEach` but has the -limitation of requiring the list to be resolved at build time. -See tests/unit/customizations/cloudformation/modules/vpc-module.yaml. +limitation of requiring the list to be resolved at build time. See +tests/unit/customizations/cloudformation/modules/vpc-module.yaml. An example of a Map is defining subnets in a VPC. @@ -97,7 +98,7 @@ VPC: Type: AWS::EC2::VPC Properties: - CidrBlock: !Ref CidrBlock + CidrBlock: !Ref CidrBlock EnableDnsHostnames: true EnableDnsSupport: true InstanceTenancy: default @@ -131,6 +132,9 @@ from awscli.customizations.cloudformation.parse_sub import WordType from awscli.customizations.cloudformation.parse_sub import parse_sub +from awscli.customizations.cloudformation.module_conditions import ( + parse_conditions, +) LOG = logging.getLogger(__name__) @@ -158,6 +162,8 @@ MAP = "Map" MAP_PLACEHOLDER = "$MapValue" INDEX_PLACEHOLDER = "$MapIndex" +CONDITIONS = "Conditions" +CONDITION = "Condition" def process_module_section(template, base_path, parent_path): @@ -364,6 +370,9 @@ def __init__(self, template, module_config): # Outputs defined in the module self.outputs = {} + # Conditions defined in the module + self.conditions = {} + def __str__(self): "Print out a string with module details for logs" return ( @@ -408,6 +417,14 @@ def process(self): self.validate_overrides() + if CONDITIONS in module_dict: + cs = module_dict[CONDITIONS] + + def find_ref(v): + return self.find_ref(v) + + self.conditions = parse_conditions(cs, find_ref) + for logical_id, resource in self.resources.items(): self.process_resource(logical_id, resource) @@ -545,6 +562,11 @@ def validate_overrides(self): def process_resource(self, logical_id, resource): "Process a single resource" + # First, check to see if a Conditions omits this resource + if CONDITION in resource and resource[CONDITION] in self.conditions: + if self.conditions[resource[CONDITION]] is False: + return + # For each property (and property-like attribute), # replace the value if it appears in parent overrides. attrs = [ diff --git a/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml b/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml new file mode 100644 index 000000000000..65e290258657 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml @@ -0,0 +1,46 @@ +Resources: + ATable: + Type: AWS::DynamoDB::Table + Properties: + BillingMode: PAY_PER_REQUEST + TableName: table-a + AttributeDefinitions: + - AttributeName: id + AttributeType: S + KeySchema: + - AttributeName: id + KeyType: HASH + ALambdaPolicy: + Type: AWS::IAM::RolePolicy + Condition: IfLambdaRoleIsSet + Metadata: + Comment: This resource is created only if the LambdaRoleArn is set + Properties: + PolicyDocument: + Statement: + - Action: + - dynamodb:BatchGetItem + - dynamodb:GetItem + - dynamodb:Query + - dynamodb:Scan + - dynamodb:BatchWriteItem + - dynamodb:PutItem + - dynamodb:UpdateItem + Effect: Allow + Resource: + - Fn::GetAtt: + - ATable + - Arn + PolicyName: table-a-policy + RoleName: my-role + BTable: + Type: AWS::DynamoDB::Table + Properties: + BillingMode: PAY_PER_REQUEST + TableName: table-b + AttributeDefinitions: + - AttributeName: id + AttributeType: S + KeySchema: + - AttributeName: id + KeyType: HASH diff --git a/tests/unit/customizations/cloudformation/modules/conditional-module.yaml b/tests/unit/customizations/cloudformation/modules/conditional-module.yaml new file mode 100644 index 000000000000..f2639ba3b53e --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/conditional-module.yaml @@ -0,0 +1,52 @@ +Parameters: + + TableName: + Type: String + + LambdaRoleName: + Type: String + Description: If set, allow the lambda function to access this table + Default: "" + +Conditions: + IfLambdaRoleIsSet: + Fn::Not: + - Fn::Equals: + - !Ref LambdaRoleName + - "" + +Resources: + Table: + Type: AWS::DynamoDB::Table + Properties: + BillingMode: PAY_PER_REQUEST + TableName: !Sub ${TableName} + AttributeDefinitions: + - AttributeName: id + AttributeType: S + KeySchema: + - AttributeName: id + KeyType: HASH + + LambdaPolicy: + Type: AWS::IAM::RolePolicy + Condition: IfLambdaRoleIsSet + Metadata: + Comment: This resource is created only if the LambdaRoleArn is set + Properties: + PolicyDocument: + Statement: + - Action: + - dynamodb:BatchGetItem + - dynamodb:GetItem + - dynamodb:Query + - dynamodb:Scan + - dynamodb:BatchWriteItem + - dynamodb:PutItem + - dynamodb:UpdateItem + Effect: Allow + Resource: + - !GetAtt Table.Arn + PolicyName: !Sub ${TableName}-policy + RoleName: !Ref LambdaRoleName + diff --git a/tests/unit/customizations/cloudformation/modules/conditional-template.yaml b/tests/unit/customizations/cloudformation/modules/conditional-template.yaml new file mode 100644 index 000000000000..1e7b3ca6345b --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/conditional-template.yaml @@ -0,0 +1,13 @@ +Resources: + A: + Type: LocalModule + Source: ./conditional-module.yaml + Properties: + TableName: table-a + LambdaRoleName: my-role + B: + Type: LocalModule + Source: ./conditional-module.yaml + Properties: + TableName: table-b + diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 0fa4fa12d880..2ca2bfc4241e 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -91,6 +91,7 @@ def test_main(self): "policy", "vpc", "map", + "conditional", ] for test in tests: base = "unit/customizations/cloudformation/modules" From 2f0143bab567386b930322b2b0e845740f7adfd0 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Fri, 20 Dec 2024 15:48:32 -0800 Subject: [PATCH 26/26] Added more complete unit test for conditionals --- .../cloudformation/module_conditions.py | 79 ++++++++++++++++--- .../customizations/cloudformation/modules.py | 9 ++- .../modules/cond-intrinsics-expect.yaml | 7 ++ .../modules/cond-intrinsics-module.yaml | 40 ++++++++++ .../modules/cond-intrinsics-template.yaml | 9 +++ .../cloudformation/test_modules.py | 1 + 6 files changed, 129 insertions(+), 16 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/cond-intrinsics-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/cond-intrinsics-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/cond-intrinsics-template.yaml diff --git a/awscli/customizations/cloudformation/module_conditions.py b/awscli/customizations/cloudformation/module_conditions.py index 1f712da85902..992424f10fce 100644 --- a/awscli/customizations/cloudformation/module_conditions.py +++ b/awscli/customizations/cloudformation/module_conditions.py @@ -28,6 +28,7 @@ NOT = "Fn::Not" OR = "Fn::Or" REF = "Ref" +CONDITION = "Condition" def parse_conditions(d, find_ref): @@ -35,29 +36,81 @@ def parse_conditions(d, find_ref): retval = {} for k, v in d.items(): - retval[k] = istrue(v, find_ref) + retval[k] = istrue(v, find_ref, retval) return retval -def istrue(v, find_ref): +def resolve_if(v, find_ref, prior): + "Resolve Fn::If" + msg = f"If expression should be a list with 3 elements: {v}" + if not isinstance(v, list): + raise exceptions.InvalidModuleError(msg=msg) + if len(v) != 3: + raise exceptions.InvalidModuleError(msg=msg) + if istrue(v[0], find_ref, prior): + return v[1] + return v[2] + + +# pylint: disable=too-many-branches,too-many-statements +def istrue(v, find_ref, prior): "Recursive function to evaluate a Condition" + retval = False if EQUALS in v: eq = v[EQUALS] if len(eq) == 2: val0 = eq[0] val1 = eq[1] - if REF in eq[0]: - val0 = find_ref(eq[0][REF]) - if REF in eq[1]: - val1 = find_ref(eq[1][REF]) - return val0 == val1 - elif NOT in v: + if IF in val0: + val0 = resolve_if(val0[IF], find_ref, prior) + if IF in val1: + val1 = resolve_if(val1[IF], find_ref, prior) + if REF in val0: + val0 = find_ref(val0[REF]) + if REF in val1: + val1 = find_ref(val1[REF]) + retval = val0 == val1 + else: + msg = f"Equals expression should be a list with 2 elements: {eq}" + raise exceptions.InvalidModuleError(msg=msg) + if NOT in v: if not isinstance(v[NOT], list): - msg = "Not expression should be a list with 1 element: {v[NOT]}" + msg = f"Not expression should be a list with 1 element: {v[NOT]}" raise exceptions.InvalidModuleError(msg=msg) - return not istrue(v[NOT][0], find_ref) - - # TODO - Implement the other intrisics + retval = not istrue(v[NOT][0], find_ref, prior) + if AND in v: + vand = v[AND] + msg = f"And expression should be a list with 2 elements: {vand}" + if not isinstance(vand, list): + raise exceptions.InvalidModuleError(msg=msg) + if len(vand) != 2: + raise exceptions.InvalidModuleError(msg=msg) + retval = istrue(vand[0], find_ref, prior) and istrue( + vand[1], find_ref, prior + ) + if OR in v: + vor = v[OR] + msg = f"Or expression should be a list with 2 elements: {vor}" + if not isinstance(vor, list): + raise exceptions.InvalidModuleError(msg=msg) + if len(vor) != 2: + raise exceptions.InvalidModuleError(msg=msg) + retval = istrue(vor[0], find_ref, prior) or istrue( + vor[1], find_ref, prior + ) + if IF in v: + # Shouldn't ever see an IF here + msg = f"Unexpected If: {v[IF]}" + raise exceptions.InvalidModuleError(msg=msg) + if CONDITION in v: + condition_name = v[CONDITION] + if condition_name in prior: + retval = prior[condition_name] + else: + msg = f"Condition {condition_name} was not evaluated yet" + raise exceptions.InvalidModuleError(msg=msg) + # TODO: Should we re-order the conditions? + # We are depending on the author putting them in order - return False + return retval diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 01e8ad9df96e..94b96fe61c78 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -563,9 +563,12 @@ def process_resource(self, logical_id, resource): "Process a single resource" # First, check to see if a Conditions omits this resource - if CONDITION in resource and resource[CONDITION] in self.conditions: - if self.conditions[resource[CONDITION]] is False: - return + if CONDITION in resource: + if resource[CONDITION] in self.conditions: + if self.conditions[resource[CONDITION]] is False: + return + del resource[CONDITION] + # else leave it and assume it's in the parent? # For each property (and property-like attribute), # replace the value if it appears in parent overrides. diff --git a/tests/unit/customizations/cloudformation/modules/cond-intrinsics-expect.yaml b/tests/unit/customizations/cloudformation/modules/cond-intrinsics-expect.yaml new file mode 100644 index 000000000000..649ec70a9de9 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/cond-intrinsics-expect.yaml @@ -0,0 +1,7 @@ +Resources: + ABucket0: + Type: AWS::S3::Bucket + ABucket1: + Type: AWS::S3::Bucket + ABucket2: + Type: AWS::S3::Bucket diff --git a/tests/unit/customizations/cloudformation/modules/cond-intrinsics-module.yaml b/tests/unit/customizations/cloudformation/modules/cond-intrinsics-module.yaml new file mode 100644 index 000000000000..b4e1353addab --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/cond-intrinsics-module.yaml @@ -0,0 +1,40 @@ +Parameters: + Foo: + Type: String + +Conditions: + Show0: + Fn::Equals: + - !Ref Foo + - bar + Show1: + Fn::And: + - Fn::Equals: + - !Ref Foo + - bar + - Condition: Show0 + Show2: + Fn::Not: + - Fn::Or: + - Fn::Equals: + - !Ref Foo + - baz + - Fn::Equals: + - Fn::If: + - Fn::Equals: + - a + - a + - x + - y + - y + +Resources: + Bucket0: + Type: AWS::S3::Bucket + Condition: Show0 + Bucket1: + Type: AWS::S3::Bucket + Condition: Show1 + Bucket2: + Type: AWS::S3::Bucket + Condition: Show2 diff --git a/tests/unit/customizations/cloudformation/modules/cond-intrinsics-template.yaml b/tests/unit/customizations/cloudformation/modules/cond-intrinsics-template.yaml new file mode 100644 index 000000000000..c3daddf7c8f2 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/cond-intrinsics-template.yaml @@ -0,0 +1,9 @@ +Modules: + A: + Source: ./cond-intrinsics-module.yaml + Properties: + Foo: bar + B: + Source: ./cond-intrinsics-module.yaml + Properties: + Foo: baz diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 2ca2bfc4241e..73fb2f61f698 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -92,6 +92,7 @@ def test_main(self): "vpc", "map", "conditional", + "cond-intrinsics", ] for test in tests: base = "unit/customizations/cloudformation/modules"