feat: Create group with optional assumable roles

schollii commented 6 months ago

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?

[x] I have updated at least one of the examples/* to demonstrate and validate my change(s)
[x] 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 commented 6 months ago

@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

run terraform fmt on the files/folder modified
run terraform-docs but not sure
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)
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 commented 4 months ago

Any chance of some eyes on this?

github-actions[bot] commented 3 months ago

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

schollii commented 3 months ago

I guess I'll fork the repo, but I would much prefer not to. Can I hear a definitive yes/no, as right now I can't tell if it has even been seen.

github-actions[bot] commented 2 months ago

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] commented 2 months ago

This PR was automatically closed because of stale in 10 days

github-actions[bot] commented 1 month ago

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.

terraform-aws-modules / terraform-aws-iam