Skip to content

fix(transport): keep stored next hop aligned with import rewrite - #2978

Merged
lance0 merged 4 commits into
mainfrom
fix/rib-next-hop-self-stored-attribute
Oct 8, 2026
Merged

lance0 merged 4 commits into
mainfrom
fix/rib-next-hop-self-stored-attribute

Conversation

@lance0

@lance0 lance0 commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Problem

An IPv4 unicast route learned in a body UPDATE keeps the received NEXT_HOP among its stored attributes. Import policy writes a specific IPv4 next hop into that attribute. next-hop self, and an IPv6 next hop on an IPv4 route, are resolved by the session and change only Route::next_hop, so the stored attribute keeps the received address.

Export to passthrough peers (iBGP and route-server clients, with no export-policy rewrite) copies the stored attribute (prepare_unicast_attributes). Such peers were told the received address, while the RIB, the FIB, export policy match next-hop, gRPC and explain all used the import-chosen one.

The effect depends on the peer:

  • Extended Next Hop negotiated: the mismatch also pushed the route into a 4-octet IPv4 MP_REACH_NLRI. That is the form the exporter otherwise avoids because OpenBGPD resets the session on it.
  • Import IPv6 next hop, peer without Extended Next Hop: the route went out with the stale IPv4 address.

Reader audit

Every non-test match on PathAttribute::NextHop, plus the readers that take a next hop from routes:

Reader Verdict
transport/session/export.rs:529 prepare_unicast_attributes passthrough of stored NEXT_HOP Wrong: sends the stale address (regression below)
export.rs:1017 Extended Next Hop body-vs-MP choice compares stored NEXT_HOP to the chosen next hop Wrong encoding when stale: falls back to IPv4 MP_REACH
export.rs:579–608 synthesize NEXT_HOP when absent (override, self, eBGP, else route.next_hop) Correct
export.rs:669 MP export drops NEXT_HOP; :930 without-next-hop variant; :1045 body form requires one Correct
rib/bmp_sync.rs:256, mrt/codec.rs:603/814/823 (BMP Loc-RIB, MRT, warm checkpoint) Wrong on main (duplicate attribute), fixed separately to emit only route.next_hop
mrt/reader.rs:346, cli/ribsnap.rs:437, cli/ribsnap_bmp.rs:971 Parse external MRT/BMP bytes; correct
transport/session/inbound.rs:372 (malformed-UPDATE log), :2008 (received next hop, input to import) Pre-policy wire values; correct
policy/engine.rs:1916 apply_modifications Producer: keeps the attribute only for a specific IPv4 next hop (origin of the staleness)
api/injection_service.rs:237, src/evpn_imet.rs:366, src/evpn_segment.rs:2047, src/evpn_originator/rib_write.rs:280 Producers consistent with the route's next hop; EVPN export drops the attribute
wire/validate.rs, wire/attribute.rs Wire codec
gRPC route_to_proto (api/rib_service.rs:1834, ignores the attribute), explain (api/policy_service.rs:1821), export policy RouteContext (rib/manager/distribution/unicast.rs:727,1077,1222,1822,2045,2399; rib/manager/queries.rs:2324), FIB (src/fib_runtime.rs), Loc-RIB/best path, ORR, BLACKHOLE, RPKI/ASPA Use Route::next_hop or no next hop; correct

Other readers of route.attributes do not match NEXT_HOP, so they cannot read a next hop from it: adj_rib_in, srv6, rs_control, export_memo, flowspec_validation, the EVPN Linux dataplane, and the CLI neighbor view.

Change

When an import next-hop action applies to a body IPv4 route, inbound makes the stored NEXT_HOP agree with the resolved next hop:

  • For an IPv4 next hop, it rewrites the stored attribute.
  • For an IPv6 next hop, it drops the attribute, matching how routes received with an IPv6 next hop are stored.

Routes without a next-hop action skip the check, because their stored attribute is the received one. The aligned set is memoized, so every NLRI of one UPDATE still shares one attribute Arc. Storage keeps the attribute; stripping it would touch every producer and attribute interning on the hot path.

Validation

  • Two session tests import through process_update with a real import policy, then build the export candidate for passthrough peers:
    • next-hop self: iBGP, iBGP with Extended Next Hop, and a route-server client must each send body NLRI with one NEXT_HOP equal to the import-resolved address.
    • Import IPv6 next hop: not exportable without Extended Next Hop; MP_REACH with the IPv6 next hop with it.
    • Both also require the two NLRI of the UPDATE to share one stored set.
  • On main, both failed: the iBGP export carried NextHop(10.0.0.2) where 10.0.0.1 was expected, and the IPv6 case exported body NLRI instead of refusing.
  • With the alignment removed, both fail again. With the shared aligned set removed, the Arc sharing assertion fails.
  • cargo test -p rustbgpd-transport --lib: 801 passed.
  • just gate exited 0 (10499 passed, 0 failed, 19 ignored across 155 test suites, plus links, contracts, strict Clippy and rustdoc). just gate-rib exited 0.

Note

The MRT/BMP fix currently in review adds crates/transport/src/session/tests/mrt_next_hop.rs. One of its assertions pins the stale stored value under next-hop self. Whichever of the two changes lands second must update that assertion: the stored attribute will then equal Route::next_hop.

Import next-hop self and an IPv6 import next hop changed only the route's next hop, leaving the received NEXT_HOP among its stored attributes. Passthrough exports (iBGP, route-server clients) send that stored attribute, so they advertised the received address instead of the one the RIB selected and installed. Inbound now rewrites (or, for an IPv6 next hop, drops) the stored attribute when a next-hop action applies, shared across the NLRI of one UPDATE.

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

The single-entry alignment cache defeats bounded attribute sharing for interleaved policy results.

1 open finding
What changed in this PR

Aligns stored IPv4 route attributes with import-policy next-hop rewrites, ensuring consistent passthrough exports.

Changes:

  • Rewrites or removes stale stored NEXT_HOP attributes.
  • Adds passthrough export regression tests.
  • Documents the operator-visible fix.
File Description
crates/​transport/​src/​session/​inbound.rs Aligns and memoizes imported next-hop attributes.
crates/​transport/​src/​session/​tests/​next_hop.rs Tests IPv4 and IPv6 rewritten next-hop exports.
changelog.d/​fixed-import-next-hop-self-passthrough.md Records the behavioral change.

🧠 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/transport/src/session/inbound.rs Outdated
lance0 added 3 commits October 7, 2026 19:38
The stored next-hop alignment cached one result, so prefix-dependent import outcomes interleaved within one UPDATE cloned an attribute set per NLRI. Keep a bounded set of aligned results keyed by source set and next hop, with the import memo's bound.

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 implementation and regression coverage are coherent; only minor documentation wording needs correction.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment on lines +3 to 7
//! Inbound keeps a `NEXT_HOP` among an IPv4 unicast route's stored
//! attributes, aligned with `Route::next_hop`, the effective (post-import-
//! policy) next hop. The MRT dump, the warm checkpoint and the BMP Loc-RIB
//! synthesizer emit the next hop from `Route::next_hop` and must not also
//! emit the stored attribute.
@lance0
lance0 marked this pull request as ready for review October 8, 2026 00:32
@lance0
lance0 merged commit c5eca6c into main Oct 8, 2026
98 checks passed
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