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

Add exception handling to win_domain_controller #58234

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@agowa338
Copy link
Contributor

commented Jun 22, 2019

SUMMARY

Add error handling for two exceptions.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_domain_controller

ADDITIONAL INFORMATION

If a user specifies a domain to join that cannot be resolved by dns, the Install-ADDSDomainController command throws an Microsoft.ADRoles.Deployment.Common.Tests.TestFailedException exception.

If the module is either invoked twice without a reboot, or a reboot is pending for another reason the Install-ADDSDomainController command throws an Microsoft.DirectoryServices.Deployment.DCPromoExecutionException exception with Errorcode 15.

I added error handling for both of them, so that a user can handle them from within the playbook.

@ansibot

This comment has been minimized.

} catch {
# We cannot directly catch Microsoft.ADRoles.Deployment.Common.Tests.TestFailedException
# As the type is within an internal flagged class of Microsoft.ADRoles.Deployment.Common
if (

This comment has been minimized.

Copy link
@jborean93

jborean93 Jul 4, 2019

Contributor

I don't see a reason why we need this special handling here, when I test it locally the exception already shows the failed to resolve domain like message

Install-ADDSDomainController : Verification of user credential permissions failed. An Active Directory domain
controller for the domain "fake.domain.com" could not be contacted.
Ensure that you supplied the correct DNS domain name.
At line:1 char:1
+ Install-ADDSDomainController -DomainName 'fake.domain.com' -NoRebootO ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [Install-ADDSDomainController], TestFailedException
    + FullyQualifiedErrorId : Test.VerifyUserCredentialPermissions.DCPromo.General.25,Microsoft.DirectoryServices.Depl
   oyment.PowerShell.Commands.InstallADDSDomainControllerCommand

It does have some extra fluff but I don't think we need to complicate the catch logic especially since this is an internal exception.

This comment has been minimized.

Copy link
@agowa338

agowa338 Jul 5, 2019

Author Contributor

I just did not want to do a catch all and this was the best solution I found to only match that particular exception. How could this be improved?
Or is it ok to do a catch all here?

This comment has been minimized.

Copy link
@jborean93

jborean93 Jul 5, 2019

Contributor

Honestly just don’t catch all as Ansible will now properly display the traceback and error message for you. You should only catch exceptions if the actual error is vague and you can improve it.

This comment has been minimized.

Copy link
@agowa338

agowa338 Jul 5, 2019

Author Contributor

But if I don't catch it the playbook fails. And exactly when this error is thrown that should not happen.

This comment has been minimized.

Copy link
@jborean93

jborean93 Jul 5, 2019

Contributor

Oh keep the first exception handler for DCPromoExecutionEngine but just remove the whole generic exception catch block.

This comment has been minimized.

Copy link
@agowa338

agowa338 Jul 5, 2019

Author Contributor

But than the user gets an error that cannot be handled within the playbook (and the whole play will fail) if Install-ADDSDomainController is unable to resolve the dns domain name... With the current exception handling a user could catch the error within the playbook and take action.

Maybe there is a misunderstanding:

Ansible will now properly display the traceback and error message for you

The error is warped into a Fail-Json, so that ansible can not just display the error and error the whole play, but also to allow a user to work with the error using ignore_error, register and/or retry. And for that to work, ansible needs to know about this error.

This comment has been minimized.

Copy link
@jborean93

jborean93 Jul 5, 2019

Contributor

You can still pick up unhandled exceptions in a task, we even have a test for it. This test shows you what happens when you don't catch an explicit throw. The msg return value is the string Unhandled exception while executing module: $($_.Exception.Message) whereas the exception return value is the PowerShell error message and script stack trace.

Then this test shows you what happens when a .net function raises an unhandled exception. The same thing occurs and you can use msg or exception to parse.

Here is the module code that it is running to show you how the errors are fired https://github.com/ansible/ansible/blob/devel/test/integration/targets/win_exec_wrapper/library/test_fail.ps1.

So for this case you would have something like

- win_domain_controller:
    dns_domain_name: unknown.domain
    domain_admin_user: username@unknown.domain
    domain_admin_password: password123!
    safe_mode_password: password123!
    state: domain_controller
  register: dc_result
  failed_when: dc_result is failed and dc_result.msg != "could not be contacted"
@jborean93

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Also it would be great if you could add a changelog similar to https://github.com/ansible/ansible/pull/53480/files#diff-0b9ea3a63f047de3086e081e4eb479f2R3 for the module.

@ansibot ansibot added the stale_ci label Jul 4, 2019

@agowa338 agowa338 closed this Jul 5, 2019

@agowa338 agowa338 reopened this Jul 5, 2019

@agowa338

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

closing and reopening to trigger the ci

@samdoran

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

This needs to be rebased in order for tests to run properly.

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.