From a4a98d97fc2935f2b66043b73c9f7465a08b9c48 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Wed, 19 Dec 2018 16:08:47 -0800 Subject: [PATCH 1/2] Fix guest network configuration by properly extracting extra options Extra options are extracted from the machine configuration for the network being configured to allow for customized network manager behavior. The network entries must be filtered to remove non-network entries (like port forwards) before accessing by index. Fixes #9546 --- plugins/guests/alt/cap/configure_networks.rb | 7 +- .../guests/redhat/cap/configure_networks.rb | 7 +- .../guests/alt/cap/configure_networks_test.rb | 42 +++++++-- .../redhat/cap/configure_networks_test.rb | 86 +++++++++++++++++-- 4 files changed, 120 insertions(+), 22 deletions(-) diff --git a/plugins/guests/alt/cap/configure_networks.rb b/plugins/guests/alt/cap/configure_networks.rb index 851849700..f4a778523 100644 --- a/plugins/guests/alt/cap/configure_networks.rb +++ b/plugins/guests/alt/cap/configure_networks.rb @@ -19,9 +19,12 @@ module VagrantPlugins # Check if NetworkManager is installed on the system nmcli_installed = nmcli?(comm) + net_configs = machine.config.vm.networks.map do |type, opts| + opts if type.to_s.end_with?("_network") + end.compact networks.each.with_index do |network, i| network[:device] = interfaces[network[:interface]] - extra_opts = machine.config.vm.networks[i].last.dup + extra_opts = net_configs[i].dup || {} if nmcli_installed # Now check if the device is actively being managed by NetworkManager @@ -46,7 +49,7 @@ module VagrantPlugins end # Render a new configuration - template_options = network.merge(extra_opts) + template_options = extra_opts.merge(network) # ALT expects netmasks to be in the CIDR notation, but users may # specify IPV4 netmasks like "255.255.255.0". This magic converts diff --git a/plugins/guests/redhat/cap/configure_networks.rb b/plugins/guests/redhat/cap/configure_networks.rb index 44fa5fa67..c5794aa38 100644 --- a/plugins/guests/redhat/cap/configure_networks.rb +++ b/plugins/guests/redhat/cap/configure_networks.rb @@ -19,9 +19,12 @@ module VagrantPlugins # Check if NetworkManager is installed on the system nmcli_installed = nmcli?(comm) + net_configs = machine.config.vm.networks.map do |type, opts| + opts if type.to_s.end_with?("_network") + end.compact networks.each.with_index do |network, i| network[:device] = interfaces[network[:interface]] - extra_opts = machine.config.vm.networks[i].last.dup + extra_opts = net_configs[i].dup || {} if nmcli_installed # Now check if the device is actively being managed by NetworkManager @@ -47,7 +50,7 @@ module VagrantPlugins # Render a new configuration entry = TemplateRenderer.render("guests/redhat/network_#{network[:type]}", - options: network.merge(extra_opts), + options: extra_opts.merge(network), ) # Upload the new configuration diff --git a/test/unit/plugins/guests/alt/cap/configure_networks_test.rb b/test/unit/plugins/guests/alt/cap/configure_networks_test.rb index f19924d0f..c5c4db6ca 100644 --- a/test/unit/plugins/guests/alt/cap/configure_networks_test.rb +++ b/test/unit/plugins/guests/alt/cap/configure_networks_test.rb @@ -11,7 +11,7 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do let(:config) { double("config", vm: vm) } let(:guest) { double("guest") } let(:machine) { double("machine", guest: guest, config: config) } - let(:networks){ [[{}], [{}]] } + let(:networks){ [[:public_network, network_1], [:private_network, network_2]] } let(:vm){ double("vm", networks: networks) } before do @@ -57,6 +57,14 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do end context "with NetworkManager installed" do + let(:net1_nm_controlled) { true } + let(:net2_nm_controlled) { true } + + let(:networks){ [ + [:public_network, network_1.merge(nm_controlled: net1_nm_controlled)], + [:private_network, network_2.merge(nm_controlled: net2_nm_controlled)] + ] } + before do allow(cap).to receive(:nmcli?).and_return true end @@ -67,6 +75,11 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do end context "with nm_controlled option omitted" do + let(:networks){ [ + [:public_network, network_1], + [:private_network, network_2] + ] } + it "downs networks via nmcli, creates ifaces and restart NetworksManager" do cap.configure_networks(machine, [network_1, network_2]) expect(comm.received_commands[0]).to match(/nmcli.*disconnect/) @@ -77,8 +90,6 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do end context "with nm_controlled option set to true" do - let(:networks){ [[{nm_controlled: true}], [{nm_controlled: true}]] } - it "downs networks via nmcli, creates ifaces and restart NetworksManager" do cap.configure_networks(machine, [network_1, network_2]) expect(comm.received_commands[0]).to match(/nmcli.*disconnect/) @@ -89,7 +100,8 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do end context "with nm_controlled option set to false" do - let(:networks){ [[{nm_controlled: false}], [{nm_controlled: false}]] } + let(:net1_nm_controlled) { false } + let(:net2_nm_controlled) { false } it "downs networks manually, creates ifaces, starts networks manually and restart NetworksManager" do cap.configure_networks(machine, [network_1, network_2]) @@ -102,7 +114,8 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do end context "with nm_controlled option set to false on first device" do - let(:networks){ [[{nm_controlled: false}], [{nm_controlled: true}]] } + let(:net1_nm_controlled) { false } + let(:net2_nm_controlled) { true } it "downs networks, creates ifaces and starts the networks with one managed manually and one NetworkManager controlled" do cap.configure_networks(machine, [network_1, network_2]) @@ -121,6 +134,11 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do end context "with nm_controlled option omitted" do + let(:networks){ [ + [:public_network, network_1], + [:private_network, network_2] + ] } + it "creates and starts the networks manually" do cap.configure_networks(machine, [network_1, network_2]) expect(comm.received_commands[0]).to match(/ifdown/) @@ -132,7 +150,8 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do end context "with nm_controlled option set to true" do - let(:networks){ [[{nm_controlled: true}], [{nm_controlled: true}]] } + let(:net1_nm_controlled) { true } + let(:net2_nm_controlled) { true } it "creates and starts the networks via nmcli" do cap.configure_networks(machine, [network_1, network_2]) @@ -144,7 +163,8 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do end context "with nm_controlled option set to false" do - let(:networks){ [[{nm_controlled: false}], [{nm_controlled: false}]] } + let(:net1_nm_controlled) { false } + let(:net2_nm_controlled) { false } it "creates and starts the networks via ifup " do cap.configure_networks(machine, [network_1, network_2]) @@ -157,7 +177,8 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do end context "with nm_controlled option set to false on first device" do - let(:networks){ [[{nm_controlled: false}], [{nm_controlled: true}]] } + let(:net1_nm_controlled) { false } + let(:net2_nm_controlled) { true } it "creates and starts the networks with one managed manually and one NetworkManager controlled" do cap.configure_networks(machine, [network_1, network_2]) @@ -202,7 +223,10 @@ describe "VagrantPlugins::GuestALT::Cap::ConfigureNetworks" do end context "with nm_controlled option set" do - let(:networks){ [[{nm_controlled: false}], [{nm_controlled: true}]] } + let(:networks){ [ + [:public_network, network_1.merge(nm_controlled: true)], + [:private_network, network_2.merge(nm_controlled: true)] + ] } it "raises an error" do expect{ cap.configure_networks(machine, [network_1, network_2]) }.to raise_error(Vagrant::Errors::NetworkManagerNotInstalled) diff --git a/test/unit/plugins/guests/redhat/cap/configure_networks_test.rb b/test/unit/plugins/guests/redhat/cap/configure_networks_test.rb index 5c621a1f0..f61df6651 100644 --- a/test/unit/plugins/guests/redhat/cap/configure_networks_test.rb +++ b/test/unit/plugins/guests/redhat/cap/configure_networks_test.rb @@ -11,7 +11,7 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do let(:config) { double("config", vm: vm) } let(:guest) { double("guest") } let(:machine) { double("machine", guest: guest, config: config) } - let(:networks){ [[{}], [{}]] } + let(:networks){ [[:public_network, network_1], [:private_network, network_2]] } let(:vm){ double("vm", networks: networks) } before do @@ -56,7 +56,59 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do } end + let(:network_3) do + { + interface: 2, + type: "static", + ip: "33.33.33.11", + netmask: "255.255.0.0", + gateway: "33.33.0.1", + } + end + + context "network configuration file" do + let(:networks){ [[:public_network, network_1], [:private_network, network_2], [:private_network, network_3]] } + let(:tempfile) { double("tempfile") } + + before do + allow(tempfile).to receive(:binmode) + allow(tempfile).to receive(:write) + allow(tempfile).to receive(:fsync) + allow(tempfile).to receive(:close) + allow(tempfile).to receive(:path) + allow(Tempfile).to receive(:open).and_yield(tempfile) + end + + it "should generate two configuration files" do + expect(Tempfile).to receive(:open).twice + cap.configure_networks(machine, [network_1, network_2]) + end + + it "should generate three configuration files" do + expect(Tempfile).to receive(:open).thrice + cap.configure_networks(machine, [network_1, network_2, network_3]) + end + + it "should generate configuration with network_2 IP address" do + expect(tempfile).to receive(:write).with(/#{network_2[:ip]}/) + cap.configure_networks(machine, [network_1, network_2, network_3]) + end + + it "should generate configuration with network_3 IP address" do + expect(tempfile).to receive(:write).with(/#{network_3[:ip]}/) + cap.configure_networks(machine, [network_1, network_2, network_3]) + end + end + context "with NetworkManager installed" do + let(:net1_nm_controlled) { true } + let(:net2_nm_controlled) { true } + + let(:networks){ [ + [:public_network, network_1.merge(nm_controlled: net1_nm_controlled)], + [:private_network, network_2.merge(nm_controlled: net2_nm_controlled)] + ] } + before do allow(cap).to receive(:nmcli?).and_return true end @@ -67,6 +119,11 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do end context "with nm_controlled option omitted" do + let(:networks){ [ + [:public_network, network_1], + [:private_network, network_2] + ] } + it "creates and starts the networks via nmcli" do cap.configure_networks(machine, [network_1, network_2]) expect(comm.received_commands[0]).to match(/nmcli/) @@ -75,8 +132,6 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do end context "with nm_controlled option set to true" do - let(:networks){ [[{nm_controlled: true}], [{nm_controlled: true}]] } - it "creates and starts the networks via nmcli" do cap.configure_networks(machine, [network_1, network_2]) expect(comm.received_commands[0]).to match(/nmcli/) @@ -85,7 +140,8 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do end context "with nm_controlled option set to false" do - let(:networks){ [[{nm_controlled: false}], [{nm_controlled: false}]] } + let(:net1_nm_controlled) { false } + let(:net2_nm_controlled) { false } it "creates and starts the networks via ifup and disables devices in NetworkManager" do cap.configure_networks(machine, [network_1, network_2]) @@ -96,7 +152,8 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do end context "with nm_controlled option set to false on first device" do - let(:networks){ [[{nm_controlled: false}], [{nm_controlled: true}]] } + let(:net1_nm_controlled) { false } + let(:net2_nm_controlled) { true } it "creates and starts the networks with one managed manually and one NetworkManager controlled" do cap.configure_networks(machine, [network_1, network_2]) @@ -113,6 +170,11 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do end context "with nm_controlled option omitted" do + let(:networks){ [ + [:public_network, network_1], + [:private_network, network_2] + ] } + it "creates and starts the networks manually" do cap.configure_networks(machine, [network_1, network_2]) expect(comm.received_commands[0]).to match(/ifdown/) @@ -123,7 +185,8 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do end context "with nm_controlled option set to true" do - let(:networks){ [[{nm_controlled: true}], [{nm_controlled: true}]] } + let(:net1_nm_controlled) { true } + let(:net2_nm_controlled) { true } it "creates and starts the networks via nmcli" do cap.configure_networks(machine, [network_1, network_2]) @@ -134,7 +197,8 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do end context "with nm_controlled option set to false" do - let(:networks){ [[{nm_controlled: false}], [{nm_controlled: false}]] } + let(:net1_nm_controlled) { false } + let(:net2_nm_controlled) { false } it "creates and starts the networks via ifup " do cap.configure_networks(machine, [network_1, network_2]) @@ -146,7 +210,8 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do end context "with nm_controlled option set to false on first device" do - let(:networks){ [[{nm_controlled: false}], [{nm_controlled: true}]] } + let(:net1_nm_controlled) { false } + let(:net2_nm_controlled) { true } it "creates and starts the networks with one managed manually and one NetworkManager controlled" do cap.configure_networks(machine, [network_1, network_2]) @@ -174,7 +239,10 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do end context "with nm_controlled option set" do - let(:networks){ [[{nm_controlled: false}], [{nm_controlled: true}]] } + let(:networks){ [ + [:public_network, network_1.merge(nm_controlled: true)], + [:private_network, network_2.merge(nm_controlled: true)] + ] } it "raises an error" do expect{ cap.configure_networks(machine, [network_1, network_2]) }.to raise_error(Vagrant::Errors::NetworkManagerNotInstalled) From 9f1e7d98958585f623c3cc7fdb225dbc854ae06f Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 20 Dec 2018 07:43:43 -0800 Subject: [PATCH 2/2] Prevent nil dup as unsupported in 2.3 --- plugins/guests/alt/cap/configure_networks.rb | 2 +- plugins/guests/redhat/cap/configure_networks.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/guests/alt/cap/configure_networks.rb b/plugins/guests/alt/cap/configure_networks.rb index f4a778523..e9f64a940 100644 --- a/plugins/guests/alt/cap/configure_networks.rb +++ b/plugins/guests/alt/cap/configure_networks.rb @@ -24,7 +24,7 @@ module VagrantPlugins end.compact networks.each.with_index do |network, i| network[:device] = interfaces[network[:interface]] - extra_opts = net_configs[i].dup || {} + extra_opts = net_configs[i] ? net_configs[i].dup : {} if nmcli_installed # Now check if the device is actively being managed by NetworkManager diff --git a/plugins/guests/redhat/cap/configure_networks.rb b/plugins/guests/redhat/cap/configure_networks.rb index c5794aa38..c618c8e53 100644 --- a/plugins/guests/redhat/cap/configure_networks.rb +++ b/plugins/guests/redhat/cap/configure_networks.rb @@ -24,7 +24,7 @@ module VagrantPlugins end.compact networks.each.with_index do |network, i| network[:device] = interfaces[network[:interface]] - extra_opts = net_configs[i].dup || {} + extra_opts = net_configs[i] ? net_configs[i].dup : {} if nmcli_installed # Now check if the device is actively being managed by NetworkManager