Mpls fixes #495
Mpls fixes #495
Conversation
Signed-off-by: Daniel Walton <dwalton@cumulusnetworks.com>
If the client registers for 0.0.0.0/0 the length will be 5 Signed-off-by: Daniel Walton <dwalton@cumulusnetworks.com>
ZEBRA_FLAG_SELECTED hasn't been set yet by the time lsp_install is called. The call path is: rib_process -> rib_process_add_fib -> zebra_mpls_lsp_install -> lsp_install but ZEBRA_FLAG_SELECTED is set in rib_process after it calls rib_process_add_fib. I can't think of anything that it would hurt to install the LSP regardless of whether ZEBRA_FLAG_SELECTED is set later. I also cleaned up some UI (json and display the pretty label names instead of their numeric values). Signed-off-by: Daniel Walton <dwalton@cumulusnetworks.com>
Signed-off-by: Daniel Walton <dwalton@cumulusnetworks.com>
Signed-off-by: Daniel Walton <dwalton@cumulusnetworks.com>
| stream_put (s, &attre->mp_nexthop_local, IPV6_MAX_BYTELEN); | ||
| } else { | ||
| stream_putc (s, IPV6_MAX_BYTELEN); |
There was a problem hiding this comment.
Shouldn't this be attre->mp_nexthop_len?
There was a problem hiding this comment.
In this scenario for ipv4+label the mp_nexthop_len is 4 but since we are using ENHE we need to use IPV6_MAX_BYTELEN
There was a problem hiding this comment.
Isn't this code used for v6 unicast and multicast SAFIs too?
There was a problem hiding this comment.
It is but even then we are still good. We are in a switch statement for the nexthop AFI and the case is AFI_IP6 so no matter what the length has to be either BGP_ATTR_NHLEN_IPV6_GLOBAL_AND_LL or IPV6_MAX_BYTELEN.
There was a problem hiding this comment.
Okay, I was distracted by the v6/vpn afi/safi case;-) Looks good to me.
| */ | ||
| nh_afi = afi_iana2int (pkt_nh_afi); | ||
|
|
||
| if (afi != AFI_IP || safi != SAFI_UNICAST || nh_afi != AFI_IP6) | ||
| if (afi != AFI_IP || nh_afi != AFI_IP6 || !(safi == SAFI_UNICAST || safi == SAFI_LABELED_UNICAST)) |
There was a problem hiding this comment.
Is there a reason to not also allow this for the other supported SAFIs (VPN, Encap, EVPN, ...)?
There was a problem hiding this comment.
Long term no but that isn't something I'm tackling at the moment
There was a problem hiding this comment.
Great - we're on the same page then.
Continous 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: SuccessfulBasic Tests: FailedStatic analyzer (clang): Successful Topology tests on Ubuntu 16.04: FailedTopology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-649/test Topology Tests failed for Topology tests on Ubuntu 16.04 Topology Tests memoy analysis: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-649/artifact/TOPOU1604/MemoryLeaks/ |
Continous 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-649/ This is a comment from an EXPERIMENTAL automated CI system. |
|
Does this PR need anything else before integration? |
|
I'm sure I'll have some more mpls fixes as I'm working out kinks to get our mpls tests passing but as far as this pull request goes I think we are good. @louberger if you are good with the changes can you commit the pull request? Thanks |
|
someone familiar with the zebra mpls should review it and then do the
merge...
…On 5/11/2017 9:07 AM, Daniel Walton wrote:
I'm sure I'll have some more mpls fixes as I'm working out kinks to
get our mpls tests passing but as far as this pull request goes I
think we are good. @louberger <https://github.com/louberger> if you
are good with the changes can you commit the pull request? Thanks
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#495 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AGRWyP9CBqBUJ4n1kaqefD9v5NUYvfLgks5r4wgmgaJpZM4NWt_x>.
|
|
Please give me a few hours to review this... a bit swamped right now with other things. |
There was a problem hiding this comment.
Changes look ok to me.
However, BGP Labeled Unicast is partially broken. bgpd is announcing labeled IPv4 routes with IPv6 nexthops, which in ultimate analysis means that the BGP labels are not installed in the FIB/LFIB.
Apparently commits 1d4b7d0 and fe3ca08 exposed a bug in bgpd.
Please take a look at these lines:
https://github.com/donaldsharp/frr/blob/343e90c32/bgpd/bgp_updgrp_packet.c#L773
https://github.com/donaldsharp/frr/blob/343e90c32/bgpd/bgp_attr.c#L2851
In the bgp_packet_mpattr_start() function, we're using the length of the nexthop (mp_nexthop_len) to get its address-family, IPv4 or IPv6.
Now look at this line:
https://github.com/donaldsharp/frr/blob/343e90c32/bgpd/bgp_attr.c#L966
By default 'mp_nexthop_len' is always set to IPV6_MAX_BYTELEN, even for regular IPv4 routes.
So from my understanding we're failing to update 'mp_nexthop_len' in a few places.
I tried the diff below and it fixes the problem for labeled unicast:
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -3809,6 +3809,12 @@ bgp_static_update (struct bgp *bgp, struct prefix *p,
attr.med = bgp_static->igpmetric;
attr.flag |= ATTR_FLAG_BIT (BGP_ATTR_MULTI_EXIT_DISC);
+ if (afi == AFI_IP && safi == SAFI_LABELED_UNICAST)
+ {
+ bgp_attr_extra_get (&attr)->mp_nexthop_global_in = bgp_static->igpnexthop;
+ bgp_attr_extra_get (&attr)->mp_nexthop_len = IPV4_MAX_BYTELEN;
+ }
+
if (bgp_static->atomic)
attr.flag |= ATTR_FLAG_BIT (BGP_ATTR_ATOMIC_AGGREGATE);
It was based on a similar change done in bgp_static_update_safi() for EVPN/L3VPN: https://github.com/donaldsharp/frr/blob/343e90c32/bgpd/bgp_route.c#L4091-L4098
The problem with bgp_static_update_safi() is that 'igpnexthop' is not set anywhere in the code since commit fc9a856, so 'mp_nexthop_len' is not effectively updated in that function.
@dwalton76 please feel free to commit my change if you think it's correct. But I'd appreciate if you check this problem in bgp_static_update_safi() as well as EVPN/VPNv4 might be affected too.
|
BTW, we should consider writing some topotests every time we introduce a new feature, to make sure that everything works at least on a basic level and detect regressions like this earlier. |
|
label distribution is largely broken on master IMO. See Issue #473.
I do agree that these issues should have been caught in CI. I'll work
with @mwinter-osr to get some basic tests added.
Lou
…On 05/12/2017 12:28 AM, Renato Westphal wrote:
***@***.**** approved this pull request.
Changes look ok to me.
However, BGP Labeled Unicast is partially broken. bgpd is announcing
labeled IPv4 routes with IPv6 nexthops, which in ultimate analysis means
that the BGP labels are not installed in the FIB/LFIB.
Apparently commits 1d4b7d0
<1d4b7d0>
and fe3ca08
<fe3ca08>
exposed a bug in bgpd.
Please take a look at these lines:
https://github.com/donaldsharp/frr/blob/343e90c32/bgpd/bgp_updgrp_packet.c#L773
https://github.com/donaldsharp/frr/blob/343e90c32/bgpd/bgp_attr.c#L2851
In the bgp_packet_mpattr_start() function, we're using the length of the
nexthop (mp_nexthop_len) to get its address-family, IPv4 or IPv6.
Now look at this line:
https://github.com/donaldsharp/frr/blob/343e90c32/bgpd/bgp_attr.c#L966
By default 'mp_nexthop_len' is always set to IPV6_MAX_BYTELEN, even for
regular IPv4 routes.
So from my understanding we're failing to update 'mp_nexthop_len' in a
few places.
I tried the diff below and it fixes the problem for labeled unicast:
|--- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -3809,6 +3809,12 @@
bgp_static_update (struct bgp *bgp, struct prefix *p, attr.med =
bgp_static->igpmetric; attr.flag |= ATTR_FLAG_BIT
(BGP_ATTR_MULTI_EXIT_DISC); + if (afi == AFI_IP && safi ==
SAFI_LABELED_UNICAST) + { + bgp_attr_extra_get
(&attr)->mp_nexthop_global_in = bgp_static->igpnexthop; +
bgp_attr_extra_get (&attr)->mp_nexthop_len = IPV4_MAX_BYTELEN; + } + if
(bgp_static->atomic) attr.flag |= ATTR_FLAG_BIT
(BGP_ATTR_ATOMIC_AGGREGATE); |
It was based on a similar change done in bgp_static_update_safi() for
EVPN/L3VPN:
https://github.com/donaldsharp/frr/blob/343e90c32/bgpd/bgp_route.c#L4091-L4098
The problem with bgp_static_update_safi() is that 'igpnexthop' is not
set anywhere in the code since commit fc9a856
<fc9a856>,
so 'mp_nexthop_len' is not effectively updated in that function.
@dwalton76 <https://github.com/dwalton76> please feel free to commit my
change if you think it's correct. But I'd appreciate if you check this
problem in bgp_static_update_safi() as well as EVPN/VPNv4 might be
affected too.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#495 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGRWyFb4PncgKkd7shQG0hZlpR-qwK4_ks5r4-AIgaJpZM4NWt_x>.
|
|
@rwestphal agreed there were a couple of commits that exposed some bugs. I am not seeing I do if I enable ENHE for that peer but in that case that is the desired behavior. I also see that mp_nexthop_len is set to 4 for SAFI_UNICAST routes, I'm not sure yet what the difference is in my setup vs yours that is causing the different behavior. So I would say lets merge in this pull request since it is a step forward and then do some more digging on when mp_nexthop_len is/isn't getting set correctly. |
|
Sure, I'll merge this one now. For reference, this is the test topology I used to test BGP-LU here: # +---------+
# | |
# | RT1 |
# | 1.1.1.1 |-lo1 (2001:db8:1000::1/128)
# | |
# +---------+
# |rt1-eth0 (.1)
# |
# |10.0.1.0/24
# |2001:DB8:1::/64
# |
# |rt2-eth0 (.2)
# +---------+
# | |
# | RT2 |
# | 2.2.2.2 |-lo1 (2001:db8:1000::2/128)
# | |
# +---------+
# |rt2-eth1 (.2)
# |
# |
# +----------+----------+
# | 10.0.2.0/24 |
# | 2001:DB8:2::/64 |
# | |
# |rt3-eth0 (.3) |rt4-eth0 (.4)
# +---------+ +---------+
# | | | |
#(2001:db8:1000::3/128) lo1-| RT3 | | RT4 |-lo1 (2001:db8:1000::4/128)
# | 3.3.3.3 | | 4.4.4.4 |
# | | | |
# +---------+ +---------+
---
frr:
base-configs:
zebra: |
password 1
log file /tmp/%r-zebra.log
line vty
!
debug zebra mpls
debug zebra kernel
bgpd:
password 1
log file /tmp/%r-bgpd.log
line vty
!
debug bgp zebra
debug bgp update in
routers:
rt1:
links:
rt1-lo1:
ipv4: 1.1.1.1/32
ipv6: 2001:db8:1000::1/128
rt1-eth0:
peer-node: rt2
peer-iface: rt2-eth0
mac: AA:01:00:00:00:01
mpls: yes
ipv4: 10.0.1.1/24
ipv6: 2001:db8:1::1/64
frr:
zebra:
run: yes
config: |
mpls label global-block 16 1000
mpls label bind 1.1.1.1/32 101
mpls label bind 2.2.2.2/32 102
mpls label bind 3.3.3.3/32 103
mpls label bind 4.4.4.4/32 104
bgpd:
run: yes
config: |
router bgp 1
neighbor 10.0.1.2 remote-as 1
!
address-family ipv4 unicast
network 1.1.1.1/32
exit-address-family
!
address-family ipv4 labeled-unicast
network 1.1.1.1/32
neighbor 10.0.1.2 activate
exit-address-family
!
rt2:
links:
rt2-lo1:
ipv4: 2.2.2.2/32
ipv6: 2001:db8:1000::2/128
rt2-eth0:
peer-node: rt1
peer-iface: rt1-eth0
mac: AA:02:00:00:00:01
mpls: yes
ipv4: 10.0.1.2/24
ipv6: 2001:db8:1::2/64
rt2-eth1:
peer-node: sw1
peer-iface: sw1-1
mac: AA:02:00:00:00:02
mpls: yes
ipv4: 10.0.2.2/24
ipv6: 2001:db8:2::2/64
frr:
zebra:
run: yes
config: |
mpls label global-block 16 1000
mpls label bind 1.1.1.1/32 201
mpls label bind 2.2.2.2/32 202
mpls label bind 3.3.3.3/32 203
mpls label bind 4.4.4.4/32 204
bgpd:
run: yes
config: |
router bgp 1
neighbor 10.0.1.1 remote-as 1
neighbor 10.0.2.3 remote-as 1
neighbor 10.0.2.4 remote-as 1
!
address-family ipv4 unicast
network 2.2.2.2/32
neighbor 10.0.1.1 route-reflector-client
neighbor 10.0.1.1 next-hop-self force
neighbor 10.0.2.3 route-reflector-client
neighbor 10.0.2.3 next-hop-self force
neighbor 10.0.2.4 route-reflector-client
neighbor 10.0.2.4 next-hop-self force
exit-address-family
!
address-family ipv4 labeled-unicast
network 2.2.2.2/32
neighbor 10.0.1.1 activate
neighbor 10.0.1.1 route-reflector-client
neighbor 10.0.1.1 next-hop-self force
neighbor 10.0.2.3 activate
neighbor 10.0.2.3 route-reflector-client
neighbor 10.0.2.3 next-hop-self force
neighbor 10.0.2.4 activate
neighbor 10.0.2.4 route-reflector-client
neighbor 10.0.2.4 next-hop-self force
exit-address-family
!
rt3:
links:
rt3-lo1:
ipv4: 3.3.3.3/32
ipv6: 2001:db8:1000::3/128
rt3-eth0:
peer-node: sw1
peer-iface: sw1-2
mac: AA:03:00:00:00:01
mpls: yes
ipv4: 10.0.2.3/24
ipv6: 2001:db8:2::3/64
frr:
zebra:
run: yes
config: |
mpls label global-block 16 1000
mpls label bind 1.1.1.1/32 301
mpls label bind 2.2.2.2/32 302
mpls label bind 3.3.3.3/32 303
mpls label bind 4.4.4.4/32 304
bgpd:
run: yes
config: |
router bgp 1
neighbor 10.0.2.2 remote-as 1
!
address-family ipv4 unicast
network 3.3.3.3/32
exit-address-family
!
address-family ipv4 labeled-unicast
network 3.3.3.3/32
neighbor 10.0.2.2 activate
exit-address-family
!
rt4:
links:
rt4-lo1:
ipv4: 4.4.4.4/32
ipv6: 2001:db8:1000::4/128
rt4-eth0:
peer-node: sw1
peer-iface: sw1-3
mac: AA:04:00:00:00:01
mpls: yes
ipv4: 10.0.2.4/24
ipv6: 2001:db8:2::4/64
frr:
zebra:
run: yes
config: |
mpls label global-block 16 1000
mpls label bind 1.1.1.1/32 401
mpls label bind 2.2.2.2/32 402
mpls label bind 3.3.3.3/32 403
mpls label bind 4.4.4.4/32 404
bgpd:
run: yes
config: |
router bgp 1
neighbor 10.0.2.2 remote-as 1
!
address-family ipv4 unicast
network 4.4.4.4/32
exit-address-family
!
address-family ipv4 labeled-unicast
network 4.4.4.4/32
neighbor 10.0.2.2 activate
exit-address-family
!
switches:
sw1:
links:
sw1-1:
peer-node: rt2
peer-iface: rt2-eth1
sw1-2:
peer-node: rt3
peer-iface: rt3-eth0
sw1-3:
peer-node: rt4
peer-iface: rt4-eth0 |
No description provided.