From e2a67686a29898aa5c63e064cea16df5323e9c44 Mon Sep 17 00:00:00 2001
From: Benjamin Gandon <bgandon@users.noreply.github.com>
Date: Wed, 14 Feb 2024 10:05:13 +0100
Subject: [PATCH 01/10] No need for complicated variable names, 'err' is just
 fine

---
 src/bosh-virtualbox-cpi/vm/network/add_host_only.go | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
index d8089c73..caeaa716 100644
--- a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
+++ b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
@@ -67,13 +67,13 @@ func (n Networks) createHostOnly(gateway, netmask string) (string, error) {
 		cidrRange, _ := net.IPv4Mask(addr[0], addr[1], addr[2], addr[3]).Size()
 		_, subnet, err := net.ParseCIDR(fmt.Sprintf("%s/%v", gateway, cidrRange))
 
-		lowerIp, errGetFirstIP := systemInfo.GetFirstIP(subnetFirstIP)
-		if errGetFirstIP != nil {
-			return "", errGetFirstIP
+		lowerIp, err := systemInfo.GetFirstIP(subnetFirstIP)
+		if err != nil {
+			return "", err
 		}
-		upperIp, errGetLastIP := systemInfo.GetLastIP(subnet)
-		if errGetLastIP != nil {
-			return "", errGetLastIP
+		upperIp, err := systemInfo.GetLastIP(subnet)
+		if err != nil {
+			return "", err
 		}
 
 		args := []string{"hostonlynet",

From cf3fa5de938aa6995f7c87997378bf0675ebe585 Mon Sep 17 00:00:00 2001
From: Benjamin Gandon <bgandon@users.noreply.github.com>
Date: Wed, 14 Feb 2024 10:06:11 +0100
Subject: [PATCH 02/10] Improve variable name to be clear about the intent

---
 src/bosh-virtualbox-cpi/vm/network/add_host_only.go | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
index caeaa716..f55e91ce 100644
--- a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
+++ b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
@@ -56,7 +56,7 @@ func (n Networks) createHostOnly(gateway, netmask string) (string, error) {
 
 	var matches []string
 	var errorMessage string
-	var matchesLen int
+	var expectedMatchesLen int
 
 	if systemInfo.IsMacOSXVBoxSpecial6or7Case() {
 		addr := net.ParseIP(netmask).To4()
@@ -105,7 +105,7 @@ func (n Networks) createHostOnly(gateway, netmask string) (string, error) {
 			"Internal inconsistency: Expected len(%s matches) == 1:",
 			createdHostOnlyNetMatch,
 		)
-		matchesLen = 1
+		expectedMatchesLen = 1
 	} else {
 		args := []string{"hostonlyif", "create"}
 		output, err := n.driver.ExecuteComplex(args, driver.ExecuteOpts{})
@@ -117,14 +117,14 @@ func (n Networks) createHostOnly(gateway, netmask string) (string, error) {
 			"Internal inconsistency: Expected len(%s matches) == 2:",
 			createdHostOnlyMatch,
 		)
-		matchesLen = 2
+		expectedMatchesLen = 2
 	}
 
-	if len(matches) != matchesLen {
+	if len(matches) != expectedMatchesLen {
 		panic(errorMessage)
 	}
 
-	return matches[matchesLen-1], nil
+	return matches[expectedMatchesLen-1], nil
 }
 
 func (n Networks) configureHostOnly(name, gateway, netmask string) error {

From 12442c28487341e8bc6468988fb909be00b70beb Mon Sep 17 00:00:00 2001
From: Benjamin Gandon <bgandon@users.noreply.github.com>
Date: Wed, 14 Feb 2024 10:16:18 +0100
Subject: [PATCH 03/10] No need for differentiated method calls as the actual
 code will ignore the arguments in the Vbox v[56].* case

---
 src/bosh-virtualbox-cpi/vm/network/add_host_only.go | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
index f55e91ce..ddacd8f0 100644
--- a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
+++ b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
@@ -13,22 +13,12 @@ var (
 )
 
 func (n Networks) AddHostOnly(name, gateway, netmask string) (bool, error) {
-	systemInfo, err := n.NewSystemInfo()
-	if err != nil {
-		return false, err
-	}
-
 	// VB does not allow naming host-only networks inside version <= 6 , exit if it's not the first one
 	if len(name) > 0 && name != "vboxnet0" {
 		return false, nil
 	}
 
-	var createdName string
-	if systemInfo.IsMacOSXVBoxSpecial6or7Case() {
-		createdName, err = n.createHostOnly(gateway, netmask)
-	} else {
-		createdName, err = n.createHostOnly("", "")
-	}
+	createdName, err := n.createHostOnly(gateway, netmask)
 
 	if err != nil {
 		return true, err

From 59df03228bba6cebb1fb2e3a4d1f7ad3f664d89d Mon Sep 17 00:00:00 2001
From: Benjamin Gandon <bgandon@users.noreply.github.com>
Date: Wed, 14 Feb 2024 10:55:12 +0100
Subject: [PATCH 04/10] Remove TKE specific requirement that is irrelevant here

As they say, they have a special requirement for limiting network masks to
2^16 allocatable IPs only:

	// For IPv6, the max size will be limited to 65536
	// This is due to the allocator keeping track of all the
	// allocated IP's in a bitmap. This will keep the size of
	// the bitmap to 64k.

Ref: https://github.com/tkestack/tke/blob/v1.9.2/pkg/util/ipallocator/allocator.go#L271-L274

But here there in the VirtualBox CPI is no reason for such a limitation. We
just need the correct calculation of mask length.
---
 src/bosh-virtualbox-cpi/vm/network/system_info.go | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/bosh-virtualbox-cpi/vm/network/system_info.go b/src/bosh-virtualbox-cpi/vm/network/system_info.go
index ba5f9816..e65cd1dc 100644
--- a/src/bosh-virtualbox-cpi/vm/network/system_info.go
+++ b/src/bosh-virtualbox-cpi/vm/network/system_info.go
@@ -76,10 +76,6 @@ func rangeSize(subnet *net.IPNet) int64 {
 	if bits == 32 && (bits-ones) >= 31 || bits == 128 && (bits-ones) >= 127 {
 		return 0
 	}
-
-	if bits == 128 && (bits-ones) >= 16 {
-		return int64(1) << uint(16)
-	}
 	return int64(1) << uint(bits-ones)
 }
 

From cc658e0de5b3f84679ca7a525c9a75517cbf4378 Mon Sep 17 00:00:00 2001
From: Benjamin Gandon <bgandon@users.noreply.github.com>
Date: Wed, 14 Feb 2024 10:19:17 +0100
Subject: [PATCH 05/10] Use proper variable names, matching its actual content

---
 src/bosh-virtualbox-cpi/vm/network/add_host_only.go | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
index ddacd8f0..f887cab6 100644
--- a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
+++ b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
@@ -49,13 +49,13 @@ func (n Networks) createHostOnly(gateway, netmask string) (string, error) {
 	var expectedMatchesLen int
 
 	if systemInfo.IsMacOSXVBoxSpecial6or7Case() {
-		addr := net.ParseIP(netmask).To4()
+		maskIP := net.ParseIP(netmask).To4()
 		subnetFirstIP := &net.IPNet{
 			IP:   net.ParseIP(gateway),
-			Mask: net.IPv4Mask(addr[0], addr[1], addr[2], addr[3]),
+			Mask: net.IPv4Mask(maskIP[0], maskIP[1], maskIP[2], maskIP[3]),
 		}
-		cidrRange, _ := net.IPv4Mask(addr[0], addr[1], addr[2], addr[3]).Size()
-		_, subnet, err := net.ParseCIDR(fmt.Sprintf("%s/%v", gateway, cidrRange))
+		maskLength, _ := net.IPv4Mask(maskIP[0], maskIP[1], maskIP[2], maskIP[3]).Size()
+		_, subnet, _ := net.ParseCIDR(fmt.Sprintf("%s/%v", gateway, maskLength))
 
 		lowerIp, err := systemInfo.GetFirstIP(subnetFirstIP)
 		if err != nil {

From 880b3dfc1c74e703a40bc62849e651af7f2522b2 Mon Sep 17 00:00:00 2001
From: Benjamin Gandon <bgandon@users.noreply.github.com>
Date: Thu, 15 Feb 2024 18:02:44 +0100
Subject: [PATCH 06/10] Factor common expressions and properly report errors

---
 .../vm/network/add_host_only.go                 | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
index f887cab6..ba324a1b 100644
--- a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
+++ b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"net"
 	"regexp"
+
+	bosherr "github.com/cloudfoundry/bosh-utils/errors"
 )
 
 var (
@@ -50,11 +52,20 @@ func (n Networks) createHostOnly(gateway, netmask string) (string, error) {
 
 	if systemInfo.IsMacOSXVBoxSpecial6or7Case() {
 		maskIP := net.ParseIP(netmask).To4()
+		if maskIP == nil {
+			return "", bosherr.Errorf("expected netmask to be valid IP v4 (got '%s')", netmask)
+		}
+		gwIP := net.ParseIP(gateway)
+		if gwIP == nil {
+			return "", bosherr.Errorf("expected gateway to be valid IP v4 (got '%s')", gateway)
+		}
+
+		mask := net.IPv4Mask(maskIP[0], maskIP[1], maskIP[2], maskIP[3])
 		subnetFirstIP := &net.IPNet{
-			IP:   net.ParseIP(gateway),
-			Mask: net.IPv4Mask(maskIP[0], maskIP[1], maskIP[2], maskIP[3]),
+			IP:   gwIP,
+			Mask: mask,
 		}
-		maskLength, _ := net.IPv4Mask(maskIP[0], maskIP[1], maskIP[2], maskIP[3]).Size()
+		maskLength, _ := mask.Size()
 		_, subnet, _ := net.ParseCIDR(fmt.Sprintf("%s/%v", gateway, maskLength))
 
 		lowerIp, err := systemInfo.GetFirstIP(subnetFirstIP)

From be20acb4120b969dbac90f45ddbb0698bfafaddf Mon Sep 17 00:00:00 2001
From: Benjamin Gandon <bgandon@users.noreply.github.com>
Date: Thu, 15 Feb 2024 18:16:29 +0100
Subject: [PATCH 07/10] Keep names relevant and focused + Update comments to
 matche the reality

---
 src/bosh-virtualbox-cpi/vm/network/add_host_only.go | 6 +++---
 src/bosh-virtualbox-cpi/vm/network/host_only.go     | 5 +++--
 src/bosh-virtualbox-cpi/vm/network/networks.go      | 4 ++--
 src/bosh-virtualbox-cpi/vm/network/system_info.go   | 5 ++---
 src/bosh-virtualbox-cpi/vm/nics.go                  | 6 ++++--
 5 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
index ba324a1b..3cdcf5e6 100644
--- a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
+++ b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
@@ -50,7 +50,7 @@ func (n Networks) createHostOnly(gateway, netmask string) (string, error) {
 	var errorMessage string
 	var expectedMatchesLen int
 
-	if systemInfo.IsMacOSXVBoxSpecial6or7Case() {
+	if systemInfo.IsMacOSVbox7() {
 		maskIP := net.ParseIP(netmask).To4()
 		if maskIP == nil {
 			return "", bosherr.Errorf("expected netmask to be valid IP v4 (got '%s')", netmask)
@@ -134,7 +134,7 @@ func (n Networks) configureHostOnly(name, gateway, netmask string) error {
 		return err
 	}
 
-	if systemInfo.IsMacOSXVBoxSpecial6or7Case() == false {
+	if systemInfo.IsMacOSVbox7() == false {
 		args := []string{"hostonlyif", "ipconfig", name}
 
 		if len(gateway) > 0 {
@@ -163,7 +163,7 @@ func (n Networks) cleanUpPartialHostOnlyCreate(name string) {
 		"remove",
 		name,
 	}
-	if systemInfo.IsMacOSXVBoxSpecial6or7Case() {
+	if systemInfo.IsMacOSVbox7() {
 		args = []string{
 			"hostonlynet",
 			"remove",
diff --git a/src/bosh-virtualbox-cpi/vm/network/host_only.go b/src/bosh-virtualbox-cpi/vm/network/host_only.go
index c392975b..4ac994a0 100644
--- a/src/bosh-virtualbox-cpi/vm/network/host_only.go
+++ b/src/bosh-virtualbox-cpi/vm/network/host_only.go
@@ -3,9 +3,10 @@ package network
 import (
 	"bosh-virtualbox-cpi/driver"
 	"fmt"
-	boshlog "github.com/cloudfoundry/bosh-utils/logger"
 	"net"
 	"os"
+
+	boshlog "github.com/cloudfoundry/bosh-utils/logger"
 )
 
 type HostOnly struct {
@@ -43,7 +44,7 @@ func (n HostOnly) Enable() error {
 	}
 
 	var finalArgs []string
-	if systemInfo.IsMacOSXVBoxSpecial6or7Case() {
+	if systemInfo.IsMacOSVbox7() {
 		finalArgs = []string{"hostonlynet", "modify", fmt.Sprintf("--name=%s", n.name), "--enable"}
 	} else {
 		args := []string{"hostonlyif", "ipconfig", n.name}
diff --git a/src/bosh-virtualbox-cpi/vm/network/networks.go b/src/bosh-virtualbox-cpi/vm/network/networks.go
index 63298044..5d43bcd0 100644
--- a/src/bosh-virtualbox-cpi/vm/network/networks.go
+++ b/src/bosh-virtualbox-cpi/vm/network/networks.go
@@ -152,7 +152,7 @@ func (n Networks) HostOnlys() ([]Network, error) {
 	}
 
 	commandName := "hostonlyifs"
-	if systemInfo.IsMacOSXVBoxSpecial6or7Case() {
+	if systemInfo.IsMacOSVbox7() {
 		commandName = "hostonlynets"
 	}
 
@@ -218,7 +218,7 @@ func (n Networks) outputChunks(output string) []string {
 	if err != nil {
 		return nil
 	}
-	if systemInfo.IsMacOSXVBoxSpecial6or7Case() {
+	if systemInfo.IsMacOSVbox7() {
 		output = strings.Replace(output, "\n\n", "\n", -1)
 	}
 
diff --git a/src/bosh-virtualbox-cpi/vm/network/system_info.go b/src/bosh-virtualbox-cpi/vm/network/system_info.go
index e65cd1dc..4ebaf6a3 100644
--- a/src/bosh-virtualbox-cpi/vm/network/system_info.go
+++ b/src/bosh-virtualbox-cpi/vm/network/system_info.go
@@ -37,9 +37,8 @@ func (s SystemInfo) GetLastIP(subnet *net.IPNet) (net.IP, error) {
 	return getIndexedIP(subnet, int(size-1))
 }
 
-// IsMacOSXVBoxSpecial6or7Case Identify if you are system is running on MAC OS X and the used
-// VirtualBox version is 6.1 or 7
-func (s SystemInfo) IsMacOSXVBoxSpecial6or7Case() bool {
+// IsMacOSVbox7 Identifies if you're running VirtualBox version 7 on macOS
+func (s SystemInfo) IsMacOSVbox7() bool {
 	if s.osVersion == "darwin" && (s.vBoxMajorVersion == "7") {
 		return true
 	} else {
diff --git a/src/bosh-virtualbox-cpi/vm/nics.go b/src/bosh-virtualbox-cpi/vm/nics.go
index 23ae89eb..548cca10 100644
--- a/src/bosh-virtualbox-cpi/vm/nics.go
+++ b/src/bosh-virtualbox-cpi/vm/nics.go
@@ -2,13 +2,15 @@ package vm
 
 import (
 	"fmt"
-	boshlog "github.com/cloudfoundry/bosh-utils/logger"
 	"math/rand"
 	"os"
 	"strconv"
 	"strings"
 
+	boshlog "github.com/cloudfoundry/bosh-utils/logger"
+
 	network "bosh-virtualbox-cpi/vm/network"
+
 	apiv1 "github.com/cloudfoundry/bosh-cpi-go/apiv1"
 	bosherr "github.com/cloudfoundry/bosh-utils/errors"
 
@@ -76,7 +78,7 @@ func (n NICs) addNIC(nic string, net Network, host Host) (string, error) {
 			return "", err
 		}
 
-		if systemInfo.IsMacOSXVBoxSpecial6or7Case() {
+		if systemInfo.IsMacOSVbox7() {
 			args = append(args, []string{"hostonlynet", "--host-only-net" + nic, actualNet.Name()}...)
 		} else {
 			args = append(args, []string{"hostonly", "--hostonlyadapter" + nic, actualNet.Name()}...)

From 7f024d5708a173aa37083b22c6118617f560ecba Mon Sep 17 00:00:00 2001
From: Benjamin Gandon <bgandon@users.noreply.github.com>
Date: Thu, 15 Feb 2024 18:52:35 +0100
Subject: [PATCH 08/10] Expect later version of VboxManage CLI to stay in line
 with v7 changes

---
 src/bosh-virtualbox-cpi/vm/network/add_host_only.go | 7 +++----
 src/bosh-virtualbox-cpi/vm/network/host_only.go     | 2 +-
 src/bosh-virtualbox-cpi/vm/network/networks.go      | 4 ++--
 src/bosh-virtualbox-cpi/vm/network/system_info.go   | 6 +++---
 src/bosh-virtualbox-cpi/vm/nics.go                  | 2 +-
 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
index 3cdcf5e6..6f8833c6 100644
--- a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
+++ b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
@@ -50,7 +50,7 @@ func (n Networks) createHostOnly(gateway, netmask string) (string, error) {
 	var errorMessage string
 	var expectedMatchesLen int
 
-	if systemInfo.IsMacOSVbox7() {
+	if systemInfo.IsMacOSVboxV7OrLater() {
 		maskIP := net.ParseIP(netmask).To4()
 		if maskIP == nil {
 			return "", bosherr.Errorf("expected netmask to be valid IP v4 (got '%s')", netmask)
@@ -108,8 +108,7 @@ func (n Networks) createHostOnly(gateway, netmask string) (string, error) {
 		)
 		expectedMatchesLen = 1
 	} else {
-		args := []string{"hostonlyif", "create"}
-		output, err := n.driver.ExecuteComplex(args, driver.ExecuteOpts{})
+		output, err := n.driver.Execute("hostonlyif", "create")
 		if err != nil {
 			return "", err
 		}
@@ -134,7 +133,7 @@ func (n Networks) configureHostOnly(name, gateway, netmask string) error {
 		return err
 	}
 
-	if systemInfo.IsMacOSVbox7() == false {
+	if !systemInfo.IsMacOSVboxV7OrLater() {
 		args := []string{"hostonlyif", "ipconfig", name}
 
 		if len(gateway) > 0 {
diff --git a/src/bosh-virtualbox-cpi/vm/network/host_only.go b/src/bosh-virtualbox-cpi/vm/network/host_only.go
index 4ac994a0..d4b0367f 100644
--- a/src/bosh-virtualbox-cpi/vm/network/host_only.go
+++ b/src/bosh-virtualbox-cpi/vm/network/host_only.go
@@ -44,7 +44,7 @@ func (n HostOnly) Enable() error {
 	}
 
 	var finalArgs []string
-	if systemInfo.IsMacOSVbox7() {
+	if systemInfo.IsMacOSVboxV7OrLater() {
 		finalArgs = []string{"hostonlynet", "modify", fmt.Sprintf("--name=%s", n.name), "--enable"}
 	} else {
 		args := []string{"hostonlyif", "ipconfig", n.name}
diff --git a/src/bosh-virtualbox-cpi/vm/network/networks.go b/src/bosh-virtualbox-cpi/vm/network/networks.go
index 5d43bcd0..ac94e084 100644
--- a/src/bosh-virtualbox-cpi/vm/network/networks.go
+++ b/src/bosh-virtualbox-cpi/vm/network/networks.go
@@ -152,7 +152,7 @@ func (n Networks) HostOnlys() ([]Network, error) {
 	}
 
 	commandName := "hostonlyifs"
-	if systemInfo.IsMacOSVbox7() {
+	if systemInfo.IsMacOSVboxV7OrLater() {
 		commandName = "hostonlynets"
 	}
 
@@ -218,7 +218,7 @@ func (n Networks) outputChunks(output string) []string {
 	if err != nil {
 		return nil
 	}
-	if systemInfo.IsMacOSVbox7() {
+	if systemInfo.IsMacOSVboxV7OrLater() {
 		output = strings.Replace(output, "\n\n", "\n", -1)
 	}
 
diff --git a/src/bosh-virtualbox-cpi/vm/network/system_info.go b/src/bosh-virtualbox-cpi/vm/network/system_info.go
index 4ebaf6a3..d71bf695 100644
--- a/src/bosh-virtualbox-cpi/vm/network/system_info.go
+++ b/src/bosh-virtualbox-cpi/vm/network/system_info.go
@@ -37,9 +37,9 @@ func (s SystemInfo) GetLastIP(subnet *net.IPNet) (net.IP, error) {
 	return getIndexedIP(subnet, int(size-1))
 }
 
-// IsMacOSVbox7 Identifies if you're running VirtualBox version 7 on macOS
-func (s SystemInfo) IsMacOSVbox7() bool {
-	if s.osVersion == "darwin" && (s.vBoxMajorVersion == "7") {
+// Identifies if you're running VirtualBox version 7 (or later) on macOS
+func (s SystemInfo) IsMacOSVboxV7OrLater() bool {
+	if s.osVersion == "darwin" && (s.vBoxMajorVersion >= "7") {
 		return true
 	} else {
 		return false
diff --git a/src/bosh-virtualbox-cpi/vm/nics.go b/src/bosh-virtualbox-cpi/vm/nics.go
index 548cca10..b07ac317 100644
--- a/src/bosh-virtualbox-cpi/vm/nics.go
+++ b/src/bosh-virtualbox-cpi/vm/nics.go
@@ -78,7 +78,7 @@ func (n NICs) addNIC(nic string, net Network, host Host) (string, error) {
 			return "", err
 		}
 
-		if systemInfo.IsMacOSVbox7() {
+		if systemInfo.IsMacOSVboxV7OrLater() {
 			args = append(args, []string{"hostonlynet", "--host-only-net" + nic, actualNet.Name()}...)
 		} else {
 			args = append(args, []string{"hostonly", "--hostonlyadapter" + nic, actualNet.Name()}...)

From 2f8f41d26dfffa5f848cbefb423e664a04fe8fb5 Mon Sep 17 00:00:00 2001
From: Benjamin Gandon <bgandon@users.noreply.github.com>
Date: Fri, 16 Feb 2024 03:22:18 +0100
Subject: [PATCH 09/10] Allow hostonlynets networks of any name to be created

With VirtualBox v7 on macOS we can create networks of 'hostonlynets' with a
new CLI experience that allows any name for the created network.

We leverage this new possibility, while still retaining the former feature
with networks of 'hostonlyif' type, when the name of a non-existing and
to-be-created network could possibly be guessed in advance, and if correctly
guessed, then everything was fine.
---
 src/bosh-virtualbox-cpi/vm/host.go            |  15 +-
 .../vm/network/add_host_only.go               | 172 ++++++++----------
 .../vm/network/system_info.go                 |   4 +-
 3 files changed, 83 insertions(+), 108 deletions(-)

diff --git a/src/bosh-virtualbox-cpi/vm/host.go b/src/bosh-virtualbox-cpi/vm/host.go
index dd096079..48cec60d 100644
--- a/src/bosh-virtualbox-cpi/vm/host.go
+++ b/src/bosh-virtualbox-cpi/vm/host.go
@@ -30,6 +30,11 @@ func (h Host) FindNetwork(net Network) (bnet.Network, error) {
 	}
 }
 
+// Enable the network that the VM is to be attached to, and create that
+// network if necessary.
+//
+// In the VM creation workflow, the network is enabled _before_ being looked
+// up with (vm.Host).FindNetworks(Network).
 func (h Host) EnableNetworks(nets Networks) error {
 	for _, net := range nets {
 		switch net.CloudPropertyType() {
@@ -86,6 +91,8 @@ func (n *hostNetwork) Find() (bnet.Network, error) {
 	return nil, fmt.Errorf("Expected to find network '%s'", n.net.CloudPropertyName())
 }
 
+// Enable the network, and try to create it if it does not exist and if is not
+// already been tried.
 func (n *hostNetwork) Enable() error {
 	actualNets, err := n.adapter.List()
 	if err != nil {
@@ -189,13 +196,7 @@ func (n hostOnlysAdapter) List() ([]bnet.Network, error) {
 }
 
 func (n hostOnlysAdapter) Create(net Network) error {
-	canCreate, err := n.AddHostOnly(net.CloudPropertyName(), net.Gateway(), net.Netmask())
-	if err != nil {
-		return err
-	} else if !canCreate {
-		return fmt.Errorf("Expected to find Host-only network '%s'", net.CloudPropertyName())
-	}
-	return nil
+	return n.AddHostOnly(net.CloudPropertyName(), net.Gateway(), net.Netmask())
 }
 
 func (n hostOnlysAdapter) Matches(net Network, actualNet bnet.Network) bool {
diff --git a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
index 6f8833c6..2f523efd 100644
--- a/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
+++ b/src/bosh-virtualbox-cpi/vm/network/add_host_only.go
@@ -10,121 +10,99 @@ import (
 )
 
 var (
-	createdHostOnlyMatch    = regexp.MustCompile(`Interface '(.+)' was successfully created`)
-	createdHostOnlyNetMatch = regexp.MustCompile(`Name:            vboxnet0`)
+	createdHostOnlyMatch = regexp.MustCompile(`Interface '(.+)' was successfully created`)
 )
 
-func (n Networks) AddHostOnly(name, gateway, netmask string) (bool, error) {
-	// VB does not allow naming host-only networks inside version <= 6 , exit if it's not the first one
-	if len(name) > 0 && name != "vboxnet0" {
-		return false, nil
-	}
-
-	createdName, err := n.createHostOnly(gateway, netmask)
-
+func (n Networks) AddHostOnly(expectedName, gateway, netmask string) error {
+	systemInfo, err := n.NewSystemInfo()
 	if err != nil {
-		return true, err
+		return err
 	}
 
-	if len(name) > 0 && createdName != name {
-		n.cleanUpPartialHostOnlyCreate(createdName)
-		return true, fmt.Errorf("expected created host-only network '%s' to have name '%s'", createdName, name)
+	var createdName string
+	if systemInfo.IsMacOSVboxV7OrLater() {
+		if err := n.createHostOnlyNet(expectedName, gateway, netmask); err != nil {
+			return err
+		}
+		createdName = expectedName
+	} else {
+		// Virtualox v6 or earlier choses itself the name of newly created
+		// host-only networks
+		createdName, err := n.createLegacyHostOnly()
+		if err != nil {
+			return err
+		}
+		if len(expectedName) > 0 && createdName != expectedName {
+			n.cleanUpPartialHostOnlyCreate(createdName)
+			return fmt.Errorf("expected created host-only network '%s' to have name '%s'. Have you made an incorrect guess?", createdName, expectedName)
+		}
 	}
 
 	err = n.configureHostOnly(createdName, gateway, netmask)
 	if err != nil {
 		n.cleanUpPartialHostOnlyCreate(createdName)
-		return true, err
+		return err
 	}
 
-	return true, nil
+	return nil
 }
 
-func (n Networks) createHostOnly(gateway, netmask string) (string, error) {
+func (n Networks) createHostOnlyNet(name, gateway, netmask string) error {
 	systemInfo, err := n.NewSystemInfo()
 	if err != nil {
-		return "", err
+		return err
+	}
+	maskIP := net.ParseIP(netmask).To4()
+	if maskIP == nil {
+		return bosherr.Errorf("expected netmask to be valid IP v4 (got '%s')", netmask)
+	}
+	gwIP := net.ParseIP(gateway)
+	if gwIP == nil {
+		return bosherr.Errorf("expected gateway to be valid IP v4 (got '%s')", gateway)
 	}
 
-	var matches []string
-	var errorMessage string
-	var expectedMatchesLen int
-
-	if systemInfo.IsMacOSVboxV7OrLater() {
-		maskIP := net.ParseIP(netmask).To4()
-		if maskIP == nil {
-			return "", bosherr.Errorf("expected netmask to be valid IP v4 (got '%s')", netmask)
-		}
-		gwIP := net.ParseIP(gateway)
-		if gwIP == nil {
-			return "", bosherr.Errorf("expected gateway to be valid IP v4 (got '%s')", gateway)
-		}
-
-		mask := net.IPv4Mask(maskIP[0], maskIP[1], maskIP[2], maskIP[3])
-		subnetFirstIP := &net.IPNet{
-			IP:   gwIP,
-			Mask: mask,
-		}
-		maskLength, _ := mask.Size()
-		_, subnet, _ := net.ParseCIDR(fmt.Sprintf("%s/%v", gateway, maskLength))
-
-		lowerIp, err := systemInfo.GetFirstIP(subnetFirstIP)
-		if err != nil {
-			return "", err
-		}
-		upperIp, err := systemInfo.GetLastIP(subnet)
-		if err != nil {
-			return "", err
-		}
-
-		args := []string{"hostonlynet",
-			"add", fmt.Sprintf("--name=%s", "vboxnet0"),
-			fmt.Sprintf("--netmask=%s", netmask), fmt.Sprintf("--lower-ip=%s", lowerIp.String()),
-			fmt.Sprintf("--upper-ip=%s", upperIp.String()), "--disable"}
+	mask := net.IPv4Mask(maskIP[0], maskIP[1], maskIP[2], maskIP[3])
+	subnetFirstIP := &net.IPNet{
+		IP:   gwIP,
+		Mask: mask,
+	}
+	maskLength, _ := mask.Size()
+	_, subnet, _ := net.ParseCIDR(fmt.Sprintf("%s/%v", gateway, maskLength))
 
-		// The output of the hostonlynet interface creation is empty. We need another solution to handle and verify the
-		// VboxManage creation.
-		_, err = n.driver.ExecuteComplex(args, driver.ExecuteOpts{})
-		if err != nil {
-			return "", err
-		}
+	var lowerIP, upperIP net.IP
+	if lowerIP, err = systemInfo.GetFirstIP(subnetFirstIP); err != nil {
+		return err
+	}
+	if upperIP, err = systemInfo.GetLastIP(subnet); err != nil {
+		return err
+	}
 
-		args = []string{"list", "hostonlynets"}
-		output, err := n.driver.ExecuteComplex(args, driver.ExecuteOpts{})
-		if err != nil {
-			return "", err
-		}
+	args := []string{"hostonlynet",
+		"add", fmt.Sprintf("--name=%s", name),
+		fmt.Sprintf("--netmask=%s", netmask), fmt.Sprintf("--lower-ip=%s", lowerIP.String()),
+		fmt.Sprintf("--upper-ip=%s", upperIP.String()), "--disable"}
 
-		matches = createdHostOnlyNetMatch.FindStringSubmatch(output)
-		//Define the return value of the created Host only Adapter. We're only creating one adapter,
-		//so we can also define the used name hard coded.
-		if len(matches) == 1 {
-			matches[0] = "vboxnet0"
-		}
+	// The output of the hostonlynet interface creation is empty. We need another solution to handle and verify the
+	// VboxManage creation.
+	_, err = n.driver.ExecuteComplex(args, driver.ExecuteOpts{})
+	if err != nil {
+		return err
+	}
+	return nil
+}
 
-		errorMessage = fmt.Sprintf(
-			"Internal inconsistency: Expected len(%s matches) == 1:",
-			createdHostOnlyNetMatch,
-		)
-		expectedMatchesLen = 1
-	} else {
-		output, err := n.driver.Execute("hostonlyif", "create")
-		if err != nil {
-			return "", err
-		}
-		matches = createdHostOnlyMatch.FindStringSubmatch(output)
-		errorMessage = fmt.Sprintf(
-			"Internal inconsistency: Expected len(%s matches) == 2:",
-			createdHostOnlyMatch,
-		)
-		expectedMatchesLen = 2
+func (n Networks) createLegacyHostOnly() (string, error) {
+	output, err := n.driver.Execute("hostonlyif", "create")
+	if err != nil {
+		return "", err
 	}
 
-	if len(matches) != expectedMatchesLen {
-		panic(errorMessage)
+	matches := createdHostOnlyMatch.FindStringSubmatch(output)
+	if len(matches) != 2 {
+		panic(fmt.Sprintf("Internal inconsistency: Expected len(%s matches) == 2:", createdHostOnlyMatch))
 	}
 
-	return matches[expectedMatchesLen-1], nil
+	return matches[1], nil
 }
 
 func (n Networks) configureHostOnly(name, gateway, netmask string) error {
@@ -157,17 +135,11 @@ func (n Networks) cleanUpPartialHostOnlyCreate(name string) {
 			"Failed to get the SystemInfo: %s", err)
 	}
 
-	args := []string{
-		"hostonlyif",
-		"remove",
-		name,
-	}
-	if systemInfo.IsMacOSVbox7() {
-		args = []string{
-			"hostonlynet",
-			"remove",
-			fmt.Sprintf("--name=%s", name),
-		}
+	args := []string{"hostonlyif", "remove"}
+	if systemInfo.IsMacOSVboxV7OrLater() {
+		args = append(args, fmt.Sprintf("--name=%s", name))
+	} else {
+		args = append(args, name)
 	}
 
 	_, err = n.driver.ExecuteComplex(args, driver.ExecuteOpts{})
diff --git a/src/bosh-virtualbox-cpi/vm/network/system_info.go b/src/bosh-virtualbox-cpi/vm/network/system_info.go
index d71bf695..b2fee92d 100644
--- a/src/bosh-virtualbox-cpi/vm/network/system_info.go
+++ b/src/bosh-virtualbox-cpi/vm/network/system_info.go
@@ -6,6 +6,8 @@ import (
 	"net"
 	"runtime"
 	"strings"
+
+	bosherr "github.com/cloudfoundry/bosh-utils/errors"
 )
 
 type SystemInfo struct {
@@ -57,7 +59,7 @@ func (n Networks) getVboxVersion() (string, string, error) {
 	matches := strings.Split(output, ".")
 
 	if len(matches) > 3 {
-		panic(fmt.Sprintf("Internal inconsistency: Expected len(%s matches) >= 3:", createdHostOnlyMatch))
+		return "", "", bosherr.Errorf("Expected VirtualBox version to have 3 dot-separated parts (got '%s')", output)
 	}
 
 	return matches[0], matches[1], nil

From 649147926d746a6e53e3764c08d1617b1b68609b Mon Sep 17 00:00:00 2001
From: Benjamin Gandon <bgandon@users.noreply.github.com>
Date: Fri, 16 Feb 2024 03:27:38 +0100
Subject: [PATCH 10/10] Update docs and align the text with the copy published
 in bosh.io

---
 docs/cloud-props.md | 163 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 142 insertions(+), 21 deletions(-)

diff --git a/docs/cloud-props.md b/docs/cloud-props.md
index bcc0ca01..b5a0b624 100644
--- a/docs/cloud-props.md
+++ b/docs/cloud-props.md
@@ -8,51 +8,172 @@ Example:
 
 ```yaml
 azs:
-- name: z1
+  - name: z1
 ```
 
 ### Network
 
 Schema for `cloud_properties` section used by network subnet:
 
-* **name** [String, optional]: Name of the network. Example: `vboxnet0`.
-* **type** [String, optional]: Type of the network. See [`VBoxManage modifyvm` networking settings](https://www.virtualbox.org/manual/ch08.html#idp46691722135120) for valid values. Example: `hostonly`. Default: `hostonly`.
-
-Example of manual network matching any name:
+* **name** [String, optional]: Name of the VirtualBox network. When not
+  specified (i.e. empty or null value), a new network of the specified type
+  will be created. For convenience, when the specified network does not exist,
+  the CPI will try to create one with that name (see the many caveats about
+  this below). Example: `vboxnet0`.
+* **type** [String, optional]: Type of the VirtualBox network. Supported
+  values: `hostonly`, `bridged`, `natnetwork`.
+  See [`VBoxManage modifyvm` networking settings][modifyvm_net_settings] for
+  more info. Default: `hostonly`.
+
+[modifyvm_net_settings]: https://www.virtualbox.org/manual/ch08.html#vboxmanage-modifyvm-networking
+
+!!! Caveats on `hostonly` networking names
+    When a `hostonly` network name si specified, but no such network exist,
+    the diffetent version of the CPI will behave differently, depending on the
+    VirtualBox version an operating system version.
+    With CPI version 0.4.x or earlier, then only `vboxnet0` is accepted as a
+    name for a network to create, and any other name will produce an error.
+    When the `name` property is not specified, then it will default to
+    `vboxnet0`.
+    With version 0.5.x and later, the name of the created network may be
+    specified with more freedom, with two dinstinct behaviors: VirtualBox v7+
+    on macOS will create a `hostonlynet` honoring the specified name, whereas
+    all Linux versions and macOS version 6.x (or earlier) will use the
+    specified name as a guess for the name that VirtulBox will pick when
+    creating the `hostonlyif` network (indeed those versions of VirtualBox are
+    naming `hostonlyif` networks sequentially using the `vboxnetX` pattern
+    where `X` is a digit, starting at 0). And whenever the guess is wrong, the
+    CPI will produce an error.
+
+Example of manual network:
 
 ```yaml
 networks:
 - name: private
   type: manual
   subnets:
-  - range:   192.168.50.0/24
-    gateway: 192.168.50.1
-    dns:     [192.168.50.1]
+  - range:   192.168.56.0/24
+    gateway: 192.168.56.1
+    dns:     [192.168.56.1]
+    cloud_properties:
+      name: vboxnet0
 ```
 
-### VM
+### VM Types / VM Extensions
 
 Schema for `cloud_properties` section:
 
 * **cpus** [Integer, optional]: Number of CPUs. Example: `1`. Default: `1`.
-* **memory** [Integer, optional]: RAM in megabytes. Example: `1024`. Default: `512`.
-* **ephemeral_disk** [Integer, optional]: Ephemeral disk size in megabytes. Example: `10240`. Default: `5000`.
-* **paravirtprovider** [String, optional]: Paravirtual provider type. See [`VBoxManage modifyvm` general settings](https://www.virtualbox.org/manual/ch08.html#vboxmanage-modifyvm) for valid values. Default: `default`.
-* **audio** [String, optional]: Audio type. See [`VBoxManage modifyvm` general settings](https://www.virtualbox.org/manual/ch08.html#vboxmanage-modifyvm) for valid values. Default: `none`.
+* **memory** [Integer, optional]: RAM in megabytes. Example: `1024`. Default:
+  `512`.
+* **ephemeral_disk** [Integer, optional]: Ephemeral disk size in megabytes.
+  Example: `10240`. Default: `5000`.
+* **paravirtprovider** [String, optional]: Paravirtual provider type. See
+  [`VBoxManage modifyvm` General Settings][modifyvm_general_settings] for
+  valid values. Default: `default`.
+* **audio** [String, optional]: Audio type. See [`VBoxManage modifyvm` Other
+  Hardware Settings][modifyvm_other_hardware] for valid values, e.g. `none`,
+  `default`, `null`, `dsound`, `was`, `oss`, `alsa`, `pulse`, `coreaudio`.
+  Default: `none`.
+
+!!! Caveats on audio
+    Audio is expected to be broken with VirtualBox 7+ on macOS. Contributions
+    are welcome.
+
+[modifyvm_general_settings]: https://www.virtualbox.org/manual/ch08.html#vboxmanage-modifyvm-general
+[modifyvm_other_hardware]: https://www.virtualbox.org/manual/ch08.html#vboxmanage-modifyvm-other-hardware
 
 Example of a VM type:
 
 ```yaml
 vm_types:
-- name: default
-  cloud_properties:
-    cpus: 2
-    memory: 2_048
-    ephemeral_disk: 4_096
-    paravirtprovider: kvm
-    audio: alsa
+  - name: default
+    cloud_properties:
+      cpus: 2
+      memory: 2_048
+      ephemeral_disk: 4_096
+      paravirtprovider: kvm
+      audio: alsa
 ```
 
-### Disk
+### Disk Types
 
 Currently the CPI does not support any cloud properties for disks.
+
+Example of 10GB disk:
+
+```yaml
+disk_types:
+  - name: default
+    disk_size: 10_240
+```
+
+---
+## Global Configuration {: #global }
+
+The CPI uses individual VirtualBox VMs and disks. Since the CPI can only talk to a single VirtualBox server it can only manage resources on a single machine.
+
+Example of a CPI configuration:
+
+```yaml
+properties:
+  host: 192.168.56.1
+  username: ubuntu
+  private_key: |
+    -----BEGIN RSA PRIVATE KEY-----
+    MIIEowIBAAKCAQEAr/c6pUbrq/U+s0dSU+Z6dxrHC7LOGDijv8LYN5cc7alYg+TV
+    ...
+    fe5h79YLG+gJDqVQyKJm0nDRCVz0IkM7Nhz8j07PNJzWjee/kcvv
+    -----END RSA PRIVATE KEY-----
+
+  agent:
+    mbus: "https://mbus:mbus-password@0.0.0.0:6868"
+
+  ntp:
+    - 0.pool.ntp.org
+    - 1.pool.ntp.org
+
+  blobstore:
+    provider: local
+    path: /var/vcap/micro_bosh/data/cache
+```
+
+See the docuemtnation for [the `virtualbox_cpi` job][virtualbox_cpi_job] for
+more details.
+
+[virtualbox_cpi_job]: https://bosh.io/jobs/virtualbox_cpi?source=github.com/cppforlife/bosh-virtualbox-cpi-release
+
+---
+## Example Cloud Config {: #cloud-config }
+
+```yaml
+azs:
+  - name: z1
+  - name: z2
+
+vm_types:
+  - name: default
+
+disk_types:
+  - name: default
+    disk_size: 3000
+
+networks:
+  - name: default
+    type: manual
+    subnets:
+      - range: 192.168.56.0/24
+        gateway: 192.168.56.1
+        azs: [z1, z2]
+        reserved: [192.168.56.6]
+        dns: [192.168.56.1]
+        cloud_properties:
+          name: vboxnet0
+
+compilation:
+  workers: 2
+  reuse_compilation_vms: true
+  az: z1
+  vm_type: default
+  network: default
+```