bgpd: bmp: don't prepend local-AS to AS_PATH in BMP updates

[kalash-nexthop]

bgpd: bmp: don't prepend local-AS to AS_PATH in BMP updates

BMP Route Monitoring messages for monitored routes carried an AS_PATH
with the local AS prepended, as if the route were being re-advertised
outbound. Observed with a peer in AS 40001 advertising 100.100.100.1/32
to a router in AS 40002:

"as_path":[40002,40001], "as_path_count":2, "peer_asn":40001

The collector expected AS_PATH=[40001] -- the value as received from
the peer -- but saw [40002,40001] because the local AS was prepended
on its way out to the collector.

Root cause: bmp_update() in bgp_bmp.c reuses bgp_packet_attribute(),
the outbound attribute serializer, which performs peer-specific outbound
transformations on AS_PATH (prepend peer->local_as for eBGP, confed-seq
handling, change-local-as). These are correct for sending an UPDATE to
a peer, but BMP is a reporting channel -- pre-policy, post-policy, and
loc-rib must all emit the AS_PATH as stored (received / selected),
never with the outbound local-AS prepended.

Fix: add a bool for_bmp parameter to bgp_packet_attribute(). When
true:

• Skip the peer-sort-based AS_PATH transformations and emit
  attr->aspath as-is (no local-AS prepend, no confed-seq
  manipulation).

• Force 32-bit ASN encoding regardless of the peer's AS4 capability
  state. This is needed for loc-rib re-dumps after a VRF/peer flap:
  peer->cap can be momentarily cleared while the session
  renegotiates, which would otherwise cause aspath_put() to emit
  16-bit AS_PATH segments. Modern BMP collectors always parse ASNs as
  32-bit, so a 16-bit segment produces a byte-level parse error and
  the collector disconnects, losing all subsequent peer-up / peer-down
  / route-monitor messages.

bmp_update() passes for_bmp=true (covers pre-policy, post-policy,
and loc-rib). The outbound callers in bgp_updgrp_packet.c pass false
to keep their current behaviour.

Also update the bgp_bmp topotest expected JSON files for all three
modes: previously they encoded the prepended local-AS ("65501 65502"
where 65501 is the local AS); now they expect the raw AS_PATH
"65502" as received from the peer in AS 65502.

[greptile-apps]

Greptile Summary

Fixes a long-standing correctness bug in BMP Route Monitoring: bgp_packet_attribute() was applying outbound eBGP/confed AS_PATH transformations (local-AS prepend, confed-seq) to BMP messages, causing collectors to see a modified AS_PATH instead of the stored received path. The fix adds a bool for_bmp parameter that suppresses those transformations and forces 32-bit AS encoding (guarding against a secondary disconnection bug when a peer's AS4 capability is transiently cleared). All three call sites are correctly updated, and the topotest expected JSON files are aligned with the corrected behaviour across pre-policy, post-policy, and loc-rib modes.

Confidence Score: 4/5

Safe to merge; the fix is correct, all callers are updated, and memory management is unaffected.

Only P2 findings (a slightly inaccurate inline comment). Core logic, memory ownership, and the for_bmp guard are all correct. Score held at 4 rather than 5 due to the minor comment inaccuracy.

bgpd/bgp_attr.c — the new inline comment at line ~5363 overstates that Adj-RIB-In is the only BMP mode FRR supports.

Important Files Changed

Filename	Overview
bgpd/bgp_attr.c	Adds for_bmp parameter to bgp_packet_attribute(); when true, forces 32-bit AS encoding and skips all peer-sort-based AS_PATH transformations (eBGP local-AS prepend, confed-seq). Memory management is correct — the existing aspath != attr->aspath guard at line 5704 handles cleanup safely for all branches including the new for_bmp=true path.
bgpd/bgp_attr.h	Function declaration updated to add bool for_bmp parameter, matching the implementation change in bgp_attr.c.
bgpd/bgp_bmp.c	Passes for_bmp=true to bgp_packet_attribute() so BMP Route Monitoring messages emit the AS_PATH as stored, without outbound eBGP/confed-seq transformations.
bgpd/bgp_updgrp_packet.c	Both bgp_packet_attribute() call sites updated to pass for_bmp=false, preserving existing outbound BGP UPDATE behaviour.
tests/topotests/bgp_bmp/bmp1/bmp-update-loc-rib-step1.json	Expected AS_PATH updated from "65501 65502" to "65502" to reflect the raw received path without the local-AS prepend.
tests/topotests/bgp_bmp/bmp1vrf/bmp-update-pre-policy-step1.json	VRF-specific expected AS_PATH corrected from "65501 65502" to "65502" for the pre-policy mode, consistent with all other BMP step files.

Sequence Diagram

sequenceDiagram
    participant Peer as BGP Peer (AS 65502)
    participant FRR as FRR Router (AS 65501)
    participant BMP as BMP Collector

    Peer->>FRR: UPDATE [AS_PATH=65502] prefix=172.31.0.15/32
    FRR->>FRR: Store attr->aspath = [65502]

    note over FRR,BMP: Before fix: bmp_update() called bgp_packet_attribute()<br/>with no for_bmp flag — eBGP path applied local-AS prepend
    FRR--xBMP: BMP Route Monitor [AS_PATH=65501 65502] ❌

    note over FRR,BMP: After fix: bmp_update() passes for_bmp=true
    FRR->>BMP: BMP Route Monitor [AS_PATH=65502] ✅ (as stored)

    note over FRR: Outbound BGP UPDATE to other peers unchanged
    FRR->>Peer: UPDATE [AS_PATH=65501 65502] (for_bmp=false, normal eBGP prepend)

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: bgpd/bgp_attr.c
Line: 5363-5371

Comment:
**Inaccurate comment: FRR already emits loc-rib messages**

The comment says "BMP Adj-RIB-In monitoring (the only BMP mode FRR currently supports)", but the test-suite changes in this very PR update expected JSON for `loc-rib`, `pre-policy`, and `post-policy` modes — meaning FRR already supports at least three BMP table types. Calling it "the only BMP mode" is misleading to future readers and may cause confusion when RFC 8671 Adj-RIB-Out work begins.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "bgpd: bmp: don't prepend local-AS to AS_..." | Re-trigger Greptile}

[donaldsharp]

I see two separate bug fixes, at the very least. Why is this not two commits?

I would also like to see discussion from some of our bgp operators on what they want to do here since this will be a non-backwards compatible change to what is delivered and we need to ensure that this is thought out and that operators have a path forward that makes sense to them.

[kalash-nexthop]

I see two separate bug fixes, at the very least. Why is this not two commits?

I would also like to see discussion from some of our bgp operators on what they want to do here since this will be a non-backwards compatible change to what is delivered and we need to ensure that this is thought out and that operators have a path forward that makes sense to them.

Thanks for looking @donaldsharp . Appreciate the quick feedback.

You are right. While fixing the BMP local-as issue, I had to fix the 32-bit ASN encoding issue in order for the modified tests to pass. So there are exactly two fixes here.

I can split this now. Should have done it like that to begin with, but the local-as path tests in the second PR wouldn't pass without the first change, and I wasn't sure what's the strategy for posting stacked PRs (first time contributor here).

About the backward compatibility concern, is there any action item on me?

[donaldsharp]

I just think I want to discuss this in next weeks tech meeting. We should go over it because it's a new PR that came in during the week. Feel free to join and have the discussion with us.

[ton31337]

Please fix frrbot styling issues.

[kalash-nexthop]

Please fix frrbot styling issues.

Done. Thanks.

[kemp-rv]

Happy to see this option go in. I was just about to file a bug report on the same issue, about prepending local AS to BMP monitor messages. I concur with the reading in RFC7854 4271 and 9069. This flag addition should help turn off that prepend behavior for BMP monitor.

[ton31337]

@kalash-nexthop what do you think by adding a new configuration knob to control this (append vs. no-append)? Then we don't need any changes once we have adj-rib-out.

[kalash-nexthop]

@kalash-nexthop what do you think by adding a new configuration knob to control this (append vs. no-append)? Then we don't need any changes once we have adj-rib-out.

@ton31337 A new config knob would still mean either rib-in policy behavior would be wrong or the future rib-out behavior (based on operator's configured value). IMHO we shouldn't leave the behavior here to operator preference.

The current single-arg design already accommodates the future Adj-RIB-Out cleanly, as a future bmp_update_post_policy_adj_rib_out() would call bgp_packet_attribute(... for_bmp=false ...) which would make it fall into the regular eBGP/confed branches, and get the local-AS prepend exactly as the egress packet builder does.

That said, the arg for_bmp might look confusing at that point, and we can rename it to convey its purpose better. I can change it to something like skip_egress_xforms to make it unambiguous (where only adj-rib-in bmp policy would set it to true). What do you think?

[kemp-rv]

Just to concur, it looks like @kalash-nexthop has everything correct for what we would expect. I like the flag rename suggestion, but any name is fine in my book.

[ton31337]

Please squash these two commits (styling fix + actual fix) into a single one.

[kalash-nexthop]

Please squash these two commits (styling fix + actual fix) into a single one.

Done. Thanks @ton31337

[kalash-nexthop]

Three CI runs in a row have failed on different unrelated tests (bgp_soo, then ospf_rfc4222_dscp, then bgp_conditional_advertisement_track_peer). The same failures are hitting other open PRs — #22008, #21992, #21967, #21943 on bgp_soo, and #22009 on bgp_conditional_advertisement_track_peer. This seems to be a CI/test-side flakiness. CI used to pass earlier on this PR before I rebased today. Any advise @ton31337 ?

[ton31337]

bgp soo & friends should have been fixed in master already, so let's see what we have right now.

[eqvinox]

Quick update from talking with other BMP people: nobody seems to be entirely sure what the behavior should be in this case, and there is very little practical data.

[kemp-rv]

Quick update from talking with other BMP people: nobody seems to be entirely sure what the behavior should be in this case, and there is very little practical data.

I'd like to push back on this comment. People at CAIDA noticed this bug at the point it was introduced (years ago). I myself only recently saw the impact when checking on some of the data that RouteViews generates. So there is no confusion on our part. We are seeing the impact of the prepend bug in the data in our system currently.

Let me help you understand a little bit. You have a monitor. It collects BMP Monitor data. It is not pushing traffic around. Adding the asnum of the monitor in the pre-policy, post-policy-in, or loc-rib, as if it were part of a BGP UPDATE msg that came to the peer, makes no sense. So I would again say we are hopeful that the patch in this commit, or the equivalent, goes through.

adj-RIB-out is the only place you could argue for a prepend. And we're not talking about that case here.

[kalash-nexthop]

Three CI runs in a row have failed on different unrelated tests (bgp_soo, then ospf_rfc4222_dscp, then bgp_conditional_advertisement_track_peer). The same failures are hitting other open PRs — #22008, #21992, #21967, #21943 on bgp_soo, and #22009 on bgp_conditional_advertisement_track_peer. This seems to be a CI/test-side flakiness. CI used to pass earlier on this PR before I rebased today. Any advise @ton31337 ?

pipeline passes now.

[eqvinox]

Quick update from talking with other BMP people: nobody seems to be entirely sure what the behavior should be in this case, and there is very little practical data.

I'd like to push back on this comment. People at CAIDA noticed this bug at the point it was introduced (years ago). I myself only recently saw the impact when checking on some of the data that RouteViews generates. So there is no confusion on our part. We are seeing the impact of the prepend bug in the data in our system currently.

I poked some implementers (and one BMP operator) and you see the result above [for Adj-RIB-out. See end of this comment.] The response was basically "hmm, we never thought about that", to the degree that they didn't even know what their code does. No more and no less. I'm sad to hear that this is causing issues for your data collection.

Let me help you understand a little bit. You have a monitor. It collects BMP Monitor data. It is not pushing traffic around.

Sigh.

The tone here is rather condescending. I know who you are and respect your work, so I'm ignoring that. Still, it's not helpful for the discussion.

FYI: I wrote the original BMP code in FRR. I'm not perfect, my code has bugs, this is one of them. I do, however, understand well enough what BMP is, does, and how it's used.

Adding the asnum of the monitor in the pre-policy, post-policy-in, or loc-rib, as if it were part of a BGP UPDATE msg that came to the peer, makes no sense. So I would again say we are hopeful that the patch in this commit, or the equivalent, goes through.

adj-RIB-out is the only place you could argue for a prepend. And we're not talking about that case here.

You might've noticed I haven't previously commented on this PR. My going around asking people was triggered by the discussion of this PR on our weekly tech call. My understanding from the call was that this PR is for Adj-RIB-out. Funnily enough, this miscommunication might've happened because for Adj-RIB-in I could've answered that right there on the call. It made no sense to me that there'd be a discussion on that.

Anyway. I'm not sure what the uncertainty was that triggered the discussion on the call, I'm going to assume it's resolved.

I do hope we didn't lose a bug report about this (I couldn't find one), but if the CAIDA people knew about this for years and just didn't report it, that's… unfortunate. We can't fix bugs we're not aware of.

[kemp-rv]

Quick update from talking with other BMP people: nobody seems to be entirely sure what the behavior should be in this case, and there is very little practical data.

I'd like to push back on this comment. People at CAIDA noticed this bug at the point it was introduced (years ago). I myself only recently saw the impact when checking on some of the data that RouteViews generates. So there is no confusion on our part. We are seeing the impact of the prepend bug in the data in our system currently.

I poked some implementers (and one BMP operator) and you see the result above [for Adj-RIB-out. See end of this comment.] The response was basically "hmm, we never thought about that", to the degree that they didn't even know what their code does. No more and no less. I'm sad to hear that this is causing issues for your data collection.

Let me help you understand a little bit. You have a monitor. It collects BMP Monitor data. It is not pushing traffic around.

Sigh.

The tone here is rather condescending. I know who you are and respect your work, so I'm ignoring that. Still, it's not helpful for the discussion.

FYI: I wrote the original BMP code in FRR. I'm not perfect, my code has bugs, this is one of them. I do, however, understand well enough what BMP is, does, and how it's used.

Adding the asnum of the monitor in the pre-policy, post-policy-in, or loc-rib, as if it were part of a BGP UPDATE msg that came to the peer, makes no sense. So I would again say we are hopeful that the patch in this commit, or the equivalent, goes through.
adj-RIB-out is the only place you could argue for a prepend. And we're not talking about that case here.

You might've noticed I haven't previously commented on this PR. My going around asking people was triggered by the discussion of this PR on our weekly tech call. My understanding from the call was that this PR is for Adj-RIB-out. Funnily enough, this miscommunication might've happened because for Adj-RIB-in I could've answered that right there on the call. It made no sense to me that there'd be a discussion on that.

Anyway. I'm not sure what the uncertainty was that triggered the discussion on the call, I'm going to assume it's resolved.

I do hope we didn't lose a bug report about this (I couldn't find one), but if the CAIDA people knew about this for years and just didn't report it, that's… unfortunate. We can't fix bugs we're not aware of.

No issues here. We are nothing but appreciative for all the hard work on the project.

Sorry if that came across as condescending. I was just trying to be brief, and as clear as I could be to avoid any confusion about the thread topic. With the fix being so close to merge, I wanted to make sure that it was clear that there really are people who know what the issue is, and have a use for this particular fix.

Again, we appreciate all the hard work here.

[kalash-nexthop]

Hi @eqvinox, I appreciate the time and effort you spent in soliciting feedback from other bmp folks on this. From our (Nexthop systems) side, this issue is caught while executing a customer testcase, who clearly want the behavior of not prepending local-as to these BMP updates. Sadly, the RFC doesn't explicitly mention this the datapoint that I have here is anecdotal in that sense.

[eqvinox]

@kalash-nexthop you said you'd split it in 2 commits, are you still going to do that? Personally it's below my threshold for caring since the 32bit fix is 1 code line, but you did say yes you'll do that 😃

[eqvinox]

Sorry if that came across as condescending. I was just trying to be brief […]

No worries. If anything I was just making sure we don't get a slow creep in the community at large, better to say something on a tiny thing vs. having a big deal later after tone's drifted 😄

Again, we appreciate all the hard work here.

Thank you! Like I said I appreciate and respect your work as well! Fun fact: I'm writing this while attending RIPE 92, trying to make sure we keep up with the needs the DFZ operator community. The DFZ is hard enough to understand with all the monitoring and analysis tools we have, without them we'd be blundering around in the dark.

[kalash-nexthop]

@kalash-nexthop you said you'd split it in 2 commits, are you still going to do that? Personally it's below my threshold for caring since the 32bit fix is 1 code line, but you did say yes you'll do that 😃

@eqvinox Sorry I couldn't do it earlier. I didn't explain the issue I hit when I was trying to split this (although I briefly mentioned earlier in this PR that without the 32bit asn I couldn't get the CI for local-as related change to pass).

When I applied just the local-AS hunks, bgp_bmp topotests failed; adding the 32-bit force made them pass. My best guess was an interaction with the A-flag-never-set bug in FRR's BMP per-peer header during peer-flap triggered in router id change test. Maybe the test was flaky and folding 32bit asn change in this PR itself was indirectly making it go away.

Shall I try to split the PR and see if the topotests pass? Asking again as I saw two approvals on this already.

[eqvinox]

@kalash-nexthop you said you'd split it in 2 commits, are you still going to do that? Personally it's below my threshold for caring since the 32bit fix is 1 code line, but you did say yes you'll do that 😃

@eqvinox Sorry I couldn't do it earlier. I didn't explain the issue I hit when I was trying to split this (although I briefly mentioned earlier in this PR that without the 32bit asn I couldn't get the CI for local-as related change to pass).

When I applied just the local-AS hunks, bgp_bmp topotests failed; adding the 32-bit force made them pass. My best guess was an interaction with the A-flag-never-set bug in FRR's BMP per-peer header during peer-flap triggered in router id change test. Maybe the test was flaky and folding 32bit asn change in this PR itself was indirectly making it go away.

Shall I try to split the PR and see if the topotests pass? Asking again as I saw two approvals on this already.

commit != PR, looks like we had a misunderstanding :) - you can just split it into 2 commits and leave it in the same PR.

[kalash-nexthop]

@kalash-nexthop you said you'd split it in 2 commits, are you still going to do that? Personally it's below my threshold for caring since the 32bit fix is 1 code line, but you did say yes you'll do that 😃

@eqvinox Sorry I couldn't do it earlier. I didn't explain the issue I hit when I was trying to split this (although I briefly mentioned earlier in this PR that without the 32bit asn I couldn't get the CI for local-as related change to pass).
When I applied just the local-AS hunks, bgp_bmp topotests failed; adding the 32-bit force made them pass. My best guess was an interaction with the A-flag-never-set bug in FRR's BMP per-peer header during peer-flap triggered in router id change test. Maybe the test was flaky and folding 32bit asn change in this PR itself was indirectly making it go away.
Shall I try to split the PR and see if the topotests pass? Asking again as I saw two approvals on this already.

commit != PR, looks like we had a misunderstanding :) - you can just split it into 2 commits and leave it in the same PR.

Ah right, thanks for clarifying. Split this into two commits now.

[kalash-nexthop]

@ton31337 @eqvinox The PR is ready to be merged, but the merge workflow needs to be approved by a maintainer. Do I need to contact someone else or would you be able to trigger that workflow?

[kalash-nexthop]

Thanks a lot @eqvinox !