[Modules] Support parsing Yaml fragments with CFN short forms (#699)

* Issue 693 [Modules] Support parsing Yaml fragments with CFN short forms

* Issue 689 Only use cfn-flip for parsing module fragments

* Issue 689 Apply cfn-lint to the original file

To prevent any false warnings from we should apply cfn-lint to the original file and not rely on parsing and then serializing to json before running lint.

* Issue 693 Better error message when there is no module fragment file

* Issue 693 Ensure unhandled exceptions in cfn-lint don't block the user

When something unexpected happens when cfn-lint parses the template we don't want to block the user from running cfn submit. This should be an edge case though as we already parse the template before running cfn-lint albeit using a different library.

setup.cfg

@@ -29,7 +29,7 @@ include_trailing_comma = true
 combine_as_imports = True
 force_grid_wrap = 0
 known_first_party = rpdk
-known_third_party = boto3,botocore,cfnlint,colorama,docker,hypothesis,jinja2,jsonschema,nested_lookup,ordered_set,pkg_resources,pytest,pytest_localserver,setuptools,yaml
+known_third_party = boto3,botocore,cfn_tools,cfnlint,colorama,docker,hypothesis,jinja2,jsonschema,nested_lookup,ordered_set,pkg_resources,pytest,pytest_localserver,setuptools,yaml
 
 [tool:pytest]
 # can't do anything about 3rd part modules, so don't spam us

setup.py

@@ -52,6 +52,7 @@ def find_version(*file_paths):
         "docker>=4.3.1",
         "ordered-set>=4.0.2",
         "cfn-lint>=0.43.0",
+        "cfn_flip>=1.2.3",
         "nested-lookup",
     ],
     entry_points={

src/rpdk/core/cli.py

@@ -131,8 +131,7 @@ def no_command(args):
         print("=== Unhandled exception ===", file=sys.stderr)
         print("Please report this issue to the team.", file=sys.stderr)
         print(
-            "Issue tracker: "
-            "https://github.com/aws-cloudformation/aws-cloudformation-rpdk/issues",
+            "Issue tracker: github.com/aws-cloudformation/cloudformation-cli/issues",
             file=sys.stderr,
         )
 

src/rpdk/core/fragment/generator.py

@@ -11,19 +11,17 @@
 import os
 from pathlib import Path
 
-import cfnlint.config
-import cfnlint.core
-import yaml
-
 from rpdk.core.data_loaders import resource_json
 from rpdk.core.exceptions import FragmentValidationError
 
+from .lint_warning_printer import print_cfn_lint_warnings
+from .module_fragment_reader import get_template_file_size_in_bytes, read_raw_fragments
+
 LOG = logging.getLogger(__name__)
 FRAGMENT_DIR = "fragments"
 SAMPLE_FRAGMENT_OUTPUT = "sample.json"
 SCHEMA_NAME = "schema.json"
 SAMPLE_FRAGMENT = "../data/examples/module/sample.json"
-ALLOWED_EXTENSIONS = {".json", ".yaml", ".yml"}
 RESOURCE_LIMIT = 500
 OUTPUT_LIMIT = 200
 MAPPING_LIMIT = 200
@@ -45,7 +43,7 @@ def __init__(self, type_name, root=None):
         LOG.debug("Fragment directory: %s", self.fragment_dir)
 
     def generate_schema(self):
-        raw_fragments = self._read_raw_fragments()
+        raw_fragments = read_raw_fragments(self.fragment_dir)
 
         schema = {}
         properties = {}
@@ -70,58 +68,19 @@ def validate_fragments(self):
         Note: Fn::ImportValue was checked when loading the fragments
             since it can occur anywhere in the template.
         """
-        raw_fragments = self._read_raw_fragments()
+        raw_fragments = read_raw_fragments(self.fragment_dir)
         self.__validate_file_size_limit()
         self.__validate_resources(raw_fragments)
         self.__validate_parameters(raw_fragments)
         self.__validate_no_transforms_present(raw_fragments)
         self.__validate_outputs(raw_fragments)
         self.__validate_mappings(raw_fragments)
-        self.__validate_fragment_thru_cfn_lint(raw_fragments)
-
-    def __validate_fragment_thru_cfn_lint(self, raw_fragments):
-        lint_warnings = self.__get_cfn_lint_matches(raw_fragments)
-        if not lint_warnings:
-            LOG.warning("Module fragment is valid.")
-        else:
-            LOG.warning(
-                "Module fragment is probably valid, but there are "
-                "warnings/errors from cfn-lint "
-                "(https://github.com/aws-cloudformation/cfn-python-lint):"
-            )
-            for lint_warning in lint_warnings:
-                print(
-                    "\t{} (from rule {})".format(
-                        lint_warning.message, lint_warning.rule
-                    ),
-                )
+        print_cfn_lint_warnings(self.fragment_dir)
 
     def __validate_outputs(self, raw_fragments):
         self.__validate_no_exports_present(raw_fragments)
         self.__validate_output_limit(raw_fragments)
 
-    @staticmethod
-    def __get_cfn_lint_matches(raw_fragment):
-        filename = "temporary_fragment.json"
-
-        with open(filename, "w") as outfile:
-            json.dump(raw_fragment, outfile, indent=4, default=str)
-
-        template = cfnlint.decode.cfn_json.load(filename)
-
-        # Initialize the ruleset to be applied (no overrules, no excludes)
-        rules = cfnlint.core.get_rules([], [], [], [], False, [])
-
-        # Default region used by cfn-lint
-        regions = ["us-east-1"]
-
-        # Runs Warning and Error rules
-        matches = cfnlint.core.run_checks(filename, template, rules, regions)
-
-        os.remove(filename)
-
-        return matches
-
     @staticmethod
     def __validate_no_exports_present(raw_fragments):
         if "Outputs" in raw_fragments:
@@ -236,16 +195,13 @@ def __validate_mapping_attribute_limit(self, raw_fragments):
                     )
 
     def __validate_file_size_limit(self):
-        total_size = self.__get_template_file_size_in_bytes()
+        total_size = get_template_file_size_in_bytes(self.fragment_dir)
         if total_size > self.template_file_size_in_bytes_limit:
             raise FragmentValidationError(
                 "The total file size of the template"
                 " fragments exceeds the CloudFormation Template size limit"
             )
 
-    def __get_template_file_size_in_bytes(self):
-        return os.stat(self._get_fragment_file()).st_size
-
     @staticmethod
     def __build_resources(raw_fragments):
         raw_resources = {}
@@ -328,37 +284,6 @@ def _create_fragment_directory(self):
         else:
             print("Directory ", self.fragment_dir, " already exists")
 
-    def _read_raw_fragments(self):
-        return self._load_fragment(self._get_fragment_file())
-
-    def _load_fragment(self, fragment_file):
-        try:
-            with open(fragment_file, "r", encoding="utf-8") as f:
-                return yaml.safe_load(
-                    self.__first_pass_syntax_check(self.__convert_function(f.read()))
-                )
-        except (json.JSONDecodeError, yaml.parser.ParserError) as e:
-            raise FragmentValidationError(
-                "Fragment file '{}' is invalid: {}".format(fragment_file, str(e))
-            ) from e
-
-    def _get_fragment_file(self):
-        all_fragment_files = []
-        for root, _directories, files in os.walk(self.fragment_dir):
-            for f in files:
-                ext = os.path.splitext(f)[-1].lower()
-                if ext in ALLOWED_EXTENSIONS:
-                    all_fragment_files.append(os.path.join(root, f))
-        if len(all_fragment_files) > 1:
-            raise FragmentValidationError(
-                "A Module can only consist of a "
-                "single template file, but there are "
-                + str(len(all_fragment_files))
-                + ": "
-                + str(all_fragment_files)
-            )
-        return all_fragment_files[0]
-
     @staticmethod
     def _overwrite(path, contents):
         LOG.debug("Overwriting '%s'", path)
@@ -367,23 +292,3 @@ def _overwrite(path, contents):
                 contents(f)
             else:
                 f.write(contents)
-
-    @staticmethod
-    def __first_pass_syntax_check(template):
-        if "Fn::ImportValue" in template:
-            raise FragmentValidationError(
-                "Template fragment can't contain any Fn::ImportValue."
-            )
-        return template
-
-    @staticmethod
-    def __convert_function(template):
-        """
-        When generating schema, we don't care about the actual reference.
-        So the following will only make a valid YAML file.
-        """
-        return (
-            template.replace("!Transform", "Fn::Transform")
-            .replace("!ImportValue", "Fn::ImportValue")
-            .replace("!", "")
-        )

src/rpdk/core/fragment/lint_warning_printer.py

@@ -0,0 +1,53 @@
+import json
+import logging
+
+import cfnlint.config
+import cfnlint.core
+
+from .module_fragment_reader import _get_fragment_file
+
+LOG = logging.getLogger(__name__)
+
+
+def print_cfn_lint_warnings(fragment_dir):
+    lint_warnings = __get_cfn_lint_matches(fragment_dir)
+    if not lint_warnings:
+        LOG.warning("Module fragment is valid.")
+    else:
+        LOG.warning(
+            "Module fragment might be valid, but there are "
+            "warnings from cfn-lint "
+            "(https://github.com/aws-cloudformation/cfn-python-lint):"
+        )
+        for lint_warning in lint_warnings:
+            print(
+                "\t{} (from rule {})".format(lint_warning.message, lint_warning.rule),
+            )
+
+
+def __get_cfn_lint_matches(fragment_dir):
+    filepath = _get_fragment_file(fragment_dir)
+    try:
+        try:
+            template = cfnlint.decode.cfn_json.load(filepath)
+        except json.decoder.JSONDecodeError:
+            template = cfnlint.decode.cfn_yaml.load(filepath)
+    except Exception as e:  # pylint: disable=broad-except
+        LOG.error(
+            "Skipping cfn-lint validation due to an internal error.\n"
+            "Please report this issue to the team (include rpdk.log file)\n"
+            "Issue tracker: github.com/aws-cloudformation/cloudformation-cli/issues"
+        )
+        LOG.error(str(e))
+        return []
+
+    # Initialize the ruleset to be applied (no overrules, no excludes)
+    rules = cfnlint.core.get_rules([], [], [], [], False, [])
+
+    # Default region used by cfn-lint
+    regions = ["us-east-1"]
+
+    # Runs Warning and Error rules
+    matches = cfnlint.core.run_checks(filepath, template, rules, regions)
+
+    return matches

src/rpdk/core/fragment/module_fragment_reader.py

@@ -0,0 +1,58 @@
+import logging
+import os
+
+import yaml
+from cfn_tools import load_yaml
+
+from rpdk.core.exceptions import FragmentValidationError
+
+LOG = logging.getLogger(__name__)
+ALLOWED_EXTENSIONS = {".json", ".yaml", ".yml"}
+
+
+def read_raw_fragments(fragment_dir):
+    return _load_fragment(_get_fragment_file(fragment_dir))
+
+
+def get_template_file_size_in_bytes(fragment_dir):
+    return os.stat(_get_fragment_file(fragment_dir)).st_size
+
+
+def _load_fragment(fragment_file):
+    try:
+        with open(fragment_file, "r", encoding="utf-8") as f:
+            return load_yaml(__first_pass_syntax_check(f.read()))
+    except (yaml.parser.ParserError, yaml.scanner.ScannerError) as e:
+        raise FragmentValidationError(
+            "Fragment file '{}' is invalid: {}".format(fragment_file, str(e))
+        ) from e
+
+
+def _get_fragment_file(fragment_dir):
+    all_fragment_files = []
+    for root, _directories, files in os.walk(fragment_dir):
+        for f in files:
+            ext = os.path.splitext(f)[-1].lower()
+            if ext in ALLOWED_EXTENSIONS:
+                all_fragment_files.append(os.path.join(root, f))
+    if len(all_fragment_files) == 0:
+        raise FragmentValidationError(
+            f"No module fragment files found in the fragments folder ending on one of {ALLOWED_EXTENSIONS}"
+        )
+    if len(all_fragment_files) > 1:
+        raise FragmentValidationError(
+            "A Module can only consist of a "
+            "single template file, but there are "
+            + str(len(all_fragment_files))
+            + ": "
+            + str(all_fragment_files)
+        )
+    return all_fragment_files[0]
+
+
+def __first_pass_syntax_check(template):
+    if "Fn::ImportValue" in template or "!ImportValue" in template:
+        raise FragmentValidationError(
+            "Template fragment can't contain any Fn::ImportValue."
+        )
+    return template

tests/data/sample_fragments/ec2withsub.yaml

@@ -0,0 +1,13 @@
+AWSTemplateFormatVersion: "2010-09-09"
+Resources:
+  EC2Instance:
+    Type: AWS::EC2::Instance
+    Properties:
+      ImageId: ami-09c5e030f74651050
+      UserData:
+        Fn::Base64: !Sub |
+          #!/bin/bash -e
+          sudo yum install -y https://s3.region.amazonaws.com/amazon-ssm-region/latest/linux_amd64/amazon-ssm-agent.rpm
+          sudo systemctl status amazon-ssm-agent
+          sudo systemctl enable amazon-ssm-agent
+          sudo systemctl start amazon-ssm-agent