Repository: aws-quickstart/taskcat
Pull request #518 (2020-03-10 22:12:17)

cache cfnlint template objects
This addresses concurrency/scale issues identified when a project has broad region coverage, multiple tests, and `s3_regional_buckets: true`. 

Each template file in a project would be opened and read from the filesystem tests*regions*number_of_uses_as_child times. 

an ssm client was also created every time an s3url needed to be resolved, this is now done using a static mapping that is auto-updated via pre-commit.

Also, the recent template parsing addition has added several debug log statements that print values, imho these could be best obtained from an interactive debugger when troubleshooting and shouldn't be in taskcat outputs.



Patch:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index e0a1e32..b5b2a74 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -10,6 +10,7 @@ repos:
   rev: 19.10b0
   hooks:
   - id: black
+    exclude: regions_to_partitions.py
 - repo: https://github.com/pre-commit/pre-commit-hooks
   rev: v2.4.0
   hooks:
@@ -74,6 +75,13 @@ repos:
     entry: git-secrets --scan
     language: system
     always_run: true
+  - id: update-region-mappings
+    name: update-region-mappings
+    description: update region-partition mappings
+    entry: ./update_partition_region_map.py
+    language: system
+    pass_filenames: false
+    always_run: true
   - id: pylint-local
     name: pylint-local
     description: Run pylint in the local virtualenv
diff --git a/taskcat/_cfn/stack.py b/taskcat/_cfn/stack.py
index a6ffc40..7135ada 100644
--- a/taskcat/_cfn/stack.py
+++ b/taskcat/_cfn/stack.py
@@ -13,7 +13,7 @@ from uuid import UUID, uuid4
 import boto3
 import yaml
 
-from taskcat._cfn.template import Template
+from taskcat._cfn.template import Template, tcat_template_cache
 from taskcat._common_utils import ordered_dump, pascal_to_snake, s3_url_maker
 from taskcat._dataclasses import Tag, TestRegion
 
@@ -274,6 +274,7 @@ class Stack:  # pylint: disable=too-many-instance-attributes
                 region.client("s3"),
                 region.s3_bucket.auto_generated,
             ),
+            template_cache=tcat_template_cache,
         )
         stack_id = cfn_client.create_stack(
             StackName=stack_name,
@@ -339,6 +340,7 @@ class Stack:  # pylint: disable=too-many-instance-attributes
             template_path=str(absolute_path),
             project_root=parent_stack.template.project_root,
             url=url,
+            template_cache=tcat_template_cache,
         )
         stack = cls(
             parent_stack.region,
diff --git a/taskcat/_cfn/stack_url_helper.py b/taskcat/_cfn/stack_url_helper.py
index e5df472..b185a3f 100644
--- a/taskcat/_cfn/stack_url_helper.py
+++ b/taskcat/_cfn/stack_url_helper.py
@@ -117,7 +117,6 @@ class StackURLHelper:
     @staticmethod
     def values_to_dict(values):
         """Rewrite sub vars with actual variable values"""
-        LOG.debug("Rewrite Values: {}".format(values))
         # Create dictionary of values
         values_dict_string = values.replace("(", "{")
         values_dict_string = values_dict_string.replace(")", "}")
@@ -144,8 +143,6 @@ class StackURLHelper:
                         value, '"' + value + '"'
                     )
 
-        LOG.debug("Rewrite Values cleaned: {}".format(values_dict_string))
-
         values_dict = json.loads(values_dict_string)
 
         return values_dict
@@ -154,8 +151,6 @@ class StackURLHelper:
         """ Return expression with values replaced """
         results = []
 
-        LOG.debug("Fn::Sub: '{}'".format(expression))
-
         # Builtins - Fudge some defaults here since we don't have runtime info
         # ${AWS::Region} ${AWS::AccountId}
         expression = self.rewrite_sub_vars_with_values(expression, self.SUBSTITUTION)
@@ -163,9 +158,7 @@ class StackURLHelper:
         # Handle Sub of form [ StringToSub, { "key" : "value", "key": "value" }]
         if "[" in expression:
             temp_expression = expression.split("[")[1].split(",")[0]
-            LOG.debug("Fn::Sub: (expression) {}".format(temp_expression))
             values = expression.split("[")[1].split("(")[1].split(")")[0]
-            LOG.debug("Fn::Sub: (values) {}".format(values))
             values = self.values_to_dict("(" + values + ")")
             temp_expression = self.rewrite_sub_vars_with_values(temp_expression, values)
         else:
@@ -175,7 +168,6 @@ class StackURLHelper:
         result = self.rewrite_sub_vars(temp_expression)
 
         results.append(result)
-        LOG.debug("Fn::Sub: '{}'".format(temp_expression))
 
         return results
 
@@ -204,13 +196,11 @@ class StackURLHelper:
     def evaluate_fn_if(expression):
         """ Return both possible parts of the expression """
         results = []
-        LOG.debug("Fn::If: {}".format(expression))
         value_true = expression.split(",")[1].strip()
         value_false = expression.split(",")[2].strip().strip("]")
         # if we don't have '' this can break things
         results.append("'" + value_true.strip("'") + "'")
         results.append("'" + value_false.strip("'") + "'")
-        LOG.debug("Fn::If: {}".format(results))
         return results
 
     def evaluate_fn_ref(self, expression):
@@ -219,12 +209,9 @@ class StackURLHelper:
         results = []
 
         temp = expression.split(": ")[1]
-        LOG.debug("Ref: {}".format(temp))
         if temp.strip("'") in self.SUBSTITUTION.keys():
-            LOG.debug("Ref: (found) {}".format(temp))
             temp = self.SUBSTITUTION[temp.strip("'")]
             temp = "'" + temp + "'"
-            LOG.debug("Ref: (found) {}".format(temp))
 
         results.append(temp)
 
@@ -311,13 +298,9 @@ class StackURLHelper:
 
             for replacement in replacements:
                 template_url_temp = template_url
-                LOG.debug("evaluate_string: (before) {}".format(template_url))
-                LOG.debug("expression: {}".format(parts[0]))
-                LOG.debug("replacement: {}".format(replacement))
                 template_url_temp = template_url_temp.replace(
                     "{" + parts[0] + "}", replacement
                 )
-                LOG.debug("evaluate_string: (after) {}".format(template_url_temp))
 
                 evaluated_strings = self.evaluate_string(
                     template_url_temp, depth=(depth + 1)
@@ -394,7 +377,6 @@ class StackURLHelper:
             final_template_path = Path(
                 "/".join([str(project_root), str(child_template_path_tmp)])
             )
-            LOG.debug("find_local_child_template: {}".format(final_template_path))
             if final_template_path.exists() and final_template_path.is_file():
                 return str(final_template_path)
 
@@ -410,7 +392,6 @@ class StackURLHelper:
             final_template_path = Path(
                 "/".join([str(project_root), str(child_template_path_tmp)])
             )
-            LOG.debug("find_local_child_template: {}".format(final_template_path))
             if final_template_path.exists() and final_template_path.is_file():
                 return str(final_template_path)
 
diff --git a/taskcat/_cfn/template.py b/taskcat/_cfn/template.py
index 4fd12b0..173bfc7 100644
--- a/taskcat/_cfn/template.py
+++ b/taskcat/_cfn/template.py
@@ -1,6 +1,7 @@
 import logging
 import re
 from pathlib import Path
+from time import sleep
 from typing import Dict, List, Union
 
 import cfnlint
@@ -10,6 +11,31 @@ from taskcat.exceptions import TaskCatException
 LOG = logging.getLogger(__name__)
 
 
+class TemplateCache:
+    def __init__(self, store: dict = None):
+        self._templates = store if store else {}
+        self._lock: Dict[str, bool] = {}
+
+    def get(self, template_path: str) -> cfnlint.Template:
+        while self._lock.get(template_path):
+            sleep(0.1)
+        if template_path not in self._templates:
+            try:
+                self._lock[template_path] = True
+                self._templates[template_path] = cfnlint.decode.cfn_yaml.load(
+                    template_path
+                )
+                self._lock[template_path] = False
+            except Exception:  # pylint: disable=broad-except
+                self._lock[template_path] = False
+                raise
+        return self._templates[template_path]
+
+
+template_cache_store: Dict[str, cfnlint.Template] = {}
+tcat_template_cache = TemplateCache(template_cache_store)  # pylint: disable=C0103
+
+
 class Template:
     def __init__(
         self,
@@ -17,9 +43,11 @@ class Template:
         project_root: Union[str, Path] = "",
         url: str = "",
         s3_key_prefix: str = "",
+        template_cache: TemplateCache = tcat_template_cache,
     ):
+        self.template_cache = template_cache
         self.template_path: Path = Path(template_path).expanduser().resolve()
-        self.template = cfnlint.decode.cfn_yaml.load(str(self.template_path))
+        self.template = self.template_cache.get(str(self.template_path))
         with open(template_path, "r") as file_handle:
             self.raw_template = file_handle.read()
         project_root = (
@@ -66,9 +94,6 @@ class Template:
         template_parameters=None,
     ):
         try:
-            LOG.debug(
-                "Evaluating TemplateURL expression: '%s'", template_url,
-            )
 
             helper = StackURLHelper(
                 template_mappings=template_mappings,
@@ -80,7 +105,6 @@ class Template:
             )
 
             if len(urls) > 0:
-                LOG.debug("TemplateURL '%s' evaluated to '%s'", template_url, urls[0])
                 return urls[0]
 
         except Exception as e:  # pylint: disable=broad-except
@@ -141,6 +165,7 @@ class Template:
                         self.project_root,
                         self._get_relative_url(child),
                         self._s3_key_prefix,
+                        tcat_template_cache,
                     )
                 except Exception:  # pylint: disable=broad-except
                     LOG.debug("Traceback:", exc_info=True)
diff --git a/taskcat/_common_utils.py b/taskcat/_common_utils.py
index 6ef778e..d70e146 100644
--- a/taskcat/_common_utils.py
+++ b/taskcat/_common_utils.py
@@ -8,11 +8,11 @@ from functools import reduce
 from pathlib import Path
 from time import sleep
 
-import boto3
 import yaml
 
 from dulwich.config import ConfigFile, parse_submodules
 from taskcat.exceptions import TaskCatException
+from taskcat.regions_to_partitions import REGIONS
 
 LOG = logging.getLogger(__name__)
 
@@ -55,12 +55,11 @@ def s3_url_maker(bucket, key, s3_client, autobucket=False):
     return url
 
 
-def get_s3_domain(region, ssm_client=None):
-    ssm_client = ssm_client if ssm_client else boto3.client("ssm")
-    partition = ssm_client.get_parameter(
-        Name=f"/aws/service/global-infrastructure/regions/{region}/partition"
-    )["Parameter"]["Value"]
-    return S3_PARTITION_MAP[partition]
+def get_s3_domain(region):
+    try:
+        return S3_PARTITION_MAP[REGIONS[region]]
+    except KeyError:
+        raise TaskCatException(f"cannot find the S3 hostname for region {region}")
 
 
 def s3_bucket_name_from_url(url):
diff --git a/taskcat/_config.py b/taskcat/_config.py
index 75499bc..af03898 100644
--- a/taskcat/_config.py
+++ b/taskcat/_config.py
@@ -6,7 +6,7 @@ from typing import Dict, Optional, Union
 
 import yaml
 
-from taskcat._cfn.template import Template
+from taskcat._cfn.template import Template, tcat_template_cache
 from taskcat._client_factory import Boto3Cache
 from taskcat._dataclasses import (
     BaseConfig,
@@ -171,7 +171,9 @@ class Config:
         if not file_path.is_file():
             raise TaskCatException(f"invalid template path {file_path}")
         try:
-            template = Template(str(file_path)).template
+            template = Template(
+                str(file_path), template_cache=tcat_template_cache
+            ).template
         except Exception as e:
             LOG.warning(f"failed to load template from {file_path}")
             LOG.debug(str(e), exc_info=True)
@@ -369,6 +371,7 @@ class Config:
                 template_path=project_root / test.template,
                 project_root=project_root,
                 s3_key_prefix=f"{self.config.project.name}/",
+                template_cache=tcat_template_cache,
             )
         return templates
 
diff --git a/taskcat/regions_to_partitions.py b/taskcat/regions_to_partitions.py
new file mode 100644
index 0000000..cb724be
--- /dev/null
+++ b/taskcat/regions_to_partitions.py
@@ -0,0 +1,63 @@
+REGIONS = {
+    "ap-east-1": "aws",
+    "ap-northeast-1": "aws",
+    "ap-northeast-2": "aws",
+    "ap-south-1": "aws",
+    "ap-southeast-1": "aws",
+    "ap-southeast-2": "aws",
+    "ca-central-1": "aws",
+    "eu-central-1": "aws",
+    "eu-north-1": "aws",
+    "eu-west-1": "aws",
+    "eu-west-2": "aws",
+    "eu-west-3": "aws",
+    "me-south-1": "aws",
+    "sa-east-1": "aws",
+    "us-east-1": "aws",
+    "us-east-2": "aws",
+    "us-west-1": "aws",
+    "us-west-2": "aws",
+    "cn-north-1": "aws-cn",
+    "cn-northwest-1": "aws-cn",
+    "us-gov-east-1": "aws-us-gov",
+    "us-gov-west-1": "aws-us-gov",
+    "us-iso-east-1": "aws-iso",
+    "us-isob-east-1": "aws-iso-b"
+}
+
+PARTITIONS = {
+    "aws": [
+        "ap-east-1",
+        "ap-northeast-1",
+        "ap-northeast-2",
+        "ap-south-1",
+        "ap-southeast-1",
+        "ap-southeast-2",
+        "ca-central-1",
+        "eu-central-1",
+        "eu-north-1",
+        "eu-west-1",
+        "eu-west-2",
+        "eu-west-3",
+        "me-south-1",
+        "sa-east-1",
+        "us-east-1",
+        "us-east-2",
+        "us-west-1",
+        "us-west-2"
+    ],
+    "aws-cn": [
+        "cn-north-1",
+        "cn-northwest-1"
+    ],
+    "aws-us-gov": [
+        "us-gov-east-1",
+        "us-gov-west-1"
+    ],
+    "aws-iso": [
+        "us-iso-east-1"
+    ],
+    "aws-iso-b": [
+        "us-isob-east-1"
+    ]
+}
diff --git a/update_partition_region_map.py b/update_partition_region_map.py
new file mode 100755
index 0000000..7b8521d
--- /dev/null
+++ b/update_partition_region_map.py
@@ -0,0 +1,26 @@
+#!/usr/bin/env python
+# because something like https://github.com/boto/boto3/issues/1868 doesn't exist
+
+import json
+
+import requests
+
+ENDPOINT_JSON = (
+    "https://raw.githubusercontent.com/boto/botocore/master/botocore/"
+    "data/endpoints.json"
+)
+
+if __name__ == "__main__":
+    resp = requests.get(ENDPOINT_JSON)
+    if resp.status_code != 200:
+        raise Exception(f"{resp.status_code} {resp.reason}")
+    endpoints = resp.json()
+    partitions = {}
+    regions = {}
+    for p in endpoints["partitions"]:
+        partitions[p["partition"]] = list(p["regions"])
+        for r in p["regions"]:
+            regions[r] = p["partition"]
+    with open("./taskcat/regions_to_partitions.py", "w") as fh:
+        fh.write(f"REGIONS = {json.dumps(regions, indent=4)}\n\n")
+        fh.write(f"PARTITIONS = {json.dumps(partitions, indent=4)}\n")


Test patch:
diff --git a/tests/test_common_utils.py b/tests/test_common_utils.py
index 3de4a2e..b1335d2 100644
--- a/tests/test_common_utils.py
+++ b/tests/test_common_utils.py
@@ -52,10 +52,10 @@ class TestCommonUtils(unittest.TestCase):
         m_get_s3_domain.assert_called_once()
 
     def test_get_s3_domain(self):
-        m_ssm = mock.Mock()
-        m_ssm.get_parameter.return_value = {"Parameter": {"Value": "aws-cn"}}
-        actual = get_s3_domain("test-region", m_ssm)
+        actual = get_s3_domain("cn-north-1")
         self.assertEqual("amazonaws.com.cn", actual)
+        with self.assertRaises(TaskCatException):
+            get_s3_domain("totally-invalid-region")
 
     def test_merge_dicts(self):
         input = [{}, {}]