From 93ac3232c895603657134062350762e862937bc4 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 17 Dec 2025 20:09:55 +0000 Subject: [PATCH] hostagent: avoid identity collisions with MAC fallback (Related to #836) --- internal/hostagent/agent.go | 94 ++++++++++++++++++- internal/hostagent/agent_new_test.go | 44 ++++++++- internal/hostagent/agent_test.go | 43 ++++++++- internal/monitoring/monitor.go | 58 ++++++++---- .../monitoring/monitor_host_agents_test.go | 69 ++++++++++++++ 5 files changed, 282 insertions(+), 26 deletions(-) diff --git a/internal/hostagent/agent.go b/internal/hostagent/agent.go index ea6053d..17f4e36 100644 --- a/internal/hostagent/agent.go +++ b/internal/hostagent/agent.go @@ -7,9 +7,11 @@ import ( "encoding/json" "errors" "fmt" + "net" "net/http" "os" "runtime" + "sort" "strings" "time" @@ -71,6 +73,7 @@ type Agent struct { const defaultInterval = 30 * time.Second var readFile = os.ReadFile +var netInterfaces = net.Interfaces var ( hostInfoWithContext = gohost.InfoWithContext @@ -698,7 +701,7 @@ func isLXCContainer() bool { func getReliableMachineID(gopsutilHostID string, logger zerolog.Logger) string { gopsutilID := strings.TrimSpace(gopsutilHostID) - // On Linux, always prefer /etc/machine-id as it's guaranteed unique per installation. + // On Linux, prefer /etc/machine-id when available. // This avoids ID collisions from: // - LXC containers sharing host's DMI product UUID // - Cloned VMs with identical hardware UUIDs @@ -706,9 +709,9 @@ func getReliableMachineID(gopsutilHostID string, logger zerolog.Logger) string { if runtime.GOOS == "linux" { if data, err := readFile("/etc/machine-id"); err == nil { machineID := strings.TrimSpace(string(data)) - if len(machineID) >= 32 { - // Format as UUID if it's a 32-char hex string (like machine-id typically is) - if len(machineID) == 32 { + if machineID != "" && len(machineID) >= 8 { + // Format as UUID if it's a 32-char hex string (like machine-id typically is). + if len(machineID) == 32 && isHexString(machineID) { machineID = fmt.Sprintf("%s-%s-%s-%s-%s", machineID[0:8], machineID[8:12], machineID[12:16], machineID[16:20], machineID[20:32]) @@ -725,7 +728,90 @@ func getReliableMachineID(gopsutilHostID string, logger zerolog.Logger) string { return machineID } } + + if macID := getPrimaryMACIdentifier(); macID != "" { + logger.Debug(). + Str("machineID", macID). + Msg("Linux host missing usable /etc/machine-id, using MAC address for unique identification") + return macID + } } return gopsutilID } + +func isHexString(input string) bool { + for i := 0; i < len(input); i++ { + ch := input[i] + switch { + case ch >= '0' && ch <= '9': + case ch >= 'a' && ch <= 'f': + case ch >= 'A' && ch <= 'F': + default: + return false + } + } + return input != "" +} + +func getPrimaryMACIdentifier() string { + interfaces, err := netInterfaces() + if err != nil { + return "" + } + + sort.Slice(interfaces, func(i, j int) bool { + return interfaces[i].Name < interfaces[j].Name + }) + + // Prefer a stable-looking interface name first to avoid selecting docker bridges + // or other virtual interfaces when physical interfaces are present. + for pass := 0; pass < 2; pass++ { + for _, iface := range interfaces { + if len(iface.HardwareAddr) == 0 { + continue + } + if iface.Flags&net.FlagLoopback != 0 { + continue + } + if pass == 0 && isLikelyVirtualInterfaceName(iface.Name) { + continue + } + + mac := strings.ToLower(iface.HardwareAddr.String()) + normalized := strings.NewReplacer(":", "", "-", "", ".", "").Replace(mac) + if normalized == "" { + continue + } + return "mac-" + normalized + } + } + + return "" +} + +func isLikelyVirtualInterfaceName(name string) bool { + name = strings.ToLower(strings.TrimSpace(name)) + switch { + case name == "": + return true + case name == "lo": + return true + case strings.HasPrefix(name, "docker"): + return true + case strings.HasPrefix(name, "veth"): + return true + case strings.HasPrefix(name, "br-"): + return true + case strings.HasPrefix(name, "cni"): + return true + case strings.HasPrefix(name, "flannel"): + return true + case strings.HasPrefix(name, "virbr"): + return true + case strings.HasPrefix(name, "zt"): + return true + default: + return false + } +} diff --git a/internal/hostagent/agent_new_test.go b/internal/hostagent/agent_new_test.go index 09a08bd..c3fb2f6 100644 --- a/internal/hostagent/agent_new_test.go +++ b/internal/hostagent/agent_new_test.go @@ -3,6 +3,8 @@ package hostagent import ( "context" "crypto/tls" + "errors" + "net" "net/http" "os" "runtime" @@ -137,12 +139,14 @@ func TestNew_DefaultPulseURL(t *testing.T) { } } -func TestNew_FallsBackToHostnameWhenMachineIDEmpty(t *testing.T) { +func TestNew_FallsBackToHostnameWhenMachineIDAndMACEmpty(t *testing.T) { originalHostInfo := hostInfoWithContext originalReadFile := readFile + originalNetInterfaces := netInterfaces t.Cleanup(func() { hostInfoWithContext = originalHostInfo readFile = originalReadFile + netInterfaces = originalNetInterfaces }) hostInfoWithContext = func(context.Context) (*gohost.InfoStat, error) { @@ -153,6 +157,7 @@ func TestNew_FallsBackToHostnameWhenMachineIDEmpty(t *testing.T) { }, nil } readFile = func(string) ([]byte, error) { return nil, os.ErrNotExist } + netInterfaces = func() ([]net.Interface, error) { return nil, errors.New("no interfaces") } agent, err := New(Config{APIToken: "token", LogLevel: zerolog.InfoLevel}) if err != nil { @@ -165,3 +170,40 @@ func TestNew_FallsBackToHostnameWhenMachineIDEmpty(t *testing.T) { t.Fatalf("agentID = %q, want %q", agent.agentID, "host-from-info") } } + +func TestNew_FallsBackToMACWhenMachineIDEmpty(t *testing.T) { + originalHostInfo := hostInfoWithContext + originalReadFile := readFile + originalNetInterfaces := netInterfaces + t.Cleanup(func() { + hostInfoWithContext = originalHostInfo + readFile = originalReadFile + netInterfaces = originalNetInterfaces + }) + + hostInfoWithContext = func(context.Context) (*gohost.InfoStat, error) { + return &gohost.InfoStat{ + Hostname: "host-from-info", + HostID: "", + KernelArch: runtime.GOARCH, + }, nil + } + readFile = func(string) ([]byte, error) { return nil, os.ErrNotExist } + netInterfaces = func() ([]net.Interface, error) { + return []net.Interface{ + {Name: "eth0", HardwareAddr: net.HardwareAddr{0x02, 0x00, 0x00, 0x00, 0x00, 0x01}}, + }, nil + } + + agent, err := New(Config{APIToken: "token", LogLevel: zerolog.InfoLevel}) + if err != nil { + t.Fatalf("New: %v", err) + } + + if agent.machineID != "mac-020000000001" { + t.Fatalf("machineID = %q, want %q", agent.machineID, "mac-020000000001") + } + if agent.agentID != "mac-020000000001" { + t.Fatalf("agentID = %q, want %q", agent.agentID, "mac-020000000001") + } +} diff --git a/internal/hostagent/agent_test.go b/internal/hostagent/agent_test.go index 6cabb55..7efeb01 100644 --- a/internal/hostagent/agent_test.go +++ b/internal/hostagent/agent_test.go @@ -1,6 +1,8 @@ package hostagent import ( + "errors" + "net" "os" "runtime" "testing" @@ -80,10 +82,15 @@ func TestGetReliableMachineID(t *testing.T) { logger := zerolog.Nop() originalReadFile := readFile - t.Cleanup(func() { readFile = originalReadFile }) + originalNetInterfaces := netInterfaces + t.Cleanup(func() { + readFile = originalReadFile + netInterfaces = originalNetInterfaces + }) t.Run("trims whitespace", func(t *testing.T) { readFile = func(string) ([]byte, error) { return nil, os.ErrNotExist } + netInterfaces = func() ([]net.Interface, error) { return nil, errors.New("no interfaces") } result := getReliableMachineID(" test-id ", logger) if result != "test-id" { t.Errorf("getReliableMachineID trimmed result = %q, want %q", result, "test-id") @@ -100,6 +107,7 @@ func TestGetReliableMachineID(t *testing.T) { } return nil, os.ErrNotExist } + netInterfaces = func() ([]net.Interface, error) { return nil, errors.New("no interfaces") } result := getReliableMachineID("gopsutil-product-uuid", logger) const want = "01234567-89ab-cdef-0123-456789abcdef" @@ -108,24 +116,49 @@ func TestGetReliableMachineID(t *testing.T) { } }) - t.Run("Linux falls back to gopsutil ID when /etc/machine-id missing", func(t *testing.T) { + t.Run("Linux falls back to MAC when /etc/machine-id missing", func(t *testing.T) { readFile = func(string) ([]byte, error) { return nil, os.ErrNotExist } + netInterfaces = func() ([]net.Interface, error) { + return []net.Interface{ + { + Name: "eth0", + HardwareAddr: net.HardwareAddr{0x00, 0x11, 0x22, 0xAA, 0xBB, 0xCC}, + }, + }, nil + } + result := getReliableMachineID("gopsutil-product-uuid", logger) + if result != "mac-001122aabbcc" { + t.Errorf("getReliableMachineID() = %q, want %q", result, "mac-001122aabbcc") + } + }) + + t.Run("Linux falls back to gopsutil ID when machine-id missing and MAC unavailable", func(t *testing.T) { + readFile = func(string) ([]byte, error) { return nil, os.ErrNotExist } + netInterfaces = func() ([]net.Interface, error) { return nil, errors.New("no interfaces") } result := getReliableMachineID("gopsutil-product-uuid", logger) if result != "gopsutil-product-uuid" { t.Errorf("getReliableMachineID() = %q, want %q", result, "gopsutil-product-uuid") } }) - t.Run("Linux falls back when machine-id is too short", func(t *testing.T) { + t.Run("Linux falls back to MAC when machine-id is too short", func(t *testing.T) { readFile = func(name string) ([]byte, error) { if name == "/etc/machine-id" { return []byte("short\n"), nil } return nil, os.ErrNotExist } + netInterfaces = func() ([]net.Interface, error) { + return []net.Interface{ + { + Name: "eth0", + HardwareAddr: net.HardwareAddr{0xDE, 0xAD, 0xBE, 0xEF, 0x00, 0x01}, + }, + }, nil + } result := getReliableMachineID("gopsutil-product-uuid", logger) - if result != "gopsutil-product-uuid" { - t.Errorf("getReliableMachineID() = %q, want %q", result, "gopsutil-product-uuid") + if result != "mac-deadbeef0001" { + t.Errorf("getReliableMachineID() = %q, want %q", result, "mac-deadbeef0001") } }) } else { diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index 2d7165a..d178363 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -655,8 +655,8 @@ type Monitor struct { dlqInsightMap map[string]*dlqInsight nodeLastOnline map[string]time.Time // Track last time each node was seen online (for grace period) resourceStore ResourceStoreInterface // Optional unified resource store for polling optimization - mockMetricsCancel context.CancelFunc - mockMetricsWg sync.WaitGroup + mockMetricsCancel context.CancelFunc + mockMetricsWg sync.WaitGroup } type rrdMemCacheEntry struct { @@ -1939,26 +1939,26 @@ func (m *Monitor) ApplyHostReport(report agentshost.Report, tokenRecord *config. return models.Host{}, fmt.Errorf("host report missing hostname") } - identifier := strings.TrimSpace(report.Host.ID) - if identifier != "" { - identifier = sanitizeDockerHostSuffix(identifier) + baseIdentifier := strings.TrimSpace(report.Host.ID) + if baseIdentifier != "" { + baseIdentifier = sanitizeDockerHostSuffix(baseIdentifier) } - if identifier == "" { + if baseIdentifier == "" { if machine := sanitizeDockerHostSuffix(report.Host.MachineID); machine != "" { - identifier = machine + baseIdentifier = machine } } - if identifier == "" { + if baseIdentifier == "" { if agentID := sanitizeDockerHostSuffix(report.Agent.ID); agentID != "" { - identifier = agentID + baseIdentifier = agentID } } - if identifier == "" { + if baseIdentifier == "" { if hostName := sanitizeDockerHostSuffix(hostname); hostName != "" { - identifier = hostName + baseIdentifier = hostName } } - if identifier == "" { + if baseIdentifier == "" { seedParts := uniqueNonEmptyStrings( report.Host.MachineID, report.Agent.ID, @@ -1969,14 +1969,38 @@ func (m *Monitor) ApplyHostReport(report agentshost.Report, tokenRecord *config. } seed := strings.Join(seedParts, "|") sum := sha1.Sum([]byte(seed)) - identifier = fmt.Sprintf("host-%s", hex.EncodeToString(sum[:6])) + baseIdentifier = fmt.Sprintf("host-%s", hex.EncodeToString(sum[:6])) } existingHosts := m.state.GetHosts() - if tokenRecord != nil && tokenRecord.ID != "" { + identifier := baseIdentifier + if tokenRecord != nil && strings.TrimSpace(tokenRecord.ID) != "" { tokenID := strings.TrimSpace(tokenRecord.ID) - bindingID := identifier + + bindingID := baseIdentifier + for _, candidate := range existingHosts { + if candidate.ID != bindingID { + continue + } + if strings.TrimSpace(candidate.Hostname) == hostname && strings.TrimSpace(candidate.TokenID) == tokenID { + break + } + + seed := strings.Join([]string{tokenID, hostname, bindingID}, "|") + sum := sha1.Sum([]byte(seed)) + suffix := hex.EncodeToString(sum[:4]) + + base := bindingID + if base == "" { + base = "host" + } + if len(base) > 40 { + base = base[:40] + } + bindingID = fmt.Sprintf("%s-%s", base, suffix) + break + } m.mu.Lock() if m.hostTokenBindings == nil { @@ -2003,6 +2027,8 @@ func (m *Monitor) ApplyHostReport(report agentshost.Report, tokenRecord *config. Msg("Bound host agent token to hostname") } m.mu.Unlock() + + identifier = bindingID } var previous models.Host @@ -5317,7 +5343,7 @@ func (m *Monitor) pollPVEInstance(ctx context.Context, instanceName string, clie pollErr = ctx.Err() return default: - // Always try the efficient cluster/resources endpoint first + // Always try the efficient cluster/resources endpoint first // This endpoint works on both clustered and standalone nodes // Testing confirmed it works on standalone nodes like pimox useClusterEndpoint := m.pollVMsAndContainersEfficient(ctx, instanceName, instanceCfg.ClusterName, instanceCfg.IsCluster, client, nodeEffectiveStatus) diff --git a/internal/monitoring/monitor_host_agents_test.go b/internal/monitoring/monitor_host_agents_test.go index 0f13dd8..f73c01d 100644 --- a/internal/monitoring/monitor_host_agents_test.go +++ b/internal/monitoring/monitor_host_agents_test.go @@ -181,6 +181,75 @@ func TestApplyHostReportAllowsTokenReuseAcrossHosts(t *testing.T) { } } +func TestApplyHostReportDisambiguatesCollidingIdentifiersAcrossTokens(t *testing.T) { + t.Helper() + + monitor := &Monitor{ + state: models.NewState(), + alertManager: alerts.NewManager(), + hostTokenBindings: make(map[string]string), + config: &config.Config{}, + } + t.Cleanup(func() { monitor.alertManager.Stop() }) + + now := time.Now().UTC() + baseReport := agentshost.Report{ + Agent: agentshost.AgentInfo{ + ID: "agent-one", + Version: "1.0.0", + IntervalSeconds: 30, + }, + Host: agentshost.HostInfo{ + ID: "colliding-machine-id", + Hostname: "nas-one", + Platform: "linux", + OSName: "synology", + OSVersion: "7.0", + }, + Timestamp: now, + Metrics: agentshost.Metrics{ + CPUUsagePercent: 1.0, + }, + } + + hostOne, err := monitor.ApplyHostReport(baseReport, &config.APITokenRecord{ID: "token-one"}) + if err != nil { + t.Fatalf("ApplyHostReport hostOne: %v", err) + } + if hostOne.ID == "" { + t.Fatalf("expected hostOne to have an identifier") + } + + secondReport := baseReport + secondReport.Agent.ID = "agent-two" + secondReport.Host.Hostname = "nas-two" + secondReport.Timestamp = now.Add(30 * time.Second) + + hostTwo, err := monitor.ApplyHostReport(secondReport, &config.APITokenRecord{ID: "token-two"}) + if err != nil { + t.Fatalf("ApplyHostReport hostTwo: %v", err) + } + if hostTwo.ID == "" { + t.Fatalf("expected hostTwo to have an identifier") + } + if hostTwo.ID == hostOne.ID { + t.Fatalf("expected disambiguated host IDs, got %q", hostTwo.ID) + } + + hostTwoRepeat, err := monitor.ApplyHostReport(secondReport, &config.APITokenRecord{ID: "token-two"}) + if err != nil { + t.Fatalf("ApplyHostReport hostTwo repeat: %v", err) + } + if hostTwoRepeat.ID != hostTwo.ID { + t.Fatalf("expected stable host ID for repeated reports, got %q want %q", hostTwoRepeat.ID, hostTwo.ID) + } + + snapshot := monitor.state.GetSnapshot() + if got := len(snapshot.Hosts); got != 2 { + t.Fatalf("expected 2 hosts in state, got %d", got) + } +} + func TestRemoveHostAgentUnbindsToken(t *testing.T) { t.Helper()