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

Wrap Get-MachineSid's body in a try/catch #58483

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@mletterle
Copy link

commented Jun 28, 2019

SUMMARY

Get-MachineSid has failed in a multitude of ways over the years, and #47813 describes at least two more scenarios. A comment on an earlier issue suggested just wrapping it in a try/catch. Sounded good to me!

Fixes: #47813

Also see: #38257 and #29524

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

modules/windows/setup.ps1

@ansibot

This comment has been minimized.

@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

Not sure that simply ignoring every possible exception is the right way.
@jborean93 - any thoughts?

@nitzmahone

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Given the multitude of ways that Get-MachineSid can fail, and that our general policy is "setup shouldn't break for an inability to gather a particular fact", a blanket catch block is probably the right solution for this particular case. That said, I think the behavior when encountering one of those failures is up for debate, since sometimes the failure is "correct" (eg container sans lanmanservice) vs "an actual problem" (eg, service isn't/can't start, some other problem). Even in those legit failure cases, though, we can't necessarily know if the failure to compute a particular fact is a problem for the automation in progress...

There are a few behavioral options we could pursue- I imagine the final solution will be some combination of:

  • return $null for the broken fact
  • return a nonsensical string for the broken fact
  • return a dictionary with error detail for the broken fact
  • return a warning in the setup result that includes the error details
  • return a wrapped object for the broken fact that trips a runtime warning (also possibly including more error detail like stacktrace) if the fact is referenced in a template on the controller (a feature coming in 2.9), otherwise silent

There are pluses and minuses to each of these. Once the wrapped runtime warning support is in, I'd probably lean toward the last option, but "lazy error" is a case I just dreamed up for it (it was originally intended to fire warnings on usage of deprecated result values in templates), so I haven't implemented that part yet.

@rnsc

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

Hi, I just tested this patch on Ansible 2.8.2 because I had issues with the setup module failing on Windows targets that were joined on an AD.

I no longer have the error with this patch and I can ran setup without issues on any Windows targets I have at hand.

Will this be back-ported to Ansible 2.8? Otherwise I'll have to keep a copy of this module locally until 2.9 is released.

@mletterle mletterle force-pushed the mletterle:optional_machinesid branch from d6902ac to ead26ed Jul 11, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

The test ansible-test sanity --test pslint [explain] failed with 3 errors:

lib/ansible/modules/windows/setup.ps1:33:1: PSAvoidTrailingWhitespace Line has trailing whitespace
lib/ansible/modules/windows/setup.ps1:34:10: PSAvoidTrailingWhitespace Line has trailing whitespace
lib/ansible/modules/windows/setup.ps1:51:7: PSAvoidUsingEmptyCatchBlock Empty catch block is used. Please use Write-Error or throw statements in catch blocks.

click here for bot help

@ansibot ansibot added the ci_verified label Jul 11, 2019

Wrap Get-MachineSid's body in a try/catch
It's not critical information and there's been a number of issues over
the years with trying to retrieve it. If an exception is thrown just
return null.

Fixes: #47813

@mletterle mletterle force-pushed the mletterle:optional_machinesid branch from ead26ed to 935f0e0 Jul 11, 2019

@nitzmahone
Copy link
Member

left a comment

+1 for now- this works as a stopgap to keep setup working, and we can revisit a saner per-fact impl with wrapped return value error triggers later... Agree it should also be backported to 2.8.x. (needs a changelog fragment)

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.