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: Replace aws_iam_policy_attachment to aws_iam_role_policy_attachment #195

Conversation

wsim-plaid
Copy link
Contributor

@wsim-plaid wsim-plaid commented Sep 10, 2021

Description

This PR replaces aws_iam_policy_attachment to aws_iam_role_policy_attachment

Motivation and Context

Terraform official documentation recommends not to use aws_iam_policy_attachment but to use aws_iam_role_policy_attachment, aws_iam_user_policy_attachment, or aws_iam_group_policy_attachment (reference: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy_attachment)

image

How Has This Been Tested?

  • [V] I have tested and validated these changes using one or more of the provided examples/* projects

@wsim-plaid wsim-plaid force-pushed the replace_aws_iam_policy_attachment branch from 1ef9124 to 96b1ddf Compare September 10, 2021 19:16
@antonbabenko
Copy link
Member

Thanks for the PR!

#45 and #40 were about the same but I don't have time to look into this in detail just yet.

Can't remember right now why it is implemented the way it is.

@wsim-plaid
Copy link
Contributor Author

wsim-plaid commented Sep 10, 2021

@antonbabenko Thanks for the quick response! I quickly looked into #40 and #45 - #40 was closed because #45 was implemented. However, #45 only updated 2 resources in the template, and this PR (#195) updates the rest in the template.

@antonbabenko
Copy link
Member

Well, sometimes I reply fast :)

When I see new PRs I am trying to figure out if the request is even valid, more specifically:

  1. Why there were no such exact issues opened in the past?
  2. Why did we fix it just in a couple of places and not in all?

I need to think a bit but probably I will approve this PR next week.

@wsim-plaid
Copy link
Contributor Author

@antonbabenko Sounds great! I appreciate it!
(Just to give you a bit more context why I opened up this PR, we're not allowing this specific resource internally and our engineering wants to use this module for their development and this PR should unblock them :)) Let me know if there's anything I can help.

@antonbabenko antonbabenko changed the title fix: replace aws_iam_policy_attachment to aws_iam_role_policy_attachment fix: Replace aws_iam_policy_attachment to aws_iam_role_policy_attachment Sep 11, 2021
@antonbabenko antonbabenko merged commit 7c53da1 into terraform-aws-modules:master Sep 11, 2021
@antonbabenko
Copy link
Member

Even though this PR will require recreation of some resources it is still having the same value for the users (the same policies will be attached), so I think it is fine to treat this as a minor release.

v2.17.0 has been just released.

Thanks @wsim-plaid ! Hopefully, your engineers will have no issues with Lambda and Terraform from now on :)

@wsim-plaid
Copy link
Contributor Author

Thank you so much for quick review (again) Anton!!

@pdecat
Copy link
Contributor

pdecat commented Sep 15, 2021

Terraform official documentation recommends not to use

From my understanding, this is more of a warning to explain the implications of using that resource, than a recommendation to not use it.
Having a resource which does exclusive attachment can be useful to detect policy attachments not created by a terraform configuration.
Also, creating a policy dedicated to each lambda allows to avoid issues.

@antonbabenko
Copy link
Member

Good point, I also think that the "warning" in the Terraform official documentation should be more like a "notice".

I personally prefer to have exclusive controls over functions and manage all IAM relations for Lambdas using this module but sometimes users want to reuse created IAM policies in their other infrastructure components and thus break the exclusivity of an attachment.

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

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 github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants