bgpd: vpn - vrf route leaking#1739
Conversation
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2547/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
New warnings:
Static Analysis warning summary compared to base:
21 Static Analyzer issues remaining.See details at |
|
The "new" static analysis issue at ripd/ripd.c line 2325 seems to have already been addressed in commit aea175a |
🚧 Basic BGPD CI results: Partial FAILURE, 1 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2552/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
New warnings:
Static Analysis warning summary compared to base:
21 Static Analyzer issues remaining.See details at |
pguibert6WIND
left a comment
There was a problem hiding this comment.
first walk through the changes.
I guess there will be some more zebra changes ?
| } | ||
| #endif | ||
| zlog_debug("%s: withdraw vpn->vrf", __func__); /* TBD delete me */ | ||
| /* vpn -> vrf (happend within bgp but we hijack redist bits */ |
There was a problem hiding this comment.
this trace will be cleaned I suppose.
| ECOMMUNITY_FORMAT_ROUTE_MAP, | ||
| ECOMMUNITY_ROUTE_TARGET); | ||
| vty_out(vty, " rt fromvpn %s\n", b); | ||
| XFREE(MTYPE_ECOMMUNITY_STR, b); |
There was a problem hiding this comment.
was there an agreement on the vty naming convention ?
rt from vpn <=> rt export
rt to vpn <=> rt import
?
There was a problem hiding this comment.
I do not think we achieved consensus. This is placeholder CLI so we can exercise code until we finalize syntax.
bgpd/bgp_mplsvpn.c
Outdated
| info_vrf->type); | ||
|
|
||
| if (info_vrf->type == ZEBRA_ROUTE_BGP_VPN) { | ||
| /* loop check! */ |
There was a problem hiding this comment.
useless braces.
you can simplify
| #undef VPN_PREFIXLEN_MIN_BYTES | ||
| } | ||
|
|
||
| static int ecom_intersect(struct ecommunity *e1, struct ecommunity *e2) |
There was a problem hiding this comment.
Could this be moved to bgp_ecommunity.c and Could we please add a little description of what the API is suppose to do ?
There was a problem hiding this comment.
Using ecom_intersect() is fine but as a second step, we should do some optimization. In EVPN, we have built a hash of VRFs that are interested in a particular RT and so there is just a hash lookup (per RT in route) followed by getting the interested VRFs. @mkanjari or I could help do the optimization too after initial commit of this code.
There was a problem hiding this comment.
I agree that optimization in this way is a good idea. After this round is committed, let's revisit.
bgpd/bgp_mplsvpn.c
Outdated
|
|
||
| bgp_attr_unintern(&new_attr); | ||
| if (debug) | ||
| zlog_debug("%s: Found route, no change", |
There was a problem hiding this comment.
Can we have a more specific debug statement ? This is pretty generic and won't give any information while debugging. Or if this is not necessary, can we please remove it ?
There was a problem hiding this comment.
It gives an indication of which code branch was taken. What additional information would you like to see?
There was a problem hiding this comment.
some additional information like the prefix at the least ? generally we will grep for a specific prefix or some attr while debugging. It will be helpful if we print that information too in the same line.
There was a problem hiding this comment.
It sounds like a good idea. Let me look at these more closely...
bgpd/bgp_mplsvpn.c
Outdated
| bgp_unlock_node(bn); | ||
|
|
||
| if (debug) | ||
| zlog_debug("%s: Found route, changed attr", __func__); |
There was a problem hiding this comment.
Same as above, we need a more spefic debug statement here.
| bgp_unlock_node(bn); | ||
| bgp_process(bgp, bn, afi, safi); | ||
|
|
||
| if (debug) |
|
@paulzlabn - I have gone over the code flow pretty completely. The code is pretty easy to follow! Well done! I will refrain from comments on the CLI since we're discussing that internally - except for one point below. I have a few general comments below and a few specific comments which I'll include inline.
|
bgpd/bgp_route.c
Outdated
| bgp_aggregate_increment(bgp, p, ri, afi, safi); | ||
| bgp_process(bgp, rn, afi, safi); | ||
|
|
||
| if (SAFI_UNICAST == safi |
There was a problem hiding this comment.
This check seems wrong, the safi should be SAFI_MPLS_VPN here
There was a problem hiding this comment.
What safi is supposed to be here? @paulzlabn I see agreement but I don't see the change here
There was a problem hiding this comment.
@donaldsharp In bgp_static_withdraw_safi() and bgp_static_update_safi(), it should be SAFI_MPLS_VPN because it applies to routes originating in ipv4|ipv6 vpn and leaking to a unicast vrf. In bgp_static_update() and bgp_static_withdraw(), it should be SAFI_UNICAST because it applies to routes that originate in a unicast vrf and leaking to ipv4|ipv6 vpn.
bgpd/bgp_mplsvpn.c
Outdated
|
|
||
| for (bi = bn->info; bi; bi = bi->next) { | ||
|
|
||
| if (bi->type == ZEBRA_ROUTE_BGP_VPN) |
There was a problem hiding this comment.
This is mentioned in my general comment too, but in addition, this won't work for VRF-route-leaking because the route exported from (one) VRF to the VPN table is of type ZEBRA_ROUTE_BGP_VPN and this check will prevent it being imported into another VRF - unless I misunderstood.
There was a problem hiding this comment.
Agreed. For now I have added a check also between the source and destination vrf, which should permit vrf1->vpn->vrf2.
bgpd/bgp_vty.c
Outdated
| if (nhself) { | ||
| UNSET_FLAG(bgp->vpn_policy[afi].flags, | ||
| BGP_VPN_POLICY_TOVPN_NEXTHOP_SET); | ||
| continue; |
There was a problem hiding this comment.
This "continue" seems incorrect. Don't we have to do the vpn_leak_postchange() even if the nexthop is set to "self"?
| } | ||
|
|
||
| /* Set originator ID to "me" */ | ||
| SET_FLAG(static_attr.flag, ATTR_FLAG_BIT(BGP_ATTR_ORIGINATOR_ID)); |
There was a problem hiding this comment.
I thought the use/need of the Originator ID is only for Route Reflection. Why is it being set here?
There was a problem hiding this comment.
I believe that the originator id is only included in the outgoing update when bgpd is acting as a RR:
bgp_attr.c:
3078 /* Route Reflector. */
3079 if (peer->sort == BGP_PEER_IBGP && from
3080 && from->sort == BGP_PEER_IBGP) {
3081 /* Originator ID. */
3082 stream_putc(s, BGP_ATTR_FLAG_OPTIONAL);
3083 stream_putc(s, BGP_ATTR_ORIGINATOR_ID);
3084 stream_putc(s, 4);
3085
3086 if (attr->flag & ATTR_FLAG_BIT(BGP_ATTR_ORIGINATOR_ID))
3087 stream_put_in_addr(s, &attr->originator_id);
3088 else
3089 stream_put_in_addr(s, &from->remote_id);
imported/exported routes still have their original peer value, which can lead to the wrong originator id when we act as RR (problem we encountered some years ago in the vnc/rfapi code). Always setting it seems to do the right thing. Does this approach break anything?
There was a problem hiding this comment.
It seems okay to always set the originator id -- it is generally ignored except in the case of an RR. Might be worth having @dwalton76 comment here, however, to be certain.
|
One more general comment. The calls to vpn_leak_from_vrf_update()/withdraw() is missing in the route redistribute code path (say, when 'static' or 'ospf' route in a VRF is redistributed into BGP). |
|
Vivek, thank you for the detailed comments. I will follow up to your comments on specific code blocks individually, but here are responses to your numbered items:
In case (a), the current code tests bi->extra->parent against (struct bgp *)bgp_vrf, so the type check is not critical. However, we should be able to flag VPN RIB routes that came from a vrf in the "show route" output somehow. Also, if I stop abusing bi->extra->parent (per your comment 3), there needs to be some way for the code to identify VPN RIB routes that were imported from a specific vrf. Maybe a new vrf field in struct bgp_info_extra? In case (b), if we have a BGP_ROUTE_VPN sub_type, that would suffice (see vpn_leak_to_vrf_withdraw_all())
b) bi->extra->parent: Although I don't see a conflict with the existing code (maybe there is one I don't see), I agree that putting the source bgp_vrf pointer there is not the original purpose. If nobody minds the extra memory usage, I could add a new field to struct bgp_info_extra.
|
|
Hi Vivek,
With respect to the point below - a typical L3VPN use case is to fully
interconnect customer sites so all route export is just what is needed.
I certainly agree that this isn't the only use case and often only a
select set of routes should be exported/redistributed -- and this is
supported via routemaps. Make sense?
Lou
…On 2/15/2018 8:21 AM, Vivek Venkatraman wrote:
The code attempts to export all routes (i.e., route entry/info) from
VRF table to VPN table and vice versa (based on configs). This is
debatable, it may be ok to just export the selected path.
|
Thank you for catching that, fixing now... |
|
@vivek-cumulus Here is a proposal to do away with ZEBRA_ROUTE_BGP_VPN and parent = bgp_vrf |
🚧 Basic BGPD CI results: Partial FAILURE, 1 tests failedResults table
For details, please contact louberger |
|
@pguibert6WIND @qlyoung where are we on the review? @paulzlabn can you update to get rid of the conflict? |
|
I just realized there were some more comments from last week which I hadn't addressed. Changeset incoming... |
🛑 Basic BGPD CI results: FAILUREResults table
For details, please contact louberger |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedFedora24 amd64 build: FailedMake failed for Fedora24 amd64 build: Fedora24 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI015BUILD/config.status/config.status FreeBSD11 amd64 build: FailedMake failed for FreeBSD11 amd64 build: FreeBSD11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI009BUILD/config.status/config.status NetBSD7 amd64 build: FailedMake failed for NetBSD7 amd64 build: NetBSD7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI012BUILD/config.status/config.status Ubuntu1604 amd64 build: FailedMake failed for Ubuntu1604 amd64 build: Ubuntu1604 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI014BUILD/config.status/config.status Ubuntu 16.04 i386: FailedMake failed for Ubuntu 16.04 i386: Ubuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/U1604I386/config.status/config.status NetBSD6 amd64 build: FailedMake failed for NetBSD6 amd64 build: NetBSD6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI007BUILD/config.status/config.status CentOS7 amd64 build: FailedMake failed for CentOS7 amd64 build: CentOS7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI005BUILD/config.status/config.status Debian8 amd64 build: FailedMake failed for Debian8 amd64 build: Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI008BLD/config.status/config.status CentOS6 amd64 build: FailedMake failed for CentOS6 amd64 build: CentOS6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI006BUILD/config.status/config.status FreeBSD10 amd64 build: FailedMake failed for FreeBSD10 amd64 build: FreeBSD10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI003BUILD/config.status/config.status FreeBSD9 amd64 build: FailedMake failed for FreeBSD9 amd64 build: FreeBSD9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI004BUILD/config.status/config.status Debian9 amd64 build: FailedDebian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI021BUILD/config.status/config.status OmniOS amd64 build: FailedMake failed for OmniOS amd64 build: OmniOS amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI010BUILD/config.status/config.status Ubuntu1404 amd64 build: FailedMake failed for Ubuntu1404 amd64 build: Ubuntu1404 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI001BUILD/config.status/config.status OpenBSD60 amd64 build: FailedMake failed for OpenBSD60 amd64 build: OpenBSD60 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI011BUILD/config.status/config.status Ubuntu1204 amd64 build: FailedMake failed for Ubuntu1204 amd64 build: Ubuntu1204 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI002BUILD/config.status/config.status Warnings Generated during build:Checkout code: Successful with additional warnings:Fedora24 amd64 build: FailedMake failed for Fedora24 amd64 build: Fedora24 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI015BUILD/config.status/config.status FreeBSD11 amd64 build: FailedMake failed for FreeBSD11 amd64 build: FreeBSD11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI009BUILD/config.status/config.status NetBSD7 amd64 build: FailedMake failed for NetBSD7 amd64 build: NetBSD7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI012BUILD/config.status/config.status Ubuntu1604 amd64 build: FailedMake failed for Ubuntu1604 amd64 build: Ubuntu1604 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI014BUILD/config.status/config.status Ubuntu 16.04 i386: FailedMake failed for Ubuntu 16.04 i386: Ubuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/U1604I386/config.status/config.status NetBSD6 amd64 build: FailedMake failed for NetBSD6 amd64 build: NetBSD6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI007BUILD/config.status/config.status CentOS7 amd64 build: FailedMake failed for CentOS7 amd64 build: CentOS7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI005BUILD/config.status/config.status Debian8 amd64 build: FailedMake failed for Debian8 amd64 build: Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI008BLD/config.status/config.status CentOS6 amd64 build: FailedMake failed for CentOS6 amd64 build: CentOS6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI006BUILD/config.status/config.status FreeBSD10 amd64 build: FailedMake failed for FreeBSD10 amd64 build: FreeBSD10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI003BUILD/config.status/config.status FreeBSD9 amd64 build: FailedMake failed for FreeBSD9 amd64 build: FreeBSD9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI004BUILD/config.status/config.status Debian9 amd64 build: FailedDebian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI021BUILD/config.status/config.status OmniOS amd64 build: FailedMake failed for OmniOS amd64 build: OmniOS amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI010BUILD/config.status/config.status Ubuntu1404 amd64 build: FailedMake failed for Ubuntu1404 amd64 build: Ubuntu1404 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI001BUILD/config.status/config.status OpenBSD60 amd64 build: FailedMake failed for OpenBSD60 amd64 build: OpenBSD60 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI011BUILD/config.status/config.status Ubuntu1204 amd64 build: FailedMake failed for Ubuntu1204 amd64 build: Ubuntu1204 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2903/artifact/CI002BUILD/config.status/config.status |
mwinter-osr
left a comment
There was a problem hiding this comment.
Executed full RFC compliance for OSPFv2, OSPFv3, ISIS IPv4 & IPv6, BGP4 IPv4 & IPv6, LDP and all good (same as master & 4.0)
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedFreeBSD11 amd64 build: Successful Debian9 amd64 build: FailedDebian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2907/artifact/CI021BUILD/config.status/config.status Ubuntu 16.04 i386: FailedPackage building failed for Ubuntu 16.04 i386: Ubuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2907/artifact/U1604I386/config.status/config.status Ubuntu1604 amd64 build: FailedPackage building failed for Ubuntu1604 amd64 build: Ubuntu1604 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2907/artifact/CI014BUILD/config.status/config.status Debian8 amd64 build: FailedPackage building failed for Debian8 amd64 build: Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2907/artifact/CI008BLD/config.status/config.status Warnings Generated during build:Checkout code: Successful with additional warnings:Debian9 amd64 build: FailedDebian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2907/artifact/CI021BUILD/config.status/config.status Ubuntu 16.04 i386: FailedPackage building failed for Ubuntu 16.04 i386: Ubuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2907/artifact/U1604I386/config.status/config.status Ubuntu1604 amd64 build: FailedPackage building failed for Ubuntu1604 amd64 build: Ubuntu1604 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2907/artifact/CI014BUILD/config.status/config.status Debian8 amd64 build: FailedPackage building failed for Debian8 amd64 build: Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2907/artifact/CI008BLD/config.status/config.status |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
🚧 Basic BGPD CI results: Partial FAILURE, 1 tests failedResults table
For details, please contact louberger |
Filing issue against myself to fix issues at later time
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2908/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base19 Static Analyzer issues remaining.See details at |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
|
I spoke with @vivek-cumulus and he indicated it would be good to merge this as well. |
|
ok to merge. |
PR FRRouting#1739 added code to leak routes between (default VRF) VPN safi and unicast RIBs in any VRF. That set of changes included temporary CLI including vpn-policy blocks to specify RD/RT/label/&c. After considerable discussion, we arrived at a consensus CLI shown below. The code of this PR implements the vpn-specific parts of this syntax: router bgp <as> [vrf <FOO>] address-family <afi> unicast rd (vpn|evpn) export (AS:NN | IP:nn) label (vpn|evpn) export (0..1048575) rt (vpn|evpn) (import|export|both) RTLIST... nexthop vpn (import|export) (A.B.C.D | X:X::X:X) route-map (vpn|evpn|vrf NAME) (import|export) MAP [no] import|export [vpn|evpn|evpn8] [no] import|export vrf NAME User documentation of the vpn-specific parts of the above syntax is in PR FRRouting#1937 Signed-off-by: G. Paul Ziemba <paulz@labn.net>
PR FRRouting#1739 added code to leak routes between (default VRF) VPN safi and unicast RIBs in any VRF. That set of changes included temporary CLI including vpn-policy blocks to specify RD/RT/label/&c. After considerable discussion, we arrived at a consensus CLI shown below. The code of this PR implements the vpn-specific parts of this syntax: router bgp <as> [vrf <FOO>] address-family <afi> unicast rd (vpn|evpn) export (AS:NN | IP:nn) label (vpn|evpn) export (0..1048575) rt (vpn|evpn) (import|export|both) RTLIST... nexthop vpn (import|export) (A.B.C.D | X:X::X:X) route-map (vpn|evpn|vrf NAME) (import|export) MAP [no] import|export [vpn|evpn|evpn8] [no] import|export vrf NAME User documentation of the vpn-specific parts of the above syntax is in PR FRRouting#1937 Signed-off-by: G. Paul Ziemba <paulz@labn.net>
bgpd: vpn-vrf route leaking (includes default instance unicast as special case of vrf)