Merge pull request #8531 from chrisroberts/network/guest-rhel

guests/rhel: Update network configuration

lib/vagrant/errors.rb

@@ -460,6 +460,10 @@ module Vagrant
       error_key(:network_type_not_supported)
     end
 
+    class NetworkManagerNotInstalled < VagrantError
+      error_key(:network_manager_not_installed)
+    end
+
     class NFSBadExports < VagrantError
       error_key(:nfs_bad_exports)
     end

lib/vagrant/util.rb

@@ -6,6 +6,7 @@ module Vagrant
     autoload :CredentialScrubber,        'vagrant/util/credential_scrubber'
     autoload :Env,                       'vagrant/util/env'
     autoload :HashWithIndifferentAccess, 'vagrant/util/hash_with_indifferent_access'
+    autoload :GuestInspection,           'vagrant/util/guest_inspection'
     autoload :Platform,                  'vagrant/util/platform'
     autoload :Retryable,                 'vagrant/util/retryable'
     autoload :SafeExec,                  'vagrant/util/safe_exec'

lib/vagrant/util/guest_inspection.rb

@@ -0,0 +1,47 @@
+module Vagrant
+  module Util
+    # Helper methods for inspecting guests to determine if specific services
+    # or applications are installed and in use
+    module GuestInspection
+      # Linux specific inspection helpers
+      module Linux
+
+        ## systemd helpers
+
+        # systemd is in used
+        #
+        # @return [Boolean]
+        def systemd?(comm)
+          comm.test("systemctl | grep '^-\.mount'")
+        end
+
+        # systemd hostname set is via hostnamectl
+        #
+        # @return [Boolean]
+        def hostnamectl?(comm)
+          comm.test("hostnamectl")
+        end
+
+        ## nmcli helpers
+
+        # nmcli is installed
+        #
+        # @return [Boolean]
+        def nmcli?(comm)
+          comm.test("nmcli")
+        end
+
+        # NetworkManager currently controls device
+        #
+        # @param comm [Communicator]
+        # @param device_name [String]
+        # @return [Boolean]
+        def nm_controlled?(comm, device_name)
+          comm.test("nmcli d show #{device_name}") &&
+            !comm.test("nmcli d show #{device_name} | grep unmanaged")
+        end
+
+      end
+    end
+  end
+end

plugins/guests/linux/cap/network_interfaces.rb

@@ -19,6 +19,13 @@ module VagrantPlugins
           machine.communicate.sudo("#{path} -o -0 addr | grep -v LOOPBACK | awk '{print $2}' | sed 's/://'") do |type, data|
             s << data if type == :stdout
           end
+          # In some cases net devices may be added to the guest and will not
+          # properly show up when using `ip`. This pulls any device information
+          # that can be found in /proc and adds it to the list of interfaces
+          s << "\n"
+          machine.communicate.sudo("cat /proc/net/dev | grep -E '^[a-z0-9 ]+:' | awk '{print $1}' | sed 's/://'", error_check: false) do |type, data|
+            s << data if type == :stdout
+          end
           ifaces = s.split("\n")
           @@logger.debug("Unsorted list: #{ifaces.inspect}")
           # Break out integers from strings and sort the arrays to provide
@@ -35,7 +42,7 @@ module VagrantPlugins
               end
             end
           end
-          ifaces = ifaces.sort do |lhs, rhs|
+          ifaces = ifaces.uniq.sort do |lhs, rhs|
             result = 0
             slice_length = [rhs.size, lhs.size].min
             slice_length.times do |idx|

plugins/guests/redhat/cap/configure_networks.rb

@@ -7,21 +7,47 @@ module VagrantPlugins
     module Cap
       class ConfigureNetworks
         include Vagrant::Util
+        extend Vagrant::Util::GuestInspection::Linux
 
         def self.configure_networks(machine, networks)
           comm = machine.communicate
 
           network_scripts_dir = machine.guest.capability(:network_scripts_dir)
 
-          commands   = []
+          commands   = {:start => [], :middle => [], :end => []}
           interfaces = machine.guest.capability(:network_interfaces)
 
+          # Check if NetworkManager is installed on the system
+          nmcli_installed = nmcli?(comm)
           networks.each.with_index do |network, i|
             network[:device] = interfaces[network[:interface]]
+            extra_opts = machine.config.vm.networks[i].last.dup
+
+            if nmcli_installed
+              # Now check if the device is actively being managed by NetworkManager
+              nm_controlled = nm_controlled?(comm, network[:device])
+            end
+
+            if !extra_opts.key?(:nm_controlled)
+              extra_opts[:nm_controlled] = !!nm_controlled
+            end
+
+            extra_opts[:nm_controlled] = case extra_opts[:nm_controlled]
+                                      when true
+                                        "yes"
+                                      when false, nil
+                                        "no"
+                                      else
+                                        extra_opts[:nm_controlled].to_s
+                                      end
+
+            if extra_opts[:nm_controlled] == "yes" && !nmcli_installed
+              raise Vagrant::Errors::NetworkManagerNotInstalled, device: network[:device]
+            end
 
             # Render a new configuration
             entry = TemplateRenderer.render("guests/redhat/network_#{network[:type]}",
-              options: network,
+              options: network.merge(extra_opts),
             )
 
             # Upload the new configuration
@@ -36,24 +62,27 @@ module VagrantPlugins
 
             # Add the new interface and bring it back up
             final_path = "#{network_scripts_dir}/ifcfg-#{network[:device]}"
-            commands << <<-EOH.gsub(/^ */, '')
+
-              # Down the interface before munging the config file. This might
+            if nm_controlled
-              # fail if the interface is not actually set up yet so ignore
+              if extra_opts[:nm_controlled] == "no"
-              # errors.
+                commands[:start] << "nmcli d disconnect iface '#{network[:device]}'"
-              /sbin/ifdown '#{network[:device]}'
-              # Move new config into place
-              mv -f '#{remote_path}' '#{final_path}'
-              # attempt to force network manager to reload configurations
-              nmcli c reload || true
-            EOH
               end
-
+            else
-          commands << <<-EOH.gsub(/^ */, '')
+              commands[:start] << "/sbin/ifdown '#{network[:device]}'"
-            # Restart network
+            end
-            service network restart
+            commands[:middle] << "mv -f '#{remote_path}' '#{final_path}'"
-          EOH
+            if extra_opts[:nm_controlled] == "no"
-
+              commands[:end] << "/sbin/ifup '#{network[:device]}'"
+            end
+          end
+          if nmcli_installed
+            commands[:middle] << "((nmcli c help 2>&1 | grep reload) && nmcli c reload) || " \
+              "(test -f /etc/init.d/NetworkManager && /etc/init.d/NetworkManager restart) || " \
+              "((systemctl | grep NetworkManager.service) && systemctl NetworkManager restart)"
+          end
+          commands = commands[:start] + commands[:middle] + commands[:end]
           comm.sudo(commands.join("\n"))
+          comm.wait_for_ready(5)
         end
       end
     end

templates/guests/redhat/network_dhcp.erb

@@ -3,4 +3,5 @@
 BOOTPROTO=dhcp
 ONBOOT=yes
 DEVICE=<%= options[:device] %>
+NM_CONTROLLED=<%= options.fetch(:nm_controlled, "no") %>
 #VAGRANT-END

templates/guests/redhat/network_static.erb

@@ -1,6 +1,6 @@
 #VAGRANT-BEGIN
 # The contents below are automatically generated by Vagrant. Do not modify.
-NM_CONTROLLED=no
+NM_CONTROLLED=<%= options.fetch(:nm_controlled, "no") %>
 BOOTPROTO=none
 ONBOOT=yes
 IPADDR=<%= options[:ip] %>

templates/guests/redhat/network_static6.erb

@@ -1,6 +1,6 @@
 #VAGRANT-BEGIN
 # The contents below are automatically generated by Vagrant. Do not modify.
-NM_CONTROLLED=no
+NM_CONTROLLED=<%= options.fetch(:nm_controlled, "no") %>
 BOOTPROTO=static
 ONBOOT=yes
 DEVICE=<%= options[:device] %>

templates/locales/en.yml

@@ -882,6 +882,13 @@ en:
         %{message}
       network_type_not_supported: |-
         The %{type} network type is not supported for this box or guest.
+      network_manager_not_installed: |-
+        Vagrant was instructed to configure the %{device} network device to
+        be managed by NetworkManager. However, the configured guest VM does
+        not have NetworkManager installed. To fix this error please remove
+        the `nm_controlled` setting from local Vagantfile. If NetworkManager
+        is required to manage the network devices, please use a box with
+        NetworkManager installed.
       nfs_bad_exports: |-
         NFS is reporting that your exports file is invalid. Vagrant does
         this check before making any changes to the file. Please correct

test/unit/plugins/guests/linux/cap/network_interfaces_test.rb

@@ -22,61 +22,61 @@ describe "VagrantPlugins::GuestLinux::Cap::NetworkInterfaces" do
     let(:cap){ caps.get(:network_interfaces) }
 
     it "sorts discovered classic interfaces" do
-      expect(comm).to receive(:sudo).and_yield(:stdout, "eth1\neth2\neth0")
+      expect(comm).to receive(:sudo).twice.and_yield(:stdout, "eth1\neth2\neth0")
       result = cap.network_interfaces(machine)
       expect(result).to eq(["eth0", "eth1", "eth2"])
     end
 
     it "sorts discovered predictable network interfaces" do
-      expect(comm).to receive(:sudo).and_yield(:stdout, "enp0s8\nenp0s3\nenp0s5")
+      expect(comm).to receive(:sudo).twice.and_yield(:stdout, "enp0s8\nenp0s3\nenp0s5")
       result = cap.network_interfaces(machine)
       expect(result).to eq(["enp0s3", "enp0s5", "enp0s8"])
     end
 
     it "sorts discovered classic interfaces naturally" do
-      expect(comm).to receive(:sudo).and_yield(:stdout, "eth1\neth2\neth12\neth0\neth10")
+      expect(comm).to receive(:sudo).twice.and_yield(:stdout, "eth1\neth2\neth12\neth0\neth10")
       result = cap.network_interfaces(machine)
       expect(result).to eq(["eth0", "eth1", "eth2", "eth10", "eth12"])
     end
 
     it "sorts discovered predictable network interfaces naturally" do
-      expect(comm).to receive(:sudo).and_yield(:stdout, "enp0s8\nenp0s3\nenp0s5\nenp0s10\nenp1s3")
+      expect(comm).to receive(:sudo).twice.and_yield(:stdout, "enp0s8\nenp0s3\nenp0s5\nenp0s10\nenp1s3")
       result = cap.network_interfaces(machine)
       expect(result).to eq(["enp0s3", "enp0s5", "enp0s8", "enp0s10", "enp1s3"])
     end
 
     it "sorts ethernet devices discovered with classic naming first in list" do
-      expect(comm).to receive(:sudo).and_yield(:stdout, "eth1\neth2\ndocker0\nbridge0\neth0")
+      expect(comm).to receive(:sudo).twice.and_yield(:stdout, "eth1\neth2\ndocker0\nbridge0\neth0")
       result = cap.network_interfaces(machine)
       expect(result).to eq(["eth0", "eth1", "eth2", "bridge0", "docker0"])
     end
 
     it "sorts ethernet devices discovered with predictable network interfaces naming first in list" do
-      expect(comm).to receive(:sudo).and_yield(:stdout, "enp0s8\ndocker0\nenp0s3\nbridge0\nenp0s5")
+      expect(comm).to receive(:sudo).twice.and_yield(:stdout, "enp0s8\ndocker0\nenp0s3\nbridge0\nenp0s5")
       result = cap.network_interfaces(machine)
       expect(result).to eq(["enp0s3", "enp0s5", "enp0s8", "bridge0", "docker0"])
     end
 
     it "sorts ethernet devices discovered with predictable network interfaces naming first in list with less" do
-      expect(comm).to receive(:sudo).and_yield(:stdout, "enp0s3\nenp0s8\ndocker0")
+      expect(comm).to receive(:sudo).twice.and_yield(:stdout, "enp0s3\nenp0s8\ndocker0")
       result = cap.network_interfaces(machine)
       expect(result).to eq(["enp0s3", "enp0s8", "docker0"])
     end
 
     it "does not include ethernet devices aliases within prefix device listing" do
-      expect(comm).to receive(:sudo).and_yield(:stdout, "eth1\neth2\ndocker0\nbridge0\neth0\ndocker1\neth0:0")
+      expect(comm).to receive(:sudo).twice.and_yield(:stdout, "eth1\neth2\ndocker0\nbridge0\neth0\ndocker1\neth0:0")
       result = cap.network_interfaces(machine)
       expect(result).to eq(["eth0", "eth1", "eth2", "bridge0", "docker0", "docker1", "eth0:0"])
     end
 
     it "does not include ethernet devices aliases within prefix device listing with dot separators" do
-      expect(comm).to receive(:sudo).and_yield(:stdout, "eth1\neth2\ndocker0\nbridge0\neth0\ndocker1\neth0.1@eth0")
+      expect(comm).to receive(:sudo).twice.and_yield(:stdout, "eth1\neth2\ndocker0\nbridge0\neth0\ndocker1\neth0.1@eth0")
       result = cap.network_interfaces(machine)
       expect(result).to eq(["eth0", "eth1", "eth2", "bridge0", "docker0", "docker1", "eth0.1@eth0"])
     end
 
     it "properly sorts non-consistent device name formats" do
-      expect(comm).to receive(:sudo).and_yield(:stdout, "eth0\neth1\ndocker0\nveth437f7f9\nveth06b3e44\nveth8bb7081")
+      expect(comm).to receive(:sudo).twice.and_yield(:stdout, "eth0\neth1\ndocker0\nveth437f7f9\nveth06b3e44\nveth8bb7081")
       result = cap.network_interfaces(machine)
       expect(result).to eq(["eth0", "eth1", "docker0", "veth8bb7081", "veth437f7f9", "veth06b3e44"])
     end

test/unit/plugins/guests/redhat/cap/configure_networks_test.rb

@@ -7,9 +7,12 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do
       .guest_capabilities[:redhat]
   end
 
-  let(:guest) { double("guest") }
-  let(:machine) { double("machine", guest: guest) }
   let(:comm) { VagrantTests::DummyCommunicator::Communicator.new(machine) }
+  let(:config) { double("config", vm: vm) }
+  let(:guest) { double("guest") }
+  let(:machine) { double("machine", guest: guest, config: config) }
+  let(:networks){ [[{}], [{}]] }
+  let(:vm){ double("vm", networks: networks) }
 
   before do
     allow(machine).to receive(:communicate).and_return(comm)
@@ -23,6 +26,10 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do
     let(:cap) { caps.get(:configure_networks) }
 
     before do
+      allow(guest).to receive(:capability)
+        .with(:flavor)
+        .and_return(:rhel)
+
       allow(guest).to receive(:capability)
         .with(:network_scripts_dir)
         .and_return("/scripts")
@@ -49,17 +56,136 @@ describe "VagrantPlugins::GuestRedHat::Cap::ConfigureNetworks" do
       }
     end
 
-    it "creates and starts the networks" do
+    context "with NetworkManager installed" do
-      allow(guest).to receive(:capability)
+      before do
-        .with(:flavor)
+        allow(cap).to receive(:nmcli?).and_return true
-        .and_return(:rhel)
+      end
 
+      context "with devices managed by NetworkManager" do
+        before do
+          allow(cap).to receive(:nm_controlled?).and_return true
+        end
+
+        context "with nm_controlled option omitted" do
+          it "creates and starts the networks via nmcli" do
             cap.configure_networks(machine, [network_1, network_2])
-      expect(comm.received_commands[0]).to match(/\/sbin\/ifdown 'eth1'/)
+            expect(comm.received_commands[0]).to match(/nmcli/)
-      expect(comm.received_commands[0]).to match(/ifcfg-eth1/)
+            expect(comm.received_commands[0]).to_not match(/(ifdown|ifup)/)
-      expect(comm.received_commands[0]).to match(/\/sbin\/ifdown 'eth2'/)
+          end
-      expect(comm.received_commands[0]).to match(/ifcfg-eth2/)
+        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/)
+            expect(comm.received_commands[0]).to_not match(/(ifdown|ifup)/)
+          end
+        end
+
+        context "with nm_controlled option set to false" do
+          let(:networks){ [[{nm_controlled: false}], [{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])
+            expect(comm.received_commands[0]).to match(/nmcli.*disconnect/)
             expect(comm.received_commands[0]).to match(/nmcli c reload/)
+            expect(comm.received_commands[0]).to match(/ifup/)
+            expect(comm.received_commands[0]).to_not match(/ifdown/)
+          end
+        end
+
+        context "with nm_controlled option set to false on first device" do
+          let(:networks){ [[{nm_controlled: false}], [{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])
+            expect(comm.received_commands[0]).to match(/nmcli.*disconnect.*eth1/)
+            expect(comm.received_commands[0]).to match(/nmcli c reload/)
+            expect(comm.received_commands[0]).to match(/ifup.*eth1/)
+            expect(comm.received_commands[0]).to match(/nmcli.*reload/)
+            expect(comm.received_commands[0]).to_not match(/ifdown/)
+          end
+        end
+      end
+
+      context "with devices not managed by NetworkManager" do
+        before do
+          allow(cap).to receive(:nm_controlled?).and_return false
+        end
+
+        context "with nm_controlled option omitted" do
+          it "creates and starts the networks manually" do
+            cap.configure_networks(machine, [network_1, network_2])
+            expect(comm.received_commands[0]).to match(/ifdown/)
+            expect(comm.received_commands[0]).to match(/nmcli c reload/)
+            expect(comm.received_commands[0]).to match(/ifup/)
+            expect(comm.received_commands[0]).to_not match(/nmcli c up/)
+            expect(comm.received_commands[0]).to_not match(/nmcli d disconnect/)
+          end
+        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/)
+            expect(comm.received_commands[0]).to match(/ifdown/)
+            expect(comm.received_commands[0]).to_not match(/ifup/)
+          end
+        end
+
+        context "with nm_controlled option set to false" do
+          let(:networks){ [[{nm_controlled: false}], [{nm_controlled: false}]] }
+
+          it "creates and starts the networks via ifup " do
+            cap.configure_networks(machine, [network_1, network_2])
+            expect(comm.received_commands[0]).to match(/ifup/)
+            expect(comm.received_commands[0]).to match(/ifdown/)
+            expect(comm.received_commands[0]).to match(/nmcli c reload/)
+            expect(comm.received_commands[0]).to_not match(/nmcli c up/)
+            expect(comm.received_commands[0]).to_not match(/nmcli d disconnect/)
+          end
+        end
+
+        context "with nm_controlled option set to false on first device" do
+          let(:networks){ [[{nm_controlled: false}], [{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])
+            expect(comm.received_commands[0]).to_not match(/nmcli.*disconnect/)
+            expect(comm.received_commands[0]).to match(/ifdown/)
+            expect(comm.received_commands[0]).to match(/ifup.*eth1/)
+            expect(comm.received_commands[0]).to match(/nmcli.*reload/)
+          end
+        end
+      end
+    end
+
+    context "without NetworkManager installed" do
+      before do
+        allow(cap).to receive(:nmcli?).and_return false
+      end
+
+      context "with nm_controlled option omitted" do
+
+        it "creates and starts the networks manually" do
+          cap.configure_networks(machine, [network_1, network_2])
+          expect(comm.received_commands[0]).to match(/ifdown/)
+          expect(comm.received_commands[0]).to match(/ifup/)
+          expect(comm.received_commands[0]).to_not match(/nmcli/)
+        end
+      end
+
+      context "with nm_controlled option set" do
+        let(:networks){ [[{nm_controlled: false}], [{nm_controlled: true}]] }
+
+        it "raises an error" do
+          expect{ cap.configure_networks(machine, [network_1, network_2]) }.to raise_error(Vagrant::Errors::NetworkManagerNotInstalled)
+        end
+      end
     end
   end
 end

test/unit/templates/guests/redhat/network_dhcp_test.rb

@@ -15,7 +15,25 @@ describe "templates/guests/redhat/network_dhcp" do
       BOOTPROTO=dhcp
       ONBOOT=yes
       DEVICE=en0
+      NM_CONTROLLED=no
       #VAGRANT-END
     EOH
   end
+
+  it "renders the template with NetworkManager enabled" do
+    result = Vagrant::Util::TemplateRenderer.render(template, options: {
+      device: "en0",
+      nm_controlled: "yes"
+    })
+    expect(result).to eq <<-EOH.gsub(/^ {6}/, "")
+      #VAGRANT-BEGIN
+      # The contents below are automatically generated by Vagrant. Do not modify.
+      BOOTPROTO=dhcp
+      ONBOOT=yes
+      DEVICE=en0
+      NM_CONTROLLED=yes
+      #VAGRANT-END
+    EOH
+  end
+
 end