A fleet reconnecting at once ran every connect setup concurrently, each
building links and validating terminators, with nothing bounding how many were
in flight. The work itself is sub-second; the failure is in the aggregate, and
nothing reported it, so a controller burying itself under reconnects looked
from the outside like one that had simply gone quiet.
- splits the connect into the decision and the setup: the decision stays synchronous, so the bind handler can still fail a refused connect and at most one connection per router is current at a time
- runs the setup on a goroutine bounded by a semaphore, defaulting to 200 at once, configurable as routerConnectConcurrency
- never refuses for capacity: a refusal fails the bind and strands the router's channel group, and the expensive part of connecting is already paid by the time this is reached, so the setup waits instead
- re-acquires the per-router lock and re-checks currency inside the setup, so it is serialized against a concurrent disconnect and skipped entirely if the connection was superseded while queued
- gives up after a configurable timeout and closes the control channel rather than skipping the setup, since skipping leaves a router that is connected, believes it is, and has no links built
- reports the queue depth as a gauge and the wait and the work as separate timers, since the two have opposite remedies and the timers record only on completion, so the gauge is the one signal visible during a wedge
The scan walks the link table and removes from it in a second pass, and on a
large table the gap between them is wide enough for a dialer to reconnect and
re-announce its links. Acting on the first pass's answer removes a link the
controller has just been told about, and a router announces its set only on
reconnect, so it does not say so again until it next connects. A soak run lost
53 links this way.
- re-tests each candidate immediately before removing it, and skips one whose dialer returned mid-scan
- takes a keep predicate, so a caller can spare links whose dialer does not re-announce them
- records whether a retired link's dialer is connected under a different object, which distinguishes a router that is genuinely away from a link still pointing at a connection that has been replaced; the second is a stale pointer rather than a dead dialer and wants a different fix
Controllers compute peer state independently and nothing orders their messages
against each other, so a router could be told a destination was removed and
then told by another controller, from state computed before the removal, that
it was healthy again. The destination came back and the router went on dialing
a router that was gone.
- carries the controller store index a peer state was computed against, and zero where the reporter has none to offer
- remembers a removal against its index, so a report computed against an older one cannot recreate the destination
- loses a tie: a router being deleted stays in a controller's connected map until its control channel teardown runs, so the same store state can still produce a healthy report
- suppresses only existence, never health, since health is each controller's own view of what it is connected to and a controller lagging in replication reports a low index alongside a perfectly good health report
- names the reporting controller on each update, so the registry can union health across controllers while ordering existence by index
- forgets a removal after a few minutes, which outlives any report already in flight
A link a router reported without re-announcing is known only from that report,
under an id the router will never mention again, so its disconnect is the only
removal the link will ever get. A router that flags its reconnect announcement
a full refresh is the opposite case: it states authoritatively which links it
still has and the controller prunes the rest against that.
- retires a disconnecting router's links only where that router does not prune its own
- leaves the links of a router that does prune them, since the announcement is a bounded best-effort send and an outage outliving its retries would otherwise leave this controller holding no links for a router that is up
- records on the router whether it announces a full refresh, taken from what it said on connect
A fleet that churns routers accumulated links to routers that were not there.
The cleanup dropped the deleted router's index but left the links themselves in
the link table and in the surviving endpoint's set, where nothing would remove
them: a deleted router's id never returns, since a recreated router is assigned
a new one, so nothing later could clear them. Each one still counted, still
validated and still offered itself for routing.
- retires every link with the deleted router at either end, dialed or accepted, since the index is keyed on both
- retires the way a fault does rather than merely deleting, so circuits using the link are rerouted and the removal reaches the event stream
- retires before the index is dropped, since retirement reads that index to find what to retire
- retires by id without checking whether the id has come back, which needs a router created with a chosen id and a delete and recreate inside the commit-handler window; guarding it is not worth the machinery
Peer state is only recomputed when a router connects or disconnects, and a
deleted router does neither again, so a removal a router misses is never
repeated and it goes on dialing the dead destination. A link report naming the
destination is the evidence that it was missed, and reports repeat, so the
correction can repeat with them.
- re-asserts a removal to the router that named it, queued against that router alone rather than fanned out to every connected one
- computes the state at send time, so a destination that turns out to exist after all is reported as it is rather than as removed
- tells every router that names the destination, whatever it advertised, since the statement is true regardless of who hears it
- answers only from the leader, since this is an unsolicited assertion that tears links down, and a controller lagging on a create would otherwise tell a router to drop a destination that is fine
A nil destination is ordinary: the link is recorded and repaired when that
router connects. It also covers a router that was deleted, and that link can
never be repaired, since a recreated router is assigned a new id. Left alone it
is a link to an entity that does not exist: it counts, it validates, it offers
itself for routing, and it cannot be rendered by the API, which fails the whole
listing rather than one row.
- refuses a report whose destination is neither connected here nor present in the store, which is what stops the dialers re-announcing it back into existence on every reconnect
- answers the lookup from the router cache, falling through to the store on a miss
- treats a missing lookup as existence, so a report is never refused on the strength of one
- gives the two tests that reported a link to an unpersisted destination a destination that exists
A controller or router that is starved shows up as slow handlers, timeouts and
back-pressure, all of which look like faults in whatever happened to be running
at the time. Nothing in the metrics said whether the host itself had capacity, so
telling a slow component from a slow machine meant going to the host.
- reports host CPU and memory usage through the existing metrics registry, on
both the controller and the router
- carries them on the metrics message so they arrive with everything else rather
than needing a separate collection path
- is off unless enabled, since the collection reads the OS on an interval
Unrelated to gossip; it was committed as part of that work and is separated here.
(cherry picked from commit bdab4b4aba)
- adds a structured xlink.LinkKey ({DialerBinding, Protocol, DestId,
ListenerBinding}) used by all staleness and GC checks, replacing the
pre-rendered key string each side used to pass around
- adds the CheckStaleLinks ctrl message and the `ziti ops verify stale-links`
CLI for operator-driven, two-sided staleness verification with optional --gc,
which acts only when both endpoints agree
- implements auto-GC via the router.link.v1 gcMode (preserve/orphaned/changed):
the router walks its xlinks after each Apply that mutates listeners, dialers,
or gcMode and closes one-sided-stale entries under the configured mode
- sweeps a removal under orphaned whatever the removed config said, including
preserve, since a removal leaves no listeners and no dialers and every
remaining link is one the router can never re-establish
- reports staleness as a three-state verdict (unknown/notStale/stale) so a side
that cannot judge a link abstains instead of guessing; a destination absent
from the registry's listener snapshot means "listeners unknown", not "peer has
none", which keeps a briefly unhealthy peer from losing its live links
- rebuilds the link surface only when a listener or dialer definition actually
moves, so a gcMode-only change no longer rebinds every listen socket
- gates CheckStaleLinks on a new RouterStaleLinkCheck capability and explains
each unqueryable endpoint, including one left unasked by the filter, so a
partial verdict says which of the three causes applies
- detects a peer re-advertising the address a link was dialed to from the
dialing side, not just from the peer's own listener side
- pins every verdict to the link iteration it was computed against, faults that
iteration rather than the current one, and takes the conditional removal's
result as the decision, so a link re-dialed while the sweep was running is
neither closed nor reported as collected
- resolves which side a report speaks for from the sending router against the
controller's link map rather than from the side the report names, so one
router cannot fill both sides and satisfy the both-agree rule --gc depends on
- bounds the query fan-out with a semaphore, matching the other validation
sweeps, since each query carries a minute-long reply timeout
- fails the CLI when the result stream ends before every link is reported, and
drains results already buffered before declaring any missing
- rejects an unrecognized match mode rather than resolving it to changed, the
broadest removal criterion
- consolidates the controller-managed router configuration changelog into one
section, replacing the status that said routers do not yet consume configs at
runtime, and documents the per-config-type opt-in
- restores the Current Beta Features section, dropped when the 2.0 changelog was
archived, and lists controller-managed router configuration in it
- adds unparseable to the codespell ignore list, a spelling variant the
dictionary flags as a typo
For #3743.
BuildRouterLinks runs once per router connect and scanned the whole link table to
find the connecting router's links. The table is proportional to the square of the
router count in a full mesh, so a wave in which every router reconnects walks the
cube of it in link visits. That never shows in steady state; it arrives during a
mass reconnect, when the connect path is already the bottleneck.
Indexing every link under both endpoint router ids fixes the cost, and introduces
the problem that the table and the index are two structures written in sequence.
Three things follow from that, and the destination repair below turned out to
depend on the connect path's ordering as well.
Performance:
- indexes every link under both endpoint router ids, maintained in the add and
remove paths all link mutations funnel through, so a connect costs its own
router's link count rather than the whole table
- prunes a full refresh against the reporting router's own links. That report is
what every reconnect sends, and it walked the whole table to find the router's
stale links, so the cost the connect path shed was still being paid one message
later
- indexes link ids rather than link pointers, so a per-router query resolves each
id against the table and hands back whatever it currently holds. A reconnect
replaces a link's Router object and a higher iteration replaces the Link object,
but neither replaces the id, which is exactly why the scan existed
- holds each router's link ids in a LockedSet rather than a sharded map, which
cost a map and a mutex per shard for an index that is usually empty and is held
for as long as the router has existed: 2.9KB per router id against 150 bytes
- drops a router's index when the router is deleted, guarded on the store still
having no router under that id and checked inside the index map's removal
callback. A router id can come back, since fabric router ids are the enrollment
certificate's common name, and nothing orders the cleanup against that: store
event handlers run after bolt has released the writer lock, so one can execute
after the id was recreated, connected and reported links. Indexing a link takes
the same shard as the guarded read, so an entry for a live router cannot appear
without its create having committed first, which the read then sees, and an entry
arriving later blocks and lands in a freshly created index
- keeps an index when the store cannot be read, since that is not evidence the
router is gone, and dropping one otherwise could discard a link another goroutine
had already fetched the index to record
Keeping the two structures agreeing:
- serializes Add and Remove on the link's id. Without it a removal running between
an add's two writes takes the table entry, finds nothing yet in the index, and
lets the add index a link the table no longer holds, which nothing cleans up.
Only RouterReportedLink held that lock before; the four other removal paths held
nothing
- serializes the connect-time pairing on that id as well, which was the last writer
left outside it. A removal reading no destination could complete while the
pairing was still deciding, leaving the link in the destination's link set after
the table had dropped it, where path computation still routed over it
- unindexes an id only when the table no longer holds a link under it, so a
replacement that has taken the table slot is not unindexed by the older link's
removal. A stale id is the safe direction to err in, since a query filters it,
where an id wrongly dropped leaves a live link no per-router query finds
- has per-router queries resolve ids against the table, so an id it no longer
holds is not handed out as a live link
Pairing a link with its destination:
- re-resolves the destination once the link is in the table, and on every later
report, so a link that reached a bad state heals. A report resolves the
destination before the link is in the table and a router's connect repairs links
already in it, so a router connecting between those two moments was seen by
neither, leaving a link with no adjacency that every operator-facing view still
called healthy
- makes that re-resolve unconditional rather than only for a link holding no
destination. The resolve happens under the source router's connect stripe, not
the destination's, so the destination can be replaced between the resolve and the
report landing, and a link left on a connection that is no longer registered
carries no adjacency just as one holding none does
- registers a connecting router before building its links, which is what makes
that re-resolve close the window rather than narrow it
- adds Link.PointDestAt so the two repair paths cannot both index the link on the
destination, since that index is a slice that does not deduplicate
Also here:
- logs the link count on both sides of the reconnect exchange, and why a report was
discarded. A router announces its links once per reconnect and is not asked
again, so a discarded report left it with no links for a router that believed it
had announced them, with nothing recorded to compare. The discard reports the
state it decided on, read once, since a reconnect landing between the decision
and the log would otherwise have it name a state that discards nothing
- gives test routers version info, which a connected router always has because the
accept path refuses a hello without it, and which the router sync path
dereferences with no nil check
The destination repair is backported from the gossip link-state branch, where the
connect path registers before building links for unrelated reasons. That ordering
is load-bearing here and is now explicit at the call site.
- adds a fablab model (zititest/models/upgrade-test) that stands up a multi-region HA controller cluster, routers, and loop4 sim traffic clients (SDK, ERT, ZET, tunneler) and drives repeatable in-place upgrade/downgrade iterations, re-bootstrapping in place each iteration
- exercises HA cluster disruption and recovery (snapshot restore, node rejoin) between iterations and validates steady-state traffic and expected terminators after each disruption
- honors the raft restartSelf setting on migration-snapshot restore, so a restored controller restarts itself instead of exiting, and promotes RestartController to a package-level function
- re-sends loop4 run-scenario requests to sims that reconnect mid-scenario, tracking each scenario's expected client set explicitly and ignoring results from clients outside it, so post-disruption client reconnection no longer stalls validation
- closes leaked loop4 sim-control connections on error and on supersession
- rotates component logs and pins the log-pipe binary to the local build, and drops router debug logging, to keep long iterating runs disk-bounded
- excludes the doc/unsettled/ scratch area for in-progress design docs from version control
The controller decided which of two racing connections for a router was current
by comparing router instances, but loaded one per connect by evicting the router
cache and reading back through it. Two connects could both evict, and whichever
read second was handed the instance the first had just published. A shared
instance makes the two connections indistinguishable: the connect path cannot
reject the second into an occupied slot, and when either channel dies the
disconnect path finds itself current and tears down the registration the other is
still using. The surviving channel is never re-bound, so the router stays
connected at the transport layer while absent from the model, unable to recover.
Connect and disconnect were also unserialized, so a stale or superseded
disconnect could interleave with a live connection and take its links with it.
- serializes a router's connect and disconnect with a per-router striped lock
- keeps at most one connection per router: a connect into an occupied slot is
rejected via an error from ConnectRouter, so the bind fails and NewChannel
closes it without starting rx or registering it, and the occupant is displaced;
the router redials into the freed slot
- displaces an occupant by closing its channel and also invoking the teardown
directly, since a channel that is already closed never fires its close handler
again; without this a dead but still registered connection holds the slot
forever and every redial is rejected against a slot nothing can free
- refuses a connect whose control channel is already closed rather than
registering it, so a connection no disconnect could ever remove is never
published
- gives every connection its own router instance via RouterManager.NewCtrlChanRouter,
read through readUncached so the cache neither supplies nor receives it, which
is what makes comparing instances meaningful
- moves recording the channel and connect time out of the accept path, so a
caller cannot attach the wrong channel or forget to attach one
- serializes link publication with that teardown on the same per-router stripe.
Validating currency and then publishing without it is a check-then-act: a
report can find the connection current and, by the time it reaches the link
manager, the teardown has already snapshotted and cleared the router's links,
so the link is recreated after everything that would have removed it. It is
then absent from the router's own index while still in the link table with a
disconnected source, and a reconnect reporting the same iteration can adopt
that stale source instead of rebuilding the link
- guards the entire DisconnectRouter teardown by connection currency, all or
nothing, and clears the connected flag and link index only when the
registration was actually given up, with the flag cleared under the same shard
lock as the map removal so the two cannot be observed disagreeing; the connected flag decides whether the
controller accepts a router's link reports, so clearing it for the wrong
connection silences a router that is up and reporting
- reduces MarkConnected to publishing the connection; the takeover-close moves
into ConnectRouter's reject path
- makes the per-router unlock idempotent so callers can defer it as a leak-safety
net and still unlock early before closing a channel outside the lock
- stops the replaced RouterSender in routerTxMap.Add so a takeover does not leak
the old sender's goroutine when the broker's asynchronous RouterDisconnected
loses the race to the redial's RouterConnected
- discards pending peer state changes for a router whose channel has closed,
since sending on one fails immediately and the failed send is retried as soon
as the event loop turns, spinning the loop and flooding the log
- queues the peer-state send-done event on every path, so a missing channel can
no longer leave sendInProgress set and stall that router's updates permanently
- resolves a router's version from its connected instance when validating link
conn info, since the version arrives in the hello and so is absent from an
instance loaded from the database
- normalizes both endpoints to the connected instance in shortestPath, which is
keyed and compared by pointer and so treated an endpoint held as any other
instance of the same router as absent from the graph, reporting a router as
unroutable from itself. That worked before only because the connect path
published its instance into the router cache, so a cache read and the connected
map returned the same object; nothing stated the requirement
- configures test logging once per package in TestMain, so a test no longer
writes global logger state while a previous test's shutdown logging reads it
- adds a link ConfigHandler (router/link FactoryRegistry) that applies router.link.v1 config: Apply rebuilds the listener/dialer set wholesale, and established Xlinks survive because Listener.Close() only closes the accept loop
- translates local link: YAML into router.link.v1 JSON and pushes it through the managed-config registry at startup
- adds the UpdateLinkListeners ctrl message so the router republishes its listener set to the controller on change; the controller re-fans via the existing PeerStateChange path
- re-evaluates dialers on link group and listener changes via RescanForDialOpportunities
For #3743.
- adds a leadership-gained handler that establishes a cluster id when a
bootstrapped cluster has none, so clusters migrated on older builds (which
came up with an empty, non-durable cluster id) self-heal without operator
action
- makes db.InitClusterId set-once and idempotent: it keeps an existing cluster
id and returns the effective value instead of erroring on a different one, so
redundant or racing backfills cannot fail the critical InitClusterIdCmd or
diverge memory from disk
- passes the current timeline id through the backfill so it is left unchanged
- publishes the effective cluster id in memory after persisting it
- adds a clusterId field to SyncSnapshotCommand and writes it into the database
after the migration snapshot restore, so a controller bootstrapped by
migrating a database ends up with a durable cluster id instead of an empty one
- the snapshot restore replaces the whole FSM database with the migration
source, which carries no cluster id, so without this the id written during
bootstrap was silently wiped and the node came up with an empty, non-durable
cluster id, defeating the mesh cluster-id validation
- persists the raft index after the cluster id in RestoreSnapshot so the index
remains the completion gate: a failure before it halts (SyncSnapshotCommand is
a critical command) and replays/retries on restart rather than skipping the
command with a blank cluster id
- fails RaftRestoreFromBoltDb when the cluster id is blank after bootstrap
- regenerates cmd.pb.go for the new field
- adds a CriticalCommand marker interface for commands that establish base
state, and marks SyncSnapshotCommand (a full snapshot restore) as critical
- makes BoltDbFsm.Apply halt when a critical command fails to apply, rather
than logging the error and persisting the advanced raft index; the failed
apply's in-tx index update is rolled back and left unpersisted, so raft
replays and retries the command on restart instead of the node running
caught-up-on-index but empty-on-data
- attaches the command type to all apply log lines so a failure is
self-contained
- reorders DisconnectRouter so Router.MarkDisconnected runs before the
RerouteLink cascade, so shortestPath and connected-map reads see the
disconnecting router as gone and never compute a replacement path
through the router being removed
- snapshots the router's links before MarkDisconnected, which clears the
router's link set, so the link-removal/reroute loop still runs
- adds a common/servermetrics package that owns the metrics MetricsMessage wire
format and the reporting/usage subsystem (message builder, usage registry,
interval and usage counters), wrapping the openziti/metrics Registry for
metric collection
- moves the controllers metrics reporter into the router package and removes it
from the shared metrics package, breaking a common -> router/env import cycle
- repoints controller and router consumers to common/servermetrics; base metric
collection stays on openziti/metrics
- keeps the proto field numbers and the metrics content-type identical so the
encoding is byte-compatible across the move, and uses a distinct proto package
name so ziti's and the library's messages coexist without a global proto
registry clash
- adds a round-trip test asserting wire compatibility with the library's
MetricsMessage
- leaves openziti/metrics unchanged, so sdk-golang and the shared xgress data
plane are unaffected
- moves the channel dependency to channel/v5 v5.0.10 and sdk-golang to v1.9.0 in the root and zititest modules
- mechanically rewrites every channel/v4 import path to channel/v5
This is the import-path-only step; the API-level changes the switch requires land in the following commit. This commit does not build on its own.
- adds `ziti agent inspect <value>...` CLI that sends an InspectRequest directly to a ziti process over its agent IPC channel and pretty-prints JSON values in the response
- accepts app id 0 on controller and router agent channels so a single inspect command works against any process type
- adds Controller.agentOpInspect, backed by a new InspectionsManager.InspectLocal that runs inspect processing on the local controller only, without fanning out to routers or peer controllers
- extracts the router inspect handler into a new router/inspect package and stores a single shared instance on Router, reused by both the control channel and the agent IPC channel
- adds RouterEnv.GetInspectHandler and RouterEnv.GetXgressListeners so the control channel bind pulls the shared handler from env
- removes the now-redundant InspectRouterEnv interface from handler_ctrl/bind.go
- adds tunnel HandleAgentAsyncOp with support for stackdump and sdk inspect keys
- adds ErrorType constants for rejected-by-application, DNS resolution failed,
port not allowed, invalid link destination, and resources not available
- adds corresponding CircuitFailureCause strings reported in circuit events
- extracts classifyDialError() in route handler to map dial errors to specific
error codes using typed errors, syscall constants, and string matching
- detects DNS errors via *net.DNSError and string fallback for ER/T hosted
services where errors are serialized through the SDK message protocol
- detects resource exhaustion via EMFILE, ENFILE, ENOBUFS syscall errors
- introduces InvalidLinkDestinationError typed error in forwarder package
- adds unit tests covering all 16 classification cases
- adds integration tests for rejected-by-application (SDK host),
DNS resolution failed, connection refused, and port not allowed
(ER/T host mode) with circuit event verification
- adds CreateEnrollAndStartTunnelerEdgeRouterWithCfgTweaks to test context
- adds CreateCircuitV3 message type and handler for routers that have
already authorized dials locally via RDM, bypassing service session
tokens in favor of identity ID, service ID, and pre-assigned circuit ID
- renames CreateCircuitRequest/Response to CreateCircuitV2Request/V2Response
for clarity now that V3 exists
- adds CircuitManager.Reserve to atomically claim circuit IDs before routing,
preventing collisions on pre-assigned IDs
- extends CreateCircuitParams with GetCircuitId so V3 can supply a
pre-assigned circuit ID (falls back to UUID generation when empty)
- fixes IsDialableByIdentity which was incorrectly calling IsBindableByIdentity
- extracts V2 handler into its own file create_circuit_v2.go
- adds CreateCircuitV3RequestType/ResponseType (20222/20223) to edge_ctrl protobuf
- registers V3 handler in controller server
- removes legacy v1 terminator code path; all terminators now use v2 flow
- refactors edgeTerminator.close() to decouple SDK notification from control plane notification
- adds pending SDK close notification queue with retry when channel is busy
- adds post-create inspect mechanism that verifies SDK still holds the bind after terminator creation
- queues second post-create inspect when establishment takes >30s to catch SDK timeout races
- detects and discards stale reordered binds on the same connection by comparing connIds
- re-establishes replacement terminators when a delete/create race is detected
- eliminates IsEntityPresent pre-filter in removeTerminatorsHandler to prevent raft ordering races
- fixes ValidateTerminators to query identities from the correct manager with the correct filter field
- adds postCreate flag to ValidateTerminatorsV2Request so routers skip redundant SDK inspect
- returns retry-later (nil result) from router validation when inspect is temporarily unavailable
- blocks SyncAllSubscribers until completion and guards RouterDataModel replacement with in-progress flag
- fixes InheritLocalData to enable service access tracking for all subscribed identities
- adds Services.Has check in GetServiceAccessPolicies to prevent false policy grants
- validates policy-to-identity associations in ValidateServicePolicies
- adds `ziti agent tunnel dump-sdk` command for SDK context inspection via IPC agent
- adds `ziti fabric inspect sdk` command to query SDK context through routers
- fixes --expected-per-host CLI flag binding in validate terminators command
- changes bind-access-lost retry hint from NotRetriable to RetryStartOver
- moves trace route response and xgress close handling off channel handler goroutine
- fixes listTerminators test helper to URL-encode filter parameter
- improves sdk-hosting-test validation resilience with login and query retries
- adds terminator_create_flow.md documenting the full lifecycle across SDK, router, and controller
- adds detailed logging for data model sync, service access tracking, and subscriber change detection
- Adds unrouteRemovedPathNodes to send Unroute to old path nodes not
present in the new path after a reroute
- Calls it from rerouteCircuit and smartReroute after successfully
routing the new path
- Uses GetConnectedRouter to get the current router object rather than
stale path pointers
- Fixes orphaned forwarder entries on transit routers replaced during
reroute, which caused circuit validation failures
Paul Lorenz