From 0438bdcbc9d767e906c1aba5c84c3a62f21c373f Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Thu, 21 Nov 2024 16:20:56 -0800 Subject: [PATCH 01/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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/43] 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" From 36305ecc270cde3d4d4f698b62539fa8a7e8e15f Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Mon, 13 Jan 2025 13:33:26 -0800 Subject: [PATCH 27/43] Download modules from a URL --- .../customizations/cloudformation/modules.py | 32 ++++++++++++++++--- .../cloudformation/modules/url-template.yaml | 12 +++++++ 2 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/url-template.yaml diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 94b96fe61c78..06b3090b1df0 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -125,6 +125,7 @@ import copy import logging import os +import urllib from collections import OrderedDict from awscli.customizations.cloudformation import exceptions @@ -191,9 +192,16 @@ def make_module(template, name, config, base_path, parent_path): 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]) + + source_path = config[SOURCE] + + if not is_url(source_path): + relative_path = source_path + module_config[SOURCE] = os.path.join(base_path, relative_path) + module_config[SOURCE] = os.path.normpath(module_config[SOURCE]) + else: + module_config[SOURCE] = source_path + if module_config[SOURCE] == parent_path: msg = f"Module refers to itself: {parent_path}" raise exceptions.InvalidModuleError(msg=msg) @@ -279,10 +287,26 @@ def isdict(v): return isinstance(v, (dict, OrderedDict)) +def is_url(p): + "Returns true if the path looks like a URL instead of a local file" + return p.startswith("https") + + 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): + if not isinstance(source, str): + raise exceptions.InvalidModulePathError(source=source) + + if is_url(source): + try: + with urllib.request.urlopen(source) as response: + return response.read() + except Exception as e: + print(e) + raise exceptions.InvalidModulePathError(source=source) + + if not os.path.isfile(source): raise exceptions.InvalidModulePathError(source=source) with open(source, "r", encoding="utf-8") as s: diff --git a/tests/unit/customizations/cloudformation/modules/url-template.yaml b/tests/unit/customizations/cloudformation/modules/url-template.yaml new file mode 100644 index 000000000000..9e14c713c277 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/url-template.yaml @@ -0,0 +1,12 @@ +Modules: + Content: + Source: https://raw.githubusercontent.com/aws/aws-cli/2f0143bab567386b930322b2b0e845740f7adfd0/tests/unit/customizations/cloudformation/modules/basic-module.yaml + Properties: + Name: foo + Overrides: + Bucket: + Properties: + OverrideMe: def +Resources: + OtherResource: + Type: AWS::S3::Bucket From 497fb3d04240ef8d8c732bfb6f37963b66a32e2f Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Mon, 13 Jan 2025 15:50:19 -0800 Subject: [PATCH 28/43] Removed the requirement for the s3-bucket parameter --- .../cloudformation/artifact_exporter.py | 11 ++++ .../cloudformation/exceptions.py | 4 +- .../customizations/cloudformation/package.py | 33 ++++++----- .../cloudformation/_package_description.rst | 48 ++++++++++------ awscli/examples/cloudformation/package.rst | 55 ++++++++++++++++++- .../modules/example-expect.yaml | 7 +++ .../modules/example-module.yaml | 11 ++++ .../modules/example-template.yaml | 10 ++++ .../cloudformation/test_modules.py | 1 + 9 files changed, 145 insertions(+), 35 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/example-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/example-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/example-template.yaml diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index e9137efbf31d..b29c9411d4dc 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -142,6 +142,9 @@ def upload_local_artifacts(resource_id, resource_dict, property_name, local_path = make_abs_path(parent_dir, local_path) + if uploader is None: + raise exceptions.PackageBucketRequiredError() + # Or, pointing to a folder. Zip the folder and upload if is_local_folder(local_path): return zip_and_upload(local_path, uploader) @@ -157,6 +160,8 @@ def upload_local_artifacts(resource_id, resource_dict, property_name, def zip_and_upload(local_path, uploader): + if uploader is None: + raise exceptions.PackageBucketRequiredError() with zip_folder(local_path) as zipfile: return uploader.upload_with_dedup(zipfile) @@ -475,6 +480,9 @@ def do_export(self, resource_id, resource_dict, parent_dir): exported_template_str = yaml_dump(exported_template_dict) + if uploader is None: + raise exceptions.PackageBucketRequiredError() + with mktempfile() as temporary_file: temporary_file.write(exported_template_str) temporary_file.flush() @@ -561,6 +569,9 @@ def include_transform_export_handler(template_dict, uploader, parent_dir): return template_dict # We are confident at this point that `include_location` is a string containing the local path + if uploader is None: + raise exceptions.PackageBucketRequiredError() + abs_include_location = os.path.join(parent_dir, include_location) if is_local_file(abs_include_location): template_dict["Parameters"]["Location"] = uploader.upload_with_dedup(abs_include_location) diff --git a/awscli/customizations/cloudformation/exceptions.py b/awscli/customizations/cloudformation/exceptions.py index 4beec17f6503..68f478fb9bf5 100644 --- a/awscli/customizations/cloudformation/exceptions.py +++ b/awscli/customizations/cloudformation/exceptions.py @@ -53,7 +53,9 @@ class DeployBucketRequiredError(CloudFormationCommandError): "via an S3 Bucket. Please add the --s3-bucket parameter to your " "command. The local template will be copied to that S3 bucket and " "then deployed.") - + +class PackageBucketRequiredError(CloudFormationCommandError): + fmt = "Add the --s3-bucket parameter to your command to upload artifacts to S3" class InvalidForEachIntrinsicFunctionError(CloudFormationCommandError): fmt = 'The value of {resource_id} has an invalid "Fn::ForEach::" format: Must be a list of three entries' diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index 9bc7464d442e..2a4f861f074e 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -57,7 +57,6 @@ class PackageCommand(BasicCommand): { 'name': 's3-bucket', - 'required': True, 'help_text': ( 'The name of the S3 bucket where this command uploads' ' the artifacts that are referenced in your template.' @@ -124,11 +123,7 @@ class PackageCommand(BasicCommand): ] def _run_main(self, parsed_args, parsed_globals): - s3_client = self._session.create_client( - "s3", - config=Config(signature_version='s3v4'), - region_name=parsed_globals.region, - verify=parsed_globals.verify_ssl) + template_path = parsed_args.template_file if not os.path.isfile(template_path): @@ -137,13 +132,25 @@ def _run_main(self, parsed_args, parsed_globals): bucket = parsed_args.s3_bucket - self.s3_uploader = S3Uploader(s3_client, - bucket, - parsed_args.s3_prefix, - parsed_args.kms_key_id, - parsed_args.force_upload) - # attach the given metadata to the artifacts to be uploaded - self.s3_uploader.artifact_metadata = parsed_args.metadata + # Only create the s3 uploaded if we need it, + # since this command now also supports local modules. + # Local modules should be able to run without credentials. + if bucket: + s3_client = self._session.create_client( + "s3", + config=Config(signature_version='s3v4'), + region_name=parsed_globals.region, + verify=parsed_globals.verify_ssl) + + self.s3_uploader = S3Uploader(s3_client, + bucket, + parsed_args.s3_prefix, + parsed_args.kms_key_id, + parsed_args.force_upload) + # attach the given metadata to the artifacts to be uploaded + self.s3_uploader.artifact_metadata = parsed_args.metadata + else: + self.s3_uploader = None output_file = parsed_args.output_template_file use_json = parsed_args.use_json diff --git a/awscli/examples/cloudformation/_package_description.rst b/awscli/examples/cloudformation/_package_description.rst index f47ec2212916..22a1d71f9952 100644 --- a/awscli/examples/cloudformation/_package_description.rst +++ b/awscli/examples/cloudformation/_package_description.rst @@ -1,10 +1,13 @@ -Packages the local artifacts (local paths) that your AWS CloudFormation template -references. The command uploads local artifacts, such as source code for an AWS -Lambda function or a Swagger file for an AWS API Gateway REST API, to an S3 -bucket. The command returns a copy of your template, replacing references to -local artifacts with the S3 location where the command uploaded the artifacts. +Packages the local artifacts (local paths) that your AWS CloudFormation +template references. The command uploads local artifacts, such as source code +for an AWS Lambda function or a Swagger file for an AWS API Gateway REST API, +to an S3 bucket. The command can also process local modules, which are +parameterized CloudFormation snippets that are merged into the parent template. +The command returns a copy of your template, replacing references to local +artifacts with the S3 location where the command uploaded the artifacts, or +replacing local and remote module references with the resources in the module. -Use this command to quickly upload local artifacts that might be required by +Use this command to quickly process local artifacts that might be required by your template. After you package your template's artifacts, run the ``deploy`` command to deploy the returned template. @@ -32,25 +35,34 @@ This command can upload local artifacts referenced in the following places: - ``S3`` property for the ``AWS::CodeCommit::Repository`` resource -To specify a local artifact in your template, specify a path to a local file or folder, -as either an absolute or relative path. The relative path is a location -that is relative to your template's location. +To specify a local artifact in your template, specify a path to a local file or +folder, as either an absolute or relative path. The relative path is a location +that is relative to your template's location. If the artifact is a module, the +path can be a local file or a remote URL starting with 'https'. For example, if your AWS Lambda function source code is in the -``/home/user/code/lambdafunction/`` folder, specify -``CodeUri: /home/user/code/lambdafunction`` for the -``AWS::Serverless::Function`` resource. The command returns a template and replaces -the local path with the S3 location: ``CodeUri: s3://mybucket/lambdafunction.zip``. +``/home/user/code/lambdafunction/`` folder, specify ``CodeUri: +/home/user/code/lambdafunction`` for the ``AWS::Serverless::Function`` +resource. The command returns a template and replaces the local path with the +S3 location: ``CodeUri: s3://mybucket/lambdafunction.zip``. If you specify a file, the command directly uploads it to the S3 bucket. If you specify a folder, the command zips the folder and then uploads the .zip file. -For most resources, if you don't specify a path, the command zips and uploads the -current working directory. The exception is ``AWS::ApiGateway::RestApi``; -if you don't specify a ``BodyS3Location``, this command will not upload an artifact to S3. +For most resources, if you don't specify a path, the command zips and uploads +the current working directory. The exception is ``AWS::ApiGateway::RestApi``; +if you don't specify a ``BodyS3Location``, this command will not upload an +artifact to S3. Before the command uploads artifacts, it checks if the artifacts are already present in the S3 bucket to prevent unnecessary uploads. The command uses MD5 checksums to compare files. If the values match, the command doesn't upload the -artifacts. Use the ``--force-upload flag`` to skip this check and always upload the -artifacts. +artifacts. Use the ``--force-upload flag`` to skip this check and always upload +the artifacts. + +Modules can be referenced either in the top level ``Modules`` section of the +template, or from a Resource with ``Type: LocalModule``. Module references have +a ``Source`` attribute pointing to the module, either a local file or an +``https`` URL, a ``Properties`` attribute that corresponds to the module's +parameters, and an ``Overrides`` attribute that can override module output. + diff --git a/awscli/examples/cloudformation/package.rst b/awscli/examples/cloudformation/package.rst index 159b9b4f844e..9658d2e9b36e 100644 --- a/awscli/examples/cloudformation/package.rst +++ b/awscli/examples/cloudformation/package.rst @@ -1,6 +1,55 @@ -Following command exports a template named ``template.json`` by uploading local +Following command exports a template named ``template.yaml`` by uploading local artifacts to S3 bucket ``bucket-name`` and writes the exported template to -``packaged-template.json``:: +``packaged-template.yaml``:: - aws cloudformation package --template-file /path_to_template/template.json --s3-bucket bucket-name --output-template-file packaged-template.json --use-json + aws cloudformation package --template-file /path_to_template/template.yaml --s3-bucket bucket-name --output-template-file packaged-template.yaml + + +The following is an example of a template with a ``Modules`` section:: + + Modules: + Content: + Source: ./module.yaml + Properties: + Name: foo + Overrides: + Bucket: + Metadata: + OverrideMe: def + +A module configured as a Resource:: + + Resources: + Content: + Type: LocalModule + Source: ./module.yaml + Properties: + Name: foo + Overrides: + Bucket: + Metadata: + OverrideMe: def + +An example module:: + + Parameters: + Name: + Type: String + Resources: + Bucket: + Type: AWS::S3::Bucket + Metadata: + OverrideMe: abc + Properties: + BucketName: !Ref Name + +Packaging the template with this module would result in the following output:: + + Resources: + ContentBucket: + Type: AWS::S3::Bucket + Metadata: + OverrideMe: def + Properties: + BucketName: foo diff --git a/tests/unit/customizations/cloudformation/modules/example-expect.yaml b/tests/unit/customizations/cloudformation/modules/example-expect.yaml new file mode 100644 index 000000000000..0314736240bd --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/example-expect.yaml @@ -0,0 +1,7 @@ +Resources: + ContentBucket: + Type: AWS::S3::Bucket + Metadata: + OverrideMe: def + Properties: + BucketName: foo diff --git a/tests/unit/customizations/cloudformation/modules/example-module.yaml b/tests/unit/customizations/cloudformation/modules/example-module.yaml new file mode 100644 index 000000000000..66bd95fecb2a --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/example-module.yaml @@ -0,0 +1,11 @@ +Parameters: + Name: + Type: String +Resources: + Bucket: + Type: AWS::S3::Bucket + Metadata: + OverrideMe: abc + Properties: + BucketName: !Ref Name + diff --git a/tests/unit/customizations/cloudformation/modules/example-template.yaml b/tests/unit/customizations/cloudformation/modules/example-template.yaml new file mode 100644 index 000000000000..780cac34a992 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/example-template.yaml @@ -0,0 +1,10 @@ +Resources: + Content: + Type: LocalModule + Source: ./example-module.yaml + Properties: + Name: foo + Overrides: + Bucket: + Metadata: + OverrideMe: def diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 73fb2f61f698..04bc952762b7 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -93,6 +93,7 @@ def test_main(self): "map", "conditional", "cond-intrinsics", + "example", ] for test in tests: base = "unit/customizations/cloudformation/modules" From 6e36de08ba13167ab8d3247efc414800d90798f7 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Tue, 14 Jan 2025 09:53:02 -0800 Subject: [PATCH 29/43] Confirm GetAtt with double quotes works as expected --- .../cloudformation/modules/getatt-expect.yaml | 11 +++++++++++ .../cloudformation/modules/getatt-module.yaml | 10 ++++++++++ .../cloudformation/modules/getatt-template.yaml | 9 +++++++++ .../customizations/cloudformation/test_modules.py | 1 + 4 files changed, 31 insertions(+) create mode 100644 tests/unit/customizations/cloudformation/modules/getatt-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/getatt-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/getatt-template.yaml diff --git a/tests/unit/customizations/cloudformation/modules/getatt-expect.yaml b/tests/unit/customizations/cloudformation/modules/getatt-expect.yaml new file mode 100644 index 000000000000..00c96a04d2dc --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/getatt-expect.yaml @@ -0,0 +1,11 @@ +Resources: + ContentBucket: + Type: AWS::S3::Bucket + Properties: + BucketName: foo +Outputs: + BucketName: + Value: + Fn::GetAtt: + - ContentBucket + - BucketName diff --git a/tests/unit/customizations/cloudformation/modules/getatt-module.yaml b/tests/unit/customizations/cloudformation/modules/getatt-module.yaml new file mode 100644 index 000000000000..349b4a403cef --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/getatt-module.yaml @@ -0,0 +1,10 @@ +Parameters: + Name: + Type: String +Resources: + Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: !Ref Name +Outputs: + BucketName: !GetAtt "Bucket.BucketName" diff --git a/tests/unit/customizations/cloudformation/modules/getatt-template.yaml b/tests/unit/customizations/cloudformation/modules/getatt-template.yaml new file mode 100644 index 000000000000..70685556220e --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/getatt-template.yaml @@ -0,0 +1,9 @@ +Resources: + Content: + Type: LocalModule + Source: ./getatt-module.yaml + Properties: + Name: foo +Outputs: + BucketName: + Value: !GetAtt Content.BucketName diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 04bc952762b7..94c03f86a99f 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -94,6 +94,7 @@ def test_main(self): "conditional", "cond-intrinsics", "example", + "getatt", ] for test in tests: base = "unit/customizations/cloudformation/modules" From 621201832f06dd22bce5a592cd0f2cf56d731950 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Tue, 14 Jan 2025 10:08:26 -0800 Subject: [PATCH 30/43] Fix unit test self.uploader --- awscli/customizations/cloudformation/artifact_exporter.py | 2 +- awscli/examples/cloudformation/package.rst | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index b29c9411d4dc..26452ecd2de4 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -480,7 +480,7 @@ def do_export(self, resource_id, resource_dict, parent_dir): exported_template_str = yaml_dump(exported_template_dict) - if uploader is None: + if self.uploader is None: raise exceptions.PackageBucketRequiredError() with mktempfile() as temporary_file: diff --git a/awscli/examples/cloudformation/package.rst b/awscli/examples/cloudformation/package.rst index 9658d2e9b36e..5e8faeae8e61 100644 --- a/awscli/examples/cloudformation/package.rst +++ b/awscli/examples/cloudformation/package.rst @@ -42,6 +42,8 @@ An example module:: OverrideMe: abc Properties: BucketName: !Ref Name + Outputs: + BucketArn: !GetAtt Bucket.Arn Packaging the template with this module would result in the following output:: From 92ad3bda2efaa5eb3e29d4adf217a61d8bc7932e Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 15 Jan 2025 15:57:15 -0800 Subject: [PATCH 31/43] Constants section --- .../cloudformation/artifact_exporter.py | 6 + .../cloudformation/module_constants.py | 128 ++++++++++++++++++ .../cloudformation/module_visitor.py | 66 +++++++++ .../customizations/cloudformation/modules.py | 25 +++- .../cloudformation/parse_sub.py | 12 +- .../modules/constant-expect.yaml | 10 ++ .../modules/constant-module.yaml | 12 ++ .../modules/constant-template.yaml | 9 ++ .../cloudformation/test_modules.py | 42 ++++++ 9 files changed, 302 insertions(+), 8 deletions(-) create mode 100644 awscli/customizations/cloudformation/module_constants.py create mode 100644 awscli/customizations/cloudformation/module_visitor.py create mode 100644 tests/unit/customizations/cloudformation/modules/constant-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/constant-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/constant-template.yaml diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 26452ecd2de4..7fabb4dfed87 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -26,6 +26,7 @@ from awscli.customizations.cloudformation.yamlhelper import yaml_dump, \ yaml_parse from awscli.customizations.cloudformation import modules +from awscli.customizations.cloudformation import module_constants import jmespath @@ -666,6 +667,11 @@ def export(self): exported to s3. """ + # Process constants + constants = module_constants.process_constants(self.template_dict) + if constants is not None: + module_constants.replace_constants(constants, self.template_dict) + # Process modules try: self.template_dict = modules.process_module_section( diff --git a/awscli/customizations/cloudformation/module_constants.py b/awscli/customizations/cloudformation/module_constants.py new file mode 100644 index 000000000000..a69463a44f60 --- /dev/null +++ b/awscli/customizations/cloudformation/module_constants.py @@ -0,0 +1,128 @@ +# Copyright 2012-2024 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. + +""" +Module constants. + +Add a Constants section to the module or the parent template for +string constants, to help reduce copy-paste within the template. + +Refer to constants in Sub strings later in the template using ${Constant:name} + +Constants can refer to other constants that were defined previously. + +Adding constants to a template has side effects, since we have to +re-write all Subs in the template! Ideally nothing will change but +it's possible. + +Example: + + Constants: + foo: bar + baz: abc-${AWS::AccountId}-${Constant::foo} + + Resources: + Bucket: + Type: AWS::S3::Bucket + Metadata: + Test: !Sub ${Constants:baz} + Properties: + BucketName: !Sub ${Constant::foo} + +""" + +from collections import OrderedDict +from awscli.customizations.cloudformation.parse_sub import WordType +from awscli.customizations.cloudformation.parse_sub import parse_sub +from awscli.customizations.cloudformation.module_visitor import Visitor +from awscli.customizations.cloudformation import exceptions + +CONSTANTS = "Constants" +SUB = "Fn::Sub" + + +def process_constants(d): + """ + Look for a Constants item in d and if it's found, replace all instances + of those constants in Subs later in the dictionary. + Returns a dict of constants. + """ + if CONSTANTS not in d: + return None + + constants = {} + for k, v in d[CONSTANTS].items(): + s = replace_constants(constants, v) + constants[k] = s + + del d[CONSTANTS] + + return constants + + +def replace_constants(constants, s): + """ + Replace all constants in a string or in an entire dictionary. + If s is a string, returns the modified string. + If s is a dictionary, modifies the dictionary in place. + """ + if isinstance(s, str): + retval = "" + words = parse_sub(s) + for w in words: + if w.t == WordType.STR: + retval += w.w + if w.t == WordType.REF: + retval += f"${{{w.w}}}" + if w.t == WordType.AWS: + retval += f"${{AWS::{w.w}}}" + if w.t == WordType.GETATT: + retval += f"${{{w.w}}}" + if w.t == WordType.CONSTANT: + if w.w in constants: + retval += constants[w.w] + else: + msg = f"Unknown constant: {w.w}" + raise exceptions.InvalidModuleError(msg=msg) + return retval + + if isdict(s): + + def vf(v): + if isdict(v.d) and SUB in v.d and v.p is not None: + s = v.d[SUB] + if isinstance(s, str): + newval = replace_constants(constants, s) + if is_sub_needed(newval): + v.p[v.k] = {SUB: newval} + else: + v.p[v.k] = newval + + v = Visitor(s) + v.visit(vf) + + return None + + +def isdict(d): + "Returns true if d is a dict" + return isinstance(d, (dict, OrderedDict)) + + +def is_sub_needed(s): + "Returns true if the string has any Sub variables" + words = parse_sub(s) + for w in words: + if w.t != WordType.STR: + return True + return False diff --git a/awscli/customizations/cloudformation/module_visitor.py b/awscli/customizations/cloudformation/module_visitor.py new file mode 100644 index 000000000000..4e6ca789a30f --- /dev/null +++ b/awscli/customizations/cloudformation/module_visitor.py @@ -0,0 +1,66 @@ +# Copyright 2012-2024 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. + +""" +Visitor pattern for recursively modifying all elements in a dictionary. +""" + +from collections import OrderedDict + + +class Visitor: + """ + This class implements a visitor pattern, to run a function on + all elements of a template or subset. + + Example of a visitor that replaces all strings: + + v = Visitor(template_dict, None, "") + def vf(v): + if isinstance(v.d, string) and v.p is not None: + v.p[v.k] = "replacement" + """ + + def __init__(self, d, p=None, k=""): + """ + Initialize the visitor with a dictionary. This can be the entire + template or a subset. + + :param d A dict or OrderedDict + :param p The parent dictionary (default is None for the template) + :param k the key for d (p[k] = d) (default is "" for the template) + """ + self.d = d + self.p = p + self.k = k + + def __str__(self): + return f"d: {self.d}, p: {self.p}, k: {self.k}" + + def visit(self, visit_func): + """ + Run the specified function on all nodes. + + :param visit_func A function that accepts a Visitor + """ + + def walk(visitor): + visit_func(visitor) + if isinstance(visitor.d, (dict, OrderedDict)): + for k, v in visitor.d.items(): + walk(Visitor(v, visitor.d, k)) + if isinstance(visitor.d, list): + for i, v in enumerate(visitor.d): + walk(Visitor(v, visitor.d, i)) + + walk(self) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 06b3090b1df0..0b03097988d6 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -1,4 +1,4 @@ -# Copyright 2012-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +# Copyright 2012-2024 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 @@ -130,6 +130,10 @@ from awscli.customizations.cloudformation import exceptions from awscli.customizations.cloudformation import yamlhelper +from awscli.customizations.cloudformation.module_constants import ( + process_constants, + replace_constants, +) from awscli.customizations.cloudformation.parse_sub import WordType from awscli.customizations.cloudformation.parse_sub import parse_sub @@ -414,6 +418,12 @@ def process(self): content = read_source(self.source) module_dict = yamlhelper.yaml_parse(content) + + # Process constants + constants = process_constants(module_dict) + if constants is not None: + replace_constants(constants, module_dict) + if RESOURCES not in module_dict: # The module may only have sub modules in the Modules section self.resources = {} @@ -435,7 +445,7 @@ def process(self): section = RESOURCES process_resources_section(module_dict, bp, self.source, self) except Exception as e: - msg = f"Failed to process {section} section: {e}" + msg = f"Failed to process {self.source} {section} section: {e}" LOG.exception(msg) raise exceptions.InvalidModuleError(msg=msg) @@ -447,7 +457,12 @@ def process(self): def find_ref(v): return self.find_ref(v) - self.conditions = parse_conditions(cs, find_ref) + try: + self.conditions = parse_conditions(cs, find_ref) + except Exception as e: + msg = f"Failed to process conditions in {self.source}: {e}" + LOG.exception(msg) + raise exceptions.InvalidModuleError(msg=msg) for logical_id, resource in self.resources.items(): self.process_resource(logical_id, resource) @@ -657,6 +672,8 @@ def process_overrides(self, logical_id, resource, attr_name): return resource_overrides = self.overrides[logical_id] + if resource_overrides is None: + return if attr_name not in resource_overrides: return @@ -765,7 +782,7 @@ def resolve_sub(self, v, d, n): found = self.find_ref(word.w) if found is not None: if isinstance(found, str): - need_sub = False + # need_sub = False resolved = found else: if REF in found: diff --git a/awscli/customizations/cloudformation/parse_sub.py b/awscli/customizations/cloudformation/parse_sub.py index f096d70b4812..4375d122500d 100644 --- a/awscli/customizations/cloudformation/parse_sub.py +++ b/awscli/customizations/cloudformation/parse_sub.py @@ -25,10 +25,11 @@ class WordType(Enum): "Word type enumeration" - STR = 0 # A literal string fragment - REF = 1 # ${ParamOrResourceName} - AWS = 2 # ${AWS::X} - GETATT = 3 # ${X.Y} + STR = 0 # A literal string fragment + REF = 1 # ${ParamOrResourceName} + AWS = 2 # ${AWS::X} + GETATT = 3 # ${X.Y} + CONSTANT = 4 # ${Constant::name} class State(Enum): "State machine enumeration" @@ -86,11 +87,14 @@ def parse_sub(sub_str, leave_bang=False): # Figure out what type it is if buf.startswith("AWS::"): word_type = WordType.AWS + elif buf.startswith("Constant::"): + word_type = WordType.CONSTANT elif '.' in buf: word_type = WordType.GETATT else: word_type = WordType.REF buf = buf.replace("AWS::", "", 1) + buf = buf.replace("Constant::", "", 1) words.append(SubWord(word_type, buf)) buf = '' state = State.READSTR diff --git a/tests/unit/customizations/cloudformation/modules/constant-expect.yaml b/tests/unit/customizations/cloudformation/modules/constant-expect.yaml new file mode 100644 index 000000000000..d9074de4d355 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/constant-expect.yaml @@ -0,0 +1,10 @@ +Metadata: + test: zzz +Resources: + ContentBucket: + Type: AWS::S3::Bucket + Metadata: + Test: + Fn::Sub: bar-abc-${AWS::AccountId}-xyz + Properties: + BucketName: bar diff --git a/tests/unit/customizations/cloudformation/modules/constant-module.yaml b/tests/unit/customizations/cloudformation/modules/constant-module.yaml new file mode 100644 index 000000000000..768e11418294 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/constant-module.yaml @@ -0,0 +1,12 @@ +Constants: + foo: bar + sub: "${Constant::foo}-abc-${AWS::AccountId}" + +Resources: + Bucket: + Type: AWS::S3::Bucket + Metadata: + Test: !Sub ${Constant::sub}-xyz + Properties: + BucketName: !Sub ${Constant::foo} + diff --git a/tests/unit/customizations/cloudformation/modules/constant-template.yaml b/tests/unit/customizations/cloudformation/modules/constant-template.yaml new file mode 100644 index 000000000000..b76f325aa509 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/constant-template.yaml @@ -0,0 +1,9 @@ +Constants: + yyy: zzz +Metadata: + test: !Sub ${Constant::yyy} +Resources: + Content: + Type: LocalModule + Source: ./constant-module.yaml + diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index 94c03f86a99f..bcd81bc9d627 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -7,6 +7,11 @@ from awscli.customizations.cloudformation import modules from awscli.customizations.cloudformation.parse_sub import SubWord, WordType from awscli.customizations.cloudformation.parse_sub import parse_sub +from awscli.customizations.cloudformation.module_visitor import Visitor +from awscli.customizations.cloudformation.module_constants import ( + process_constants, + replace_constants, +) MODULES = "Modules" RESOURCES = "Resources" @@ -95,6 +100,7 @@ def test_main(self): "cond-intrinsics", "example", "getatt", + "constant", ] for test in tests: base = "unit/customizations/cloudformation/modules" @@ -102,6 +108,10 @@ def test_main(self): td = yamlhelper.yaml_parse(t) e = modules.read_source(f"{base}/{test}-expect.yaml") + constants = process_constants(td) + if constants is not None: + replace_constants(constants, td) + # Modules section td = modules.process_module_section(td, base, t) @@ -110,3 +120,35 @@ def test_main(self): processed = yamlhelper.yaml_dump(td) self.assertEqual(e, processed) + + def test_visitor(self): + "Test module_visitor" + + template_dict = {} + resources = {} + template_dict["Resources"] = resources + resources["Bucket"] = { + "Type": "AWS::S3::Bucket", + "Properties": { + "BucketName": "foo", + "TestList": [ + "Item0", + {"BucketName": "foo"}, + {"Item2": {"BucketName": "foo"}}, + ], + }, + } + v = Visitor(template_dict, None, "") + + def vf(v): + if isinstance(v.d, str) and v.p is not None: + if v.k == "BucketName": + v.p[v.k] = "bar" + + v.visit(vf) + self.assertEqual( + resources["Bucket"]["Properties"]["BucketName"], "bar" + ) + test_list = resources["Bucket"]["Properties"]["TestList"] + self.assertEqual(test_list[1]["BucketName"], "bar") + self.assertEqual(test_list[2]["Item2"]["BucketName"], "bar") From fa7c208ce9033a68c6d503eae5294a90e1644f1c Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 15 Jan 2025 16:02:40 -0800 Subject: [PATCH 32/43] Constants example --- .../examples/cloudformation/_package_description.rst | 8 ++++++++ awscli/examples/cloudformation/package.rst | 12 ++++++++++++ 2 files changed, 20 insertions(+) diff --git a/awscli/examples/cloudformation/_package_description.rst b/awscli/examples/cloudformation/_package_description.rst index 22a1d71f9952..f75a788f2a53 100644 --- a/awscli/examples/cloudformation/_package_description.rst +++ b/awscli/examples/cloudformation/_package_description.rst @@ -65,4 +65,12 @@ a ``Source`` attribute pointing to the module, either a local file or an ``https`` URL, a ``Properties`` attribute that corresponds to the module's parameters, and an ``Overrides`` attribute that can override module output. +This command also allows you to add a ``Constants`` section to the template +or to a local module. This section is a simple set of key-value pairs that +can be used to reduce copy-paste within the template. Constants values are +strings that can be references within ``Fn::Sub`` functions using the format +``${Constant::NAME}``. + + + diff --git a/awscli/examples/cloudformation/package.rst b/awscli/examples/cloudformation/package.rst index 5e8faeae8e61..2de7d63b1576 100644 --- a/awscli/examples/cloudformation/package.rst +++ b/awscli/examples/cloudformation/package.rst @@ -55,3 +55,15 @@ Packaging the template with this module would result in the following output:: Properties: BucketName: foo +The following is an example of adding constants to a template:: + + Constants: + foo: bar + baz: ${Constant:foo}-xyz-${AWS::AccountId} + + Resources: + Bucket: + Type: AWS::S3::Bucket + Properties: + BucketName: !Sub ${Constant::baz} + From 30250b317389e32794ca84a848524e80e28b0d66 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 15 Jan 2025 16:17:57 -0800 Subject: [PATCH 33/43] Minor tweaks to comments --- .../cloudformation/module_constants.py | 13 +++++++++---- .../customizations/cloudformation/module_visitor.py | 3 ++- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/awscli/customizations/cloudformation/module_constants.py b/awscli/customizations/cloudformation/module_constants.py index a69463a44f60..40115b4bc068 100644 --- a/awscli/customizations/cloudformation/module_constants.py +++ b/awscli/customizations/cloudformation/module_constants.py @@ -17,7 +17,7 @@ Add a Constants section to the module or the parent template for string constants, to help reduce copy-paste within the template. -Refer to constants in Sub strings later in the template using ${Constant:name} +Refer to constants in Sub strings later in the template using ${Constant::name} Constants can refer to other constants that were defined previously. @@ -53,9 +53,11 @@ def process_constants(d): """ - Look for a Constants item in d and if it's found, replace all instances - of those constants in Subs later in the dictionary. - Returns a dict of constants. + Look for a Constants item in d and if it's found, return it + as a dict. Looks for references to previously defined constants + and substitutes them. + Deletes the Constants item from d. + Returns a dict of the constants. """ if CONSTANTS not in d: return None @@ -98,6 +100,9 @@ def replace_constants(constants, s): if isdict(s): + # Recursively dive into d and replace all string constants + # that are found in Subs. Error if the constant does not exist. + def vf(v): if isdict(v.d) and SUB in v.d and v.p is not None: s = v.d[SUB] diff --git a/awscli/customizations/cloudformation/module_visitor.py b/awscli/customizations/cloudformation/module_visitor.py index 4e6ca789a30f..f12fdebc3c71 100644 --- a/awscli/customizations/cloudformation/module_visitor.py +++ b/awscli/customizations/cloudformation/module_visitor.py @@ -25,10 +25,11 @@ class Visitor: Example of a visitor that replaces all strings: - v = Visitor(template_dict, None, "") + v = Visitor(template_dict) def vf(v): if isinstance(v.d, string) and v.p is not None: v.p[v.k] = "replacement" + v.vist(vf) """ def __init__(self, d, p=None, k=""): From 32fa228c42ed5ac3513169165b4a10bd8b324ae3 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Mon, 27 Jan 2025 15:32:59 -0800 Subject: [PATCH 34/43] Constants --- awscli/customizations/cloudformation/module_constants.py | 2 +- awscli/customizations/cloudformation/parse_sub.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/awscli/customizations/cloudformation/module_constants.py b/awscli/customizations/cloudformation/module_constants.py index 40115b4bc068..db28525118af 100644 --- a/awscli/customizations/cloudformation/module_constants.py +++ b/awscli/customizations/cloudformation/module_constants.py @@ -35,7 +35,7 @@ Bucket: Type: AWS::S3::Bucket Metadata: - Test: !Sub ${Constants:baz} + Test: !Sub ${Constant:baz} Properties: BucketName: !Sub ${Constant::foo} diff --git a/awscli/customizations/cloudformation/parse_sub.py b/awscli/customizations/cloudformation/parse_sub.py index 4375d122500d..18153c9f63f5 100644 --- a/awscli/customizations/cloudformation/parse_sub.py +++ b/awscli/customizations/cloudformation/parse_sub.py @@ -87,7 +87,7 @@ def parse_sub(sub_str, leave_bang=False): # Figure out what type it is if buf.startswith("AWS::"): word_type = WordType.AWS - elif buf.startswith("Constant::"): + elif buf.startswith("Constant::") or buf.startswith("Constants::"): word_type = WordType.CONSTANT elif '.' in buf: word_type = WordType.GETATT @@ -95,6 +95,8 @@ def parse_sub(sub_str, leave_bang=False): word_type = WordType.REF buf = buf.replace("AWS::", "", 1) buf = buf.replace("Constant::", "", 1) + # Very common typo to put Constants instead of Constant + buf = buf.replace("Constants::", "", 1) words.append(SubWord(word_type, buf)) buf = '' state = State.READSTR From 49615a8a770c650258fdeabd15cf3cd08cce59ae Mon Sep 17 00:00:00 2001 From: "Eric Z. Beard" Date: Thu, 30 Jan 2025 13:47:16 -0800 Subject: [PATCH 35/43] Use a constant for module source url in test --- .../customizations/cloudformation/modules/url-template.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/customizations/cloudformation/modules/url-template.yaml b/tests/unit/customizations/cloudformation/modules/url-template.yaml index 9e14c713c277..febd0aee8ca0 100644 --- a/tests/unit/customizations/cloudformation/modules/url-template.yaml +++ b/tests/unit/customizations/cloudformation/modules/url-template.yaml @@ -1,6 +1,8 @@ +Constants: + ModuleSource: https://raw.githubusercontent.com/aws/aws-cli/2f0143bab567386b930322b2b0e845740f7adfd0/tests/unit/customizations/cloudformation/modules Modules: Content: - Source: https://raw.githubusercontent.com/aws/aws-cli/2f0143bab567386b930322b2b0e845740f7adfd0/tests/unit/customizations/cloudformation/modules/basic-module.yaml + Source: !Sub ${Constant::ModuleSource}/basic-module.yaml Properties: Name: foo Overrides: From 23442ce2cf5335d2267ece5c40bd9cdc1c4c9bea Mon Sep 17 00:00:00 2001 From: "Eric Z. Beard" Date: Tue, 4 Feb 2025 15:00:01 -0800 Subject: [PATCH 36/43] Test sending arrays to props and fix output bug --- .../cloudformation/module_conditions.py | 16 ++++++++++++---- awscli/customizations/cloudformation/modules.py | 6 ++---- .../cloudformation/modules/output-expect.yaml | 5 +++++ .../cloudformation/modules/output-template.yaml | 6 ++++++ .../cloudformation/modules/proparray-expect.yaml | 7 +++++++ .../cloudformation/modules/proparray-module.yaml | 9 +++++++++ .../modules/proparray-template.yaml | 8 ++++++++ .../cloudformation/test_modules.py | 1 + 8 files changed, 50 insertions(+), 8 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/proparray-expect.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/proparray-module.yaml create mode 100644 tests/unit/customizations/cloudformation/modules/proparray-template.yaml diff --git a/awscli/customizations/cloudformation/module_conditions.py b/awscli/customizations/cloudformation/module_conditions.py index 992424f10fce..925a07cc133c 100644 --- a/awscli/customizations/cloudformation/module_conditions.py +++ b/awscli/customizations/cloudformation/module_conditions.py @@ -20,6 +20,7 @@ We have to be able to fully resolve it locally. """ +from collections import OrderedDict from awscli.customizations.cloudformation import exceptions AND = "Fn::And" @@ -56,19 +57,21 @@ def resolve_if(v, find_ref, prior): # pylint: disable=too-many-branches,too-many-statements def istrue(v, find_ref, prior): "Recursive function to evaluate a Condition" + if not isdict(v): + return False retval = False if EQUALS in v: eq = v[EQUALS] if len(eq) == 2: val0 = eq[0] val1 = eq[1] - if IF in val0: + if isdict(val0) and IF in val0: val0 = resolve_if(val0[IF], find_ref, prior) - if IF in val1: + if isdict(val1) and IF in val1: val1 = resolve_if(val1[IF], find_ref, prior) - if REF in val0: + if isdict(val0) and REF in val0: val0 = find_ref(val0[REF]) - if REF in val1: + if isdict(val1) and REF in val1: val1 = find_ref(val1[REF]) retval = val0 == val1 else: @@ -114,3 +117,8 @@ def istrue(v, find_ref, prior): # We are depending on the author putting them in order return retval + + +def isdict(v): + "Returns True if the type is a dict or OrderedDict" + return isinstance(v, (dict, OrderedDict)) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 0b03097988d6..c61d5e42bc3d 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -544,6 +544,8 @@ def resolve_output_sub(self, v, d, n): resolved = "${" + self.name + ".".join(getatt) + "}" elif SUB in output: resolved = "${" + self.name + output[SUB] + "}" + elif REF in output: + resolved = "${" + self.name + output[REF] + "}" sub += resolved d[n] = {SUB: sub} @@ -715,8 +717,6 @@ 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(v, d, n) elif k == SUB: @@ -766,7 +766,6 @@ def resolve_sub(self, 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 @@ -830,7 +829,6 @@ def find_ref(self, name): :param name The name to search for :return The referenced element or None """ - # 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 diff --git a/tests/unit/customizations/cloudformation/modules/output-expect.yaml b/tests/unit/customizations/cloudformation/modules/output-expect.yaml index a4ebb8fe8ea5..a005d962c56b 100644 --- a/tests/unit/customizations/cloudformation/modules/output-expect.yaml +++ b/tests/unit/customizations/cloudformation/modules/output-expect.yaml @@ -15,3 +15,8 @@ Resources: Type: AWS::S3::Bucket Properties: BucketName: foo + A: + Type: A::B::C + Properties: + Object: + Arn: !Sub ${ContentBucket.Arn} diff --git a/tests/unit/customizations/cloudformation/modules/output-template.yaml b/tests/unit/customizations/cloudformation/modules/output-template.yaml index 872d3423597b..19526cc9e97d 100644 --- a/tests/unit/customizations/cloudformation/modules/output-template.yaml +++ b/tests/unit/customizations/cloudformation/modules/output-template.yaml @@ -3,6 +3,12 @@ Modules: Source: ./output-module.yaml Properties: Name: foo +Resources: + A: + Type: A::B::C + Properties: + Object: + Arn: !Sub ${Content.BucketArn} Outputs: ExampleOutput: Value: !GetAtt Content.BucketArn diff --git a/tests/unit/customizations/cloudformation/modules/proparray-expect.yaml b/tests/unit/customizations/cloudformation/modules/proparray-expect.yaml new file mode 100644 index 000000000000..bb13421109cf --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/proparray-expect.yaml @@ -0,0 +1,7 @@ +Resources: + FooBaz: + Type: A::B::C + Properties: + AnArray: + - a + - b diff --git a/tests/unit/customizations/cloudformation/modules/proparray-module.yaml b/tests/unit/customizations/cloudformation/modules/proparray-module.yaml new file mode 100644 index 000000000000..412c4b62f79c --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/proparray-module.yaml @@ -0,0 +1,9 @@ +Parameters: + Bar: + Type: Array + +Resources: + Baz: + Type: A::B::C + Properties: + AnArray: !Ref Bar diff --git a/tests/unit/customizations/cloudformation/modules/proparray-template.yaml b/tests/unit/customizations/cloudformation/modules/proparray-template.yaml new file mode 100644 index 000000000000..ebed47dac043 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/proparray-template.yaml @@ -0,0 +1,8 @@ +Resources: + Foo: + Type: LocalModule + Source: proparray-module.yaml + Properties: + Bar: + - a + - b diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index bcd81bc9d627..c3ef4a9629bf 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -101,6 +101,7 @@ def test_main(self): "example", "getatt", "constant", + "proparray", ] for test in tests: base = "unit/customizations/cloudformation/modules" From 73fd605b2d20d3863495a4fdca0a15d06604651f Mon Sep 17 00:00:00 2001 From: "Eric Z. Beard" Date: Tue, 4 Feb 2025 16:53:50 -0800 Subject: [PATCH 37/43] Fixing unit tests --- .../cloudformation/module_constants.py | 10 +------ .../customizations/cloudformation/modules.py | 26 ++++++++++++++----- .../cloudformation/parse_sub.py | 8 ++++++ .../cloudformation/modules/output-expect.yaml | 26 ++++++++++++------- .../modules/output-module2.yaml | 9 +++++++ .../modules/output-template.yaml | 7 ++++- .../cloudformation/test_modules.py | 7 +++-- 7 files changed, 64 insertions(+), 29 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/output-module2.yaml diff --git a/awscli/customizations/cloudformation/module_constants.py b/awscli/customizations/cloudformation/module_constants.py index db28525118af..a3c32d7b3581 100644 --- a/awscli/customizations/cloudformation/module_constants.py +++ b/awscli/customizations/cloudformation/module_constants.py @@ -44,6 +44,7 @@ from collections import OrderedDict from awscli.customizations.cloudformation.parse_sub import WordType from awscli.customizations.cloudformation.parse_sub import parse_sub +from awscli.customizations.cloudformation.parse_sub import is_sub_needed from awscli.customizations.cloudformation.module_visitor import Visitor from awscli.customizations.cloudformation import exceptions @@ -122,12 +123,3 @@ def vf(v): def isdict(d): "Returns true if d is a dict" return isinstance(d, (dict, OrderedDict)) - - -def is_sub_needed(s): - "Returns true if the string has any Sub variables" - words = parse_sub(s) - for w in words: - if w.t != WordType.STR: - return True - return False diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index c61d5e42bc3d..5e78aa6dcc89 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -136,7 +136,10 @@ ) from awscli.customizations.cloudformation.parse_sub import WordType -from awscli.customizations.cloudformation.parse_sub import parse_sub +from awscli.customizations.cloudformation.parse_sub import ( + parse_sub, + is_sub_needed, +) from awscli.customizations.cloudformation.module_conditions import ( parse_conditions, ) @@ -556,9 +559,13 @@ def resolve_output_getatt(self, v, d, n): 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: + if REF in output: + ref = output[REF] + d[n] = {GETATT: [self.name, ref]} + elif GETATT in output: getatt = output[GETATT] if len(getatt) < 2: msg = f"GetAtt {getatt} in Output {v[1]} is invalid" @@ -768,29 +775,33 @@ def resolve_sub(self, v, d, n): """ words = parse_sub(v, True) 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 + "}" - need_sub = True elif word.t == WordType.REF: 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: if REF in found: resolved = "${" + found[REF] + "}" elif SUB in found: resolved = found[SUB] + elif GETATT in found: + tokens = found[GETATT] + if len(tokens) < 2: + msg = ( + "Invalid Sub referencing a GetAtt. " + + f"{word.w}: {found}" + ) + raise exceptions.InvalidModuleError(msg=msg) + resolved = "${" + ".".join(tokens) + "}" sub += resolved elif word.t == WordType.GETATT: - need_sub = True resolved = "${" + word.w + "}" tokens = word.w.split(".", 1) if len(tokens) < 2: @@ -801,6 +812,7 @@ def resolve_sub(self, v, d, n): resolved = "${" + tokens[0] + "." + tokens[1] + "}" sub += resolved + need_sub = is_sub_needed(sub) if need_sub: d[n] = {SUB: sub} else: diff --git a/awscli/customizations/cloudformation/parse_sub.py b/awscli/customizations/cloudformation/parse_sub.py index 18153c9f63f5..7dcae4364341 100644 --- a/awscli/customizations/cloudformation/parse_sub.py +++ b/awscli/customizations/cloudformation/parse_sub.py @@ -47,6 +47,14 @@ def __init__(self, word_type, word): def __str__(self): return f"{self.t} {self.w}" +def is_sub_needed(s): + "Returns true if the string has any Sub variables" + words = parse_sub(s) + for w in words: + if w.t != WordType.STR: + return True + return False + #pylint: disable=too-many-branches,too-many-statements def parse_sub(sub_str, leave_bang=False): """ diff --git a/tests/unit/customizations/cloudformation/modules/output-expect.yaml b/tests/unit/customizations/cloudformation/modules/output-expect.yaml index a005d962c56b..e2eceac7c606 100644 --- a/tests/unit/customizations/cloudformation/modules/output-expect.yaml +++ b/tests/unit/customizations/cloudformation/modules/output-expect.yaml @@ -1,3 +1,19 @@ +Resources: + A: + Type: D::E::F + Properties: + Object: + Arn: + Fn::Sub: ${ContentBucket.Arn} + ContentBucket: + Type: AWS::S3::Bucket + Properties: + BucketName: foo + AnotherModuleFoo: + Type: A::B::C + Properties: + AName: + Fn::Sub: another-${ContentBucket.Arn} Outputs: ExampleOutput: Value: @@ -10,13 +26,3 @@ Outputs: ExampleGetSub: Value: Fn::Sub: ${ContentBucket.Arn} -Resources: - ContentBucket: - Type: AWS::S3::Bucket - Properties: - BucketName: foo - A: - Type: A::B::C - Properties: - Object: - Arn: !Sub ${ContentBucket.Arn} diff --git a/tests/unit/customizations/cloudformation/modules/output-module2.yaml b/tests/unit/customizations/cloudformation/modules/output-module2.yaml new file mode 100644 index 000000000000..6b55b250b88d --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/output-module2.yaml @@ -0,0 +1,9 @@ +Parameters: + Name: + Type: String +Resources: + Foo: + Type: A::B::C + Properties: + AName: !Sub another-${Name} + diff --git a/tests/unit/customizations/cloudformation/modules/output-template.yaml b/tests/unit/customizations/cloudformation/modules/output-template.yaml index 19526cc9e97d..def8d0047cc1 100644 --- a/tests/unit/customizations/cloudformation/modules/output-template.yaml +++ b/tests/unit/customizations/cloudformation/modules/output-template.yaml @@ -3,9 +3,14 @@ Modules: Source: ./output-module.yaml Properties: Name: foo + AnotherModule: + Source: ./output-module2.yaml + Properties: + Name: !GetAtt ContentBucket.Arn + Resources: A: - Type: A::B::C + Type: D::E::F Properties: Object: Arn: !Sub ${Content.BucketArn} diff --git a/tests/unit/customizations/cloudformation/test_modules.py b/tests/unit/customizations/cloudformation/test_modules.py index c3ef4a9629bf..2ba19821a561 100644 --- a/tests/unit/customizations/cloudformation/test_modules.py +++ b/tests/unit/customizations/cloudformation/test_modules.py @@ -84,6 +84,9 @@ def test_merge_props(self): def test_main(self): "Run tests on sample templates that include local modules" + # pylint: disable=invalid-name + self.maxDiff = None + # The tests are in the modules directory. # Each test has 3 files: # test-template.yaml, test-module.yaml, and test-expect.yaml @@ -119,8 +122,8 @@ def test_main(self): # Resources with Type LocalModule td = modules.process_resources_section(td, base, t, None) - processed = yamlhelper.yaml_dump(td) - self.assertEqual(e, processed) + processed = yamlhelper.yaml_dump(td) + self.assertEqual(e, processed, f"{test} failed") def test_visitor(self): "Test module_visitor" From b1c6a7537f649470f33a20efe8dc25b37c9398f0 Mon Sep 17 00:00:00 2001 From: "Eric Z. Beard" Date: Tue, 4 Feb 2025 16:55:59 -0800 Subject: [PATCH 38/43] Fixed conditional test --- .../cloudformation/modules/conditional-expect.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml b/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml index 65e290258657..7d0310f13709 100644 --- a/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml +++ b/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml @@ -12,7 +12,6 @@ Resources: KeyType: HASH ALambdaPolicy: Type: AWS::IAM::RolePolicy - Condition: IfLambdaRoleIsSet Metadata: Comment: This resource is created only if the LambdaRoleArn is set Properties: From 6b36ca234adce6167b8ee153c6a247d561f124fe Mon Sep 17 00:00:00 2001 From: "Eric Z. Beard" Date: Wed, 5 Feb 2025 10:47:04 -0800 Subject: [PATCH 39/43] Implement conditionals within resources --- .../cloudformation/module_visitor.py | 2 +- .../customizations/cloudformation/modules.py | 48 +++++++++++++++++++ .../modules/conditional-expect.yaml | 10 ++++ .../modules/conditional-module.yaml | 19 ++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) diff --git a/awscli/customizations/cloudformation/module_visitor.py b/awscli/customizations/cloudformation/module_visitor.py index f12fdebc3c71..43223c492982 100644 --- a/awscli/customizations/cloudformation/module_visitor.py +++ b/awscli/customizations/cloudformation/module_visitor.py @@ -58,7 +58,7 @@ def visit(self, visit_func): def walk(visitor): visit_func(visitor) if isinstance(visitor.d, (dict, OrderedDict)): - for k, v in visitor.d.items(): + for k, v in visitor.d.copy().items(): walk(Visitor(v, visitor.d, k)) if isinstance(visitor.d, list): for i, v in enumerate(visitor.d): diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 5e78aa6dcc89..e64a1f54d341 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -143,6 +143,7 @@ from awscli.customizations.cloudformation.module_conditions import ( parse_conditions, ) +from awscli.customizations.cloudformation.module_visitor import Visitor LOG = logging.getLogger(__name__) @@ -172,6 +173,7 @@ INDEX_PLACEHOLDER = "$MapIndex" CONDITIONS = "Conditions" CONDITION = "Condition" +IF = "Fn::If" def process_module_section(template, base_path, parent_path): @@ -642,6 +644,52 @@ def process_resource(self, logical_id, resource): self.template[RESOURCES][self.name + logical_id] = resource + self.process_resource_conditions() + + def process_resource_conditions(self): + "Visit all resources to look for Fn::If conditions" + + # Example + # + # Resources + # Foo: + # Properties: + # Something: + # Fn::If: + # - ConditionName + # - AnObject + # - !Ref AWS::NoValue + # + # In this case, delete the 'Something' node entirely + # Otherwise replace the Fn::If with the correct value + def vf(v): + if isdict(v.d) and IF in v.d and v.p is not None: + conditional = v.d[IF] + print(f"\nIF in {self.name}: ") + print(" v:", v) + print(" conditional:", conditional) + if len(conditional) != 3: + msg = f"Invalid conditional in {self.name}: {conditional}" + raise exceptions.InvalidModuleError(msg=msg) + condition_name = conditional[0] + trueval = conditional[1] + falseval = conditional[2] + if condition_name not in self.conditions: + return # Assume this is a parent template condition? + if self.conditions[condition_name]: + v.p[v.k] = trueval + else: + v.p[v.k] = falseval + newval = v.p[v.k] + if isdict(newval) and REF in newval: + if newval[REF] == "AWS::NoValue": + del v.p[v.k] + + v = Visitor(self.resources) + v.visit(vf) + v = Visitor(self.outputs) + v.visit(vf) + def process_overrides(self, logical_id, resource, attr_name): """ Replace overridden values in a property-like attribute of a resource. diff --git a/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml b/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml index 7d0310f13709..b07ee887a052 100644 --- a/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml +++ b/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml @@ -10,6 +10,11 @@ Resources: KeySchema: - AttributeName: id KeyType: HASH + ATestResourceConditional: + Type: A::B::C + Properties: + Show: Foo + ShowA: A ALambdaPolicy: Type: AWS::IAM::RolePolicy Metadata: @@ -43,3 +48,8 @@ Resources: KeySchema: - AttributeName: id KeyType: HASH + BTestResourceConditional: + Type: A::B::C + Properties: + Hide: Foo + ShowA: B diff --git a/tests/unit/customizations/cloudformation/modules/conditional-module.yaml b/tests/unit/customizations/cloudformation/modules/conditional-module.yaml index f2639ba3b53e..23778eea2f75 100644 --- a/tests/unit/customizations/cloudformation/modules/conditional-module.yaml +++ b/tests/unit/customizations/cloudformation/modules/conditional-module.yaml @@ -28,6 +28,25 @@ Resources: - AttributeName: id KeyType: HASH + TestResourceConditional: + Type: A::B::C + Properties: + Show: + Fn::If: + - IfLambdaRoleIsSet + - Foo + - !Ref AWS::NoValue + Hide: + Fn::If: + - IfLambdaRoleIsSet + - !Ref AWS::NoValue + - Foo + ShowA: + Fn::If: + - IfLambdaRoleIsSet + - A + - B + LambdaPolicy: Type: AWS::IAM::RolePolicy Condition: IfLambdaRoleIsSet From 2181e56835843b3e6dd8c37ca8355b6b6a5a18f7 Mon Sep 17 00:00:00 2001 From: "Eric Z. Beard" Date: Wed, 5 Feb 2025 14:40:42 -0800 Subject: [PATCH 40/43] Fixed output getatt ref --- awscli/customizations/cloudformation/modules.py | 15 ++++++++++----- .../cloudformation/modules/output-expect.yaml | 7 +++++++ .../cloudformation/modules/output-module.yaml | 1 + .../cloudformation/modules/output-template.yaml | 3 +++ 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index e64a1f54d341..1240c9069cef 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -557,7 +557,15 @@ def resolve_output_sub(self, v, d, n): # pylint:disable=too-many-branches def resolve_output_getatt(self, v, d, n): - "Resolve a GetAtt that refers to a module output" + """ + Resolve a GetAtt that refers to a module output. + + :param v The value + :param d The dictionary + :param n The name of the node + + This function sets d[n] + """ if not isinstance(v, list) or len(v) < 2: msg = f"GetAtt {v} invalid" raise exceptions.InvalidModuleError(msg=msg) @@ -566,7 +574,7 @@ def resolve_output_getatt(self, v, d, n): output = self.outputs[v[1]] if REF in output: ref = output[REF] - d[n] = {GETATT: [self.name, ref]} + d[n] = {REF: self.name + ref} elif GETATT in output: getatt = output[GETATT] if len(getatt) < 2: @@ -665,9 +673,6 @@ def process_resource_conditions(self): def vf(v): if isdict(v.d) and IF in v.d and v.p is not None: conditional = v.d[IF] - print(f"\nIF in {self.name}: ") - print(" v:", v) - print(" conditional:", conditional) if len(conditional) != 3: msg = f"Invalid conditional in {self.name}: {conditional}" raise exceptions.InvalidModuleError(msg=msg) diff --git a/tests/unit/customizations/cloudformation/modules/output-expect.yaml b/tests/unit/customizations/cloudformation/modules/output-expect.yaml index e2eceac7c606..a66ea404de8d 100644 --- a/tests/unit/customizations/cloudformation/modules/output-expect.yaml +++ b/tests/unit/customizations/cloudformation/modules/output-expect.yaml @@ -5,6 +5,10 @@ Resources: Object: Arn: Fn::Sub: ${ContentBucket.Arn} + ArnGetAtt: + Fn::GetAtt: + - ContentBucket + - Arn ContentBucket: Type: AWS::S3::Bucket Properties: @@ -26,3 +30,6 @@ Outputs: ExampleGetSub: Value: Fn::Sub: ${ContentBucket.Arn} + ExampleRef: + Value: + Ref: ContentBucket diff --git a/tests/unit/customizations/cloudformation/modules/output-module.yaml b/tests/unit/customizations/cloudformation/modules/output-module.yaml index f685ca154d84..969c0c9e7dc7 100644 --- a/tests/unit/customizations/cloudformation/modules/output-module.yaml +++ b/tests/unit/customizations/cloudformation/modules/output-module.yaml @@ -9,4 +9,5 @@ Resources: Outputs: BucketArn: !GetAtt Bucket.Arn BucketArnSub: !Sub ${Bucket.Arn} + BucketRef: !Ref Bucket diff --git a/tests/unit/customizations/cloudformation/modules/output-template.yaml b/tests/unit/customizations/cloudformation/modules/output-template.yaml index def8d0047cc1..3f756701407e 100644 --- a/tests/unit/customizations/cloudformation/modules/output-template.yaml +++ b/tests/unit/customizations/cloudformation/modules/output-template.yaml @@ -14,6 +14,7 @@ Resources: Properties: Object: Arn: !Sub ${Content.BucketArn} + ArnGetAtt: !GetAtt Content.BucketArn Outputs: ExampleOutput: Value: !GetAtt Content.BucketArn @@ -21,3 +22,5 @@ Outputs: Value: !Sub ${Content.BucketArn} ExampleGetSub: Value: !GetAtt Content.BucketArnSub + ExampleRef: + Value: !GetAtt Content.BucketRef From db57872f52ed0d0abdaebf5d59b2d1136370943f Mon Sep 17 00:00:00 2001 From: "Eric Z. Beard" Date: Wed, 5 Feb 2025 16:11:27 -0800 Subject: [PATCH 41/43] Hide conditional modules --- .../customizations/cloudformation/modules.py | 54 +++++++++++-------- .../modules/conditional-expect.yaml | 2 + .../modules/conditional-module.yaml | 5 ++ .../modules/conditional-module2.yaml | 3 ++ 4 files changed, 42 insertions(+), 22 deletions(-) create mode 100644 tests/unit/customizations/cloudformation/modules/conditional-module2.yaml diff --git a/awscli/customizations/cloudformation/modules.py b/awscli/customizations/cloudformation/modules.py index 1240c9069cef..53493d3d2c94 100644 --- a/awscli/customizations/cloudformation/modules.py +++ b/awscli/customizations/cloudformation/modules.py @@ -156,7 +156,6 @@ UPDATEPOLICY = "UpdatePolicy" DELETIONPOLICY = "DeletionPolicy" UPDATEREPLACEPOLICY = "UpdateReplacePolicy" -CONDITION = "Condition" DEFAULT = "Default" NAME = "Name" SOURCE = "Source" @@ -413,6 +412,7 @@ def __str__(self): + f"source: {self.source}, props: {self.props}" ) + # pylint: disable=too-many-branches def process(self): """ Read the module source process it. @@ -441,6 +441,37 @@ def process(self): if OUTPUTS in module_dict: self.outputs = module_dict[OUTPUTS] + # Read the Conditions section and store it as a dict of string:boolean + if CONDITIONS in module_dict: + cs = module_dict[CONDITIONS] + + def find_ref(v): + return self.find_ref(v) + + try: + self.conditions = parse_conditions(cs, find_ref) + except Exception as e: + msg = f"Failed to process conditions in {self.source}: {e}" + LOG.exception(msg) + raise exceptions.InvalidModuleError(msg=msg) + + # Check each resource to see if a Condition omits it + for logical_id, resource in self.resources.copy().items(): + if CONDITION in resource: + if resource[CONDITION] in self.conditions: + if self.conditions[resource[CONDITION]] is False: + del self.resources[logical_id] + else: + del self.resources[logical_id][CONDITION] + + # Do the same for modules in the Modules section + if MODULES in module_dict: + for k, v in module_dict[MODULES].copy().items(): + if CONDITION in v: + if v[CONDITION] in self.conditions: + if self.conditions[v[CONDITION]] is False: + del module_dict[MODULES][k] + # Recurse on nested modules bp = os.path.dirname(self.source) section = "" @@ -456,19 +487,6 @@ def process(self): self.validate_overrides() - if CONDITIONS in module_dict: - cs = module_dict[CONDITIONS] - - def find_ref(v): - return self.find_ref(v) - - try: - self.conditions = parse_conditions(cs, find_ref) - except Exception as e: - msg = f"Failed to process conditions in {self.source}: {e}" - LOG.exception(msg) - raise exceptions.InvalidModuleError(msg=msg) - for logical_id, resource in self.resources.items(): self.process_resource(logical_id, resource) @@ -620,14 +638,6 @@ 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: - 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. attrs = [ diff --git a/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml b/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml index b07ee887a052..496f8daf07c8 100644 --- a/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml +++ b/tests/unit/customizations/cloudformation/modules/conditional-expect.yaml @@ -37,6 +37,8 @@ Resources: - Arn PolicyName: table-a-policy RoleName: my-role + AHideMeFoo: + Type: A::B::C BTable: Type: AWS::DynamoDB::Table Properties: diff --git a/tests/unit/customizations/cloudformation/modules/conditional-module.yaml b/tests/unit/customizations/cloudformation/modules/conditional-module.yaml index 23778eea2f75..6115232ee1d9 100644 --- a/tests/unit/customizations/cloudformation/modules/conditional-module.yaml +++ b/tests/unit/customizations/cloudformation/modules/conditional-module.yaml @@ -15,6 +15,11 @@ Conditions: - !Ref LambdaRoleName - "" +Modules: + HideMe: + Condition: IfLambdaRoleIsSet + Source: conditional-module2.yaml + Resources: Table: Type: AWS::DynamoDB::Table diff --git a/tests/unit/customizations/cloudformation/modules/conditional-module2.yaml b/tests/unit/customizations/cloudformation/modules/conditional-module2.yaml new file mode 100644 index 000000000000..3ec62e417fd0 --- /dev/null +++ b/tests/unit/customizations/cloudformation/modules/conditional-module2.yaml @@ -0,0 +1,3 @@ +Resources: + Foo: + Type: A::B::C From 2a4b64a13db1a66927e55c9a2d1e136811ddaf7d Mon Sep 17 00:00:00 2001 From: "Eric Z. Beard" Date: Fri, 7 Feb 2025 13:58:44 -0800 Subject: [PATCH 42/43] Move export --- awscli/customizations/cloudformation/artifact_exporter.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 7fabb4dfed87..15dee23dbeaf 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -683,8 +683,6 @@ def export(self): LOG.exception(msg) raise exceptions.InvalidModuleError(msg=msg) - self.template_dict = self.export_metadata(self.template_dict) - if RESOURCES not in self.template_dict: return self.template_dict @@ -700,9 +698,8 @@ def export(self): LOG.exception(msg) raise exceptions.InvalidModuleError(msg=msg) - + self.template_dict = self.export_metadata(self.template_dict) self.template_dict = self.export_global_artifacts(self.template_dict) - self.export_resources(self.template_dict[RESOURCES]) return self.template_dict From 20ca09eb87fad3070258f56f1ae524cb75cc5f8a Mon Sep 17 00:00:00 2001 From: "Eric Z. Beard" Date: Fri, 7 Feb 2025 15:28:07 -0800 Subject: [PATCH 43/43] Move export metadata back --- awscli/customizations/cloudformation/artifact_exporter.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 15dee23dbeaf..11eaec4f8245 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -683,6 +683,8 @@ def export(self): LOG.exception(msg) raise exceptions.InvalidModuleError(msg=msg) + self.template_dict = self.export_metadata(self.template_dict) + if RESOURCES not in self.template_dict: return self.template_dict @@ -698,7 +700,6 @@ def export(self): LOG.exception(msg) raise exceptions.InvalidModuleError(msg=msg) - self.template_dict = self.export_metadata(self.template_dict) self.template_dict = self.export_global_artifacts(self.template_dict) self.export_resources(self.template_dict[RESOURCES])