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: Create group with optional assumable roles #481

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

schollii
Copy link
Contributor

@schollii schollii commented Apr 20, 2024

Description

Add create_group to iam-group-with-assumable-roles-policy

Motivation and Context

Sorry this is a bit long, because I need to show by example.

Currently, iam-group-with-policies has create_group but iam-group-with-assumable-roles-policy does not, yet iam-group-with-assumable-roles-policy fails plan if its list of assumable roles is empty. We need IAM groups that can have policies and optionally have assumable roles, which from your master branch, we achieved by using the iam-group-complete example as basis and editing:

locals {
  create_group_with_assumable_roles = (var.assumable_roles != [])
}

module "iam_group2" {
  count = local.create_group_with_assumable_roles ? 1 : 0

  source = "../../modules/iam-group-with-assumable-roles-policy"
  name   = var.group_name

  assumable_roles = var.assumable_roles
}

module "iam_group_default2" {
  source       = "../../modules/iam-group-with-policies"
  name         = local.create_group_with_assumable_roles ? module.iam_group2[0].group_name : var.group_name
  create_group = !local.create_group_with_assumable_roles

  attach_iam_self_management_policy = false
  custom_group_policy_arns          = var.policy_arns
}

This has a local because the expression would otherwise be used in 3 places, it has a module count, and conditional logic on 2 attributes of the group-with-policies.

Sufficient code to achieve same on PR branch, where create_group is available on the iam-group-with-assumable-roles-policy:

module "iam_group3" {
  source = "../../modules/iam-group-with-policies"
  name   = var.group_name

  attach_iam_self_management_policy = false
  custom_group_policy_arns          = var.policy_arns
}

module "iam_group_roles3" {
  source       = "../../modules/iam-group-with-assumable-roles-policy"
  name         = module.iam_group3.group_name
  create_group = length(var.assumable_roles) > 0

  assumable_roles = var.assumable_roles
}

No local, count, or logic. It is clean and simple to understand what is happening for anyone maintaining this code.

Note: I alternatively considered extending the iam-group-with-assumable-roles-policy module to allow an empty list of roles instead of create_group. However, I believe it is more natural to think of the "basic" group as a group with policies (when do you ever have a group without at least one policy!), and assumable roles as extra for the subset of groups where members need ability to assume roles.

That being said, the iam-group-with-policies has create_group AND supports empty list of policies, why shouldn't iam-group-with-assumable-roles-policy have both too? In this PR, I only implemented create_group.

Breaking Changes

None, at least for terraform 1.1+ since even in 1.1 it had the ability to auto propose resource -> resource[0] after adding a count to an existing resource, which is what happened here for the aws_iam_group.this. None of the existing examples break, as I verified for terraform 1.8 as explained below.

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 verified by terraform apply on master with tf 1.8, then switching to my branch and running terraform plan: terraform auto detects the need for some moves and plans no changes, this is a documented feature.

@schollii
Copy link
Contributor Author

schollii commented Apr 20, 2024

@antonbabenko I was able to fix all the failures in the Max TF pre-commit action. Any chance you could add a Contributing section to your main readme, indicating what should be run. I'm guessing based on the issues I had to fix that, prior to PR, one should

  1. run terraform fmt on the files/folder modified
  2. run terraform-docs but not sure
  3. setup one's IDE to automatically trim space at end of all modified lines and only one blank line at end of file (I had one with 2 blank lines, was rejected)
  4. update the wrapper folder but I'm not sure how, I figured out what to edit based on the diffs that the action showed

That section could also say how to run the pre-commit hooks locally that would be great; when I tried in my first PR a few weeks ago, doing this changed a pile more files than I intended so it is not as trivial as it seems.

@schollii schollii force-pushed the create-group-optional-on-assumable branch 2 times, most recently from 279b5f1 to fc51ef4 Compare April 21, 2024 00:27
@schollii schollii force-pushed the create-group-optional-on-assumable branch from fc51ef4 to 92753be Compare April 21, 2024 00:40
@schollii
Copy link
Contributor Author

schollii commented May 9, 2024

Any chance of some eyes on this?

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.

None yet

1 participant