More robust MAC address matching (#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
  }