From 3c31aa4805423a8f3d1427e1b00ac90dbe60a024 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:26:48 +0100 Subject: [PATCH] Stop Docker records flip-flopping under shared same-hostname tokens The Docker analog of the #1753 estate was still broken: two live machines reusing one short hostname and one pasted unified install token collapsed into a single flip-flopping DockerHost record, because the hostname+token identity fallback adopts a record whose machine ID disagrees with the report's. That fold is deliberate for recreated containers (whose /etc/machine-id regenerates), so it cannot simply be guarded by machine-ID inequality - the discriminator is a revisit: a recreated container transitions to its new machine ID exactly once, while two live machines alternate. Removing the collapsed record then revoked the shared token unconditionally, rejecting every surviving module - host reports included, since a unified install shares one credential - with 401 "Unauthorized access attempt". Consult the identity flap tracker before the hostname fallbacks adopt a machine-ID-disagreeing record: a report whose machine ID returns to a value already seen behind that identity is a second live machine and is not folded. The machine whose identifiers minted the record reclaims it, so the first site keeps its record and history, and the other site falls through to the token binding check, converging on the documented "Each Docker / Podman module must use a unique API token" rejection instead of silently overwriting the record every cycle. RemoveDockerHost now skips token revocation while any host or Docker record still authenticates with the credential, mirroring the host-agent removal guard. Regression coverage: an end-to-end router test walking the two-site shared-token estate (host + Docker reports, alternating cycles, removal) asserting the first site's identity stays stable, the second site gets the unique-token guidance, and the shared token survives removal; a router test proving removal of a machine's Docker record keeps the unified token its host record still uses; and a state-layer test pinning the reclaim/no-flip-flop convergence. Recreated-container adoption and the existing token-uniqueness rejections keep their tests unchanged. --- .../api/issue1753_docker_report_auth_test.go | 246 ++++++++++++++++++ internal/monitoring/docker_host_identity.go | 53 +++- .../monitoring/docker_host_identity_test.go | 6 +- .../monitoring/docker_shared_token_test.go | 99 +++++++ internal/monitoring/identity_flap_tracker.go | 33 +++ internal/monitoring/monitor_agents.go | 53 +++- 6 files changed, 476 insertions(+), 14 deletions(-) create mode 100644 internal/api/issue1753_docker_report_auth_test.go create mode 100644 internal/monitoring/docker_shared_token_test.go diff --git a/internal/api/issue1753_docker_report_auth_test.go b/internal/api/issue1753_docker_report_auth_test.go new file mode 100644 index 000000000..97bae8d75 --- /dev/null +++ b/internal/api/issue1753_docker_report_auth_test.go @@ -0,0 +1,246 @@ +package api + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/api/agentbinding" + "github.com/rcourtman/pulse-go-rewrite/internal/api/agenttokens" + "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/monitoring" + agentsdocker "github.com/rcourtman/pulse-go-rewrite/pkg/agents/docker" +) + +// Docker analog of issue #1753: two standalone sites reuse one short hostname +// and one pasted unified install token. The host-agent side of this estate was +// fixed by machine-qualifying the host token bindings; these tests walk the +// same estate through /api/agents/docker/report and fail if the Docker records +// collapse into one identity, or if removing one Docker host revokes the +// shared token while a sibling record still authenticates with it - the same +// 401 "Unauthorized access attempt" chain the reporter hit on the host path. + +func issue1753DockerInstallTokenRecord(t *testing.T, raw, site string) config.APITokenRecord { + t.Helper() + record, err := config.NewAPITokenRecord(raw, "Unified agent install ("+site+")", agenttokens.HostScopes(false)) + if err != nil { + t.Fatalf("NewAPITokenRecord: %v", err) + } + record.Metadata = map[string]string{ + "install_type": "host", + "issued_via": agentbinding.IssuedViaConfig, + agenttokens.RuntimeRoleMetadataKey: agenttokens.CredentialKindMonitoringCollector, + } + return *record +} + +func issue1753DockerReport(hostname, machineID string) agentsdocker.Report { + return agentsdocker.Report{ + Agent: agentsdocker.AgentInfo{ + // The unified agent's Docker module reports the machine ID as its + // agent ID (the hostagent fallback chain, #985/#986). + ID: machineID, + Version: "6.4.2", + Type: "unified", + IntervalSeconds: 30, + }, + Host: agentsdocker.HostInfo{ + Hostname: hostname, + MachineID: machineID, + Runtime: "docker", + }, + Containers: []agentsdocker.Container{ + {ID: "container-" + machineID, Name: "app", State: "running"}, + }, + Timestamp: time.Now().UTC(), + } +} + +func issue1753PostDockerReport(t *testing.T, router *Router, token string, report agentsdocker.Report) (int, map[string]any) { + t.Helper() + body, err := json.Marshal(report) + if err != nil { + t.Fatalf("marshal docker report: %v", err) + } + req := httptest.NewRequest(http.MethodPost, "/api/agents/docker/report", bytes.NewReader(body)) + req.Header.Set("X-API-Token", token) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + + var payload map[string]any + _ = json.Unmarshal(rec.Body.Bytes(), &payload) + return rec.Code, payload +} + +func issue1753TokenNames(cfg *config.Config) map[string]bool { + config.Mu.RLock() + defer config.Mu.RUnlock() + names := make(map[string]bool, len(cfg.APITokens)) + for _, record := range cfg.APITokens { + names[record.Name] = true + } + return names +} + +// TestIssue1753DockerSharedTokenTwoSitesSameShortHostname models the reporter's +// estate on the Docker module: both sites' unified agents report host and +// Docker inventory with the same short hostname and one shared install token. +// +// The Docker path deliberately requires a unique token per module, so the +// second site's Docker reports must converge to the documented, actionable +// rejection ("generate a new token") - never to a silent fold that leaves one +// record flip-flopping between machines. The first site's Docker identity must +// stay stable throughout, and removing the Docker record must not revoke the +// shared token the host records still authenticate with (the 401 chain from +// the issue). +func TestIssue1753DockerSharedTokenTwoSitesSameShortHostname(t *testing.T) { + const shared = "issue1753-docker-shared.13571357" + + cfg := newTestConfigWithTokens(t, issue1753DockerInstallTokenRecord(t, shared, "shared")) + monitor, err := monitoring.New(cfg) + if err != nil { + t.Fatalf("new monitor: %v", err) + } + defer monitor.Stop() + router := NewRouter(cfg, monitor, nil, nil, func() error { return nil }, "6.4.2") + + // Alternate reports the way two live 30s-interval agents do. Site B's + // very first Docker report may legitimately fold (a changed machine ID is + // indistinguishable from a recreated container until the old machine + // reports again), but from then on it must be rejected with the + // unique-token guidance rather than adopted. + var ackIDA string + var siteBRejected bool + for round := 0; round < 4; round++ { + code, payload := issue1753PostReport(t, router, shared, + issue1753Report("", "docker01", "machine-id-site-a", "10.1.0.10")) + if code != http.StatusOK { + t.Fatalf("round %d: site A host report rejected with %d (%v)", round, code, payload) + } + code, payload = issue1753PostReport(t, router, shared, + issue1753Report("", "docker01", "machine-id-site-b", "10.2.0.10")) + if code != http.StatusOK { + t.Fatalf("round %d: site B host report rejected with %d (%v)", round, code, payload) + } + + code, payload = issue1753PostDockerReport(t, router, shared, + issue1753DockerReport("docker01", "machine-id-site-a")) + if code != http.StatusOK { + t.Fatalf("round %d: site A docker report rejected with %d (%v)", round, code, payload) + } + gotID, _ := payload["agentId"].(string) + if gotID == "" { + t.Fatalf("round %d: site A docker report acknowledged without an identity", round) + } + if ackIDA == "" { + ackIDA = gotID + } else if gotID != ackIDA { + t.Fatalf("round %d: site A docker identity flapped from %q to %q", round, ackIDA, gotID) + } + + code, payload = issue1753PostDockerReport(t, router, shared, + issue1753DockerReport("docker01", "machine-id-site-b")) + switch { + case code == http.StatusOK && round == 0: + // The one ambiguous cycle: nothing distinguishes site B's first + // report from a recreated container yet. + case code == http.StatusOK: + t.Fatalf("round %d: site B's Docker module silently adopted an identity (%v) instead of the unique-token rejection; the record would flip-flop between machines", round, payload) + default: + message, _ := payload["error"].(string) + if !strings.Contains(message, "unique API token") { + t.Fatalf("round %d: site B docker report rejected with %d but without the unique-token guidance: %v", round, code, payload) + } + siteBRejected = true + } + } + if !siteBRejected { + t.Fatal("site B's Docker module was never steered to a unique token") + } + + // Site A's record must have settled on site A's machine, not the last + // reporter's: the flip-flop is what let a later removal act on the wrong + // site in the original estate. + dockerHost, ok := monitor.GetDockerHost(ackIDA) + if !ok { + t.Fatalf("site A docker host %q missing from state", ackIDA) + } + if dockerHost.MachineID != "machine-id-site-a" { + t.Fatalf("site A docker record carries machine ID %q, want %q", dockerHost.MachineID, "machine-id-site-a") + } + + // The reporter's removal step: the Docker record is removed while both + // sites' host records still authenticate with the shared token. The token + // must survive, and host reporting must keep working - its revocation is + // the 401 "Unauthorized access attempt" chain from the issue. + if _, err := monitor.RemoveDockerHost(ackIDA); err != nil { + t.Fatalf("RemoveDockerHost(%q): %v", ackIDA, err) + } + + code, payload := issue1753PostReport(t, router, shared, + issue1753Report("", "docker01", "machine-id-site-a", "10.1.0.10")) + if code != http.StatusOK { + t.Fatalf("site A host report rejected with %d after Docker host removal (%v)", code, payload) + } + code, payload = issue1753PostReport(t, router, shared, + issue1753Report("", "docker01", "machine-id-site-b", "10.2.0.10")) + if code != http.StatusOK { + t.Fatalf("site B host report rejected with %d after Docker host removal (%v)", code, payload) + } + + if names := issue1753TokenNames(cfg); !names["Unified agent install (shared)"] { + t.Fatalf("shared install token was revoked while both host records still use it (have %v)", names) + } +} + +// TestIssue1753DockerHostRemovalKeepsUnifiedSiblingToken covers the same +// revocation chain without any hostname collision: a single unified install +// shares one token between the machine's host record and its Docker record. +// Removing only the Docker host (stop monitoring Docker, keep the host) must +// not revoke the token the host module still reports with. +func TestIssue1753DockerHostRemovalKeepsUnifiedSiblingToken(t *testing.T) { + const token = "issue1753-unified-single.24682468" + + cfg := newTestConfigWithTokens(t, issue1753DockerInstallTokenRecord(t, token, "single")) + monitor, err := monitoring.New(cfg) + if err != nil { + t.Fatalf("new monitor: %v", err) + } + defer monitor.Stop() + router := NewRouter(cfg, monitor, nil, nil, func() error { return nil }, "6.4.2") + + code, payload := issue1753PostReport(t, router, token, + issue1753Report("", "nas01", "machine-id-nas01", "10.3.0.10")) + if code != http.StatusOK { + t.Fatalf("host report rejected with %d (%v)", code, payload) + } + + code, payload = issue1753PostDockerReport(t, router, token, + issue1753DockerReport("nas01", "machine-id-nas01")) + if code != http.StatusOK { + t.Fatalf("docker report rejected with %d (%v)", code, payload) + } + dockerID, _ := payload["agentId"].(string) + if dockerID == "" { + t.Fatal("expected an acknowledged Docker host identity") + } + + if _, err := monitor.RemoveDockerHost(dockerID); err != nil { + t.Fatalf("RemoveDockerHost(%q): %v", dockerID, err) + } + + code, payload = issue1753PostReport(t, router, token, + issue1753Report("", "nas01", "machine-id-nas01", "10.3.0.10")) + if code != http.StatusOK { + t.Fatalf("host report rejected with %d after Docker host removal (%v); the shared unified token must survive", code, payload) + } + + if names := issue1753TokenNames(cfg); !names["Unified agent install (single)"] { + t.Fatalf("unified install token was revoked while the host record still uses it (have %v)", names) + } +} diff --git a/internal/monitoring/docker_host_identity.go b/internal/monitoring/docker_host_identity.go index d903b4cf9..d7e2954ba 100644 --- a/internal/monitoring/docker_host_identity.go +++ b/internal/monitoring/docker_host_identity.go @@ -31,8 +31,11 @@ func tokenHintFromRecord(record *config.APITokenRecord) string { // resolveDockerHostIdentifier determines a unique identifier for a Docker host // based on its report and existing hosts. Returns the identifier, fallback identifiers, -// the existing host (if matched), and whether a match was found. -func resolveDockerHostIdentifier(report agentsdocker.Report, tokenRecord *config.APITokenRecord, hosts []*unifiedresources.DockerHostView) (string, []string, *unifiedresources.DockerHostView, bool) { +// the existing host (if matched), and whether a match was found. machineRevisit +// (optional) reports whether folding the report's machine ID into a candidate +// identity would alternate back to a machine already seen behind it; see +// findMatchingDockerHost. +func resolveDockerHostIdentifier(report agentsdocker.Report, tokenRecord *config.APITokenRecord, hosts []*unifiedresources.DockerHostView, machineRevisit func(identifier, machineID string) bool) (string, []string, *unifiedresources.DockerHostView, bool) { base := strings.TrimSpace(report.AgentKey()) fallbacks := uniqueNonEmptyStrings( base, @@ -41,7 +44,7 @@ func resolveDockerHostIdentifier(report agentsdocker.Report, tokenRecord *config strings.TrimSpace(report.Host.Hostname), ) - if existing, ok := findMatchingDockerHost(hosts, report, tokenRecord); ok { + if existing, ok := findMatchingDockerHost(hosts, report, tokenRecord, machineRevisit); ok { return dockerHostStableID(existing), fallbacks, existing, true } @@ -70,7 +73,18 @@ func resolveDockerHostIdentifier(report agentsdocker.Report, tokenRecord *config } // findMatchingDockerHost searches for an existing host that matches the report. -func findMatchingDockerHost(hosts []*unifiedresources.DockerHostView, report agentsdocker.Report, tokenRecord *config.APITokenRecord) (*unifiedresources.DockerHostView, bool) { +// +// The hostname-based fallbacks deliberately fold a report whose machine ID +// disagrees with the candidate record: containerized agents regenerate +// /etc/machine-id on recreation and must keep their identity. But two live +// machines reusing one short hostname and one shared install token also land +// in those fallbacks, and folding them collapses two sites into one +// flip-flopping record (the Docker analog of #1753). machineRevisit is the +// discriminator: a recreated container transitions to its new machine ID +// exactly once, while two live machines alternate, so a report whose machine +// ID *returns* to a value already seen behind the candidate identity is proof +// of a second machine and must not be folded. +func findMatchingDockerHost(hosts []*unifiedresources.DockerHostView, report agentsdocker.Report, tokenRecord *config.APITokenRecord, machineRevisit func(identifier, machineID string) bool) (*unifiedresources.DockerHostView, bool) { agentID := strings.TrimSpace(report.Agent.ID) tokenID := "" if tokenRecord != nil { @@ -119,6 +133,9 @@ func findMatchingDockerHost(hosts []*unifiedresources.DockerHostView, report age } if unifiedresources.HostnamesEquivalent(host.Hostname(), hostname) && strings.TrimSpace(host.TokenID()) == tokenID { + if dockerHostMachineIDRevisits(host, agentID, machineID, machineRevisit) { + continue + } return host, true } } @@ -142,6 +159,9 @@ func findMatchingDockerHost(hosts []*unifiedresources.DockerHostView, report age } if unifiedresources.HostnamesEquivalent(host.Hostname(), hostname) && strings.TrimSpace(host.TokenID()) == "" { + if dockerHostMachineIDRevisits(host, agentID, machineID, machineRevisit) { + continue + } return host, true } } @@ -150,6 +170,31 @@ func findMatchingDockerHost(hosts []*unifiedresources.DockerHostView, report age return nil, false } +// dockerHostMachineIDRevisits reports whether folding a report with the given +// machine ID into the candidate host would return the record to a machine +// identity it has already alternated away from - two live machines behind one +// hostname, not one recreated container. See findMatchingDockerHost. +// +// A record whose stable ID is derived from the report's own machine or agent +// ID belongs to the reporting machine: the returning owner reclaims it, and +// the *other* machine is the one that splits off to its own identity. This +// keeps each site on the record its identifiers minted, so removing one +// site's record never blocks the sibling through a shared alias. +func dockerHostMachineIDRevisits(host *unifiedresources.DockerHostView, agentID, machineID string, machineRevisit func(identifier, machineID string) bool) bool { + if machineRevisit == nil || host == nil || machineID == "" { + return false + } + existingMachineID := strings.TrimSpace(host.MachineID()) + if existingMachineID == "" || existingMachineID == machineID { + return false + } + stableID := dockerHostStableID(host) + if stableID != "" && (stableID == machineID || (agentID != "" && stableID == agentID)) { + return false + } + return machineRevisit(stableID, machineID) +} + // dockerHostIdentityConflicts reports whether an incoming report is clearly from // a different physical/swarm node than the existing Docker host record, so a // shared token+agentID reused from another node is not collapsed into one host. diff --git a/internal/monitoring/docker_host_identity_test.go b/internal/monitoring/docker_host_identity_test.go index 301ec8627..3a8afef97 100644 --- a/internal/monitoring/docker_host_identity_test.go +++ b/internal/monitoring/docker_host_identity_test.go @@ -673,7 +673,7 @@ func TestFindMatchingDockerHost(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - result, found := findMatchingDockerHost(dockerHostViewsForTest(tt.hosts), tt.report, tt.tokenRecord) + result, found := findMatchingDockerHost(dockerHostViewsForTest(tt.hosts), tt.report, tt.tokenRecord, nil) if found != tt.expectMatch { t.Errorf("found mismatch: got %v, want %v", found, tt.expectMatch) } @@ -902,7 +902,7 @@ func TestResolveDockerHostIdentifier(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - id, fallbacks, existing, found := resolveDockerHostIdentifier(tt.report, tt.tokenRecord, dockerHostViewsForTest(tt.hosts)) + id, fallbacks, existing, found := resolveDockerHostIdentifier(tt.report, tt.tokenRecord, dockerHostViewsForTest(tt.hosts), nil) if found != tt.expectMatch { t.Errorf("found mismatch: got %v, want %v", found, tt.expectMatch) } @@ -973,7 +973,7 @@ func TestFindMatchingDockerHost_RejectsConflictingPhysicalIdentity(t *testing.T) Host: agentsdocker.HostInfo{MachineID: "machine-2", Hostname: "hostname-2"}, } - if _, ok := findMatchingDockerHost(dockerHostViewsForTest(hosts), report, &config.APITokenRecord{ID: "token-1"}); ok { + if _, ok := findMatchingDockerHost(dockerHostViewsForTest(hosts), report, &config.APITokenRecord{ID: "token-1"}, nil); ok { t.Fatal("expected no match when the report's physical identity conflicts with the existing host") } } diff --git a/internal/monitoring/docker_shared_token_test.go b/internal/monitoring/docker_shared_token_test.go new file mode 100644 index 000000000..45a98e87a --- /dev/null +++ b/internal/monitoring/docker_shared_token_test.go @@ -0,0 +1,99 @@ +package monitoring + +import ( + "strings" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/config" + agentsdocker "github.com/rcourtman/pulse-go-rewrite/pkg/agents/docker" +) + +func sharedTokenSiteReport(machineID string, at time.Time) agentsdocker.Report { + return agentsdocker.Report{ + Agent: agentsdocker.AgentInfo{ + // The unified agent's Docker module reports the machine ID as its + // agent ID (the hostagent fallback chain, #985/#986). + ID: machineID, + Version: "6.4.2", + Type: "unified", + IntervalSeconds: 30, + }, + Host: agentsdocker.HostInfo{ + Hostname: "docker01", + MachineID: machineID, + }, + Timestamp: at, + } +} + +// Docker analog of #1753: two live machines reuse one short hostname and one +// shared install token. The hostname+token fallback may fold the second +// machine's first report (indistinguishable from a recreated container until +// the first machine reports again), but the identities must not keep +// flip-flopping: the machine that minted the record reclaims it, and the other +// machine converges to the documented unique-token rejection instead of +// silently overwriting the record every cycle. +func TestApplyDockerReportSharedTokenSameHostnameDoesNotFlipFlop(t *testing.T) { + monitor := newTestMonitor(t) + token := &config.APITokenRecord{ID: "shared-install-token", Name: "Shared install"} + base := time.Now().UTC() + + siteA, err := monitor.ApplyDockerReport(sharedTokenSiteReport("machine-id-site-a", base), token) + if err != nil { + t.Fatalf("site A first report: %v", err) + } + + // Site B's first report is the one ambiguous cycle and may fold into + // site A's record. + if _, err := monitor.ApplyDockerReport(sharedTokenSiteReport("machine-id-site-b", base.Add(time.Second)), token); err != nil { + t.Logf("site B first report rejected immediately: %v", err) + } + + // Site A's next report proves two live machines are alternating: it must + // reclaim its own record, not mint a new identity. + reclaimed, err := monitor.ApplyDockerReport(sharedTokenSiteReport("machine-id-site-a", base.Add(2*time.Second)), token) + if err != nil { + t.Fatalf("site A reclaim report: %v", err) + } + if reclaimed.ID != siteA.ID { + t.Fatalf("site A lost its record: acknowledged %q, want %q", reclaimed.ID, siteA.ID) + } + if reclaimed.MachineID != "machine-id-site-a" { + t.Fatalf("site A record carries machine ID %q after reclaim", reclaimed.MachineID) + } + + // From here on, site B must be steered to a unique token, and site A must + // stay stable - the silent alternation is what collapsed the reporter's + // estate and let a removal revoke the shared credential. + for cycle := 0; cycle < 3; cycle++ { + at := base.Add(time.Duration(3+cycle*2) * time.Second) + _, err := monitor.ApplyDockerReport(sharedTokenSiteReport("machine-id-site-b", at), token) + if err == nil { + t.Fatalf("cycle %d: site B silently adopted an identity instead of the unique-token rejection", cycle) + } + if !strings.Contains(err.Error(), "unique API token") { + t.Fatalf("cycle %d: site B rejection lacks the unique-token guidance: %v", cycle, err) + } + + steady, err := monitor.ApplyDockerReport(sharedTokenSiteReport("machine-id-site-a", at.Add(time.Second)), token) + if err != nil { + t.Fatalf("cycle %d: site A report rejected: %v", cycle, err) + } + if steady.ID != siteA.ID || steady.MachineID != "machine-id-site-a" { + t.Fatalf("cycle %d: site A flapped to ID %q machine %q", cycle, steady.ID, steady.MachineID) + } + } + + hosts := monitor.state.GetDockerHosts() + if len(hosts) != 1 { + ids := make([]string, 0, len(hosts)) + for _, h := range hosts { + ids = append(ids, h.ID+"@"+h.MachineID) + } + t.Fatalf("expected exactly site A's record to survive, got %d: %v", len(hosts), ids) + } + if hosts[0].ID != siteA.ID || hosts[0].MachineID != "machine-id-site-a" { + t.Fatalf("surviving record is %q@%q, want %q@machine-id-site-a", hosts[0].ID, hosts[0].MachineID, siteA.ID) + } +} diff --git a/internal/monitoring/identity_flap_tracker.go b/internal/monitoring/identity_flap_tracker.go index c12653d4e..1846145f8 100644 --- a/internal/monitoring/identity_flap_tracker.go +++ b/internal/monitoring/identity_flap_tracker.go @@ -102,6 +102,39 @@ func (t *identityFlapTracker) observe(hostname, secondary string, now time.Time) return conflict } +// secondaryRevisit reports whether observing the given secondary value now +// would be a revisit: the value was already seen inside the window and the +// stream has since moved to a different value. This is the read-only +// counterpart of observe for callers that must decide identity adoption +// before the report is recorded. +func (t *identityFlapTracker) secondaryRevisit(secondary string, now time.Time) bool { + secondary = strings.TrimSpace(secondary) + if secondary == "" || t.lastSecondary == "" || t.lastSecondary == secondary { + return false + } + seenAt, seen := t.secondaries[secondary] + return seen && !seenAt.Before(now.Add(-t.window)) +} + +// dockerMachineIDRevisit reports whether folding a report with this machine ID +// into the given Docker host identity would alternate back to a machine the +// identity has already been seen on: proof that two live machines are behind +// one record (the Docker analog of #1753), as opposed to one recreated +// container whose machine ID changed exactly once and never returns. Callers +// must not hold m.mu. +func (m *Monitor) dockerMachineIDRevisit(identifier, machineID string, now time.Time) bool { + if strings.TrimSpace(identifier) == "" { + return false + } + m.mu.RLock() + defer m.mu.RUnlock() + tracker, ok := m.dockerIdentityFlaps[identifier] + if !ok { + return false + } + return tracker.secondaryRevisit(machineID, now) +} + // observeIdentityFlap feeds one report's identity fields into the flap // tracker for the resolved identifier, lazily creating the tracker map and // entry, and returns the active conflict, if any. Callers must not hold m.mu. diff --git a/internal/monitoring/monitor_agents.go b/internal/monitoring/monitor_agents.go index faf036c2a..1570932ec 100644 --- a/internal/monitoring/monitor_agents.go +++ b/internal/monitoring/monitor_agents.go @@ -346,13 +346,49 @@ func (m *Monitor) RemoveDockerHost(hostID string) (models.DockerHost, error) { } } - // Revoke the API token associated with this Docker host + // Revoke the API token associated with this Docker host - unless a sibling + // record still authenticates with it: a unified install shares one token + // between the machine's host record and its Docker record, and two sites + // reusing one pasted install token hold separate records on one credential + // (the Docker analog of #1753). Revoking here would reject every surviving + // agent's next report with 401. if host.TokenID != "" { - tokenRemoved, err := m.revokeAPIToken(host.TokenID) - if err != nil { - log.Warn().Err(err).Str("tokenID", host.TokenID).Msg("API token revocation rolled back after Docker host removal") - } else if tokenRemoved != nil { - log.Info().Str("tokenID", host.TokenID).Str("tokenName", host.TokenName).Msg("API token revoked for removed Docker host") + tokenID := strings.TrimSpace(host.TokenID) + tokenStillUsed := false + if readState := m.snapshotBackedUnifiedReadState(); readState != nil { + for _, other := range readState.Hosts() { + if other != nil && strings.TrimSpace(other.TokenID()) == tokenID { + tokenStillUsed = true + break + } + } + if !tokenStillUsed { + for _, other := range readState.DockerHosts() { + if other == nil { + continue + } + if dockerHostStableID(other) == hostID || strings.TrimSpace(other.ID()) == hostID { + continue + } + if strings.TrimSpace(other.TokenID()) == tokenID { + tokenStillUsed = true + break + } + } + } + } + if !tokenStillUsed { + tokenRemoved, err := m.revokeAPIToken(tokenID) + if err != nil { + log.Warn().Err(err).Str("tokenID", tokenID).Msg("API token revocation rolled back after Docker host removal") + } else if tokenRemoved != nil { + log.Info().Str("tokenID", tokenID).Str("tokenName", host.TokenName).Msg("API token revoked for removed Docker host") + } + } else { + log.Info(). + Str("tokenID", tokenID). + Str("dockerHostID", hostID). + Msg("API token still used by other agents; skipping revocation during Docker host removal") } } @@ -1932,7 +1968,10 @@ func (m *Monitor) ApplyDockerReport(report agentsdocker.Report, tokenRecord *con if readState != nil { dockerHosts = readState.DockerHosts() } - identifier, legacyIDs, previous, hasPrevious := resolveDockerHostIdentifier(report, tokenRecord, dockerHosts) + machineRevisit := func(identifier, machineID string) bool { + return m.dockerMachineIDRevisit(identifier, machineID, time.Now()) + } + identifier, legacyIDs, previous, hasPrevious := resolveDockerHostIdentifier(report, tokenRecord, dockerHosts, machineRevisit) if strings.TrimSpace(identifier) == "" { return models.DockerHost{}, fmt.Errorf("docker report missing agent identifier") }