Some small efficiency gains plus controlmaster for rsync #440

Merged
merged 9 commits into from Jul 2, 2015

Projects

None yet

3 participants

@matschaffer
Owner

This seems to speed things up a bit and only relies on a single connection for all the rsync calls.

Fixes #365

Connected to #32

Posted to https://www.antipattern.io/code-reviews/25

added some commits Jun 22, 2015
@matschaffer Avoid re-checking if we're on windows a02b18e
@matschaffer Initial cut at control sockets for rsync f28a711
@matschaffer Actually stop checking for windows nodes f0ca029
@matschaffer Use ControlMaster=auto
Yes doesn't actually buy any speed since it opens & closes the socket every time.
589c65c
@matschaffer Make control master configurable
7755815
added some commits Jun 22, 2015
@matschaffer Pull out whitespace a96fba0
@matschaffer Add `-zt` for potentially faster rsync
Fixes #32
667b309
@ook
ook commented Jun 30, 2015

I know nothing about Chef but I'm rather surprised to see nothing about command-line escape. My first move would be to check if Chef does command line escapation and add something like Shellwords to protect you commands from weird sideeffects.

@matschaffer
Owner

It is actually mostly escaped since it's assembled as an array. The thing that's not is https://github.com/matschaffer/knife-solo/pull/440/files#diff-1b457fb6c7ed7da433ee9d90e66281a7R270 since it gets embedded as an arg itself.

I figured I can't just escape quotes since I don't know what will be used to wrap the arg I'm building (if anything).

Fair point though that I should probably see if there's some way to escape that.

On the plus side anything that would go in there is going to be something from the user's own config files or arguments so I think the bigger risk is errors & confusion rather than security.

@matschaffer
Owner

Oh, interesting! Looks like rsync itself protects against shell injection:

 rsync --rsh="ssh localhost; echo hi" a localhost:b
ssh: Could not resolve hostname localhost;: nodename nor servname provided, or not known
rsync: connection unexpectedly closed (0 bytes received so far) [sender]
rsync error: unexplained error (code 255) at /SourceCache/rsync/rsync-45/rsync/io.c(453) [sender=2.6.9]

~
❯ rsync --rsh="ssh localhost\"; echo hi" a localhost:b
Missing trailing-" in remote-shell command.
rsync error: syntax or usage error (code 1) at /SourceCache/rsync/rsync-45/rsync/main.c(351) [sender=2.6.9]

~
❯ rsync --rsh="ssh localh'ost\"; echo hi" a localhost:b
Missing trailing-' in remote-shell command.
rsync error: syntax or usage error (code 1) at /SourceCache/rsync/rsync-45/rsync/main.c(351) [sender=2.6.9]

~
❯ rsync --rsh="ssh localh'ost; echo hi" a localhost:b
Missing trailing-' in remote-shell command.
rsync error: syntax or usage error (code 1) at /SourceCache/rsync/rsync-45/rsync/main.c(351) [sender=2.6.9]
@ook
ook commented Jun 30, 2015

Nice to know.

@midwire
midwire commented Jun 30, 2015

Mat, I think it looks good in general and there's nothing wrong IMO with doing command line things using arrays like that. I've done it many times. If you feel averted you may want to check out: https://github.com/jbussdieker/ruby-rsync which wraps the RSync binary.

I'll comment on a few things I noticed but it looks good overall, IMO.

@midwire midwire commented on the diff Jun 30, 2015
lib/chef/knife/solo_cook.rb
if config[:ssh_gateway]
ssh_command = "ssh -TA #{config[:ssh_gateway]} ssh -T -o StrictHostKeyChecking=no #{ssh_args}"
else
ssh_command = "ssh #{ssh_args}"
end
- cmd = ['rsync', '-rL', rsync_debug, rsync_permissions, %Q{--rsh=#{ssh_command}}, extra_opts]
+ cmd = ['rsync', '-rL', rsync_debug, rsync_permissions, %Q{--rsh=#{ssh_command}}]
+ cmd += extra_opts
@midwire
midwire Jun 30, 2015

Why '+=' instead of using the insertion operator? I would tend to be consistent and use one or the other.

@matschaffer
matschaffer Jul 1, 2015 Owner

There right-hand sides are different (list vs scalar):

2.2.2 :001 > part1 = [1,2,3]
 => [1, 2, 3]
2.2.2 :002 > part2 = [3,4,5]
 => [3, 4, 5]
2.2.2 :003 > part1 += part2
 => [1, 2, 3, 3, 4, 5]
2.2.2 :004 > part1
 => [1, 2, 3, 3, 4, 5]
2.2.2 :005 > part1 = [1,2,3]
 => [1, 2, 3]
2.2.2 :006 > part1 << part2
 => [1, 2, 3, [3, 4, 5]]

Though point taken on consistency. If it really looks like the cmd.flatten.compact is necessary I could << the whole thing. Otherwise I'll switch everything to be +=.

Thanks!

@midwire midwire commented on an outdated diff Jun 30, 2015
lib/knife-solo/ssh_command.rb
- [host_arg, config_arg, ident_arg, forward_arg, port_arg, knownhosts_arg, stricthosts_arg].compact.join(' ')
+ args << "-F #{config[:ssh_config]}" if config[:ssh_config]
+ args << "-i #{config[:identity_file]}" if config[:identity_file]
+ args << "-o ForwardAgent=yes" if config[:forward_agent]
+ args << "-p #{config[:ssh_port]}" if config[:ssh_port]
+ args << "-o UserKnownHostsFile=#{connection_options[:user_known_hosts_file]}" if config[:host_key_verify] == false
+ args << "-o StrictHostKeyChecking=no" if config[:host_key_verify] == false
+ args << "-o ControlMaster=auto -o ControlPath=#{ssh_control_path} -o ControlPersist=3600" unless config[:ssh_control_master] == "no"
+
+ args.join(' ')
+ end
+
+ def ssh_control_path
+ dir = File.join(ENV['HOME'], '.chef', 'knife-solo-sockets')
+ FileUtils.mkpath(dir) unless File.exist?(dir)
@midwire
midwire Jun 30, 2015

You could just do:

FileUtils.mkpath_p(dir)

and avoid the unless

added some commits Jul 1, 2015
@matschaffer Change fileutils call based on feedback
a51f585
@matschaffer More code consistency in rsync command build step
6045889
@matschaffer matschaffer merged commit 8be45ab into master Jul 2, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@matschaffer matschaffer removed the in progress label Jul 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment