Repository navigation
feat: collector revision Phase 0 and Phase 1 (archive rewrite, peering collector, runtime prefix control) - #3
Merged
Merged
Conversation
- docs/review-2026-08-20.md: verbatim delegated review (needs-rework, 3 blockers, 10 majors) - plan rev 4: canonical BmpFeedKey with policy-plane separation, per-router snapshot coordinator (one collector RIB, grouped entries), event-time vs seal-time discipline, feed lifecycle semantics, ADD-PATH v1 guard, backpressure as TCP stall + loss invalidation, trigger delivery records with completion policy, spill replay-manifest contract, memory figures marked estimates pending benchmark gate, trusted router identity - examples: [REDACTED] passwords, mgmt-address binds, pinned image tag, corrected 50-peer fleet row (3-6 GB), RV/RIS-compatible wording
…sthrough
Dependency refresh:
- bgpkit-parser 0.15 -> 0.20 (fallible encoders throughout, bgp_identifier
rename, RouteRefresh arm), bzip2 0.4 -> 0.6, add ipnet-trie 0.3
Dual-stack archive records:
- UpdateRecordInput/PeerStateRecordInput peer/local IP -> IpAddr
- SnapshotRoute.prefix -> IpNet with path_id: Option<u32>
Raw-frame passthrough:
- encode_bgp4mp_raw_as4 writes the original BGP UPDATE frame verbatim in a
BGP4MP_MESSAGE_AS4 envelope; ingest_update uses it instead of
parse-then-reencode (wire fidelity preserved, unknown attributes retained)
- round-trip tests for v4, v6, and unparseable frames
TABLE_DUMP_V2:
- v4 + v6 RIB entries grouped per (family, prefix) into one RibAfiEntries
record, entries sorted by peer index, snapshot-global sequence numbers,
ADD-PATH subtypes when path_id is present
Event-time vs seal-time (review blocker 3):
- ingest paths no longer rotate writers from record timestamps; rotation is
driven by the ingestion-clock tick only; late/zero timestamps stay in the
current segment with event time preserved in the MRT header (regression
tests)
No empty RIB before baseline (review major 16):
- tick/rollover skip snapshot when no routes exist
Path hardening (review major 13):
- collector_id regex ^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$ at config load
- custom templates reject absolute/backslash/control/empty/'..' components;
post-render containment assertion
…ance test - reference interop target is duck (runs BIRD today, /etc/bird/bird.conf is the config of record): AS 400644, two Vultr MD5 sessions (v4+v6), announces 192.100.82.0/24 and 2620:aa:a000::/48; Phase 1 done = focl replaces bird there for simple announcements (deployment stays a separate operator-driven step) - Phase 1 acceptance = deterministic in-process harness-peer test (configured prefixes both directions, adj-rib-in + MRT assertions); GoBGP testcontainers stay in the interop-expansion lane
Capability negotiation:
- OPEN advertises MP-BGP ipv4+ipv6 unicast, FourOctetAs (AS_TRANS on the
wire for 4-byte local ASN), route-refresh when configured
- peer OPEN capabilities parsed from the raw frame; negotiated families,
asn4, route-refresh, graceful-restart tracked per session; remote ASN
validated incl. AS_TRANS resolution from the AS4 capability
- capability-less peers still establish plain IPv4 unicast
Correct dual-stack announcements:
- IPv4 prefixes via classic NLRI + NEXT_HOP
- IPv6 prefixes via MP_REACH_NLRI (Nlri::new_reachable); invalid non-v6
configured next hops are rejected rather than malformed-encoded
- AS path is_as4 follows negotiation
Receive path + Adj-RIB-In:
- established loop no longer discards UPDATEs: raw frame -> archive
ingest_update, parsed prefixes -> per-peer ipnet-trie (classic NLRI,
MP_REACH, MP_UNREACH withdrawals), value {next_hop, as_path, last_seen}
- rib_in() returns live trie contents (was a stub); trie cleared on
session end/NOTIFICATION
- outbound configured-prefix UPDATEs also archived so a lab archive
replay covers both sides of the session
Global passive listener:
- single bind of global.listen_addr; inbound connections matched against
configured peers before any state mutation; unmatched dropped with log
- TCP-MD5 applied on accepted sockets; inbound connection wins collisions
(outbound attempt aborted); active loop resumes after inbound session
ends
Acceptance test (tests/acceptance_peer.rs):
- in-process harness peer (open with v4+v6+AS4 caps, keepalive exchange)
receives focl's configured v4+v6 announcements, announces its own
192.0.2.0/24 + 2001:db8::/32; asserts focl rib_in contains them,
Established event fired, and MRT updates segment parses back via
bgpkit-parser with records from both directions
- classic NLRI announce + rib_in apply now require the session to actually carry IPv4 (negotiated v4 MP capability, or a capability-less RFC 4271 peer); prevents leaking v4 UPDATEs onto v6-only sessions like duck's Vultr peering - send End-of-RIB after the initial announcement burst (empty UPDATE for v4; empty MP_UNREACH for v6) so peers see table transfer complete - drop now-unused afi_for helper
format!("{}:{}", v6_addr, port) produces an unbracketed, unparseable
socket string, so active IPv6 sessions never dialed (seen live on duck:
'invalid peer socket 2001:19f0:ffff::1:179'). Parse the IP explicitly
and build SocketAddr::new.
is_as4: true in bgpkit-parser selects the AS4_PATH attribute (type 17), the RFC 6793 migration fallback, rather than 4-octet AS_PATH. On AS4 sessions the correct wire form is AS_PATH (type 2) with 4-octet segments. Announcement and EoR writes now pass the negotiated AsnLength (Bits32/Bits16) instead of hardcoded Bits32. Seen live on duck: both announcements carried attr 17.
Adds `focl prefix add|remove|list` to change the originated prefix set on a running daemon: updates are written straight into established sessions, with no restart and no session reset. The effective set is (config prefixes + runtime additions) minus runtime suppressions, so `prefix remove` on a configured prefix suppresses it and `prefix add` restores it. Runtime overrides stay in memory and are never written back to the config file. `focl reload` now re-reads the config file, applies the prefix delta (announce additions, withdraw removals) and clears runtime overrides, reporting the delta and how many overrides were reset. Peer and archive settings still require a restart, which the response states via `"scope": "prefixes"`. Address family is inferred from the prefix; `--next-hop` defaults to the configured next hop of the same family, and a mismatched family is rejected before any session write. `--dry-run` reports what would be sent without changing state and `--json` prints the raw control response. Sessions carry a per-session channel that the session loop selects alongside the socket read (split read/write halves); v4 updates use NEXT_HOP and withdrawn NLRI, v6 uses MP_REACH and MP_UNREACH.
- keep framing in a dedicated reader task: the session loop selects between control operations and incoming messages, so `read_exact` can no longer be cancelled mid-message by an operation - register a session as a runtime-change target only once it is serving, and carry its negotiated families so dispatch and dry-run targets name only the peers that can carry the update - serialize each runtime mutation with its dispatch, so concurrent control clients cannot enqueue a withdrawal before the announcement it reverses - treat a next-hop change as a change, let a runtime entry shadow the configured entry for the same network, and take the default next hop from the config baseline only - exit non-zero for a failed response printed with `--json` - add the primary sources to the prior-art survey and align the `--next-hop` help text with the encoder
- `source_of` returns `Option`: a network the state never knew reports `source: null` instead of being labelled `config`, and `prefix remove` captures the pre-removal provenance for a runtime-only entry - register the session and send its initial table under the same lock, so a runtime change racing with establishment cannot leave the new session advertising a stale set - add socket-level acceptance tests: runtime add/withdraw reach an established peer on the wire (announcement, withdrawal, session stays established), and a prefix is not dispatched to a peer that did not negotiate its family
A `[[prefixes]]` entry with `dev_attr255_interval_secs = N` attaches attribute 255 (reserved for development, RFC 2042) carrying a 13-byte BGPKIT clock payload (`BGPKIT` magic, version, refresh round, unix seconds) to that prefix's announcements, re-announced every N seconds. The refresh task is scheduled per distinct interval at startup and re-reads the effective originated set on each tick, so a runtime suppression silences it and `prefix add` keeps the configured clock for the same network. The attribute is originated with OPTIONAL|TRANSITIVE; downstream speakers set PARTIAL per RFC 4271. Config validation rejects a refresh interval of 0. Tests: a fixed payload vector (including the 65,536 round wrap), encoded wire bytes plus a parse-back flag check, config roundtrip and zero-reject, and socket-level acceptance coverage where a refresh advances the embedded time and a runtime re-add keeps the clock.
…nt specifics The Unreleased section described only the runtime prefix control and the attribute-255 clock, so the archive and peering work this branch lands had no entries. Add entries for the Phase 1 peering collector, the global passive listener, the Phase 0 dual-stack archive with raw-frame passthrough, the ingestion-clock rotation rule and the archive path validation. The plan and review documents carried the reference deployment's host name, provider, session endpoints and reviewer identity; describe the reference target generically instead.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
BGP session handling, runtime prefix state, and archive generation contain multiple correctness and protocol defects.
Review effort: Balanced
Findings: 7
Open (15)
What changed in this PR
Implements collector revision Phases 0–1, adding dual-stack archival, expanded BGP collection, and runtime prefix control.
Changes:
- Reworks MRT archival for raw frames, dual-stack records, and safer layouts.
- Adds passive peering, capability negotiation, Adj-RIB-In, and acceptance tests.
- Adds runtime prefix mutation, reload support, and attribute-255 clocks.
| File | Description |
|---|---|
tests/archive_integration.rs |
Updates archive tests for dual-stack addresses. |
tests/acceptance_peer.rs |
Adds socket-level BGP acceptance coverage. |
src/control/mod.rs |
Defines prefix control commands and arguments. |
src/config/mod.rs |
Adds clock configuration and archive validation. |
src/bin/focld.rs |
Handles reload and prefix control requests. |
src/bin/focl.rs |
Adds prefix CLI commands and output. |
src/bgp/mod.rs |
Implements peering, RIB, runtime updates, and clocks. |
src/archive/types.rs |
Generalizes archive models for dual-stack data. |
src/archive/snapshot.rs |
Adds raw BGP4MP and grouped RIB encoding. |
src/archive/mod.rs |
Changes rollover and ingestion-time behavior. |
src/archive/layout.rs |
Hardens custom archive paths. |
README.md |
Documents prefix control and clock configuration. |
docs/review-2026-08-20.md |
Records the collector design review. |
docs/prefix-control-prior-art.md |
Documents prefix-control design research. |
docs/examples.md |
Adds target-state deployment examples. |
docs/collector-revision-plan.md |
Defines the phased collector architecture. |
CHANGELOG.md |
Describes the new collector features. |
Cargo.toml |
Updates parser/compression dependencies and adds trie storage. |
Cargo.lock |
Locks the updated dependency graph. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
291
to
295
| @@ -292,8 +295,12 @@ impl ArchiveService { | |||
| peers: vec![], | |||
Comment on lines
+620
to
+638
| let result = service.run_session(&peer, stream).await; | ||
| service.clear_rib(&peer.address).await; | ||
| match result { | ||
| Ok(()) => { | ||
| service | ||
| .set_peer_state(&peer.address, PeerState::Active, None, None) | ||
| .await | ||
| } | ||
| Err(error) => { | ||
| service | ||
| .set_peer_state( | ||
| &peer.address, | ||
| PeerState::Active, | ||
| Some(error.to_string()), | ||
| None, | ||
| ) | ||
| .await | ||
| } | ||
| } |
| }; | ||
| for update in updates { | ||
| let raw = write_bgp_message(writer, &update, asn_len).await?; | ||
| if let Some(archive) = &self.inner.archive { |
…eplay
Address the six review findings on the session code of the collector
revision. No finding was declined.
* src/bgp/mod.rs:598 (TCP-MD5, passive sessions): the per-peer keys are now
installed on the listening socket before the accept loop (raw fd
TCP_MD5SIG, via the new `listener_md5_keys`/`install_listener_md5_keys`,
which skip disabled peers, peers without a password and non-IP addresses).
A peer with a configured password can now complete the handshake at all;
the accepted socket still gets the key as before.
* src/bgp/mod.rs:729 (hold time, RFC 4271 s4.2/s4.4/s6.1): OPEN advertises
the configured hold time unchanged (0 stays 0, no `.max(3)`), the new
`negotiate_hold_time` derives the negotiated value from the remote OPEN
(0 on either side disables the timers, otherwise the smaller value wins),
and the keepalive and hold timers are only armed for a non-zero negotiated
value, so a negotiated 0 really disables them.
* src/bgp/mod.rs:1624 with build_announce_updates at :1518 (RFC 6793
fallback): `base_announce_attributes` takes the negotiated capabilities and,
for a session without 4-octet AS support whose local ASN does not fit in two
octets, puts AS_TRANS in AS_PATH and the real ASN in AS4_PATH (attribute 17)
instead of letting the encoder truncate the ASN. A 2-octet local ASN still
sends a single plain AS_PATH.
* src/bgp/mod.rs:863 (route refresh, RFC 2918 s4): a received ROUTE-REFRESH
now replays the requested family's Adj-RIB-Out (the session's effective
prefix set, filtered to the family and snapshotted under the mutation lock)
and then that family's End-of-RIB, through the new
`replay_route_refresh`, while the hold-timer reset stays as it was. A
refresh for a family the session does not carry, and the RFC 7313
BoRR/EoRR subtypes, are ignored. The End-of-RIB builder moved into
`end_of_rib` because three sites now need it.
* src/bgp/mod.rs:766 (End-of-RIB, capability-less peer): the EoR loop uses
the same family predicate as the announcement path (new
`NegotiatedCapabilities::carries`, which accepts classic IPv4 for a peer
that negotiated nothing), so such a peer gets its IPv4 End-of-RIB. The
IPv4 decisions in `build_announce_updates`, `build_withdraw_updates`,
`apply_update_to_rib` and the session handle now share that predicate.
* src/bgp/mod.rs:760 vs :816 (duplicate establishment table): the pre-lock
table send and the separate End-of-RIB loop are gone. The session now
registers and sends its initial table in one critical section under the
mutation lock, and the End-of-RIB markers follow that single send, so a
peer (and the archive) no longer sees every initial announcement twice,
once of them after End-of-RIB.
Also re-exports the TCP-MD5 socket helpers as
`focl::bgp::{TcpSocketExt, TcpStreamExt}` so the socket-level acceptance test
can authenticate its own socket the way the daemon does, and records the six
fixes in the CHANGELOG.
Tests: unit tests for the listener key list, the hold-time negotiation table,
the OPEN advertisement including zero, the AS4_PATH fallback (wire bytes plus
a bgpkit-parser round trip), the family predicate and the EoR builders; socket
acceptance tests for an MD5-authenticated passive session, the IPv4 refresh
replay with its End-of-RIB, a refresh for an unnegotiated family, a
capability-less peer that receives its IPv4 table and EoR exactly once, and a
configured hold time of 0 seen on the wire. Each new test fails with the
corresponding fix reverted.
Gates: cargo fmt --all, cargo clippy --all-targets --all-features -- -D
warnings and cargo test --all-features all pass.
… dry-run, lock, family) Fixes six findings from the Copilot review of the runtime prefix control path. Each finding is cited by its line range in src/bgp/mod.rs at bd0605a. - src/bgp/mod.rs:304-313 PrefixState::remove early-returned when the network had a runtime entry in `added`. For a configured network with a `prefix add --next-hop` override, `prefix remove` dropped the override but never suppressed the config baseline: peers received a withdrawal while prefix_view/prefix list and any later session still treated the network as announced. The override is dropped and a configured network is suppressed in the same call; the return value still reports whether the effective originated set changed. - src/bgp/mod.rs:251-264 PrefixState::view mapped configured entries directly, so `prefix list` printed the configured next hop and source `config` for a network with a runtime override, contradicting effective() (:183-197) and source_of() (:220-228). The row is built from the `added` entry with source Runtime when one exists. - src/bgp/mod.rs:317-334 reset_overrides compared networks only, so a changed configured next hop or a changed/cleared attribute-255 interval updated the internal state without re-announcing, leaving established peers with the old attributes on the wire. The delta now compares the whole entry (network, next hop, interval) and treats an attribute change as an announcement, while a network that disappeared stays a withdrawal. Comment added: a changed interval only takes effect after a restart, because the refresh task set is sampled at startup. - src/bgp/mod.rs:1225-1240 `prefix add --dry-run` computed `changed` as `!is_announced`, while the real mutation also reports a change when the next hop differs (:283-300). `prefix add <existing> --next-hop <new> --dry-run` wrongly reported no change and no target peers. The dry run now runs the same `add` on a clone of the state and discards it, so it agrees with the real mutation without mutating or dispatching anything. - src/bgp/mod.rs:1259-1261 prefix_remove read previous_source before acquiring prefix_ops, so a concurrent add/remove/reload could change provenance in between and the reported `source` could be wrong (including the deliberate `source: null` for a network the state never knew). Guarantee now: the mutation lock is acquired before the provenance read, and the provenance read, `changed`, and the session dispatch all happen under that single lock hold, so the reported source describes the same state the mutation applies to. Lock ordering is not observable from a single-threaded unit test; the added test covers the observable provenance behaviour instead. - src/bgp/mod.rs:346-366 parse_prefix_entries parsed next hops without validating them against the prefix family, while the runtime path calls ensure_next_hop_family (:368, used at :1220). An IPv4 prefix with an IPv6 next hop reached classic NEXT_HOP encoding (a 16-octet NEXT_HOP on the wire; RFC 8950 capability 5 is never advertised) and the inverse was silently dropped later. Each configured entry is now validated at parse time with the same helper, and the error names the entry. Tests added in src/bgp/mod.rs: remove_after_a_next_hop_override_suppresses_the_configured_prefix, prefix_list_reports_the_runtime_override_for_a_configured_network, reload_re_announces_changed_attributes_and_withdraws_gone_networks, configured_next_hop_must_match_the_prefix_family, dry_run_add_reports_a_next_hop_change_and_family_targets, prefix_remove_reads_provenance_under_the_mutation_lock. Five of the six fail against the bd0605a code with only the tests applied; the provenance test passes both ways because lock ordering is not observable from a single-threaded test, so that finding is documented here. Gates: cargo fmt --all, cargo clippy --all-targets --all-features -- -D warnings, cargo test --all-features (48 unit, 6 acceptance, 1 archive integration, all pass; exit codes 0/0/0).
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It combines protocol, archive, concurrency, and operational changes with unresolved RIB, MRT-direction, security, and session-state issues.
Review effort: Balanced
Findings: 6
Open (9)
Resolved since last review (12)
Comment on lines
+295
to
+299
| peers: vec![], | ||
| routes: vec![], | ||
| }; | ||
| self.snapshot_now(snapshot).await?; | ||
| *last_rib = Some(rib_bucket); | ||
| if snapshot.routes.is_empty() { | ||
| tracing::debug!("skipping RIB snapshot because no baseline routes exist"); |
Comment on lines
+69
to
+73
| encode_common_header( | ||
| input.timestamp as u32, | ||
| EntryType::BGP4MP, | ||
| Bgp4MpType::MessageAs4 as u16, | ||
| payload, |
| // Snapshot under the mutation lock, so an incremental change cannot | ||
| // interleave with the replay: an earlier mutation is already in the | ||
| // snapshot, a later one is dispatched after the replay is written. | ||
| let _guard = self.inner.prefix_ops.lock().await; |
Comment on lines
+327
to
+329
| validate_relative_template(value).with_context(|| { | ||
| format!("[archive.custom_templates].{name} must be a safe relative path") | ||
| })?; |
Comment on lines
+370
to
+372
| #[cfg(target_os = "linux")] | ||
| #[tokio::test] | ||
| async fn passive_session_with_md5_completes_the_handshake() -> Result<()> { |
| for interval in intervals { | ||
| let service = self.clone(); | ||
| tokio::spawn(async move { | ||
| let mut ticker = tokio::time::interval(Duration::from_secs(u64::from(interval))); |
…ction Address the archive findings of the PR-3 review: * The scheduled RIB snapshot is built from the speaker's live Adj-RIB-In through a registered source port instead of a hard-coded empty view, so rib segments are produced again; an explicit snapshot-now still writes a segment for an empty view. * Locally generated messages are archived as BGP4MP_MESSAGE_AS4_LOCAL (RFC 6396 section 4.4.6) instead of an incoming subtype. * A session's exit from Established is archived before the RIB (and the session's local address) is cleared.
- A route-refresh replay no longer takes the mutation lock: the session loop is the only drain for its own queue, so a control client blocked on a full queue while holding the lock could never be unblocked, and the lock wait deadlocked the session against its own producer. Ordering is kept by the loop itself: the state read is one view, so a mutation applied before it is inside the replay and a later one is written after it. - The replication path resolves a manifest's relative path before use, so a manifest changed after it was queued cannot redirect the copy: absolute paths, parent components and empty paths are refused, and a local target is asserted to stay inside the destination root. - The attribute-255 ticker skips missed ticks instead of bursting them, so a stall dispatches one refresh per clock round. Each fix is pinned by a regression test that fails when the fix is reverted, and the ticker test drives the skipped-tick behavior on a paused clock.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands the collector revision through Phase 1: the archive rewrite and a real BGP peering collector, plus runtime prefix control and the attribute-255 clock. Phase 2 (BMP station) and later stay out of this branch.
The intermediate feature PRs (#1, #2) merged into this branch, which runs no CI of its own; this PR against
mainis where the branch first gets theRustworkflow.Archive (Phase 0)
bgpkit-parser0.15 -> 0.20,bzip20.6, newipnet-triedependency.src/archive/, with manifest and rollover behaviour covered bytests/archive_integration.rs.Peering collector (Phase 1)
IpAddr.read_exactis no longer cancelled mid-message by a control operation.Runtime prefix control and clock
focl prefix add|remove|list,focl reloadapplying the config's prefix delta, and the per-prefix attribute-255 clock ([[prefixes]] dev_attr255_interval_secs). Behaviour, wire format and rationale are in theUnreleasedCHANGELOG entries and indocs/prefix-control-prior-art.md.Notes
docs/collector-revision-plan.mdis the plan this branch implements; Phase 2 and later are deliberately excluded.tests/acceptance_peer.rscovers the on-the-wire paths: runtime announce and withdraw reaching an established peer, family gating, and the clock refresh and re-add paths.Unreleasedsection currently covers the prefix-control and clock features only) and a version bump.