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

feat: Use inline instead of managed policies #615

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RafaelWO
Copy link

@RafaelWO RafaelWO commented Aug 29, 2024

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
  • 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.
    
  • I have executed pre-commit run -a on my pull request

@RafaelWO RafaelWO changed the title Use inline instead of managed policies refactor: Use inline instead of managed policies Aug 29, 2024
@RafaelWO RafaelWO changed the title refactor: Use inline instead of managed policies chore: Use inline instead of managed policies Aug 29, 2024
@RafaelWO RafaelWO force-pushed the change/use-inline-policies branch from d042c7d to f86cdf4 Compare August 29, 2024 12:01
@RafaelWO RafaelWO marked this pull request as ready for review August 29, 2024 13:27
@RafaelWO RafaelWO changed the title chore: Use inline instead of managed policies feat: Use inline instead of managed policies Aug 29, 2024
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

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?

@RafaelWO
Copy link
Author

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 #26). Or is this obsolete?

@RafaelWO
Copy link
Author

RafaelWO commented Sep 2, 2024

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
}

Copy link

github-actions bot commented Oct 3, 2024

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 github-actions bot added the stale label Oct 3, 2024
@RafaelWO RafaelWO force-pushed the change/use-inline-policies branch from f86cdf4 to a1d27a7 Compare October 4, 2024 11:42
@RafaelWO
Copy link
Author

RafaelWO commented Oct 4, 2024

Should we update vpc, tracing, etc also?

@antonbabenko, I updated vpc and tracing as well (as described in my comment above) 🙂

I don't think there is anything left, or is there?

@RafaelWO RafaelWO force-pushed the change/use-inline-policies branch from a1d27a7 to 42a3fb6 Compare October 4, 2024 12:33
@github-actions github-actions bot removed the stale label Oct 5, 2024
* Remove unused variable `policy_path`
@RafaelWO RafaelWO force-pushed the change/use-inline-policies branch from 42a3fb6 to d2f44e8 Compare October 7, 2024 07:23
Copy link

github-actions bot commented Nov 7, 2024

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 github-actions bot added the stale label Nov 7, 2024
@RafaelWO
Copy link
Author

RafaelWO commented Nov 7, 2024

@antonbabenko Looking forward to your review 🙂

@github-actions github-actions bot removed the stale label Nov 8, 2024
Copy link

github-actions bot commented Dec 8, 2024

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 github-actions bot added the stale label Dec 8, 2024
@RafaelWO
Copy link
Author

@antonbabenko ping 🙂

@github-actions github-actions bot removed the stale label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Inline Policies instead of Managed
2 participants