From 33fde5ece9c61336ce02db3213c7430eabb9d2aa Mon Sep 17 00:00:00 2001 From: Claude Opus 5 Date: Thu, 30 Jul 2026 15:32:59 +0200 Subject: [PATCH] fix: give HTTP drivers a MAC-derived device identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An HTTP driver never had its MAC resolved, and the HTTP branch sets no endpoint either, so a LAN device that had not yet reported a serial had no stable device_id at all — its persistent state (battery model, RLS twin, calibration history) was orphaned whenever its address changed. Registry.Add now ARP-resolves the driver's own configured address, as it already did for MQTT and Modbus. Only the driver's own host/url is probed, never the merged HTTP allowlist, so vendor cloud endpoints are never treated as identity candidates. arp.Lookup now skips addresses that provably cannot appear in an ARP table (loopback, CGNAT/tunnel, multicast, broadcast), saving 150 ms of futile dialing. Public addresses stay eligible on purpose: an ISP can route a block straight onto the LAN, and excluding them would silently re-key those devices off their MAC. Adds the missing coverage for this path: ResolveDeviceID priority and RegisterDevice upsert semantics had no tests at all, and the arp package had none despite carrying a readFile seam built for them. The darwin parser is extracted so it is testable without shelling out, and now tolerates tab-separated output and rejects all-zero MACs. Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com> --- .changeset/http-driver-mac-identity.md | 17 ++ go/internal/arp/arp.go | 57 +++++-- go/internal/arp/arp_test.go | 180 +++++++++++++++++++++ go/internal/drivers/registry.go | 39 ++++- go/internal/drivers/registry_arp_test.go | 146 +++++++++++++++++ go/internal/state/devices_test.go | 192 +++++++++++++++++++++++ 6 files changed, 620 insertions(+), 11 deletions(-) create mode 100644 .changeset/http-driver-mac-identity.md create mode 100644 go/internal/arp/arp_test.go create mode 100644 go/internal/drivers/registry_arp_test.go create mode 100644 go/internal/state/devices_test.go diff --git a/.changeset/http-driver-mac-identity.md b/.changeset/http-driver-mac-identity.md new file mode 100644 index 00000000..bff256c6 --- /dev/null +++ b/.changeset/http-driver-mac-identity.md @@ -0,0 +1,17 @@ +--- +"ftw": patch +--- + +Give HTTP drivers a hardware-stable device identity. An HTTP driver's MAC was +never resolved, so a LAN device that had not yet reported a serial fell back to +no identity at all and its persistent state (battery model, RLS twin, +calibration history) was orphaned whenever its address changed. The registry now +ARP-resolves the driver's own configured address, exactly as it already does for +MQTT and Modbus. + +Only the driver's own `host`/`url` is probed, never the merged HTTP allowlist, +so vendor cloud endpoints are never treated as identity candidates. ARP lookups +are also now skipped for addresses that provably cannot appear in an ARP table +(loopback, carrier-grade-NAT/tunnel, multicast and broadcast), saving 150 ms of +pointless dialing. Public addresses stay eligible, so devices on a segment with +an ISP-routed block keep the MAC-derived identity they have today. diff --git a/go/internal/arp/arp.go b/go/internal/arp/arp.go index c651ecb9..b0fb1398 100644 --- a/go/internal/arp/arp.go +++ b/go/internal/arp/arp.go @@ -23,6 +23,9 @@ import ( // the host before reading. The probe is cheap (50 ms timeout) and silent // on failure — what matters is the SYN packet itself triggering ARP. // +// Only a bare IPv4 literal is accepted; hostnames are not resolved, so a +// driver configured with a DNS name gets no MAC. +// // Returns (mac, true) on success or ("", false) if the host can't be // reached on L2 or isn't on this segment at all. func Lookup(ipStr string) (string, bool) { @@ -30,6 +33,12 @@ func Lookup(ipStr string) (string, bool) { if ip == nil { return "", false } ip4 := ip.To4() if ip4 == nil { return "", false } + // Skip addresses that provably cannot produce an ARP entry, rather + // than spending 150 ms of dialing to learn nothing. Deliberately NOT + // limited to RFC1918: a public address can legitimately sit on the + // local segment (an ISP routing a small block onto the LAN), and + // excluding it would silently re-key those devices off their MAC. + if !canHaveARPEntry(ip4) { return "", false } // Nudge the ARP cache. We don't care if connect succeeds — the kernel // resolves ARP regardless when it tries to send the SYN. for _, port := range []string{"80", "502", "1883"} { @@ -45,6 +54,26 @@ func Lookup(ipStr string) (string, bool) { return "", false } +// canHaveARPEntry reports whether an IPv4 address could ever appear in the +// kernel's ARP table. Only the provably-impossible cases are excluded, so +// that a device on an unusual-but-real local segment keeps its MAC-derived +// identity. ip4 must be the 4-byte form from net.IP.To4. +func canHaveARPEntry(ip4 net.IP) bool { + switch { + case ip4[0] == 0: // "this network", including 0.0.0.0 + return false + case ip4[0] == 127: // loopback never resolves to a hardware address + return false + case ip4[0] == 100 && ip4[1] >= 64 && ip4[1] <= 127: + // Carrier-grade NAT space, where Tailscale and similar tunnels + // live. Reached over an L3 tun device, which has no L2 to ARP on. + return false + case ip4[0] >= 224: // multicast, reserved space, broadcast + return false + } + return true +} + // lookupLinux parses /proc/net/arp. // // IP address HW type Flags HW address Mask Device @@ -69,22 +98,30 @@ func lookupLinux(ipStr string) (string, bool) { func lookupDarwin(ipStr string) (string, bool) { out, err := exec.Command("/usr/sbin/arp", "-n", ipStr).CombinedOutput() if err != nil { return "", false } - // Output: "? (192.168.1.42) at aa:bb:cc:dd:ee:ff on en0 ifscope [ethernet]" - s := string(out) + return parseDarwinARP(string(out)) +} + +// parseDarwinARP extracts the MAC from `arp -n ` output: +// +// ? (192.168.1.42) at aa:bb:cc:dd:ee:ff on en0 ifscope [ethernet] +// +// A miss prints "(incomplete)" or "no entry" in the same slot; both fail +// the six-octet check. macOS drops leading zeros ("1:2:3:4:5:6"), so +// octets are padded back to the canonical form callers compare against. +func parseDarwinARP(s string) (string, bool) { idx := strings.Index(s, " at ") if idx < 0 { return "", false } - rest := s[idx+4:] - end := strings.Index(rest, " ") - if end < 0 { return "", false } - mac := strings.ToLower(rest[:end]) - if mac == "(incomplete)" || mac == "no" { return "", false } - // Normalize single-digit octets (macOS prints "1:2:3:4:5:6") - parts := strings.Split(mac, ":") + fields := strings.Fields(s[idx+len(" at "):]) + if len(fields) == 0 { return "", false } + parts := strings.Split(strings.ToLower(fields[0]), ":") if len(parts) != 6 { return "", false } for i, p := range parts { + if len(p) == 0 || len(p) > 2 { return "", false } if len(p) == 1 { parts[i] = "0" + p } } - return strings.Join(parts, ":"), true + mac := strings.Join(parts, ":") + if mac == "00:00:00:00:00:00" { return "", false } + return mac, true } // readFile is a tiny indirection so tests can stub /proc/net/arp. diff --git a/go/internal/arp/arp_test.go b/go/internal/arp/arp_test.go new file mode 100644 index 00000000..91c50dc4 --- /dev/null +++ b/go/internal/arp/arp_test.go @@ -0,0 +1,180 @@ +package arp + +import ( + "net" + "os" + "testing" +) + +// stubARPTable replaces the /proc/net/arp reader for one test. +func stubARPTable(t *testing.T, contents string) { + t.Helper() + prev := readFile + readFile = func(string) ([]byte, error) { return []byte(contents), nil } + t.Cleanup(func() { readFile = prev }) +} + +// Every case here must bail before the ARP-cache nudge, which is what +// keeps this test hermetic: no case may touch the network. A hostname is +// never resolved, IPv6 has no ARP at all, and the rest are addresses that +// provably cannot appear in an ARP table. +func TestLookupRejectsAddressesThatCannotHaveAnARPEntry(t *testing.T) { + cases := []struct{ name, host string }{ + {"empty", ""}, + {"hostname", "inverter.local"}, + {"host:port", "192.168.1.5:502"}, + {"ipv6 loopback", "::1"}, + {"ipv6 link-local", "fe80::1"}, + {"ipv6 ula", "fd00::5"}, + {"ipv4 loopback", "127.0.0.1"}, + {"unspecified", "0.0.0.0"}, + {"cgnat tunnel", "100.96.146.117"}, + {"multicast", "224.0.0.251"}, + {"broadcast", "255.255.255.255"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if mac, ok := Lookup(tc.host); ok { + t.Fatalf("Lookup(%q) = (%q, true), want no match", tc.host, mac) + } + }) + } +} + +// The gate excludes only what provably cannot resolve. A public address +// stays eligible on purpose: an ISP can route a small block straight onto +// the LAN, and excluding it would silently re-key those devices off their +// MAC and orphan their persistent state. +func TestCanHaveARPEntry(t *testing.T) { + cases := []struct { + ip string + want bool + }{ + {"10.0.0.1", true}, + {"172.16.0.1", true}, + {"192.168.1.42", true}, + {"169.254.10.1", true}, // link-local: no DHCP, still on our segment + {"8.8.8.8", true}, // public, but could be on our segment + {"172.32.0.1", true}, // outside RFC1918, still a routable unicast host + {"0.0.0.0", false}, + {"127.0.0.1", false}, + {"100.64.0.1", false}, + {"100.127.255.254", false}, + {"100.128.0.1", true}, // just past CGNAT, ordinary unicast again + {"224.0.0.251", false}, + {"239.255.255.250", false}, + {"255.255.255.255", false}, + } + for _, tc := range cases { + t.Run(tc.ip, func(t *testing.T) { + ip4 := net.ParseIP(tc.ip).To4() + if ip4 == nil { + t.Fatalf("ParseIP(%q).To4() = nil", tc.ip) + } + if got := canHaveARPEntry(ip4); got != tc.want { + t.Errorf("canHaveARPEntry(%s) = %v, want %v", tc.ip, got, tc.want) + } + }) + } +} + +const procNetARP = `IP address HW type Flags HW address Mask Device +192.168.1.42 0x1 0x2 AA:BB:CC:DD:EE:FF * eth0 +192.168.1.43 0x1 0x0 00:00:00:00:00:00 * eth0 +192.168.1.44 0x1 0x2 aa:bb:cc:dd:ee:01 * wlan0 +` + +func TestLookupLinuxParsesProcNetARP(t *testing.T) { + stubARPTable(t, procNetARP) + cases := []struct { + name string + ip string + wantMAC string + wantOK bool + }{ + // The kernel prints uppercase; device_id comparisons downstream + // assume lowercase, so normalization here is load-bearing. + {"match is lowercased", "192.168.1.42", "aa:bb:cc:dd:ee:ff", true}, + {"match on second interface", "192.168.1.44", "aa:bb:cc:dd:ee:01", true}, + {"incomplete entry is not a match", "192.168.1.43", "", false}, + {"absent host", "192.168.1.99", "", false}, + {"header row is not matched", "IP", "", false}, + {"prefix of a listed ip", "192.168.1.4", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mac, ok := lookupLinux(tc.ip) + if mac != tc.wantMAC || ok != tc.wantOK { + t.Errorf("lookupLinux(%q) = (%q, %v), want (%q, %v)", + tc.ip, mac, ok, tc.wantMAC, tc.wantOK) + } + }) + } +} + +func TestLookupLinuxHandlesUnreadableTable(t *testing.T) { + prev := readFile + readFile = func(string) ([]byte, error) { return nil, os.ErrNotExist } + t.Cleanup(func() { readFile = prev }) + + if mac, ok := lookupLinux("192.168.1.42"); ok { + t.Fatalf("lookupLinux with unreadable table = (%q, true), want no match", mac) + } +} + +func TestParseDarwinARP(t *testing.T) { + cases := []struct { + name string + out string + wantMAC string + wantOK bool + }{ + { + "typical entry", + "? (192.168.1.42) at aa:bb:cc:dd:ee:ff on en0 ifscope [ethernet]\n", + "aa:bb:cc:dd:ee:ff", true, + }, + { + // macOS drops leading zeros; the padded form is what the + // devices table and every comparison downstream expect. + "single-digit octets are padded", + "? (192.168.1.42) at 1:2:3:4:5:6 on en0 ifscope [ethernet]\n", + "01:02:03:04:05:06", true, + }, + { + "uppercase is normalized", + "? (192.168.1.42) at AA:BB:CC:DD:EE:FF on en0\n", + "aa:bb:cc:dd:ee:ff", true, + }, + { + "mac at end of output without trailing field", + "? (192.168.1.42) at aa:bb:cc:dd:ee:ff", + "aa:bb:cc:dd:ee:ff", true, + }, + { + "incomplete entry", + "? (192.168.1.42) at (incomplete) on en0 ifscope [ethernet]\n", + "", false, + }, + { + "no entry at all", + "192.168.1.99 (192.168.1.99) -- no entry\n", + "", false, + }, + {"all-zero mac", "? (192.168.1.42) at 00:00:00:00:00:00 on en0\n", "", false}, + {"too few octets", "? (192.168.1.42) at aa:bb:cc on en0\n", "", false}, + {"too many octets", "? (192.168.1.42) at aa:bb:cc:dd:ee:ff:11 on en0\n", "", false}, + {"oversized octet", "? (192.168.1.42) at aaa:bb:cc:dd:ee:ff on en0\n", "", false}, + {"empty octet", "? (192.168.1.42) at aa::cc:dd:ee:ff on en0\n", "", false}, + {"empty output", "", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mac, ok := parseDarwinARP(tc.out) + if mac != tc.wantMAC || ok != tc.wantOK { + t.Errorf("parseDarwinARP(%q) = (%q, %v), want (%q, %v)", + tc.out, mac, ok, tc.wantMAC, tc.wantOK) + } + }) + } +} diff --git a/go/internal/drivers/registry.go b/go/internal/drivers/registry.go index 10df641c..71c45c2f 100644 --- a/go/internal/drivers/registry.go +++ b/go/internal/drivers/registry.go @@ -26,7 +26,8 @@ type Registry struct { MQTTFactory func(name string, c *config.MQTTConfig) (MQTTCap, error) // ModbusFactory creates a Modbus capability. ModbusFactory func(name string, c *config.ModbusConfig) (ModbusCap, error) - // ARPLookup resolves a hostname/IP to a MAC for L2-stable identity. + // ARPLookup resolves an on-LAN IPv4 literal to a MAC for L2-stable + // identity; hostnames and off-segment addresses yield ("", false). // Optional — when nil, devices fall back to endpoint-hash IDs. ARPLookup func(host string) (mac string, ok bool) // SecretPersister, when set, durably stores a driver secret (keyed by @@ -190,6 +191,19 @@ func (r *Registry) Add(ctx context.Context, cfg config.Driver) error { if pin := strings.TrimSpace(cfg.Capabilities.HTTP.TLSPinSHA256); pin != "" { env.WithHTTPTLSPin(pin) } + // A LAN HTTP device often has no serial until it answers, and the + // HTTP branch sets no endpoint — so without this it has no stable + // identity at all and its persistent state is orphaned on a DHCP + // lease change. Probe only the driver's OWN address, never the + // merged allowlist: that list also names vendor cloud endpoints, + // which must not be identity candidates. + if r.ARPLookup != nil { + if host := httpDeviceHost(cfg.Config); host != "" { + if mac, ok := r.ARPLookup(host); ok { + env.SetMAC(mac) + } + } + } } if cfg.Capabilities.WebSocket != nil { env.WithWS(NewGorillaWS(cfg.Name)) @@ -638,6 +652,29 @@ func mergeAllowedHosts(explicit []string, drvCfg map[string]any) []string { return out } +// httpDeviceHost returns the address of the device an HTTP driver talks +// to, taken from its own config block: the `host` key, else the host part +// of `url`. Any port is stripped — ARP keys on the L3 address alone. +// Returns "" for a driver with neither (a cloud driver whose base URL +// lives in the Lua source, not in config). +func httpDeviceHost(drvCfg map[string]any) string { + if drvCfg == nil { + return "" + } + if v, ok := drvCfg["host"].(string); ok { + if h := strings.TrimSpace(v); h != "" { + host, _, _ := splitHostPortLower(h) + return host + } + } + if v, ok := drvCfg["url"].(string); ok { + if u, err := url.Parse(strings.TrimSpace(v)); err == nil && u.Hostname() != "" { + return strings.ToLower(u.Hostname()) + } + } + return "" +} + func findDriver(list []config.Driver, name string) (config.Driver, bool) { for _, d := range list { if d.Name == name { diff --git a/go/internal/drivers/registry_arp_test.go b/go/internal/drivers/registry_arp_test.go new file mode 100644 index 00000000..a463973d --- /dev/null +++ b/go/internal/drivers/registry_arp_test.go @@ -0,0 +1,146 @@ +package drivers + +import ( + "context" + "sync" + "testing" + + "github.com/srcfl/ftw/go/internal/config" + "github.com/srcfl/ftw/go/internal/telemetry" +) + +const arpTestDriver = ` +function driver_init(config) end +function driver_poll() return 1000 end +function driver_command(action, w, cmd) end +` + +// recordingARP stands in for arp.Lookup and remembers every host it was +// asked about, so a test can assert on what was NOT probed. +type recordingARP struct { + mu sync.Mutex + hosts []string + macs map[string]string +} + +func (r *recordingARP) lookup(host string) (string, bool) { + r.mu.Lock() + defer r.mu.Unlock() + r.hosts = append(r.hosts, host) + mac, ok := r.macs[host] + return mac, ok +} + +func (r *recordingARP) asked() []string { + r.mu.Lock() + defer r.mu.Unlock() + return append([]string(nil), r.hosts...) +} + +func addARPTestDriver(t *testing.T, arp *recordingARP, cfg config.Driver) *Registry { + t.Helper() + r := NewRegistry(telemetry.NewStore()) + r.ARPLookup = arp.lookup + cfg.Lua = writeTestDriver(t, arpTestDriver) + if err := r.Add(context.Background(), cfg); err != nil { + t.Fatalf("Add: %v", err) + } + t.Cleanup(func() { r.remove(cfg.Name, true) }) + return r +} + +// An HTTP driver sets no endpoint, so without an ARP-derived MAC a LAN +// device that has not yet reported a serial has no stable identity at all. +func TestAddResolvesMACForHTTPDriver(t *testing.T) { + arp := &recordingARP{macs: map[string]string{"192.168.1.50": "aa:bb:cc:dd:ee:ff"}} + r := addARPTestDriver(t, arp, config.Driver{ + Name: "heatpump", + Capabilities: config.Capabilities{HTTP: &config.HTTPCapability{}}, + Config: map[string]any{"host": "192.168.1.50"}, + }) + + env := r.Env("heatpump") + if env == nil { + t.Fatal("Env = nil after Add") + } + if _, _, mac, _ := env.FullIdentity(); mac != "aa:bb:cc:dd:ee:ff" { + t.Errorf("mac = %q, want aa:bb:cc:dd:ee:ff", mac) + } +} + +// The merged allowlist also names vendor cloud endpoints. Probing those +// would be both useless (never on our L2) and an unsolicited connection +// to a third party, so only the driver's own address may be asked about. +func TestHTTPDriverARPProbesOnlyItsOwnHost(t *testing.T) { + arp := &recordingARP{macs: map[string]string{"192.168.1.50": "aa:bb:cc:dd:ee:ff"}} + addARPTestDriver(t, arp, config.Driver{ + Name: "heatpump", + Capabilities: config.Capabilities{HTTP: &config.HTTPCapability{ + AllowedHosts: []string{"api.vendor.example", "telemetry.vendor.example"}, + }}, + Config: map[string]any{"host": "192.168.1.50"}, + }) + + asked := arp.asked() + if len(asked) != 1 || asked[0] != "192.168.1.50" { + t.Errorf("ARP probed %v, want exactly [192.168.1.50]", asked) + } +} + +func TestCloudOnlyHTTPDriverIsNotProbed(t *testing.T) { + arp := &recordingARP{} + addARPTestDriver(t, arp, config.Driver{ + Name: "easee", + Capabilities: config.Capabilities{HTTP: &config.HTTPCapability{ + AllowedHosts: []string{"api.easee.cloud"}, + }}, + }) + + if asked := arp.asked(); len(asked) != 0 { + t.Errorf("ARP probed %v for a driver with no device address, want none", asked) + } +} + +func TestHTTPDriverSurvivesUnresolvableMAC(t *testing.T) { + arp := &recordingARP{} // every lookup misses + r := addARPTestDriver(t, arp, config.Driver{ + Name: "heatpump", + Capabilities: config.Capabilities{HTTP: &config.HTTPCapability{}}, + Config: map[string]any{"host": "192.168.1.50"}, + }) + + if _, _, mac, _ := r.Env("heatpump").FullIdentity(); mac != "" { + t.Errorf("mac = %q, want empty when ARP misses", mac) + } +} + +func TestHTTPDeviceHost(t *testing.T) { + cases := []struct { + name string + cfg map[string]any + want string + }{ + {"nil config", nil, ""}, + {"empty config", map[string]any{}, ""}, + {"bare host", map[string]any{"host": "192.168.1.50"}, "192.168.1.50"}, + {"host with port", map[string]any{"host": "192.168.1.50:8080"}, "192.168.1.50"}, + {"host is trimmed and lowercased", map[string]any{"host": " Inverter.Local "}, "inverter.local"}, + {"blank host", map[string]any{"host": " "}, ""}, + {"host wins over url", map[string]any{ + "host": "192.168.1.50", "url": "https://api.vendor.example", + }, "192.168.1.50"}, + {"url fallback", map[string]any{"url": "http://192.168.1.60/api"}, "192.168.1.60"}, + {"url port is stripped", map[string]any{"url": "http://192.168.1.60:8080/api"}, "192.168.1.60"}, + {"url with credentials", map[string]any{"url": "http://user:pw@192.168.1.60/api"}, "192.168.1.60"}, + {"bracketed ipv6 url is unwrapped", map[string]any{"url": "http://[fd00::5]:8080/"}, "fd00::5"}, + {"url without host", map[string]any{"url": "/local/path"}, ""}, + {"non-string host is ignored", map[string]any{"host": 42}, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := httpDeviceHost(tc.cfg); got != tc.want { + t.Errorf("httpDeviceHost(%v) = %q, want %q", tc.cfg, got, tc.want) + } + }) + } +} diff --git a/go/internal/state/devices_test.go b/go/internal/state/devices_test.go new file mode 100644 index 00000000..b4ec4f7e --- /dev/null +++ b/go/internal/state/devices_test.go @@ -0,0 +1,192 @@ +package state + +import "testing" + +// device_id is the key every piece of persistent per-device state hangs +// off (battery models, the RLS twin, calibration history), so the +// priority order below is a compatibility surface: changing it silently +// orphans that state on the next boot. +func TestResolveDeviceIDPriority(t *testing.T) { + cases := []struct { + name string + make, serial, mac, endpoint string + want string + }{ + { + name: "hardware serial outranks mac and endpoint", + make: "Sungrow", serial: "A2340", mac: "aa:bb:cc:dd:ee:ff", + endpoint: "modbus://192.168.1.5:502", + want: "sungrow:A2340", + }, + { + name: "mac outranks endpoint", + mac: "AA:BB:CC:DD:EE:FF", endpoint: "modbus://192.168.1.5:502", + want: "mac:aabbccddeeff", + }, + { + name: "endpoint is the last resort", + endpoint: "modbus://192.168.1.5:502", + want: "ep:modbus://192.168.1.5:502", + }, + { + name: "nothing known yields no id", + want: "", + }, + { + // Half a hardware identity is not an identity — a make shared + // by every unit of a model would collide across devices. + name: "make without serial falls through to mac", + make: "Sungrow", mac: "aa:bb:cc:dd:ee:ff", + want: "mac:aabbccddeeff", + }, + { + name: "serial without make falls through to mac", + serial: "A2340", mac: "aa:bb:cc:dd:ee:ff", + want: "mac:aabbccddeeff", + }, + { + // Make is normalized but serial is not: vendor serials are + // case-significant and two units may differ only in case. + name: "make is lowercased and serial case is preserved", + make: "SunGrow", serial: "A2340b", + want: "sungrow:A2340b", + }, + { + name: "surrounding whitespace is trimmed", + make: " sungrow ", serial: " A2340 ", + want: "sungrow:A2340", + }, + { + name: "mac separators are stripped and case normalized", + mac: " AA:BB:CC:DD:EE:FF ", + want: "mac:aabbccddeeff", + }, + { + name: "whitespace-only fields count as absent", + make: " ", serial: " ", mac: " ", endpoint: "ep-fallback", + want: "ep:ep-fallback", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := ResolveDeviceID(tc.make, tc.serial, tc.mac, tc.endpoint) + if got != tc.want { + t.Errorf("ResolveDeviceID(%q, %q, %q, %q) = %q, want %q", + tc.make, tc.serial, tc.mac, tc.endpoint, got, tc.want) + } + }) + } +} + +func TestRegisterDeviceRejectsAnUnidentifiableDevice(t *testing.T) { + s := freshStore(t) + if id, err := s.RegisterDevice(Device{DriverName: "inv"}); err == nil { + t.Fatalf("RegisterDevice with no identity bits = (%q, nil), want an error", id) + } + devs, err := s.AllDevices() + if err != nil { + t.Fatal(err) + } + if len(devs) != 0 { + t.Fatalf("AllDevices = %d rows, want 0 — a nameless device must not be stored", len(devs)) + } +} + +func TestRegisterDeviceIsIdempotentAndPreservesKnownIdentity(t *testing.T) { + s := freshStore(t) + const mac = "aa:bb:cc:dd:ee:ff" + + id, err := s.RegisterDevice(Device{ + DriverName: "inv", MAC: mac, Endpoint: "http://192.168.1.5", FirstSeenMs: 1000, + }) + if err != nil { + t.Fatal(err) + } + if id != "mac:aabbccddeeff" { + t.Fatalf("device_id = %q, want mac:aabbccddeeff", id) + } + + // Re-register the same row after a cold ARP cache: the MAC is no + // longer resolvable and the serial has since arrived. The stored MAC + // must survive the empty field, otherwise the row loses the very + // identity it is keyed on. + if _, err := s.RegisterDevice(Device{ + DeviceID: id, DriverName: "inv", Make: "sungrow", Serial: "A2340", FirstSeenMs: 9999, + }); err != nil { + t.Fatal(err) + } + + devs, err := s.AllDevices() + if err != nil { + t.Fatal(err) + } + if len(devs) != 1 { + t.Fatalf("AllDevices = %d rows, want 1 — re-registering must upsert, not insert", len(devs)) + } + got := devs[0] + if got.MAC != mac { + t.Errorf("mac = %q, want %q — an empty field must not erase a known one", got.MAC, mac) + } + if got.Endpoint != "http://192.168.1.5" { + t.Errorf("endpoint = %q, want it preserved", got.Endpoint) + } + if got.Make != "sungrow" || got.Serial != "A2340" { + t.Errorf("make/serial = %q/%q, want the newly-learned values", got.Make, got.Serial) + } + if got.FirstSeenMs != 1000 { + t.Errorf("first_seen_ms = %d, want 1000 — first sighting is immutable", got.FirstSeenMs) + } +} + +// Documents a real consequence of the priority order rather than +// asserting it is desirable: registerAllDevices resolves the id afresh on +// every pass, so a device first seen by MAC is re-keyed the moment its +// driver reports a serial, leaving the mac: row behind. Any state still +// keyed on the old id is orphaned at that point. +func TestRegisterDeviceRekeysWhenSerialArrivesAfterMAC(t *testing.T) { + s := freshStore(t) + + first, err := s.RegisterDevice(Device{DriverName: "inv", MAC: "aa:bb:cc:dd:ee:ff"}) + if err != nil { + t.Fatal(err) + } + second, err := s.RegisterDevice(Device{ + DriverName: "inv", Make: "sungrow", Serial: "A2340", MAC: "aa:bb:cc:dd:ee:ff", + }) + if err != nil { + t.Fatal(err) + } + + if first == second { + t.Fatalf("device_id stayed %q after the serial arrived; this test is stale", first) + } + devs, err := s.AllDevices() + if err != nil { + t.Fatal(err) + } + if len(devs) != 2 { + t.Fatalf("AllDevices = %d rows, want 2 (the orphaned %q plus %q)", len(devs), first, second) + } +} + +func TestLookupDeviceByDriverName(t *testing.T) { + s := freshStore(t) + if got := s.LookupDeviceByDriverName("inv"); got != nil { + t.Fatalf("cold lookup = %+v, want nil", got) + } + + id, err := s.RegisterDevice(Device{DriverName: "inv", Make: "sungrow", Serial: "A2340"}) + if err != nil { + t.Fatal(err) + } + got := s.LookupDeviceByDriverName("inv") + if got == nil { + t.Fatal("lookup after register = nil, want the registered device") + } + if got.DeviceID != id { + t.Errorf("device_id = %q, want %q", got.DeviceID, id) + } + if s.LookupDeviceByDriverName("other") != nil { + t.Error("lookup of an unregistered driver name returned a device") + } +}