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

win_domain_user: add retry logic for null user principal group #54334

Open
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@Daniel-Sanchez-Fabregas
Copy link
Contributor

Daniel-Sanchez-Fabregas commented Mar 25, 2019

SUMMARY

Active Directory Principal Group could be read null when created while AD global catalogue is replicated. This patch adds retry logic to avoid raising a exception inside win_domain_user module when retrieving information with Get-ADPrincipalGroupMembership

Fixes #54331

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_domain_user

ADDITIONAL INFORMATION

Just works.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 25, 2019

The test ansible-test sanity --test pslint [explain] failed with 1 error:

test/sanity/pslint/ignore.txt:65:1: A102 Remove since "lib/ansible/modules/windows/win_domain_user.ps1" passes "PSAvoidTrailingWhitespace" test

click here for bot help

@ansibot

This comment has been minimized.

@Daniel-Sanchez-Fabregas Daniel-Sanchez-Fabregas force-pushed the Daniel-Sanchez-Fabregas:pr_54331 branch to 92f6994 Mar 26, 2019

@ansibot ansibot removed the ci_verified label Mar 26, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 26, 2019

@jborean93

This comment has been minimized.

Copy link
Contributor

jborean93 commented Apr 4, 2019

I'm unsure as to why this fix is needed, sorry I'm not too familiar with these cmdlets. Are you creating that principal before the win_domain_user task or is this an issue when creating the domain user in the module then trying to manage the groups for the newly created user?

@ansibot ansibot removed the needs_triage label Apr 4, 2019

@Daniel-Sanchez-Fabregas

This comment has been minimized.

Copy link
Contributor Author

Daniel-Sanchez-Fabregas commented Apr 4, 2019

@jborean93 It all happens when creating a new user, it automatically gets "Domain user" group as principal.

New-ADUser -Name $username
Get-ADPrincipalGroupMembership $username

When I use win_domain_user in our more loaded AD it fails randomly, digging in we saw that this AD is not fast enough when updating the group in the global catalogue. It should have a "Domain user" group, but is not there yet and fails. Part of our solution is adding a retry in Get-ADPrincipalGroupMembership

@jborean93

This comment has been minimized.

Copy link
Contributor

jborean93 commented Apr 9, 2019

Ok that makes sense, before I review the actual code are you able to align it to be like the rest of the module style, this would be things like;

  • Use the proper verb-noun syntax for the cmdlet/function you added
  • Keep the indentation 4 spaces
  • Don't use aliases for cmdlets, e.g. Start-Sleep -Seconds 1, vs sleep 1

I'm not sure if it is possible but it would be a lot better if we could use some event watcher or notification service for when the principal group member is set for the user object. Having arbitrary sleeps are not the best practice and it's not something I usually feel comfortable with.

@Daniel-Sanchez-Fabregas

This comment has been minimized.

Copy link
Contributor Author

Daniel-Sanchez-Fabregas commented Apr 11, 2019

I'm not sure if it is possible but it would be a lot better if we could use some event watcher or notification service for when the principal group member is set for the user object. Having arbitrary sleeps are not the best practice and it's not something I usually feel comfortable with.

It will need:

  • Active Directory group audits enabled
  • More user permission to read AD audit logs

Other possible approach is returning $null (or the default group "Domain user") as PrincipalGroupMembership, when Get-ADPrincipalGroupMembership fails, and get rid of the retries.

@jborean93, what path seems better for you?

@ansibot ansibot removed the stale_ci label Apr 11, 2019

@ansibot ansibot added the stale_ci label Apr 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.