Today, we’re diving into another fascinating code review, this time focusing on a pull request submitted by one of our long-time channel members. This pull request explores integrating Azure AD, ahem Entra ID, Privileged Identity Management (PIM) into Terraform modules, aiming to create PIM groups and role assignments dynamically. Let’s dig into what the OP has been working on and see how we can refine the implementation.

First Impressions and Initial Observations

Right off the bat, the OP has crafted a module to manage roles, settings, and assignments in Azure Active Directory (AAD). The goal appears to be handling privileged and non-privileged roles, defining role management policies, and establishing both eligible and active assignments. While I’m not an AAD administrator, I can evaluate the Terraform infrastructure code’s structure and potential areas for improvement.

data "azuread_user" "pim_group_member" {
  for_each            = toset(flatten([for role in local.roles : role.members]))
  user_principal_name = each.value
}

One immediate observation is the use of what I like to call “unstable iterators.” These nested for_each loops calculate complex logic inline, making the code harder to read, test, and maintain. While it works, I recommend extracting these calculations into local variables for better readability and to avoid duplication when iterating across multiple resources. For example, the OP has implemented an iterator to derive Azure AD users and roles, dynamically looping through local data structures. While this approach makes the logic flexible, it also introduces layers of complexity. Simplifying and encapsulating these patterns will reduce the cognitive overhead when revisiting the code or debugging it later.

Understanding the Data Model and Logic

The code revolves around a hierarchical data model: roles have members, and each role can either be privileged or non-privileged. From this structure, the module provisions several resources:

  1. Azure AD Users: Referencing user identities to populate PIM groups.
  2. Azure AD Directory Roles: Assigning roles dynamically based on the configuration.
  3. Azure AD Groups: Creating groups associated with roles.
  4. Role Assignments and Eligibility Requests: Depending on privilege status, roles are either assigned directly or marked for eligible access.
locals {
  roles = [
    {
      role_display_name = "Application Administrator"
      privileged        = true
      members = [
        //"user1@mydomain.com",
        //"user2@mydomain.com"
      ]
    },
    {
      role_display_name = "Attack Simulation Administrator"
      privileged        = false
      members = [
        //"user3@mydomain.com"
      ]
    },
    {
      role_display_name = "Helpdesk Administrator"
      privileged        = true
      members = [
        //"user4@mydomain.com"
      ]
    },

    {
      role_display_name = "Application Developer"
      privileged        = true
      members = [
        //"user5@mydomain.com"
      ]
    }
  ]
}

The differentiation between privileged and non-privileged roles is particularly interesting. Privileged roles use eligibility schedule requests to enable just-in-time access, while non-privileged roles receive stable, long-term assignments. This setup aligns with best practices for secure access management, but the implementation could benefit from clearer encapsulation.

The Iteration Problem

Currently, the module handles all these resources with nested for_each loops scattered throughout the code. This results in what I call “Big-O² complexity” - a loop within a loop. 

While this approach works, it can lead to harder-to-maintain code as the project scales. Right off the bat this yields to complex co-dependent for_each iterators - which causes a lot of complexity.

resource "azuread_group" "pim_group" {

  for_each                = { for role in local.roles : role.role_display_name => role }
  display_name            = "entra_pim_${replace(lower(each.value.role_display_name), " ", "_")}"
  description             = "PIM group for ${each.value.role_display_name}."
  security_enabled        = true
  assignable_to_role      = true
  prevent_duplicate_names = true
  members                 = [for upn in each.value.members : data.azuread_user.pim_group_member[upn].object_id]

}

For instance, the nested loops not only iterate through roles but also traverse role members, creating Azure AD users and group memberships for each. This results in highly repetitive patterns, where the same logic must be adjusted in multiple places if the data model evolves.

Here’s a recommendation: encapsulation through modularization. 

Instead of iterating over all roles and their members in a single module, we can create a submodule focused on managing a single role. This submodule would handle everything related to that role - users, groups, role assignments, and eligibility schedules. Then, the parent module would simply iterate over the collection of roles, instantiating the submodule for each.

Encapsulation: A Better Approach

Encapsulation would significantly streamline the module. By narrowing the scope of responsibilities, the submodule becomes easier to test, maintain, and reason about. Here’s how this would look conceptually:

  1. Parent Module: Accepts a collection of roles and iterates over them.
  2. Submodule: Handles one role at a time, creating its users, groups, and assignments.

This design eliminates the need for deeply nested loops in the main module, reducing complexity and improving readability. It also aligns with Terraform’s best practices for module design, ensuring each module has a clear and focused responsibility. Exploring Role Assignments in Azure AD

During the review, I took a detour to better understand the difference between eligibility and active role assignments. In Azure AD, eligibility assignments enable just-in-time access - users can request temporary permissions when needed. 

resource "azuread_directory_role_eligibility_schedule_request" "pim_eligible_role" {
  for_each           = { for role in local.roles : role.role_display_name => role if role.privileged }
  role_definition_id = azuread_directory_role.pim_role[each.key].template_id
  principal_id       = azuread_group.pim_group[each.key].object_id
  directory_scope_id = "/"
  justification      = "Initial Setup Assignments"
}

Active assignments, on the other hand, grant long-term, stable permissions.

resource "azuread_directory_role_assignment" "pim_assigned_role" {
  for_each            = { for role in local.roles : role.role_display_name => role if !role.privileged }
  role_id             = azuread_directory_role.pim_role[each.key].object_id
  principal_object_id = azuread_group.pim_group[each.key].object_id
}

The OP’s implementation correctly distinguishes between these two scenarios, but some conditional logic could benefit from clarification.

For example, the logic determining whether a role is privileged or not seems a bit counterintuitive. If privileged roles require stable access, the eligibility assignment logic may need adjustment. I encourage the OP to revisit these conditions to ensure they align with the intended behavior.

Final Thoughts and Takeaways

Overall, the OP has done a commendable job tackling a complex problem. The current implementation works but could benefit from some structural refinements. By introducing encapsulation and reducing nested iterations, the module would become easier to manage and scale.

If you’re tackling similar challenges in Terraform, consider adopting the modular design principles we discussed today. And for those watching, remember: clean and maintainable code is just as important as functionality.

Thank you to the OP for submitting this pull request and giving us a great learning opportunity. If you’re interested in having your Terraform code reviewed on the channel, consider becoming a Code Ninja member. Your pull requests could be featured in future reviews, where we’ll collaborate to build even better infrastructure-as-code solutions.

Until next time, this is the Azure Terraformer, signing off. Stay curious, stay coding, and keep those pull requests coming!