Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Group naming policy is not respected for 365 groups #1556

Open
tibbing opened this issue Oct 31, 2024 · 4 comments · May be fixed by #1562
Open

[BUG] Group naming policy is not respected for 365 groups #1556

tibbing opened this issue Oct 31, 2024 · 4 comments · May be fixed by #1562

Comments

@tibbing
Copy link

tibbing commented Oct 31, 2024

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritise this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritise the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform (and AzureAD Provider) Version

  • Terraform v1.9.8
  • AzureAD Provider v3.0.2

Affected Resource(s)

  • azuread_group

Terraform Configuration Files

data "azuread_client_config" "current" {}

resource "azuread_group" "example" {
  display_name               = "#example" # Naming policy requires a prefix of "#"
  owners                     = [data.azuread_client_config.current.object_id]
  security_enabled           = false
  mail_enabled               = true
  mail_nickname              = "example"
  prevent_duplicate_names    = false

  types = ["Unified"]
}

Debug Output

Debug log excerpt
2024/10/30 16:19:53 [DEBUG] ============================ Begin AzureAD Request ============================
Request ID: 992d4788-1f23-9872-c51d-cf6ff0861e1c

PATCH /beta/groups/0377fd2e-8abf-4865-83c6-e008c3c45f6c HTTP/1.1
Host: graph.microsoft.com
User-Agent: HashiCorp Terraform/1.9.8 (+https://www.terraform.io) Terraform Plugin SDK/2.34.0 terraform-provider-azuread/3.0.2 HashiCorp/go-azure-sdk (Go-http-Client/1.1 MicrosoftGraph-group/beta) pid-222c6c49-1b0a-5959-a213-6608f9eb8820
Content-Length: 110
Accept: application/json; charset=utf-8; IEEE754Compatible=false
Content-Type: application/json; charset=utf-8
Odata-Maxversion: 4.0
Odata-Version: 4.0
Accept-Encoding: gzip

{"@odata.type":"#microsoft.graph.group","displayName":"TERRAFORM_UPDATE_1f8282a3-8edf-8125-8c16-68bce37ca75f"}
============================= End AzureAD Request =============================: timestamp="2024-10-30T16:19:53.391+0100"
2024-10-30T16:19:53.391+0100 [INFO]  provider.terraform-provider-azuread_v3.0.2_x5: 2024/10/30 16:19:53 [DEBUG] PATCH https://graph.microsoft.com/beta/groups/0377fd2e-8abf-4865-83c6-e008c3c45f6c: timestamp="2024-10-30T16:19:53.391+0100"
2024-10-30T16:19:53.501+0100 [INFO]  provider.terraform-provider-azuread_v3.0.2_x5: 2024/10/30 16:19:53 [DEBUG] ============================ Begin AzureAD Response ===========================
PATCH https://graph.microsoft.com/beta/groups/0377fd2e-8abf-4865-83c6-e008c3c45f6c
Request ID: 992d4788-1f23-9872-c51d-cf6ff0861e1c

HTTP/2.0 400 Bad Request
Cache-Control: no-cache
Client-Request-Id: 7c054b90-63ec-4b13-85c7-df6ebd661751
Content-Type: application/json
Date: Wed, 30 Oct 2024 15:19:53 GMT
Request-Id: 7c054b90-63ec-4b13-85c7-df6ebd661751
Strict-Transport-Security: max-age=31536000
Vary: Accept-Encoding
X-Ms-Ags-Diagnostic: {"ServerInfo":{"DataCenter":"Sweden Central","Slice":"E","Ring":"3","ScaleUnit":"000","RoleInstance":"GVX0EPF00004FE7"}}
X-Ms-Resource-Unit: 1

{"error":{"code":"Request_BadRequest","message":"The property is missing a required prefix/suffix per your organization's Group naming requirements.","details":[{"code":"MissingPrefixSuffix","message":"The property is missing a required prefix/suffix per your organization's Group naming requirements.","target":"displayName"},{"code":"SuggestedPropertyValue","message":"displayName","target":"#TERRAFORM_UPDATE_1f8282a3-8edf-8125-8c16-68bce37ca75f"}],"innerError":{"date":"2024-10-30T15:19:53","request-id":"7c054b90-63ec-4b13-85c7-df6ebd661751","client-request-id":"7c054b90-63ec-4b13-85c7-df6ebd661751"}}}
============================= End AzureAD Response ============================: timestamp="2024-10-30T16:19:53.501+0100"

Expected Behavior

The newly created group is either not renamed, or the temporary name respects the naming policy. Perhaps there could be an argument for prefix/suffix in the module. Can we determine if the group exists by objectId rather than renaming it?

Actual Behavior

The group is temporarily renamed to TERRAFORM_UPDATE_%s here. This will fail if there is a naming policy configured with error:

The property is missing a required prefix/suffix per your organization's Group naming requirements.

Steps to Reproduce

  1. Configure a naming policy in Entra ID, requiring a prefix such as "#"
  2. Use the above configuration to create a 365 group, giving the group a valid name according to the naming policy.
  3. Run terraform apply

References

@tibbing
Copy link
Author

tibbing commented Oct 31, 2024

For reference, seems this behavior was introduced in #659 (cc @manicminer)

@manicminer
Copy link
Contributor

manicminer commented Dec 16, 2024

@tibbing That's an unfortunate side effect of the way the provider attempts to manage eventual consistency in the API. For context, groups are patched with a temporary display name in order to determine whether the group has successfully replicated on the backend.

It's possible a different field could be used here that is less likely to fall foul of a policy setting. Unfortunately the group description can't be used here as there is a separate bug that would break this when created unified groups.

EDIT: At second glance, I don't see any other field that would be suitable for this. More thought needed here.

@tibbing
Copy link
Author

tibbing commented Dec 16, 2024

@tibbing That's an unfortunate side effect of the way the provider attempts to manage eventual consistency in the API. For context, groups are patched with a temporary display name in order to determine whether the group has successfully replicated on the backend.

It's possible a different field could be used here that is less likely to fall foul of a policy setting. Unfortunately the group description can't be used here as there is a separate bug that would break this when created unified groups.

EDIT: At second glance, I don't see any other field that would be suitable for this. More thought needed here.

FWIW we've been running the forked provider (without the eventual consistency logic) for a couple of months with quite heavy usage now without any issues whatsoever. Might have been lucky so far, but do we know for sure that eventual consistency really needs to be considered here?

If yes, couldn't we add prefix/suffix arguments (or a policy regex) that isused with the temporary name so that we still comply with the naming policy?

@manicminer
Copy link
Contributor

@tibbing It's definitely an issue, one that surfaces pretty quickly across the provider user base. The backend replication of new groups is not very predictable, but anecdotally seems to be subject to a number of factors including number of objects in tenant, number of groups in tenant, number of members in groups, tenant geo location, API operations across the tenant per time period, API operations per principal per time period, and general Azure AD (Entra ID) system status. The current workaround by the provider of temporarily patching the display name was arrived at after extensive testing and user reports - unfortunately Microsoft Graph doesn't offer a reliable way of determining the replication status of a group (or any object), and read-consistent caching compounds this; hence the PATCH operation.

It could get very complicated trying to construct a temporary name that meets the naming policy, given the relatively short maxlength and wide range of group names being attempted in the wild (not to mention complexity/constraints of naming policies). I do think a better solution is warranted, I am just not yet sure what that should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants