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

Use named pipe to decrease local Windows runtime #220

Merged
merged 26 commits into from
Dec 5, 2017
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
c8a8442
Use named pipe to decrease local Windows runtime
jerryaldrichiii Nov 21, 2017
4540c4b
Modify named pipe creation to use unique name
jerryaldrichiii Nov 22, 2017
48fa142
Remove `$ProgressPreference = 'SilentlyContinue'`
jerryaldrichiii Nov 22, 2017
bed61c5
Modify code to persist pipe usage
jerryaldrichiii Nov 22, 2017
fe773e6
Removed pipe argument (default is In/Out)
jerryaldrichiii Nov 22, 2017
ccd1a8d
Privatize pipe related functions
jerryaldrichiii Nov 23, 2017
8f2437b
Add fallback to not used pipe if creation fails
jerryaldrichiii Nov 23, 2017
82ffa18
Move Windows local command logic to separate class
jerryaldrichiii Nov 27, 2017
776b8da
Revert file code format
jerryaldrichiii Nov 27, 2017
44a7a6a
Change `@platform` to `@os` for consistency
jerryaldrichiii Nov 27, 2017
9681fe6
Split Windows runners and create a `GenericRunner`
jerryaldrichiii Nov 27, 2017
d1ad987
Add test for Windows named pipe usage
jerryaldrichiii Nov 27, 2017
18a4245
Tweak `GenericRunner` and default `run_command`
jerryaldrichiii Nov 27, 2017
d3b6f86
Remove `Local::` from `CommandWrapper`
jerryaldrichiii Nov 27, 2017
bd2c7c3
Fix test for Windows named pipe
jerryaldrichiii Nov 27, 2017
e7d7d32
Make changes to integrate @jquick's cacheing magic
jerryaldrichiii Nov 27, 2017
ecad39f
Remove unneeded logic for opening an existing pipe
jerryaldrichiii Nov 27, 2017
7517045
Add comments explaining pipe check tests
jerryaldrichiii Nov 30, 2017
d494df2
Change `Shellout` to `ShellOut`
jerryaldrichiii Nov 30, 2017
ab34ccc
Change `stubs` to `expects` where appropriate
jerryaldrichiii Nov 30, 2017
824f2e3
Modify pipe tests
jerryaldrichiii Nov 30, 2017
9965ef8
Move/Modify Unit tests and add Integration tests
jerryaldrichiii Dec 1, 2017
e63627f
Fix local tests when running with others.
jquick Dec 2, 2017
17d9de7
Minor style changes
jerryaldrichiii Dec 3, 2017
6160bde
Remove extra newline between `let`s
jerryaldrichiii Dec 4, 2017
0bfd1c1
Modify TransportHelper to accept platform options
jerryaldrichiii Dec 4, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 0 additions & 41 deletions lib/train/extras/command_wrapper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -121,43 +121,6 @@ def safe_string(str)
end
end

# this is required if you run locally on windows,
# winrm connections provide a PowerShell shell automatically
# TODO: only activate in local mode
class PowerShellCommand < CommandWrapperBase
Train::Options.attach(self)

def initialize(backend, options)
@backend = backend
validate_options(options)
end

def run(script)
# wrap the script to ensure we always run it via powershell
# especially in local mode, we cannot be sure that we get a Powershell
# we may just get a `cmd`.
# TODO: we may want to opt for powershell.exe -command instead of `encodeCommand`
"powershell -NoProfile -encodedCommand #{encoded(safe_script(script))}"
end

# suppress the progress stream from leaking to stderr
def safe_script(script)
"$ProgressPreference='SilentlyContinue';" + script
end

# Encodes the script so that it can be passed to the PowerShell
# --EncodedCommand argument.
# @return [String] The UTF-16LE base64 encoded script
def encoded(script)
encoded_script = safe_script(script).encode('UTF-16LE', 'UTF-8')
Base64.strict_encode64(encoded_script)
end

def to_s
'PowerShell CommandWrapper'
end
end

class CommandWrapper
include_options LinuxCommand

Expand All @@ -168,10 +131,6 @@ def self.load(transport, options)
msg = res.verify
fail Train::UserError, "Sudo failed: #{msg}" unless msg.nil?
res
# only use powershell command for local transport. winrm transport
# uses powershell as default
elsif transport.os.windows? && transport.class == Train::Transports::Local::Connection
PowerShellCommand.new(transport, options)
end
end
end
Expand Down
171 changes: 161 additions & 10 deletions lib/train/transports/local.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,32 @@ class Local < Train.plugin(1)

include_options Train::Extras::CommandWrapper

class PipeError < ::StandardError; end

def connection(_ = nil)
@connection ||= Connection.new(@options)
end

class Connection < BaseConnection
def initialize(options)
super(options)
@cmd_wrapper = nil
@cmd_wrapper = CommandWrapper.load(self, options)

# While OS is being discovered, use the GenericRunner
@runner = GenericRunner.new
@runner.cmd_wrapper = CommandWrapper.load(self, options)

if os.windows?
# Attempt to use a named pipe but fallback to ShellOut if that fails
begin
@runner = WindowsPipeRunner.new
rescue PipeError
@runner = WindowsShellRunner.new
end
end
end

def local?
true
end

def login_command
Expand All @@ -31,17 +48,10 @@ def uri
'local://'
end

def local?
true
end

private

def run_command_via_connection(cmd)
cmd = @cmd_wrapper.run(cmd) unless @cmd_wrapper.nil?
res = Mixlib::ShellOut.new(cmd)
res.run_command
CommandResult.new(res.stdout, res.stderr, res.exitstatus)
@runner.run_command(cmd)
rescue Errno::ENOENT => _
CommandResult.new('', '', 1)
end
Expand All @@ -53,6 +63,147 @@ def file_via_connection(path)
Train::File::Local::Unix.new(self, path)
end
end

class GenericRunner
attr_writer :cmd_wrapper

def run_command(cmd)
if defined?(@cmd_wrapper) && !@cmd_wrapper.nil?
cmd = @cmd_wrapper.run(cmd)
end

res = Mixlib::ShellOut.new(cmd)
res.run_command
Local::CommandResult.new(res.stdout, res.stderr, res.exitstatus)
end
end

class WindowsShellRunner
require 'json'
require 'base64'

def run_command(script)
# Prevent progress stream from leaking into stderr
script = "$ProgressPreference='SilentlyContinue';" + script

# Encode script so PowerShell can use it
script = script.encode('UTF-16LE', 'UTF-8')
base64_script = Base64.strict_encode64(script)

cmd = "powershell -NoProfile -EncodedCommand #{base64_script}"

res = Mixlib::ShellOut.new(cmd)
res.run_command
Local::CommandResult.new(res.stdout, res.stderr, res.exitstatus)
end
end

class WindowsPipeRunner
require 'json'
require 'base64'
require 'securerandom'

def initialize
@pipe = acquire_pipe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just throw a train exception that can be handled and we have a proper fallback, which would be the WindowsShellRunner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! I make it so in the latest commits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much cleaner now, thank you!

fail PipeError if @pipe.nil?
end

def run_command(cmd)
script = "$ProgressPreference='SilentlyContinue';" + cmd
encoded_script = Base64.strict_encode64(script)
@pipe.puts(encoded_script)
@pipe.flush
res = OpenStruct.new(JSON.parse(Base64.decode64(@pipe.readline)))
Local::CommandResult.new(res.stdout, res.stderr, res.exitstatus)
end

private

def acquire_pipe
current_pipe = Dir.entries('//./pipe/').find { |f| f =~ /inspec_/ }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned about this logic. What if there's another InSpec running on the host at the same time this one attempts to acquire a pipe? What if there's two? We could potentially find a pipe that's not ours and start using it only to have it disappear between the time we've acquired it and the time we go to use it.

Maybe my lack of knowledge around windows and named pipes is misleading my judgment here... but wouldn't it be better if we stored the name of the pipe we intend to use, look for it when we want to use it, and then if it's not there or not usable for some reason, create it? Some half-baked code to illustrate my idea...

def pipe_name
  @pipe_name ||= "inspec_#{SecureRandom.hex}"
end

def reset_pipe_name
  @pipe_name = nil
end

def acquire_pipe
  pipe = open("//./pipe/#{pipe_name}", ...)
rescue
  create_pipe
end

def create_pipe
  # don't try to create a pipe of the same name if it exists but we can't open it
  reset_pipe_name if File.exist?(pipe_name)

  # do the creation and stuff, return nil if we can't create
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the logic either.

Good news, while trying to implement your solution I realized something. We don't need to open an existing pipe! The acquire_pipe method only gets called once per object so there is no need to attempt to open an existing pipe. The pipe shouldn't be shared between separate processes in any scenario.

I'll get that fixed.


pipe = nil
if current_pipe.nil?
pipe = create_pipe("inspec_#{SecureRandom.hex}")
else
begin
pipe = open("//./pipe/#{current_pipe}", 'r+')
rescue
# Pipes are closed when a Train connection ends. When running
# multiple independent scans (e.g. Unit tests) the pipe will be
# unavailable because the previous process is closing it.
# This creates a new pipe in that case
pipe = create_pipe("inspec_#{SecureRandom.hex}")
end
end

pipe
end

def create_pipe(pipe_name)
start_pipe_server(pipe_name)

pipe = nil

# PowerShell needs time to create pipe.
100.times do
begin
pipe = open("//./pipe/#{pipe_name}", 'r+')
break
rescue
sleep 0.1
end
end

pipe
end

def start_pipe_server(pipe_name)
require 'win32/process'

script = <<-EOF
$ErrorActionPreference = 'Stop'

$pipeServer = New-Object System.IO.Pipes.NamedPipeServerStream('#{pipe_name}')
$pipeReader = New-Object System.IO.StreamReader($pipeServer)
$pipeWriter = New-Object System.IO.StreamWriter($pipeServer)

$pipeServer.WaitForConnection()

# Create loop to receive and process user commands/scripts
$clientConnected = $true
while($clientConnected) {
$input = $pipeReader.ReadLine()
$command = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String($input))

# Execute user command/script and convert result to JSON
$scriptBlock = $ExecutionContext.InvokeCommand.NewScriptBlock($command)
try {
$stdout = & $scriptBlock | Out-String
$result = @{ 'stdout' = $stdout ; 'stderr' = ''; 'exitstatus' = 0 }
} catch {
$stderr = $_ | Out-String
$result = @{ 'stdout' = ''; 'stderr' = $_; 'exitstatus' = 1 }
}
$resultJSON = $result | ConvertTo-JSON

# Encode JSON in Base64 and write to pipe
$encodedResult = [System.Convert]::ToBase64String([System.Text.Encoding]::UTF8.GetBytes($resultJSON))
$pipeWriter.WriteLine($encodedResult)
$pipeWriter.Flush()
}
EOF

utf8_script = script.encode('UTF-16LE', 'UTF-8')
base64_script = Base64.strict_encode64(utf8_script)
cmd = "powershell -NoProfile -ExecutionPolicy bypass -NonInteractive -EncodedCommand #{base64_script}"

server_pid = Process.create(command_line: cmd).process_id

# Ensure process is killed when the Train process exits
at_exit { Process.kill('KILL', server_pid) }
end
end
end
end
end
27 changes: 27 additions & 0 deletions test/windows/local_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,33 @@
cmd.stderr.must_equal ''
end

it 'uses a named pipe if available' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything obvious in this test that actually tests that the named pipe is used. How are you asserting that in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point, I added words to the comments to make it more clear. Basically, it runs a PowerShell command and checks if the pipe was created by looking for the file that would be created.

# Must call `:conn` early so we can stub `SecureRandom`
connection = conn

SecureRandom.stubs(:hex).returns('with_pipe')
cmd = connection.run_command('Get-ChildItem //./pipe/ | Where-Object { $_.Name -Match "inspec_with_pipe" }')
cmd.stdout.wont_be_nil
cmd.stderr.must_equal ''
end

it 'when named pipe is not available it runs `Mixlib::Shellout`' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything obvious in this test that actually tests that Mixlib::ShellOut is used instead of the named pipe. How are you asserting that in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point, I added words to the comments to make it more clear. Basically, it runs a PowerShell command and that command checks to see if a pipe exists. If it does then we know it used the pipe and not Mixlib::ShellOut.

# Must call `:conn` early so we can stub `:acquire_pipe`
connection = conn

# Prevent named pipe from being created
Train::Transports::Local::Connection::WindowsPipeRunner
.any_instance
.stubs(:acquire_pipe)
.returns(nil)

# Verify pipe was not created
SecureRandom.stubs(:hex).returns('minitest')
cmd = connection.run_command('Get-ChildItem //./pipe/ | Where-Object { $_.Name -Match "inspec_minitest" }')
cmd.stdout.must_equal ''
cmd.stderr.must_equal ''
end

describe 'file' do
before do
@temp = Tempfile.new('foo')
Expand Down