bgp: multiple fixes for route leaking #11886
Conversation
pguibert6WIND
force-pushed
the
bgp_leak
branch
from
e7c9d14 to
4244f09
Compare
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 i386 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4U18I386-7186/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 4 Topotests debian 10 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4DEB10AMD64-7186/test Topology Tests failed for Topotests debian 10 amd64 part 4 Topotests Ubuntu 18.04 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP4U1804AMD64-7186/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 4 Topotests Ubuntu 18.04 arm8 part 4: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 4: No useful log foundSuccessful on other platforms/tests
|
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 i386 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4U18I386-7185/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 4 Topotests debian 10 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4DEB10AMD64-7185/test Topology Tests failed for Topotests debian 10 amd64 part 4 Topotests Ubuntu 18.04 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP4U1804AMD64-7185/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 4 Topotests Ubuntu 18.04 amd64 part 2: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP2U1804AMD64-7185/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 2 Topotests Ubuntu 18.04 arm8 part 4: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 4: No useful log foundSuccessful on other platforms/tests
|
pguibert6WIND
force-pushed
the
bgp_leak
branch
from
4244f09 to
8f4f740
Compare
Continuous Integration Result: SUCCESSFULContinuous 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-PULLREQ2-7191/ This is a comment from an automated CI system. |
pguibert6WIND
force-pushed
the
bgp_leak
branch
from
8f4f740 to
223e177
Compare
Continuous Integration Result: SUCCESSFULContinuous 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-PULLREQ2-7195/ This is a comment from an automated CI system. |
pguibert6WIND
changed the title
|
ci:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests debian 10 amd64 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO6DEB10AMD64-7225/test Topology Tests failed for Topotests debian 10 amd64 part 6 Successful on other platforms/tests
|
|
ci:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests debian 10 amd64 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO6DEB10AMD64-7232/test Topology Tests failed for Topotests debian 10 amd64 part 6 Topotests debian 10 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4DEB10AMD64-7232/test Topology Tests failed for Topotests debian 10 amd64 part 4 Successful on other platforms/tests
|
When redistributing connected addresses, the address family has
to be figured out. The calculation was not done, the next-hop
address length was not set, and as consequence, the nexthop
is displayed like if it was an ipv6 address, which is wrong for
ipv4 addresses.
Calculate the family for connected addresses.
Change the topotests accordingly.
Fixes: ("7226bc40d606") bgpd: ignore NEXT_HOP for MP_REACH_NLRI
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
Prefixes that are stated in the network command cannot be leaked to the other VRFs BGP table whether or not they are present in the origin VRF RIB. Always validate the nexthop of BGP static routes (i.e. defined with the network statement) if 'no network import-check' is defined on the source BGP session. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com> Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
If 'network import-check' is defined on the source BGP session, prefixes that are stated in the network command cannot be leaked to the other VRFs BGP table even if they are present in the origin VRF RIB. Always validate the nexthop of BGP static routes (i.e. defined with the network statement) if 'network import-check' is defined on the source BGP session and the prefix is present in source RIB. It fixes the issue when the 'rt import' statement is defined after the 'network' ones. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Move setting of some variables backwards to prepare next commits. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
"if not XX else" statements are confusing. Replace two "if not XX else" statements by "if XX else" to prepare next commits. The patch is only cosmetic. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
If 'network import-check' is defined on the source BGP session, prefixes that are stated in the network command cannot be leaked to the other VRFs BGP table even if they are present in the origin VRF RIB if the 'rt import' statement is defined after the 'network <prefix>' ones. When a prefix nexthop is updated, update the prefix route leaking. The current state of nexthop validation is now stored in the attributes of the bgp path info. Attributes are compared with the previous ones at route leaking update so that a nexthop validation change now triggers the update of destination VRF BGP table. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
The following configuration creates an infinite routing leaking loop because 'rt vpn both' parameters are the same in both VRFs. > router bgp 5227 vrf r1-cust4 > no bgp network import-check > bgp router-id 192.168.1.1 > address-family ipv4 unicast > network 28.0.0.0/24 > rd vpn export 10:12 > rt vpn both 52:100 > import vpn > export vpn > exit-address-family > ! > router bgp 5227 vrf r1-cust5 > no bgp network import-check > bgp router id 192.168.1.1 > address-family ipv4 unicast > network 29.0.0.0/24 > rd vpn export 10:13 > rt vpn both 52:100 > import vpn > export vpn > exit-address-family The previous commit has added a routing leak update when a nexthop update is received from zebra. It indirectly calls bgp_find_or_add_nexthop() in which a static route triggers a nexthop cache entry registration that triggers a nexthop update from zebra. Do not register again the nexthop cache entry if the BGP_STATIC_ROUTE is already set. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add a function to find the VRF or the loopback interface: the loopback interface for the default VRF and the VRF master interface otherwise. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Fix a CLANG warning Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
There is two cases where the nexthop interface is incorrect: - Case 1: leaked routes from prefixes stated in 'network <prefix>' are inactive because they have no nexthop IP address or interface. - Case 2: leaked routes from 'redistribute connected' contains the original nexthop interface. ====== Case 1 ====== > router bgp 5227 vrf r1-cust1 > bgp router-id 192.168.1.1 > no bgp network import-check > ! > address-family ipv4 unicast > network 10.2.3.4/32 > network 192.168.1.0/24 > rd vpn export 10:1 > rt vpn import 52:100 > rt vpn export 52:101 > export vpn > import vpn > exit-address-family > exit > ! > router bgp 5227 vrf r1-cust4 > bgp router-id 192.168.1.1 > ! > address-family ipv4 unicast > network 29.0.0.0/24 > rd vpn export 10:1 > rt vpn import 52:101 > rt vpn export 52:100 > export vpn > import vpn > exit-address-family > exit Extract from the routing table: > VRF r1-cust1: > S>* 192.0.0.0/24 [1/0] via 192.168.1.2, r1-eth4, weight 1, 00:47:53 > C>* 192.168.1.0/24 is directly connected, r1-eth4, 00:44:15 > B>* 29.0.0.0/24 [20/0] is directly connected, unknown (vrf r1-cust4), inactive, weight 1, 00:00:02 > > VRF r1-cust4: > B 10.2.3.4/32 [20/0] is directly connected, unknown (vrf r1-cust1) inactive, weight 1, 00:00:02 > C>* 29.0.0.0/24 is directly connected, r1-cust5, 00:27:40 > B 192.0.0.0/24 [20/0] is directly connected, unknown (vrf r1-cust1) inactive, weight 1, 00:03:40 > B 192.168.1.0/24 [20/0] is directly connected, unknown (vrf r1-cust1) inactive, weight 1, 00:00:02 ====== Case 2 ====== The previous is modified with the following settings: > router bgp 5227 vrf r1-cust1 > address-family ipv4 unicast > no network 192.168.1.0/24 > redistribute connected > ! > vrf r1-cust1 > ip route 29.0.0.0/24 r1-cust5 nexthop-vrf r1-cust5 Extract from the routing table: > VRF r1-cust1: > S>* 192.0.0.0/24 [1/0] via 192.168.1.2, r1-eth4, weight 1, 00:47:53 > C>* 192.168.1.0/24 is directly connected, r1-eth4, 00:44:15 > S>* 29.0.0.0/24 [1/0] is directly connected, r1-cust5 (vrf r1-cust5), weight 1, 00:00:30 > > VRF r1-cust4: > B 10.2.3.4/32 [20/0] is directly connected, unknown (vrf r1-cust1) inactive, weight 1, 00:00:02 > C>* 29.0.0.0/24 is directly connected, r1-cust5, 00:27:40 > B 192.0.0.0/24 [20/0] is directly connected, unknown (vrf r1-cust1) inactive, weight 1, 00:03:40 > B>* 192.168.1.0/24 [20/0] is directly connected, r1-eth4 (vrf r1-cust1), weight 1, 00:00:02 The nexthop interface is r1-eth4. It causes issue to traffic leaving r1-cust4. The following ping to r1-eth4 local address 192.168.1.1 from r1-cust5 local add does not respond. > # tcpdump -lnni r1-cust1 'icmp' & > # ip vrf exec r1-cust4 ping -c1 192.168.1.1 -I 29.0.0.1 > PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data. PING 192.168.1.1 (192.168.1.1) from 29.0.0.1 : 56(84) bytes of data. 18:49:20.635638 IP 29.0.0.1 > 192.168.1.1: ICMP echo request, id 15897, seq 1, length 64 18:49:27.113827 IP 29.0.0.1 > 29.0.0.1: ICMP host 192.168.1.1 unreachable, length 92 Fix description: When leaking prefix from other VRFs, if the nexthop IP address is not set in the bgp path info attribures, reset nh_ifindex to the index of master interface of the incoming BGP instance. The result is for case 1 and 2: > VRF r1-cust1: > S>* 192.0.0.0/24 [1/0] via 192.168.1.2, r1-eth4, weight 1, 00:47:53 > C>* 192.168.1.0/24 is directly connected, r1-eth4, 00:44:15 > B>* 29.0.0.0/24 [20/0] is directly connected, r1-cust4 (vrf r1-cust4), weight 1, 00:00:08 > > VRF r1-cust4: > B>* 10.2.3.4/32 [20/0] is directly connected, r1-cust1 (vrf r1-cust1), weight 1, 00:00:08 > C>* 29.0.0.0/24 is directly connected, r1-cust5, 00:27:40 > B>* 192.0.0.0/24 [20/0] is directly connected, r1-cust1 (vrf r1-cust1), weight 1, 00:00:08 > B>* 192.168.1.0/24 [20/0] is directly connected, r1-cust1 (vrf r1-cust1), weight 1, 00:00:08 > # tcpdump -lnni r1-cust1 'icmp' & > # ping -c1 192.168.1.1 -I 29.0.0.1 > PING 192.168.1.1 (192.168.1.1) from 29.0.0.1 : 56(84) bytes of data. > 18:48:32.506281 IP 29.0.0.1 > 192.168.1.1: ICMP echo request, id 15870, seq 1, length 64 > 64 bytes from 192.168.1.1: icmp_seq=1 ttl=64 time=0.050 ms > 18:48:32.506304 IP 192.168.1.1 > 29.0.0.1: ICMP echo reply, id 15870, seq 1, length 64 Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com> 1, 00:47:53 4:15 vrf r1-cust4), inactive, weight 1, 00:00:02 vrf r1-cust1) inactive, weight 1, 00:00:02 40 (vrf r1-cust1) inactive, weight 1, 00:03:40 n (vrf r1-cust1) inactive, weight 1, 00:00:02 dress is not the index of 1, 00:47:53 4:15 (vrf r1-cust4), weight 1, 00:00:08 (vrf r1-cust1), weight 1, 00:00:08 40 (vrf r1-cust1), weight 1, 00:00:08 t1 (vrf r1-cust1), weight 1, 00:00:08
Leaked connected routes have now the following nexthop interfaces: - lo for routes imported from the default VRF - or the VRF interface for routes imported from the other VRFs. Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
|
ci:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests debian 10 amd64 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO6DEB10AMD64-7246/test Topology Tests failed for Topotests debian 10 amd64 part 6 Topotests debian 10 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4DEB10AMD64-7246/test Topology Tests failed for Topotests debian 10 amd64 part 4 Successful on other platforms/tests
|
|
test fail because of calling 'ip vrf exec <> ping ' : BPF prog: 'Operation not permitted' |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 arm8 part 4: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 4: No useful log foundTopotests Ubuntu 18.04 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP4U1804AMD64-7264/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 4 Topotests Ubuntu 18.04 i386 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4U18I386-7264/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 4 Topotests debian 10 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4DEB10AMD64-7264/test Topology Tests failed for Topotests debian 10 amd64 part 4 Successful on other platforms/tests
|
pguibert6WIND
force-pushed
the
bgp_leak
branch
from
f831ca3 to
4b11f18
Compare
|
the last commit may probably need to have boot parameters for debian 10 : "cgroup_no_v1=net_prio,net_cls" as suggested by https://bugzilla.kernel.org/show_bug.cgi?id=203483 |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests debian 10 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4DEB10AMD64-7269/test Topology Tests failed for Topotests debian 10 amd64 part 4 Successful on other platforms/tests
|
|
@pguibert6WIND actually you can use 2 tested with this ping version: will double check in CI |
The iproute2 utility is able to handle two '-I' parameters, with the first one which is the vrf name where the ping command should sit, and the second one which is the source address. This change avoids using 'ip vrf exec' which may fail on some distributions like debian 10. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
pguibert6WIND
force-pushed
the
bgp_leak
branch
from
4b11f18 to
8d2bf91
Compare
lets see what happens with that change. |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests debian 10 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4DEB10AMD64-7273/test Topology Tests failed for Topotests debian 10 amd64 part 4 Topotests Ubuntu 18.04 arm8 part 4: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 4: No useful log foundTopotests Ubuntu 18.04 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP4U1804AMD64-7273/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 4 Topotests Ubuntu 18.04 i386 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO6U18I386-7273/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 6 Topotests Ubuntu 18.04 amd64 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO6U18AMD64-7273/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 6 Topotests Ubuntu 18.04 arm8 part 6: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 6: No useful log foundTopotests Ubuntu 18.04 i386 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4U18I386-7273/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 4 Topotests debian 10 amd64 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO6DEB10AMD64-7273/test Topology Tests failed for Topotests debian 10 amd64 part 6 Successful on other platforms/tests
|
@eqvinox proposed solution does not work. |
|
Sigh. It was added in 2019: iputils/iputils@9e08707 ping on Ubuntu 18.04 is |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests debian 10 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4DEB10AMD64-7294/test Topology Tests failed for Topotests debian 10 amd64 part 4 Successful on other platforms/tests
|
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 arm8 part 4: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 4: No useful log foundTopotests Ubuntu 18.04 i386 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4U18I386-7295/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 4 Topotests debian 10 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4DEB10AMD64-7295/test Topology Tests failed for Topotests debian 10 amd64 part 4 Topotests Ubuntu 18.04 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP4U1804AMD64-7295/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 4 Successful on other platforms/tests
|
pguibert6WIND
force-pushed
the
bgp_leak
branch
from
c5b820f to
4cdea32
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 arm8 part 4: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 4: No useful log foundTopotests Ubuntu 18.04 i386 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4U18I386-7298/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 4 Topotests debian 10 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO4DEB10AMD64-7298/test Topology Tests failed for Topotests debian 10 amd64 part 4 Topotests Ubuntu 18.04 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP4U1804AMD64-7298/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 4 Successful on other platforms/tests
|
| bgp_attr_get_transit(attr2) && | ||
| attr1->rmap_table_id == attr2->rmap_table_id && | ||
| (attr1->encap_tunneltype == attr2->encap_tunneltype) && | ||
| encap_same(attr1->encap_subtlvs, attr2->encap_subtlvs) | ||
| #ifdef ENABLE_BGP_VNC |
There was a problem hiding this comment.
the formatting of all the other defines are changed but these ... should these be changed as well?
|
CI:rerun Abusing this PR to verify CI upgrade |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 similar comment
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
waiting on the conflicts here to re-review and see where we are ... |
continuation of #11127