Skip to content

fix(mrt): encode one next hop for received IPv4 routes - #2977

Merged
lance0 merged 2 commits into
mainfrom
fix/mrt-single-ipv4-next-hop
Oct 7, 2026
Merged

lance0 merged 2 commits into
mainfrom
fix/mrt-single-ipv4-next-hop

Conversation

@lance0

@lance0 lance0 commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Problem

The RIB keeps the received NEXT_HOP among an IPv4 unicast route's stored attributes, and route injection and import policy keep one too. Three encoders re-emit stored routes, and each added NEXT_HOP from Route::next_hop without skipping the stored copy:

  • MRT RIB entries: periodic and on-demand dumps, and the warm checkpoint snapshot.
  • synthesize_attributes.
  • BMP Loc-RIB announcements.

As a result:

  • Every BGP-learned IPv4 unicast route was dumped with two NEXT_HOP attributes.
  • Warm checkpoint publication failed its recovery check (warm bundle MRT recovery discarded N path attributes), so a daemon with any such route never published a checkpoint.
  • After an import next-hop self, the second copy held the stale received address.

This was reproduced on the official v0.75.0 binary and on main.

Change

  • Route::attributes_except_next_hop() defines which stored attributes these encoders may copy. The next hop comes only from Route::next_hop, the post-import-policy next hop the RIB selects and installs with. That is also the value an MRT TABLE_DUMP_V2 entry for an Adj-RIB-In post-policy or Loc-RIB view should record.
  • The MRT encoder, synthesize_attributes and the BMP Loc-RIB synthesizer use it. The BMP helper now encodes attributes one at a time from an iterator rather than splitting a slice.
  • RIB storage itself is unchanged. Inbound, injection, import policy and EVPN all produce stored NEXT_HOP, and export already rewrites it. Stripping it at every producer would be a broader change to the hot path and to interning.

Validation

  • New session tests in crates/transport/src/session/tests/mrt_next_hop.rs build the route through PeerSession::process_update, as inbound stores it:
    • The MRT dump carries exactly one NEXT_HOP, and nothing is discarded.
    • The post-policy next hop is recorded for next-hop self and for a specific next hop.
    • A warm checkpoint publishes, loads, and recovers the prefix, next hop and attributes.
    • A BMP Loc-RIB announcement carries one NEXT_HOP.
    • IPv6 MP_REACH keeps its global and link-local next hop.
  • The MRT encoder equivalence test now includes a stored, stale NEXT_HOP.
  • On unfixed main, all four IPv4 tests failed:
    • two with discarded 1 (the MRT dump tests);
    • one at write_warm_bundle (the warm checkpoint test);
    • one with duplicate attribute type 3 (the BMP test).
  • With each fix site removed again, the matching tests fail (rc=101):
    • MRT encoder: dump, post-policy and warm tests, plus the codec equivalence test.
    • synthesize_attributes: the equivalence test.
    • BMP: the BMP test.
  • The IPv6 guard was shown sensitive by dropping the link-local half.
  • Live check, two containers on an internal network: the fixed daemon and a v0.75.0 peer.
    • The peer announced 198.51.100.0/24 and BLACKHOLE 203.0.113.1/32, and the fixed daemon installed both.
    • On SIGTERM it published a warm checkpoint (view_route_counts [2]), and its GR marker carries the checkpoint generation.
    • rbgp mrt-dump entries and the checkpoint snapshot each carry one NEXT_HOP (192.0.2.3).
    • The same setup on v0.75.0 failed publication.
  • cargo fmt --check and the commit hooks pass. just gate exited 0 (10502 passed, 0 failed, 19 ignored across 155 test suites, plus links, contracts, strict Clippy and rustdoc). just gate-rib exited 0.

Compatibility

  • No published bundle contains the duplicate, because publication failed closed, and startup never restores bundles. The warm reader therefore stays strict.
  • MRT files from earlier releases keep the duplicate. A reader following RFC 7606 discards all but the first copy. The encoder always placed the synthesized post-policy next hop before the stored one, so such readers already recovered the right next hop.

The RIB keeps the received NEXT_HOP among an IPv4 unicast route's attributes, and import next-hop self updates only the route's next hop. MRT RIB entries, warm checkpoints and BMP Loc-RIB announcements emitted the route's next hop and the stored attribute, so each entry carried NEXT_HOP twice and warm checkpoint publication failed its recovery check. These encoders now emit only the route's post-policy next hop.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

BMP encoding introduces repeated temporary allocations on the RIB hot path by losing scratch-buffer reuse.

1 open finding
What changed in this PR

Fixes duplicate IPv4 NEXT_HOP attributes when stored RIB routes are re-encoded, allowing warm checkpoint publication and preserving the post-policy next hop.

Changes:

  • Shares stored-attribute filtering across MRT and BMP encoders.
  • Adds regression tests for received routes, policy rewrites, checkpoint recovery, and IPv6 preservation.
  • Documents the fix and compatibility behavior.
File Description
crates/​transport/​src/​session/​tests/​mrt_next_hop.rs Adds end-to-end next-hop regression tests.
crates/​transport/​src/​session/​tests/​mod.rs Registers the new tests.
crates/​transport/​Cargo.toml Adds test dependencies.
crates/​rib/​src/​route.rs Adds shared next-hop filtering.
crates/​rib/​src/​bmp_sync.rs Filters stored next hops during BMP synthesis.
crates/​mrt/​src/​codec.rs Prevents duplicate next hops in MRT encoding.
changelog.d/​fixed-mrt-single-ipv4-next-hop.md Documents operational impact and compatibility.
Cargo.lock Records test dependencies.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/rib/src/bmp_sync.rs Outdated
Splicing the synthesized next hop into the BMP Loc-RIB announcement encoded each attribute with its own call, allocating a fresh value scratch per attribute. Encode the spliced borrowed attributes once through UpdateMessage::try_build_from_attribute_iter instead.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused encoder changes consistently preserve effective next hops, with targeted regression coverage and no unresolved findings.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@lance0
lance0 marked this pull request as ready for review October 7, 2026 23:38
@lance0
lance0 merged commit 5369593 into main Oct 7, 2026
101 checks passed
lance0 added a commit that referenced this pull request Oct 8, 2026
Add a debug-only check that an encoded path-attribute list carries each attribute type at most once, so an encoder that copies stored attributes and also synthesizes one of the same type fails in tests rather than on the wire. The wire attribute encoder, which every export UPDATE (per-session and update-group) and BMP message passes through, compiles the check only under the new non-default strict-encode-invariants feature with debug assertions; this workspace's transport, rib and mrt test builds enable it, while default, release and embedder builds are unchanged. MRT RIB entries get an equivalent walk that skips blocks it cannot read faithfully, so oversized entries still report FieldTooLarge.

Reintroducing the duplicate NEXT_HOP fixed in #2977, or removing export's existing-NEXT_HOP check, trips the guard in the affected tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants