Skip to content

bgpd: Fix SRv6 SID export route-map update not taking effect#21283

Merged
donaldsharp merged 2 commits intoFRRouting:masterfrom
cscarpitta:fix_srv6_bgp_grt_rmap_change
Mar 22, 2026
Merged

bgpd: Fix SRv6 SID export route-map update not taking effect#21283
donaldsharp merged 2 commits intoFRRouting:masterfrom
cscarpitta:fix_srv6_bgp_grt_rmap_change

Conversation

@cscarpitta
Copy link
Contributor

When sid export ... route-map <name> is reconfigured with a different route-map on an already-configured SID export, the new route-map is silently ignored. The code skips the early-return (rmap_str != rmap_name), but then calls bgp_srv6_unicast_announce() without updating rmap_name, so the old route-map remains in effect.

Fix this by replacing rmap_name before triggering the re-announce: decrement the reference count on the old route-map, free its name string, assign the new name, and increment the reference count on the new route-map.

@frrbot frrbot bot added bgp tests Topotests, make check, etc labels Mar 22, 2026
When `sid export ... route-map <name>` is reconfigured with a
different route-map on an already-configured SID export, the new
route-map is silently ignored. The code skips the early-return
(rmap_str != rmap_name), but then calls
bgp_srv6_unicast_announce() without updating rmap_name, so the
old route-map remains in effect.

Fix this by replacing rmap_name before triggering the re-announce:
decrement the reference count on the old route-map, free its name
string, assign the new name, and increment the reference count on
the new route-map.

Signed-off-by: Carmine Scarpitta <[email protected]>
@greptile-apps
Copy link

greptile-apps bot commented Mar 22, 2026

Greptile Summary

This PR fixes a bug where reconfiguring sid export ... route-map <name> with a different route-map on an already-configured SID export silently kept the old route-map in effect. The fix correctly updates rmap_name — decrementing the old route-map's reference count, freeing its name string, duplicating the new name, and incrementing the new route-map's reference count — before calling bgp_srv6_unicast_announce(). A new topotest validates the fix end-to-end.

Changes:

  • bgpd/bgp_vty.c: In the sid_export DEFPY handler, adds the missing rmap_name update steps (ref decrement, free, strdup, ref increment) in the "already configured, rmap changed" branch before triggering re-announcement.
  • tests/topotests/bgp_srv6_unicast/test_bgp_srv6_unicast.py: Adds test_bgp_srv6_sid_rmap_update() which swaps the route-map on a live SID export and confirms the policy change propagates to peers R2 and R3, then restores the original and confirms recovery.

Confidence Score: 5/5

  • This PR is safe to merge — it is a minimal, well-targeted bug fix with correct memory and reference-count management, mirroring the established pattern used elsewhere in the same function.
  • The fix is logically sound: at the point the new code executes, rmap_str is guaranteed non-NULL (the early-return gate handles the NULL case), and the if (bgp->srv6_unicast[afi].rmap_name) guard safely covers the case where no previous route-map was set. The new code exactly mirrors the decrement/free/strdup/increment pattern used in the no branch and the initial-configuration path in the same function. The topotest exercises the precise failure scenario from the bug report and also verifies rollback. No unrelated code is touched.
  • No files require special attention.

Important Files Changed

Filename Overview
bgpd/bgp_vty.c Inserts proper rmap_name update (decrement old ref, free, strdup new, increment new ref) before re-announcing, fixing the bug where a reconfigured route-map was silently ignored.
tests/topotests/bgp_srv6_unicast/test_bgp_srv6_unicast.py Adds test_bgp_srv6_sid_rmap_update() to verify that replacing a route-map on an existing SID export actually takes effect, directly exercising the fixed code path.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[sid export route-map VTY command] --> B{SID already configured?}
    B -- No --> C[Initial setup: XSTRDUP rmap_name, increment ref, ensure SID]
    B -- Yes --> D{rmap_str same as current rmap_name?}
    D -- Same or NULL --> E[Return CMD_SUCCESS no-op]
    D -- Different --> F[Decrement old rmap ref count]
    F --> G[XFREE old rmap_name]
    G --> H[XSTRDUP new rmap_name]
    H --> I[Increment new rmap ref count]
    I --> J[bgp_srv6_unicast_announce with new route-map]
    J --> K[Return CMD_SUCCESS - new route-map in effect]
Loading

Reviews (1): Last reviewed commit: "tests: Add test for sid export route-map..." | Re-trigger Greptile

Add test_bgp_srv6_sid_rmap_update() to verify that replacing an
already-configured sid export route-map with a different one
correctly applies the new policy.

The test reconfigures R1 with a new route-map (filter2) that
excludes 10.0.0.1/32 from SID assignment, confirms the prefix
loses its SRv6 encapsulation on R2 and becomes uninstalled on
R3, then restores the original route-map (filter) and verifies
that SRv6 encapsulation is re-established on both peers.

Signed-off-by: Carmine Scarpitta <[email protected]>
@cscarpitta cscarpitta force-pushed the fix_srv6_bgp_grt_rmap_change branch from 78b5149 to 141ca52 Compare March 22, 2026 09:02
@donaldsharp donaldsharp merged commit ae7c04c into FRRouting:master Mar 22, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bgp master rebase PR needs rebase size/M tests Topotests, make check, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants