From aca7155c8cf5b9b509beb890422247736d0874a2 Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Sat, 5 Jun 2021 16:14:32 +0200 Subject: [PATCH 1/5] Create AccTest to catch issue --- .../role_assignment_resource_test.go | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/azurerm/internal/services/authorization/role_assignment_resource_test.go b/azurerm/internal/services/authorization/role_assignment_resource_test.go index 4e156c2da838..30d9a18ccd79 100644 --- a/azurerm/internal/services/authorization/role_assignment_resource_test.go +++ b/azurerm/internal/services/authorization/role_assignment_resource_test.go @@ -236,6 +236,23 @@ func TestAccRoleAssignment_condition(t *testing.T) { }) } +func TestAccRoleAssignment_resourceScoped(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") + id := uuid.New().String() + + r := RoleAssignmentResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.roleResourceScoped(data, id), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + ), + }, + data.ImportStep("skip_service_principal_aad_check"), + }) +} + func (r RoleAssignmentResource) Exists(ctx context.Context, client *clients.Client, state *pluginsdk.InstanceState) (*bool, error) { id, err := parse.RoleAssignmentID(state.ID) if err != nil { @@ -291,6 +308,42 @@ resource "azurerm_role_assignment" "test" { `, id) } +func (RoleAssignmentResource) roleResourceScoped(data acceptance.TestData, id string) string { + return fmt.Sprintf(` +provider "azurerm" { + features {} +} + +data "azurerm_client_config" "test" { +} + +resource "azurerm_resource_group" "test" { + name = "acctestRG-role-assigment-%d" + location = "%s" +} + +resource "azurerm_storage_account" "test" { + name = "unlikely23xst2acct%s" + resource_group_name = azurerm_resource_group.test.name + + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "LRS" + + tags = { + environment = "production" + } +} + +resource "azurerm_role_assignment" "test" { + name = "%s" + scope = azurerm_storage_account.test.id + role_definition_name = "Storage Account Contributor" + principal_id = data.azurerm_client_config.test.object_id +} +`, data.RandomInteger, data.Locations.Primary, data.RandomString, id) +} + func (RoleAssignmentResource) requiresImportConfig(id string) string { return fmt.Sprintf(` %s From 00f08802759f684251a59135170680ce34ff5383 Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Sat, 5 Jun 2021 17:54:45 +0200 Subject: [PATCH 2/5] First try to fix it --- azurerm/helpers/azure/resourceid.go | 18 +++++--- .../authorization/parse/role_assignment.go | 42 ++++++++++++----- .../parse/role_assignment_test.go | 46 ++++++++++++++++--- 3 files changed, 81 insertions(+), 25 deletions(-) diff --git a/azurerm/helpers/azure/resourceid.go b/azurerm/helpers/azure/resourceid.go index b000ef94d6f2..1f86b5a2860d 100644 --- a/azurerm/helpers/azure/resourceid.go +++ b/azurerm/helpers/azure/resourceid.go @@ -40,6 +40,7 @@ func ParseAzureResourceID(id string) (*ResourceID, error) { } var subscriptionID string + var provider string // Put the constituent key-value pairs into a map componentMap := make(map[string]string, len(components)/2) @@ -52,11 +53,16 @@ func ParseAzureResourceID(id string) (*ResourceID, error) { return nil, fmt.Errorf("Key/Value cannot be empty strings. Key: '%s', Value: '%s'", key, value) } - // Catch the subscriptionID before it can be overwritten by another "subscriptions" - // value in the ID which is the case for the Service Bus subscription resource - if key == "subscriptions" && subscriptionID == "" { + switch { + case key == "subscriptions" && subscriptionID == "": + // Catch the subscriptionID before it can be overwritten by another "subscriptions" + // value in the ID which is the case for the Service Bus subscription resource subscriptionID = value - } else { + case key == "providers" && provider == "": + // Catch the provider before it can be overwritten by another "providers" + // value in the ID which can be the case for the Role Assignment resource + provider = value + default: componentMap[key] = value } } @@ -82,10 +88,8 @@ func ParseAzureResourceID(id string) (*ResourceID, error) { delete(componentMap, "resourcegroups") } - // It is OK not to have a provider in the case of a resource group - if provider, ok := componentMap["providers"]; ok { + if provider != "" { idObj.Provider = provider - delete(componentMap, "providers") } return idObj, nil diff --git a/azurerm/internal/services/authorization/parse/role_assignment.go b/azurerm/internal/services/authorization/parse/role_assignment.go index 5c4a8e45365d..a299803e3cd0 100644 --- a/azurerm/internal/services/authorization/parse/role_assignment.go +++ b/azurerm/internal/services/authorization/parse/role_assignment.go @@ -8,14 +8,16 @@ import ( ) type RoleAssignmentId struct { - SubscriptionID string - ResourceGroup string - ManagementGroup string - Name string - TenantId string + SubscriptionID string + ResourceGroup string + ManagementGroup string + ResourceScope string + ResourceProvider string + Name string + TenantId string } -func NewRoleAssignmentID(subscriptionId, resourceGroup, managementGroup, name, tenantId string) (*RoleAssignmentId, error) { +func NewRoleAssignmentID(subscriptionId, resourceGroup, resourceProvider, resourceScope, managementGroup, name, tenantId string) (*RoleAssignmentId, error) { if subscriptionId == "" && resourceGroup == "" && managementGroup == "" { return nil, fmt.Errorf("one of subscriptionId, resourceGroup, or managementGroup must be provided") } @@ -33,17 +35,24 @@ func NewRoleAssignmentID(subscriptionId, resourceGroup, managementGroup, name, t } return &RoleAssignmentId{ - SubscriptionID: subscriptionId, - ResourceGroup: resourceGroup, - ManagementGroup: managementGroup, - Name: name, - TenantId: tenantId, + SubscriptionID: subscriptionId, + ResourceGroup: resourceGroup, + ResourceProvider: resourceProvider, + ResourceScope: resourceScope, + ManagementGroup: managementGroup, + Name: name, + TenantId: tenantId, }, nil } // in general case, the id format does not change // for cross tenant scenario, add the tenantId info func (id RoleAssignmentId) AzureResourceID() string { + if id.ResourceScope != "" { + fmtString := "/subscriptions/%s/resourceGroups/%s/providers/%s/%s/providers/Microsoft.Authorization/roleAssignments/%s" + return fmt.Sprintf(fmtString, id.SubscriptionID, id.ResourceGroup, id.ResourceProvider, id.ResourceScope, id.Name) + } + if id.ManagementGroup != "" { fmtString := "/providers/Microsoft.Management/managementGroups/%s/providers/Microsoft.Authorization/roleAssignments/%s" return fmt.Sprintf(fmtString, id.ManagementGroup, id.Name) @@ -90,6 +99,17 @@ func RoleAssignmentID(input string) (*RoleAssignmentId, error) { } roleAssignmentId.SubscriptionID = id.SubscriptionID roleAssignmentId.ResourceGroup = id.ResourceGroup + if id.Provider != "Microsoft.Authorization" && id.Provider != "" { + roleAssignmentId.ResourceProvider = id.Provider + // logic to save resource scope + result := strings.Split(input, "/") + for k, v := range result { + if v == id.Provider && len(result) >= k+1 { + roleAssignmentId.ResourceScope = fmt.Sprintf("%s/%s", result[k+1], result[k+2]) + } + } + } + if roleAssignmentId.Name, err = id.PopSegment("roleAssignments"); err != nil { return nil, err } diff --git a/azurerm/internal/services/authorization/parse/role_assignment_test.go b/azurerm/internal/services/authorization/parse/role_assignment_test.go index e9e2c95f5a05..1c3c76dde165 100644 --- a/azurerm/internal/services/authorization/parse/role_assignment_test.go +++ b/azurerm/internal/services/authorization/parse/role_assignment_test.go @@ -10,16 +10,19 @@ var _ resourceid.Formatter = RoleAssignmentId{} func TestRoleAssignmentIDFormatter(t *testing.T) { testData := []struct { - SubscriptionId string - ResourceGroup string - ManagementGroup string - Name string - TenantId string - Expected string + SubscriptionId string + ResourceGroup string + ResourceProvider string + ResourceScope string + ManagementGroup string + Name string + TenantId string + Expected string }{ { SubscriptionId: "", ResourceGroup: "", + ResourceScope: "", ManagementGroup: "", Name: "23456781-2349-8764-5631-234567890121", TenantId: "", @@ -27,6 +30,7 @@ func TestRoleAssignmentIDFormatter(t *testing.T) { { SubscriptionId: "12345678-1234-9876-4563-123456789012", ResourceGroup: "group1", + ResourceScope: "", ManagementGroup: "managementGroup1", Name: "23456781-2349-8764-5631-234567890121", TenantId: "", @@ -34,6 +38,7 @@ func TestRoleAssignmentIDFormatter(t *testing.T) { { SubscriptionId: "12345678-1234-9876-4563-123456789012", ResourceGroup: "", + ResourceScope: "", ManagementGroup: "managementGroup1", Name: "23456781-2349-8764-5631-234567890121", TenantId: "", @@ -41,6 +46,7 @@ func TestRoleAssignmentIDFormatter(t *testing.T) { { SubscriptionId: "12345678-1234-9876-4563-123456789012", ResourceGroup: "", + ResourceScope: "", ManagementGroup: "", Name: "23456781-2349-8764-5631-234567890121", TenantId: "", @@ -49,6 +55,7 @@ func TestRoleAssignmentIDFormatter(t *testing.T) { { SubscriptionId: "12345678-1234-9876-4563-123456789012", ResourceGroup: "group1", + ResourceScope: "", ManagementGroup: "", Name: "23456781-2349-8764-5631-234567890121", TenantId: "", @@ -57,6 +64,7 @@ func TestRoleAssignmentIDFormatter(t *testing.T) { { SubscriptionId: "", ResourceGroup: "", + ResourceScope: "", ManagementGroup: "12345678-1234-9876-4563-123456789012", Name: "23456781-2349-8764-5631-234567890121", TenantId: "", @@ -65,15 +73,26 @@ func TestRoleAssignmentIDFormatter(t *testing.T) { { SubscriptionId: "", ResourceGroup: "", + ResourceScope: "", ManagementGroup: "12345678-1234-9876-4563-123456789012", Name: "23456781-2349-8764-5631-234567890121", TenantId: "34567812-3456-7653-6742-345678901234", Expected: "/providers/Microsoft.Management/managementGroups/12345678-1234-9876-4563-123456789012/providers/Microsoft.Authorization/roleAssignments/23456781-2349-8764-5631-234567890121|34567812-3456-7653-6742-345678901234", }, + { + SubscriptionId: "12345678-1234-9876-4563-123456789012", + ResourceGroup: "group1", + ResourceProvider: "Microsoft.Storage", + ResourceScope: "storageAccounts/nameStorageAccount", + ManagementGroup: "", + Name: "23456781-2349-8764-5631-234567890121", + TenantId: "34567812-3456-7653-6742-345678901234", + Expected: "/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/group1/providers/Microsoft.Storage/storageAccounts/nameStorageAccount/providers/Microsoft.Authorization/roleAssignments/23456781-2349-8764-5631-234567890121|34567812-3456-7653-6742-345678901234", + }, } for _, v := range testData { t.Logf("testing %+v", v) - actual, err := NewRoleAssignmentID(v.SubscriptionId, v.ResourceGroup, v.ManagementGroup, v.Name, v.TenantId) + actual, err := NewRoleAssignmentID(v.SubscriptionId, v.ResourceGroup, v.ResourceProvider, v.ResourceScope, v.ManagementGroup, v.Name, v.TenantId) if err != nil { if v.Expected == "" { continue @@ -140,6 +159,7 @@ func TestRoleAssignmentID(t *testing.T) { Expected: &RoleAssignmentId{ SubscriptionID: "12345678-1234-9876-4563-123456789012", ResourceGroup: "", + ResourceScope: "", ManagementGroup: "", Name: "23456781-2349-8764-5631-234567890121", }, @@ -176,6 +196,18 @@ func TestRoleAssignmentID(t *testing.T) { TenantId: "34567812-3456-7653-6742-345678901234", }, }, + { + Input: "/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/group1/providers/Microsoft.Storage/storageAccounts/nameStorageAccount/providers/Microsoft.Authorization/roleAssignments/23456781-2349-8764-5631-234567890121|34567812-3456-7653-6742-345678901234", + Expected: &RoleAssignmentId{ + SubscriptionID: "12345678-1234-9876-4563-123456789012", + ResourceGroup: "group1", + ResourceProvider: "Microsoft.Storage", + ResourceScope: "storageAccounts/nameStorageAccount", + ManagementGroup: "", + Name: "23456781-2349-8764-5631-234567890121", + TenantId: "34567812-3456-7653-6742-345678901234", + }, + }, } for _, v := range testData { From 2bedf3aa13446fdeb641236499cf75a2518093e4 Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Sun, 6 Jun 2021 11:48:50 +0200 Subject: [PATCH 3/5] Fix unittests --- azurerm/helpers/azure/resourceid.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/azurerm/helpers/azure/resourceid.go b/azurerm/helpers/azure/resourceid.go index 1f86b5a2860d..7edd6f0aaffd 100644 --- a/azurerm/helpers/azure/resourceid.go +++ b/azurerm/helpers/azure/resourceid.go @@ -11,10 +11,11 @@ import ( // level fields, and other key-value pairs available via a map in the // Path field. type ResourceID struct { - SubscriptionID string - ResourceGroup string - Provider string - Path map[string]string + SubscriptionID string + ResourceGroup string + Provider string + SecondaryProvider string + Path map[string]string } // ParseAzureResourceID converts a long-form Azure Resource Manager ID @@ -92,6 +93,11 @@ func ParseAzureResourceID(id string) (*ResourceID, error) { idObj.Provider = provider } + if secondaryProvider := componentMap["providers"]; secondaryProvider != "" { + idObj.SecondaryProvider = secondaryProvider + delete(componentMap, "providers") + } + return idObj, nil } From 8960d472796dd62bd06fd1c6196095e452da1b4d Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Sun, 6 Jun 2021 12:27:41 +0200 Subject: [PATCH 4/5] Extra unit test on ResourceIDs --- azurerm/helpers/azure/resourceid_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/azurerm/helpers/azure/resourceid_test.go b/azurerm/helpers/azure/resourceid_test.go index aa69bc8d8174..6d85a5fd4ac6 100644 --- a/azurerm/helpers/azure/resourceid_test.go +++ b/azurerm/helpers/azure/resourceid_test.go @@ -147,6 +147,20 @@ func TestParseAzureResourceID(t *testing.T) { }, false, }, + { + "/subscriptions/11111111-1111-1111-1111-111111111111/resourceGroups/example-resources/providers/Microsoft.Storage/storageAccounts/nameStorageAccount/providers/Microsoft.Authorization/roleAssignments/22222222-2222-2222-2222-222222222222", + &azure.ResourceID{ + SubscriptionID: "11111111-1111-1111-1111-111111111111", + ResourceGroup: "example-resources", + Provider: "Microsoft.Storage", + SecondaryProvider: "Microsoft.Authorization", + Path: map[string]string{ + "storageAccounts": "nameStorageAccount", + "roleAssignments": "22222222-2222-2222-2222-222222222222", + }, + }, + false, + }, { // missing resource group "/subscriptions/11111111-1111-1111-1111-111111111111/providers/Microsoft.ApiManagement/service/service1/subscriptions/22222222-2222-2222-2222-222222222222", From a547954618b43440771993831b3cece69143d194 Mon Sep 17 00:00:00 2001 From: Aris van Ommeren Date: Mon, 7 Jun 2021 08:42:11 +0200 Subject: [PATCH 5/5] Fix comments --- .../authorization/parse/role_assignment.go | 8 +++--- .../parse/role_assignment_test.go | 26 ++++++++++++++++--- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/azurerm/internal/services/authorization/parse/role_assignment.go b/azurerm/internal/services/authorization/parse/role_assignment.go index a299803e3cd0..c60de5e5ed26 100644 --- a/azurerm/internal/services/authorization/parse/role_assignment.go +++ b/azurerm/internal/services/authorization/parse/role_assignment.go @@ -102,11 +102,9 @@ func RoleAssignmentID(input string) (*RoleAssignmentId, error) { if id.Provider != "Microsoft.Authorization" && id.Provider != "" { roleAssignmentId.ResourceProvider = id.Provider // logic to save resource scope - result := strings.Split(input, "/") - for k, v := range result { - if v == id.Provider && len(result) >= k+1 { - roleAssignmentId.ResourceScope = fmt.Sprintf("%s/%s", result[k+1], result[k+2]) - } + result := strings.Split(input, "/providers/") + if len(result) == 3 { + roleAssignmentId.ResourceScope = strings.TrimPrefix(result[1], fmt.Sprintf("%s/", id.Provider)) } } diff --git a/azurerm/internal/services/authorization/parse/role_assignment_test.go b/azurerm/internal/services/authorization/parse/role_assignment_test.go index 1c3c76dde165..0917eeb90840 100644 --- a/azurerm/internal/services/authorization/parse/role_assignment_test.go +++ b/azurerm/internal/services/authorization/parse/role_assignment_test.go @@ -208,6 +208,18 @@ func TestRoleAssignmentID(t *testing.T) { TenantId: "34567812-3456-7653-6742-345678901234", }, }, + { + Input: "/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/group1/providers/Microsoft.AppPlatform/Spring/spring1/apps/app1/providers/Microsoft.Authorization/roleAssignments/23456781-2349-8764-5631-234567890121|34567812-3456-7653-6742-345678901234", + Expected: &RoleAssignmentId{ + SubscriptionID: "12345678-1234-9876-4563-123456789012", + ResourceGroup: "group1", + ResourceProvider: "Microsoft.AppPlatform", + ResourceScope: "Spring/spring1/apps/app1", + ManagementGroup: "", + Name: "23456781-2349-8764-5631-234567890121", + TenantId: "34567812-3456-7653-6742-345678901234", + }, + }, } for _, v := range testData { @@ -231,15 +243,23 @@ func TestRoleAssignmentID(t *testing.T) { } if actual.SubscriptionID != v.Expected.SubscriptionID { - t.Fatalf("Expected %q but got %q for Role Assignment Name", v.Expected.SubscriptionID, actual.SubscriptionID) + t.Fatalf("Expected %q but got %q for Role Assignment Subscription ID", v.Expected.SubscriptionID, actual.SubscriptionID) } if actual.ResourceGroup != v.Expected.ResourceGroup { - t.Fatalf("Expected %q but got %q for Role Assignment Name", v.Expected.ResourceGroup, actual.ResourceGroup) + t.Fatalf("Expected %q but got %q for Role Assignment Resource Group", v.Expected.ResourceGroup, actual.ResourceGroup) + } + + if actual.ResourceProvider != v.Expected.ResourceProvider { + t.Fatalf("Expected %q but got %q for Role Assignment Resource Provider", v.Expected.ResourceProvider, actual.ResourceProvider) + } + + if actual.ResourceScope != v.Expected.ResourceScope { + t.Fatalf("Expected %q but got %q for Role Assignment Resource Scope", v.Expected.ResourceScope, actual.ResourceScope) } if actual.ManagementGroup != v.Expected.ManagementGroup { - t.Fatalf("Expected %q but got %q for Role Assignment Name", v.Expected.ManagementGroup, actual.ManagementGroup) + t.Fatalf("Expected %q but got %q for Role Assignment Management Group", v.Expected.ManagementGroup, actual.ManagementGroup) } } }