Merge pull request #1 from nathanleclaire/rwilliams-vbox-duplicate-ho…

…stonlyif

Add tests for host only network retrieval feature

Signed-off-by: Ron Williams <ron.a.williams@gmail.com>

drivers/virtualbox/network.go

@@ -10,6 +10,10 @@ import (
 	"strings"
 )
 
+const (
+	buggyNetmask = "0f000000"
+)
+
 var (
 	reHostonlyInterfaceCreated = regexp.MustCompile(`Interface '(.+)' was successfully created`)
 )
@@ -121,28 +125,30 @@ func listHostOnlyNetworks() (map[string]*hostOnlyNetwork, error) {
 	return m, nil
 }
 
-func getHostOnlyNetwork(hostIP net.IP, netmask net.IPMask) (*hostOnlyNetwork, error) {
-	nets, err := listHostOnlyNetworks()
-	if err != nil {
-		return nil, err
-	}
+func getHostOnlyNetwork(nets map[string]*hostOnlyNetwork, hostIP net.IP, netmask net.IPMask) (*hostOnlyNetwork, error) {
 	for _, n := range nets {
-		//handle known issue where vbox throws us a bad netmask
-		//if the value returned is "0f000000" it's the newly
-		//created adpater and should be the right one
+		// 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() == "0f000000") {
+			(netmask.String() == n.IPv4.Mask.String() || n.IPv4.Mask.String() == buggyNetmask) {
 			return n, nil
 		}
 	}
-	return nil, nil
+	return nil, errors.New("Valid host only network not found")
 }
 
 func getOrCreateHostOnlyNetwork(hostIP net.IP, netmask net.IPMask, dhcpIP net.IP, dhcpUpperIP net.IP, dhcpLowerIP net.IP) (*hostOnlyNetwork, error) {
-	hostOnlyNet, err := getHostOnlyNetwork(hostIP, netmask)
-	if err != nil || hostOnlyNet != nil {
+	nets, err := listHostOnlyNetworks()
+	if err != nil {
+		return nil, err
+	}
+
+	hostOnlyNet, err := getHostOnlyNetwork(nets, hostIP, netmask)
+	if err != nil {
 		return hostOnlyNet, err
 	}
+
 	// No existing host-only interface found. Create a new one.
 	hostOnlyNet, err = createHostonlyNet()
 	if err != nil {

drivers/virtualbox/virtualbox_test.go

@@ -2,6 +2,7 @@ package virtualbox
 
 import (
 	"net"
+	"reflect"
 	"testing"
 )
 
@@ -29,3 +30,91 @@ 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)
+	}
+}