From 0b5f4c8454f1f650e6ec2417c3c5a900ba1cab4f Mon Sep 17 00:00:00 2001 From: Ron Williams Date: Sat, 19 Sep 2015 00:04:52 -0700 Subject: [PATCH 1/2] Handle bad netmask returned by virtualbox after hostonlyif creation. Fixes #1843 Signed-off-by: Ron Williams --- drivers/virtualbox/network.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/virtualbox/network.go b/drivers/virtualbox/network.go index 95f569a353..f1dfca89ec 100644 --- a/drivers/virtualbox/network.go +++ b/drivers/virtualbox/network.go @@ -127,8 +127,11 @@ func getHostOnlyNetwork(hostIP net.IP, netmask net.IPMask) (*hostOnlyNetwork, er return nil, err } 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 if hostIP.Equal(n.IPv4.IP) && - netmask.String() == n.IPv4.Mask.String() { + (netmask.String() == n.IPv4.Mask.String() || n.IPv4.Mask.String() == "0f000000") { return n, nil } } From fe5b5cf86d912c568435dd376bf9c192654e9e42 Mon Sep 17 00:00:00 2001 From: Nathan LeClaire Date: Tue, 22 Sep 2015 15:48:23 -0700 Subject: [PATCH 2/2] Add tests for host only network retrieval feature Signed-off-by: Nathan LeClaire --- drivers/virtualbox/network.go | 30 +++++---- drivers/virtualbox/virtualbox_test.go | 89 +++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 12 deletions(-) diff --git a/drivers/virtualbox/network.go b/drivers/virtualbox/network.go index f1dfca89ec..939e9ec8e8 100644 --- a/drivers/virtualbox/network.go +++ b/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 { diff --git a/drivers/virtualbox/virtualbox_test.go b/drivers/virtualbox/virtualbox_test.go index 5df5e5f4cb..c640799d0b 100644 --- a/drivers/virtualbox/virtualbox_test.go +++ b/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) + } +}