From 49587c9884c6b5e492c4c96de9267f131bbdba1f Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Mon, 27 Feb 2023 21:16:22 +0200 Subject: [PATCH] Workaround: auto-retry group creation without calling principal as owner, partial workaround for https://github.com/microsoftgraph/msgraph-metadata/issues/92 --- internal/services/groups/group_resource.go | 74 +++++++++++++++++-- .../services/groups/group_resource_test.go | 27 +++++++ 2 files changed, 94 insertions(+), 7 deletions(-) diff --git a/internal/services/groups/group_resource.go b/internal/services/groups/group_resource.go index f8b8e1149b..ddd9fd1614 100644 --- a/internal/services/groups/group_resource.go +++ b/internal/services/groups/group_resource.go @@ -6,6 +6,7 @@ import ( "fmt" "log" "net/http" + "regexp" "strings" "time" @@ -22,7 +23,10 @@ import ( "github.com/manicminer/hamilton/msgraph" ) -const groupResourceName = "azuread_group" +const ( + groupResourceName = "azuread_group" + groupDuplicateValueError = "Request contains a property with duplicate values" +) func groupResource() *schema.Resource { return &schema.Resource{ @@ -582,6 +586,7 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter properties.Owners = &ownersFirst20 var group *msgraph.Group + var status int var err error if v, ok := d.GetOk("administrative_unit_ids"); ok { @@ -589,21 +594,76 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter for i, administrativeUnitId := range administrativeUnitIds { // Create the group in the first administrative unit, as this requires fewer permissions than creating it at tenant level if i == 0 { - group, _, err = administrativeUnitsClient.CreateGroup(ctx, administrativeUnitId, &properties) + group, status, err = administrativeUnitsClient.CreateGroup(ctx, administrativeUnitId, &properties) if err != nil { - return tf.ErrorDiagF(err, "Creating group in administrative unit with ID %q, %q", administrativeUnitId, displayName) + if status == http.StatusBadRequest && regexp.MustCompile(groupDuplicateValueError).MatchString(err.Error()) { + // Retry the request, without the calling principal as owner + newOwners := make(msgraph.Owners, 0) + for _, o := range *properties.Owners { + if id := o.ID(); id != nil && *id != callerId { + newOwners = append(newOwners, o) + } + } + + // No point in retrying if the caller wasn't specified + if len(newOwners) == len(*properties.Owners) { + log.Printf("[DEBUG] Not retrying group creation for %q within AU %q as owner was not specified", displayName, administrativeUnitId) + return tf.ErrorDiagF(err, "Creating group in administrative unit with ID %q, %q", administrativeUnitId, displayName) + } + + // If the API is refusing the calling principal as owner, it will typically automatically append the caller in the background, + // and subsequent GETs for the group will include the calling principal as owner, as if it were specified when creating. + log.Printf("[DEBUG] Retrying group creation for %q within AU %q without calling principal as owner", displayName, administrativeUnitId) + properties.Owners = &newOwners + group, status, err = administrativeUnitsClient.CreateGroup(ctx, administrativeUnitId, &properties) + if err != nil { + return tf.ErrorDiagF(err, "Creating group in administrative unit with ID %q, %q", administrativeUnitId, displayName) + } + } else { + return tf.ErrorDiagF(err, "Creating group in administrative unit with ID %q, %q", administrativeUnitId, displayName) + } } } else { - err := addGroupToAdministrativeUnit(ctx, administrativeUnitsClient, administrativeUnitId, group) + err = addGroupToAdministrativeUnit(ctx, administrativeUnitsClient, administrativeUnitId, group) if err != nil { - return tf.ErrorDiagF(err, "Could not add group %q to administrative unit with object ID: %q", *group.ID(), administrativeUnitId) + return tf.ErrorDiagF(err, "Adding group %q to administrative unit with object ID: %q", *group.ID(), administrativeUnitId) } } } } else { - group, _, err = client.Create(ctx, properties) + group, status, err = client.Create(ctx, properties) if err != nil { - return tf.ErrorDiagF(err, "Creating group %q", displayName) + if status == http.StatusBadRequest && regexp.MustCompile(groupDuplicateValueError).MatchString(err.Error()) { + // Retry the request, without the calling principal as owner + newOwners := make(msgraph.Owners, 0) + for _, o := range *properties.Owners { + if id := o.ID(); id != nil && *id != callerId { + newOwners = append(newOwners, o) + } + } + + // No point in retrying if the caller wasn't specified + if len(newOwners) == len(*properties.Owners) { + log.Printf("[DEBUG] Not retrying group creation for %q as owner was not specified", displayName) + return tf.ErrorDiagF(err, "Creating group %q", displayName) + } + + // If the API is refusing the calling principal as owner, it will typically automatically append the caller in the background, + // and subsequent GETs for the group will include the calling principal as owner, as if it were specified when creating. + log.Printf("[DEBUG] Retrying group creation for %q without calling principal as owner", displayName) + if len(newOwners) == 0 { + properties.Owners = nil + } else { + properties.Owners = &newOwners + } + + group, status, err = client.Create(ctx, properties) + if err != nil { + return tf.ErrorDiagF(err, "Creating group %q", displayName) + } + } else { + return tf.ErrorDiagF(err, "Creating group %q", displayName) + } } } diff --git a/internal/services/groups/group_resource_test.go b/internal/services/groups/group_resource_test.go index f948cbd0a6..a2fc9e4f80 100644 --- a/internal/services/groups/group_resource_test.go +++ b/internal/services/groups/group_resource_test.go @@ -159,6 +159,21 @@ func TestAccGroup_dynamicMembership(t *testing.T) { }) } +func TestAccGroup_callerOwner(t *testing.T) { + data := acceptance.BuildTestData(t, "azuread_group", "test") + r := GroupResource{} + + data.ResourceTest(t, r, []resource.TestStep{ + { + Config: r.withCallerAsOwner(data), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + ), + }, + data.ImportStep(), + }) +} + func TestAccGroup_owners(t *testing.T) { data := acceptance.BuildTestData(t, "azuread_group", "test") r := GroupResource{} @@ -714,6 +729,18 @@ resource "azuread_group" "test" { `, r.templateThreeUsers(data), data.RandomInteger) } +func (GroupResource) withCallerAsOwner(data acceptance.TestData) string { + return fmt.Sprintf(` +data "azuread_client_config" "test" {} + +resource "azuread_group" "test" { + display_name = "acctestGroup-%[1]d" + security_enabled = true + owners = [data.azuread_client_config.test.object_id] +} +`, data.RandomInteger) +} + func (GroupResource) withServicePrincipalOwner(data acceptance.TestData) string { return fmt.Sprintf(` resource "azuread_application" "test" {