From f5b70bc074dc19b785637a2dff526dd5006fda3f Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Wed, 5 Oct 2022 14:29:44 +0100 Subject: [PATCH] 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 | 2 +- lib/vagrant-libvirt/config.rb | 12 +- lib/vagrant-libvirt/driver.rb | 26 ++- lib/vagrant-libvirt/provider.rb | 2 +- spec/unit/config_spec.rb | 16 +- spec/unit/driver_spec.rb | 239 ++++++++++++++------ 6 files changed, 201 insertions(+), 96 deletions(-) diff --git a/lib/vagrant-libvirt/action/forward_ports.rb b/lib/vagrant-libvirt/action/forward_ports.rb index 5ee7bb8..89f641b 100644 --- a/lib/vagrant-libvirt/action/forward_ports.rb +++ b/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 diff --git a/lib/vagrant-libvirt/config.rb b/lib/vagrant-libvirt/config.rb index 55c4c91..8d482d3 100644 --- a/lib/vagrant-libvirt/config.rb +++ b/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.list_host_devices.map { |iface| iface.name } + - 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 + machine.provider.driver.host_devices.select do |dev| + next if dev.empty? + dev != "lo" && !@host_device_exclude_prefixes.any? { |exclude| dev.start_with?(exclude) } end end diff --git a/lib/vagrant-libvirt/driver.rb b/lib/vagrant-libvirt/driver.rb index 47e3d53..18b928f 100644 --- a/lib/vagrant-libvirt/driver.rb +++ b/lib/vagrant-libvirt/driver.rb @@ -197,12 +197,28 @@ module VagrantPlugins domain end - def list_host_devices - connection.client.list_all_interfaces - end + def host_devices + @host_devices ||= begin + 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 - connection.client.list_all_networks + result = Vagrant::Util::Subprocess.execute(*ip_cmd) + 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) diff --git a/lib/vagrant-libvirt/provider.rb b/lib/vagrant-libvirt/provider.rb index c0afdd6..6d91c72 100644 --- a/lib/vagrant-libvirt/provider.rb +++ b/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 diff --git a/spec/unit/config_spec.rb b/spec/unit/config_spec.rb index e0f0843..cd034ff 100644 --- a/spec/unit/config_spec.rb +++ b/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), - instance_double(Libvirt::Interface), - ] } - let(:libvirt_networks) { [ - instance_double(Libvirt::Network), - instance_double(Libvirt::Network), + 'lo', + 'eth0', + 'virbr0', ] } 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(: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') + expect(driver).to receive(:host_devices).and_return(host_devices).at_least(:once).times end it 'should validate use of existing device' do diff --git a/spec/unit/driver_spec.rb b/spec/unit/driver_spec.rb index 27a73df..6b08635 100644 --- a/spec/unit/driver_spec.rb +++ b/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 - 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.connection).to eq(connection1) - expect(machine2.provider.driver.connection).to eq(connection2) + describe 'connections' do + 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 - it 'should configure the connection once' do - expect(Fog::Compute).to receive(:new).once().and_return(connection1) + # 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") } - 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) + # 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).to receive('driver').and_call_original + allow(machine2.provider).to receive('driver').and_call_original 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) + describe '#connection' do + 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.system_connection).to eq(system_connection1) - expect(machine.provider.driver.system_connection).to eq(system_connection1) + expect(machine.provider.driver.connection).to eq(connection1) + expect(machine2.provider.driver.connection).to eq(connection2) + 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(system_connection1).to receive(:list_all_networks).and_return(networks) + expect(subject).to receive(:system_connection).and_return(system_connection) + 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') }