From f912a81362140a72e6be7c3dd85f48994ea6c4d6 Mon Sep 17 00:00:00 2001 From: Matt Wrock Date: Sun, 24 Jan 2016 23:33:16 -0800 Subject: [PATCH] powershell and cmd calls should use commnand_executor to reuse oprn winrm shell --- plugins/communicators/winrm/communicator.rb | 1 + plugins/communicators/winrm/shell.rb | 74 +++++++++---------- .../plugins/communicators/winrm/shell_test.rb | 43 +++++++---- 3 files changed, 65 insertions(+), 53 deletions(-) diff --git a/plugins/communicators/winrm/communicator.rb b/plugins/communicators/winrm/communicator.rb index e22a4f9b0..fe884d272 100644 --- a/plugins/communicators/winrm/communicator.rb +++ b/plugins/communicators/winrm/communicator.rb @@ -142,6 +142,7 @@ module VagrantPlugins opts[:good_exit] = Array(opts[:good_exit]) command = wrap_in_scheduled_task(command, opts[:interactive]) if opts[:elevated] + @logger.debug("#{opts[:shell]} executing:\n#{command}") output = shell.send(opts[:shell], command, &block) execution_output(output, opts) end diff --git a/plugins/communicators/winrm/shell.rb b/plugins/communicators/winrm/shell.rb index 1a8d7906d..f55c8fb75 100644 --- a/plugins/communicators/winrm/shell.rb +++ b/plugins/communicators/winrm/shell.rb @@ -54,20 +54,21 @@ module VagrantPlugins end def powershell(command, &block) - # Suppress the progress stream from leaking to stderr - command = "$ProgressPreference='SilentlyContinue';\r\n" + command - command << "\r\n" # Ensure an exit code - command << "if ($?) { exit 0 } else { if($LASTEXITCODE) { exit $LASTEXITCODE } else { exit 1 } }" - execute_shell(command, :powershell, &block) + command += "\r\nif ($?) { exit 0 } else { if($LASTEXITCODE) { exit $LASTEXITCODE } else { exit 1 } }" + execute_with_rescue(executor.method("run_powershell_script"), command, &block) end def cmd(command, &block) - execute_shell(command, :cmd, &block) + execute_with_rescue(executor.method("run_cmd"), command, &block) end def wql(query, &block) - execute_shell(query, :wql, &block) + retryable(tries: @config.max_tries, on: @@exceptions_to_retry_on, sleep: @config.retry_delay) do + handle_output(session.method("run_wql"), query, &block) + end + rescue => e + raise_winrm_exception(e, "run_wql", query) end def upload(from, to) @@ -82,42 +83,35 @@ module VagrantPlugins protected - def execute_shell(command, shell=:powershell, &block) - raise Errors::WinRMInvalidShell, shell: shell unless [:powershell, :cmd, :wql].include?(shell) - - begin - execute_shell_with_retry(command, shell, &block) - rescue => e - raise_winrm_exception(e, shell, command) - end + def execute_with_rescue(method, command, &block) + handle_output(method, command, &block) + rescue => e + raise_winrm_exception(e, method.name, command) end - def execute_shell_with_retry(command, shell, &block) - retryable(tries: @config.max_tries, on: @@exceptions_to_retry_on, sleep: @config.retry_delay) do - @logger.debug("#{shell} executing:\n#{command}") - output = session.send(shell, command) do |out, err| - block.call(:stdout, out) if block_given? && out - block.call(:stderr, err) if block_given? && err - end + def handle_output(execute_method, command, &block) + output = execute_method.call(command) do |out, err| + block.call(:stdout, out) if block_given? && out + block.call(:stderr, err) if block_given? && err + end - @logger.debug("Output: #{output.inspect}") + @logger.debug("Output: #{output.inspect}") - # Verify that we didn't get a parser error, and if so we should - # set the exit code to 1. Parse errors return exit code 0 so we - # need to do this. - if output[:exitcode] == 0 - (output[:data] || []).each do |data| - next if !data[:stderr] - if data[:stderr].include?("ParserError") - @logger.warn("Detected ParserError, setting exit code to 1") - output[:exitcode] = 1 - break - end + # Verify that we didn't get a parser error, and if so we should + # set the exit code to 1. Parse errors return exit code 0 so we + # need to do this. + if output[:exitcode] == 0 + (output[:data] || []).each do |data| + next if !data[:stderr] + if data[:stderr].include?("ParserError") + @logger.warn("Detected ParserError, setting exit code to 1") + output[:exitcode] = 1 + break end end - - return output end + + return output end def raise_winrm_exception(exception, shell = nil, command = nil) @@ -178,6 +172,10 @@ module VagrantPlugins @session ||= new_session end + def executor + @executor ||= session.create_executor + end + def endpoint case @config.transport.to_sym when :ssl @@ -195,7 +193,9 @@ module VagrantPlugins host: @host, port: @port, basic_auth_only: @config.basic_auth_only, - no_ssl_peer_verification: !@config.ssl_peer_verification } + no_ssl_peer_verification: !@config.ssl_peer_verification, + retry_delay: @config.retry_delay, + retry_limit: @config.max_tries } end end #WinShell class end diff --git a/test/unit/plugins/communicators/winrm/shell_test.rb b/test/unit/plugins/communicators/winrm/shell_test.rb index 1ad92802e..5cf1f6810 100644 --- a/test/unit/plugins/communicators/winrm/shell_test.rb +++ b/test/unit/plugins/communicators/winrm/shell_test.rb @@ -6,7 +6,8 @@ require Vagrant.source_root.join("plugins/communicators/winrm/config") describe VagrantPlugins::CommunicatorWinRM::WinRMShell do include_context "unit" - let(:session) { double("winrm_session") } + let(:session) { double("winrm_session", create_executor: executor) } + let(:executor) { double("command_executor") } let(:port) { config.transport == :ssl ? 5986 : 5985 } let(:config) { VagrantPlugins::CommunicatorWinRM::Config.new.tap do |c| @@ -15,6 +16,8 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do c.max_tries = 3 c.retry_delay = 0 c.basic_auth_only = false + c.retry_delay = 1 + c.max_tries = 2 c.finalize! end } @@ -27,23 +30,12 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do describe ".powershell" do it "should call winrm powershell" do - expect(session).to receive(:powershell).with(/^dir.+/).and_return({ exitcode: 0 }) + expect(executor).to receive(:run_powershell_script).with(/^dir.+/).and_return({ exitcode: 0 }) expect(subject.powershell("dir")[:exitcode]).to eq(0) end - it "should retry when a WinRMAuthorizationError is received" do - expect(session).to receive(:powershell).with(/^dir.+/).exactly(3).times.and_raise( - # Note: The initialize for WinRMAuthorizationError may require a status_code as - # the second argument in a future WinRM release. Currently it doesn't track the - # status code. - WinRM::WinRMAuthorizationError.new("Oh no!! Unauthrorized") - ) - expect { subject.powershell("dir") }.to raise_error( - VagrantPlugins::CommunicatorWinRM::Errors::AuthenticationFailed) - end - it "should raise an execution error when an exception occurs" do - expect(session).to receive(:powershell).with(/^dir.+/).and_raise( + expect(executor).to receive(:run_powershell_script).with(/^dir.+/).and_raise( StandardError.new("Oh no! a 500 SOAP error!")) expect { subject.powershell("dir") }.to raise_error( VagrantPlugins::CommunicatorWinRM::Errors::ExecutionError) @@ -52,11 +44,29 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do describe ".cmd" do it "should call winrm cmd" do - expect(session).to receive(:cmd).with("dir").and_return({ exitcode: 0 }) + expect(executor).to receive(:run_cmd).with("dir").and_return({ exitcode: 0 }) expect(subject.cmd("dir")[:exitcode]).to eq(0) end end + describe ".wql" do + it "should call winrm wql" do + expect(session).to receive(:run_wql).with("select * from Win32_OperatingSystem").and_return({ exitcode: 0 }) + expect(subject.wql("select * from Win32_OperatingSystem")[:exitcode]).to eq(0) + end + + it "should retry when a WinRMAuthorizationError is received" do + expect(session).to receive(:run_wql).with("select * from Win32_OperatingSystem").exactly(2).times.and_raise( + # Note: The initialize for WinRMAuthorizationError may require a status_code as + # the second argument in a future WinRM release. Currently it doesn't track the + # status code. + WinRM::WinRMAuthorizationError.new("Oh no!! Unauthrorized") + ) + expect { subject.wql("select * from Win32_OperatingSystem") }.to raise_error( + VagrantPlugins::CommunicatorWinRM::Errors::AuthenticationFailed) + end + end + describe ".endpoint" do context 'when transport is :ssl' do let(:config) { @@ -93,7 +103,8 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do it "should create endpoint options" do expect(subject.send(:endpoint_options)).to eq( { user: "username", pass: "password", host: "localhost", port: 5985, - basic_auth_only: false, no_ssl_peer_verification: false }) + basic_auth_only: false, no_ssl_peer_verification: false, + retry_delay: 1, retry_limit: 2 }) end end