From 2c6eb34af88bb6fa94c13413a57c85d343782a5d Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 27 Sep 2024 12:55:39 +0300 Subject: [PATCH 1/3] tests: Drop test_bgp_with_loopback_with_same_subnet_p1 It's replaced and simplified by c3fd1e9520c619babb3004cea6df622ca67b0dfa. JSON topo is just horrible to debug. Signed-off-by: Donatas Abraitis --- .../test_bgp_basic_functionality.py | 289 ------------------ 1 file changed, 289 deletions(-) diff --git a/tests/topotests/bgp_basic_functionality_topo1/test_bgp_basic_functionality.py b/tests/topotests/bgp_basic_functionality_topo1/test_bgp_basic_functionality.py index c97fc5f0eb..5662e5935b 100644 --- a/tests/topotests/bgp_basic_functionality_topo1/test_bgp_basic_functionality.py +++ b/tests/topotests/bgp_basic_functionality_topo1/test_bgp_basic_functionality.py @@ -831,7 +831,6 @@ def test_bgp_with_loopback_interface(request): for bgp_neighbor in topo["routers"][routerN]["bgp"]["address_family"]["ipv4"][ "unicast" ]["neighbor"].keys(): - # Adding ['source_link'] = 'lo' key:value pair topo["routers"][routerN]["bgp"]["address_family"]["ipv4"]["unicast"][ "neighbor" @@ -876,294 +875,6 @@ def test_bgp_with_loopback_interface(request): write_test_footer(tc_name) -def test_bgp_with_loopback_with_same_subnet_p1(request): - """ - Verify routes not installed in zebra when /32 routes received - with loopback BGP session subnet - """ - - tgen = get_topogen() - if BGP_CONVERGENCE is not True: - pytest.skip("skipped because of BGP Convergence failure") - - # test case name - tc_name = request.node.name - write_test_header(tc_name) - - # Creating configuration from JSON - reset_config_on_routers(tgen) - step("Delete BGP seesion created initially") - input_dict_r1 = { - "r1": {"bgp": {"delete": True}}, - "r2": {"bgp": {"delete": True}}, - "r3": {"bgp": {"delete": True}}, - "r4": {"bgp": {"delete": True}}, - } - result = create_router_bgp(tgen, topo, input_dict_r1) - assert result is True, "Testcase {} : Failed \n Error: {}".format(tc_name, result) - - step("Create BGP session over loop address") - topo_modify = deepcopy(topo) - - for routerN in sorted(topo["routers"].keys()): - for addr_type in ADDR_TYPES: - for bgp_neighbor in topo_modify["routers"][routerN]["bgp"][ - "address_family" - ][addr_type]["unicast"]["neighbor"].keys(): - - # Adding ['source_link'] = 'lo' key:value pair - topo_modify["routers"][routerN]["bgp"]["address_family"][addr_type][ - "unicast" - ]["neighbor"][bgp_neighbor]["dest_link"] = { - "lo": {"source_link": "lo", "ebgp_multihop": 2} - } - - result = create_router_bgp(tgen, topo_modify["routers"]) - assert result is True, "Testcase {} : Failed \n Error: {}".format(tc_name, result) - - step("Disable IPv6 BGP nbr from ipv4 address family") - raw_config = { - "r1": { - "raw_config": [ - "router bgp {}".format(topo["routers"]["r1"]["bgp"]["local_as"]), - "address-family ipv4 unicast", - "no neighbor {} activate".format( - topo["routers"]["r2"]["links"]["lo"]["ipv6"].split("/")[0] - ), - "no neighbor {} activate".format( - topo["routers"]["r3"]["links"]["lo"]["ipv6"].split("/")[0] - ), - ] - }, - "r2": { - "raw_config": [ - "router bgp {}".format(topo["routers"]["r2"]["bgp"]["local_as"]), - "address-family ipv4 unicast", - "no neighbor {} activate".format( - topo["routers"]["r1"]["links"]["lo"]["ipv6"].split("/")[0] - ), - "no neighbor {} activate".format( - topo["routers"]["r3"]["links"]["lo"]["ipv6"].split("/")[0] - ), - ] - }, - "r3": { - "raw_config": [ - "router bgp {}".format(topo["routers"]["r3"]["bgp"]["local_as"]), - "address-family ipv4 unicast", - "no neighbor {} activate".format( - topo["routers"]["r1"]["links"]["lo"]["ipv6"].split("/")[0] - ), - "no neighbor {} activate".format( - topo["routers"]["r2"]["links"]["lo"]["ipv6"].split("/")[0] - ), - "no neighbor {} activate".format( - topo["routers"]["r4"]["links"]["lo"]["ipv6"].split("/")[0] - ), - ] - }, - "r4": { - "raw_config": [ - "router bgp {}".format(topo["routers"]["r4"]["bgp"]["local_as"]), - "address-family ipv4 unicast", - "no neighbor {} activate".format( - topo["routers"]["r3"]["links"]["lo"]["ipv6"].split("/")[0] - ), - ] - }, - } - - step("Configure kernel routes") - result = apply_raw_config(tgen, raw_config) - assert result is True, "Testcase {} : Failed Error: {}".format(tc_name, result) - - r1_ipv4_lo = topo["routers"]["r1"]["links"]["lo"]["ipv4"] - r1_ipv6_lo = topo["routers"]["r1"]["links"]["lo"]["ipv6"] - r2_ipv4_lo = topo["routers"]["r2"]["links"]["lo"]["ipv4"] - r2_ipv6_lo = topo["routers"]["r2"]["links"]["lo"]["ipv6"] - r3_ipv4_lo = topo["routers"]["r3"]["links"]["lo"]["ipv4"] - r3_ipv6_lo = topo["routers"]["r3"]["links"]["lo"]["ipv6"] - r4_ipv4_lo = topo["routers"]["r4"]["links"]["lo"]["ipv4"] - r4_ipv6_lo = topo["routers"]["r4"]["links"]["lo"]["ipv6"] - - r1_r2 = topo["routers"]["r1"]["links"]["r2"]["ipv6"].split("/")[0] - r2_r1 = topo["routers"]["r2"]["links"]["r1"]["ipv6"].split("/")[0] - r1_r3 = topo["routers"]["r1"]["links"]["r3"]["ipv6"].split("/")[0] - r3_r1 = topo["routers"]["r3"]["links"]["r1"]["ipv6"].split("/")[0] - r2_r3 = topo["routers"]["r2"]["links"]["r3"]["ipv6"].split("/")[0] - r3_r2 = topo["routers"]["r3"]["links"]["r2"]["ipv6"].split("/")[0] - r3_r4 = topo["routers"]["r3"]["links"]["r4"]["ipv6"].split("/")[0] - r4_r3 = topo["routers"]["r4"]["links"]["r3"]["ipv6"].split("/")[0] - - r1_r2_ipv4 = topo["routers"]["r1"]["links"]["r2"]["ipv4"].split("/")[0] - r2_r1_ipv4 = topo["routers"]["r2"]["links"]["r1"]["ipv4"].split("/")[0] - r1_r3_ipv4 = topo["routers"]["r1"]["links"]["r3"]["ipv4"].split("/")[0] - r3_r1_ipv4 = topo["routers"]["r3"]["links"]["r1"]["ipv4"].split("/")[0] - r2_r3_ipv4 = topo["routers"]["r2"]["links"]["r3"]["ipv4"].split("/")[0] - r3_r2_ipv4 = topo["routers"]["r3"]["links"]["r2"]["ipv4"].split("/")[0] - r3_r4_ipv4 = topo["routers"]["r3"]["links"]["r4"]["ipv4"].split("/")[0] - r4_r3_ipv4 = topo["routers"]["r4"]["links"]["r3"]["ipv4"].split("/")[0] - - r1_r2_intf = topo["routers"]["r1"]["links"]["r2"]["interface"] - r2_r1_intf = topo["routers"]["r2"]["links"]["r1"]["interface"] - r1_r3_intf = topo["routers"]["r1"]["links"]["r3"]["interface"] - r3_r1_intf = topo["routers"]["r3"]["links"]["r1"]["interface"] - r2_r3_intf = topo["routers"]["r2"]["links"]["r3"]["interface"] - r3_r2_intf = topo["routers"]["r3"]["links"]["r2"]["interface"] - r3_r4_intf = topo["routers"]["r3"]["links"]["r4"]["interface"] - r4_r3_intf = topo["routers"]["r4"]["links"]["r3"]["interface"] - - ipv4_list = [ - ("r1", r1_r2_intf, r2_ipv4_loopback), - ("r1", r1_r3_intf, r3_ipv4_loopback), - ("r2", r2_r1_intf, r1_ipv4_loopback), - ("r2", r2_r3_intf, r3_ipv4_loopback), - ("r3", r3_r1_intf, r1_ipv4_loopback), - ("r3", r3_r2_intf, r2_ipv4_loopback), - ("r3", r3_r4_intf, r4_ipv4_loopback), - ("r4", r4_r3_intf, r3_ipv4_loopback), - ] - - ipv6_list = [ - ("r1", r1_r2_intf, r2_ipv6_loopback, r2_r1), - ("r1", r1_r3_intf, r3_ipv6_loopback, r3_r1), - ("r2", r2_r1_intf, r1_ipv6_loopback, r1_r2), - ("r2", r2_r3_intf, r3_ipv6_loopback, r3_r2), - ("r3", r3_r1_intf, r1_ipv6_loopback, r1_r3), - ("r3", r3_r2_intf, r2_ipv6_loopback, r2_r3), - ("r3", r3_r4_intf, r4_ipv6_loopback, r4_r3), - ("r4", r4_r3_intf, r3_ipv6_loopback, r3_r4), - ] - - for dut, intf, loop_addr in ipv4_list: - result = addKernelRoute(tgen, dut, intf, loop_addr) - assert result is True, "Testcase {}:Failed \n Error: {}".format(tc_name, result) - - for dut, intf, loop_addr, next_hop in ipv6_list: - result = addKernelRoute(tgen, dut, intf, loop_addr, next_hop) - assert result is True, "Testcase {}:Failed \n Error: {}".format(tc_name, result) - - step("Configure static routes") - - input_dict = { - "r1": { - "static_routes": [ - {"network": r2_ipv4_loopback, "next_hop": r2_r1_ipv4}, - {"network": r3_ipv4_loopback, "next_hop": r3_r1_ipv4}, - {"network": r2_ipv6_loopback, "next_hop": r2_r1}, - {"network": r3_ipv6_loopback, "next_hop": r3_r1}, - ] - }, - "r2": { - "static_routes": [ - {"network": r1_ipv4_loopback, "next_hop": r1_r2_ipv4}, - {"network": r3_ipv4_loopback, "next_hop": r3_r2_ipv4}, - {"network": r1_ipv6_loopback, "next_hop": r1_r2}, - {"network": r3_ipv6_loopback, "next_hop": r3_r2}, - ] - }, - "r3": { - "static_routes": [ - {"network": r1_ipv4_loopback, "next_hop": r1_r3_ipv4}, - {"network": r2_ipv4_loopback, "next_hop": r2_r3_ipv4}, - {"network": r4_ipv4_loopback, "next_hop": r4_r3_ipv4}, - {"network": r1_ipv6_loopback, "next_hop": r1_r3}, - {"network": r2_ipv6_loopback, "next_hop": r2_r3}, - {"network": r4_ipv6_loopback, "next_hop": r4_r3}, - ] - }, - "r4": { - "static_routes": [ - {"network": r3_ipv4_loopback, "next_hop": r3_r4_ipv4}, - {"network": r3_ipv6_loopback, "next_hop": r3_r4}, - ] - }, - } - result = create_static_routes(tgen, input_dict) - assert result is True, "Testcase {} :Failed \n Error: {}".format(tc_name, result) - - step("Verify BGP session convergence") - - result = verify_bgp_convergence(tgen, topo_modify) - assert result is True, "Testcase {} :Failed \n Error: {}".format(tc_name, result) - - step("Configure redistribute connected on R2 and R4") - input_dict_1 = { - "r2": { - "bgp": { - "address_family": { - "ipv4": { - "unicast": {"redistribute": [{"redist_type": "connected"}]} - }, - "ipv6": { - "unicast": {"redistribute": [{"redist_type": "connected"}]} - }, - } - } - }, - "r4": { - "bgp": { - "address_family": { - "ipv4": { - "unicast": {"redistribute": [{"redist_type": "connected"}]} - }, - "ipv6": { - "unicast": {"redistribute": [{"redist_type": "connected"}]} - }, - } - } - }, - } - - result = create_router_bgp(tgen, topo, input_dict_1) - assert result is True, "Testcase {} :Failed \n Error: {}".format(tc_name, result) - - step("Verify Ipv4 and Ipv6 network installed in R1 RIB but not in FIB") - input_dict_r1 = { - "r1": { - "static_routes": [ - {"network": "1.0.2.17/32"}, - {"network": "2001:db8:f::2:17/128"}, - ] - } - } - - dut = "r1" - protocol = "bgp" - for addr_type in ADDR_TYPES: - result = verify_fib_routes( - tgen, addr_type, dut, input_dict_r1, expected=False - ) # pylint: disable=E1123 - assert result is not True, ( - "Testcase {} : Failed \n " - "Expected: Routes should not be present in {} FIB \n " - "Found: {}".format(tc_name, dut, result) - ) - - step("Verify Ipv4 and Ipv6 network installed in r3 RIB but not in FIB") - input_dict_r3 = { - "r3": { - "static_routes": [ - {"network": "1.0.4.17/32"}, - {"network": "2001:db8:f::4:17/128"}, - ] - } - } - dut = "r3" - protocol = "bgp" - for addr_type in ADDR_TYPES: - result = verify_fib_routes( - tgen, addr_type, dut, input_dict_r1, expected=False - ) # pylint: disable=E1123 - assert result is not True, ( - "Testcase {} : Failed \n " - "Expected: Routes should not be present in {} FIB \n " - "Found: {}".format(tc_name, dut, result) - ) - - write_test_footer(tc_name) - - if __name__ == "__main__": args = ["-s"] + sys.argv[1:] sys.exit(pytest.main(args)) From c66642d7f054a43e9843312d2155ae4e52c05ad8 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 27 Sep 2024 10:52:55 +0300 Subject: [PATCH 2/3] bgpd: Relax the same prefix and nexthop to be valid Treat as next-hop invalid if import check is enabled. Fixes: 654a5978f695087af062bfc9a382321fa2ccc4ae ("bgpd: prevent routes loop through itself") Fixes: https://github.com/FRRouting/frr/issues/16877 Signed-off-by: Donatas Abraitis --- bgpd/bgp_nht.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index 8719af56b3..49042e8c23 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -347,12 +347,11 @@ int bgp_find_or_add_nexthop(struct bgp *bgp_route, struct bgp *bgp_nexthop, &p.u.prefix6)) ifindex = pi->peer->connection->su.sin6.sin6_scope_id; - if (!is_bgp_static_route && orig_prefix - && prefix_same(&p, orig_prefix)) { + if (!is_bgp_static_route && orig_prefix && prefix_same(&p, orig_prefix) && + CHECK_FLAG(bgp_route->flags, BGP_FLAG_IMPORT_CHECK)) { if (BGP_DEBUG(nht, NHT)) { - zlog_debug( - "%s(%pFX): prefix loops through itself", - __func__, &p); + zlog_debug("%s(%pFX): prefix loops through itself (import-check enabled)", + __func__, &p); } return 0; } From dab1441128fe81457488cf75d8a640a7376aea7c Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 27 Sep 2024 10:52:22 +0300 Subject: [PATCH 3/3] tests: Check if loopback routes are considered valid for nexthop tracking Signed-off-by: Donatas Abraitis --- tests/topotests/bgp_self_prefix/__init__.py | 0 tests/topotests/bgp_self_prefix/r1/frr.conf | 19 +++ tests/topotests/bgp_self_prefix/r2/frr.conf | 20 ++++ tests/topotests/bgp_self_prefix/r3/frr.conf | 20 ++++ .../bgp_self_prefix/test_bgp_self_prefix.py | 111 ++++++++++++++++++ 5 files changed, 170 insertions(+) create mode 100644 tests/topotests/bgp_self_prefix/__init__.py create mode 100644 tests/topotests/bgp_self_prefix/r1/frr.conf create mode 100644 tests/topotests/bgp_self_prefix/r2/frr.conf create mode 100644 tests/topotests/bgp_self_prefix/r3/frr.conf create mode 100644 tests/topotests/bgp_self_prefix/test_bgp_self_prefix.py diff --git a/tests/topotests/bgp_self_prefix/__init__.py b/tests/topotests/bgp_self_prefix/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/topotests/bgp_self_prefix/r1/frr.conf b/tests/topotests/bgp_self_prefix/r1/frr.conf new file mode 100644 index 0000000000..879afb1947 --- /dev/null +++ b/tests/topotests/bgp_self_prefix/r1/frr.conf @@ -0,0 +1,19 @@ +! +int lo + ip address 10.0.0.1/32 +! +int r1-eth0 + ip address 192.168.1.1/24 +! +router bgp 65000 + no bgp ebgp-requires-policy + no bgp network import-check + neighbor 10.0.0.2 remote-as internal + neighbor 10.0.0.2 update-source lo + neighbor 10.0.0.2 next-hop-self + neighbor 10.0.0.3 remote-as external + neighbor 10.0.0.3 update-source lo + neighbor 10.0.0.3 next-hop-self +! +ip route 10.0.0.2/32 192.168.1.2 +ip route 10.0.0.3/32 192.168.1.3 diff --git a/tests/topotests/bgp_self_prefix/r2/frr.conf b/tests/topotests/bgp_self_prefix/r2/frr.conf new file mode 100644 index 0000000000..eb0db356ea --- /dev/null +++ b/tests/topotests/bgp_self_prefix/r2/frr.conf @@ -0,0 +1,20 @@ +! +int lo + ip address 10.0.0.2/32 +! +int r2-eth0 + ip address 192.168.1.2/24 +! +router bgp 65000 + no bgp ebgp-requires-policy + no bgp network import-check + neighbor 10.0.0.1 remote-as internal + neighbor 10.0.0.1 timers 1 3 + neighbor 10.0.0.1 timers connect 1 + neighbor 10.0.0.1 update-source lo + neighbor 10.0.0.1 next-hop-self + address-family ipv4 unicast + network 10.0.0.2/32 + exit-address-family +! +ip route 10.0.0.1/32 192.168.1.1 diff --git a/tests/topotests/bgp_self_prefix/r3/frr.conf b/tests/topotests/bgp_self_prefix/r3/frr.conf new file mode 100644 index 0000000000..e2348f4a68 --- /dev/null +++ b/tests/topotests/bgp_self_prefix/r3/frr.conf @@ -0,0 +1,20 @@ +! +int lo + ip address 10.0.0.3/32 +! +int r3-eth0 + ip address 192.168.1.3/24 +! +router bgp 65003 + no bgp ebgp-requires-policy + no bgp network import-check + neighbor 10.0.0.1 remote-as external + neighbor 10.0.0.1 timers 1 3 + neighbor 10.0.0.1 timers connect 1 + neighbor 10.0.0.1 update-source lo + neighbor 10.0.0.1 next-hop-self + address-family ipv4 unicast + network 10.0.0.3/32 + exit-address-family +! +ip route 10.0.0.1/32 192.168.1.1 diff --git a/tests/topotests/bgp_self_prefix/test_bgp_self_prefix.py b/tests/topotests/bgp_self_prefix/test_bgp_self_prefix.py new file mode 100644 index 0000000000..1045800368 --- /dev/null +++ b/tests/topotests/bgp_self_prefix/test_bgp_self_prefix.py @@ -0,0 +1,111 @@ +#!/usr/bin/env python +# SPDX-License-Identifier: ISC + +# Copyright (c) 2024 by +# Donatas Abraitis +# + +import os +import sys +import json +import pytest +import functools + +CWD = os.path.dirname(os.path.realpath(__file__)) +sys.path.append(os.path.join(CWD, "../")) + +# pylint: disable=C0413 +from lib import topotest +from lib.topogen import Topogen, get_topogen + +pytestmark = [pytest.mark.bgpd] + + +def setup_module(mod): + topodef = {"s1": ("r1", "r2", "r3")} + tgen = Topogen(topodef, mod.__name__) + tgen.start_topology() + + router_list = tgen.routers() + + for _, (rname, router) in enumerate(router_list.items(), 1): + router.load_frr_config(os.path.join(CWD, "{}/frr.conf".format(rname))) + + tgen.start_router() + + +def teardown_module(mod): + tgen = get_topogen() + tgen.stop_topology() + + +def test_bgp_self_prefix(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + r1 = tgen.gears["r1"] + r3 = tgen.gears["r3"] + + def _bgp_converge(): + output = json.loads(r1.vtysh_cmd("show bgp ipv4 unicast json")) + expected = { + "routes": { + "10.0.0.2/32": [ + { + "valid": True, + "path": "", + "nexthops": [ + {"ip": "10.0.0.2", "hostname": "r2", "afi": "ipv4"} + ], + } + ], + "10.0.0.3/32": [ + { + "valid": True, + "path": "65003", + "nexthops": [ + {"ip": "10.0.0.3", "hostname": "r3", "afi": "ipv4"} + ], + } + ], + } + } + + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_converge, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Can't converge" + + def _bgp_check_received_routes(): + output = json.loads(r3.vtysh_cmd("show bgp ipv4 unicast json")) + expected = { + "routes": { + "10.0.0.2/32": [ + { + "valid": True, + "bestpath": True, + "nexthops": [ + {"ip": "10.0.0.1", "hostname": "r1", "afi": "ipv4"} + ], + } + ], + } + } + + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_received_routes, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Can't see 10.0.0.2/32" + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args))