hostagent: avoid identity collisions with MAC fallback (Related to #836)

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 {
+			if machineID != "" && len(machineID) >= 8 {
-				// Format as UUID if it's a 32-char hex string (like machine-id typically is)
+				// Format as UUID if it's a 32-char hex string (like machine-id typically is).
-				if len(machineID) == 32 {
+				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
+	}
+}

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")
+	}
+}

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" {
+			if result != "mac-deadbeef0001" {
-				t.Errorf("getReliableMachineID() = %q, want %q", result, "gopsutil-product-uuid")
+				t.Errorf("getReliableMachineID() = %q, want %q", result, "mac-deadbeef0001")
 			}
 		})
 	} else {

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
+	mockMetricsCancel        context.CancelFunc
-	mockMetricsWg             sync.WaitGroup
+	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)
+	baseIdentifier := strings.TrimSpace(report.Host.ID)
-	if identifier != "" {
+	if baseIdentifier != "" {
-		identifier = sanitizeDockerHostSuffix(identifier)
+		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)

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()