gettek / terraform-azurerm-policy-as-code

Terraform modules that simplify the workflow of custom and built-in Azure Policies
https://learn.microsoft.com/en-us/azure/governance/policy/concepts/policy-as-code
MIT License
146 stars 68 forks source link

Assignment name scoped to Management group causes duplicate Assignment Ids when attempting to create Set Assignments using the same Policy Set Initiative to multiple locations #111

Open jinkang23 opened 4 weeks ago

jinkang23 commented 4 weeks ago

When deploying the same Policy initiative to multiple Azure regions scoped at Management group (i.e. Tenant Root Group), this creates a duplicate Assignment Id causing the deployment to fail. This can be worked around by generating a globally unique guid and passing that in as assignment_name value, but I would like to ask that both def_assignment and set_assignment sub-modules support generating a random value using random_id resource instead.

locals {
  policy_set_definitions_built_in = {
    enable_allLogs_category_group_resource_logging_for_supported_resources_to_storage       = "b6b86da9-e527-49de-ac59-6af0a9db10b8"
  }
}
data "azurerm_subscription" "current" {}

data "azurerm_management_group" "root" {
  name = data.azurerm_subscription.current.tenant_id
}

data "azurerm_policy_set_definition" "this" {
  for_each = local.policy_set_definitions_built_in

  name = each.value 
}

module "asgmt_root_mg_platform_resource_diagnostics_to_storage_this" {
  for_each = toset(["centralus","eastus2"]) #*<-- deploys to all azure locations specified 

  source  = "gettek/policy-as-code/azurerm//modules/set_assignment"
  version = "2.9.2"

  initiative          = data.azurerm_policy_set_definition.this["enable_allLogs_category_group_resource_logging_for_supported_resources_to_storage"]
  assignment_scope    = data.azurerm_management_group.root.id
  assignment_location = each.key
  assignment_display_name = "Resource Diagnostics Settings to Storage - ${each.key}"

  # resource remediation options
  re_evaluate_compliance = true
  skip_remediation       = true 
  skip_role_assignment   = false
  role_definition_ids    = [data.azurerm_role_definition.contributor.id] # using explicit roles

  # NOTE: You may omit parameters at assignment to use the definitions 'defaultValue'
  assignment_parameters = {
    effect                = "DeployIfNotExists"
    resourceLocation      = each.key
    storageAccount        = local.diagnostics_storage_accounts[each.key].storage_account_id
    diagnosticSettingName = "setByPolicy-Storage" 
  }

  non_compliance_messages = {
    null = "Flagged by Initiative: ${data.azurerm_policy_set_definition.this["enable_allLogs_category_group_resource_logging_for_supported_resources_to_storage"].display_name}"
  }
}

Expected Behavior

Policy Set Assignments for module.asgmt_root_mg_platform_resource_diagnostics_to_storage_this should successfully create two assignments, one per each region specified using the for_each: centralus, eastus2.

Current Behavior

First Policy Set Assignment is created succesfully (i.e. centralus), however, the second Policy Set Assignment fails with 400 Bad Request due to duplicate Policy set Assignment ID.

│ Error: creating Scoped Policy Assignment (Scope: "/providers/Microsoft.Management/managementGroups/00000000-0000-0000-0000-000000000000"
│ Policy Assignment Name: "b6b86da9-e527-49de-ac59-"): unexpected status 400 (400 Bad Request) with error: FailedIdentityOperation: Identity operation for resource '/providers/Microsoft.Management/managementGroups/00000000-0000-0000-0000-000000000000/providers/Microsoft.Authorization/policyAssignments/b6b86da9-e527-49de-ac59-' failed with error 'Failed to perform resource identity operation. Status: 'BadRequest'. Response: '{"error":{"code":"BadRequest","message":""}}'.'.
│
│   with module.asgmt_root_mg_platform_resource_diagnostics_to_storage_this["centralus"].azurerm_management_group_policy_assignment.set[0],
│   on .terraform/modules/asgmt_root_mg_platform_resource_diagnostics_to_storage_this/modules/set_assignment/main.tf line 5, in resource "azurerm_management_group_policy_assignment" "set":
│    5: resource "azurerm_management_group_policy_assignment" "set" {
│
│ creating Scoped Policy Assignment (Scope: "/providers/Microsoft.Management/managementGroups/00000000-0000-0000-0000-000000000000"
│ Policy Assignment Name: "b6b86da9-e527-49de-ac59-"): unexpected status 400 (400 Bad Request) with error: FailedIdentityOperation: Identity operation for resource
│ '/providers/Microsoft.Management/managementGroups/00000000-0000-0000-0000-000000000000/providers/Microsoft.Authorization/policyAssignments/b6b86da9-e527-49de-ac59-' failed with error 'Failed to perform
│ resource identity operation. Status: 'BadRequest'. Response: '{"error":{"code":"BadRequest","message":""}}'.'.

Possible Solution

Update set_assignment and def_assignment sub-modules with following changes:

# variables.tf 
variable "generate_assignment_id" {
  type        = bool
  description = "Generate a random assignment ID"
  default     = false

  validation {
    condition     = var.generate_assignment_id == true ? var.assignment_name == null : true
    error_message = "You must not specify `assignment_name` if `generate_assignment_id` is true."
  }
}

resource "random_id" "assignment_id" {
  keepers = {
    assignment_scope = var.assignment_scope
    assignment_name  = var.assignment_name
    initiative_name  = var.initiative.name
  }
  byte_length = 12
}

locals {

# if `var.generate_assignment_id` == `true`, then use random hex id (exactly 24 chars long) as assignment name, otherwise, use assignment_name or initiative name. This allows for backwards compatibility. 

  assignment_name_trim = local.assignment_scope.mg > 0 ? 24 : 64
  assignment_name      = var.generate_assignment_id ? random_id.assignment_id.hex : try((lower(substr(coalesce(var.assignment_name, var.initiative.name), 0, local.assignment_name_trim))), "")

}
gettek commented 3 weeks ago

Hi @jinkang23,

Thank you for raising this, unfortunately I cannot see how this will be beneficial when one can set assignment_name to give unique values at runtime.

Perhaps you could consider creating the random_id in a for_each loop instead and parsing the key/value mapping to the assignment, thus populating assignment_name to meet your requirements.

Hope this helps

jinkang23 commented 3 weeks ago

Hello @gettek - Thank you for your response.

I have considered that workaround and will technically work in this case. However, I raised this issue in hoping that the modules can be updated to support randomly generating the assignment_name natively as I believe it would be a more elegant approach to solving this issue instead of trying to track the random_id key/value mapping external to the module. That degree of overhead can become a bit complex once we start to manage many Policy assignments at management group scopes and would hope that you would reconsider adding this as a QoL enhancement. Thank you!