From 1237a9bcd5383c5d95ea5ce048bcab86817c1fd6 Mon Sep 17 00:00:00 2001 From: Simon Walker Date: Sat, 21 Sep 2024 22:09:26 +0100 Subject: [PATCH 1/5] Improve error messages for parameters --- .../localstack/services/cloudformation/engine/parameters.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/localstack-core/localstack/services/cloudformation/engine/parameters.py b/localstack-core/localstack/services/cloudformation/engine/parameters.py index e2df3e0606bb9..3c8f9151ba874 100644 --- a/localstack-core/localstack/services/cloudformation/engine/parameters.py +++ b/localstack-core/localstack/services/cloudformation/engine/parameters.py @@ -90,7 +90,7 @@ def resolve_parameters( default_value = pm["DefaultValue"] if default_value is None: raise Exception( - "Invalid. Needs to have either param specified or Default. (TODO)" + f"Invalid. Parameter '{pm_key}' needs to have either param specified or Default." ) # TODO: test and verify resolved_param["ParameterValue"] = default_value @@ -100,13 +100,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"] From 1c567b0fa57bc9caae63b0b67e95e3938bfdb57a Mon Sep 17 00:00:00 2001 From: Simon Walker Date: Thu, 26 Sep 2024 22:36:07 +0100 Subject: [PATCH 2/5] Handle missing first level attributes in mappings --- .../engine/template_deployer.py | 5 ++++ .../cloudformation/engine/test_mappings.py | 24 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/localstack-core/localstack/services/cloudformation/engine/template_deployer.py b/localstack-core/localstack/services/cloudformation/engine/template_deployer.py index a3480e096b3f1..1c086c0b0c21e 100644 --- a/localstack-core/localstack/services/cloudformation/engine/template_deployer.py +++ b/localstack-core/localstack/services/cloudformation/engine/template_deployer.py @@ -490,6 +490,11 @@ 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}'" + ) + second_level_attribute = value[keys_list[0]][2] if not isinstance(second_level_attribute, str): second_level_attribute = resolve_refs_recursively( diff --git a/tests/aws/services/cloudformation/engine/test_mappings.py b/tests/aws/services/cloudformation/engine/test_mappings.py index 0a18c0580262a..1ad195ec23d89 100644 --- 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,29 @@ 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.validated @pytest.mark.skip(reason="not implemented") def test_mapping_with_invalid_refs(self, aws_client, deploy_cfn_template, cleanups, snapshot): From 0fa5780c048898754c3b4dc6914f9a291cef71ce Mon Sep 17 00:00:00 2001 From: Simon Walker Date: Thu, 26 Sep 2024 22:43:05 +0100 Subject: [PATCH 3/5] Handle errors with second level mapping --- .../engine/template_deployer.py | 7 +++++- .../cloudformation/engine/test_mappings.py | 23 +++++++++++++++++++ .../templates/mappings/simple-mapping.yaml | 6 ++++- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/localstack-core/localstack/services/cloudformation/engine/template_deployer.py b/localstack-core/localstack/services/cloudformation/engine/template_deployer.py index 1c086c0b0c21e..466d2f35084bc 100644 --- a/localstack-core/localstack/services/cloudformation/engine/template_deployer.py +++ b/localstack-core/localstack/services/cloudformation/engine/template_deployer.py @@ -494,6 +494,7 @@ def _resolve_refs_recursively( 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): @@ -507,8 +508,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( diff --git a/tests/aws/services/cloudformation/engine/test_mappings.py b/tests/aws/services/cloudformation/engine/test_mappings.py index 1ad195ec23d89..21b08801e8dab 100644 --- a/tests/aws/services/cloudformation/engine/test_mappings.py +++ b/tests/aws/services/cloudformation/engine/test_mappings.py @@ -91,6 +91,29 @@ def test_async_mapping_error_first_level(self, deploy_cfn_template): 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") diff --git a/tests/aws/templates/mappings/simple-mapping.yaml b/tests/aws/templates/mappings/simple-mapping.yaml index e634ee410eed5..5d7694e7a5a8b 100644 --- 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: From fcbfc767b450170f20747b3223524fc2d2c900e1 Mon Sep 17 00:00:00 2001 From: Simon Walker Date: Fri, 27 Sep 2024 14:57:58 +0100 Subject: [PATCH 4/5] Add more error messages and logging --- .../engine/template_deployer.py | 32 +++++++++++++++---- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/localstack-core/localstack/services/cloudformation/engine/template_deployer.py b/localstack-core/localstack/services/cloudformation/engine/template_deployer.py index 466d2f35084bc..6657b628a93c0 100644 --- a/localstack-core/localstack/services/cloudformation/engine/template_deployer.py +++ b/localstack-core/localstack/services/cloudformation/engine/template_deployer.py @@ -243,8 +243,8 @@ 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 @@ -252,8 +252,8 @@ def resolve_refs_recursively( 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}" ) @@ -540,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 not condition: + 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, @@ -556,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] From 5109aa9db41b9a662e7acb4bf10a39c32a135dba Mon Sep 17 00:00:00 2001 From: Simon Walker Date: Tue, 1 Oct 2024 15:00:24 +0100 Subject: [PATCH 5/5] Check for `None` rather than falsey values --- .../services/cloudformation/engine/template_deployer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/localstack-core/localstack/services/cloudformation/engine/template_deployer.py b/localstack-core/localstack/services/cloudformation/engine/template_deployer.py index 6657b628a93c0..0d574f2271a62 100644 --- a/localstack-core/localstack/services/cloudformation/engine/template_deployer.py +++ b/localstack-core/localstack/services/cloudformation/engine/template_deployer.py @@ -544,7 +544,7 @@ def _resolve_refs_recursively( if stripped_fn_lower == "if": condition, option1, option2 = value[keys_list[0]] condition = conditions.get(condition) - if not condition: + if condition is None: LOG.warning( "Cannot find condition '%s' in conditions mapping: '%s'", condition,