terraform-aws-lambda icon indicating copy to clipboard operation
terraform-aws-lambda copied to clipboard

feat: Use inline instead of managed policies

Open RafaelWO opened this issue 1 year ago β€’ 4 comments

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 -a on my pull request

RafaelWO avatar Aug 29 '24 11:08 RafaelWO

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?

RafaelWO avatar Aug 30 '24 08:08 RafaelWO

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
}

RafaelWO avatar Sep 02 '24 15:09 RafaelWO

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

github-actions[bot] avatar Oct 03 '24 00:10 github-actions[bot]

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?

RafaelWO avatar Oct 04 '24 11:10 RafaelWO

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

github-actions[bot] avatar Nov 07 '24 00:11 github-actions[bot]

@antonbabenko Looking forward to your review :slightly_smiling_face:

RafaelWO avatar Nov 07 '24 07:11 RafaelWO

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

github-actions[bot] avatar Dec 08 '24 00:12 github-actions[bot]

@antonbabenko ping :slightly_smiling_face:

RafaelWO avatar Dec 10 '24 14:12 RafaelWO

would be great if this gets merged!

hossamdash avatar Dec 24 '24 18:12 hossamdash

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?

RafaelWO avatar Jan 08 '25 06:01 RafaelWO

This PR is included in version 7.20.0 :tada:

antonbabenko avatar Jan 08 '25 20:01 antonbabenko

@RafaelWO Thank you a lot for this contribution!

antonbabenko avatar Jan 08 '25 20:01 antonbabenko

It was a pleasure 😊

RafaelWO avatar Jan 08 '25 21:01 RafaelWO

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.

github-actions[bot] avatar Feb 08 '25 02:02 github-actions[bot]