Merge pull request #10698 from chrisroberts/f-scp

Update communicator upload behavior to handle `/.` path directives

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,48 @@ 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)
+                  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
+                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 +765,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

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

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

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,28 @@ 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 "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
@@ -609,7 +658,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

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

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

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