Skip to content
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

fix: multiple mq source event policy name (add DynamicPolicyName) #2953

Merged
merged 10 commits into from
Feb 27, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@ class MSKEvent(BaseModel):
class MQEventProperties(BaseModel):
BatchSize: Optional[PassThroughProp] = mqeventproperties("BatchSize")
Broker: PassThroughProp = mqeventproperties("Broker")
DynamicPolicyName: Optional[bool] # TODO: add docs
ssenchenko marked this conversation as resolved.
Show resolved Hide resolved
Enabled: Optional[PassThroughProp] = mqeventproperties("Enabled")
FilterCriteria: Optional[PassThroughProp] = mqeventproperties("FilterCriteria")
MaximumBatchingWindowInSeconds: Optional[PassThroughProp] = mqeventproperties("MaximumBatchingWindowInSeconds")
Expand Down
41 changes: 39 additions & 2 deletions samtranslator/model/eventsources/pull.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

from samtranslator.internal.deprecation_control import deprecated
from samtranslator.metrics.method_decorator import cw_timer
from samtranslator.model import PassThroughProperty, PropertyType, ResourceMacro
from samtranslator.model import PassThroughProperty, Property, PropertyType, ResourceMacro
from samtranslator.model.eventsources import FUNCTION_EVETSOURCE_METRIC_PREFIX
from samtranslator.model.exceptions import InvalidEventException
from samtranslator.model.iam import IAMRolePolicies
Expand Down Expand Up @@ -424,9 +424,46 @@ class MQ(PullEventSource):
property_types: Dict[str, PropertyType] = {
**PullEventSource.property_types,
"Broker": PassThroughProperty(True),
"DynamicPolicyName": Property(False, is_type(bool)),
}

Broker: PassThrough
DynamicPolicyName: Optional[bool]

@property
def _policy_name(self) -> str:
"""Generate policy name based on DynamicPolicyName flag and MQ logical ID.

Policy name is required though its update is "No interuption".
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-policy.html#cfn-iam-policy-policyname #noqa

Historically, policy name was hardcoded as `SamAutoGeneratedAMQPolicy` but it led to a policy name clash
and failure to deploy, if a Function had at least 2 MQ event sources.
Since policy is attached to the Lambda execution role,
policy name should be based on MQ logical ID not to clash with policy names of other MQ event sources.
However, to support backwards compatibility, we need to keep policy `SamAutoGeneratedAMQPolicy` by default,
because customers might have code which relys on that policy name consistancy.

To support both old policy name and ability to have more than one MQ event source, we introduce new field
`DynamicPolicyName` which when set to true will use MQ logical ID to generate policy name.

Q: Why to introduce a new field and not to make policy name dynamic by default if there are multiple
MQ event sources?
A: Since a customer could have a single MQ source and rely on it's policy name in their code. If that customer
decides to add a new MQ source, they don't want to change the policy name for the first MQ all over their
code base. But they can opt in using a dynamic policy name for all other MQ sources they add.

Q: Why not use dynamic policy names automatically for all MQ event sources but first?
A: SAM-T doesn't have state and doesn't know what was the CFN resource attribute in a previous transformation.
Hence, trying to "use dynamic policy names automatically for all MQ event sources but first" can rely only
on event source order. If a customer added a new MQ source __before__ an old one, an old one would receive
a dynamic name and would break (potentially) customer's code.

Returns
-------
Name of the policy which will be attached to the Lambda Execution role.
"""
return f"{self.logical_id}AMQPolicy" if self.DynamicPolicyName else "SamAutoGeneratedAMQPolicy"

def get_event_source_arn(self) -> Optional[PassThrough]:
return self.Broker
Expand All @@ -438,7 +475,7 @@ def get_policy_statements(self) -> Optional[List[Dict[str, Any]]]:
basic_auth_uri = self._validate_source_access_configurations(["BASIC_AUTH", "VIRTUAL_HOST"], "BASIC_AUTH")

document = {
"PolicyName": "SamAutoGeneratedAMQPolicy",
"PolicyName": self._policy_name,
"PolicyDocument": {
"Statement": [
{
Expand Down
4 changes: 4 additions & 0 deletions samtranslator/schema/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -195466,6 +195466,10 @@
"markdownDescription": "The Amazon Resource Name \\(ARN\\) of the Amazon MQ broker\\. \n*Type*: String \n*Required*: Yes \n*AWS CloudFormation compatibility*: This property is passed directly to the [`EventSourceArn`](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-lambda-eventsourcemapping.html#cfn-lambda-eventsourcemapping-eventsourcearn) property of an `AWS::Lambda::EventSourceMapping` resource\\.",
"title": "Broker"
},
"DynamicPolicyName": {
"title": "Dynamicpolicyname",
"type": "boolean"
},
"Enabled": {
"allOf": [
{
Expand Down
4 changes: 4 additions & 0 deletions schema_source/sam.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1960,6 +1960,10 @@
"markdownDescription": "The Amazon Resource Name \\(ARN\\) of the Amazon MQ broker\\. \n*Type*: String \n*Required*: Yes \n*AWS CloudFormation compatibility*: This property is passed directly to the [`EventSourceArn`](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-lambda-eventsourcemapping.html#cfn-lambda-eventsourcemapping-eventsourcearn) property of an `AWS::Lambda::EventSourceMapping` resource\\.",
"title": "Broker"
},
"DynamicPolicyName": {
"title": "Dynamicpolicyname",
"type": "boolean"
},
"Enabled": {
"allOf": [
{
Expand Down
4 changes: 2 additions & 2 deletions tests/model/eventsources/test_mq_event_source.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def test_get_policy_statements(self):
policy_statements = self.mq_event_source.get_policy_statements()
expected_policy_document = [
{
"PolicyName": "SamAutoGeneratedAMQPolicy",
"PolicyName": self.mq_event_source._policy_name,
"PolicyDocument": {
"Statement": [
{
Expand Down Expand Up @@ -69,7 +69,7 @@ def test_get_policy_statements_with_secrets_manager_kms_key_id(self):
policy_statements = self.mq_event_source.get_policy_statements()
expected_policy_document = [
{
"PolicyName": "SamAutoGeneratedAMQPolicy",
"PolicyName": self.mq_event_source._policy_name,
"PolicyDocument": {
"Statement": [
{
Expand Down
44 changes: 44 additions & 0 deletions tests/translator/input/function_with_multiple_mq.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
Resources:
MyFunction:
Type: AWS::Serverless::Function
Properties:
FunctionName: Test
CodeUri: s3://sam-demo-bucket/queues.zip
Handler: app.handler
Runtime: nodejs18.x
MemorySize: 256
Architectures:
- x86_64
Events:
BrokerOne:
Type: MQ
Properties:
Broker: !Sub 'arn:${AWS::Partition}:mq:us-east-2:123456789012:broker:MyBroker:b-1234a5b6-78cd-901e-2fgh-3i45j6k17801'
BatchSize: 1
Queues:
- MyQueue
SourceAccessConfigurations:
- Type: BASIC_AUTH
URI: !Sub 'arn:${AWS::Partition}:secretsmanager:us-west-2:123456789012:secret:my-path/my-secret-name-1a2b01'
BrokerTwo:
Type: MQ
Properties:
Broker: !Sub 'arn:${AWS::Partition}:mq:us-east-2:123456789012:broker:MyBroker:b-1234a5b6-78cd-901e-2fgh-3i45j6k17802'
BatchSize: 1
DynamicPolicyName: true
Queues:
- MyQueue
SourceAccessConfigurations:
- Type: BASIC_AUTH
URI: !Sub 'arn:${AWS::Partition}:secretsmanager:us-west-2:123456789012:secret:my-path/my-secret-name-1a2b02'
BrokerThree:
Type: MQ
Properties:
Broker: !Sub 'arn:${AWS::Partition}:mq:us-east-2:123456789012:broker:MyBroker:b-1234a5b6-78cd-901e-2fgh-3i45j6k17803'
BatchSize: 1
DynamicPolicyName: true
Queues:
- MyQueue
SourceAccessConfigurations:
- Type: BASIC_AUTH
URI: !Sub 'arn:${AWS::Partition}:secretsmanager:us-west-2:123456789012:secret:my-path/my-secret-name-1a2b03'
208 changes: 208 additions & 0 deletions tests/translator/output/aws-cn/function_with_multiple_mq.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
{
"Resources": {
"MyFunction": {
"Properties": {
"Architectures": [
"x86_64"
],
"Code": {
"S3Bucket": "sam-demo-bucket",
"S3Key": "queues.zip"
},
"FunctionName": "Test",
"Handler": "app.handler",
"MemorySize": 256,
"Role": {
"Fn::GetAtt": [
"MyFunctionRole",
"Arn"
]
},
"Runtime": "nodejs18.x",
"Tags": [
{
"Key": "lambda:createdBy",
"Value": "SAM"
}
]
},
"Type": "AWS::Lambda::Function"
},
"MyFunctionBrokerOne": {
"Properties": {
"BatchSize": 1,
"EventSourceArn": {
"Fn::Sub": "arn:${AWS::Partition}:mq:us-east-2:123456789012:broker:MyBroker:b-1234a5b6-78cd-901e-2fgh-3i45j6k17801"
},
"FunctionName": {
"Ref": "MyFunction"
},
"Queues": [
"MyQueue"
],
"SourceAccessConfigurations": [
{
"Type": "BASIC_AUTH",
"URI": {
"Fn::Sub": "arn:${AWS::Partition}:secretsmanager:us-west-2:123456789012:secret:my-path/my-secret-name-1a2b01"
}
}
]
},
"Type": "AWS::Lambda::EventSourceMapping"
},
"MyFunctionBrokerThree": {
"Properties": {
"BatchSize": 1,
"EventSourceArn": {
"Fn::Sub": "arn:${AWS::Partition}:mq:us-east-2:123456789012:broker:MyBroker:b-1234a5b6-78cd-901e-2fgh-3i45j6k17803"
},
"FunctionName": {
"Ref": "MyFunction"
},
"Queues": [
"MyQueue"
],
"SourceAccessConfigurations": [
{
"Type": "BASIC_AUTH",
"URI": {
"Fn::Sub": "arn:${AWS::Partition}:secretsmanager:us-west-2:123456789012:secret:my-path/my-secret-name-1a2b03"
}
}
]
},
"Type": "AWS::Lambda::EventSourceMapping"
},
"MyFunctionBrokerTwo": {
"Properties": {
"BatchSize": 1,
"EventSourceArn": {
"Fn::Sub": "arn:${AWS::Partition}:mq:us-east-2:123456789012:broker:MyBroker:b-1234a5b6-78cd-901e-2fgh-3i45j6k17802"
},
"FunctionName": {
"Ref": "MyFunction"
},
"Queues": [
"MyQueue"
],
"SourceAccessConfigurations": [
{
"Type": "BASIC_AUTH",
"URI": {
"Fn::Sub": "arn:${AWS::Partition}:secretsmanager:us-west-2:123456789012:secret:my-path/my-secret-name-1a2b02"
}
}
]
},
"Type": "AWS::Lambda::EventSourceMapping"
},
"MyFunctionRole": {
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": [
"sts:AssumeRole"
],
"Effect": "Allow",
"Principal": {
"Service": [
"lambda.amazonaws.com"
]
}
}
],
"Version": "2012-10-17"
},
"ManagedPolicyArns": [
"arn:aws-cn:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
],
"Policies": [
{
"PolicyDocument": {
"Statement": [
{
"Action": [
"secretsmanager:GetSecretValue"
],
"Effect": "Allow",
"Resource": {
"Fn::Sub": "arn:${AWS::Partition}:secretsmanager:us-west-2:123456789012:secret:my-path/my-secret-name-1a2b01"
}
},
{
"Action": [
"mq:DescribeBroker"
],
"Effect": "Allow",
"Resource": {
"Fn::Sub": "arn:${AWS::Partition}:mq:us-east-2:123456789012:broker:MyBroker:b-1234a5b6-78cd-901e-2fgh-3i45j6k17801"
}
}
]
},
"PolicyName": "SamAutoGeneratedAMQPolicy"
},
{
"PolicyDocument": {
"Statement": [
{
"Action": [
"secretsmanager:GetSecretValue"
],
"Effect": "Allow",
"Resource": {
"Fn::Sub": "arn:${AWS::Partition}:secretsmanager:us-west-2:123456789012:secret:my-path/my-secret-name-1a2b03"
}
},
{
"Action": [
"mq:DescribeBroker"
],
"Effect": "Allow",
"Resource": {
"Fn::Sub": "arn:${AWS::Partition}:mq:us-east-2:123456789012:broker:MyBroker:b-1234a5b6-78cd-901e-2fgh-3i45j6k17803"
}
}
]
},
"PolicyName": "MyFunctionBrokerThreeAMQPolicy"
},
{
"PolicyDocument": {
"Statement": [
{
"Action": [
"secretsmanager:GetSecretValue"
],
"Effect": "Allow",
"Resource": {
"Fn::Sub": "arn:${AWS::Partition}:secretsmanager:us-west-2:123456789012:secret:my-path/my-secret-name-1a2b02"
}
},
{
"Action": [
"mq:DescribeBroker"
],
"Effect": "Allow",
"Resource": {
"Fn::Sub": "arn:${AWS::Partition}:mq:us-east-2:123456789012:broker:MyBroker:b-1234a5b6-78cd-901e-2fgh-3i45j6k17802"
}
}
]
},
"PolicyName": "MyFunctionBrokerTwoAMQPolicy"
}
],
"Tags": [
{
"Key": "lambda:createdBy",
"Value": "SAM"
}
]
},
"Type": "AWS::IAM::Role"
}
}
}
Loading