localstack · simonrw · Jan 5, 2025 · Sep 21, 2024 · Sep 26, 2024 · Sep 26, 2024
diff --git a/localstack-core/localstack/services/cloudformation/engine/parameters.py b/localstack-core/localstack/services/cloudformation/engine/parameters.py
@@ -91,7 +91,7 @@ def resolve_parameters(
             if default_value is None:
                 LOG.error("New parameter without a default value: %s", pm_key)
                 raise Exception(
-                    f"Invalid. Needs to have either param specified or Default for key={pm_key}. (TODO)"
+                    f"Invalid. Parameter '{pm_key}' needs to have either param specified or Default."
                 )  # TODO: test and verify
 
             resolved_param["ParameterValue"] = default_value
@@ -101,13 +101,13 @@ def resolve_parameters(
                 and new_parameter.get("ParameterValue") is not None
             ):
                 raise Exception(
-                    "Can't set both 'UsePreviousValue' and a concrete value. (TODO)"
+                    f"Can't set both 'UsePreviousValue' and a concrete value for parameter '{pm_key}'."
                 )  # TODO: test and verify
 
             if new_parameter.get("UsePreviousValue", False):
                 if old_parameter is None:
                     raise Exception(
-                        "Set 'UsePreviousValue' but stack has no previous value for this parameter. (TODO)"
+                        f"Set 'UsePreviousValue' but stack has no previous value for parameter '{pm_key}'."
                     )  # TODO: test and verify
 
                 resolved_param["ParameterValue"] = old_parameter["ParameterValue"]

diff --git a/localstack-core/localstack/services/cloudformation/engine/template_deployer.py b/localstack-core/localstack/services/cloudformation/engine/template_deployer.py
@@ -243,17 +243,17 @@ def resolve_refs_recursively(
                 ssm_client = connect_to(aws_access_key_id=account_id, region_name=region_name).ssm
                 try:
                     return ssm_client.get_parameter(Name=reference_key)["Parameter"]["Value"]
-                except ClientError:
-                    LOG.error("client error accessing SSM parameter '%s'", reference_key)
+                except ClientError as e:
+                    LOG.error("client error accessing SSM parameter '%s': %s", reference_key, e)
                     raise
             elif service_name == "ssm-secure":
                 ssm_client = connect_to(aws_access_key_id=account_id, region_name=region_name).ssm
                 try:
                     return ssm_client.get_parameter(Name=reference_key, WithDecryption=True)[
                         "Parameter"
                     ]["Value"]
-                except ClientError:
-                    LOG.error("client error accessing SSM parameter '%s'", reference_key)
+                except ClientError as e:
+                    LOG.error("client error accessing SSM parameter '%s': %s", reference_key, e)
                     raise
             elif service_name == "secretsmanager":
                 # reference key needs to be parsed further
@@ -285,7 +285,7 @@ def resolve_refs_recursively(
                         SecretId=secret_id, **kwargs
                     )["SecretString"]
                 except ClientError:
-                    LOG.error("client error while trying to access key '%s'", secret_id)
+                    LOG.error("client error while trying to access key '%s': %s", secret_id)
                     raise
 
                 if json_key:
@@ -414,6 +414,9 @@ def _resolve_refs_recursively(
 
             none_values = [v for v in join_values if v is None]
             if none_values:
+                LOG.warning(
+                    "Cannot resolve Fn::Join '%s' due to null values: '%s'", value, join_values
+                )
                 raise Exception(
                     f"Cannot resolve CF Fn::Join {value} due to null values: {join_values}"
                 )
@@ -490,6 +493,12 @@ def _resolve_refs_recursively(
                 first_level_attribute,
             )
 
+            if first_level_attribute not in selected_map:
+                raise Exception(
+                    f"Cannot find map key '{first_level_attribute}' in mapping '{mapping_id}'"
+                )
+            first_level_mapping = selected_map[first_level_attribute]
+
             second_level_attribute = value[keys_list[0]][2]
             if not isinstance(second_level_attribute, str):
                 second_level_attribute = resolve_refs_recursively(
@@ -502,8 +511,12 @@ def _resolve_refs_recursively(
                     parameters,
                     second_level_attribute,
                 )
+            if second_level_attribute not in first_level_mapping:
+                raise Exception(
+                    f"Cannot find map key '{second_level_attribute}' in mapping '{mapping_id}' under key '{first_level_attribute}'"
+                )
 
-            return selected_map.get(first_level_attribute).get(second_level_attribute)
+            return first_level_mapping[second_level_attribute]
 
         if stripped_fn_lower == "importvalue":
             import_value_key = resolve_refs_recursively(
@@ -530,7 +543,17 @@ def _resolve_refs_recursively(
 
         if stripped_fn_lower == "if":
             condition, option1, option2 = value[keys_list[0]]
-            condition = conditions[condition]
+            condition = conditions.get(condition)
+            if condition is None:
+                LOG.warning(
+                    "Cannot find condition '%s' in conditions mapping: '%s'",
+                    condition,
+                    conditions.keys(),
+                )
+                raise KeyError(
+                    f"Cannot find condition '{condition}' in conditions mapping: '{conditions.keys()}'"
+                )
+
             result = resolve_refs_recursively(
                 account_id,
                 region_name,
@@ -546,7 +569,12 @@ def _resolve_refs_recursively(
         if stripped_fn_lower == "condition":
             # FIXME: this should only allow strings, no evaluation should be performed here
             #   see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-condition.html
-            return conditions[value[keys_list[0]]]
+            key = value[keys_list[0]]
+            result = conditions.get(key)
+            if result is None:
+                LOG.warning("Cannot find key '%s' in conditions: '%s'", key, conditions.keys())
+                raise KeyError(f"Cannot find key '{key}' in conditions: '{conditions.keys()}'")
+            return result
 
         if stripped_fn_lower == "not":
             condition = value[keys_list[0]][0]

diff --git a/tests/aws/services/cloudformation/engine/test_mappings.py b/tests/aws/services/cloudformation/engine/test_mappings.py
@@ -3,6 +3,7 @@
 import pytest
 
 from localstack.testing.pytest import markers
+from localstack.testing.pytest.fixtures import StackDeployError
 from localstack.utils.files import load_file
 from localstack.utils.strings import short_uid
 
@@ -68,6 +69,52 @@ def test_mapping_with_nonexisting_key(self, aws_client, cleanups, snapshot):
             )
         snapshot.match("mapping_nonexisting_key_exc", e.value.response)
 
+    @markers.aws.only_localstack
+    def test_async_mapping_error_first_level(self, deploy_cfn_template):
+        """
+        We don't (yet) support validating mappings synchronously in `create_changeset` like AWS does, however
+        we don't fail with a good error message at all. This test ensures that the deployment fails with a
+        nicer error message than a Python traceback about "`None` has no attribute `get`".
+        """
+        topic_name = f"test-topic-{short_uid()}"
+        with pytest.raises(StackDeployError) as exc_info:
+            deploy_cfn_template(
+                template_path=os.path.join(
+                    THIS_DIR,
+                    "../../../templates/mappings/simple-mapping.yaml",
+                ),
+                parameters={
+                    "TopicName": topic_name,
+                    "TopicNameSuffixSelector": "C",
+                },
+            )
+
+        assert "Cannot find map key 'C' in mapping 'TopicSuffixMap'" in str(exc_info.value)
+
+    @markers.aws.only_localstack
+    def test_async_mapping_error_second_level(self, deploy_cfn_template):
+        """
+        Similar to the `test_async_mapping_error_first_level` test above, but
+        checking the second level of mapping lookup
+        """
+        topic_name = f"test-topic-{short_uid()}"
+        with pytest.raises(StackDeployError) as exc_info:
+            deploy_cfn_template(
+                template_path=os.path.join(
+                    THIS_DIR,
+                    "../../../templates/mappings/simple-mapping.yaml",
+                ),
+                parameters={
+                    "TopicName": topic_name,
+                    "TopicNameSuffixSelector": "A",
+                    "TopicAttributeSelector": "NotValid",
+                },
+            )
+
+        assert "Cannot find map key 'NotValid' in mapping 'TopicSuffixMap' under key 'A'" in str(
+            exc_info.value
+        )
+
     @markers.aws.validated
     @pytest.mark.skip(reason="not implemented")
     def test_mapping_with_invalid_refs(self, aws_client, deploy_cfn_template, cleanups, snapshot):

diff --git a/tests/aws/templates/mappings/simple-mapping.yaml b/tests/aws/templates/mappings/simple-mapping.yaml
@@ -5,6 +5,10 @@ Parameters:
   TopicNameSuffixSelector:
     Type: String
 
+  TopicAttributeSelector:
+    Type: String
+    Default: Suffix
+
 Resources:
   MyTopic:
     Type: AWS::SNS::Topic
@@ -13,7 +17,7 @@ Resources:
         "Fn::Join":
           - "-"
           - - !Ref TopicName
-            - !FindInMap [TopicSuffixMap, !Ref TopicNameSuffixSelector, Suffix]
+            - !FindInMap [TopicSuffixMap, !Ref TopicNameSuffixSelector, !Ref TopicAttributeSelector]
 
 Mappings:
   TopicSuffixMap: