Query host interfaces directly as libvirt may not include them (#1627)

On some distros the libvirt does not appear to always return all of the
host interfaces. Switch to using 'ip -j link show' to read them directly
from the system in order to ensure all devices are read.

Refactor the driver tests to better isolate between test setup for the
different sets of functions and avoid accidental setting of
configuration details that may not be obvious.

Fixes: #1624

lib/vagrant-libvirt/action/forward_ports.rb

@@ -110,7 +110,7 @@ module VagrantPlugins
               end
           ).map { |s| ['-o', s] }.flatten
 
-          options += ['-o', "ProxyCommand=\"#{ssh_info[:proxy_command]}\""] if machine.provider_config.proxy_command
+          options += ['-o', "ProxyCommand=\"#{ssh_info[:proxy_command]}\""] if machine.provider_config.proxy_command && !machine.provider_config.proxy_command.empty?
 
           ssh_cmd = ['ssh'] + options + params
 

lib/vagrant-libvirt/config.rb

@@ -19,6 +19,7 @@ module VagrantPlugins
 
       # The name of the server, where Libvirtd is running.
       attr_accessor :host
+      attr_accessor :port
 
       # If use ssh tunnel to connect to Libvirt.
       attr_accessor :connect_via_ssh
@@ -1218,14 +1219,9 @@ module VagrantPlugins
       end
 
       def host_devices(machine)
-        @host_devices ||= begin
+        machine.provider.driver.host_devices.select do |dev|
-          (
+          next if dev.empty?
-            machine.provider.driver.list_host_devices.map { |iface| iface.name } +
+          dev != "lo" && !@host_device_exclude_prefixes.any? { |exclude| dev.start_with?(exclude) }
-            machine.provider.driver.list_networks.map { |net| net.bridge_name }
-          ).uniq.select do |dev|
-            next if dev.empty?
-            dev != "lo" && !@host_device_exclude_prefixes.any? { |exclude| dev.start_with?(exclude) }
-          end
         end
       end
 

lib/vagrant-libvirt/driver.rb

@@ -197,12 +197,28 @@ module VagrantPlugins
         domain
       end
 
-      def list_host_devices
+      def host_devices
-        connection.client.list_all_interfaces
+        @host_devices ||= begin
-      end
+          cmd = []
+          unless @machine.provider_config.proxy_command.nil? || @machine.provider_config.proxy_command.empty?
+            cmd = ['ssh', @machine.provider_config.host]
+            cmd += ['-p', @machine.provider_config.port.to_s] if @machine.provider_config.port
+            cmd += ['-l', @machine.provider_config.username] if @machine.provider_config.username
+            cmd += ['-i', @machine.provider_config.id_ssh_key_file] if @machine.provider_config.id_ssh_key_file
+          end
+          ip_cmd = cmd + %W(ip -j link show)
 
-      def list_networks
+          result = Vagrant::Util::Subprocess.execute(*ip_cmd)
-        connection.client.list_all_networks
+          raise Errors::FogLibvirtConnectionError unless result.exit_code == 0
+
+          info = JSON.parse(result.stdout)
+
+          (
+            info.map { |iface| iface['ifname'] } +
+            connection.client.list_all_interfaces.map { |iface| iface.name } +
+            connection.client.list_all_networks.map { |net| net.bridge_name }
+          ).uniq.reject(&:empty?)
+        end
       end
 
       def attach_device(xml)

lib/vagrant-libvirt/provider.rb

@@ -70,7 +70,7 @@ module VagrantPlugins
           forward_x11: @machine.config.ssh.forward_x11
         }
 
-        ssh_info[:proxy_command] = @machine.provider_config.proxy_command if @machine.provider_config.proxy_command
+        ssh_info[:proxy_command] = @machine.provider_config.proxy_command if @machine.provider_config.proxy_command && !@machine.provider_config.proxy_command.empty?
 
         ssh_info
       end

spec/unit/config_spec.rb

@@ -741,23 +741,15 @@ describe VagrantPlugins::ProviderLibvirt::Config do
     context 'with public_network defined' do
       let(:libvirt_client)  { instance_double(::Libvirt::Connect) }
       let(:host_devices) { [
-        instance_double(Libvirt::Interface),
+        'lo',
-        instance_double(Libvirt::Interface),
+        'eth0',
-      ] }
+        'virbr0',
-      let(:libvirt_networks) { [
-        instance_double(Libvirt::Network),
-        instance_double(Libvirt::Network),
       ] }
       let(:driver) { instance_double(::VagrantPlugins::ProviderLibvirt::Driver) }
       before do
         machine.config.vm.network :public_network, dev: 'eth0', ip: "192.168.2.157"
         allow(machine.provider).to receive(:driver).and_return(driver)
-        expect(driver).to receive(:list_host_devices).and_return(host_devices)
+        expect(driver).to receive(:host_devices).and_return(host_devices).at_least(:once).times
-        expect(driver).to receive(:list_networks).and_return(libvirt_networks)
-        expect(host_devices[0]).to receive(:name).and_return('eth0')
-        expect(host_devices[1]).to receive(:name).and_return('virbr0')
-        expect(libvirt_networks[0]).to receive(:bridge_name).and_return('')
-        expect(libvirt_networks[1]).to receive(:bridge_name).and_return('virbr0')
       end
 
       it 'should validate use of existing device' do

spec/unit/driver_spec.rb

@@ -14,87 +14,94 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
 
   subject { described_class.new(machine) }
 
-  let(:vagrantfile) do
-    <<-EOF
-    Vagrant.configure('2') do |config|
-      config.vm.define :test1 do |node|
-        node.vm.provider :libvirt do |domain|
-          domain.uri = "qemu+ssh://user@remote1/system"
-        end
-      end
-      config.vm.define :test2 do |node|
-        node.vm.provider :libvirt do |domain|
-          domain.uri = "qemu+ssh://vms@remote2/system"
-        end
-      end
-    end
-    EOF
-  end
-
-  # need to override the default package iso_env as using a different
-  # name for the test machines above.
-  let(:machine)    { iso_env.machine(:test1, :libvirt) }
-  let(:machine2)   { iso_env.machine(:test2, :libvirt) }
-  let(:connection1) { double("connection 1") }
-  let(:connection2) { double("connection 2") }
-  let(:system_connection1) { double("system connection 1") }
-  let(:system_connection2) { double("system connection 2") }
-
   # make it easier for distros that want to switch the default value for
   # qemu_use_session to true by ensuring it is explicitly false for tests.
   before do
     allow(machine.provider_config).to receive(:qemu_use_session).and_return(false)
     allow(logger).to receive(:info)
     allow(logger).to receive(:debug)
-    allow(machine.provider).to receive('driver').and_call_original
-    allow(machine2.provider).to receive('driver').and_call_original
   end
 
-  describe '#connection' do
+  describe 'connections' do
-    it 'should configure a separate connection per machine' do
+    let(:vagrantfile) do
-      expect(Fog::Compute).to receive(:new).with(
+      <<-EOF
-        hash_including({:libvirt_uri => 'qemu+ssh://user@remote1/system'})).and_return(connection1)
+      Vagrant.configure('2') do |config|
-      expect(Fog::Compute).to receive(:new).with(
+        config.vm.define :test1 do |node|
-        hash_including({:libvirt_uri => 'qemu+ssh://vms@remote2/system'})).and_return(connection2)
+          node.vm.provider :libvirt do |domain|
-
+            domain.uri = "qemu+ssh://user@remote1/system"
-      expect(machine.provider.driver.connection).to eq(connection1)
+          end
-      expect(machine2.provider.driver.connection).to eq(connection2)
+        end
+        config.vm.define :test2 do |node|
+          node.vm.provider :libvirt do |domain|
+            domain.uri = "qemu+ssh://vms@remote2/system"
+          end
+        end
+      end
+      EOF
     end
 
-    it 'should configure the connection once' do
+    # need to override the default package iso_env as using a different
-      expect(Fog::Compute).to receive(:new).once().and_return(connection1)
+    # name for the test machines above.
+    let(:machine)    { iso_env.machine(:test1, :libvirt) }
+    let(:machine2)   { iso_env.machine(:test2, :libvirt) }
+    let(:connection1) { double("connection 1") }
+    let(:connection2) { double("connection 2") }
+    let(:system_connection1) { double("system connection 1") }
+    let(:system_connection2) { double("system connection 2") }
 
-      expect(machine.provider.driver.connection).to eq(connection1)
+    # make it easier for distros that want to switch the default value for
-      expect(machine.provider.driver.connection).to eq(connection1)
+    # qemu_use_session to true by ensuring it is explicitly false for tests.
-      expect(machine.provider.driver.connection).to eq(connection1)
+    before do
-    end
+      allow(machine.provider).to receive('driver').and_call_original
-  end
+      allow(machine2.provider).to receive('driver').and_call_original
-
-  describe '#system_connection' do
-    # note that the urls for the two tests are currently
-    # incorrect here as they should be the following:
-    #   qemu+ssh://user@remote1/system
-    #   qemu+ssh://vms@remote2/system
-    #
-    # In that the system uri should be resolved based on
-    # the provider uri so that for:
-    #   uri => qemu+ssh://user@remote1/session
-    # system_uri should be 'qemu+ssh://user@remote1/system'
-    # and not 'qemu:///system'.
-    it 'should configure a separate connection per machine' do
-      expect(Libvirt).to receive(:open_read_only).with('qemu+ssh://user@remote1/system').and_return(system_connection1)
-      expect(Libvirt).to receive(:open_read_only).with('qemu+ssh://vms@remote2/system').and_return(system_connection2)
-
-      expect(machine.provider.driver.system_connection).to eq(system_connection1)
-      expect(machine2.provider.driver.system_connection).to eq(system_connection2)
     end
 
-    it 'should configure the connection once' do
+    describe '#connection' do
-      expect(Libvirt).to receive(:open_read_only).with('qemu+ssh://user@remote1/system').and_return(system_connection1)
+      it 'should configure a separate connection per machine' do
+        expect(Fog::Compute).to receive(:new).with(
+          hash_including({:libvirt_uri => 'qemu+ssh://user@remote1/system'})).and_return(connection1)
+        expect(Fog::Compute).to receive(:new).with(
+          hash_including({:libvirt_uri => 'qemu+ssh://vms@remote2/system'})).and_return(connection2)
 
-      expect(machine.provider.driver.system_connection).to eq(system_connection1)
+        expect(machine.provider.driver.connection).to eq(connection1)
-      expect(machine.provider.driver.system_connection).to eq(system_connection1)
+        expect(machine2.provider.driver.connection).to eq(connection2)
-      expect(machine.provider.driver.system_connection).to eq(system_connection1)
+      end
+
+      it 'should configure the connection once' do
+        expect(Fog::Compute).to receive(:new).once().and_return(connection1)
+
+        expect(machine.provider.driver.connection).to eq(connection1)
+        expect(machine.provider.driver.connection).to eq(connection1)
+        expect(machine.provider.driver.connection).to eq(connection1)
+      end
+    end
+
+    describe '#system_connection' do
+      # note that the urls for the two tests are currently
+      # incorrect here as they should be the following:
+      #   qemu+ssh://user@remote1/system
+      #   qemu+ssh://vms@remote2/system
+      #
+      # In that the system uri should be resolved based on
+      # the provider uri so that for:
+      #   uri => qemu+ssh://user@remote1/session
+      # system_uri should be 'qemu+ssh://user@remote1/system'
+      # and not 'qemu:///system'.
+      it 'should configure a separate connection per machine' do
+        expect(Libvirt).to receive(:open_read_only).with('qemu+ssh://user@remote1/system').and_return(system_connection1)
+        expect(Libvirt).to receive(:open_read_only).with('qemu+ssh://vms@remote2/system').and_return(system_connection2)
+
+        expect(machine.provider.driver.system_connection).to eq(system_connection1)
+        expect(machine2.provider.driver.system_connection).to eq(system_connection2)
+      end
+
+      it 'should configure the connection once' do
+        expect(Libvirt).to receive(:open_read_only).with('qemu+ssh://user@remote1/system').and_return(system_connection1)
+
+        expect(machine.provider.driver.system_connection).to eq(system_connection1)
+        expect(machine.provider.driver.system_connection).to eq(system_connection1)
+        expect(machine.provider.driver.system_connection).to eq(system_connection1)
+      end
     end
   end
 
@@ -179,6 +186,7 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
       end
 
       context 'when qemu_use_session is enabled' do
+        let(:system_connection) { double("system connection") }
         let(:networks) { [instance_double(::Fog::Libvirt::Compute::Real)] }
         let(:dhcp_leases) {
           {
@@ -199,8 +207,8 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
 
         it 'should retrieve the address via the system dhcp-leases API' do
           expect(domain).to receive(:mac).and_return("52:54:00:8b:dc:5f")
-          expect(subject).to receive(:system_connection).and_return(system_connection1)
+          expect(subject).to receive(:system_connection).and_return(system_connection)
-          expect(system_connection1).to receive(:list_all_networks).and_return(networks)
+          expect(system_connection).to receive(:list_all_networks).and_return(networks)
           expect(networks[0]).to receive(:dhcp_leases).and_return([dhcp_leases])
 
           expect(subject.get_ipaddress).to eq("192.168.122.43")
@@ -221,6 +229,99 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
     end
   end
 
+  describe '#host_devices' do
+    let(:vagrantfile_providerconfig) do
+      <<-EOF
+        libvirt.uri = "qemu:///system"
+      EOF
+    end
+
+    let(:ip_link_show) {
+      JSON.dump(
+        [
+          # trimmed element details of what would be returned by 'ip -j link show'
+          { "ifindex": 1, "ifname": "lo", "group": "default", "link_type": "loopback"},
+          { "ifindex": 2, "ifname": "eth0", "group": "default", "link_type": "ether"},
+          { "ifindex": 3, "ifname": "eth1", "group": "default", "link_type": "ether"},
+          { "ifindex": 4, "ifname": "virbr0", "group": "default", "link_type": "ether"},
+        ]
+      )
+    }
+
+    let(:libvirt_interfaces) { [
+      instance_double(Libvirt::Interface),
+      instance_double(Libvirt::Interface),
+    ] }
+    let(:libvirt_networks) { [
+      instance_double(Libvirt::Network),
+      instance_double(Libvirt::Network),
+    ] }
+
+    before do
+      allow(subject).to receive(:connection).and_return(connection)
+
+      allow(Vagrant::Util::Subprocess).to receive(:execute) do |*arr|
+        expect(arr[0]).to eq('ip')
+      end.and_return(Vagrant::Util::Subprocess::Result.new(exit_code=0, stdout=ip_link_show, stderr=''))
+
+      expect(libvirt_client).to receive(:list_all_interfaces).and_return(libvirt_interfaces)
+      expect(libvirt_client).to receive(:list_all_networks).and_return(libvirt_networks)
+      expect(libvirt_interfaces[0]).to receive(:name).and_return('eth0')
+      expect(libvirt_interfaces[1]).to receive(:name).and_return('virbr0')
+      expect(libvirt_networks[0]).to receive(:bridge_name).and_return('')
+      expect(libvirt_networks[1]).to receive(:bridge_name).and_return('virbr0')
+    end
+
+    it 'should query system and libvirt' do
+      expect(subject.host_devices).to eq(['lo', 'eth0', 'eth1', 'virbr0'])
+    end
+
+    it 'should handle empty string' do
+      expect(machine.provider_config).to receive(:proxy_command).and_return('').twice
+
+      expect(subject.host_devices).to eq(['lo', 'eth0', 'eth1', 'virbr0'])
+    end
+
+    it 'should cache the result' do
+      expect(machine.provider_config).to receive(:proxy_command).and_return(nil).once
+
+      expect(subject.host_devices).to eq(['lo', 'eth0', 'eth1', 'virbr0'])
+      expect(subject.host_devices).to eq(['lo', 'eth0', 'eth1', 'virbr0'])
+    end
+
+    context 'when libvirt is remote' do
+      let(:vagrantfile_providerconfig) do
+        <<-EOF
+          libvirt.uri = "qemu+ssh://remote-server/system"
+        EOF
+      end
+
+      before do
+        allow(machine.provider_config).to receive(:proxy_command).and_return('ssh remote-server -W %h:%p')
+      end
+
+      it 'should use ssh for ip link' do
+        expect(Vagrant::Util::Subprocess).to receive(:execute) do |*arr|
+          expect(arr[0..3]).to eq(['ssh', 'remote-server', 'ip', '-j'])
+        end.and_return(Vagrant::Util::Subprocess::Result.new(exit_code=0, stdout=ip_link_show, stderr=''))
+
+        expect(subject.host_devices).to eq(['lo', 'eth0', 'eth1', 'virbr0'])
+      end
+
+      it 'should construct the ssh command with all options when needed' do
+        machine.provider_config.port = 2022
+        machine.provider_config.username = 'remote-user'
+        machine.provider_config.id_ssh_key_file = 'my-key-file'
+
+        expect(Vagrant::Util::Subprocess).to receive(:execute) do |*arr|
+          expect(arr[0..9]).to eq(['ssh', 'remote-server', '-p', '2022', '-l', 'remote-user', '-i', 'my-key-file', 'ip', '-j'])
+        end.and_return(Vagrant::Util::Subprocess::Result.new(exit_code=0, stdout=ip_link_show, stderr=''))
+
+        expect(subject.host_devices).to eq(['lo', 'eth0', 'eth1', 'virbr0'])
+      end
+    end
+  end
+
   describe '#state' do
     let(:domain) { double('domain') }