From 59a2543c15244f7dad19d5bb6d9b557e252b49be Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Tue, 2 Feb 2021 11:33:22 +0100 Subject: [PATCH 1/3] r/role_assignment: adding validation for `scope` This commit introduces validation to the `scope` field, validating that it's either a Management Group ID, Resource Group ID, Subscription ID or otherwise a Resource ID - to workaround the API usability issues identified in #9569. This isn't perfect, but the error messages coming back from the API are particularly unhelpful to users unfamiliar with how the API works. --- .teamcity/components/settings.kt | 3 +++ .../services/authorization/role_assignment_resource.go | 10 ++++++++++ 2 files changed, 13 insertions(+) diff --git a/.teamcity/components/settings.kt b/.teamcity/components/settings.kt index 47e9bb48a914..04f53468c3c0 100644 --- a/.teamcity/components/settings.kt +++ b/.teamcity/components/settings.kt @@ -22,6 +22,9 @@ var serviceTestConfigurationOverrides = mapOf( // Spring Cloud only allows a max of 10 provisioned "appplatform" to testConfiguration(5, defaultStartHour), + // these tests all conflict with one another + "authorization": to testConfiguration(1, defaultStartHour), + // The AKS API has a low rate limit "containers" to testConfiguration(5, defaultStartHour), diff --git a/azurerm/internal/services/authorization/role_assignment_resource.go b/azurerm/internal/services/authorization/role_assignment_resource.go index 812302f0976a..d039b675b6bc 100644 --- a/azurerm/internal/services/authorization/role_assignment_resource.go +++ b/azurerm/internal/services/authorization/role_assignment_resource.go @@ -12,8 +12,12 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/helper/validation" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/tf" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/clients" + managementGroupValidate "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/managementgroup/validate" + resourceValidate "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/resource/validate" + subscriptionValidate "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/subscription/validate" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/tf/suppress" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/timeouts" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils" @@ -48,6 +52,12 @@ func resourceArmRoleAssignment() *schema.Resource { Type: schema.TypeString, Required: true, ForceNew: true, + ValidateFunc: validation.Any( + managementGroupValidate.ManagementGroupID, + subscriptionValidate.SubscriptionID, + resourceValidate.ResourceGroupID, + azure.ValidateResourceID, + ), }, "role_definition_id": { From c364c6d557838e0ced40f2088d281260784efe61 Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Tue, 2 Feb 2021 17:03:33 +0100 Subject: [PATCH 2/3] r/role_assignment: switching the tests out to be regular functions We don't need to run these in sequence since TC is limited to 1 --- .../role_assignment_resource_test.go | 51 ++++--------------- 1 file changed, 10 insertions(+), 41 deletions(-) diff --git a/azurerm/internal/services/authorization/role_assignment_resource_test.go b/azurerm/internal/services/authorization/role_assignment_resource_test.go index a13b8b753d6d..e80f0843433a 100644 --- a/azurerm/internal/services/authorization/role_assignment_resource_test.go +++ b/azurerm/internal/services/authorization/role_assignment_resource_test.go @@ -17,38 +17,7 @@ import ( type RoleAssignmentResource struct{} -func TestAccRoleAssignment(t *testing.T) { - // NOTE: this is a combined test rather than separate split out tests due to - // Azure only being happy about provisioning a couple at a time - acceptance.RunTestsInSequence(t, map[string]map[string]func(t *testing.T){ - "basic": { - "roleName": testAccRoleAssignment_roleName, - "custom": testAccRoleAssignment_custom, - }, - "basic_empty_name": { - "emptyName": testAccRoleAssignment_emptyName, - }, - "built_in": { - "builtin": testAccRoleAssignment_builtin, - }, - "data_actions": { - "dataActions": testAccRoleAssignment_dataActions, - }, - "requires_import": { - "requiresImport": testAccRoleAssignment_requiresImport, - }, - "assignment": { - "sp": testAccActiveDirectoryServicePrincipal_servicePrincipal, - "spType": testAccActiveDirectoryServicePrincipal_servicePrincipalWithType, - "group": testAccActiveDirectoryServicePrincipal_group, - }, - "management": { - "assign": testAccRoleAssignment_managementGroup, - }, - }) -} - -func testAccRoleAssignment_emptyName(t *testing.T) { +func TestAccRoleAssignment_emptyName(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") r := RoleAssignmentResource{} @@ -64,7 +33,7 @@ func testAccRoleAssignment_emptyName(t *testing.T) { }) } -func testAccRoleAssignment_roleName(t *testing.T) { +func TestAccRoleAssignment_roleName(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") id := uuid.New().String() @@ -83,7 +52,7 @@ func testAccRoleAssignment_roleName(t *testing.T) { }) } -func testAccRoleAssignment_requiresImport(t *testing.T) { +func TestAccRoleAssignment_requiresImport(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") id := uuid.New().String() @@ -105,7 +74,7 @@ func testAccRoleAssignment_requiresImport(t *testing.T) { }) } -func testAccRoleAssignment_dataActions(t *testing.T) { +func TestAccRoleAssignment_dataActions(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") id := uuid.New().String() @@ -123,7 +92,7 @@ func testAccRoleAssignment_dataActions(t *testing.T) { }) } -func testAccRoleAssignment_builtin(t *testing.T) { +func TestAccRoleAssignment_builtin(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") id := uuid.New().String() @@ -140,7 +109,7 @@ func testAccRoleAssignment_builtin(t *testing.T) { }) } -func testAccRoleAssignment_custom(t *testing.T) { +func TestAccRoleAssignment_custom(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") roleDefinitionId := uuid.New().String() roleAssignmentId := uuid.New().String() @@ -159,7 +128,7 @@ func testAccRoleAssignment_custom(t *testing.T) { }) } -func testAccActiveDirectoryServicePrincipal_servicePrincipal(t *testing.T) { +func TestAccRoleAssignment_ServicePrincipal(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") ri := acceptance.RandTimeInt() id := uuid.New().String() @@ -177,7 +146,7 @@ func testAccActiveDirectoryServicePrincipal_servicePrincipal(t *testing.T) { }) } -func testAccActiveDirectoryServicePrincipal_servicePrincipalWithType(t *testing.T) { +func TestAccRoleAssignment_ServicePrincipalWithType(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") ri := acceptance.RandTimeInt() id := uuid.New().String() @@ -194,7 +163,7 @@ func testAccActiveDirectoryServicePrincipal_servicePrincipalWithType(t *testing. }) } -func testAccActiveDirectoryServicePrincipal_group(t *testing.T) { +func TestAccRoleAssignment_ServicePrincipalGroup(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") ri := acceptance.RandTimeInt() id := uuid.New().String() @@ -212,7 +181,7 @@ func testAccActiveDirectoryServicePrincipal_group(t *testing.T) { } // TODO - "real" management group with appropriate required for testing -func testAccRoleAssignment_managementGroup(t *testing.T) { +func TestAccRoleAssignment_managementGroup(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test") groupId := uuid.New().String() From 8885f54902c1326664116ed918b7fc86ad3a2447 Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Wed, 3 Feb 2021 14:32:31 +0000 Subject: [PATCH 3/3] Typo in .teamcity/components/settings.kt --- .teamcity/components/settings.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.teamcity/components/settings.kt b/.teamcity/components/settings.kt index 04f53468c3c0..deafbc5cbc50 100644 --- a/.teamcity/components/settings.kt +++ b/.teamcity/components/settings.kt @@ -23,7 +23,7 @@ var serviceTestConfigurationOverrides = mapOf( "appplatform" to testConfiguration(5, defaultStartHour), // these tests all conflict with one another - "authorization": to testConfiguration(1, defaultStartHour), + "authorization" to testConfiguration(1, defaultStartHour), // The AKS API has a low rate limit "containers" to testConfiguration(5, defaultStartHour),