providers/virtualbox: extract reading dhcpservers from hostonlyifs

This is just a refactor, no behavior change.

Instead of stitching together dhcpserver info in the structure returned
from `read_host_only_interfaces`, sprout a new driver method called
`read_dhcp_servers` to return that information separately.

This means that driver clients (well there's really only _one_ client in
`ProviderVirtualBox::Action::Network`) have to do a bit more work to get
interface and DHCP server information.

But this gives us (a) a cleaner and more consistent driver interface and
(b) groundwork for a fix for #3083, which will require interacting with
DHCP servers outside of the context of host-only interfaces.

plugins/providers/virtualbox/action/network.rb

@@ -329,10 +329,11 @@ module VagrantPlugins
           if config[:type] == :dhcp
             # Check that if there is a DHCP server attached on our interface,
             # then it is identical. Otherwise, we can't set it.
-            if interface[:dhcp]
+            existing_dhcp_server = find_matching_dhcp_server(interface)
-              valid = interface[:dhcp][:ip] == config[:dhcp_ip] &&
+            if existing_dhcp_server
-                  interface[:dhcp][:lower] == config[:dhcp_lower] &&
+              valid = existing_dhcp_server[:ip] == config[:dhcp_ip] &&
-                  interface[:dhcp][:upper] == config[:dhcp_upper]
+                  existing_dhcp_server[:lower] == config[:dhcp_lower] &&
+                  existing_dhcp_server[:upper] == config[:dhcp_upper]
 
               raise Vagrant::Errors::NetworkDHCPAlreadyAttached if !valid
 
@@ -471,6 +472,16 @@ module VagrantPlugins
 
           nil
         end
+
+        def find_matching_dhcp_server(interface)
+          dhcp_servers.detect do |dhcp_server|
+            interface[:name] && interface[:name] == dhcp_server[:network]
+          end
+        end
+
+        def dhcp_servers
+          @dhcp_servers ||= @env[:machine].provider.driver.read_dhcp_servers
+        end
       end
     end
   end

plugins/providers/virtualbox/driver/base.rb

@@ -180,6 +180,22 @@ module VagrantPlugins
         def read_bridged_interfaces
         end
 
+        # Returns a list of configured DHCP servers
+        #
+        # Each DHCP server is represented as a Hash with the following details:
+        #
+        # {
+        #  :network => String, # name of the associated network interface as
+        #                      #   parsed from the NetworkName, e.g. "vboxnet0"
+        #  :ip      => String, # IP address of the DHCP server, e.g. "172.28.128.2"
+        #  :lower   => String, # lower IP address of the DHCP lease range, e.g. "172.28.128.3"
+        #  :upper   => String, # upper IP address of the DHCP lease range, e.g. "172.28.128.254"
+        # }
+        #
+        # @return [Array<Hash>] See comment above for details
+        def read_dhcp_servers
+        end
+
         # Returns the guest additions version that is installed on this VM.
         #
         # @return [String]
@@ -196,7 +212,16 @@ module VagrantPlugins
 
         # Returns a list of available host only interfaces.
         #
-        # @return [Hash]
+        # Each interface is represented as a Hash with the following details:
+        #
+        # {
+        #  :name    => String, # interface name, e.g. "vboxnet0"
+        #  :ip      => String, # IP address of the interface, e.g. "172.28.128.1"
+        #  :netmask => String, # netmask associated with the interface, e.g. "255.255.255.0"
+        #  :status  => String, # status of the interface, e.g. "Up", "Down"
+        # }
+        #
+        # @return [Array<Hash>] See comment above for details
         def read_host_only_interfaces
         end
 

plugins/providers/virtualbox/driver/meta.rb

@@ -96,6 +96,7 @@ module VagrantPlugins
           :import,
           :read_forwarded_ports,
           :read_bridged_interfaces,
+          :read_dhcp_servers,
           :read_guest_additions_version,
           :read_guest_ip,
           :read_guest_property,

plugins/providers/virtualbox/driver/version_4_0.rb

@@ -255,6 +255,26 @@ module VagrantPlugins
           end
         end
 
+        def read_dhcp_servers
+          execute("list", "dhcpservers", retryable: true).split("\n\n").collect do |block|
+            info = {}
+
+            block.split("\n").each do |line|
+              if network = line[/^NetworkName:\s+HostInterfaceNetworking-(.+?)$/, 1]
+                info[:network] = network
+              elsif ip = line[/^IP:\s+(.+?)$/, 1]
+                info[:ip] = ip
+              elsif lower = line[/^lowerIPAddress:\s+(.+?)$/, 1]
+                info[:lower] = lower
+              elsif upper = line[/^upperIPAddress:\s+(.+?)$/, 1]
+                info[:upper] = upper
+              end
+            end
+
+            info
+          end
+        end
+
         def read_guest_additions_version
           output = execute("guestproperty", "get", @uuid, "/VirtualBox/GuestAdd/Version",
                            retryable: true)
@@ -287,26 +307,6 @@ module VagrantPlugins
         end
 
         def read_host_only_interfaces
-          dhcp = {}
-          execute("list", "dhcpservers", retryable: true).split("\n\n").each do |block|
-            info = {}
-
-            block.split("\n").each do |line|
-              if network = line[/^NetworkName:\s+HostInterfaceNetworking-(.+?)$/, 1]
-                info[:network] = network
-              elsif ip = line[/^IP:\s+(.+?)$/, 1]
-                info[:ip] = ip
-              elsif lower = line[/^lowerIPAddress:\s+(.+?)$/, 1]
-                info[:lower] = lower
-              elsif upper = line[/^upperIPAddress:\s+(.+?)$/, 1]
-                info[:upper] = upper
-              end
-            end
-
-            # Set the DHCP info
-            dhcp[info[:network]] = info
-          end
-
           execute("list", "hostonlyifs", retryable: true).split("\n\n").collect do |block|
             info = {}
 
@@ -322,9 +322,6 @@ module VagrantPlugins
               end
             end
 
-            # Set the DHCP info if it exists
-            info[:dhcp] = dhcp[info[:name]] if dhcp[info[:name]]
-
             info
           end
         end

plugins/providers/virtualbox/driver/version_4_1.rb

@@ -260,6 +260,26 @@ module VagrantPlugins
           end
         end
 
+        def read_dhcp_servers
+          execute("list", "dhcpservers", retryable: true).split("\n\n").collect do |block|
+            info = {}
+
+            block.split("\n").each do |line|
+              if network = line[/^NetworkName:\s+HostInterfaceNetworking-(.+?)$/, 1]
+                info[:network] = network
+              elsif ip = line[/^IP:\s+(.+?)$/, 1]
+                info[:ip] = ip
+              elsif lower = line[/^lowerIPAddress:\s+(.+?)$/, 1]
+                info[:lower] = lower
+              elsif upper = line[/^upperIPAddress:\s+(.+?)$/, 1]
+                info[:upper] = upper
+              end
+            end
+
+            info
+          end
+        end
+
         def read_guest_additions_version
           output = execute("guestproperty", "get", @uuid, "/VirtualBox/GuestAdd/Version",
                            retryable: true)
@@ -292,26 +312,6 @@ module VagrantPlugins
         end
 
         def read_host_only_interfaces
-          dhcp = {}
-          execute("list", "dhcpservers", retryable: true).split("\n\n").each do |block|
-            info = {}
-
-            block.split("\n").each do |line|
-              if network = line[/^NetworkName:\s+HostInterfaceNetworking-(.+?)$/, 1]
-                info[:network] = network
-              elsif ip = line[/^IP:\s+(.+?)$/, 1]
-                info[:ip] = ip
-              elsif lower = line[/^lowerIPAddress:\s+(.+?)$/, 1]
-                info[:lower] = lower
-              elsif upper = line[/^upperIPAddress:\s+(.+?)$/, 1]
-                info[:upper] = upper
-              end
-            end
-
-            # Set the DHCP info
-            dhcp[info[:network]] = info
-          end
-
           execute("list", "hostonlyifs", retryable: true).split("\n\n").collect do |block|
             info = {}
 
@@ -327,9 +327,6 @@ module VagrantPlugins
               end
             end
 
-            # Set the DHCP info if it exists
-            info[:dhcp] = dhcp[info[:name]] if dhcp[info[:name]]
-
             info
           end
         end

plugins/providers/virtualbox/driver/version_4_2.rb

@@ -283,6 +283,26 @@ module VagrantPlugins
           end
         end
 
+        def read_dhcp_servers
+          execute("list", "dhcpservers", retryable: true).split("\n\n").collect do |block|
+            info = {}
+
+            block.split("\n").each do |line|
+              if line =~ /^NetworkName:\s+HostInterfaceNetworking-(.+?)$/
+                info[:network] = $1.to_s
+              elsif line =~ /^IP:\s+(.+?)$/
+                info[:ip] = $1.to_s
+              elsif line =~ /^lowerIPAddress:\s+(.+?)$/
+                info[:lower] = $1.to_s
+              elsif line =~ /^upperIPAddress:\s+(.+?)$/
+                info[:upper] = $1.to_s
+              end
+            end
+
+            info
+          end
+        end
+
         def read_guest_additions_version
           output = execute("guestproperty", "get", @uuid, "/VirtualBox/GuestAdd/Version",
                            retryable: true)
@@ -323,26 +343,6 @@ module VagrantPlugins
         end
 
         def read_host_only_interfaces
-          dhcp = {}
-          execute("list", "dhcpservers", retryable: true).split("\n\n").each do |block|
-            info = {}
-
-            block.split("\n").each do |line|
-              if line =~ /^NetworkName:\s+HostInterfaceNetworking-(.+?)$/
-                info[:network] = $1.to_s
-              elsif line =~ /^IP:\s+(.+?)$/
-                info[:ip] = $1.to_s
-              elsif line =~ /^lowerIPAddress:\s+(.+?)$/
-                info[:lower] = $1.to_s
-              elsif line =~ /^upperIPAddress:\s+(.+?)$/
-                info[:upper] = $1.to_s
-              end
-            end
-
-            # Set the DHCP info
-            dhcp[info[:network]] = info
-          end
-
           execute("list", "hostonlyifs", retryable: true).split("\n\n").collect do |block|
             info = {}
 
@@ -358,9 +358,6 @@ module VagrantPlugins
               end
             end
 
-            # Set the DHCP info if it exists
-            info[:dhcp] = dhcp[info[:name]] if dhcp[info[:name]]
-
             info
           end
         end

plugins/providers/virtualbox/driver/version_4_3.rb

@@ -292,6 +292,26 @@ module VagrantPlugins
           end
         end
 
+        def read_dhcp_servers
+          execute("list", "dhcpservers", retryable: true).split("\n\n").collect do |block|
+            info = {}
+
+            block.split("\n").each do |line|
+              if line =~ /^NetworkName:\s+HostInterfaceNetworking-(.+?)$/
+                info[:network] = $1.to_s
+              elsif line =~ /^IP:\s+(.+?)$/
+                info[:ip] = $1.to_s
+              elsif line =~ /^lowerIPAddress:\s+(.+?)$/
+                info[:lower] = $1.to_s
+              elsif line =~ /^upperIPAddress:\s+(.+?)$/
+                info[:upper] = $1.to_s
+              end
+            end
+
+            info
+          end
+        end
+
         def read_guest_additions_version
           output = execute("guestproperty", "get", @uuid, "/VirtualBox/GuestAdd/Version",
                            retryable: true)
@@ -333,26 +353,6 @@ module VagrantPlugins
         end
 
         def read_host_only_interfaces
-          dhcp = {}
-          execute("list", "dhcpservers", retryable: true).split("\n\n").each do |block|
-            info = {}
-
-            block.split("\n").each do |line|
-              if line =~ /^NetworkName:\s+HostInterfaceNetworking-(.+?)$/
-                info[:network] = $1.to_s
-              elsif line =~ /^IP:\s+(.+?)$/
-                info[:ip] = $1.to_s
-              elsif line =~ /^lowerIPAddress:\s+(.+?)$/
-                info[:lower] = $1.to_s
-              elsif line =~ /^upperIPAddress:\s+(.+?)$/
-                info[:upper] = $1.to_s
-              end
-            end
-
-            # Set the DHCP info
-            dhcp[info[:network]] = info
-          end
-
           execute("list", "hostonlyifs", retryable: true).split("\n\n").collect do |block|
             info = {}
 
@@ -368,9 +368,6 @@ module VagrantPlugins
               end
             end
 
-            # Set the DHCP info if it exists
-            info[:dhcp] = dhcp[info[:name]] if dhcp[info[:name]]
-
             info
           end
         end

test/unit/plugins/providers/virtualbox/action/network_test.rb

@@ -0,0 +1,95 @@
+require_relative "../base"
+
+require "vagrant/util/platform"
+
+describe VagrantPlugins::ProviderVirtualBox::Action::Network do
+  include_context "unit"
+  include_context "virtualbox"
+
+  let(:iso_env) do
+    # We have to create a Vagrantfile so there is a root path
+    env = isolated_environment
+    env.vagrantfile("")
+    env.create_vagrant_env
+  end
+
+  let(:machine) do
+    iso_env.machine(iso_env.machine_names[0], :virtualbox).tap do |m|
+      m.provider.stub(driver: driver)
+    end
+  end
+
+  let(:env)    {{ machine: machine, ui: machine.ui }}
+  let(:app)    { lambda { |*args| }}
+  let(:driver) { double("driver") }
+
+  let(:nics)         { {} }
+
+  subject { described_class.new(app, env) }
+
+  before do
+    allow(driver).to receive(:enable_adapters)
+    allow(driver).to receive(:read_network_interfaces)   { nics }
+  end
+
+  it "calls the next action in the chain" do
+    called = false
+    app = lambda { |*args| called = true }
+
+    action = described_class.new(app, env)
+    action.call(env)
+
+    expect(called).to eq(true)
+  end
+
+  context "with a dhcp private network" do
+    let(:bridgedifs)  { [] }
+    let(:hostonlyifs) { [] }
+    let(:dhcpservers) { [] }
+    let(:guest)       { double("guest") }
+
+    before do
+      machine.config.vm.network 'private_network', type: :dhcp
+      allow(driver).to receive(:read_bridged_interfaces) { bridgedifs }
+      allow(driver).to receive(:read_host_only_interfaces) { hostonlyifs }
+      allow(driver).to receive(:read_dhcp_servers) { dhcpservers }
+      allow(machine).to receive(:guest) { guest }
+    end
+
+    it "creates a host only interface and a dhcp server using default ips, then tells the guest to configure the network after boot" do
+      allow(driver).to receive(:create_host_only_network) {{ name: 'HostInterfaceNetworking-vboxnet0' }}
+      allow(driver).to receive(:create_dhcp_server)
+      allow(guest).to receive(:capability)
+
+      subject.call(env)
+
+      expect(driver).to have_received(:create_host_only_network).with({
+        adapter_ip: '172.28.128.1',
+        netmask: '255.255.255.0',
+      })
+
+      expect(driver).to have_received(:create_dhcp_server).with('HostInterfaceNetworking-vboxnet0', {
+        adapter_ip: "172.28.128.1",
+        auto_config: true,
+        ip: "172.28.128.1",
+        mac: nil,
+        netmask: "255.255.255.0",
+        nic_type: nil,
+        type: :dhcp,
+        dhcp_ip: "172.28.128.2",
+        dhcp_lower: "172.28.128.3",
+        dhcp_upper: "172.28.128.254",
+        adapter: 2
+      })
+
+      expect(guest).to have_received(:capability).with(:configure_networks, [{
+        type: :dhcp,
+        adapter_ip: "172.28.128.1",
+        ip: "172.28.128.1",
+        netmask: "255.255.255.0",
+        auto_config: true,
+        interface: nil
+      }])
+    end
+  end
+end

test/unit/plugins/providers/virtualbox/support/shared/virtualbox_driver_version_4_x_examples.rb

@@ -3,6 +3,74 @@ shared_examples "a version 4.x virtualbox driver" do |options|
     raise ArgumentError, "Need virtualbox context to use these shared examples." if !(defined? vbox_context)
   end
 
+  describe "read_dhcp_servers" do
+    before {
+      expect(subprocess).to receive(:execute).
+        with("VBoxManage", "list", "dhcpservers", an_instance_of(Hash)).
+        and_return(subprocess_result(stdout: output))
+    }
+
+    context "with empty output" do
+      let(:output) { "" }
+
+      it "returns an empty list" do
+        expect(subject.read_dhcp_servers).to eq([])
+      end
+    end
+
+    context "with a single dhcp server" do
+      let(:output) {
+        <<-OUTPUT.gsub(/^ */, '')
+          NetworkName:    HostInterfaceNetworking-vboxnet0
+          IP:             172.28.128.2
+          NetworkMask:    255.255.255.0
+          lowerIPAddress: 172.28.128.3
+          upperIPAddress: 172.28.128.254
+          Enabled:        Yes
+
+        OUTPUT
+      }
+
+
+      it "returns a list with one entry describing that server" do
+        expect(subject.read_dhcp_servers).to eq([{
+          network: 'vboxnet0',
+          ip:      '172.28.128.2',
+          lower:   '172.28.128.3',
+          upper:   '172.28.128.254',
+        }])
+      end
+    end
+
+    context "with a multiple dhcp servers" do
+      let(:output) {
+        <<-OUTPUT.gsub(/^ */, '')
+          NetworkName:    HostInterfaceNetworking-vboxnet0
+          IP:             172.28.128.2
+          NetworkMask:    255.255.255.0
+          lowerIPAddress: 172.28.128.3
+          upperIPAddress: 172.28.128.254
+          Enabled:        Yes
+
+          NetworkName:    HostInterfaceNetworking-vboxnet1
+          IP:             10.0.0.2
+          NetworkMask:    255.255.255.0
+          lowerIPAddress: 10.0.0.3
+          upperIPAddress: 10.0.0.254
+          Enabled:        Yes
+        OUTPUT
+      }
+
+
+      it "returns a list with one entry for each server" do
+        expect(subject.read_dhcp_servers).to eq([
+          {network: 'vboxnet0', ip: '172.28.128.2', lower: '172.28.128.3', upper: '172.28.128.254'},
+          {network: 'vboxnet1', ip: '10.0.0.2', lower: '10.0.0.3', upper: '10.0.0.254'},
+        ])
+      end
+    end
+  end
+
   describe "read_guest_property" do
     it "reads the guest property of the machine referenced by the UUID" do
       key  = "/Foo/Bar"
@@ -50,4 +118,86 @@ shared_examples "a version 4.x virtualbox driver" do |options|
         to raise_error Vagrant::Errors::VirtualBoxGuestPropertyNotFound
     end
   end
+
+  describe "read_host_only_interfaces" do
+    before {
+      expect(subprocess).to receive(:execute).
+        with("VBoxManage", "list", "hostonlyifs", an_instance_of(Hash)).
+        and_return(subprocess_result(stdout: output))
+    }
+
+    context "with empty output" do
+      let(:output) { "" }
+
+      it "returns an empty list" do
+        expect(subject.read_host_only_interfaces).to eq([])
+      end
+    end
+
+    context "with a single host only interface" do
+      let(:output) {
+        <<-OUTPUT.gsub(/^ */, '')
+          Name:            vboxnet0
+          GUID:            786f6276-656e-4074-8000-0a0027000000
+          DHCP:            Disabled
+          IPAddress:       172.28.128.1
+          NetworkMask:     255.255.255.0
+          IPV6Address:
+          IPV6NetworkMaskPrefixLength: 0
+          HardwareAddress: 0a:00:27:00:00:00
+          MediumType:      Ethernet
+          Status:          Up
+          VBoxNetworkName: HostInterfaceNetworking-vboxnet0
+
+        OUTPUT
+      }
+
+      it "returns a list with one entry describing that interface" do
+        expect(subject.read_host_only_interfaces).to eq([{
+          name:    'vboxnet0',
+          ip:      '172.28.128.1',
+          netmask: '255.255.255.0',
+          status:  'Up',
+        }])
+      end
+    end
+
+    context "with multiple host only interfaces" do
+      let(:output) {
+        <<-OUTPUT.gsub(/^ */, '')
+          Name:            vboxnet0
+          GUID:            786f6276-656e-4074-8000-0a0027000000
+          DHCP:            Disabled
+          IPAddress:       172.28.128.1
+          NetworkMask:     255.255.255.0
+          IPV6Address:
+          IPV6NetworkMaskPrefixLength: 0
+          HardwareAddress: 0a:00:27:00:00:00
+          MediumType:      Ethernet
+          Status:          Up
+          VBoxNetworkName: HostInterfaceNetworking-vboxnet0
+
+          Name:            vboxnet1
+          GUID:            5764a976-8479-8388-1245-8a0048080840
+          DHCP:            Disabled
+          IPAddress:       10.0.0.1
+          NetworkMask:     255.255.255.0
+          IPV6Address:
+          IPV6NetworkMaskPrefixLength: 0
+          HardwareAddress: 0a:00:27:00:00:01
+          MediumType:      Ethernet
+          Status:          Up
+          VBoxNetworkName: HostInterfaceNetworking-vboxnet1
+
+        OUTPUT
+      }
+
+      it "returns a list with one entry for each interface" do
+        expect(subject.read_host_only_interfaces).to eq([
+          {name: 'vboxnet0', ip: '172.28.128.1', netmask: '255.255.255.0', status: 'Up'},
+          {name: 'vboxnet1', ip: '10.0.0.1', netmask: '255.255.255.0', status: 'Up'},
+        ])
+      end
+    end
+  end
 end