diff --git a/internal/monitoring/docker_reenroll_block_test.go b/internal/monitoring/docker_reenroll_block_test.go new file mode 100644 index 000000000..e7522a0c2 --- /dev/null +++ b/internal/monitoring/docker_reenroll_block_test.go @@ -0,0 +1,94 @@ +package monitoring + +import ( + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/config" + agentsdocker "github.com/rcourtman/pulse-go-rewrite/pkg/agents/docker" +) + +func reenrollBlockTestReport(agentID string, at time.Time) agentsdocker.Report { + return agentsdocker.Report{ + Agent: agentsdocker.AgentInfo{ + ID: agentID, + Version: "6.4.1", + Type: "unified", + IntervalSeconds: 30, + }, + Host: agentsdocker.HostInfo{ + Hostname: "alpine-docker", + MachineID: "machine-alpine-docker", + TotalCPU: 4, + }, + Containers: []agentsdocker.Container{ + {ID: "container-" + agentID, Name: "portainer"}, + }, + Timestamp: at, + } +} + +// Removing a Docker host blocks its reports, but a token minted after the +// removal is explicit re-enroll intent and must clear the block — the same +// rule host agents follow (#1581). Before this rule the re-enrolled host was +// rejected until a server restart happened to wipe the in-memory block, which +// read as "Docker tab missing until reboot" (#1728). The block must also +// survive a restart via the persisted store, so the restart itself never +// becomes the thing that lifts it. +func TestDockerRemovalBlockClearsOnFreshTokenReenroll(t *testing.T) { + monitor := newTestMonitor(t) + base := time.Now().UTC().Add(-2 * time.Hour) + + oldToken := &config.APITokenRecord{ID: "token-old", CreatedAt: base} + host, err := monitor.ApplyDockerReport(reenrollBlockTestReport("agent-alpine", base.Add(time.Minute)), oldToken) + if err != nil { + t.Fatalf("initial report: %v", err) + } + + if _, err := monitor.RemoveDockerHost(host.ID); err != nil { + t.Fatalf("remove docker host: %v", err) + } + + // The still-running old agent keeps presenting its pre-removal token and + // stays blocked. + if _, err := monitor.ApplyDockerReport(reenrollBlockTestReport("agent-alpine", base.Add(2*time.Minute)), oldToken); err == nil { + t.Fatal("expected pre-removal token report to stay blocked") + } + + // Simulate a server restart: the in-memory map resets while the persisted + // entry remains. The block must hold. + monitor.mu.Lock() + monitor.removedDockerHosts = make(map[string]time.Time) + monitor.mu.Unlock() + if _, err := monitor.ApplyDockerReport(reenrollBlockTestReport("agent-alpine", base.Add(3*time.Minute)), oldToken); err == nil { + t.Fatal("expected persisted removal block to survive restart") + } + + // A token minted after removal clears the block and the report lands. + freshToken := &config.APITokenRecord{ID: "token-new", CreatedAt: time.Now().UTC()} + reenrolled, err := monitor.ApplyDockerReport(reenrollBlockTestReport("agent-alpine", base.Add(4*time.Minute)), freshToken) + if err != nil { + t.Fatalf("fresh-token re-enroll rejected: %v", err) + } + + hosts := monitor.state.GetDockerHosts() + if len(hosts) != 1 || hosts[0].ID != reenrolled.ID { + ids := make([]string, 0, len(hosts)) + for _, h := range hosts { + ids = append(ids, h.ID) + } + t.Fatalf("expected exactly the re-enrolled host in state, got %v", ids) + } + + monitor.mu.RLock() + _, stillBlockedInMemory := monitor.removedDockerHosts[host.ID] + monitor.mu.RUnlock() + if stillBlockedInMemory { + t.Fatal("expected in-memory removal block to be cleared by fresh-token re-enroll") + } + for _, entry := range monitor.state.GetRemovedDockerHosts() { + if entry.ID == host.ID { + t.Fatal("expected persisted removal block to be cleared by fresh-token re-enroll") + } + } +} diff --git a/internal/monitoring/monitor_agents.go b/internal/monitoring/monitor_agents.go index a77646603..c6e65a5e3 100644 --- a/internal/monitoring/monitor_agents.go +++ b/internal/monitoring/monitor_agents.go @@ -1087,6 +1087,39 @@ func (m *Monitor) SetDockerHostCustomDisplayName(hostID string, customName strin return host, nil } +// lookupRemovedDockerHost reports whether any of the presented Docker host +// identities is blocked by a deliberate removal, returning the blocked entry's +// ID and removal time. It consults the in-memory map first and the persisted +// store second: the map resets on restart while the persisted entry keeps +// blocking (#1581), so memory absence alone must not admit a blocked host. +func (m *Monitor) lookupRemovedDockerHost(ids []string) (string, time.Time, bool) { + m.mu.RLock() + for _, id := range ids { + id = strings.TrimSpace(id) + if id == "" { + continue + } + if ts, ok := m.removedDockerHosts[id]; ok { + m.mu.RUnlock() + return id, ts, true + } + } + m.mu.RUnlock() + + for _, entry := range m.state.GetRemovedDockerHosts() { + entryID := strings.TrimSpace(entry.ID) + if entryID == "" { + continue + } + for _, id := range ids { + if strings.TrimSpace(id) == entryID { + return entryID, entry.RemovedAt, true + } + } + } + return "", time.Time{}, false +} + // AllowDockerHostReenroll removes a host ID from the removal blocklist so it can report again. func (m *Monitor) AllowDockerHostReenroll(hostID string) error { hostID = strings.TrimSpace(hostID) @@ -1759,22 +1792,34 @@ func (m *Monitor) ApplyDockerReport(report agentsdocker.Report, tokenRecord *con return models.DockerHost{}, fmt.Errorf("docker report missing agent identifier") } - // Check if this host was deliberately removed - reject report to prevent resurrection - m.mu.RLock() - removedAt, wasRemoved := m.removedDockerHosts[identifier] - if !wasRemoved { - for _, legacyID := range legacyIDs { - if legacyID == "" || legacyID == identifier { - continue - } - if ts, ok := m.removedDockerHosts[legacyID]; ok { - removedAt = ts - wasRemoved = true - break - } + // Check if this host was deliberately removed - reject report to prevent + // resurrection. Both stores must be consulted: the in-memory map resets on + // restart while the persisted entry keeps blocking (#1581), so a restart + // must not silently lift a deliberate block. + blockedID, removedAt, wasRemoved := m.lookupRemovedDockerHost(append([]string{identifier}, legacyIDs...)) + + 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 or Allow reconnect is clicked. A still-running old agent + // keeps presenting its pre-removal token and stays blocked. This + // mirrors the host-agent rule from #1581. + if err := m.AllowDockerHostReenroll(blockedID); err != nil { + log.Warn(). + Err(err). + Str("dockerHostID", blockedID). + Msg("Failed to clear Docker host removal block for fresh re-enroll token; report remains blocked") + } else { + log.Info(). + Str("dockerHostID", blockedID). + Time("removedAt", removedAt). + Time("tokenCreatedAt", tokenRecord.CreatedAt). + Msg("Cleared Docker host removal block: report presented a token created after removal") + wasRemoved = false } } - m.mu.RUnlock() if wasRemoved { log.Info().