Move proxy_command to config and support templating (#1226)

Migrate the proxy_command specification to the config and add support
for user override template to be used for edge cases. Moving it to the
config allows mistakes in the interpolation to be caught before the
machine is brought up.

Note this uses a more restrictive replacement to avoid requiring
escaping of '%' or '$'.

Issue #921 already partially resolved thanks to @ElArtista, this
completes the fix by allowing users to override as needed.

Fixes: #921

README.md

@@ -400,6 +400,13 @@ URI](http://libvirt.org/uri.html):
   Default is `$HOME/.ssh/id_rsa`. Prepends `$HOME/.ssh/` if no directory
 * `socket` - Path to the Libvirt unix socket (e.g.
   `/var/run/libvirt/libvirt-sock`)
+* `proxy_command` - For advanced usage. When connecting to remote libvirt
+  instances, if the default constructed proxy\_command which uses `-W %h:%p`
+  does not work, set this as needed. It performs interpolation using `{key}`
+  and supports only `{host}`, `{username}`, and `{id_ssh_key_file}`. This is
+  to try and avoid issues with escaping `%` and `$` which might be necessary
+  to the ssh command itself. e.g.:
+  `libvirt.proxy_command = "ssh {host} -l {username} -i {id_ssh_key_file} nc %h %p"`
 * `uri` - For advanced usage. Directly specifies what Libvirt connection URI
   vagrant-libvirt should use. Overrides all other connection configuration
   options

lib/vagrant-libvirt/action/forward_ports.rb

@@ -107,7 +107,7 @@ module VagrantPlugins
                 "IdentityFile='\"#{pk}\"'"
               end).map { |s| s.prepend('-o ') }.join(' ')
 
-          options += " -o ProxyCommand=\"#{ssh_info[:proxy_command]}\"" if machine.provider_config.connect_via_ssh
+          options += " -o ProxyCommand=\"#{ssh_info[:proxy_command]}\"" if machine.provider_config.proxy_command
 
           # TODO: instead of this, try and lock and get the stdin from spawn...
           ssh_cmd = ''

lib/vagrant-libvirt/config.rb

@@ -37,6 +37,8 @@ module VagrantPlugins
       # ID SSH key file
       attr_accessor :id_ssh_key_file
 
+      attr_accessor :proxy_command
+
       # Libvirt storage pool name, where box image and instance snapshots will
       # be stored.
       attr_accessor :storage_pool_name
@@ -192,6 +194,7 @@ module VagrantPlugins
         @password          = UNSET_VALUE
         @id_ssh_key_file   = UNSET_VALUE
         @socket            = UNSET_VALUE
+        @proxy_command     = UNSET_VALUE
         @storage_pool_name = UNSET_VALUE
         @snapshot_pool_name = UNSET_VALUE
         @random_hostname   = UNSET_VALUE
@@ -765,6 +768,7 @@ module VagrantPlugins
         end
 
         finalize_from_uri
+        finalize_proxy_command
 
         @storage_pool_name = 'default' if @storage_pool_name == UNSET_VALUE
         @snapshot_pool_name = @storage_pool_name if @snapshot_pool_name == UNSET_VALUE
@@ -1022,6 +1026,31 @@ module VagrantPlugins
 
         @id_ssh_key_file = id_ssh_key_file
       end
+
+      def finalize_proxy_command
+        if @connect_via_ssh
+          if @proxy_command == UNSET_VALUE
+            proxy_command = "ssh '#{@host}' "
+            proxy_command << "-l '#{@username}' " if @username
+            proxy_command << "-i '#{@id_ssh_key_file}' " if @id_ssh_key_file
+            proxy_command << '-W %h:%p'
+          else
+            inputs = { host: @host }
+            inputs[:username] = @username if @username
+            inputs[:id_ssh_key_file] = @id_ssh_key_file if @id_ssh_key_file
+
+            proxy_command = @proxy_command
+            # avoid needing to escape '%' symbols
+            inputs.each do |key, value|
+              proxy_command.gsub!("{#{key}}", value)
+            end
+          end
+
+          @proxy_command = proxy_command
+        else
+          @proxy_command = nil
+        end
+      end
     end
   end
 end

lib/vagrant-libvirt/provider.rb

@@ -68,13 +68,7 @@ module VagrantPlugins
           forward_x11: @machine.config.ssh.forward_x11
         }
 
-        if @machine.provider_config.connect_via_ssh
-          proxy_command = "ssh '#{@machine.provider_config.host}' "
-          proxy_command << "-l '#{@machine.provider_config.username}' " if @machine.provider_config.username
-          proxy_command << "-i '#{@machine.provider_config.id_ssh_key_file}' " if @machine.provider_config.id_ssh_key_file
-          proxy_command << '-W %h:%p'
-          ssh_info[:proxy_command] = proxy_command
-        end
+        ssh_info[:proxy_command] = @machine.provider_config.proxy_command if @machine.provider_config.proxy_command
 
         ssh_info
       end

spec/unit/config_spec.rb

@@ -335,6 +335,101 @@ describe VagrantPlugins::ProviderLibvirt::Config do
         end
       end
     end
+
+    context '@proxy_command' do
+      before(:example) do
+        stub_const("ENV", fake_env)
+        fake_env['HOME'] = "/home/tests"
+      end
+
+      [
+        # no connect_via_ssh
+        [
+          {:host => "remote"},
+          nil,
+        ],
+
+        # connect_via_ssh
+        [ # host
+          {:connect_via_ssh => true, :host => 'remote'},
+          "ssh 'remote' -W %h:%p",
+        ],
+        [ # include user
+          {:connect_via_ssh => true, :host => 'remote', :username => 'myuser'},
+          "ssh 'remote' -l 'myuser' -W %h:%p",
+        ],
+        [ # include user and default ssh key exists
+          {:connect_via_ssh => true, :host => 'remote', :username => 'myuser'},
+          "ssh 'remote' -l 'myuser' -i '/home/tests/.ssh/id_rsa' -W %h:%p",
+          {
+            :setup => ContextualProc.new {
+              expect(File).to receive(:file?).with("/home/tests/.ssh/id_rsa").and_return(true)
+            }
+          }
+        ],
+
+        # disable id_ssh_key_file
+        [
+          {:connect_via_ssh => true, :host => 'remote', :id_ssh_key_file => nil},
+          "ssh 'remote' -W %h:%p",
+        ],
+        [ # include user
+          {:connect_via_ssh => true, :host => 'remote', :id_ssh_key_file => nil},
+          "ssh 'remote' -W %h:%p",
+        ],
+
+        # use @uri
+        [
+          {:uri => 'qemu+ssh://remote/system'},
+          "ssh 'remote' -W %h:%p",
+        ],
+        [
+          {:uri => 'qemu+ssh://myuser@remote/system'},
+          "ssh 'remote' -l 'myuser' -W %h:%p",
+        ],
+        [
+          {:uri => 'qemu+ssh://remote/system?keyfile=/some/path/to/keyfile'},
+          "ssh 'remote' -i '/some/path/to/keyfile' -W %h:%p",
+          {:allow_failure => "keyfile not yet inferred from uri"},
+        ],
+
+        # provide custom template
+        [
+          {:connect_via_ssh => true, :host => 'remote', :proxy_command => "ssh {host} nc %h %p" },
+          "ssh remote nc %h %p",
+        ],
+        [
+          {:connect_via_ssh => true, :host => 'remote', :username => 'myuser', :proxy_command => "ssh {host} nc %h %p" },
+          "ssh remote nc %h %p",
+        ],
+        [
+          {:connect_via_ssh => true, :host => 'remote', :username => 'myuser', :proxy_command => "ssh {host} -l {username} nc %h %p" },
+          "ssh remote -l myuser nc %h %p",
+        ],
+      ].each do |inputs, proxy_command, options|
+        opts = {}
+        opts.merge!(options) if options
+
+        it "should handle inputs #{inputs}" do
+          # allow some of these to fail for now if marked as such
+          if !opts[:allow_failure].nil?
+            pending(opts[:allow_failure])
+          end
+
+          if !opts[:setup].nil?
+            opts[:setup].apply(binding)
+          end
+
+          inputs.each do |k, v|
+            subject.instance_variable_set("@#{k}", v)
+          end
+
+          subject.finalize!
+
+          expect(subject.proxy_command).to eq(proxy_command)
+        end
+      end
+    end
   end
 
   def assert_invalid