Improved WinRM error handling (including better `ready?` and `wait_for_ready`) #4943

maxlinc · 2014-12-11T19:25:55Z

I split this from #4236 to make things easier to review. That PR contains just changes for SSL. This PR builds on that PR so the diff currently shows SSL support, but it'll shrink once that PR is merged and I rebase.

A few notes:

Authentication failures are currently retriable, and I haven't changed the default max_times, which was set to 20, with a 10 second sleep. That means WinRM will retry authentication for 10 minutes attempt authentication!
This uses a pre-release gem for WinRM. So it's technically releasable (as opposed to using git), but ideally we'd wait for an official WinRM release.
I've modeled it after SSH. I'm not sure about all the possible errors we could get from WinRM, but I think its a good assumption that it either throws - or should throw applicable errors from Errno.
Since it's modeled after SSH there is a lot of duplicate code, and duplicate translation keys. It might be worth refactoring so they share errors and translations (e.g. Vagrant::Errors::Communicator::ConnectionRefused instead of Vagrant::Errors::SSHConnectionRefused and VagrantPlugins::CommunicatorWinRM::Errors::ConnectionRefused), but I'd rather focus on WinRM and save refactoring for a separate PR.

… configurable

…e differing default values

Conflicts: test/unit/plugins/communicators/winrm/shell_test.rb

sethvargo · 2014-12-15T05:08:45Z

vagrant.gemspec

@@ -28,7 +28,7 @@ Gem::Specification.new do |s|
  s.add_dependency "rb-kqueue", "~> 0.2.0"
  s.add_dependency "rest-client", ">= 1.6.0", "< 2.0"
  s.add_dependency "wdm", "~> 0.1.0"
-  s.add_dependency "winrm", "~> 1.1.3"
+  s.add_dependency "winrm", "= 1.3.0.dev.2"


This scares me. Is there a released version we can depend on?

There is now. @sneal released WInRM 1.3 today. I've updated the gemspec on here and I'm updating my other open PRs related to WinRM.

Note that I've got a few open PRs related to WinRM. I tried to split it to make it a little easier to review...

sethvargo · 2014-12-15T05:09:02Z

This looks really really good, modulo the dependency on a dev version of winrm.

mitchellh · 2014-12-15T06:03:47Z

Agreed with @sethvargo.

maxlinc · 2014-12-16T23:39:48Z

@mitchellh / @sethvargo / @sneal

What about the first note?

Authentication failures are currently retriable, and I haven't changed the default max_times, which was set to 20, with a 10 second sleep. That means WinRM will retry authentication for 10 minutes!

I'm not sure if I:

Should just lower max_tries and/or the sleep time, since the defaults seem too high.
Do something specifically for authentication - either failing on the first auth failure or only allowing a few failures.
Replace max_tries with some other logic. I think the SSH communicator used to have a max_tries option, and it was replaced timeout logic - at least for the initial connection. I'm not sure SSH retries if the connection drops while running commands after the initial connection. WinRM probably should because I think the protocol is less resilient.

sethvargo · 2014-12-17T01:43:31Z

@maxlinc I don't think that's unreasonable for WinRM to be honest...

mitchellh · 2014-12-17T01:45:09Z

@maxlinc Vagrant handles Ctrl-C pretty well, so I think the best logic is to make the wait_for_retry smarter like browser do with page loads: After X minutes, say "WinRM is taking awhile to come online. Sometimes Windows takes this long to boot, but if you were expecting it to boot faster, this can be indicative of a problem. Press Ctrl-C to cancel."

sneal · 2015-01-20T21:52:56Z

plugins/communicators/winrm/shell.rb

+          case exception.status_code
+          # If the error is a 401, we can return a more specific error message
+          when 401
+            raise Errors::AuthenticationFailed,


This shouldn't be needed with WinRM 1.3, it should always raise a WinRMAuthorizationError

…r_handling Conflicts: vagrant.gemspec

sneal · 2015-02-07T23:27:52Z

Testing this out, looks really good so far.

sneal · 2015-02-14T02:04:47Z

@maxlinc Can you fix the merge conflict? This is tested and ready to be merged. Thanks for your efforts here, the error output is much better.

In the future we should make it fail fast if we get an auth error instead of retrying for a couple of minutes. At least now you see an auth error in stdout instead of Vagrant appearing to be frozen.

…r_handling Conflicts: plugins/communicators/winrm/config.rb plugins/communicators/winrm/shell.rb test/unit/plugins/communicators/winrm/shell_test.rb

maxlinc · 2015-02-16T15:37:23Z

@sneal merged.

Improved WinRM error handling (including better `ready?` and `wait_for_ready`)

We gained a ton of improvemnts to WinRM error handling in #4943, but we also got one bug. The new code raises an exception when `winrm_info` does not return right away. This was preventing us from catching the retry/timout logic that's meant to wait until boot_timeout for the WinRM communicator to be ready. This restores the proper behavior by rescuing the WinRMNotReady exception and continuing to retry until the surrounding timeout fires.

pdericson and others added 20 commits December 11, 2014 13:19

Add WinRM over SSL support

a99d32f

Add ssl option to winrm config.rb

4455d24

Add winrmssl port forward (port 5986)

2f52a9c

shell.rb: fix precedence

52d8fdd

config.rb: set default port based on @ssl

243cc5d

Use Winrm 1.2.0 and fix tests

2b9626f

Rename no_ssl_peer_verification to ssl_peer_verification, and make it…

1beb221

… configurable

DRY the Shell: Don't duplicate the Config, especially since there wer…

b348004

…e differing default values

Change @ssl to @transport

072bb26

Fix broken variable references

02f4adc

Change default transport back to :plaintext, for backwards compatibility

2caaf82

Only forward one port for winrm

62ddd92

Fix accessors used by in communicator.rb

24f919c

Use pre-release gem version w/ better errors

e787291

Better error handling for WinRM (using winrm v1.3.0.dev.2)

ba7b964

Just use ready?

24de8a1

Fix tests - all pass but auth retry test is extremely slow

e7e50d3

Fix default forwarded ports

b5a9477

WinRM SSL support

234adaa

Merge branch 'winrmssl' into winrm_error_handling

0b30dcd

Conflicts: test/unit/plugins/communicators/winrm/shell_test.rb

sethvargo reviewed Dec 15, 2014
View reviewed changes

This was referenced Dec 15, 2014

Add WinRM over SSL support - (replaces #3960) #4236

Merged

Linux host / Windows guest support (any provider) #4932

Closed

maxlinc added 2 commits January 20, 2015 16:07

Merge branch 'master' into winrm_error_handling

edc8679

WinRM 1.3 released w/ proper SSL support

2b810c3

Merge branch 'master' into winrm_error_handling

29845e9

sneal reviewed Jan 20, 2015
View reviewed changes

maxlinc added 3 commits January 28, 2015 10:44

Merge branch 'master' of github.com:mitchellh/vagrant into winrm_erro…

c37c125

…r_handling Conflicts: vagrant.gemspec

Change authorization error tests to match WinRM 1.3

5d5e13b

Add retry_delay setting to speed up test

3988295

Merge branch 'master' of github.com:mitchellh/vagrant into winrm_erro…

583254b

…r_handling Conflicts: plugins/communicators/winrm/config.rb plugins/communicators/winrm/shell.rb test/unit/plugins/communicators/winrm/shell_test.rb

sneal added a commit that referenced this pull request Feb 16, 2015

Merge pull request #4943 from maxlinc/winrm_error_handling

92762ee

Improved WinRM error handling (including better `ready?` and `wait_for_ready`)

sneal merged commit 92762ee into hashicorp:master Feb 16, 2015

phinze mentioned this pull request Sep 2, 2015

communicators/winrm: respect boot_timeout when fetching winrm_info #6229

Merged

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved WinRM error handling (including better `ready?` and `wait_for_ready`) #4943

Improved WinRM error handling (including better `ready?` and `wait_for_ready`) #4943

maxlinc commented Dec 11, 2014

sethvargo Dec 15, 2014

maxlinc Jan 20, 2015

sethvargo commented Dec 15, 2014

mitchellh commented Dec 15, 2014

maxlinc commented Dec 16, 2014

sethvargo commented Dec 17, 2014

mitchellh commented Dec 17, 2014

sneal Jan 20, 2015

sneal commented Feb 7, 2015

sneal commented Feb 14, 2015

maxlinc commented Feb 16, 2015

Improved WinRM error handling (including better ready? and wait_for_ready) #4943

Improved WinRM error handling (including better ready? and wait_for_ready) #4943

Conversation

maxlinc commented Dec 11, 2014

sethvargo Dec 15, 2014

Choose a reason for hiding this comment

maxlinc Jan 20, 2015

Choose a reason for hiding this comment

sethvargo commented Dec 15, 2014

mitchellh commented Dec 15, 2014

maxlinc commented Dec 16, 2014

sethvargo commented Dec 17, 2014

mitchellh commented Dec 17, 2014

sneal Jan 20, 2015

Choose a reason for hiding this comment

sneal commented Feb 7, 2015

sneal commented Feb 14, 2015

maxlinc commented Feb 16, 2015

Improved WinRM error handling (including better `ready?` and `wait_for_ready`) #4943

Improved WinRM error handling (including better `ready?` and `wait_for_ready`) #4943