amadan
edge: rotate/detach contract pinned — retry failed reloads, doc the session asymmetry
Two Important review fixes: - SetDynamicAuth's doc comment now states the rotate/detach asymmetry explicitly: rotation refuses NEW dials with the old token but leaves an already-live session running (closing it would take the box down for the registry-write-to-box-learns-new-token gap); detach is the hard revocation that force-closes a live channel immediately. The code was already correct (CloseChannel only fires for names missing from the new set, not names present under a changed hash) — this fixes the comment to say so. - Watch no longer baselines `last = fp` before Reload succeeds: a detach is a revocation, and discarding a failed reload's fingerprint used to make the change invisible to every later tick until some unrelated write moved the fingerprint again. `last` now only advances after a successful Reload, so a failing tick retries on the next one; reload failures are throttled the same errs%300 way poll failures already were. Plus the pinning test the review asked for: TestRotateVsDetachAsymmetry rotates a token and asserts a new dial with the old token is refused while the pre-existing live channel keeps running, then detaches the same box and asserts that live channel IS closed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 files changed,
+121
−14
internal/edge/edge.go+15 −2internal/edge/tunnel.go+26 −12internal/edge/tunnel_test.go+80 −0
diff --git a/internal/edge/edge.go b/internal/edge/edge.go| index 8f30a9973..53f2f00dd 100644 |
| --- a/internal/edge/edge.go |
| +++ b/internal/edge/edge.go |
| @@ -852,6 +852,7 @@ func (e *Edge) Watch(stop <-chan struct{}) { |
| // that gap; one redundant reload at startup is the cost of closing it.) |
| var last string |
| errs := 0 |
| + reloadErrs := 0 |
| tick := time.NewTicker(2 * time.Second) |
| defer tick.Stop() |
| for { |
| @@ -872,11 +873,23 @@ func (e *Edge) Watch(stop <-chan struct{}) { |
| if fp == last { |
| continue |
| } |
| - last = fp |
| + // last is baselined only on a SUCCESSFUL reload: a fleet detach is a |
| + // revocation, and if Reload fails on this tick, the fingerprint it |
| + // was meant to apply must not be discarded — baselining it here |
| + // (before Reload runs) would make the change invisible to every |
| + // later tick until some UNRELATED write moves the fingerprint again, |
| + // silently sitting on a revocation that never took effect. Leaving |
| + // last unchanged means the very next tick sees the same fp != last |
| + // and retries. |
| if err := e.Reload(); err != nil { |
| - log.Printf("edge: registry-watch reload failed: %v", err) |
| + reloadErrs++ |
| + if reloadErrs == 1 || reloadErrs%300 == 0 { |
| + log.Printf("edge: registry-watch reload failed (#%d): %v", reloadErrs, err) |
| + } |
| continue |
| } |
| + reloadErrs = 0 |
| + last = fp |
| log.Printf("edge: registry changed — reloaded (%d route(s))", len(e.current())) |
| } |
| } |
diff --git a/internal/edge/tunnel.go b/internal/edge/tunnel.go| index 8730105fd..d73f4eb50 100644 |
| --- a/internal/edge/tunnel.go |
| +++ b/internal/edge/tunnel.go |
| @@ -222,19 +222,33 @@ func (h *TunnelHub) pingLoop(name string, cc *http2.ClientConn, raw net.Conn) { |
| // SetDynamicAuth replaces the entire registry-backed credential set — |
| // channel "<fleet>/<label>" -> sha256 hex of its bearer token — with auth. |
| -// The whole-map REPLACE (not an incremental merge) is what makes a rotated |
| -// or detached box stop authenticating the instant this returns: the next |
| -// ServeHTTP call for a vanished name simply misses the map. Called by |
| -// Edge.Reload on every registry poll (edge.go), so this is the whole |
| -// rotate/detach convergence path — no separate polling of its own. |
| +// The whole-map REPLACE (not an incremental merge) is what makes a NEW dial |
| +// with a stale token get refused the instant this returns, for both |
| +// rotation and detach: the next ServeHTTP call either hashes the presented |
| +// token against a DIFFERENT stored hash (rotation: the name is still |
| +// present, just under a new hash) or misses the map entirely (detach: the |
| +// name is gone). Either way, a new dial with the old token fails now. |
| // |
| -// A name present in the OLD set but missing from auth has just been |
| -// revoked; any conn already established under its (now-invalid) token must |
| -// be torn down too, or an already-authenticated h2 session would keep |
| -// carrying traffic past the revocation until its next failed ping |
| -// (tunnelPingEvery, up to 15s) instead of closing on this call. CloseChannel |
| -// is called for each such name AFTER the map swap, so the swap (which |
| -// blocks any NEW dial with the old token) always lands first. |
| +// ROTATE and DETACH are deliberately asymmetric on an ALREADY-LIVE session, |
| +// and that asymmetry is load-bearing, not an oversight: |
| +// |
| +// - Detach: the name vanishes from auth entirely, so it is in the OLD set |
| +// but missing from the new one — the loop below calls CloseChannel on |
| +// it, force-closing any live conn right now. Detach is the hard |
| +// revocation path; a box taken out of the fleet must not keep a |
| +// channel open on its old credential for even one more request. |
| +// - Rotate: the name STAYS present in auth, just under a new hash — the |
| +// loop below sees it as still-authorized (present in both old and new) |
| +// and does NOT close it. An already-established h2 session therefore |
| +// keeps carrying traffic on its OLD token's connection until the |
| +// legitimate box notices (its own ping/reconnect cycle) and re-dials |
| +// with the NEW token — not until this call returns. This is |
| +// deliberate: closing on every rotation would take the box's channel |
| +// down for the gap between the registry write and the box learning its |
| +// new token, and rotation (unlike detach) is not "this box must be cut |
| +// off now" — it is "this box's credential changed." A caller that wants |
| +// rotation to also force an immediate reconnect must close explicitly |
| +// (CloseChannel) rather than relying on SetDynamicAuth to do it. |
| func (h *TunnelHub) SetDynamicAuth(auth map[string]string) { |
| h.mu.Lock() |
| old := h.dynamicAuth |
diff --git a/internal/edge/tunnel_test.go b/internal/edge/tunnel_test.go| index b2d90a116..fdc017273 100644 |
| --- a/internal/edge/tunnel_test.go |
| +++ b/internal/edge/tunnel_test.go |
| @@ -515,6 +515,86 @@ func TestReloadDetachedFleetBoxClosesLiveChannel(t *testing.T) { |
| } |
| } |
| +// TestRotateVsDetachAsymmetry pins the deliberate difference between the two |
| +// registry-driven revocation paths, through the real Reload wiring: |
| +// |
| +// - Rotate (SetFleetBoxToken, same box still active): a NEW dial with the |
| +// OLD token is refused immediately, but an ALREADY-LIVE channel is left |
| +// alone — closing it would take the box's connection down for the gap |
| +// between the registry write and the box itself learning its new token. |
| +// - Detach (SetFleetBoxStatus "detached"): the hard revocation — an |
| +// ALREADY-LIVE channel is force-closed on the very next Reload. |
| +func TestRotateVsDetachAsymmetry(t *testing.T) { |
| + reg := openReg(t) |
| + if err := reg.UpsertFleet(registry.Fleet{Name: "acme", Account: "amadan", Bucket: "acme-data"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + oldHash := sha256Hex("old-tok") |
| + if err := reg.AddFleetBox(registry.FleetBox{Fleet: "acme", Label: "pi-1", TokenHash: oldHash, Status: "active"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + e, err := New(reg, nil) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + hub := NewTunnelHub("tunnel.test", nil) |
| + e.EnableTunnels(hub) |
| + if err := e.Reload(); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + // A live channel, as if pi-1 had already dialed in on the old token. |
| + c1, c2 := net.Pipe() |
| + defer c2.Close() |
| + hub.mu.Lock() |
| + hub.conns["acme/pi-1"] = &tunnelConn{raw: c1} |
| + hub.mu.Unlock() |
| + |
| + // --- Rotate --- |
| + newHash := sha256Hex("new-tok") |
| + if err := reg.SetFleetBoxToken("acme", "pi-1", newHash); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := e.Reload(); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + // (a) A NEW dial with the OLD token is refused now. |
| + rec := httptest.NewRecorder() |
| + hub.ServeHTTP(rec, tunnelReq("acme/pi-1", "old-tok")) |
| + if rec.Code != http.StatusUnauthorized { |
| + t.Fatalf("rotate: new dial with old token: got %d, want 401", rec.Code) |
| + } |
| + // A NEW dial with the NEW token is accepted. |
| + rec2 := httptest.NewRecorder() |
| + hub.ServeHTTP(rec2, tunnelReq("acme/pi-1", "new-tok")) |
| + if rec2.Code != http.StatusInternalServerError { |
| + t.Fatalf("rotate: new dial with new token: got %d, want 500 (past auth)", rec2.Code) |
| + } |
| + // (b) The EXISTING live channel from before the rotation is untouched — |
| + // that's the designed asymmetry, not a bug. |
| + hub.mu.Lock() |
| + c, stillLive := hub.conns["acme/pi-1"] |
| + hub.mu.Unlock() |
| + if !stillLive || c.raw != c1 { |
| + t.Fatal("rotate: the pre-existing live channel was closed — rotation must not evict an established session") |
| + } |
| + |
| + // --- Detach --- |
| + if err := reg.SetFleetBoxStatus("acme", "pi-1", "detached"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := e.Reload(); err != nil { |
| + t.Fatal(err) |
| + } |
| + hub.mu.Lock() |
| + _, stillThere := hub.conns["acme/pi-1"] |
| + hub.mu.Unlock() |
| + if stillThere { |
| + t.Fatal("detach: the live channel from before detach was not closed — detach must be a hard revocation") |
| + } |
| +} |
| + |
| // A tunnel-served host is never activator-managed: the process lives on the |
| // far side of the channel, so a wake here would start a second, local copy. |
| // The retire pass must also drop it, killing the child a repointed route left |