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