Skip to content

Fix: CFn: mappings with references #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class TemplateError(RuntimeError):
"""
Error thrown on a programming error from the user
"""
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from typing import Any

from localstack.services.cloudformation.deployment_utils import PLACEHOLDER_AWS_NO_VALUE
from localstack.services.cloudformation.engine.errors import TemplateError
from localstack.utils.urls import localstack_host

AWS_URL_SUFFIX = localstack_host().host # value is "amazonaws.com" in real AWS
Expand Down Expand Up @@ -181,7 +182,52 @@ def resolve_condition(
)
case "Fn::FindInMap":
map_name, top_level_key, second_level_key = v
return mappings[map_name][top_level_key][second_level_key]
if isinstance(map_name, dict) and "Ref" in map_name:
ref_name = map_name["Ref"]
param = parameters.get(ref_name)
if not param:
raise TemplateError(
f"Invalid reference: '{ref_name}' does not exist in parameters: '{parameters}'"
)
map_name = param.get("ResolvedValue") or param.get("ParameterValue")
Comment on lines +185 to +192
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider extracting this Ref resolution logic into a separate function to reduce code duplication


if isinstance(top_level_key, dict) and "Ref" in top_level_key:
ref_name = top_level_key["Ref"]
param = parameters.get(ref_name)
if not param:
raise TemplateError(
f"Invalid reference: '{ref_name}' does not exist in parameters: '{parameters}'"
)
top_level_key = param.get("ResolvedValue") or param.get("ParameterValue")

if isinstance(second_level_key, dict) and "Ref" in second_level_key:
ref_name = second_level_key["Ref"]
param = parameters.get(ref_name)
if not param:
raise TemplateError(
f"Invalid reference: '{ref_name}' does not exist in parameters: '{parameters}'"
)
second_level_key = param.get("ResolvedValue") or param.get("ParameterValue")

mapping = mappings.get(map_name)
if not mapping:
raise TemplateError(
f"Invalid reference: '{map_name}' could not be found in the template mappings: '{list(mappings.keys())}'"
)

top_level_map = mapping.get(top_level_key)
if not top_level_map:
raise TemplateError(
f"Invalid reference: '{top_level_key}' could not be found in the '{map_name}' mapping: '{list(mapping.keys())}'"
)

value = top_level_map.get(second_level_key)
if not value:
raise TemplateError(
f"Invalid reference: '{second_level_key}' could not be found in the '{top_level_key}' mapping: '{top_level_map}'"
)
Comment on lines +224 to +228
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider using .get() with a default value instead of checking for None


return value
case "Fn::If":
if_condition_name, true_branch, false_branch = v
if resolve_condition(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ def deploy_loop(
context = {**payload["callbackContext"], **event.custom_context}
payload["callbackContext"] = context
payload["requestData"]["resourceProperties"] = event.resource_model
resource["Properties"] = event.resource_model
Comment on lines 477 to +480
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider using a more descriptive variable name instead of 'context'


if current_iteration == 0:
time.sleep(0)
Expand Down
63 changes: 61 additions & 2 deletions tests/aws/services/cloudformation/api/test_nested_stacks.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import os

import pytest
from botocore.exceptions import ClientError
from botocore.exceptions import ClientError, WaiterError

from localstack import config
from localstack.testing.aws.util import is_aws_cloud
from localstack.testing.pytest import markers
from localstack.utils.files import load_file
Expand Down Expand Up @@ -81,7 +82,6 @@ def test_nested_stack_output_refs(deploy_cfn_template, s3_create_bucket, aws_cli
assert f"{nested_bucket_name}-suffix" == result.outputs["CustomOutput"]


@pytest.mark.skip(reason="Nested stacks don't work properly")
@markers.aws.validated
def test_nested_with_nested_stack(deploy_cfn_template, s3_create_bucket, aws_client):
bucket_name = s3_create_bucket()
Expand Down Expand Up @@ -291,3 +291,62 @@ def test_nested_stacks_conditions(deploy_cfn_template, s3_create_bucket, aws_cli
StackName=stack.outputs["NestedStackArn"]
)
assert ":" not in nested_stack["Stacks"][0]["StackName"]


@markers.aws.validated
def test_deletion_of_failed_nested_stack(s3_create_bucket, aws_client, region_name, snapshot):
"""
This test confirms that after deleting a stack parent with a failed nested stack. The nested stack is also deleted
"""

bucket_name = s3_create_bucket()
aws_client.s3.upload_file(
os.path.join(
os.path.dirname(__file__), "../../../templates/cfn_failed_nested_stack_child.yml"
),
Bucket=bucket_name,
Key="child.yml",
)

stack_name = f"stack-{short_uid()}"
child_template_url = (
f"https://{bucket_name}.s3.{config.LOCALSTACK_HOST.host_and_port()}/child.yml"
)
if is_aws_cloud():
child_template_url = f"https://{bucket_name}.s3.{region_name}.amazonaws.com/child.yml"

aws_client.cloudformation.create_stack(
StackName=stack_name,
TemplateBody=load_file(
os.path.join(
os.path.dirname(__file__), "../../../templates/cfn_failed_nested_stack_parent.yml"
),
),
Parameters=[
{"ParameterKey": "TemplateUri", "ParameterValue": child_template_url},
],
OnFailure="DO_NOTHING",
Capabilities=["CAPABILITY_NAMED_IAM"],
)

with pytest.raises(WaiterError):
aws_client.cloudformation.get_waiter("stack_create_complete").wait(StackName=stack_name)

stack_status = aws_client.cloudformation.describe_stacks(StackName=stack_name)["Stacks"][0][
"StackStatus"
]
assert stack_status == "CREATE_FAILED"

stacks = aws_client.cloudformation.describe_stacks()["Stacks"]
nested_stack_name = [
stack for stack in stacks if f"{stack_name}-ChildStack-" in stack["StackName"]
][0]["StackName"]

aws_client.cloudformation.delete_stack(StackName=stack_name)
aws_client.cloudformation.get_waiter("stack_delete_complete").wait(StackName=stack_name)

with pytest.raises(ClientError) as ex:
aws_client.cloudformation.describe_stacks(StackName=nested_stack_name)

snapshot.match("error", ex.value.response)
snapshot.add_transformer(snapshot.transform.regex(nested_stack_name, "<nested-stack-name>"))
Original file line number Diff line number Diff line change
Expand Up @@ -63,5 +63,21 @@
}
}
}
},
"tests/aws/services/cloudformation/api/test_nested_stacks.py::test_deletion_of_failed_nested_stack": {
"recorded-date": "17-09-2024, 20:09:36",
"recorded-content": {
"error": {
"Error": {
"Code": "ValidationError",
"Message": "Stack with id <nested-stack-name> does not exist",
"Type": "Sender"
},
"ResponseMetadata": {
"HTTPHeaders": {},
"HTTPStatusCode": 400
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
{
"tests/aws/services/cloudformation/api/test_nested_stacks.py::test_deletion_of_failed_nested_stack": {
"last_validated_date": "2024-09-17T20:09:36+00:00"
},
"tests/aws/services/cloudformation/api/test_nested_stacks.py::test_nested_output_in_params": {
"last_validated_date": "2023-02-07T09:57:47+00:00"
}
Expand Down
20 changes: 20 additions & 0 deletions tests/aws/services/cloudformation/engine/test_mappings.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,3 +153,23 @@ def test_mapping_minimum_nesting_depth(self, aws_client, cleanups, snapshot):
],
)
snapshot.match("mapping_minimum_level_exc", e.value.response)

@markers.aws.validated
def test_mapping_ref_map_key(self, deploy_cfn_template, aws_client, snapshot):
topic_name = f"topic-{short_uid()}"
stack = deploy_cfn_template(
template_path=os.path.join(
THIS_DIR, "../../../templates/mappings/mapping-ref-map-key.yaml"
),
parameters={
"MapName": "MyMap",
"MapKey": "A",
"TopicName": topic_name,
},
)

topic_arn = stack.outputs.get("TopicArn")
assert topic_arn is not None

describe_res = aws_client.sns.get_topic_attributes(TopicArn=topic_arn)
snapshot.match("topic-attributes", describe_res)
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,70 @@
}
}
}
},
"tests/aws/services/cloudformation/engine/test_mappings.py::TestCloudFormationMappings::test_mapping_ref_map_key": {
"recorded-date": "18-09-2024, 21:26:20",
"recorded-content": {
"topic-attributes": {
"Attributes": {
"DisplayName": "",
"EffectiveDeliveryPolicy": {
"http": {
"defaultHealthyRetryPolicy": {
"minDelayTarget": 20,
"maxDelayTarget": 20,
"numRetries": 3,
"numMaxDelayRetries": 0,
"numNoDelayRetries": 0,
"numMinDelayRetries": 0,
"backoffFunction": "linear"
},
"disableSubscriptionOverrides": false,
"defaultRequestPolicy": {
"headerContentType": "text/plain; charset=UTF-8"
}
}
},
"Owner": "111111111111",
"Policy": {
"Version": "2008-10-17",
"Id": "__default_policy_ID",
"Statement": [
{
"Sid": "__default_statement_ID",
"Effect": "Allow",
"Principal": {
"AWS": "*"
},
"Action": [
"SNS:GetTopicAttributes",
"SNS:SetTopicAttributes",
"SNS:AddPermission",
"SNS:RemovePermission",
"SNS:DeleteTopic",
"SNS:Subscribe",
"SNS:ListSubscriptionsByTopic",
"SNS:Publish"
],
"Resource": "arn:<partition>:sns:<region>:111111111111:topic-f1301a86",
"Condition": {
"StringEquals": {
"AWS:SourceOwner": "111111111111"
}
}
}
]
},
"SubscriptionsConfirmed": "0",
"SubscriptionsDeleted": "0",
"SubscriptionsPending": "0",
"TopicArn": "arn:<partition>:sns:<region>:111111111111:topic-f1301a86"
},
"ResponseMetadata": {
"HTTPHeaders": {},
"HTTPStatusCode": 200
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
"tests/aws/services/cloudformation/engine/test_mappings.py::TestCloudFormationMappings::test_mapping_minimum_nesting_depth": {
"last_validated_date": "2023-06-12T14:47:25+00:00"
},
"tests/aws/services/cloudformation/engine/test_mappings.py::TestCloudFormationMappings::test_mapping_ref_map_key": {
"last_validated_date": "2024-09-18T21:26:20+00:00"
},
"tests/aws/services/cloudformation/engine/test_mappings.py::TestCloudFormationMappings::test_mapping_with_invalid_refs": {
"last_validated_date": "2023-06-12T14:47:24+00:00"
},
Expand Down
34 changes: 34 additions & 0 deletions tests/aws/templates/cfn_failed_nested_stack_child.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
Resources:
MyLambdaFunction:
Type: AWS::Lambda::Function
Properties:
FunctionName: testFunction
Handler: index.handler
Runtime: python3.8
Role: !GetAtt LambdaExecutionRole.Arn
Code:
S3Bucket: non-existent-bucket-name # Invalid S3 bucket that does not exist
S3Key: non-existent-key.zip

LambdaExecutionRole:
Type: AWS::IAM::Role
Properties:
RoleName: LambdaExecutionRole
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Using a hardcoded RoleName may cause issues with multiple deployments or updates. Consider using a dynamic naming strategy

AssumeRolePolicyDocument:
Version: '2012-10-17'
Statement:
- Effect: Allow
Principal:
Service: lambda.amazonaws.com
Action: sts:AssumeRole
Policies:
- PolicyName: LambdaBasicExecution
PolicyDocument:
Version: '2012-10-17'
Statement:
- Effect: Allow
Action:
- logs:CreateLogGroup
- logs:CreateLogStream
- logs:PutLogEvents
Resource: arn:aws:logs:*:*:*
14 changes: 14 additions & 0 deletions tests/aws/templates/cfn_failed_nested_stack_parent.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
AWSTemplateFormatVersion: '2010-09-09'
Parameters:
TemplateUri:
Type: String

Resources:
ChildStack:
Type: AWS::CloudFormation::Stack
Properties:
TemplateURL: !Ref TemplateUri

Outputs:
BucketStackId:
Value: !Ref ChildStack
Comment on lines +13 to +14
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider adding a description for this output to improve template readability.

36 changes: 36 additions & 0 deletions tests/aws/templates/mappings/mapping-ref-map-key.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
Mappings:
MyMap:
A:
value: "true"
C:
value: "false"

Conditions:
MyCondition: !Equals
- !FindInMap [ !Ref MapName, !Ref MapKey, value ]
- "true"

Parameters:
MapName:
Type: String

MapKey:
Type: String

TopicName:
Type: String

Resources:
MyTopic:
Type: AWS::SNS::Topic
Properties:
TopicName: !Ref TopicName
Condition: MyCondition

Dummy:
Type: AWS::SNS::Topic

Outputs:
TopicArn:
Value: !Ref MyTopic