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

azuread_group import of dynamic groups tries to fetch all members #1000

Open
bufferoverflow opened this issue Feb 7, 2023 · 9 comments
Open

Comments

@bufferoverflow
Copy link

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.3.7 on linux_amd64
  • terraform-provider-azuread 2.33.0

Affected Resource(s)

  • azuread_group

Terraform Configuration Files

data "azuread_client_config" "current" {}

resource "azuread_group" "example" {
  display_name     = "MyGroup"
  owners           = [data.azuread_client_config.current.object_id]
  security_enabled = true
  types            = ["DynamicMembership"]

  dynamic_membership {
    enabled = true
    rule    = "user.department -eq \"Sales\""
  }
}

Debug Output

Panic Output


terraform import azuread_group.example 11111111-1111-1111-1111-111111111111
azuread_group.example: Importing from ID "11111111-1111-1111-1111-111111111111"...
azuread_group.example: Import prepared!
  Prepared azuread_group for import
azuread_group.example: Refreshing state... [id=11111111-1111-1111-1111-111111111111]
╷
│ Error: Could not retrieve members for group with object ID "11111111-1111-1111-1111-111111111111"
│ 
│ GroupsClient.BaseClient.Get(): Get
│ "https://graph.microsoft.com/beta/11111111-1111-1111-1111-111111111111/groups/11111111-1111-1111-1111-111111111111/members?$select=id&$skiptoken=RFNwdAoAAAAAAAAAAAAAFAAAADkNvV0eD5ZIlXXJLwzCW7YJAwAAAAAAAAAAAAAAAAAXMS4yLjg0sdfgsdfhsdhsdghdghgdh0LjIzMzEGAAAAAAAB2N8lgrwOrUWyjdSo3hXDiwE8AAAAAQMAAAA":
│ http: RoundTripper implementation (*retryablehttp.RoundTripper) returned a nil *Response with a nil error
╵

Expected Behavior

Import does not read all members from a dynamic group.

Actual Behavior

It fails to import as it tries to fetch all members ( > 80000 entries )

Steps to Reproduce

  1. Create a dynamic group with 80000 entries
  2. Import the state

Important Factoids

References

  • #0000
@Threpio
Copy link
Contributor

Threpio commented Feb 8, 2023

Interesting that the expected behavior would be to not read all members of a dynamic group - I am not familiar with how the state is meant to handle Dynamic Member Groups in other contexts.

When a dynamic rules group is created does it not include the members in the state?

This is pretty much caused by the groupResourceRead function which includes the following line with no application logic:

members, _, err := client.ListMembers(ctx, *group.ID())

This could be changed if desired to not read members array in a dynamic membership but would be a breaking change for some uses!

@bufferoverflow
Copy link
Author

I agree that read all the members could make sense for some use cases. But the state will change continuously for dynamic groups especially in large companies. Therefore it becomes a mission impossible to use azuread_group for dynamic groups within large companies.

Maybe an option such as fetch_dynamic_members = false (default true) within azuread_group could do the trick?

@Threpio Threpio mentioned this issue Feb 8, 2023
@jakefrancois5
Copy link

+1. I see the pull request is closed though. We would like to use TF to manage dynamic groups but it's unrealistic when we have multiple dynamic groups with 10's of thousands of members and some closing in on 100,000.

@jakefrancois5
Copy link

@manicminer Could you re-examine this pull request, perhaps as a feature rather than a bug fix? #1003

While I agree it's not the prettiest work-around for for this error, it would at least make this usable. Additionally, it would significantly reduce Terraform plan times to have the ability to explicitly exclude group members from being pulled in.
I would like to see a similar feature implemented in V3 of these modules as well.

@manicminer
Copy link
Contributor

@jakefrancois5 We'll be fixing this by splitting the group resource into multiple resources, which will yield an optional azuread_group_members resource.

@manicminer manicminer added this to the v3.0.0 milestone Apr 11, 2023
@magic-happenz
Copy link

@manicminer could you explain how splitting the resources can solve the problem? I am facing it right now and my plan just fails after refreshing like forever. I don't see how I could avoid it from happening using the dedicated member resource. I create a group in my plan which I assign ownership to another user. I ignore basically any change on the members, owners, type etc. When the owner now enables dynamic membership or adds thousands of users my plan is still trying to fetch those which is not what I want, at least not if it breaks my pipeline. Any way to fix this?

@manicminer manicminer removed this from the v3.0.0 milestone Sep 27, 2024
@stawik-mesa
Copy link
Contributor

@manicminer I am using now the latest provider 3.0.1 and we are also running into this issue.

Would it make sense to implement a new flag which skips the members update during the refresh?
I am constantly running into timeouts and memory issues due to the fact that the dynamic group which was created via Terraform is fetching all 60.000 members during the plan. Would be great if we can fix this with another feature flag.

Or is there already another solution for this issue?
e.g. #1003

@bufferoverflow
Copy link
Author

welcome to the club @stawik-mesa 😸

@oeghaneyan
Copy link

I'm not sure why this was pulled from the v3.0 milestone but this issue also seems related to issue 954. Both are being reviewed internally to be addressed.

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

Successfully merging a pull request may close this issue.

7 participants