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

@@ -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

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,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

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