feat: aad group creation

[matt-FFFFFF]

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Sub-module to create n number of AAD groups and assign them to the subscription (or child scopes)

Describe the solution you'd like

Something like this...

module "lz-vending" {
  source  = "Azure/lz-vending/azurerm"
  version = "2.1.1"

  # other vars...
  aad_group_enabled = true
  aad_groups = {
    contrib_group = {
      name = "myContribGroup"
      role_assignments = [
        {
          definition     = "Contributor"
          relative_scope = ""
        }
      ]
    }
  }
}

Additional context

[jaredfholgate]

@matt-FFFFFF and @kewalaka I understand this from a convenience perspective. I'm just wondering if this moving the module into the monolith territory and perhaps composition would be an alternative approach with some example documentation. I am obviously slightly biased given I am the owner of the module, but we do have an AVM module that could help with this. https://github.com/Azure/terraform-azurerm-avm-res-authorization-roleassignment

This change will add a dependency on the azuread provider in the lz-vending module that could be avoided if we used composition instead. Not sure if that is a bad thing, but just something to consider.

I'm honestly not sure which is the 'correct' approach here, but wanted to raise it as a suggestion.

I also appreciate the AVM module doesn't support conditions or creating the aad group yet, but they could be added fairly quickly if desired.

[kewalaka]

hi @jaredfholgate I see this more as adding support for AAD groups rather than a role assignments thing.

I grant I was perhaps a little lazy in the way I did role assignments - I included it in the module because it saved me having to create a separate locals map to compute the combination of "AAD to associated RA". This is particularly useful to hide complexity in my use case because I'm using the "map of yaml files" pattern in the root module, which already makes for some fun locals.

Whether it fits in the module - it started life as an external component for a customer LZ vending machine based on this module, and it works grand that way. I hear you on avoiding this being the tipping point leading to a super-LZ-module, so if you decide not to accept I am just fine about it. For me - it was written - so why not share 😊

[kewalaka]

@jaredfholgate i've looked at your RA module properly now and I admit I had not been paying attention - I see all the things it does re managing the translation of names and the various targets, not just RBAC.

Nice 💖.

This definitely motivates me to do a refactor - I have my own home-brew approach for AAD group membership & this is deffo for the chop now!

[kewalaka]

@jaredfholgate a problem I've found with using an external role assignments module where the resources that I want to permissions to are created in the LZ module.

For this scenario, I place a 'depends_on' the role assignment module so that it waits for the LZ module to finish.

The depends on triggers the data blocks within the role assignment module to be re-calculated, this makes attributes on the permissions "known after apply" and therefore get dropped and re-created.

As a potential workaround, I'm experimenting with terraform_data & triggers_replace at the moment.

The code isn't public so i can't share - hopefully the above makes sense.

[MoussaBangre]

i ve been trying to have the ad groups created separately with another module, but then when referencing it in the vending module i got the for_each limitation with the map: Error: Invalid for_each argument
│
│ on .terraform/modules/vending.lz_vending/main.roleassignment.tf line 12, in module "roleassignment":
│ 12: for_each = { for k, v in var.role_assignments : k => v if var.role_assignment_enabled }
│ ├────────────────
│ │ var.role_assignment_enabled is true
│ │ var.role_assignments is a map of object, known only after apply
│
│ The "for_each" map includes keys derived from resource attributes that cannot be determined until apply, and so Terraform cannot determine the full set of keys that will identify the instances
│ of this resource.
│
│ When working with unknown values in for_each, it's better to define the map keys statically in your configuration and place apply-time results only in the map values.
│
│ Alternatively, you could use the -target planning option to first apply only the resources that the for_each value depends on, and then apply a second time to fully converge.

Any idea ?

[matt-FFFFFF]

You need to make the map keys static strings. It won't work if the map keys are unknown.