Fix route53 record set groups (#2408)

* Fix route53 record set groups

* Fix record set group for http api

* improve readability

* add more tests

* fix tests

* Add test case for intrinsic hosted zone

Author: hawflau

samtranslator/model/api/api_generator.py

@@ -22,7 +22,7 @@
 from samtranslator.swagger.swagger import SwaggerEditor
 from samtranslator.model.intrinsics import is_intrinsic, fnSub
 from samtranslator.model.lambda_ import LambdaPermission
-from samtranslator.translator import logical_id_generator
+from samtranslator.translator.logical_id_generator import LogicalIdGenerator
 from samtranslator.translator.arn_generator import ArnGenerator
 from samtranslator.model.tags.resource_tagging import get_tag_list
 from samtranslator.utils.py27hash_fix import Py27Dict, Py27UniStr
@@ -393,7 +393,7 @@ def _construct_stage(self, deployment, swagger, redeploy_restapi_parameters):
         if stage_name_prefix.isalnum():
             stage_logical_id = self.logical_id + stage_name_prefix + "Stage"
         else:
-            generator = logical_id_generator.LogicalIdGenerator(self.logical_id + "Stage", stage_name_prefix)
+            generator = LogicalIdGenerator(self.logical_id + "Stage", stage_name_prefix)
             stage_logical_id = generator.gen()
         stage = ApiGatewayStage(stage_logical_id, attributes=self.passthrough_resource_attributes)
         stage.RestApiId = ref(self.logical_id)
@@ -417,7 +417,7 @@ def _construct_stage(self, deployment, swagger, redeploy_restapi_parameters):
 
         return stage
 
-    def _construct_api_domain(self, rest_api):
+    def _construct_api_domain(self, rest_api, route53_record_set_groups):
         """
         Constructs and returns the ApiGateway Domain and BasepathMapping
         """
@@ -430,7 +430,7 @@ def _construct_api_domain(self, rest_api):
             )
 
         self.domain["ApiDomainName"] = "{}{}".format(
-            "ApiGatewayDomainName", logical_id_generator.LogicalIdGenerator("", self.domain.get("DomainName")).gen()
+            "ApiGatewayDomainName", LogicalIdGenerator("", self.domain.get("DomainName")).gen()
         )
 
         domain = ApiGatewayDomainName(self.domain.get("ApiDomainName"), attributes=self.passthrough_resource_attributes)
@@ -534,17 +534,23 @@ def _construct_api_domain(self, rest_api):
                     self.logical_id,
                     "HostedZoneId or HostedZoneName is required to enable Route53 support on Custom Domains.",
                 )
-            logical_id = logical_id_generator.LogicalIdGenerator(
+
+            logical_id_suffix = LogicalIdGenerator(
                 "", route53.get("HostedZoneId") or route53.get("HostedZoneName")
             ).gen()
-            record_set_group = Route53RecordSetGroup(
-                "RecordSetGroup" + logical_id, attributes=self.passthrough_resource_attributes
-            )
-            if "HostedZoneId" in route53:
-                record_set_group.HostedZoneId = route53.get("HostedZoneId")
-            if "HostedZoneName" in route53:
-                record_set_group.HostedZoneName = route53.get("HostedZoneName")
-            record_set_group.RecordSets = self._construct_record_sets_for_domain(self.domain)
+            logical_id = "RecordSetGroup" + logical_id_suffix
+
+            record_set_group = route53_record_set_groups.get(logical_id)
+            if not record_set_group:
+                record_set_group = Route53RecordSetGroup(logical_id, attributes=self.passthrough_resource_attributes)
+                if "HostedZoneId" in route53:
+                    record_set_group.HostedZoneId = route53.get("HostedZoneId")
+                if "HostedZoneName" in route53:
+                    record_set_group.HostedZoneName = route53.get("HostedZoneName")
+                record_set_group.RecordSets = []
+                route53_record_set_groups[logical_id] = record_set_group
+
+            record_set_group.RecordSets += self._construct_record_sets_for_domain(self.domain)
 
         return domain, basepath_resource_list, record_set_group
 
@@ -585,14 +591,14 @@ def _construct_alias_target(self, domain):
         return alias_target
 
     @cw_timer(prefix="Generator", name="Api")
-    def to_cloudformation(self, redeploy_restapi_parameters):
+    def to_cloudformation(self, redeploy_restapi_parameters, route53_record_set_groups):
         """Generates CloudFormation resources from a SAM API resource
 
         :returns: a tuple containing the RestApi, Deployment, and Stage for an empty Api.
         :rtype: tuple
         """
         rest_api = self._construct_rest_api()
-        domain, basepath_mapping, route53 = self._construct_api_domain(rest_api)
+        domain, basepath_mapping, route53 = self._construct_api_domain(rest_api, route53_record_set_groups)
         deployment = self._construct_deployment(rest_api)
 
         swagger = None

samtranslator/model/api/http_api_generator.py

@@ -13,7 +13,7 @@
 from samtranslator.model.exceptions import InvalidResourceException
 from samtranslator.model.s3_utils.uri_parser import parse_s3_uri
 from samtranslator.open_api.open_api import OpenApiEditor
-from samtranslator.translator import logical_id_generator
+from samtranslator.translator.logical_id_generator import LogicalIdGenerator
 from samtranslator.model.tags.resource_tagging import get_tag_list
 from samtranslator.model.intrinsics import is_intrinsic, is_intrinsic_no_value
 from samtranslator.model.route53 import Route53RecordSetGroup
@@ -218,7 +218,7 @@ def _add_cors(self):
         # Assign the OpenApi back to template
         self.definition_body = editor.openapi
 
-    def _construct_api_domain(self, http_api):
+    def _construct_api_domain(self, http_api, route53_record_set_groups):
         """
         Constructs and returns the ApiGateway Domain and BasepathMapping
         """
@@ -231,7 +231,7 @@ def _construct_api_domain(self, http_api):
             )
 
         self.domain["ApiDomainName"] = "{}{}".format(
-            "ApiGatewayDomainNameV2", logical_id_generator.LogicalIdGenerator("", self.domain.get("DomainName")).gen()
+            "ApiGatewayDomainNameV2", LogicalIdGenerator("", self.domain.get("DomainName")).gen()
         )
 
         domain = ApiGatewayV2DomainName(
@@ -302,31 +302,40 @@ def _construct_api_domain(self, http_api):
         basepath_resource_list = self._construct_basepath_mappings(basepaths, http_api)
 
         # Create the Route53 RecordSetGroup resource
-        record_set_group = self._construct_route53_recordsetgroup()
+        record_set_group = self._construct_route53_recordsetgroup(route53_record_set_groups)
 
         return domain, basepath_resource_list, record_set_group
 
-    def _construct_route53_recordsetgroup(self):
-        record_set_group = None
-        if self.domain.get("Route53") is not None:
-            route53 = self.domain.get("Route53")
-            if route53.get("HostedZoneId") is None and route53.get("HostedZoneName") is None:
-                raise InvalidResourceException(
-                    self.logical_id,
-                    "HostedZoneId or HostedZoneName is required to enable Route53 support on Custom Domains.",
-                )
-            logical_id = logical_id_generator.LogicalIdGenerator(
-                "", route53.get("HostedZoneId") or route53.get("HostedZoneName")
-            ).gen()
-            record_set_group = Route53RecordSetGroup(
-                "RecordSetGroup" + logical_id, attributes=self.passthrough_resource_attributes
+    def _construct_route53_recordsetgroup(self, route53_record_set_groups):
+        if self.domain.get("Route53") is None:
+            return
+        route53 = self.domain.get("Route53")
+        if not isinstance(route53, dict):
+            raise InvalidResourceException(
+                self.logical_id,
+                "Invalid property type '{}' for Route53. "
+                "Expected a map defines an Amazon Route 53 configuration'.".format(type(route53).__name__),
             )
+        if route53.get("HostedZoneId") is None and route53.get("HostedZoneName") is None:
+            raise InvalidResourceException(
+                self.logical_id,
+                "HostedZoneId or HostedZoneName is required to enable Route53 support on Custom Domains.",
+            )
+
+        logical_id_suffix = LogicalIdGenerator("", route53.get("HostedZoneId") or route53.get("HostedZoneName")).gen()
+        logical_id = "RecordSetGroup" + logical_id_suffix
+
+        record_set_group = route53_record_set_groups.get(logical_id)
+        if not record_set_group:
+            record_set_group = Route53RecordSetGroup(logical_id, attributes=self.passthrough_resource_attributes)
             if "HostedZoneId" in route53:
                 record_set_group.HostedZoneId = route53.get("HostedZoneId")
             elif "HostedZoneName" in route53:
                 record_set_group.HostedZoneName = route53.get("HostedZoneName")
-            record_set_group.RecordSets = self._construct_record_sets_for_domain(self.domain)
+            record_set_group.RecordSets = []
+            route53_record_set_groups[logical_id] = record_set_group
 
+        record_set_group.RecordSets += self._construct_record_sets_for_domain(self.domain)
         return record_set_group
 
     def _construct_basepath_mappings(self, basepaths, http_api):
@@ -592,7 +601,7 @@ def _construct_stage(self):
         elif stage_name_prefix == DefaultStageName:
             stage_logical_id = self.logical_id + "ApiGatewayDefaultStage"
         else:
-            generator = logical_id_generator.LogicalIdGenerator(self.logical_id + "Stage", stage_name_prefix)
+            generator = LogicalIdGenerator(self.logical_id + "Stage", stage_name_prefix)
             stage_logical_id = generator.gen()
         stage = ApiGatewayV2Stage(stage_logical_id, attributes=self.passthrough_resource_attributes)
         stage.ApiId = ref(self.logical_id)
@@ -628,14 +637,14 @@ def _add_description(self):
         self.definition_body = open_api_editor.openapi
 
     @cw_timer(prefix="Generator", name="HttpApi")
-    def to_cloudformation(self):
+    def to_cloudformation(self, route53_record_set_groups):
         """Generates CloudFormation resources from a SAM HTTP API resource
 
         :returns: a tuple containing the HttpApi and Stage for an empty Api.
         :rtype: tuple
         """
         http_api = self._construct_http_api()
-        domain, basepath_mapping, route53 = self._construct_api_domain(http_api)
+        domain, basepath_mapping, route53 = self._construct_api_domain(http_api, route53_record_set_groups)
         stage = self._construct_stage()
 
         return http_api, stage, domain, basepath_mapping, route53

samtranslator/model/sam_resources.py

@@ -1107,6 +1107,7 @@ def to_cloudformation(self, **kwargs):
         redeploy_restapi_parameters = kwargs.get("redeploy_restapi_parameters")
         shared_api_usage_plan = kwargs.get("shared_api_usage_plan")
         template_conditions = kwargs.get("conditions")
+        route53_record_set_groups = kwargs.get("route53_record_set_groups", {})
 
         api_generator = ApiGenerator(
             self.logical_id,
@@ -1152,7 +1153,7 @@ def to_cloudformation(self, **kwargs):
             basepath_mapping,
             route53,
             usage_plan_resources,
-        ) = api_generator.to_cloudformation(redeploy_restapi_parameters)
+        ) = api_generator.to_cloudformation(redeploy_restapi_parameters, route53_record_set_groups)
 
         resources.extend([rest_api, deployment, stage])
         resources.extend(permissions)
@@ -1212,8 +1213,6 @@ def to_cloudformation(self, **kwargs):
         resources = []
         intrinsics_resolver = kwargs["intrinsics_resolver"]
         self.CorsConfiguration = intrinsics_resolver.resolve_parameter_refs(self.CorsConfiguration)
-
-        intrinsics_resolver = kwargs["intrinsics_resolver"]
         self.Domain = intrinsics_resolver.resolve_parameter_refs(self.Domain)
 
         api_generator = HttpApiGenerator(
@@ -1243,7 +1242,7 @@ def to_cloudformation(self, **kwargs):
             domain,
             basepath_mapping,
             route53,
-        ) = api_generator.to_cloudformation()
+        ) = api_generator.to_cloudformation(kwargs.get("route53_record_set_groups", {}))
 
         resources.append(http_api)
         if domain:

samtranslator/translator/translator.py

@@ -125,6 +125,7 @@ def translate(self, sam_template, parameter_values, feature_toggle=None, passthr
         shared_api_usage_plan = SharedApiUsagePlan()
         document_errors = []
         changed_logical_ids = {}
+        route53_record_set_groups = {}
         for logical_id, resource_dict in self._get_resources_to_iterate(sam_template, macro_resolver):
             try:
                 macro = macro_resolver.resolve_resource_type(resource_dict).from_dict(
@@ -144,6 +145,7 @@ def translate(self, sam_template, parameter_values, feature_toggle=None, passthr
                 kwargs["redeploy_restapi_parameters"] = self.redeploy_restapi_parameters
                 kwargs["shared_api_usage_plan"] = shared_api_usage_plan
                 kwargs["feature_toggle"] = self.feature_toggle
+                kwargs["route53_record_set_groups"] = route53_record_set_groups
                 translated = macro.to_cloudformation(**kwargs)
 
                 supported_resource_refs = macro.get_resource_references(translated, supported_resource_refs)

tests/model/api/test_http_api_generator.py

@@ -216,11 +216,14 @@ class TestCustomDomains(TestCase):
         "passthrough_resource_attributes": None,
         "domain": None,
     }
+    route53_record_set_groups = {}
 
     def test_no_domain(self):
         self.kwargs["domain"] = None
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
-        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(
+            http_api, self.route53_record_set_groups
+        )
         self.assertIsNone(domain)
         self.assertIsNone(basepath)
         self.assertIsNone(route)
@@ -229,7 +232,7 @@ def test_no_domain_name(self):
         self.kwargs["domain"] = {"CertificateArn": "someurl"}
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
         with pytest.raises(InvalidResourceException) as e:
-            HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+            HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api, self.route53_record_set_groups)
         self.assertEqual(
             e.value.message,
             "Resource with id [HttpApiId] is invalid. "
@@ -240,7 +243,7 @@ def test_no_cert_arn(self):
         self.kwargs["domain"] = {"DomainName": "example.com"}
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
         with pytest.raises(InvalidResourceException) as e:
-            HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+            HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api, self.route53_record_set_groups)
         self.assertEqual(
             e.value.message,
             "Resource with id [HttpApiId] is invalid. "
@@ -250,7 +253,9 @@ def test_no_cert_arn(self):
     def test_basic_domain_default_endpoint(self):
         self.kwargs["domain"] = {"DomainName": "example.com", "CertificateArn": "some-url"}
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
-        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(
+            http_api, self.route53_record_set_groups
+        )
         self.assertIsNotNone(domain, None)
         self.assertIsNotNone(basepath, None)
         self.assertEqual(len(basepath), 1)
@@ -264,7 +269,9 @@ def test_basic_domain_regional_endpoint(self):
             "EndpointConfiguration": "REGIONAL",
         }
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
-        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(
+            http_api, self.route53_record_set_groups
+        )
         self.assertIsNotNone(domain, None)
         self.assertIsNotNone(basepath, None)
         self.assertEqual(len(basepath), 1)
@@ -279,7 +286,7 @@ def test_basic_domain_edge_endpoint(self):
         }
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
         with pytest.raises(InvalidResourceException) as e:
-            HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+            HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api, self.route53_record_set_groups)
         self.assertEqual(
             e.value.message,
             "Resource with id [HttpApiId] is invalid. EndpointConfiguration for Custom Domains must be one of ['REGIONAL'].",
@@ -293,7 +300,7 @@ def test_bad_endpoint(self):
         }
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
         with pytest.raises(InvalidResourceException) as e:
-            HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+            HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api, self.route53_record_set_groups)
         self.assertEqual(
             e.value.message,
             "Resource with id [HttpApiId] is invalid. "
@@ -307,7 +314,9 @@ def test_basic_route53(self):
             "Route53": {"HostedZoneId": "xyz"},
         }
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
-        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(
+            http_api, self.route53_record_set_groups
+        )
         self.assertIsNotNone(domain, None)
         self.assertIsNotNone(basepath, None)
         self.assertEqual(len(basepath), 1)
@@ -322,7 +331,9 @@ def test_basepaths(self):
             "Route53": {"HostedZoneId": "xyz"},
         }
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
-        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(
+            http_api, self.route53_record_set_groups
+        )
         self.assertIsNotNone(domain, None)
         self.assertIsNotNone(basepath, None)
         self.assertEqual(len(basepath), 3)
@@ -338,7 +349,7 @@ def test_invalid_basepaths(self):
         }
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
         with pytest.raises(InvalidResourceException) as e:
-            HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+            HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api, self.route53_record_set_groups)
         self.assertEqual(
             e.value.message, "Resource with id [HttpApiId] is invalid. " + "Invalid Basepath name provided."
         )
@@ -351,7 +362,9 @@ def test_basepaths(self):
             "Route53": {"HostedZoneId": "xyz", "HostedZoneName": "abc", "IpV6": True},
         }
         http_api = HttpApiGenerator(**self.kwargs)._construct_http_api()
-        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(http_api)
+        domain, basepath, route = HttpApiGenerator(**self.kwargs)._construct_api_domain(
+            http_api, self.route53_record_set_groups
+        )
         self.assertIsNotNone(domain, None)
         self.assertIsNotNone(basepath, None)
         self.assertEqual(len(basepath), 8)