More robust MAC address matching (kubernetes#19750)

On darwin bootp uses non-standard MAC address format[1]:
"8e:1:99:9c:54:b1" instead of "8e:01:99:9c:54:b1". We fixed this by
trimming leading "0" in the string before looking up the IP address.

There are several issues with the current code:
- Fragile, will break if bootp changes the format (unlikely)
- Fixing the wrong place, the drivers should not care about the MAC
  address format.
- The tests were confusing, showing that we can match standard MAC
  addresses while the actual code could match only non-standard bootp
  addresses.
- Logging wrong MAC address since we trimmed leading zeros before
  logging

This change replace trimming leading zeros with parsing MAC address
strings and comparing the bytes. The test includes now both standard and
non-standard MAC addresses.

[1] https://openradar.appspot.com/FB15382970

pkg/drivers/common.go

@@ -18,11 +18,12 @@ package drivers
 
 import (
 	"bufio"
+	"bytes"
 	"fmt"
 	"io"
+	"net"
 	"os"
 	"path/filepath"
-	"regexp"
 	"strings"
 	"syscall"
 
@@ -40,8 +41,6 @@ import (
 // LeasesPath is the path to dhcpd leases
 const LeasesPath = "/var/db/dhcpd_leases"
 
-var leadingZeroRegexp = regexp.MustCompile(`0([A-Fa-f0-9](:|$))`)
-
 // This file is for common code shared among internal machine drivers
 // Code here should not be called from within minikube
 
@@ -190,7 +189,7 @@ func fixMachinePermissions(path string) error {
 type DHCPEntry struct {
 	Name      string
 	IPAddress string
-	HWAddress string
+	HWAddress net.HardwareAddr
 	ID        string
 	Lease     string
 }
@@ -201,6 +200,13 @@ func GetIPAddressByMACAddress(mac string) (string, error) {
 }
 
 func getIPAddressFromFile(mac, path string) (string, error) {
+	// Due to https://openradar.appspot.com/FB15382970 we need to parse the MAC
+	// address and compare the bytes.
+	macAddress, err := parseMAC(mac)
+	if err != nil {
+		return "", err
+	}
+
 	log.Debugf("Searching for %s in %s ...", mac, path)
 	file, err := os.Open(path)
 	if err != nil {
@@ -215,7 +221,10 @@ func getIPAddressFromFile(mac, path string) (string, error) {
 	log.Debugf("Found %d entries in %s!", len(dhcpEntries), path)
 	for _, dhcpEntry := range dhcpEntries {
 		log.Debugf("dhcp entry: %+v", dhcpEntry)
-		if dhcpEntry.HWAddress == mac {
+		if dhcpEntry.HWAddress == nil {
+			continue
+		}
+		if bytes.Equal(dhcpEntry.HWAddress, macAddress) {
 			log.Debugf("Found match: %s", mac)
 			return dhcpEntry.IPAddress, nil
 		}
@@ -252,7 +261,13 @@ func parseDHCPdLeasesFile(file io.Reader) ([]DHCPEntry, error) {
 			dhcpEntry.IPAddress = val
 		case "hw_address":
 			// The mac addresses have a '1,' at the start.
-			dhcpEntry.HWAddress = val[2:]
+			macAddress, err := parseMAC(val[2:])
+			if err != nil {
+				log.Warnf("unable to parse hw_address in dhcp leases file: %q: %s",
+					val[2:], err)
+				continue
+			}
+			dhcpEntry.HWAddress = macAddress
 		case "identifier":
 			dhcpEntry.ID = val
 		case "lease":
@@ -264,7 +279,17 @@ func parseDHCPdLeasesFile(file io.Reader) ([]DHCPEntry, error) {
 	return dhcpEntries, scanner.Err()
 }
 
-// TrimMacAddress trimming "0" of the ten's digit
-func TrimMacAddress(rawUUID string) string {
-	return leadingZeroRegexp.ReplaceAllString(rawUUID, "$1")
+// parseMAC parse both standard fixeed size MAC address "%02x:..." and the
+// variable size MAC address on drawin "%x:...".
+func parseMAC(mac string) (net.HardwareAddr, error) {
+	hw := make(net.HardwareAddr, 6)
+	n, err := fmt.Sscanf(mac, "%x:%x:%x:%x:%x:%x",
+		&hw[0], &hw[1], &hw[2], &hw[3], &hw[4], &hw[5])
+	if n != len(hw) {
+		return nil, fmt.Errorf("invalid MAC address: %q", mac)
+	}
+	if err != nil {
+		return nil, fmt.Errorf("unable to parse MAC address: %q: %s", mac, err)
+	}
+	return hw, nil
 }

pkg/drivers/common_test.go

@@ -54,6 +54,13 @@ var validLeases = []byte(`{
         identifier=1,a2:b3:c4:d5:e6:f7
         lease=0x597e1267
 }
+{
+        name=bootp
+        ip_address=192.168.105.2
+        hw_address=1,8e:1:99:9c:54:b1
+        identifier=1,8e:1:99:9c:54:b1
+        lease=0x597e1268
+}
 {
         name=bar
         ip_address=192.168.64.3
@@ -98,6 +105,14 @@ func Test_getIpAddressFromFile(t *testing.T) {
 			"1.2.3.4",
 			false,
 		},
+		{
+			// bootp use "%x" format instead of "%02x"
+			// https://openradar.appspot.com/FB15382970
+			"valid-bootp",
+			args{"8e:01:99:9c:54:b1", dhcpFile},
+			"192.168.105.2",
+			false,
+		},
 		{
 			"duplicate",
 			args{"a4:b5:c6:d7:e8:f9", dhcpFile},

pkg/drivers/hyperkit/driver.go

@@ -261,8 +261,6 @@ func (d *Driver) Start() error {
 		return errors.Wrap(err, "getting MAC address from UUID")
 	}
 
-	// Need to strip 0's
-	mac = pkgdrivers.TrimMacAddress(mac)
 	log.Debugf("Generated MAC %s", mac)
 
 	log.Debugf("Starting with cmdline: %s", d.Cmdline)

pkg/drivers/qemu/qemu.go

@@ -505,10 +505,7 @@ func (d *Driver) Start() error {
 	case "socket_vmnet":
 		var err error
 		getIP := func() error {
-			// QEMU requires MAC address with leading 0s
-			// But socket_vmnet writes the MAC address to the dhcp leases file with leading 0s stripped
-			mac := pkgdrivers.TrimMacAddress(d.MACAddress)
-			d.IPAddress, err = pkgdrivers.GetIPAddressByMACAddress(mac)
+			d.IPAddress, err = pkgdrivers.GetIPAddressByMACAddress(d.MACAddress)
 			if err != nil {
 				return errors.Wrap(err, "failed to get IP address")
 			}

pkg/drivers/vfkit/vfkit.go

@@ -265,8 +265,6 @@ func (d *Driver) Start() error {
 		return err
 	}
 
-	// Need to strip 0's
-	mac = pkgdrivers.TrimMacAddress(mac)
 	if err := d.setupIP(mac); err != nil {
 		return err
 	}