Merge pull request #1893 from nathanleclaire/fix_hostonlyif_oops

Fix failing case creating host only interface
docker · Sep 23, 2015 · f2bb2e0 · f2bb2e0
2 parents 4ced2d8 + 3b85723
commit f2bb2e0
Show file tree

Hide file tree

Showing 3 changed files with 108 additions and 114 deletions.
diff --git a/drivers/virtualbox/network.go b/drivers/virtualbox/network.go
@@ -125,17 +125,18 @@ func listHostOnlyNetworks() (map[string]*hostOnlyNetwork, error) {
 	return m, nil
 }
 
-func getHostOnlyNetwork(nets map[string]*hostOnlyNetwork, hostIP net.IP, netmask net.IPMask) (*hostOnlyNetwork, error) {
+func getHostOnlyNetwork(nets map[string]*hostOnlyNetwork, hostIP net.IP, netmask net.IPMask) *hostOnlyNetwork {
 	for _, n := range nets {
 		// Second part of this conditional handles a race where
 		// VirtualBox returns us the incorrect netmask value for the
 		// newly created interface.
 		if hostIP.Equal(n.IPv4.IP) &&
 			(netmask.String() == n.IPv4.Mask.String() || n.IPv4.Mask.String() == buggyNetmask) {
-			return n, nil
+			return n
 		}
 	}
-	return nil, errors.New("Valid host only network not found")
+
+	return nil
 }
 
 func getOrCreateHostOnlyNetwork(hostIP net.IP, netmask net.IPMask, dhcpIP net.IP, dhcpUpperIP net.IP, dhcpLowerIP net.IP) (*hostOnlyNetwork, error) {
@@ -144,30 +145,29 @@ func getOrCreateHostOnlyNetwork(hostIP net.IP, netmask net.IPMask, dhcpIP net.IP
 		return nil, err
 	}
 
-	hostOnlyNet, err := getHostOnlyNetwork(nets, hostIP, netmask)
-	if err != nil {
-		return hostOnlyNet, err
-	}
+	hostOnlyNet := getHostOnlyNetwork(nets, hostIP, netmask)
 
-	// No existing host-only interface found. Create a new one.
-	hostOnlyNet, err = createHostonlyNet()
-	if err != nil {
-		return nil, err
-	}
-	hostOnlyNet.IPv4.IP = hostIP
-	hostOnlyNet.IPv4.Mask = netmask
-	if err := hostOnlyNet.Save(); err != nil {
-		return nil, err
-	}
+	if hostOnlyNet == nil {
+		// No existing host-only interface found. Create a new one.
+		hostOnlyNet, err = createHostonlyNet()
+		if err != nil {
+			return nil, err
+		}
+		hostOnlyNet.IPv4.IP = hostIP
+		hostOnlyNet.IPv4.Mask = netmask
+		if err := hostOnlyNet.Save(); err != nil {
+			return nil, err
+		}
 
-	dhcp := dhcpServer{}
-	dhcp.IPv4.IP = dhcpIP
-	dhcp.IPv4.Mask = netmask
-	dhcp.LowerIP = dhcpUpperIP
-	dhcp.UpperIP = dhcpLowerIP
-	dhcp.Enabled = true
-	if err := addHostonlyDHCP(hostOnlyNet.Name, dhcp); err != nil {
-		return nil, err
+		dhcp := dhcpServer{}
+		dhcp.IPv4.IP = dhcpIP
+		dhcp.IPv4.Mask = netmask
+		dhcp.LowerIP = dhcpUpperIP
+		dhcp.UpperIP = dhcpLowerIP
+		dhcp.Enabled = true
+		if err := addHostonlyDHCP(hostOnlyNet.Name, dhcp); err != nil {
+			return nil, err
+		}
 	}
 
 	return hostOnlyNet, nil

diff --git a/drivers/virtualbox/network_test.go b/drivers/virtualbox/network_test.go
@@ -1 +1,84 @@
 package virtualbox
+
+import (
+	"net"
+	"reflect"
+	"testing"
+)
+
+// Tests that when we have a host only network which matches our expectations,
+// it gets returned correctly.
+func TestGetHostOnlyNetworkHappy(t *testing.T) {
+	cidr := "192.168.99.0/24"
+	ip, ipnet, err := net.ParseCIDR(cidr)
+	if err != nil {
+		t.Fatalf("Error parsing cidr: %s", err)
+	}
+	expectedHostOnlyNetwork := &hostOnlyNetwork{
+		IPv4: *ipnet,
+	}
+	vboxNets := map[string]*hostOnlyNetwork{
+		"HostInterfaceNetworking-vboxnet0": expectedHostOnlyNetwork,
+	}
+
+	n := getHostOnlyNetwork(vboxNets, ip, ipnet.Mask)
+	if !reflect.DeepEqual(n, expectedHostOnlyNetwork) {
+		t.Fatalf("Expected result of calling getHostOnlyNetwork to be the same as expected but it was not:\nexpected: %+v\nactual: %+v\n", expectedHostOnlyNetwork, n)
+	}
+}
+
+// Tests that we are able to properly detect when a host only network which
+// matches our expectations can not be found.
+func TestGetHostOnlyNetworkNotFound(t *testing.T) {
+	// Note that this has a different ip is different from "ip" below.
+	cidr := "192.168.99.0/24"
+	ip, ipnet, err := net.ParseCIDR(cidr)
+	if err != nil {
+		t.Fatalf("Error parsing cidr: %s", err)
+	}
+
+	ip = net.ParseIP("192.168.59.0").To4()
+
+	// Suppose a vbox net is created, but it doesn't align with our
+	// expectation.
+	vboxNet := &hostOnlyNetwork{
+		IPv4: *ipnet,
+	}
+	vboxNets := map[string]*hostOnlyNetwork{
+		"HostInterfaceNetworking-vboxnet0": vboxNet,
+	}
+
+	n := getHostOnlyNetwork(vboxNets, ip, ipnet.Mask)
+	if n != nil {
+		t.Fatalf("Expected vbox net to be nil but it has a value: %+v\n", n)
+	}
+}
+
+// Tests a special case where Virtualbox creates the host only network
+// successfully but mis-reports the netmask.
+func TestGetHostOnlyNetworkWindows10Bug(t *testing.T) {
+	cidr := "192.168.99.0/24"
+	ip, ipnet, err := net.ParseCIDR(cidr)
+	if err != nil {
+		t.Fatalf("Error parsing cidr: %s", err)
+	}
+
+	// This is a faulty netmask: a VirtualBox bug causes it to be
+	// misreported.
+	ipnet.Mask = net.IPMask(net.ParseIP("15.0.0.0").To4())
+
+	expectedHostOnlyNetwork := &hostOnlyNetwork{
+		IPv4: *ipnet,
+	}
+
+	vboxNets := map[string]*hostOnlyNetwork{
+		"HostInterfaceNetworking-vboxnet0": expectedHostOnlyNetwork,
+	}
+
+	// The Mask that we are passing in will be the "legitimate" mask, so it
+	// must differ from the magic buggy mask.
+	n := getHostOnlyNetwork(vboxNets, ip, net.IPMask(net.ParseIP("255.255.255.0").To4()))
+	if !reflect.DeepEqual(n, expectedHostOnlyNetwork) {
+		t.Fatalf("Expected result of calling getHostOnlyNetwork to be the same as expected but it was not:\nexpected: %+v\nactual: %+v\n", expectedHostOnlyNetwork, n)
+	}
+}
diff --git a/drivers/virtualbox/virtualbox_test.go b/drivers/virtualbox/virtualbox_test.go
@@ -2,7 +2,6 @@ package virtualbox
 
 import (
 	"net"
-	"reflect"
 	"testing"
 )
 
@@ -30,91 +29,3 @@ func TestGetRandomIPinSubnet(t *testing.T) {
 		t.Fatalf("expected third octet of %d; received %d", testIP[2], newIP[2])
 	}
 }
-
-// Tests that when we have a host only network which matches our expectations,
-// it gets returned correctly.
-func TestGetHostOnlyNetworkHappy(t *testing.T) {
-	cidr := "192.168.99.0/24"
-	ip, ipnet, err := net.ParseCIDR(cidr)
-	if err != nil {
-		t.Fatalf("Error parsing cidr: %s", err)
-	}
-	expectedHostOnlyNetwork := &hostOnlyNetwork{
-		IPv4: *ipnet,
-	}
-	vboxNets := map[string]*hostOnlyNetwork{
-		"HostInterfaceNetworking-vboxnet0": expectedHostOnlyNetwork,
-	}
-
-	n, err := getHostOnlyNetwork(vboxNets, ip, ipnet.Mask)
-	if err != nil {
-		t.Fatalf("Was not expecting an error calling getHostOnlyNetwork: %s", err)
-	}
-
-	if !reflect.DeepEqual(n, expectedHostOnlyNetwork) {
-		t.Fatalf("Expected result of calling getHostOnlyNetwork to be the same as expected but it was not:\nexpected: %+v\nactual: %+v\n", expectedHostOnlyNetwork, n)
-	}
-}
-
-// Tests that we are able to properly detect when a host only network which
-// matches our expectations can not be found.
-func TestGetHostOnlyNetworkNotFound(t *testing.T) {
-	// Note that this has a different ip is different from "ip" below.
-	cidr := "192.168.99.0/24"
-	ip, ipnet, err := net.ParseCIDR(cidr)
-	if err != nil {
-		t.Fatalf("Error parsing cidr: %s", err)
-	}
-
-	ip = net.ParseIP("192.168.59.0").To4()
-
-	// Suppose a vbox net is created, but it doesn't align with our
-	// expectation.
-	vboxNet := &hostOnlyNetwork{
-		IPv4: *ipnet,
-	}
-	vboxNets := map[string]*hostOnlyNetwork{
-		"HostInterfaceNetworking-vboxnet0": vboxNet,
-	}
-
-	n, err := getHostOnlyNetwork(vboxNets, ip, ipnet.Mask)
-	if err == nil {
-		t.Fatalf("Expected to get an error but instead got no error")
-	}
-	if n != nil {
-		t.Fatalf("Expected vbox net to be nil but it has a value: %+v\n", n)
-	}
-}
-
-// Tests a special case where Virtualbox creates the host only network
-// successfully but mis-reports the netmask.
-func TestGetHostOnlyNetworkWindows10Bug(t *testing.T) {
-	cidr := "192.168.99.0/24"
-	ip, ipnet, err := net.ParseCIDR(cidr)
-	if err != nil {
-		t.Fatalf("Error parsing cidr: %s", err)
-	}
-
-	// This is a faulty netmask: a VirtualBox bug causes it to be
-	// misreported.
-	ipnet.Mask = net.IPMask(net.ParseIP("15.0.0.0").To4())
-
-	expectedHostOnlyNetwork := &hostOnlyNetwork{
-		IPv4: *ipnet,
-	}
-
-	vboxNets := map[string]*hostOnlyNetwork{
-		"HostInterfaceNetworking-vboxnet0": expectedHostOnlyNetwork,
-	}
-
-	// The Mask that we are passing in will be the "legitimate" mask, so it
-	// must differ from the magic buggy mask.
-	n, err := getHostOnlyNetwork(vboxNets, ip, net.IPMask(net.ParseIP("255.255.255.0").To4()))
-	if err != nil {
-		t.Fatalf("Was not expecting an error calling getHostOnlyNetwork: %s", err)
-	}
-
-	if !reflect.DeepEqual(n, expectedHostOnlyNetwork) {
-		t.Fatalf("Expected result of calling getHostOnlyNetwork to be the same as expected but it was not:\nexpected: %+v\nactual: %+v\n", expectedHostOnlyNetwork, n)
-	}
-}