From 07a3b3a79d666c34c1be3931558d61e966d2500b Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Wed, 19 Apr 2023 12:45:56 +0100 Subject: [PATCH 1/4] Fix regression: writeback configuration should be optional+computed --- internal/services/groups/group_resource.go | 7 +------ internal/services/groups/group_resource_test.go | 7 +++++++ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/services/groups/group_resource.go b/internal/services/groups/group_resource.go index ce712702e4..ae9bc2892b 100644 --- a/internal/services/groups/group_resource.go +++ b/internal/services/groups/group_resource.go @@ -183,6 +183,7 @@ func groupResource() *schema.Resource { Description: "Indicates the target on-premise group type the group will be written back as", Type: schema.TypeString, Optional: true, + Computed: true, ValidateFunc: validation.StringInSlice([]string{ msgraph.UniversalDistributionGroup, msgraph.UniversalSecurityGroup, @@ -363,12 +364,6 @@ func groupResourceCustomizeDiff(ctx context.Context, diff *schema.ResourceDiff, } } - // onPremisesGroupType can't be unset - oldOnPremisesGroupType, newOnPremisesGroupType := diff.GetChange("onpremises_group_type") - if oldOnPremisesGroupType.(string) != "" && newOnPremisesGroupType == "" { - diff.ForceNew("onpremises_group_type") - } - mailEnabled := diff.Get("mail_enabled").(bool) securityEnabled := diff.Get("security_enabled").(bool) groupTypes := make([]msgraph.GroupType, 0) diff --git a/internal/services/groups/group_resource_test.go b/internal/services/groups/group_resource_test.go index c1c709c14d..1b0d2232a3 100644 --- a/internal/services/groups/group_resource_test.go +++ b/internal/services/groups/group_resource_test.go @@ -516,6 +516,13 @@ func TestAccGroup_writebackUpdate(t *testing.T) { ), }, data.ImportStep(), + { + Config: r.basic(data), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + ), + }, + data.ImportStep(), }) } From 7f2e2ddb228d4084474b93a2aded8bd35f08111e Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Wed, 19 Apr 2023 23:24:35 +0100 Subject: [PATCH 2/4] Workaround for https://github.com/microsoftgraph/msgraph-metadata/issues/333 --- internal/services/groups/group_data_source.go | 25 +++--- internal/services/groups/group_resource.go | 76 +++++++++---------- internal/services/groups/groups.go | 9 ++- 3 files changed, 59 insertions(+), 51 deletions(-) diff --git a/internal/services/groups/group_data_source.go b/internal/services/groups/group_data_source.go index 98d264eeef..c034792313 100644 --- a/internal/services/groups/group_data_source.go +++ b/internal/services/groups/group_data_source.go @@ -370,18 +370,19 @@ func groupDataSourceRead(ctx context.Context, d *schema.ResourceData, meta inter if err != nil { return tf.ErrorDiagF(err, "Could not retrieve group with object ID %q", d.Id()) } - - if groupExtra != nil && groupExtra.AllowExternalSenders != nil { - allowExternalSenders = *groupExtra.AllowExternalSenders - } - if groupExtra != nil && groupExtra.AutoSubscribeNewMembers != nil { - autoSubscribeNewMembers = *groupExtra.AutoSubscribeNewMembers - } - if groupExtra != nil && groupExtra.HideFromAddressLists != nil { - hideFromAddressLists = *groupExtra.HideFromAddressLists - } - if groupExtra != nil && groupExtra.HideFromOutlookClients != nil { - hideFromOutlookClients = *groupExtra.HideFromOutlookClients + if groupExtra != nil { + if groupExtra.AllowExternalSenders != nil { + allowExternalSenders = *groupExtra.AllowExternalSenders + } + if groupExtra.AutoSubscribeNewMembers != nil { + autoSubscribeNewMembers = *groupExtra.AutoSubscribeNewMembers + } + if groupExtra.HideFromAddressLists != nil { + hideFromAddressLists = *groupExtra.HideFromAddressLists + } + if groupExtra.HideFromOutlookClients != nil { + hideFromOutlookClients = *groupExtra.HideFromOutlookClients + } } } diff --git a/internal/services/groups/group_resource.go b/internal/services/groups/group_resource.go index a61cdbde60..f2d100386c 100644 --- a/internal/services/groups/group_resource.go +++ b/internal/services/groups/group_resource.go @@ -744,17 +744,15 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter // Newly created Unified groups now get a description added out-of-band, so we'll wait a couple of minutes to see if this appears and then clear it if description == "" { // Ignoring the error result here because the description might not be updated out of band, in which case we skip over this - updated, _ := helpers.WaitForUpdateWithTimeout(ctx, 2*time.Minute, func(ctx context.Context) (*bool, error) { + if updated, _ := helpers.WaitForUpdateWithTimeout(ctx, 2*time.Minute, func(ctx context.Context) (*bool, error) { client.BaseClient.DisableRetries = true group, _, err := client.Get(ctx, *group.ID(), odata.Query{}) if err != nil { return nil, err } return utils.Bool(group.Description != nil && *group.Description != ""), nil - }) - - if updated { - status, err := client.Update(ctx, msgraph.Group{ + }); updated { + status, err = client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), }, @@ -768,9 +766,9 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter } // Wait for Description to be removed - if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) { + if err = helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) { client.BaseClient.DisableRetries = true - group, _, err := client.Get(ctx, *group.ID(), odata.Query{}) + group, _, err = client.Get(ctx, *group.ID(), odata.Query{}) if err != nil { return nil, err } @@ -800,11 +798,11 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter // Wait for AllowExternalSenders to be updated if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) { client.BaseClient.DisableRetries = true - group, err := groupGetAdditional(ctx, client, *group.ID()) + groupExtra, err := groupGetAdditional(ctx, client, *group.ID()) if err != nil { return nil, err } - return utils.Bool(group.AllowExternalSenders != nil && *group.AllowExternalSenders == allowExternalSenders), nil + return utils.Bool(groupExtra != nil && groupExtra.AllowExternalSenders != nil && *groupExtra.AllowExternalSenders == allowExternalSenders), nil }); err != nil { return tf.ErrorDiagF(err, "Waiting for update of `external_senders_allowed` for group with object ID %q", *group.ID()) } @@ -824,11 +822,11 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter // Wait for AutoSubscribeNewMembers to be updated if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) { client.BaseClient.DisableRetries = true - group, err := groupGetAdditional(ctx, client, *group.ID()) + groupExtra, err := groupGetAdditional(ctx, client, *group.ID()) if err != nil { return nil, err } - return utils.Bool(group.AutoSubscribeNewMembers != nil && *group.AutoSubscribeNewMembers == autoSubscribeNewMembers), nil + return utils.Bool(groupExtra != nil && groupExtra.AutoSubscribeNewMembers != nil && *groupExtra.AutoSubscribeNewMembers == autoSubscribeNewMembers), nil }); err != nil { return tf.ErrorDiagF(err, "Waiting for update of `auto_subscribe_new_members` for group with object ID %q", *group.ID()) } @@ -848,11 +846,11 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter // Wait for HideFromAddressLists to be updated if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) { client.BaseClient.DisableRetries = true - group, err := groupGetAdditional(ctx, client, *group.ID()) + groupExtra, err := groupGetAdditional(ctx, client, *group.ID()) if err != nil { return nil, err } - return utils.Bool(group.HideFromAddressLists != nil && *group.HideFromAddressLists == hideFromAddressList), nil + return utils.Bool(groupExtra != nil && groupExtra.HideFromAddressLists != nil && *groupExtra.HideFromAddressLists == hideFromAddressList), nil }); err != nil { return tf.ErrorDiagF(err, "Waiting for update of `hide_from_address_lists` for group with object ID %q", *group.ID()) } @@ -872,11 +870,11 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter // Wait for HideFromOutlookClients to be updated if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) { client.BaseClient.DisableRetries = true - group, err := groupGetAdditional(ctx, client, *group.ID()) + groupExtra, err := groupGetAdditional(ctx, client, *group.ID()) if err != nil { return nil, err } - return utils.Bool(group.HideFromOutlookClients != nil && *group.HideFromOutlookClients == hideFromOutlookClients), nil + return utils.Bool(groupExtra != nil && groupExtra.HideFromOutlookClients != nil && *groupExtra.HideFromOutlookClients == hideFromOutlookClients), nil }); err != nil { return tf.ErrorDiagF(err, "Waiting for update of `hide_from_outlook_clients` for group with object ID %q", *group.ID()) } @@ -1021,7 +1019,7 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter } // AllowExternalSenders can only be set in its own PATCH request; including other properties returns a 400 - if v, ok := d.GetOkExists("external_senders_allowed"); ok && (extra.AllowExternalSenders == nil || *extra.AllowExternalSenders != v.(bool)) { //nolint:staticcheck + if v, ok := d.GetOkExists("external_senders_allowed"); ok && (extra == nil || extra.AllowExternalSenders == nil || *extra.AllowExternalSenders != v.(bool)) { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), @@ -1034,18 +1032,18 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter // Wait for AllowExternalSenders to be updated if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) { client.BaseClient.DisableRetries = true - group, err := groupGetAdditional(ctx, client, *group.ID()) + groupExtra, err := groupGetAdditional(ctx, client, *group.ID()) if err != nil { return nil, err } - return utils.Bool(group.AllowExternalSenders != nil && *group.AllowExternalSenders == v.(bool)), nil + return utils.Bool(groupExtra != nil && groupExtra.AllowExternalSenders != nil && *groupExtra.AllowExternalSenders == v.(bool)), nil }); err != nil { return tf.ErrorDiagF(err, "Waiting for update of `external_senders_allowed` for group with object ID %q", *group.ID()) } } // AutoSubscribeNewMembers can only be set in its own PATCH request; including other properties returns a 400 - if v, ok := d.GetOkExists("auto_subscribe_new_members"); ok && (extra.AutoSubscribeNewMembers == nil || *extra.AutoSubscribeNewMembers != v.(bool)) { //nolint:staticcheck + if v, ok := d.GetOkExists("auto_subscribe_new_members"); ok && (extra == nil || extra.AutoSubscribeNewMembers == nil || *extra.AutoSubscribeNewMembers != v.(bool)) { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), @@ -1058,18 +1056,18 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter // Wait for AutoSubscribeNewMembers to be updated if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) { client.BaseClient.DisableRetries = true - group, err := groupGetAdditional(ctx, client, *group.ID()) + groupExtra, err := groupGetAdditional(ctx, client, *group.ID()) if err != nil { return nil, err } - return utils.Bool(group.AutoSubscribeNewMembers != nil && *group.AutoSubscribeNewMembers == v.(bool)), nil + return utils.Bool(groupExtra != nil && groupExtra.AutoSubscribeNewMembers != nil && *groupExtra.AutoSubscribeNewMembers == v.(bool)), nil }); err != nil { return tf.ErrorDiagF(err, "Waiting for update of `auto_subscribe_new_members` for group with object ID %q", *group.ID()) } } // HideFromAddressLists can only be set in its own PATCH request; including other properties returns a 400 - if v, ok := d.GetOkExists("hide_from_address_lists"); ok && (extra.HideFromAddressLists == nil || *extra.HideFromAddressLists != v.(bool)) { //nolint:staticcheck + if v, ok := d.GetOkExists("hide_from_address_lists"); ok && (extra == nil || extra.HideFromAddressLists == nil || *extra.HideFromAddressLists != v.(bool)) { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), @@ -1082,18 +1080,18 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter // Wait for HideFromAddressLists to be updated if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) { client.BaseClient.DisableRetries = true - group, err := groupGetAdditional(ctx, client, *group.ID()) + groupExtra, err := groupGetAdditional(ctx, client, *group.ID()) if err != nil { return nil, err } - return utils.Bool(group.HideFromAddressLists != nil && *group.HideFromAddressLists == v.(bool)), nil + return utils.Bool(groupExtra != nil && groupExtra.HideFromAddressLists != nil && *groupExtra.HideFromAddressLists == v.(bool)), nil }); err != nil { return tf.ErrorDiagF(err, "Waiting for update of `hide_from_address_lists` for group with object ID %q", *group.ID()) } } // HideFromOutlookClients can only be set in its own PATCH request; including other properties returns a 400 - if v, ok := d.GetOkExists("hide_from_outlook_clients"); ok && (extra.HideFromOutlookClients == nil || *extra.HideFromOutlookClients != v.(bool)) { //nolint:staticcheck + if v, ok := d.GetOkExists("hide_from_outlook_clients"); ok && (extra == nil || extra.HideFromOutlookClients == nil || *extra.HideFromOutlookClients != v.(bool)) { //nolint:staticcheck if _, err := client.Update(ctx, msgraph.Group{ DirectoryObject: msgraph.DirectoryObject{ Id: group.ID(), @@ -1106,11 +1104,11 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter // Wait for HideFromOutlookClients to be updated if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) { client.BaseClient.DisableRetries = true - group, err := groupGetAdditional(ctx, client, *group.ID()) + groupExtra, err := groupGetAdditional(ctx, client, *group.ID()) if err != nil { return nil, err } - return utils.Bool(group.HideFromOutlookClients != nil && *group.HideFromOutlookClients == v.(bool)), nil + return utils.Bool(groupExtra != nil && groupExtra.HideFromOutlookClients != nil && *groupExtra.HideFromOutlookClients == v.(bool)), nil }); err != nil { return tf.ErrorDiagF(err, "Waiting for update of `hide_from_outlook_clients` for group with object ID %q", *group.ID()) } @@ -1308,17 +1306,19 @@ func groupResourceRead(ctx context.Context, d *schema.ResourceData, meta interfa return tf.ErrorDiagF(err, "Could not retrieve group with object UID %q", d.Id()) } - if groupExtra != nil && groupExtra.AllowExternalSenders != nil { - allowExternalSenders = *groupExtra.AllowExternalSenders - } - if groupExtra != nil && groupExtra.AutoSubscribeNewMembers != nil { - autoSubscribeNewMembers = *groupExtra.AutoSubscribeNewMembers - } - if groupExtra != nil && groupExtra.HideFromAddressLists != nil { - hideFromAddressLists = *groupExtra.HideFromAddressLists - } - if groupExtra != nil && groupExtra.HideFromOutlookClients != nil { - hideFromOutlookClients = *groupExtra.HideFromOutlookClients + if groupExtra != nil { + if groupExtra.AllowExternalSenders != nil { + allowExternalSenders = *groupExtra.AllowExternalSenders + } + if groupExtra.AutoSubscribeNewMembers != nil { + autoSubscribeNewMembers = *groupExtra.AutoSubscribeNewMembers + } + if groupExtra.HideFromAddressLists != nil { + hideFromAddressLists = *groupExtra.HideFromAddressLists + } + if groupExtra.HideFromOutlookClients != nil { + hideFromOutlookClients = *groupExtra.HideFromOutlookClients + } } } diff --git a/internal/services/groups/groups.go b/internal/services/groups/groups.go index c149255042..78702e12f2 100644 --- a/internal/services/groups/groups.go +++ b/internal/services/groups/groups.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "math/rand" + "net/http" "time" "github.com/hashicorp/go-azure-sdk/sdk/odata" @@ -44,8 +45,14 @@ func groupFindByName(ctx context.Context, client *msgraph.GroupsClient, displayN func groupGetAdditional(ctx context.Context, client *msgraph.GroupsClient, id string) (*msgraph.Group, error) { query := odata.Query{Select: []string{"allowExternalSenders", "autoSubscribeNewMembers", "hideFromAddressLists", "hideFromOutlookClients"}} - groupExtra, _, err := client.Get(ctx, id, query) + groupExtra, status, err := client.Get(ctx, id, query) if err != nil { + if status == http.StatusNotFound { + // API returns 404 when these M365-only fields are requested for a group in a non-M365 tenant, so we + // don't raise an error in this case and proceed as if they are not set. + // See https://github.com/microsoftgraph/msgraph-metadata/issues/333 + return nil, nil + } return nil, fmt.Errorf("retrieving additional fields: %+v", err) } return groupExtra, nil From ca15497e106073255e1217e71283c886cf94720f Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Wed, 19 Apr 2023 23:25:49 +0100 Subject: [PATCH 3/4] Add issue comment for https://github.com/microsoftgraph/msgraph-metadata/issues/331 --- internal/services/groups/group_resource.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/services/groups/group_resource.go b/internal/services/groups/group_resource.go index f2d100386c..e7a9eee11a 100644 --- a/internal/services/groups/group_resource.go +++ b/internal/services/groups/group_resource.go @@ -742,6 +742,7 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter if hasGroupType(groupTypes, msgraph.GroupTypeUnified) { // Newly created Unified groups now get a description added out-of-band, so we'll wait a couple of minutes to see if this appears and then clear it + // See https://github.com/microsoftgraph/msgraph-metadata/issues/331 if description == "" { // Ignoring the error result here because the description might not be updated out of band, in which case we skip over this if updated, _ := helpers.WaitForUpdateWithTimeout(ctx, 2*time.Minute, func(ctx context.Context) (*bool, error) { From 6e185e5e17421bc3337cf33a30af5ebf58c153d5 Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Wed, 19 Apr 2023 23:43:06 +0100 Subject: [PATCH 4/4] Docs update for #1076 --- docs/resources/group.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/resources/group.md b/docs/resources/group.md index 84b2c740ae..4c841d4db6 100644 --- a/docs/resources/group.md +++ b/docs/resources/group.md @@ -141,7 +141,7 @@ The following arguments are supported: !> **Warning** Do not use the `members` property at the same time as the [azuread_group_member](https://registry.terraform.io/providers/hashicorp/azuread/latest/docs/resources/group_member) resource for the same group. Doing so will cause a conflict and group members will be removed. -* `onpremises_group_type` - (Optional) The on-premises group type that the AAD group will be written as, when writeback is enabled. Possible values are `UniversalDistributionGroup`, `UniversalMailEnabledSecurityGroup`, or `UniversalSecurityGroup`. Once set, unsetting this property forces a new resource to be created. +* `onpremises_group_type` - (Optional) The on-premises group type that the AAD group will be written as, when writeback is enabled. Possible values are `UniversalDistributionGroup`, `UniversalMailEnabledSecurityGroup`, or `UniversalSecurityGroup`. * `owners` - (Optional) A set of object IDs of principals that will be granted ownership of the group. Supported object types are users or service principals. By default, the principal being used to execute Terraform is assigned as the sole owner. Groups cannot be created with no owners or have all their owners removed. -> **Group Ownership** It's recommended to always specify one or more group owners, including the principal being used to execute Terraform, such as in the example above. When removing group owners, if a user principal has been assigned ownership, the last user cannot be removed as an owner. Microsoft 365 groups are required to always have at least one owner which _must be a user_ (i.e. not a service principal).