Skip to content

Conversation

@pguibert6WIND
Copy link
Member

Add BGP redistribution from BGP peers behind PE devices, in SRv6 BGP

Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Please drop all those unnecessary stuff from frr.conf everywhere.

bgpd/bgp_nht.c Outdated
bgp_nexthop->vpn_policy[afi].tovpn_sid_locator &&
(bgp_nexthop->vpn_policy[afi].tovpn_sid || bgp_nexthop->tovpn_sid) &&
attr->srv6_l3vpn &&
0 == memcmp(&attr->srv6_l3vpn->sid,
Copy link
Member

Choose a reason for hiding this comment

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

IPV6_ADDR_CMP()

Copy link
Member Author

Choose a reason for hiding this comment

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

code changed, I use prefix_match() instead; it is better.
please re-review

@@ -0,0 +1,24 @@
frr defaults traditional
!
hostname ce1
Copy link
Member

Choose a reason for hiding this comment

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

Drop this, useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

frr defaults traditional
!
hostname ce1
password zebra
Copy link
Member

Choose a reason for hiding this comment

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

Drop this, useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

hostname ce1
password zebra
!
log stdout notifications
Copy link
Member

Choose a reason for hiding this comment

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

Drop this, useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

password zebra
!
log stdout notifications
log commands
Copy link
Member

Choose a reason for hiding this comment

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

Drop this.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

!
log stdout notifications
log commands
log file bgpd.log
Copy link
Member

Choose a reason for hiding this comment

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

Drop this.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@jvoss
Copy link
Contributor

jvoss commented Mar 15, 2025

I was hoping this would resolve the issues I am experiencing in #18322. However I believe local SID is still being validated against the BGP nexthop cache; at least in my case (topotest: bgp_srv6l3vpn_to_bgp_vrf4 from my PR).

The valid VRF routes from CEs still result in invalid entries in VPN tables for me with this change.

@ton31337
Copy link
Member

@jvoss is this PR trying to solve the same issue as #18322?

@jvoss
Copy link
Contributor

jvoss commented Mar 16, 2025

@jvoss is this PR trying to solve the same issue as #18322?

I believe it is the intention reviewing the topotest configuration. I am trying to use SRv6 in a fairly standard L3VPN configuration where CEs peer within VRFs to advertise their own routes.

However, this did not work in my testing and still left routes exported to the VPN tables invalid.

@pguibert6WIND
Copy link
Member Author

@jvoss is this PR trying to solve the same issue as #18322?

I believe it is the intention reviewing the topotest configuration. I am trying to use SRv6 in a fairly standard L3VPN configuration where CEs peer within VRFs to advertise their own routes.

However, this did not work in my testing and still left routes exported to the VPN tables invalid.

@jvoss , this is certainly due to the SID allocation mecanism. There are no flags in BGP that tell that a SID allocation is in progress, and no hooks for re-calling nexthop registration..

@pguibert6WIND pguibert6WIND force-pushed the srv6l3vpn_to_bgp_vrf_redistribute branch from f506037 to 61ca422 Compare March 18, 2025 07:24
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

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

looks good

@riw777
Copy link
Member

riw777 commented Mar 18, 2025

looks like there might still be some issues in the topo test?

@jvoss
Copy link
Contributor

jvoss commented Mar 18, 2025

@jvoss , this is certainly due to the SID allocation mecanism. There are no flags in BGP that tell that a SID allocation is in progress, and no hooks for re-calling nexthop registration..

Thanks @pguibert6WIND, that makes sense. FWIW - I re-built from this PR and tested in an actual test network instead of just topotests. It works for me on initial startup, however if a CE clears the session, when it is re-established the routes are invalid again.

As a quick patch, if I fall back to just marking nexthops valid in leak_update_nexthop_valid() when a SID is being attached, then it is no longer an issue. However this may allow invalid VRF routes to be marked valid in VPN. We may be able to check ultimate validity here too but in my experiments it did not seem to re-leak the route if the validity changed later in the source table (but I could be wrong).

bgpd/bgp_zebra.c Outdated
bgp_vrf->vpn_policy[AFI_IP6].tovpn_sid_transpose_label =
label;
UNSET_FLAG(bgp_vrf->vpn_policy[AFI_IP6].flags,
BGP_VPN_POLICY_TOVPN_SID_REQUEST_IN_PROGRESS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we unset the BGP_VPN_POLICY_TOVPN_SID_REQUEST_IN_PROGRESS flag also when SID allocation fails (case ZAPI_SRV6_SID_FAIL_ALLOC)?

Copy link
Member Author

@pguibert6WIND pguibert6WIND Mar 21, 2025

Choose a reason for hiding this comment

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

actually, SID allocation failure is not the culprit; all is related to the decision: which nexthop to use, and when should we consider to invalidate exported vpn prefixes. Please @cscarpitta , re-review.

@riw777
Copy link
Member

riw777 commented Mar 18, 2025

looks like failures are related ...

@pguibert6WIND pguibert6WIND force-pushed the srv6l3vpn_to_bgp_vrf_redistribute branch from 61ca422 to 23195c0 Compare March 21, 2025 09:45
@pguibert6WIND pguibert6WIND marked this pull request as draft March 21, 2025 09:45
@pguibert6WIND pguibert6WIND force-pushed the srv6l3vpn_to_bgp_vrf_redistribute branch from 23195c0 to cc38474 Compare March 21, 2025 15:34
@pguibert6WIND pguibert6WIND marked this pull request as ready for review March 21, 2025 15:35
@pguibert6WIND pguibert6WIND marked this pull request as draft March 21, 2025 21:54
pguibert6WIND and others added 9 commits March 24, 2025 09:17
Use the unified configuration for bgp_srv6l3vpn_to_bgp_vrf test.

Signed-off-by: Philippe Guibert <[email protected]>
The aspath value has no need to be controlled. Unset the bgp capability
to send aspath information to zebra.

Signed-off-by: Philippe Guibert <[email protected]>
Use experimental AS values to play the test.
Add BGP peering on CEs, and use the default-originate functionality on
each PE facing CPEs.

Signed-off-by: Philippe Guibert <[email protected]>
…3vpn

Add a BGP update in CE1 for redistribution. The expectation is that this
BGP update will be leaked to the L3VPN. Reversely, if the locator is
unset, the L3VPN prefix will be invalidated.

Signed-off-by: Philippe Guibert <[email protected]>
This test ensures route redistribution across an srv6 VPN network
is well taken into account.

Signed-off-by: Jonathan Voss <[email protected]>
Signed-off-by: Philippe Guibert <[email protected]>
The resulting VPN prefix of a BGP route from a L3VPN in an srv6 setup
is not advertised to remote devices.

> r1# show bgp ipv6 vpn
> BGP table version is 2, local router ID is 1.1.1.1, vrf id 0
> Default local pref 100, local AS 65500
> Status codes:  s suppressed, d damped, h history, u unsorted, * valid, > best, = multipath,
>                i internal, r RIB-failure, S Stale, R Removed
> Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self
> Origin codes:  i - IGP, e - EGP, ? - incomplete
> RPKI validation codes: V valid, I invalid, N Not found
>
>      Network          Next Hop            Metric LocPrf Weight Path
> Route Distinguisher: 1:10
>      2011:1::/64      2001:1::2@6<             0    100      0 i
>     UN=2001:1::2 EC{99:99} label=4096 sid=2001:db8:1:1:: sid_structure=[40,24,8,0] type=bgp, subtype=5

What happens is that the SID of this BGP update is used as nexthop.
Consequently, the prefix is not valid because of nexthop unreachable.
obviously the locator prefix is not reachable in that L3VRF, and the
real nexthop 2001:1::2 should be used.

> r1# show bgp vrf vrf10 nexthop  detail
> Current BGP nexthop cache:
>  2001:db8:1:1:100:: invalid, #paths 1
>   Last update: Fri Mar 14 21:18:59 2025
>   Paths:
>     2/3 2011:1::/64 RD 1:10 VRF default flags 0x4000

Fix this by considering the SID of a given BGP update, only if the SID
is not ours.

Signed-off-by: Philippe Guibert <[email protected]>
When detaching the locator from the main BGP instance, the used SIDs
and locators are removed from the srv6 per-afi or per-vrf contects.
Under those conditions, it is not possible to attempt to export new
VPN updates. Do invalidate the nexthop for leaking.

Restrict the control for exported VPN prefixes and not for unicast
imported prefixes.

Signed-off-by: Philippe Guibert <[email protected]>
When exporting a VPN SRv6 route, the path may not be considered valid if
the nexthop is not valid. This is the case when the 'nexthop vpn export'
command is used. The below example illustrates that the VPN path to
2001:1::/64 is not selected, as the expected nexthop to find in vrf10 is
the one configured:

> # show running-config
> router bgp 1 vrf vrf10
>  address-family ipv6 unicast
>   nexthop vpn export 2001::1

> # show bgp ipv6 vpn
> [..]
> Route Distinguisher: 1:10
>      2001:1::/64      2001::1@4                0             0 65001 i
>     UN=2001::1 EC{99:99} label=16 sid=2001:db8:1:1:: sid_structure=[40,24,16,0] type=bgp, subtype=5

The analysis indicates that the 2001::1 nexthop is considered.

> 2025/03/20 21:47:53.751853 BGP: [RD1WY-YE9EC] leak_update: entry: leak-to=VRF default, p=2001:1::/64, type=10, sub_type=0
> 2025/03/20 21:47:53.751855 BGP: [VWNP2-DNMFV] Found existing bnc 2001::1/128(0)(VRF vrf10) flags 0x82 ifindex 0 #paths 2 peer 0x0, resolved prefix UNK prefix
> 2025/03/20 21:47:53.751856 BGP: [VWC2R-4REXZ] leak_update_nexthop_valid: 2001:1::/64 nexthop is not valid (in VRF vrf10)
> 2025/03/20 21:47:53.751857 BGP: [HX87B-ZXWX9] leak_update: ->VRF default: 2001:1::/64: Found route, no change

Actually, to check the nexthop validity, only the source path in the VRF
has the correct nexthop. Fix this by reusing the source path information
instead of the current one.

> 2025/03/20 22:43:51.703521 BGP: [RD1WY-YE9EC] leak_update: entry: leak-to=VRF default, p=2001:1::/64, type=10, sub_type=0
> 2025/03/20 22:43:51.703523 BGP: [VWNP2-DNMFV] Found existing bnc fe80::b812:37ff:fe13:d441/128(0)(VRF vrf10) flags 0x87 ifindex 0 #paths 2 peer 0x0, resolved prefix fe80::/64
> 2025/03/20 22:43:51.703525 BGP: [VWC2R-4REXZ] leak_update_nexthop_valid: 2001:1::/64 nexthop is valid (in VRF vrf10)
> 2025/03/20 22:43:51.703526 BGP: [HX87B-ZXWX9] leak_update: ->VRF default: 2001:1::/64: Found route, no change

Signed-off-by: Philippe Guibert <[email protected]>
When srv6 is disabled due to misconfiguration, exported VPN prefixes
are invalidated, except for the ones that have their nexthop modified
with the 'nexthop vpn export' command. The previous commit also
invalidates those vpn prefixes.

Apply the changes to the test by not considering some prefixes as
selected. Enforce the expected route count.

Signed-off-by: Philippe Guibert <[email protected]>
@pguibert6WIND pguibert6WIND force-pushed the srv6l3vpn_to_bgp_vrf_redistribute branch from cc38474 to 03b57b4 Compare March 24, 2025 08:19
@pguibert6WIND pguibert6WIND marked this pull request as ready for review March 24, 2025 08:20
Add more control on the expected outputs, by using an exact json
comparison.

Signed-off-by: Philippe Guibert <[email protected]>
Assuming attr is null, a dereference can happen in the function
make_prefix(). Add the protection over attr before accessing the
variable.

Signed-off-by: Philippe Guibert <[email protected]>
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Please drop all these unnecessary options (hostname, password, log ...) from topotest configs.

Many useless commande are still persistent in the
bgp_srv6l3vpn_to_bgp_vrf tests. Remove the useless commands.

Signed-off-by: Philippe Guibert <[email protected]>
@pguibert6WIND
Copy link
Member Author

Please drop all these unnecessary options (hostname, password, log ...) from topotest configs.

done. unified config should be done separately, there are many other places..

@jvoss
Copy link
Contributor

jvoss commented Mar 26, 2025

fwiw - these latest changes work great in my test network. Thanks @pguibert6WIND

Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

LGTM

@ton31337 ton31337 requested a review from cscarpitta April 3, 2025 10:40
@riw777 riw777 merged commit ab67e55 into FRRouting:master Apr 3, 2025
13 checks passed
cscarpitta added a commit to cscarpitta/sonic-buildimage that referenced this pull request Oct 21, 2025
Port the following fixes from FRR mainline:
- zebra: Add CLI to display SRv6 SIDs allocated (FRRouting/frr#16836)
- bgpd: add usid behavior for bgp srv6 instructions (FRRouting/frr#18611)
- bgpd: Implement Link-Local Next Hop capability (FRRouting/frr#17871)
- Add BGP redistribution in SRv6 BGP (FRRouting/frr#18396)
- bgpd: Add support for BGP to use SRv6 SID in an explicit way (FRRouting/frr#18519)
- lib, staticd, isisd: add B6.ENCAPS codepoint extensions (FRRouting/frr#18597)
- zebra: Provide SID value when sending SRv6 SID release notify message (FRRouting/frr#18971)
- zebra: Cleanup SRv6 output of show running-config (FRRouting/frr#18970)
- zebra: Free up tt when failure mode happens (FRRouting/frr@0521673)
- SRv6: Add support for multiple SRv6 locators (FRRouting/frr#18806)

Signed-off-by: Carmine Scarpitta <[email protected]>
cscarpitta added a commit to cscarpitta/sonic-buildimage that referenced this pull request Oct 21, 2025
Port the following fixes from FRR mainline:
- zebra: Add CLI to display SRv6 SIDs allocated (FRRouting/frr#16836)
- bgpd: add usid behavior for bgp srv6 instructions (FRRouting/frr#18611)
- bgpd: Implement Link-Local Next Hop capability (FRRouting/frr#17871)
- Add BGP redistribution in SRv6 BGP (FRRouting/frr#18396)
- bgpd: Add support for BGP to use SRv6 SID in an explicit way (FRRouting/frr#18519)
- lib, staticd, isisd: add B6.ENCAPS codepoint extensions (FRRouting/frr#18597)
- zebra: Provide SID value when sending SRv6 SID release notify message (FRRouting/frr#18971)
- zebra: Cleanup SRv6 output of show running-config (FRRouting/frr#18970)
- zebra: Free up tt when failure mode happens (FRRouting/frr@0521673)
- SRv6: Add support for multiple SRv6 locators (FRRouting/frr#18806)

Signed-off-by: Carmine Scarpitta <[email protected]>
cscarpitta added a commit to cscarpitta/sonic-buildimage that referenced this pull request Oct 27, 2025
Port the following fixes from FRR mainline:
- zebra: Add CLI to display SRv6 SIDs allocated (FRRouting/frr#16836)
- bgpd: add usid behavior for bgp srv6 instructions (FRRouting/frr#18611)
- bgpd: Implement Link-Local Next Hop capability (FRRouting/frr#17871)
- Add BGP redistribution in SRv6 BGP (FRRouting/frr#18396)
- bgpd: Add support for BGP to use SRv6 SID in an explicit way (FRRouting/frr#18519)
- lib, staticd, isisd: add B6.ENCAPS codepoint extensions (FRRouting/frr#18597)
- zebra: Provide SID value when sending SRv6 SID release notify message (FRRouting/frr#18971)
- zebra: Cleanup SRv6 output of show running-config (FRRouting/frr#18970)
- zebra: Free up tt when failure mode happens (FRRouting/frr@0521673)
- SRv6: Add support for multiple SRv6 locators (FRRouting/frr#18806)
- zebra: Fix SRv6 explicit SID allocation to use the provided locator (FRRouting/frr@a9044d2)
Signed-off-by: Carmine Scarpitta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants