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: Enable override policy name iam-group-with-assumable-roles-policy #468

Conversation

schollii
Copy link
Contributor

@schollii schollii commented Mar 17, 2024

Description

Add an extra variable so that the assume-roles policy created by iam-group-with-assumable-roles-policy module can be customized with minimal effort: just have an optional suffix that defaults to empty so this PR is fully backwards compatible. Eg

module "iam_group_with_assumable_roles_policy_production_admin" {
  source = "../../modules/iam-group-with-assumable-roles-policy"

  name = "production-admin"
  assumable_roles = [module.iam_assumable_roles_in_prod.admin_iam_role_arn]
  assumable_roles_policy_name_suffix = "-assumable-roles"
  ...

Motivation and Context

I have been using this module for many months and I really dislike the default name, which is just the group name, because I for one have IAM groups that have a policies named after them since they represenent the group's "main permissions". Eg if I create group Foo, I have policy named Foo that gives that group it's basic set of permissions (eg read-only permissions), and I want a policy FooAssumeRoles that provides the roles that can be assumed by members of that group.

Breaking Changes

None

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

I applied using the example, in my own AWS account, on master branch. Then I switched to my branch and re-planned: no changes. Then, I set a non-empty value to assumable_roles_policy_name_suffix and the diff was correct: policy would be replaced due to new name and the attachment too.

@bryantbiggs
Copy link
Member

@Smana is there a reason you are outputting these statements across a number of our repositories? You've just displayed several details that most would not want to be publicly available

@Smana
Copy link

Smana commented Apr 2, 2024

I'm confused, I'm currently testing the tofu-controller branch planner and I probably misconfigured something. I'm gonna delete these comments, sorry for the inconvenience.

@Smana
Copy link

Smana commented Apr 2, 2024

That should be done by now. I'm gonna talk to the maintainers because honestly I don't know what I did wrong here. Let me know if I missed something.

@schollii schollii changed the title Enable override policy name fix: Enable override policy name Apr 2, 2024
@schollii
Copy link
Contributor Author

schollii commented Apr 2, 2024

@bryantbiggs I am not able to see why the "collect workflow inputs" pre-commit check fails, no logs for that action except "internal error"
Screenshot_20240402-082119

@antonbabenko
Copy link
Member

@schollii I rerun GH Actions, and now it shows that something is not right - https://github.com/terraform-aws-modules/terraform-aws-iam/actions/runs/8318806399/job/23343434805?pr=468

@schollii
Copy link
Contributor Author

schollii commented Apr 2, 2024

Yeah the collect workflow passes now. Not sure why all these additional files got changed, it's when I ran the pre-commit tool locally I think. Also I forgot to restore the example file so it will run on your aws, I needed to test in my own account. I'll clean all this up, sorry for the messy PR.

@schollii schollii force-pushed the enable-override-policy-name branch from 3697705 to 09690f4 Compare April 5, 2024 00:15
@schollii schollii force-pushed the enable-override-policy-name branch from 09690f4 to 59465ce Compare April 5, 2024 00:17
@schollii
Copy link
Contributor Author

schollii commented Apr 5, 2024

OK @antonbabenko the PR contains only the 4 files I intended to change and there is 1 wrapper file which I'm not sure should be changed but all pre-commit checks pass now

@antonbabenko antonbabenko changed the title fix: Enable override policy name fix: Enable override policy name iam-group-with-assumable-roles-policy Apr 5, 2024
@antonbabenko antonbabenko changed the title fix: Enable override policy name iam-group-with-assumable-roles-policy feat: Enable override policy name iam-group-with-assumable-roles-policy Apr 5, 2024
@antonbabenko antonbabenko merged commit bf013d2 into terraform-aws-modules:master Apr 5, 2024
36 checks passed
@oliver-sentianse
Copy link

Thanks for quick merge, I'll be making use of it today!

antonbabenko pushed a commit that referenced this pull request Apr 8, 2024
## [5.39.0](v5.38.0...v5.39.0) (2024-04-08)

### Features

* Enable override policy name iam-group-with-assumable-roles-policy ([#468](#468)) ([bf013d2](bf013d2))
* Update VPC CNI policy to 3/4/24 ([#476](#476)) ([f9d5e28](f9d5e28))
@antonbabenko
Copy link
Member

This PR is included in version 5.39.0 🎉

Copy link

github-actions bot commented May 9, 2024

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 May 9, 2024
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.

5 participants