From 5499ce1d89e33805a40f8a01344c76069d988d78 Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Mon, 20 May 2024 16:55:18 +0100 Subject: [PATCH] bugfix: fix a regression creatnig PIM role assignments with no expiration --- .../pim_active_role_assignment_resource.go | 9 +- ...im_active_role_assignment_resource_test.go | 92 +++++---- .../pim_eligible_role_assignment_resource.go | 9 +- ..._eligible_role_assignment_resource_test.go | 176 ++++++++---------- 4 files changed, 145 insertions(+), 141 deletions(-) diff --git a/internal/services/authorization/pim_active_role_assignment_resource.go b/internal/services/authorization/pim_active_role_assignment_resource.go index a245ca783452..004df1a6ab4c 100644 --- a/internal/services/authorization/pim_active_role_assignment_resource.go +++ b/internal/services/authorization/pim_active_role_assignment_resource.go @@ -117,6 +117,7 @@ func (PimActiveRoleAssignmentResource) Arguments() map[string]*pluginsdk.Schema Type: pluginsdk.TypeList, MaxItems: 1, Optional: true, + Computed: true, ForceNew: true, Description: "The schedule details for this role assignment", Elem: &pluginsdk.Resource{ @@ -236,11 +237,13 @@ func (r PimActiveRoleAssignmentResource) Create() sdk.ResourceFunc { return metadata.ResourceRequiresImport(r.ResourceType(), id) } - var scheduleInfo *roleassignmentschedulerequests.RoleAssignmentScheduleRequestPropertiesScheduleInfo + scheduleInfo := &roleassignmentschedulerequests.RoleAssignmentScheduleRequestPropertiesScheduleInfo{ + Expiration: &roleassignmentschedulerequests.RoleAssignmentScheduleRequestPropertiesScheduleInfoExpiration{ + Type: pointer.To(roleassignmentschedulerequests.TypeNoExpiration), + }, + } if len(config.ScheduleInfo) > 0 { - scheduleInfo = &roleassignmentschedulerequests.RoleAssignmentScheduleRequestPropertiesScheduleInfo{} - if config.ScheduleInfo[0].StartDateTime != "" { scheduleInfo.StartDateTime = pointer.To(config.ScheduleInfo[0].StartDateTime) } diff --git a/internal/services/authorization/pim_active_role_assignment_resource_test.go b/internal/services/authorization/pim_active_role_assignment_resource_test.go index ca53fabf2784..81c3b26841d4 100644 --- a/internal/services/authorization/pim_active_role_assignment_resource_test.go +++ b/internal/services/authorization/pim_active_role_assignment_resource_test.go @@ -22,23 +22,21 @@ import ( type PimActiveRoleAssignmentResource struct{} -// TODO: update the management policy configuration so that it can have no expiration -// Depends on new resource - azurerm_role_management_policy - https://github.com/hashicorp/terraform-provider-azurerm/pull/20496 -// func TestAccPimActiveRoleAssignment_noExpiration(t *testing.T) { -// data := acceptance.BuildTestData(t, "azurerm_pim_active_role_assignment", "test") -// r := PimActiveRoleAssignmentResource{} - -// data.ResourceTest(t, r, []acceptance.TestStep{ -// { -// Config: r.noExpirationConfig(), -// Check: acceptance.ComposeTestCheckFunc( -// check.That(data.ResourceName).ExistsInAzure(r), -// check.That(data.ResourceName).Key("scope").Exists(), -// ), -// }, -// data.ImportStep("schedule.0.start_date_time"), -// }) -// } +func TestAccPimActiveRoleAssignment_noExpiration(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_pim_active_role_assignment", "test") + r := PimActiveRoleAssignmentResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.noExpirationConfig(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("scope").Exists(), + ), + }, + data.ImportStep("schedule.0.start_date_time"), + }) +} func TestAccPimActiveRoleAssignment_expirationByDurationHoursConfig(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_pim_active_role_assignment", "test") @@ -156,30 +154,52 @@ func (r PimActiveRoleAssignmentResource) Exists(ctx context.Context, client *cli return utils.Bool(false), nil } -// func (PimActiveRoleAssignmentResource) noExpirationConfig() string { -// return ` -// data "azurerm_subscription" "primary" {} +func (PimActiveRoleAssignmentResource) template(data acceptance.TestData) string { + return fmt.Sprintf(` +data "azuread_domains" "test" { + only_initial = true +} + +resource "azuread_user" "test" { + user_principal_name = "acctestUser-%[1]d1@${data.azuread_domains.test.domains.0.domain_name}" + display_name = "acctestUser-%[1]d1" + password = "p@$$Wd%[2]s" +} + +resource "azuread_group" "test" { + display_name = "acctest-group-%[1]d" + security_enabled = true +} + +`, data.RandomInteger, data.RandomString) +} -// data "azurerm_client_config" "test" {} +func (r PimActiveRoleAssignmentResource) noExpirationConfig(data acceptance.TestData) string { + return fmt.Sprintf(` +data "azurerm_subscription" "primary" {} -// data "azurerm_role_definition" "test" { -// name = "Monitoring Data Reader" -// } +data "azurerm_client_config" "test" {} -// resource "azurerm_pim_active_role_assignment" "test" { -// scope = data.azurerm_subscription.primary.id -// role_definition_id = "${data.azurerm_subscription.primary.id}${data.azurerm_role_definition.test.id}" -// principal_id = data.azurerm_client_config.test.object_id +data "azurerm_role_definition" "test" { + name = "Monitoring Data Reader" +} -// justification = "No Expiration" +%[1]s -// ticket { -// number = "1" -// system = "example ticket system" -// } -// } -// ` -// } +resource "azurerm_pim_active_role_assignment" "test" { + scope = data.azurerm_subscription.primary.id + role_definition_id = "${data.azurerm_subscription.primary.id}${data.azurerm_role_definition.test.id}" + principal_id = data.azurerm_client_config.test.object_id + + justification = "No Expiration" + + ticket { + number = "1" + system = "example ticket system" + } +} +`, r.template(data)) +} func (PimActiveRoleAssignmentResource) expirationByDurationHours(data acceptance.TestData) string { return fmt.Sprintf(` diff --git a/internal/services/authorization/pim_eligible_role_assignment_resource.go b/internal/services/authorization/pim_eligible_role_assignment_resource.go index 886d7b65695a..36d86292a1e0 100644 --- a/internal/services/authorization/pim_eligible_role_assignment_resource.go +++ b/internal/services/authorization/pim_eligible_role_assignment_resource.go @@ -118,6 +118,7 @@ func (PimEligibleRoleAssignmentResource) Arguments() map[string]*pluginsdk.Schem Type: pluginsdk.TypeList, MaxItems: 1, Optional: true, + Computed: true, ForceNew: true, Description: "The schedule details for this eligible role assignment", Elem: &pluginsdk.Resource{ @@ -237,11 +238,13 @@ func (r PimEligibleRoleAssignmentResource) Create() sdk.ResourceFunc { return metadata.ResourceRequiresImport(r.ResourceType(), id) } - var scheduleInfo *roleeligibilityschedulerequests.RoleEligibilityScheduleRequestPropertiesScheduleInfo + scheduleInfo := &roleeligibilityschedulerequests.RoleEligibilityScheduleRequestPropertiesScheduleInfo{ + Expiration: &roleeligibilityschedulerequests.RoleEligibilityScheduleRequestPropertiesScheduleInfoExpiration{ + Type: pointer.To(roleeligibilityschedulerequests.TypeNoExpiration), + }, + } if len(config.ScheduleInfo) > 0 { - scheduleInfo = &roleeligibilityschedulerequests.RoleEligibilityScheduleRequestPropertiesScheduleInfo{} - if config.ScheduleInfo[0].StartDateTime != "" { scheduleInfo.StartDateTime = pointer.To(config.ScheduleInfo[0].StartDateTime) } diff --git a/internal/services/authorization/pim_eligible_role_assignment_resource_test.go b/internal/services/authorization/pim_eligible_role_assignment_resource_test.go index 0464a78538b1..1fdef7c2389a 100644 --- a/internal/services/authorization/pim_eligible_role_assignment_resource_test.go +++ b/internal/services/authorization/pim_eligible_role_assignment_resource_test.go @@ -22,23 +22,21 @@ import ( type PimEligibleRoleAssignmentResource struct{} -// TODO: update the management policy configuration so that it can have no expiration -// Depends on new resource - azurerm_role_management_policy - https://github.com/hashicorp/terraform-provider-azurerm/pull/20496 -// func TestAccPimEligibleRoleAssignment_noExpiration(t *testing.T) { -// data := acceptance.BuildTestData(t, "azurerm_pim_eligible_role_assignment", "test") -// r := PimEligibleRoleAssignmentResource{} - -// data.ResourceTest(t, r, []acceptance.TestStep{ -// { -// Config: r.noExpirationConfig(data), -// Check: acceptance.ComposeTestCheckFunc( -// check.That(data.ResourceName).ExistsInAzure(r), -// check.That(data.ResourceName).Key("scope").Exists(), -// ), -// }, -// data.ImportStep("schedule.0.start_date_time"), -// }) -// } +func TestAccPimEligibleRoleAssignment_noExpiration(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_pim_eligible_role_assignment", "test") + r := PimEligibleRoleAssignmentResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.noExpirationConfig(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("scope").Exists(), + ), + }, + data.ImportStep("schedule.0.start_date_time"), + }) +} func TestAccPimEligibleRoleAssignment_expirationByDurationHoursConfig(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_pim_eligible_role_assignment", "test") @@ -155,7 +153,7 @@ func (r PimEligibleRoleAssignmentResource) Exists(ctx context.Context, client *c return utils.Bool(false), nil } -func aadUser(data acceptance.TestData) string { +func (PimEligibleRoleAssignmentResource) template(data acceptance.TestData) string { return fmt.Sprintf(` data "azuread_domains" "test" { only_initial = true @@ -175,54 +173,34 @@ resource "azuread_group" "test" { `, data.RandomInteger, data.RandomString) } -func aadGroup(data acceptance.TestData) string { +func (r PimEligibleRoleAssignmentResource) noExpirationConfig(data acceptance.TestData) string { return fmt.Sprintf(` -data "azuread_domains" "test" { - only_initial = true -} - -resource "azuread_user" "test" { - user_principal_name = "acctestUser-%[1]d1@${data.azuread_domains.test.domains.0.domain_name}" - display_name = "acctestUser-%[1]d1" - password = "p@$$Wd%[2]s" -} +data "azurerm_subscription" "primary" {} -resource "azuread_group" "test" { - display_name = "acctest-group-%[1]d" - security_enabled = true -} +data "azurerm_client_config" "test" {} -`, data.RandomInteger, data.RandomString) +data "azurerm_role_definition" "test" { + name = "Disk Backup Reader" } -// func (PimEligibleRoleAssignmentResource) noExpirationConfig(data acceptance.TestData) string { -// return fmt.Sprintf(` -// data "azurerm_subscription" "primary" {} - -// data "azurerm_client_config" "test" {} - -// data "azurerm_role_definition" "test" { -// name = "Disk Backup Reader" -// } - -// %s +%[1]s -// resource "azurerm_pim_eligible_role_assignment" "test" { -// scope = data.azurerm_subscription.primary.id -// role_definition_id = "${data.azurerm_subscription.primary.id}${data.azurerm_role_definition.test.id}" -// principal_id = azuread_user.test.object_id +resource "azurerm_pim_eligible_role_assignment" "test" { + scope = data.azurerm_subscription.primary.id + role_definition_id = "${data.azurerm_subscription.primary.id}${data.azurerm_role_definition.test.id}" + principal_id = azuread_user.test.object_id -// justification = "No Expiration" + justification = "No Expiration" -// ticket { -// number = "1" -// system = "example ticket system" -// } -// } -// `, aadUser(data)) -// } + ticket { + number = "1" + system = "example ticket system" + } +} +`, r.template(data)) +} -func (PimEligibleRoleAssignmentResource) expirationByDurationHours(data acceptance.TestData) string { +func (r PimEligibleRoleAssignmentResource) expirationByDurationHours(data acceptance.TestData) string { return fmt.Sprintf(` data "azurerm_subscription" "primary" {} @@ -260,10 +238,10 @@ resource "azurerm_pim_eligible_role_assignment" "test" { system = "example ticket system" } } -`, aadUser(data), data.RandomInteger, data.Locations.Primary) +`, r.template(data), data.RandomInteger, data.Locations.Primary) } -func (PimEligibleRoleAssignmentResource) expirationByDurationDays(data acceptance.TestData) string { +func (r PimEligibleRoleAssignmentResource) expirationByDurationDays(data acceptance.TestData) string { return fmt.Sprintf(` data "azurerm_subscription" "primary" {} @@ -307,112 +285,100 @@ resource "azurerm_pim_eligible_role_assignment" "test" { system = "example ticket system" } } -`, aadUser(data), data.RandomInteger, data.Locations.Primary) +`, r.template(data), data.RandomInteger, data.Locations.Primary) } -func (PimEligibleRoleAssignmentResource) importSetup(data acceptance.TestData) string { +func (r PimEligibleRoleAssignmentResource) expirationByDate(data acceptance.TestData) string { return fmt.Sprintf(` data "azurerm_subscription" "primary" {} data "azurerm_client_config" "test" {} data "azurerm_role_definition" "test" { - name = "AcrPull" + name = "Workbook Reader" } -%s +%[1]s resource "time_static" "test" {} +resource "time_offset" "test" { + offset_days = 7 +} resource "azurerm_pim_eligible_role_assignment" "test" { scope = data.azurerm_subscription.primary.id role_definition_id = "${data.azurerm_subscription.primary.id}${data.azurerm_role_definition.test.id}" - principal_id = azuread_user.test.object_id + + principal_id = azuread_group.test.object_id schedule { start_date_time = time_static.test.rfc3339 expiration { - duration_hours = 3 + end_date_time = time_offset.test.rfc3339 } } - justification = "Expiration Duration Set" + justification = "Expiration End Date Set" ticket { number = "1" system = "example ticket system" } } -`, aadUser(data)) +`, r.template(data)) } -func (r PimEligibleRoleAssignmentResource) requiresImport(data acceptance.TestData) string { - return fmt.Sprintf(` -%s - -resource "azurerm_pim_eligible_role_assignment" "import" { - scope = azurerm_pim_eligible_role_assignment.test.scope - role_definition_id = azurerm_pim_eligible_role_assignment.test.role_definition_id - principal_id = azurerm_pim_eligible_role_assignment.test.principal_id -} -`, r.importSetup(data)) -} - -func (PimEligibleRoleAssignmentResource) expirationByDate(data acceptance.TestData) string { +func (r PimEligibleRoleAssignmentResource) pending(data acceptance.TestData) string { return fmt.Sprintf(` data "azurerm_subscription" "primary" {} data "azurerm_client_config" "test" {} data "azurerm_role_definition" "test" { - name = "Workbook Reader" + name = "Key Vault Contributor" } -%s +%[1]s -resource "time_static" "test" {} resource "time_offset" "test" { - offset_days = 7 + offset_days = 1 } resource "azurerm_pim_eligible_role_assignment" "test" { scope = data.azurerm_subscription.primary.id role_definition_id = "${data.azurerm_subscription.primary.id}${data.azurerm_role_definition.test.id}" - - principal_id = azuread_group.test.object_id + principal_id = azuread_user.test.object_id schedule { - start_date_time = time_static.test.rfc3339 + start_date_time = time_offset.test.rfc3339 expiration { - end_date_time = time_offset.test.rfc3339 + duration_hours = 8 } } - justification = "Expiration End Date Set" + justification = "Expiration Duration Set" ticket { number = "1" system = "example ticket system" } } -`, aadGroup(data)) +`, r.template(data)) } -func (PimEligibleRoleAssignmentResource) pending(data acceptance.TestData) string { +func (r PimEligibleRoleAssignmentResource) importSetup(data acceptance.TestData) string { return fmt.Sprintf(` data "azurerm_subscription" "primary" {} data "azurerm_client_config" "test" {} data "azurerm_role_definition" "test" { - name = "Key Vault Contributor" + name = "AcrPull" } -%s +%[1]s -resource "time_offset" "test" { - offset_days = 1 -} +resource "time_static" "test" {} resource "azurerm_pim_eligible_role_assignment" "test" { scope = data.azurerm_subscription.primary.id @@ -420,9 +386,9 @@ resource "azurerm_pim_eligible_role_assignment" "test" { principal_id = azuread_user.test.object_id schedule { - start_date_time = time_offset.test.rfc3339 + start_date_time = time_static.test.rfc3339 expiration { - duration_hours = 8 + duration_hours = 3 } } @@ -433,5 +399,17 @@ resource "azurerm_pim_eligible_role_assignment" "test" { system = "example ticket system" } } -`, aadGroup(data)) +`, r.template(data)) +} + +func (r PimEligibleRoleAssignmentResource) requiresImport(data acceptance.TestData) string { + return fmt.Sprintf(` +%[1]s + +resource "azurerm_pim_eligible_role_assignment" "import" { + scope = azurerm_pim_eligible_role_assignment.test.scope + role_definition_id = azurerm_pim_eligible_role_assignment.test.role_definition_id + principal_id = azurerm_pim_eligible_role_assignment.test.principal_id +} +`, r.importSetup(data)) }