From 6b105d704db70b165fdbbb2bc58ac966899ece33 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 25 Feb 2019 14:11:15 -0800 Subject: [PATCH 1/2] Update communicator upload behavior to handle `/.` path directives This update was prompted by updates in openssh to the scp behavior making source directory paths suffixed with `.` no longer valid resulting in errors on upload. The upload implementation within the ssh communicator has been updated to retain the existing behavior. Included in this update is modifications to the winrm communicator so the upload functionality matches that of the ssh communicator respecting the trailing `.` behavior on source paths. With the communicators updated to properly handle the paths, the file provisioner was also updated to simply apply previously defined path update rules only. Fixes #10675 --- plugins/communicators/ssh/communicator.rb | 49 ++++++++++++++++--- plugins/communicators/winrm/shell.rb | 7 +++ plugins/provisioners/file/provisioner.rb | 41 +++++----------- .../communicators/ssh/communicator_test.rb | 48 ++++++++++++++++-- .../plugins/communicators/winrm/shell_test.rb | 33 +++++++++++-- .../communicators/winssh/communicator_test.rb | 4 +- .../provisioners/file/provisioner_test.rb | 23 ++------- 7 files changed, 141 insertions(+), 64 deletions(-) diff --git a/plugins/communicators/ssh/communicator.rb b/plugins/communicators/ssh/communicator.rb index 9ef4b3e09..373db189a 100644 --- a/plugins/communicators/ssh/communicator.rb +++ b/plugins/communicators/ssh/communicator.rb @@ -9,6 +9,7 @@ require 'log4r' require 'net/ssh' require 'net/ssh/proxy/command' require 'net/scp' +require 'net/sftp' require 'vagrant/util/ansi_escape_code_remover' require 'vagrant/util/file_mode' @@ -290,15 +291,47 @@ module VagrantPlugins def upload(from, to) @logger.debug("Uploading: #{from} to #{to}") - scp_connect do |scp| - if File.directory?(from) - # Recursively upload directories - scp.upload!(from, to, recursive: true) + if File.directory?(from) + if from.end_with?(".") + @logger.debug("Uploading directory contents of: #{from}") + from = from.sub(/\.$/, "") else - # Open file read only to fix issue [GH-1036] - scp.upload!(File.open(from, "r"), to) + @logger.debug("Uploading full directory container of: #{from}") + to = File.join(to, File.basename(File.expand_path(from))) end end + + scp_connect do |scp| + uploader = lambda do |path, remote_dest=nil| + if File.directory?(path) + Dir.new(path).each do |entry| + next if entry == "." || entry == ".." + full_path = File.join(path, entry) + dest = File.join(to, path.sub(/^#{Regexp.escape(from)}/, "")) + create_remote_directory(dest) + uploader.call(full_path, dest) + end + else + if remote_dest + dest = File.join(remote_dest, File.basename(path)) + else + dest = to + if to.end_with?(File::SEPARATOR) + create_remote_directory(dest) + dest = File.join(to, File.basename(path)) + end + end + @logger.debug("Uploading file #{path} to remote #{dest}") + upload_file = File.open(path, "rb") + begin + scp.upload!(upload_file, dest) + ensure + upload_file.close + end + end + end + uploader.call(from) + end rescue RuntimeError => e # Net::SCP raises a runtime error for this so the only way we have # to really catch this exception is to check the message to see if @@ -731,6 +764,10 @@ module VagrantPlugins template.sub("%ENV_KEY%", env_key).sub("%ENV_VALUE%", env_value) + "\n" end + def create_remote_directory(dir) + execute("mkdir -p \"#{dir}\"") + end + def machine_config_ssh @machine.config.ssh end diff --git a/plugins/communicators/winrm/shell.rb b/plugins/communicators/winrm/shell.rb index f515bed9e..61d09475c 100644 --- a/plugins/communicators/winrm/shell.rb +++ b/plugins/communicators/winrm/shell.rb @@ -108,6 +108,13 @@ module VagrantPlugins # @return [FixNum] Total size transfered from host to guest def upload(from, to) file_manager = WinRM::FS::FileManager.new(connection) + if from.is_a?(String) && File.directory?(from) + if from.end_with?(".") + from = from[0, from.length - 1] + else + to = File.join(to, File.basename(File.expand_path(from))) + end + end if from.is_a?(Array) # Preserve return FixNum of bytes transfered return_bytes = 0 diff --git a/plugins/provisioners/file/provisioner.rb b/plugins/provisioners/file/provisioner.rb index e763b54d2..4cc5d7888 100644 --- a/plugins/provisioners/file/provisioner.rb +++ b/plugins/provisioners/file/provisioner.rb @@ -6,40 +6,23 @@ module VagrantPlugins source = File.expand_path(config.source) destination = expand_guest_path(config.destination) - # if source is a directory, make it then trim destination with dirname - # Make sure the remote path exists + # If the source is a directory determine if any path modifications + # need to be applied to the source for upload behavior. If the original + # source value ends with a "." or if the original source does not end + # with a "." but the original destination ends with a file separator + # then append a "." character to the new source. This ensures that + # the contents of the directory are uploaded to the destination and + # not folder itself. if File.directory?(source) - # We need to make sure the actual destination folder - # also exists before uploading, otherwise - # you will get nested folders - # - # https://serverfault.com/questions/538368/make-scp-always-overwrite-or-create-directory - # https://unix.stackexchange.com/questions/292641/get-scp-path-behave-like-rsync-path/292732 - command = "mkdir -p \"%s\"" % destination - if !destination.end_with?(File::SEPARATOR) && - !source.end_with?("#{File::SEPARATOR}.") - # We also need to append a '/.' to the source folder so we copy - # the contents rather than the folder itself, in case a users - # destination folder differs from its source - # - # If source is set as `source/` it will lose the trailing - # slash due to how `File.expand_path` works, so we don't need - # a conditional for that case. - if @machine.config.vm.communicator == :winrm - # windows needs an array of paths because of the - # winrm-fs function Vagrant is using to upload file/folder. - source = Dir["#{source}#{File::SEPARATOR}*"] - else - source << "#{File::SEPARATOR}." - end + if config.source.end_with?(".") || + (!config.destination.end_with?(File::SEPARATOR) && + !config.source.end_with?("#{File::SEPARATOR}.")) + source = File.join(source, ".") end - else - command = "mkdir -p \"%s\"" % File.dirname(destination) end - comm.execute(command) @machine.ui.detail(I18n.t("vagrant.actions.vm.provision.file.locations", - src: source, dst: destination)) + src: config.source, dst: config.destination)) # now upload the file comm.upload(source, destination) end diff --git a/test/unit/plugins/communicators/ssh/communicator_test.rb b/test/unit/plugins/communicators/ssh/communicator_test.rb index d43cec3ea..0cde8b32d 100644 --- a/test/unit/plugins/communicators/ssh/communicator_test.rb +++ b/test/unit/plugins/communicators/ssh/communicator_test.rb @@ -497,12 +497,39 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do describe ".upload" do before do expect(communicator).to receive(:scp_connect).and_yield(scp) + allow(communicator).to receive(:create_remote_directory) end - it "uploads a directory if local path is a directory" do - Dir.mktmpdir('vagrant-test') do |dir| - expect(scp).to receive(:upload!).with(dir, '/destination', recursive: true) - communicator.upload(dir, '/destination') + context "directory uploads" do + let(:test_dir) { @dir } + let(:test_file) { File.join(test_dir, "test-file") } + let(:dir_name) { File.basename(test_dir) } + let(:file_name) { File.basename(test_file) } + + before do + @dir = Dir.mktmpdir("vagrant-test") + FileUtils.touch(test_file) + end + + after { FileUtils.rm_rf(test_dir) } + + it "uploads directory when directory path provided" do + expect(scp).to receive(:upload!).with(instance_of(File), + File.join("", "destination", dir_name, file_name)) + communicator.upload(test_dir, "/destination") + end + + it "uploads contents of directory when dot suffix provided on directory" do + expect(scp).to receive(:upload!).with(instance_of(File), + File.join("", "destination", file_name)) + communicator.upload(File.join(test_dir, "."), "/destination") + end + + it "creates directories before upload" do + expect(communicator).to receive(:create_remote_directory).with( + /#{Regexp.escape(File.join("", "destination", dir_name))}/) + allow(scp).to receive(:upload!) + communicator.upload(test_dir, "/destination") end end @@ -516,6 +543,17 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end end + it "uploads file to directory if destination ends with file separator" do + file = Tempfile.new('vagrant-test') + begin + expect(scp).to receive(:upload!).with(instance_of(File), "/destination/dir/#{File.basename(file.path)}") + expect(communicator).to receive(:create_remote_directory).with("/destination/dir/") + communicator.upload(file.path, "/destination/dir/") + ensure + file.delete + end + end + it "raises custom error on permission errors" do file = Tempfile.new('vagrant-test') begin @@ -609,7 +647,7 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do end it "includes the default cipher array for encryption" do - cipher_array = %w(aes256-ctr aes192-ctr aes128-ctr + cipher_array = %w(aes256-ctr aes192-ctr aes128-ctr aes256-cbc aes192-cbc aes128-cbc rijndael-cbc@lysator.liu.se blowfish-ctr blowfish-cbc cast128-ctr cast128-cbc diff --git a/test/unit/plugins/communicators/winrm/shell_test.rb b/test/unit/plugins/communicators/winrm/shell_test.rb index 0c1a57ca6..57874160d 100644 --- a/test/unit/plugins/communicators/winrm/shell_test.rb +++ b/test/unit/plugins/communicators/winrm/shell_test.rb @@ -33,13 +33,17 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do describe "#upload" do let(:fm) { double("file_manager") } + + before do + allow(WinRM::FS::FileManager).to receive(:new).with(connection) + .and_return(fm) + end + it "should call file_manager.upload for each passed in path" do from = ["/path", "/path/folder", "/path/folder/file.py"] to = "/destination" size = 80 - allow(WinRM::FS::FileManager).to receive(:new).with(connection) - .and_return(fm) allow(fm).to receive(:upload).and_return(size) expect(fm).to receive(:upload).exactly(from.size).times @@ -51,13 +55,34 @@ describe VagrantPlugins::CommunicatorWinRM::WinRMShell do to = "/destination" size = 80 - allow(WinRM::FS::FileManager).to receive(:new).with(connection) - .and_return(fm) allow(fm).to receive(:upload).and_return(size) expect(fm).to receive(:upload).exactly(1).times expect(subject.upload(from, to)).to eq(size) end + + context "when source is a directory" do + let(:source) { "path/sourcedir" } + + before do + allow(File).to receive(:directory?).with(/#{Regexp.escape(source)}/).and_return(true) + end + + it "should add source directory name to destination" do + expect(fm).to receive(:upload) do |from, to| + expect(to).to include("sourcedir") + end + subject.upload(source, "/dest") + end + + it "should not add source directory name to destination when source ends with '.'" do + source << "/." + expect(fm).to receive(:upload) do |from, to| + expect(to).to eq("/dest") + end + subject.upload(source, "/dest") + end + end end describe ".powershell" do diff --git a/test/unit/plugins/communicators/winssh/communicator_test.rb b/test/unit/plugins/communicators/winssh/communicator_test.rb index f0d36e825..8da9b2196 100644 --- a/test/unit/plugins/communicators/winssh/communicator_test.rb +++ b/test/unit/plugins/communicators/winssh/communicator_test.rb @@ -226,12 +226,14 @@ describe VagrantPlugins::CommunicatorWinSSH::Communicator do describe ".upload" do before do + allow(communicator).to receive(:create_remote_directory) expect(communicator).to receive(:scp_connect).and_yield(scp) end it "uploads a directory if local path is a directory" do Dir.mktmpdir('vagrant-test') do |dir| - expect(scp).to receive(:upload!).with(dir, 'C:\destination', recursive: true) + FileUtils.touch(File.join(dir, "test-file")) + expect(scp).to receive(:upload!).with(an_instance_of(File), /test-file/) communicator.upload(dir, 'C:\destination') end end diff --git a/test/unit/plugins/provisioners/file/provisioner_test.rb b/test/unit/plugins/provisioners/file/provisioner_test.rb index 0aedb8fe8..7f3c954ba 100644 --- a/test/unit/plugins/provisioners/file/provisioner_test.rb +++ b/test/unit/plugins/provisioners/file/provisioner_test.rb @@ -34,8 +34,6 @@ describe VagrantPlugins::FileUpload::Provisioner do allow(config).to receive(:source).and_return("/source") allow(config).to receive(:destination).and_return("/foo/bar") - expect(communicator).to receive(:execute).with("mkdir -p \"/foo\"") - subject.provision end @@ -43,8 +41,6 @@ describe VagrantPlugins::FileUpload::Provisioner do allow(config).to receive(:source).and_return("/source") allow(config).to receive(:destination).and_return("/foo bar/bar") - expect(communicator).to receive(:execute).with("mkdir -p \"/foo bar\"") - subject.provision end @@ -52,8 +48,6 @@ describe VagrantPlugins::FileUpload::Provisioner do allow(config).to receive(:source).and_return("/source/file.sh") allow(config).to receive(:destination).and_return("/foo/bar/file.sh") - expect(communicator).to receive(:execute).with("mkdir -p \"/foo/bar\"") - subject.provision end @@ -105,21 +99,12 @@ describe VagrantPlugins::FileUpload::Provisioner do subject.provision end - it "sends an array of files and folders if winrm and destination doesn't end with file separator" do - files = ["/source/file.py", "/source/folder"] - allow(Dir).to receive(:[]).and_return(files) - allow(config).to receive(:source).and_return("/source") - allow(config).to receive(:destination).and_return("/foo/bar") + it "appends a '/.' to expanded source if defined in original source" do + allow(config).to receive(:source).and_return("/source/.") allow(File).to receive(:directory?).with("/source").and_return(true) - allow(machine.config.vm).to receive(:communicator).and_return(:winrm) + allow(config).to receive(:destination).and_return("/foo/bar") - expect(guest).to receive(:capability?). - with(:shell_expand_guest_path).and_return(true) - expect(guest).to receive(:capability). - with(:shell_expand_guest_path, "/foo/bar").and_return("/foo/bar") - - expect(communicator).to receive(:upload) - .with(files, "/foo/bar") + expect(communicator).to receive(:upload).with("/source/.", "/foo/bar") subject.provision end From e2b6a6645c8e77729400af330be395bc2631c0aa Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Tue, 26 Feb 2019 08:54:49 -0800 Subject: [PATCH 2/2] Always ensure remote destination directory exists --- plugins/communicators/ssh/communicator.rb | 3 ++- .../plugins/communicators/ssh/communicator_test.rb | 13 ++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/plugins/communicators/ssh/communicator.rb b/plugins/communicators/ssh/communicator.rb index 373db189a..47d7b9e84 100644 --- a/plugins/communicators/ssh/communicator.rb +++ b/plugins/communicators/ssh/communicator.rb @@ -317,10 +317,11 @@ module VagrantPlugins else dest = to if to.end_with?(File::SEPARATOR) - create_remote_directory(dest) dest = File.join(to, File.basename(path)) end end + @logger.debug("Ensuring remote directory exists for destination upload") + create_remote_directory(File.dirname(dest)) @logger.debug("Uploading file #{path} to remote #{dest}") upload_file = File.open(path, "rb") begin diff --git a/test/unit/plugins/communicators/ssh/communicator_test.rb b/test/unit/plugins/communicators/ssh/communicator_test.rb index 0cde8b32d..f1c8df03b 100644 --- a/test/unit/plugins/communicators/ssh/communicator_test.rb +++ b/test/unit/plugins/communicators/ssh/communicator_test.rb @@ -547,13 +547,24 @@ describe VagrantPlugins::CommunicatorSSH::Communicator do file = Tempfile.new('vagrant-test') begin expect(scp).to receive(:upload!).with(instance_of(File), "/destination/dir/#{File.basename(file.path)}") - expect(communicator).to receive(:create_remote_directory).with("/destination/dir/") + expect(communicator).to receive(:create_remote_directory).with("/destination/dir") communicator.upload(file.path, "/destination/dir/") ensure file.delete end end + it "creates remote directory path to destination on upload" do + file = Tempfile.new('vagrant-test') + begin + expect(scp).to receive(:upload!).with(instance_of(File), "/destination/dir/file.txt") + expect(communicator).to receive(:create_remote_directory).with("/destination/dir") + communicator.upload(file.path, "/destination/dir/file.txt") + ensure + file.delete + end + end + it "raises custom error on permission errors" do file = Tempfile.new('vagrant-test') begin