diff --git a/qemu/ops.go b/qemu/ops.go index 465f751..ecc227a 100644 --- a/qemu/ops.go +++ b/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 diff --git a/qemu/ops_test.go b/qemu/ops_test.go new file mode 100644 index 0000000..64e2f67 --- /dev/null +++ b/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) + } +} diff --git a/utils/utils.go b/utils/utils.go index a6722bc..49b44f5 100644 --- a/utils/utils.go +++ b/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 } diff --git a/utils/utils_test.go b/utils/utils_test.go new file mode 100644 index 0000000..6e37447 --- /dev/null +++ b/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) + } +}