From 2bc48774c216ff53c6e8060e321f2edbb6d368fd Mon Sep 17 00:00:00 2001 From: rcourtman Date: Thu, 16 Jul 2026 09:50:04 +0100 Subject: [PATCH] Let deleted hosts re-enroll with a freshly generated token Deleting a host writes a machine-id-keyed removal block that rejected every future report with HTTP 400, and the error pointed at an Allow reconnect control that is not wired into the UI, leaving the machine permanently unable to enroll without changing its machine-id (#1581). Three holes made the block effectively immortal: - The 24h TTL sweep only iterated the in-memory removal maps, which reset on every restart, so persisted blocks never expired. Sweep the persisted entries by their own RemovedAt for host agents, Docker hosts, and Kubernetes clusters. - AllowHostAgentReenroll (and the Docker and Kubernetes equivalents) bailed out when the ID was missing from the in-memory map, so even the API escape hatch stopped clearing persisted blocks after a restart. Check and clear the persisted store independently. - A report presenting an API token created after the removal is explicit re-add intent (the user generated a fresh install command), so clear the block and accept it. A still-running old agent keeps presenting its pre-removal token and stays blocked. Also reword the rejection to describe the two working recovery paths instead of the unwired Settings control. --- .../monitoring/canonical_guardrails_test.go | 2 +- internal/monitoring/kubernetes_agents.go | 44 ++++--- internal/monitoring/monitor_agents.go | 107 +++++++++++----- .../monitoring/monitor_host_agents_test.go | 115 ++++++++++++++++++ 4 files changed, 220 insertions(+), 48 deletions(-) diff --git a/internal/monitoring/canonical_guardrails_test.go b/internal/monitoring/canonical_guardrails_test.go index 5d0df0e14..7f7d70f63 100644 --- a/internal/monitoring/canonical_guardrails_test.go +++ b/internal/monitoring/canonical_guardrails_test.go @@ -2194,7 +2194,7 @@ func TestHostAgentRemovalGuardUsesResolvedIdentifier(t *testing.T) { } source := string(data) requiredSnippets := []string{ - "removedAt, wasRemoved := m.lookupRemovedHostAgent(identifier, hostname, report.Host.MachineID, tokenID)", + "blockedID, removedAt, wasRemoved := m.lookupRemovedHostAgent(identifier, hostname, report.Host.MachineID, tokenID)", "func removedHostAgentMatchesReport(entry models.RemovedHostAgent, identifier, hostname, machineID, tokenID string) bool {", "entryTokenID == \"\" || tokenID == \"\" || entryTokenID != tokenID", `Str("hostID", identifier)`, diff --git a/internal/monitoring/kubernetes_agents.go b/internal/monitoring/kubernetes_agents.go index 1f1402ab4..0155c7200 100644 --- a/internal/monitoring/kubernetes_agents.go +++ b/internal/monitoring/kubernetes_agents.go @@ -1113,14 +1113,12 @@ func (m *Monitor) AllowKubernetesClusterReenroll(clusterID string) error { } m.mu.Lock() - _, exists := m.removedKubernetesClusters[clusterID] - if !exists { - m.mu.Unlock() - return nil - } delete(m.removedKubernetesClusters, clusterID) m.mu.Unlock() + // Clear the persisted entry regardless of in-memory presence: the map + // resets on restart while the persisted entry keeps blocking reports + // (#1581). m.state.RemoveRemovedKubernetesCluster(clusterID) return nil } @@ -1193,20 +1191,36 @@ func (m *Monitor) evaluateKubernetesAgents(now time.Time) { } } +// cleanupRemovedKubernetesClusters expires removed cluster blocks older than +// the TTL. Persisted entries are swept by their own RemovedAt because the +// in-memory map resets on restart (see cleanupRemovedHostAgents, #1581). func (m *Monitor) cleanupRemovedKubernetesClusters(now time.Time) { - m.mu.Lock() - defer m.mu.Unlock() + expired := make(map[string]time.Time) + for _, entry := range m.state.GetRemovedKubernetesClusters() { + if now.Sub(entry.RemovedAt) > removedKubernetesClustersTTL { + expired[entry.ID] = entry.RemovedAt + } + } + + m.mu.Lock() for clusterID, removedAt := range m.removedKubernetesClusters { if now.Sub(removedAt) > removedKubernetesClustersTTL { - delete(m.removedKubernetesClusters, clusterID) - m.state.RemoveRemovedKubernetesCluster(clusterID) - if logging.IsLevelEnabled(zerolog.DebugLevel) { - log.Debug(). - Str("k8sClusterID", clusterID). - Time("removedAt", removedAt). - Msg("Cleaned up stale removed Kubernetes cluster block") - } + expired[clusterID] = removedAt + } + } + for clusterID := range expired { + delete(m.removedKubernetesClusters, clusterID) + } + m.mu.Unlock() + + for clusterID, removedAt := range expired { + m.state.RemoveRemovedKubernetesCluster(clusterID) + if logging.IsLevelEnabled(zerolog.DebugLevel) { + log.Debug(). + Str("k8sClusterID", clusterID). + Time("removedAt", removedAt). + Msg("Cleaned up stale removed Kubernetes cluster block") } } } diff --git a/internal/monitoring/monitor_agents.go b/internal/monitoring/monitor_agents.go index 266179eab..e9b7f6ea6 100644 --- a/internal/monitoring/monitor_agents.go +++ b/internal/monitoring/monitor_agents.go @@ -264,13 +264,22 @@ func (m *Monitor) AllowHostAgentReenroll(hostID string) error { if m.removedHostAgents == nil { m.removedHostAgents = make(map[string]time.Time) } - _, exists := m.removedHostAgents[hostID] - if exists { - delete(m.removedHostAgents, hostID) - } + _, existsInMemory := m.removedHostAgents[hostID] + delete(m.removedHostAgents, hostID) m.mu.Unlock() - if !exists { + // The in-memory map resets on restart while the persisted entry keeps + // blocking reports, so the persisted store must be checked and cleared + // independently of memory presence (#1581). + existsInState := false + for _, entry := range m.state.GetRemovedHostAgents() { + if strings.TrimSpace(entry.ID) == hostID { + existsInState = true + break + } + } + + if !existsInMemory && !existsInState { log.Info(). Str("hostID", hostID). Msg("allow re-enroll requested but host agent was not blocked; ignoring") @@ -286,7 +295,7 @@ func (m *Monitor) AllowHostAgentReenroll(hostID string) error { return nil } -func (m *Monitor) lookupRemovedHostAgent(identifier, hostname, machineID, tokenID string) (time.Time, bool) { +func (m *Monitor) lookupRemovedHostAgent(identifier, hostname, machineID, tokenID string) (string, time.Time, bool) { identifier = strings.TrimSpace(identifier) machineID = sanitizeDockerHostSuffix(machineID) tokenID = strings.TrimSpace(tokenID) @@ -295,16 +304,16 @@ func (m *Monitor) lookupRemovedHostAgent(identifier, hostname, machineID, tokenI removedAt, wasRemoved := m.removedHostAgents[identifier] m.mu.RUnlock() if wasRemoved { - return removedAt, true + return identifier, removedAt, true } for _, entry := range m.state.GetRemovedHostAgents() { if removedHostAgentMatchesReport(entry, identifier, hostname, machineID, tokenID) { - return entry.RemovedAt, true + return strings.TrimSpace(entry.ID), entry.RemovedAt, true } } - return time.Time{}, false + return "", time.Time{}, false } func removedHostAgentMatchesReport(entry models.RemovedHostAgent, identifier, hostname, machineID, tokenID string) bool { @@ -646,20 +655,23 @@ func (m *Monitor) AllowDockerHostReenroll(hostID string) error { m.mu.Lock() defer m.mu.Unlock() - if host, resolvedHostID, found := m.resolveDockerCommandHostLocked(hostID); found { + if _, resolvedHostID, found := m.resolveDockerCommandHostLocked(hostID); found { hostID = resolvedHostID - if _, exists := m.removedDockerHosts[hostID]; !exists { - event := log.Info(). - Str("dockerHostID", hostID) - if hostname := strings.TrimSpace(host.Hostname()); hostname != "" { - event = event.Str("dockerHost", hostname) - } - event.Msg("allow re-enroll requested but host was not blocked; ignoring") - return nil + } + + // The in-memory map resets on restart while the persisted entry keeps + // blocking reports, so the persisted store must be checked and cleared + // independently of memory presence (#1581). + _, existsInMemory := m.removedDockerHosts[hostID] + existsInState := false + for _, entry := range m.state.GetRemovedDockerHosts() { + if strings.TrimSpace(entry.ID) == hostID { + existsInState = true + break } } - if _, exists := m.removedDockerHosts[hostID]; !exists { + if !existsInMemory && !existsInState { event := log.Info(). Str("dockerHostID", hostID) if host, found := m.stateDockerHostByIDLocked(hostID); found { @@ -1766,13 +1778,29 @@ func (m *Monitor) ApplyHostReport(report agentshost.Report, tokenRecord *config. if tokenRecord != nil { tokenID = strings.TrimSpace(tokenRecord.ID) } - removedAt, wasRemoved := m.lookupRemovedHostAgent(identifier, hostname, report.Host.MachineID, tokenID) + blockedID, removedAt, wasRemoved := m.lookupRemovedHostAgent(identifier, hostname, report.Host.MachineID, tokenID) + if wasRemoved && tokenRecord != nil && !tokenRecord.CreatedAt.IsZero() && tokenRecord.CreatedAt.After(removedAt) { + // A token minted after the host was removed means the user generated a + // fresh install command for this machine: that is explicit re-enroll + // intent, so clear the block instead of rejecting until the TTL + // expires. A still-running old agent keeps presenting its pre-removal + // token and stays blocked (#1581). + if err := m.AllowHostAgentReenroll(blockedID); err == nil { + log.Info(). + Str("hostID", identifier). + Str("blockedID", blockedID). + Time("removedAt", removedAt). + Time("tokenCreatedAt", tokenRecord.CreatedAt). + Msg("Cleared host agent removal block: report presented a token created after removal") + wasRemoved = false + } + } if wasRemoved { log.Info(). Str("hostID", identifier). Time("removedAt", removedAt). Msg("Rejecting report from deliberately removed host agent") - return models.Host{}, fmt.Errorf("host agent %q had monitoring stopped at %v and cannot report again. Use Allow reconnect in Settings -> Infrastructure before reconnecting this host", identifier, removedAt.Format(time.RFC3339)) + return models.Host{}, fmt.Errorf("host agent %q had monitoring stopped at %v and cannot report again. Re-enroll by reinstalling the agent with a newly generated API token, or wait for the block to clear 24 hours after removal", identifier, removedAt.Format(time.RFC3339)) } var previous *unifiedresources.HostView @@ -2895,25 +2923,31 @@ func recoverFromPanic(goroutineName string) { } } -// cleanupRemovedDockerHosts removes entries from the removed hosts map that are older than 24 hours. +// cleanupRemovedDockerHosts expires removed Docker host blocks older than 24 +// hours. Persisted entries are swept by their own RemovedAt because the +// in-memory map resets on restart (see cleanupRemovedHostAgents, #1581). func (m *Monitor) cleanupRemovedDockerHosts(now time.Time) { - // Collect IDs to remove first to avoid holding lock during state update - var toRemove []string + expired := make(map[string]time.Time) + + for _, entry := range m.state.GetRemovedDockerHosts() { + if now.Sub(entry.RemovedAt) > removedDockerHostsTTL { + expired[entry.ID] = entry.RemovedAt + } + } m.mu.Lock() for hostID, removedAt := range m.removedDockerHosts { if now.Sub(removedAt) > removedDockerHostsTTL { - toRemove = append(toRemove, hostID) + expired[hostID] = removedAt } } m.mu.Unlock() // Remove from state and map without holding both locks - for _, hostID := range toRemove { + for hostID, removedAt := range expired { m.state.RemoveRemovedDockerHost(hostID) m.mu.Lock() - removedAt := m.removedDockerHosts[hostID] delete(m.removedDockerHosts, hostID) m.mu.Unlock() @@ -2924,23 +2958,32 @@ func (m *Monitor) cleanupRemovedDockerHosts(now time.Time) { } } -// cleanupRemovedHostAgents removes entries from the removed host-agent map that are older than 24 hours. +// cleanupRemovedHostAgents expires removed host-agent blocks older than 24 hours. +// The persisted entries must be swept by their own RemovedAt, not via the +// in-memory map: the map resets on restart while lookupRemovedHostAgent keeps +// matching persisted entries, which made a deleted host's block immortal and +// permanently rejected its agent's re-enrollment (#1581). func (m *Monitor) cleanupRemovedHostAgents(now time.Time) { - var toRemove []string + expired := make(map[string]time.Time) + + for _, entry := range m.state.GetRemovedHostAgents() { + if now.Sub(entry.RemovedAt) > removedHostAgentsTTL { + expired[entry.ID] = entry.RemovedAt + } + } m.mu.Lock() for hostID, removedAt := range m.removedHostAgents { if now.Sub(removedAt) > removedHostAgentsTTL { - toRemove = append(toRemove, hostID) + expired[hostID] = removedAt } } m.mu.Unlock() - for _, hostID := range toRemove { + for hostID, removedAt := range expired { m.state.RemoveRemovedHostAgent(hostID) m.mu.Lock() - removedAt := m.removedHostAgents[hostID] delete(m.removedHostAgents, hostID) m.mu.Unlock() diff --git a/internal/monitoring/monitor_host_agents_test.go b/internal/monitoring/monitor_host_agents_test.go index d85e83c53..142cddb9d 100644 --- a/internal/monitoring/monitor_host_agents_test.go +++ b/internal/monitoring/monitor_host_agents_test.go @@ -3271,6 +3271,121 @@ func TestRemoveHostAgent_BlocksFutureReportsUntilAllowed(t *testing.T) { } } +// TestApplyHostReport_FreshTokenClearsRemovalBlock pins the #1581 re-enroll +// path: a report presenting a token created after the removal is explicit +// re-add intent and clears the block, while a pre-removal token stays blocked. +func TestApplyHostReport_FreshTokenClearsRemovalBlock(t *testing.T) { + monitor := &Monitor{ + state: models.NewState(), + alertManager: alerts.NewManager(), + hostTokenBindings: make(map[string]string), + removedHostAgents: make(map[string]time.Time), + rateTracker: NewRateTracker(), + config: &config.Config{}, + } + t.Cleanup(func() { monitor.alertManager.Stop() }) + + hostID := "host-fresh-token" + monitor.state.UpsertHost(models.Host{ + ID: hostID, + Hostname: "fresh-token.local", + }) + if _, err := monitor.RemoveHostAgent(hostID); err != nil { + t.Fatalf("remove host agent: %v", err) + } + + report := agentshost.Report{ + Host: agentshost.HostInfo{ + ID: hostID, + Hostname: "fresh-token.local", + }, + Agent: agentshost.AgentInfo{ID: hostID}, + Timestamp: time.Now(), + } + + staleToken := &config.APITokenRecord{ + ID: "token-old", + CreatedAt: time.Now().Add(-time.Hour), + } + if _, err := monitor.ApplyHostReport(report, staleToken); err == nil { + t.Fatal("expected report with pre-removal token to stay blocked") + } + + freshToken := &config.APITokenRecord{ + ID: "token-new", + CreatedAt: time.Now().Add(time.Minute), + } + if _, err := monitor.ApplyHostReport(report, freshToken); err != nil { + t.Fatalf("expected report with post-removal token to clear the block, got %v", err) + } + + if len(monitor.state.GetRemovedHostAgents()) != 0 { + t.Fatal("expected persisted removal block to be cleared") + } +} + +// TestAllowHostAgentReenroll_ClearsPersistedBlockAfterRestart pins the #1581 +// restart hole: the in-memory map is empty after a restart, but the persisted +// entry must still be found and cleared. +func TestAllowHostAgentReenroll_ClearsPersistedBlockAfterRestart(t *testing.T) { + monitor := &Monitor{ + state: models.NewState(), + alertManager: alerts.NewManager(), + hostTokenBindings: make(map[string]string), + removedHostAgents: make(map[string]time.Time), + rateTracker: NewRateTracker(), + config: &config.Config{}, + } + t.Cleanup(func() { monitor.alertManager.Stop() }) + + hostID := "host-restart-block" + monitor.state.AddRemovedHostAgent(models.RemovedHostAgent{ + ID: hostID, + Hostname: "restart-block.local", + RemovedAt: time.Now().Add(-time.Hour), + }) + + if err := monitor.AllowHostAgentReenroll(hostID); err != nil { + t.Fatalf("allow host reenroll: %v", err) + } + if len(monitor.state.GetRemovedHostAgents()) != 0 { + t.Fatal("expected persisted removal block to be cleared without an in-memory entry") + } +} + +// TestCleanupRemovedHostAgents_ExpiresPersistedEntries pins the #1581 TTL +// hole: persisted blocks must expire by their own RemovedAt even when the +// in-memory map lost them across a restart. +func TestCleanupRemovedHostAgents_ExpiresPersistedEntries(t *testing.T) { + monitor := &Monitor{ + state: models.NewState(), + hostTokenBindings: make(map[string]string), + removedHostAgents: make(map[string]time.Time), + config: &config.Config{}, + } + + monitor.state.AddRemovedHostAgent(models.RemovedHostAgent{ + ID: "host-expired", + Hostname: "expired.local", + RemovedAt: time.Now().Add(-25 * time.Hour), + }) + monitor.state.AddRemovedHostAgent(models.RemovedHostAgent{ + ID: "host-recent", + Hostname: "recent.local", + RemovedAt: time.Now().Add(-time.Hour), + }) + + monitor.cleanupRemovedHostAgents(time.Now()) + + remaining := monitor.state.GetRemovedHostAgents() + if len(remaining) != 1 { + t.Fatalf("expected exactly one persisted block to survive, got %d", len(remaining)) + } + if remaining[0].ID != "host-recent" { + t.Fatalf("expected host-recent to survive, got %q", remaining[0].ID) + } +} + func TestApplyHostReport_MissingHostname(t *testing.T) { monitor := &Monitor{ state: models.NewState(),