stale ip fix

qemu/ops.go

@@ -57,10 +57,18 @@ func (c *MachineConfig) ResolveMachineIP() error {
 		if err != nil {
 			return err
 		}
-		lip, _ := c.GetIPAddressByMac(dhcpLeasesContent)
+		candidates, _ := c.GetIPAddressCandidates(dhcpLeasesContent)
 
-		if lip != "" {
-			c.MachineIP = lip
+		found := ""
+		for _, ip := range candidates {
+			if c.reachable(ip) {
+				found = ip
+				break
+			}
+		}
+
+		if found != "" {
+			c.MachineIP = found
 			break
 		}
 		fmt.Print(".")
@@ -75,6 +83,21 @@ func (c *MachineConfig) ResolveMachineIP() error {
 	return os.WriteFile(filepath.Join(c.Location, "config.yaml"), config, 0644)
 }
 
+// reachable reports whether a TCP connection to ip:SSHPort can be
+// established within a short timeout. A matched DHCP lease is only trusted
+// once something is actually listening there - vmnet's dhcpd_leases file can
+// retain a stale-but-unexpired entry from a previous session alongside the
+// current one, and ICMP (ping) is unreliable under vmnet-shared NAT, so a
+// real TCP dial is the most direct confirmation available.
+func (c *MachineConfig) reachable(ip string) bool {
+	conn, err := net.DialTimeout("tcp", net.JoinHostPort(ip, c.SSHPort), 2*time.Second)
+	if err != nil {
+		return false
+	}
+	conn.Close()
+	return true
+}
+
 // Exec starts an interactive shell terminal in VM
 func (c *MachineConfig) Exec(cmd string, root bool) (string, error) {
 	if cmd == "" {
@@ -641,18 +664,25 @@ func (c *MachineConfig) Start() error {
 // 	return ip
 // }
 
-// AssignIP obtains machine IP address from bootpd.plist. Only applicale to machines created on
-// VMNet
-func (c *MachineConfig) GetIPAddressByMac(dhcpLeasesContent []byte) (string, error) {
+// GetIPAddressCandidates returns every still-valid DHCP lease IP for this
+// instance's MAC address, most recently issued first. There can be more
+// than one - e.g. a leftover, not-yet-expired lease from a previous session
+// of the same VM alongside a freshly issued one - so callers should confirm
+// a candidate is actually reachable before trusting it.
+func (c *MachineConfig) GetIPAddressCandidates(dhcpLeasesContent []byte) ([]string, error) {
 	result := utils.ParseDhcpLeasesFile(string(dhcpLeasesContent))
 	dhcpData := utils.ConvertStringArrayToDhcpDataArray(result)
-	dhcpConfig := utils.MatchHwAddress(dhcpData, c.MACAddress)
+	candidates := utils.MatchHwAddressCandidates(dhcpData, c.MACAddress)
 
-	if dhcpConfig != nil {
-		return dhcpConfig.IpAddress, nil
-	} else {
-		return "", errors.New("no machine dhcp configuration found")
+	if len(candidates) == 0 {
+		return nil, errors.New("no machine dhcp configuration found")
+	}
+
+	ips := make([]string, len(candidates))
+	for i, d := range candidates {
+		ips[i] = d.IpAddress
 	}
+	return ips, nil
 }
 
 // Launch macpine downloads a fresh image and creates a VM directory

qemu/ops_test.go

@@ -0,0 +1,49 @@
+package qemu
+
+import (
+	"net"
+	"testing"
+)
+
+func TestReachable(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("failed to start test listener: %v", err)
+	}
+	defer ln.Close()
+
+	go func() {
+		for {
+			conn, err := ln.Accept()
+			if err != nil {
+				return
+			}
+			conn.Close()
+		}
+	}()
+
+	_, openPort, err := net.SplitHostPort(ln.Addr().String())
+	if err != nil {
+		t.Fatalf("failed to parse listener address: %v", err)
+	}
+
+	c := MachineConfig{SSHPort: openPort}
+	if !c.reachable("127.0.0.1") {
+		t.Errorf("expected 127.0.0.1:%s to be reachable (a listener is running there)", openPort)
+	}
+
+	// Grab a port, close the listener, then dial it - nothing is
+	// listening any more, so this should be reported unreachable and
+	// return promptly (connection refused) rather than hang.
+	closedLn, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("failed to allocate a port to close: %v", err)
+	}
+	_, closedPort, _ := net.SplitHostPort(closedLn.Addr().String())
+	closedLn.Close()
+
+	cClosed := MachineConfig{SSHPort: closedPort}
+	if cClosed.reachable("127.0.0.1") {
+		t.Errorf("expected 127.0.0.1:%s to be unreachable (nothing is listening)", closedPort)
+	}
+}

utils/utils.go

@@ -15,6 +15,7 @@ import (
 	"os/exec"
 	"path/filepath"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -536,14 +537,15 @@ func (d DhcpData) ExpiresAt() (time.Time, error) {
 	return time.Unix(sec, 0), nil
 }
 
-// MatchHwAddress returns the most recently issued, still-valid lease for the
-// given hardware address, or nil if none is found. dhcpd_leases can retain
-// entries for a MAC address long after they've expired (e.g. from a previous
-// boot of the same VM with a static MAC), so expired entries are ignored
-// rather than treated as the instance's current address.
-func MatchHwAddress(data []DhcpData, targetHwAddress string) *DhcpData {
-	var best *DhcpData
-	var bestExpiry time.Time
+// MatchHwAddressCandidates returns every still-valid (unexpired) lease for
+// the given hardware address, most recently issued first. dhcpd_leases can
+// retain more than one entry for a MAC address - e.g. a leftover,
+// not-yet-expired lease from a previous session of the same VM alongside a
+// freshly issued one for the current session - so callers that need to
+// confirm an address is actually live should try candidates in order rather
+// than trusting only the single latest-expiry one.
+func MatchHwAddressCandidates(data []DhcpData, targetHwAddress string) []DhcpData {
+	var candidates []DhcpData
 
 	for i := range data {
 		if data[i].HwAddress != targetHwAddress {
@@ -553,10 +555,14 @@ func MatchHwAddress(data []DhcpData, targetHwAddress string) *DhcpData {
 		if err != nil || time.Now().After(expiry) {
 			continue
 		}
-		if best == nil || expiry.After(bestExpiry) {
-			best = &data[i]
-			bestExpiry = expiry
-		}
+		candidates = append(candidates, data[i])
 	}
-	return best
+
+	sort.Slice(candidates, func(i, j int) bool {
+		ei, _ := candidates[i].ExpiresAt()
+		ej, _ := candidates[j].ExpiresAt()
+		return ei.After(ej)
+	})
+
+	return candidates
 }

utils/utils_test.go

@@ -0,0 +1,62 @@
+package utils
+
+import (
+	"fmt"
+	"testing"
+	"time"
+)
+
+func leaseHex(expiresIn time.Duration) string {
+	return fmt.Sprintf("0x%x", time.Now().Add(expiresIn).Unix())
+}
+
+func TestMatchHwAddressCandidates(t *testing.T) {
+	targetMAC := "56:84:17:14:59:9d"
+
+	data := []DhcpData{
+		// Expired lease for the target MAC - must be excluded entirely.
+		{IpAddress: "192.168.2.4", HwAddress: targetMAC, Lease: leaseHex(-time.Hour)},
+		// A leftover-but-still-unexpired lease from an earlier session,
+		// with a *later* expiry than the fresh one below (the scenario
+		// that used to make the stale IP win outright).
+		{IpAddress: "192.168.2.5", HwAddress: targetMAC, Lease: leaseHex(55 * time.Minute)},
+		// The genuinely fresh lease for the current session, expiring
+		// sooner only because it was issued more recently.
+		{IpAddress: "192.168.105.2", HwAddress: targetMAC, Lease: leaseHex(50 * time.Minute)},
+		// A different MAC entirely - must never be returned.
+		{IpAddress: "192.168.2.6", HwAddress: "56:a3:de:96:79:7f", Lease: leaseHex(time.Hour)},
+	}
+
+	candidates := MatchHwAddressCandidates(data, targetMAC)
+
+	if len(candidates) != 2 {
+		t.Fatalf("expected 2 unexpired candidates for %s, got %d: %+v", targetMAC, len(candidates), candidates)
+	}
+
+	if candidates[0].IpAddress != "192.168.2.5" {
+		t.Errorf("expected the later-expiry lease (192.168.2.5) first, got %s", candidates[0].IpAddress)
+	}
+	if candidates[1].IpAddress != "192.168.105.2" {
+		t.Errorf("expected the fresher-but-shorter-remaining lease (192.168.105.2) second, got %s", candidates[1].IpAddress)
+	}
+
+	for _, c := range candidates {
+		if c.IpAddress == "192.168.2.4" {
+			t.Errorf("expired lease 192.168.2.4 must not be returned")
+		}
+		if c.HwAddress != targetMAC {
+			t.Errorf("candidate for wrong hardware address returned: %+v", c)
+		}
+	}
+}
+
+func TestMatchHwAddressCandidatesNoMatch(t *testing.T) {
+	data := []DhcpData{
+		{IpAddress: "192.168.2.6", HwAddress: "56:a3:de:96:79:7f", Lease: leaseHex(time.Hour)},
+	}
+
+	candidates := MatchHwAddressCandidates(data, "56:84:17:14:59:9d")
+	if len(candidates) != 0 {
+		t.Fatalf("expected no candidates, got %+v", candidates)
+	}
+}