fix big group problem #204

baldator · 2021-02-25T13:36:37Z

SUMMARY

win_domain_group_membership might fail if the group has a lot of members

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

win_domain_group_membership

ADDITIONAL INFORMATION

Get-ADGroupMember might hit a timeout if used against a big group. The behavior is described here: https://pscustomobject.github.io/powershell/powershell%20tips/Get-AdGroupMember-Timeout/

I'm proposing a fix to this problem by implementing the solution proposed in the article.
The change isn't a breaking change as the output object's format doesn't change.

Please feel free to accept this PR in case you think the enhancement might be useful.

jborean93 · 2021-03-12T01:23:18Z

My concerns with this approach is that now instead of potentially 1 query to get the AD group members we are using 1 query to get the members and then n queries to get the ADUser object for the sAMAccountName/SID for each of those users. While it may work this would be a very chatty operation and I would hope we can do better than that. The other issue is that this just returns user principals, we also need to deal with other groups, computer accounts, etc.

At first I tried to replicate the timeout problem by creating a group with 10000 users and while I cannot seem to replicate the timeout problem I do get a different error saying the size limit has been reached.

New-ADGroup -Name MyGroup -GroupScope Global
1..10000 | ForEach-Object -Process {
    New-ADUser -Name "Test$_" -PassThru | ForEach-Object { Add-ADGroupMember -Identity "MyGroup" -Members $_ }
}

Measure-Command { &{ Get-ADGroupMember -Identity MyGroup } }

Get-ADGroupMember : The size limit for this request was exceeded
At line:1 char:22
+ Measure-Command { &{ Get-ADGroupMember -Identity MyGroup } }
+                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (MyGroup:ADGroup) [Get-ADGroupMember], ADException
    + FullyQualifiedErrorId : ActiveDirectoryServer:8227,Microsoft.ActiveDirectory.Management.Commands.GetADGroupMembe
   r

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 6
Milliseconds      : 82
Ticks             : 60827652
TotalDays         : 7.0402375E-05
TotalHours        : 0.001689657
TotalMinutes      : 0.10137942
TotalSeconds      : 6.0827652
TotalMilliseconds : 6082.7652

Using a slightly tweaked version of what's in the PR this is what happens

Measure-Command { &{
    Get-ADGroup -Identity MyGroup -Properties Member |
        Select-Object -ExpandProperty Member |
        ForEach-Object {
            Get-ADUser $_ -Properties sAMAccountName, sid
        }
} }

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 29
Milliseconds      : 118
Ticks             : 291186293
TotalDays         : 0.000337021172453704
TotalHours        : 0.00808850813888889
TotalMinutes      : 0.485310488333333
TotalSeconds      : 29.1186293
TotalMilliseconds : 29118.6293

30 seconds isn't too bad but this is a fairly basic AD test environment I have. I assume that in an actual environment this will be higher based on various environment settings.

We could just have an LDAP filter that gets all the objects

Following that article you linked to I think we can do one better to get only the sAMAccountName and SID that is required for each member rather than the whole ADUser object and also 1 one query.

$group = Get-ADGroup MyGroup

# Can probably find tune this to filter for specific types but this just a test
$filter = "(memberOf=$($group.DistinguishedName))"

Measure-Command { & { Get-ADObject -LDAPFilter $filter -Properties sAMAccountName, objectSID } }

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 1
Milliseconds      : 659
Ticks             : 16592064
TotalDays         : 1.92037777777778E-05
TotalHours        : 0.000460890666666667
TotalMinutes      : 0.02765344
TotalSeconds      : 1.6592064
TotalMilliseconds : 1659.2064

This cuts down the time a bit more but I'm not sure whether that's just a byproduct on my small AD environment. This is quick because it's just 1 call/network trip but the server still needs to scan each object and IIRC this won't work if a member is in a different domain (could be wrong about that though).

The next route would be to do a similar operation as Get-ADGroupMember but make it more efficient. We can use the .NET methods in a similar fashion

Add-Type -AssemblyName System.DirectoryServices.AccountManagement
$Context = [DirectoryServices.AccountManagement.PrincipalContext]::new('Domain')
$group = Get-ADGroup MyGroup

Measure-Command { & {
    $members = [DirectoryServices.AccountManagement.GroupPrincipal]::FindByIdentity(
        $Context, 'DistinguishedName', $group.DistinguishedName).Members

    foreach ($member in $members) {
         [PSCustomObject]@{sAMAAccountName = $member.sAMAAccountName; Sid = $member.Sid}
    }
} }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 8
Milliseconds      : 38
Ticks             : 80387648
TotalDays         : 9.30412592592592E-05
TotalHours        : 0.00223299022222222
TotalMinutes      : 0.133979413333333
TotalSeconds      : 8.0387648
TotalMilliseconds : 8038.7648

While not as fast as the LDAPFilter approach it is still faster than query each object separately and maybe the time taken could be faster if the AD environment is quite large. The only thing I can see being faster is to get a list of member and then building a large -LDAPFilter that selects all the prinicpals as one large chained or filter. This would get complex quite fast as you need to deal with the filter length limits but it should theorectically be possible. I personally don't think we need to go that far.

It would be interesting if you were able to try each of the approaches here and share the timings for your environment. We might be able to select one of these for this PR and fix the original problem you are trying to solve.

baldator · 2021-03-15T21:16:56Z

Thank you for your super detailed analysis.
I'll be out of office during the next 3 weeks.
I'll test and come back to you then :)

baldator · 2021-04-06T15:49:23Z

Hi @jborean93

After running my tests I confirm the ldap filter approach is the fastest. For your references here are the results:

the tweaked version of my PR:

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 3
Milliseconds      : 697
Ticks             : 36972850
TotalDays         : 4.2792650462963E-05
TotalHours        : 0.00102702361111111
TotalMinutes      : 0.0616214166666667
TotalSeconds      : 3.697285
TotalMilliseconds : 3697.285

ldap filter approach:

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 333
Ticks             : 3338137
TotalDays         : 3.86358449074074E-06
TotalHours        : 9.27260277777778E-05
TotalMinutes      : 0.00556356166666667
TotalSeconds      : 0.3338137
TotalMilliseconds : 333.8137

.NET solution:

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 487
Ticks             : 4876865
TotalDays         : 5.64451967592593E-06
TotalHours        : 0.000135468472222222
TotalMinutes      : 0.00812810833333333
TotalSeconds      : 0.4876865
TotalMilliseconds : 487.6865

I propose to optimize the ldap query like that: (&(objectClass=groupOfNames)(memberOf=$($group.DistinguishedName)))

I've also spotted a missing @extra_args in the last Remove-ADPrincipalGroupMembership: I'm going to update the PR with these two changes.

Thank you,
Marco

jborean93 · 2021-05-08T03:47:08Z

plugins/modules/win_domain_group_membership.ps1

@@ -50,7 +50,11 @@ if ($diff_mode) {
    $result.diff = @{}
 }

-$members_before = Get-AdGroupMember -Identity $ADGroup @extra_args
+$group = Get-ADGroup $ADGroup @extra_args


No need to run this again, you can just use $ADGroup which was retrieved above.

you are right. I've fixed it

jborean93 · 2021-05-08T03:48:02Z

plugins/modules/win_domain_group_membership.ps1

@@ -50,7 +50,11 @@ if ($diff_mode) {
    $result.diff = @{}
 }

-$members_before = Get-AdGroupMember -Identity $ADGroup @extra_args
+$group = Get-ADGroup $ADGroup @extra_args
+$filter = " (&(objectClass=groupOfNames)(memberOf=$($group.DistinguishedName)))"


Can you explain the objectClass=groupOfNames part? When using this filter I get nothing returned back to me on some of my AD groups.

There's also a leading space here that probably isn't needed.

...good catch: it doesn't make sense. I propose to remove the class filter as we can have groups of any ldap object class.

jborean93 · 2021-05-08T03:49:39Z

Sorry for the delay, I just one question around the objectClass=groupOfNames you've added to the filter. Can you explain that a bit further?

baldator

Thank you for your feedback. I proposed a solution and pushed a new version.
I'm happy to discuss in case you have any additional question.

baldator · 2021-05-10T08:18:39Z

plugins/modules/win_domain_group_membership.ps1

@@ -50,7 +50,11 @@ if ($diff_mode) {
    $result.diff = @{}
 }

-$members_before = Get-AdGroupMember -Identity $ADGroup @extra_args
+$group = Get-ADGroup $ADGroup @extra_args


you are right. I've fixed it

baldator · 2021-05-10T08:20:04Z

plugins/modules/win_domain_group_membership.ps1

@@ -50,7 +50,11 @@ if ($diff_mode) {
    $result.diff = @{}
 }

-$members_before = Get-AdGroupMember -Identity $ADGroup @extra_args
+$group = Get-ADGroup $ADGroup @extra_args
+$filter = " (&(objectClass=groupOfNames)(memberOf=$($group.DistinguishedName)))"


...good catch: it doesn't make sense. I propose to remove the class filter as we can have groups of any ldap object class.

jborean93 · 2021-05-11T00:12:22Z

Thanks for working on this! The CI failure is unrelated so I'll merge this in.

baldator added 2 commits February 25, 2021 14:14

fix big group problem

cf48b16

fix syntax to pass tests

29c19e2

Update win_domain_group_membership.ps1

c9216b7

jborean93 reviewed May 8, 2021

View reviewed changes

fix as suggested by @jborean93

6fa7158

baldator commented May 10, 2021

View reviewed changes

Added changelog fragment

7f3060a

jborean93 merged commit 72e4a1a into ansible-collections:main May 11, 2021

jborean93 mentioned this pull request Jun 8, 2021

win_domain_group_membership: Group members can't be set when state is pure #256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix big group problem #204

fix big group problem #204

baldator commented Feb 25, 2021

jborean93 commented Mar 12, 2021

baldator commented Mar 15, 2021

baldator commented Apr 6, 2021

jborean93 May 8, 2021

baldator May 10, 2021

jborean93 May 8, 2021

baldator May 10, 2021

jborean93 commented May 8, 2021

baldator left a comment

baldator May 10, 2021

baldator May 10, 2021

jborean93 commented May 11, 2021

fix big group problem #204

fix big group problem #204

Conversation

baldator commented Feb 25, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

jborean93 commented Mar 12, 2021

baldator commented Mar 15, 2021

baldator commented Apr 6, 2021

jborean93 May 8, 2021

Choose a reason for hiding this comment

baldator May 10, 2021

Choose a reason for hiding this comment

jborean93 May 8, 2021

Choose a reason for hiding this comment

baldator May 10, 2021

Choose a reason for hiding this comment

jborean93 commented May 8, 2021

baldator left a comment

Choose a reason for hiding this comment

baldator May 10, 2021

Choose a reason for hiding this comment

baldator May 10, 2021

Choose a reason for hiding this comment

jborean93 commented May 11, 2021