hashicorp / terraform-provider-azurerm

Terraform provider for Azure Resource Manager
https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs
Mozilla Public License 2.0
4.6k stars 4.64k forks source link

Support for better error handling when importing azure role definitions #26682

Open scratch85 opened 3 months ago

scratch85 commented 3 months ago

Is there an existing issue for this?

Community Note

Description

Import role definitions into tfstate requires to give an id + scope (see current docs).

According to the note, the Id consists is {roleDefinitionId}|{scope}. Azure Portal and other tools just return a UUID as roleDefinitionId.

This lead me to use the import like this (for a tenant level role definition):

import {
   to = azurerm_role_definition.myrole
   id = "12345678-1234-1234-1234-1234567890ab|${data.azurerm_management_group.tenant_root_group.id}"
}

this will lead to uncaught error:

panic: runtime error: index out of range [1] with length 1

goroutine 1100 [running]:
github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization/parse.RoleDefinitionId({0xc003636b80, 0x7a})
    github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization/parse/role_definition.go:66 +0x2b7
github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization.(*RoleDefinitionResource).IDValidationFunc.RoleDefinitionResource.IDValidationFunc.func1({0x7071ce0?, 0xc00394ba20?}, {0x829f888?, 0xc003c472e8?})
...

role_definition.go is not printing a proper error message if the split string got an invalid "roleDefinitionId" missing the keyword "roleDefinitions/".

This is the split: https://github.com/hashicorp/terraform-provider-azurerm/blob/1aef7c1bc3e8afa3a15a1091ae2dd4c823e816ed/internal/services/authorization/parse/role_definition.go#L52

The error from above is thrown, because "idParts" has a length of 1 only, and the check is valid for 0 elements only (<1). As it is 1, the else is causing trouble as idParts[1] is out of bounds. https://github.com/hashicorp/terraform-provider-azurerm/blob/1aef7c1bc3e8afa3a15a1091ae2dd4c823e816ed/internal/services/authorization/parse/role_definition.go#L63-L67

Maybe the code can be changed to something like this:

if len(idParts) < 2 {
   return nil, fmt.Errorf("failed to parse Role Definition ID from resource ID %q", input)
} else {
   roleDefinitionID.RoleID = idParts[1]
}

or

if len(idParts) < 1 {
   return nil, fmt.Errorf("failed to parse Role Definition ID from resource ID %q", input)
} else if len(idParts) < 2  {
   roleDefinitionID.RoleID = idParts[0]
} else {
   roleDefinitionID.RoleID = idParts[1]
}

or

if len(idParts) < 1 {
   return nil, fmt.Errorf("failed to parse Role Definition ID from resource ID %q", input)
} else if len(idParts) < 2  {
   return nil, fmt.Errorf("failed to parse scope from Role Definition ID from resource ID %q", input)
} else {
   roleDefinitionID.RoleID = idParts[1]
}

New or Affected Resource(s)/Data Source(s)

azurerm_role_definition

Potential Terraform Configuration

No change in tf config, but in the code.

References

No response

cdituri commented 2 months ago

Can confirm your report in the debugger @scratch85, screenshot attached. Good find 👍

image

However, if roleDefinitionID.RoleID was assignable to either idParts that would make the format of the role assignment resource in tfstate variable, i.e. inconsistent. Gut instinct says the RoleDefinitionId parser method should be made stricter; or the SchemaValidateFunc of the resource itself. Will take a pass at fixing and adding a test.