-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: support conditional AWS::NoValue in SAM Function's IAM Role #3842
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
base: develop
Are you sure you want to change the base?
Changes from 4 commits
ef9cd22
6a37502
79e8680
b01c455
cf96ad9
83e1f03
615f202
026679c
31cec02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -388,16 +388,13 @@ def to_cloudformation(self, **kwargs): # type: ignore[no-untyped-def] # noqa: P | |
| managed_policy_map = kwargs.get("managed_policy_map", {}) | ||
| get_managed_policy_map = kwargs.get("get_managed_policy_map") | ||
|
|
||
| execution_role = None | ||
| if lambda_function.Role is None: | ||
| execution_role = self._construct_role( | ||
| managed_policy_map, | ||
| event_invoke_policies, | ||
| intrinsics_resolver, | ||
| get_managed_policy_map, | ||
| ) | ||
| lambda_function.Role = execution_role.get_runtime_attr("arn") | ||
| resources.append(execution_role) | ||
| execution_role = self._construct_role( | ||
| managed_policy_map, | ||
| event_invoke_policies, | ||
| intrinsics_resolver, | ||
| get_managed_policy_map, | ||
| ) | ||
| self._make_lambda_role(lambda_function, intrinsics_resolver, execution_role, resources) | ||
|
|
||
| try: | ||
| resources += self._generate_event_resources( | ||
|
|
@@ -415,6 +412,42 @@ def to_cloudformation(self, **kwargs): # type: ignore[no-untyped-def] # noqa: P | |
|
|
||
| return resources | ||
|
|
||
| def _make_lambda_role( | ||
| self, | ||
| lambda_function: LambdaFunction, | ||
| intrinsics_resolver: IntrinsicsResolver, | ||
| execution_role: IAMRole, | ||
| resources: List[Any], | ||
| ) -> None: | ||
| lambda_role = lambda_function.Role | ||
|
|
||
| if lambda_role is None: | ||
| resources.append(execution_role) | ||
| lambda_function.Role = execution_role.get_runtime_attr("arn") | ||
|
|
||
| if is_intrinsic_if(lambda_role): | ||
valerena marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| resources.append(execution_role) | ||
|
||
|
|
||
| # We need to create and if else condition here | ||
| role_resolved_value = intrinsics_resolver.resolve_parameter_refs(self.Role) | ||
valerena marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| role_list = role_resolved_value.get("Fn::If") | ||
valerena marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| # both are none values then we need to create a role | ||
| if is_intrinsic_no_value(role_list[1]) and is_intrinsic_no_value(role_list[2]): | ||
| lambda_function.Role = execution_role.get_runtime_attr("arn") | ||
|
|
||
| # first value is none so we should create condition ? create : [2] | ||
| elif is_intrinsic_no_value(role_list[1]): | ||
| lambda_function.Role = make_conditional( | ||
| role_list[0], execution_role.get_runtime_attr("arn"), role_list[2] | ||
| ) | ||
|
|
||
| # second value is none so we should create condition ? [1] : create | ||
| elif is_intrinsic_no_value(role_list[2]): | ||
| lambda_function.Role = make_conditional( | ||
| role_list[0], role_list[1], execution_role.get_runtime_attr("arn") | ||
| ) | ||
|
|
||
| def _construct_event_invoke_config( # noqa: PLR0913 | ||
| self, | ||
| function_name: str, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -741,6 +741,118 @@ def test_function_datasource_set_with_none(): | |||||||||||
| assert none_datasource | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| class TestSamFunctionRoleResolver(TestCase): | ||||||||||||
| """ | ||||||||||||
| Tests for resolving IAM role property values in SamFunction | ||||||||||||
| """ | ||||||||||||
|
|
||||||||||||
| def setUp(self): | ||||||||||||
| self.function = SamFunction("foo") | ||||||||||||
| self.function.CodeUri = "s3://foobar/foo.zip" | ||||||||||||
| self.function.Runtime = "foo" | ||||||||||||
| self.function.Handler = "bar" | ||||||||||||
| self.template = {"Conditions": {}} | ||||||||||||
|
||||||||||||
|
|
||||||||||||
| self.kwargs = { | ||||||||||||
| "intrinsics_resolver": IntrinsicsResolver({}), | ||||||||||||
| "event_resources": [], | ||||||||||||
| "managed_policy_map": {}, | ||||||||||||
| "resource_resolver": ResourceResolver({}), | ||||||||||||
| "conditions": self.template.get("Conditions", {}), | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| def test_role_none_creates_execution_role(self): | ||||||||||||
| self.function.Role = None | ||||||||||||
| cfn_resources = self.function.to_cloudformation(**self.kwargs) | ||||||||||||
| generated_roles = [x for x in cfn_resources if isinstance(x, IAMRole)] | ||||||||||||
|
|
||||||||||||
| self.assertEqual(len(generated_roles), 1) # Should create execution role | ||||||||||||
|
|
||||||||||||
| def test_role_explicit_arn_no_execution_role(self): | ||||||||||||
| test_role = "arn:aws:iam::123456789012:role/existing-role" | ||||||||||||
| self.function.Role = test_role | ||||||||||||
|
|
||||||||||||
| cfn_resources = self.function.to_cloudformation(**self.kwargs) | ||||||||||||
| generated_roles = [x for x in cfn_resources if isinstance(x, IAMRole)] | ||||||||||||
| lambda_function = next(r for r in cfn_resources if r.resource_type == "AWS::Lambda::Function") | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can do the same than for the roles with the functions: But it's not a big deal. We can probably change it in the future if needed. |
||||||||||||
|
|
||||||||||||
| self.assertEqual(len(generated_roles), 0) # Should not create execution role | ||||||||||||
| self.assertEqual(lambda_function.Role, test_role) | ||||||||||||
|
|
||||||||||||
| def test_role_fn_if_no_aws_no_value_keeps_original(self): | ||||||||||||
| role_conditional = { | ||||||||||||
| "Fn::If": ["Condition", "arn:aws:iam::123456789012:role/existing-role", {"Ref": "iamRoleArn"}] | ||||||||||||
| } | ||||||||||||
| self.function.Role = role_conditional | ||||||||||||
|
|
||||||||||||
| template = {"Conditions": {"Condition": True}} | ||||||||||||
| kwargs = dict(self.kwargs) | ||||||||||||
| kwargs["conditions"] = template.get("Conditions", {}) | ||||||||||||
|
||||||||||||
| template = {"Conditions": {"Condition": True}} | |
| kwargs = dict(self.kwargs) | |
| kwargs["conditions"] = template.get("Conditions", {}) | |
| kwargs = dict(self.kwargs) | |
| kwargs["conditions"] = {"Condition": True} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| Resources: | ||
| MinimalFunction: | ||
| Type: AWS::Serverless::Function | ||
| Properties: | ||
| CodeUri: s3://sam-demo-bucket/hello.zip | ||
| Handler: hello.handler | ||
| Runtime: python3.10 | ||
| Role: 2 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| Parameters: | ||
| iamRoleArn: | ||
| Type: String | ||
| Description: The ARN of an IAM role to use as this function's execution role. | ||
| If a role isn't specified, one is created for you with a logical ID of <function-logical-id>Role. | ||
|
|
||
| Conditions: | ||
| RoleExists: !Not [!Equals ['', !Ref iamRoleArn]] | ||
|
|
||
| Resources: | ||
| MinimalFunction: | ||
| Type: AWS::Serverless::Function | ||
| Properties: | ||
| CodeUri: s3://sam-demo-bucket/hello.zip | ||
| Handler: hello.handler | ||
| Runtime: python3.10 | ||
| Role: !If | ||
| - RoleExists | ||
| - !Ref "iamRoleArn" | ||
| - !Ref "AWS::NoValue" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| { | ||
| "_autoGeneratedBreakdownErrorMessage": [ | ||
| "Invalid Serverless Application Specification document. ", | ||
| "Number of errors found: 1. ", | ||
| "Resource with id [MinimalFunction] is invalid. ", | ||
| "Property 'Role' should be a string." | ||
| ], | ||
| "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. Resource with id [MinimalFunction] is invalid. Property 'Role' should be a string.", | ||
| "errors": [ | ||
| { | ||
| "errorMessage": "Resource with id [MinimalFunction] is invalid. Property 'Role' should be a string." | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| { | ||
| "Conditions": { | ||
| "RoleExists": { | ||
| "Fn::Not": [ | ||
| { | ||
| "Fn::Equals": [ | ||
| "", | ||
| { | ||
| "Ref": "iamRoleArn" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| "Parameters": { | ||
| "iamRoleArn": { | ||
| "Description": "The ARN of an IAM role to use as this function's execution role. If a role isn't specified, one is created for you with a logical ID of <function-logical-id>Role.", | ||
| "Type": "String" | ||
| } | ||
| }, | ||
| "Resources": { | ||
| "MinimalFunction": { | ||
| "Properties": { | ||
| "Code": { | ||
| "S3Bucket": "sam-demo-bucket", | ||
| "S3Key": "hello.zip" | ||
| }, | ||
| "Handler": "hello.handler", | ||
| "Role": { | ||
| "Fn::If": [ | ||
| "RoleExists", | ||
| { | ||
| "Ref": "iamRoleArn" | ||
| }, | ||
| { | ||
| "Fn::GetAtt": [ | ||
| "MinimalFunctionRole", | ||
| "Arn" | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| "Runtime": "python3.10", | ||
| "Tags": [ | ||
| { | ||
| "Key": "lambda:createdBy", | ||
| "Value": "SAM" | ||
| } | ||
| ] | ||
| }, | ||
| "Type": "AWS::Lambda::Function" | ||
| }, | ||
| "MinimalFunctionRole": { | ||
| "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" | ||
| ], | ||
| "Tags": [ | ||
| { | ||
| "Key": "lambda:createdBy", | ||
| "Value": "SAM" | ||
| } | ||
| ] | ||
| }, | ||
| "Type": "AWS::IAM::Role" | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| { | ||
| "_autoGeneratedBreakdownErrorMessage": [ | ||
| "Invalid Serverless Application Specification document. ", | ||
| "Number of errors found: 1. ", | ||
| "Resource with id [MinimalFunction] is invalid. ", | ||
| "Property 'Role' should be a string." | ||
| ], | ||
| "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. Resource with id [MinimalFunction] is invalid. Property 'Role' should be a string.", | ||
| "errors": [ | ||
| { | ||
| "errorMessage": "Resource with id [MinimalFunction] is invalid. Property 'Role' should be a string." | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this implementation has some unnecessary change IMO, we can keep the
if lambda_function.Role is Nonepart as is, then_make_lambda_rolecan return the created role, so we don't need to pass inlambda_functionin this function.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other concern is we are modifying values for lambda_function, execution_role, resources, conditions ( which are the function's input) inside this function which returns None, but all these values might have changed after running this function. This might make this part of the code harder to review in the future. If we need to change all these variable's value in place, I would perfer to implement this directly in
to_cloudformationinstead of this subfunction. If you really prefer to have this subfunction. Maybe we can return the new value instead of modifying them inplace. So future reviewers could easily understand these variables are modified in this subfunction.