Skip to content

fix(bmp): negotiate addpath capability by direction - #65

Open
mxyns wants to merge 8 commits into
network-analytics:mainfrom
mxyns:fix-bmp-capabilities-negociation
Open

fix(bmp): negotiate addpath capability by direction#65
mxyns wants to merge 8 commits into
network-analytics:mainfrom
mxyns:fix-bmp-capabilities-negociation

Conversation

@mxyns

@mxyns mxyns commented Oct 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Match ADD-PATH address families and combine local send/receive support with the complementary remote direction instead of intersecting identical capabilities.

Add regression coverage for directional combinations of add-path

Fixes #67

@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch from 1db334a to f3b9478 Compare October 19, 2026 13:26
@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch 2 times, most recently from 4bd61eb to e91a1f5 Compare October 19, 2026 13:39
@rodonile
rodonile requested a balanced review from Copilot October 19, 2026 14:45
Copilot AI reviewed Oct 19, 2026
View reviewed changes

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

Shared parsing state can apply inbound ADD-PATH settings to outbound traffic, causing incorrect decoding.

1 open issue
What changed in this PR

Updates BMP capability negotiation to account for traffic direction rather than intersecting identical capabilities.

Changes:

  • Matches complementary ADD-PATH send/receive support by address family.
  • Selects receiver label limits while preserving first duplicates and single-label defaults.
  • Adds regression coverage for directional and asymmetric capabilities.
File Description
crates/​bmp-pkt/​src/​codec.rs Implements directional negotiation and regression tests.

Review effort: Balanced

Comment thread crates/bmp-pkt/src/codec.rs Outdated
@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch from e91a1f5 to c0178f3 Compare October 19, 2026 23:52
@rodonile
rodonile requested a balanced review from Copilot October 20, 2026 08:46
@Gingembreny Gingembreny mentioned this pull request Oct 20, 2026
Open
Copilot AI reviewed Oct 20, 2026
View reviewed changes

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

Shared parsing state can select incorrect label limits or ADD-PATH settings when traffic direction changes.

2 open issues
1 resolved since last review

Review effort: Balanced

Comment thread crates/bmp-pkt/src/codec.rs Outdated
Comment thread crates/bmp-pkt/src/wire/deserializer/v3.rs
@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch from c0178f3 to 2d7045f Compare October 20, 2026 11:56
@mxyns mxyns changed the title fix(bmp): negotiate capabilities by direction fix(bmp): negotiate addpath capability by direction Oct 20, 2026
@mxyns mxyns mentioned this pull request Oct 20, 2026
Draft
rodonile
rodonile requested changes Oct 21, 2026
View reviewed changes
Comment thread crates/bmp-pkt/src/codec.rs
Comment thread crates/bmp-pkt/src/codec.rs
Comment thread crates/bgp-pkt/src/wire/deserializer/mod.rs
@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch from b16e204 to a36c8c8 Compare October 22, 2026 11:26
rodonile
rodonile requested changes Oct 23, 2026
View reviewed changes
Comment thread crates/bgp-pkt/src/wire/deserializer/mod.rs Outdated
Comment thread crates/bgp-pkt/src/wire/deserializer/path_attribute/path_attribute.rs Outdated
Comment thread crates/bgp-pkt/src/wire/deserializer/path_attribute/path_attribute.rs Outdated
rodonile
rodonile reviewed Oct 23, 2026
View reviewed changes
Comment thread crates/bgp-pkt/src/wire/deserializer/path_attribute/path_attribute.rs Outdated
rodonile
rodonile reviewed Oct 23, 2026
View reviewed changes
Comment thread crates/bgp-pkt/src/wire/deserializer/path_attribute/path_attribute.rs Outdated
rodonile
rodonile reviewed Oct 23, 2026
View reviewed changes
Comment thread crates/bgp-pkt/src/wire/deserializer/mod.rs
@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch from a36c8c8 to 9726190 Compare October 23, 2026 10:56
rodonile
rodonile reviewed Oct 23, 2026
View reviewed changes

@rodonile rodonile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Last nits, then ready to merge from my side

Comment thread crates/bgp-pkt/src/wire/deserializer/path_attribute/path_attribute.rs Outdated
Comment thread Cargo.toml Outdated
@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch 3 times, most recently from 2ba5119 to 1e280e7 Compare October 23, 2026 13:00
@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch from 1e280e7 to ba93219 Compare October 23, 2026 13:15
rodonile
rodonile reviewed Oct 23, 2026
View reviewed changes
Comment thread crates/bgp-pkt/src/wire/deserializer/path_attribute/path_attribute.rs Outdated
@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch 3 times, most recently from 25ca4aa to acd5831 Compare October 23, 2026 15:06
rodonile
rodonile previously approved these changes Oct 27, 2026
View reviewed changes
@rodonile
rodonile force-pushed the fix-bmp-capabilities-negociation branch from acd5831 to 8c46890 Compare October 27, 2026 14:30
Max and others added 7 commits October 28, 2026 09:17
Max @mxyns
test(bmp): reproduce bidirectional add-path decoding failure
6966db7 
Decode one Peer Up followed by inbound and outbound Route Monitoring
messages through one codec in BMP v3 and v4. Cover asymmetric
ADD-PATH support, both BGP-ID aliases, and withdrawn and announced NLRI.

Both regressions fail before session negotiation and direction-aware
parsing are implemented in the following steps.
Max @mxyns
refactor(bgp): store add-path directions in one capability map
d0109da 
Introduce AddPathMap with AddPathAddressFamily values indexed by
address family. Keep send and receive flags in one map and expose
directional lookups without allocating temporary maps.

Preserve receive-side constructor input and adapt NLRI readers and
storage tests. Leave BMP negotiation and direction selection for
the next steps.
Max @mxyns
fix(bmp): negotiate both add-path directions per session
33aa3be 
Match complementary send and receive support by address family. Store
both negotiated directions under both BGP-ID aliases and clear stale
ADD-PATH state on later Peer Up while retaining Loc-RIB families.
Max @mxyns
fix(bmp): select add-path direction for each monitored update
b1d850f 
Select send or receive support using each Route Monitoring header.
Keep v4 stateless overrides message-local and delegate capability
updates to a borrowed ADD-PATH helper.
Max @mxyns
test(bmp): cover directional add-path state isolation
76607e0 
Extend the single-session regressions with IPv6 MP_REACH/MP_UNREACH,
v4 stateless overrides, and later Peer Up capability removal. Verify
both lookup aliases and ensure overrides do not leak across messages.
@mxyns
fix(bmp): ignore addpath direction for loc-rib capability negociation
036c8ef 
@rodonile:
For loc-rib this negotiation should be skipped, it's enough to have capability present for mandating when add-path should be present: https://datatracker.ietf.org/doc/html/rfc9069#section-5.2
@mxyns
test(bmp): cleanup tests and use helper functions
41c39cf 
use helper functions to make the long tests clearer
use itertools combinations instead of 5 nested for loops
@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch from 8c46890 to ec348cc Compare October 28, 2026 09:21
@mxyns

mxyns commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Had to rebase since #69 is merged first
I also noticed that BgpParsingContext::new was still expecting a HashMap<AddPathAddressFamily, bool> so I changed it to the new type AddPathMap. I had to add Arbitrary to AddPathMap, and I also added a from_iter helper method from a list of AddPathAddressFamily.

@mxyns
mxyns force-pushed the fix-bmp-capabilities-negociation branch from ec348cc to 463dd70 Compare October 28, 2026 09:27
@rodonile
rodonile enabled auto-merge (rebase) October 28, 2026 09:55
@rodonile
rodonile requested a lite review from Copilot October 28, 2026 09:56
Copilot AI reviewed Oct 28, 2026
View reviewed changes

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.

🔵 Needs a closer look

An active moderate finding remains because Route Mirroring can derive the wrong ADD-PATH parsing direction and misparse mirrored updates when negotiated send/receive bits differ.

No open issues 🎉

2 resolved since last review

Review effort: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@rodonile rodonile rodonile left review comments
@riccardo-negri riccardo-negri Awaiting requested review from riccardo-negri
@ustorbeck ustorbeck Awaiting requested review from ustorbeck

At least 2 approving reviews are required to merge this pull request.

Assignees

@mxyns mxyns

@rodonile rodonile

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: BMP session closes with Ipv4PrefixError(Eof) when processing FRRouting route updates

3 participants

@mxyns @rodonile