terraform-aws-lambda
terraform-aws-lambda copied to clipboard
feat: Use inline instead of managed policies
Description
This MR replaces customer-managed policies with inline policies becuase the policies are only used for the Lambda function. See also Managed policies and inline policies.
Motivation and Context
Fixes #607
Breaking Changes
If users have attached the customer-managed policies to other resources, this change is breaking (in theory) since the new policies will be inline. But it is very unlikely that users did this.
How Has This Been Tested?
- [ ] I have updated at least one of the
examples/*to demonstrate and validate my change(s) - [ ] I have tested and validated these changes using one or more of the provided
examples/*projects
-
[x] I have tested this branch against on of my projects using this module. Below is the terraform plan output
Expand
Terraform will perform the following actions: # module.lambda_function.aws_iam_policy.additional_jsons[0] will be destroyed # (because aws_iam_policy.additional_jsons is not in configuration) - resource "aws_iam_policy" "additional_jsons" { - arn = "arn:aws:iam::123456789012:policy/foo-bar-dev-0" -> null - id = "arn:aws:iam::123456789012:policy/foo-bar-dev-0" -> null - name = "foo-bar-dev-0" -> null - path = "/" -> null - policy = jsonencode( { - Statement = [ - { - Action = "s3:ListBucket" - Effect = "Allow" - Resource = "arn:aws:s3:::mybucket-dev" - Sid = "ListObjectsInBucket" }, - { - Action = "s3:PutObject" - Effect = "Allow" - Resource = "arn:aws:s3:::mybucket-dev/*" - Sid = "WriteOnly" }, ] - Version = "2012-10-17" } ) -> null - policy_id = "asdfasdfa" -> null - tags = {} -> null - tags_all = { - "Environment" = "dev" - "Owner" = "Terraform" } -> null # (2 unchanged attributes hidden) } # module.lambda_function.aws_iam_policy.logs[0] will be destroyed # (because aws_iam_policy.logs is not in configuration) - resource "aws_iam_policy" "logs" { - arn = "arn:aws:iam::123456789012:policy/foo-bar-dev-logs" -> null - id = "arn:aws:iam::123456789012:policy/foo-bar-dev-logs" -> null - name = "foo-bar-dev-logs" -> null - path = "/" -> null - policy = jsonencode( { - Statement = [ - { - Action = [ - "logs:PutLogEvents", - "logs:CreateLogStream", - "logs:CreateLogGroup", ] - Effect = "Allow" - Resource = [ - "arn:aws:logs:eu-west-1:123456789012:log-group:/aws/lambda/foo-bar-dev:*:*", - "arn:aws:logs:eu-west-1:123456789012:log-group:/aws/lambda/foo-bar-dev:*", ] }, ] - Version = "2012-10-17" } ) -> null - policy_id = "asdfasdf" -> null - tags = {} -> null - tags_all = { - "Environment" = "dev" - "Owner" = "Terraform" } -> null # (2 unchanged attributes hidden) } # module.lambda_function.aws_iam_role_policy.additional_jsons[0] will be created + resource "aws_iam_role_policy" "additional_jsons" { + id = (known after apply) + name = "foo-bar-dev-0" + name_prefix = (known after apply) + policy = jsonencode( { + Statement = [ + { + Action = "s3:ListBucket" + Effect = "Allow" + Resource = "arn:aws:s3:::mybucket-dev" + Sid = "ListObjectsInBucket" }, + { + Action = "s3:PutObject" + Effect = "Allow" + Resource = "arn:aws:s3:::mybucket-dev/*" + Sid = "WriteOnly" }, ] + Version = "2012-10-17" } ) + role = "foo-bar-dev" } # module.lambda_function.aws_iam_role_policy.logs[0] will be created + resource "aws_iam_role_policy" "logs" { + id = (known after apply) + name = "foo-bar-dev-logs" + name_prefix = (known after apply) + policy = jsonencode( { + Statement = [ + { + Action = [ + "logs:PutLogEvents", + "logs:CreateLogStream", + "logs:CreateLogGroup", ] + Effect = "Allow" + Resource = [ + "arn:aws:logs:eu-west-1:123456789012:log-group:/aws/lambda/foo-bar-dev:*:*", + "arn:aws:logs:eu-west-1:123456789012:log-group:/aws/lambda/foo-bar-dev:*", ] }, ] + Version = "2012-10-17" } ) + role = "foo-bar-dev" } # module.lambda_function.aws_iam_role_policy_attachment.additional_jsons[0] will be destroyed # (because aws_iam_role_policy_attachment.additional_jsons is not in configuration) - resource "aws_iam_role_policy_attachment" "additional_jsons" { - id = "foo-bar-dev-20240826142417847400000001" -> null - policy_arn = "arn:aws:iam::123456789012:policy/foo-bar-dev-0" -> null - role = "foo-bar-dev" -> null } # module.lambda_function.aws_iam_role_policy_attachment.logs[0] will be destroyed # (because aws_iam_role_policy_attachment.logs is not in configuration) - resource "aws_iam_role_policy_attachment" "logs" { - id = "foo-bar-dev-20240826142417916300000003" -> null - policy_arn = "arn:aws:iam::123456789012:policy/foo-bar-dev-logs" -> null - role = "foo-bar-dev" -> null } Plan: 2 to add, 0 to change, 4 to destroy. -
[x] I have executed
pre-commit run -aon my pull request
From the first look at this PR, it looks pretty good, but why are only some policy attachments changed? Should we update
vpc,tracing, etc also?
Happy to change those as well but I was not sure because it looks like it they are copied from AWS-managed policies:
Copying AWS managed policy to be able to attach the same policy with multiple roles without overwrites by another function
Apparently, there was an issue with only creating an aws_iam_role_policy_attachment with the AWS-managed policy ARN (see https://github.com/terraform-aws-modules/terraform-aws-lambda/pull/26). Or is this obsolete?
Should we update vpc, tracing, etc also?
@antonbabenko, or did you mean using the policy from the "copy" data block in a aws_iam_role_policy? I.e. changing
# Copying AWS managed policy to be able to attach the same policy with multiple roles without overwrites by another function
data "aws_iam_policy" "vpc" {
count = local.create_role && var.attach_network_policy ? 1 : 0
arn = "arn:${data.aws_partition.current.partition}:iam::aws:policy/service-role/AWSLambdaENIManagementAccess"
}
resource "aws_iam_policy" "vpc" {
count = local.create_role && var.attach_network_policy ? 1 : 0
name = "${local.policy_name}-vpc"
path = var.policy_path
policy = data.aws_iam_policy.vpc[0].policy
tags = var.tags
}
resource "aws_iam_role_policy_attachment" "vpc" {
count = local.create_role && var.attach_network_policy ? 1 : 0
role = aws_iam_role.lambda[0].name
policy_arn = aws_iam_policy.vpc[0].arn
}
to
# Copying AWS managed policy to be able to attach the same policy with multiple roles without overwrites by another function
data "aws_iam_policy" "vpc" {
count = local.create_role && var.attach_network_policy ? 1 : 0
arn = "arn:${data.aws_partition.current.partition}:iam::aws:policy/service-role/AWSLambdaENIManagementAccess"
}
resource "aws_iam_role_policy" "vpc" {
count = local.create_role && var.attach_network_policy ? 1 : 0
name = "${local.policy_name}-vpc"
role = aws_iam_role.lambda[0].name
policy = data.aws_iam_policy.vpc[0].policy
}
This PR has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this PR will be closed in 10 days
Should we update vpc, tracing, etc also?
@antonbabenko, I updated vpc and tracing as well (as described in my comment above) :slightly_smiling_face:
I don't think there is anything left, or is there?
This PR has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this PR will be closed in 10 days
@antonbabenko Looking forward to your review :slightly_smiling_face:
This PR has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this PR will be closed in 10 days
@antonbabenko ping :slightly_smiling_face:
would be great if this gets merged!
I guess I somehow messed up the PR by rebasing after the master was merged into my branch :grimacing:
~~Should I open a new clean PR with my changes?~~ NVM: I fixed it.
@antonbabenko do you want to merge master into this one before merging?
This PR is included in version 7.20.0 :tada:
@RafaelWO Thank you a lot for this contribution!
It was a pleasure π
I'm going to lock this pull request because it has been closed for 30 days β³. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.