-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add BGP redistribution in SRv6 BGP #18396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add BGP redistribution in SRv6 BGP #18396
Conversation
ton31337
left a comment
There was a problem hiding this 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IPV6_ADDR_CMP()
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop this, useless.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop this, useless.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop this, useless.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop this.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
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: The valid VRF routes from CEs still result in invalid entries in VPN tables for me with this change. |
|
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.. |
f506037 to
61ca422
Compare
riw777
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
|
looks like there might still be some issues in the topo test? |
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 |
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); |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
|
looks like failures are related ... |
61ca422 to
23195c0
Compare
23195c0 to
cc38474
Compare
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]>
cc38474 to
03b57b4
Compare
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]>
ton31337
left a comment
There was a problem hiding this 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]>
done. unified config should be done separately, there are many other places.. |
|
fwiw - these latest changes work great in my test network. Thanks @pguibert6WIND |
ton31337
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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]>
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]>
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]>
Add BGP redistribution from BGP peers behind PE devices, in SRv6 BGP