From f1454d95aa4208c0a7892c485df91790e8db748b Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 7 Nov 2024 13:04:54 +0200 Subject: [PATCH 1/4] bgpd: Clear all paths including addpath once GR expires We iterated over all bgp_path_info's, but once we remove the path, we didn't check for other paths under the same bgp_dest. Signed-off-by: Donatas Abraitis (cherry picked from commit 7de464b00f1a188ec919abce56de6215f05fc4c0) --- bgpd/bgp_route.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index e158a8335f03..302feea58c4b 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -6279,7 +6279,6 @@ void bgp_clear_stale_route(struct peer *peer, afi_t afi, safi_t safi) vpn_leak_to_vrf_withdraw(pi); bgp_rib_remove(rm, pi, peer, afi, safi); - break; } } } else { @@ -6308,7 +6307,6 @@ void bgp_clear_stale_route(struct peer *peer, afi_t afi, safi_t safi) pi); bgp_rib_remove(dest, pi, peer, afi, safi); - break; } } } From 89801c2905de3f3aff145cdcb46f547a65fdd095 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 7 Nov 2024 13:08:35 +0200 Subject: [PATCH 2/4] bgpd: Set LLGR stale routes for all the paths including addpath Without this patch we set only the first path for the route (if multiple exist) as LLGR stale and stop doing that for the rest of the paths, which is wrong. Fixes: 1479ed2fb35f4a5ae1017201a7ee37ba2727163a ("bgpd: Implement LLGR helper mode") Signed-off-by: Donatas Abraitis (cherry picked from commit 895d586a5f4a15be0475296d7a5a374927d17dad) --- bgpd/bgp_fsm.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index cdd9b7ae4d51..94fca23e1849 100644 --- a/bgpd/bgp_fsm.c +++ b/bgpd/bgp_fsm.c @@ -688,6 +688,11 @@ static void bgp_set_llgr_stale(struct peer *peer, afi_t afi, safi_t safi) COMMUNITY_NO_LLGR)) continue; + if (bgp_attr_get_community(pi->attr) && + community_include(bgp_attr_get_community(pi->attr), + COMMUNITY_LLGR_STALE)) + continue; + if (bgp_debug_neighbor_events(peer)) zlog_debug( "%pBP Long-lived set stale community (LLGR_STALE) for: %pFX", @@ -698,7 +703,6 @@ static void bgp_set_llgr_stale(struct peer *peer, afi_t afi, safi_t safi) pi->attr = bgp_attr_intern(&attr); bgp_process(peer->bgp, rm, pi, afi, safi); - break; } } } else { @@ -715,6 +719,11 @@ static void bgp_set_llgr_stale(struct peer *peer, afi_t afi, safi_t safi) COMMUNITY_NO_LLGR)) continue; + if (bgp_attr_get_community(pi->attr) && + community_include(bgp_attr_get_community(pi->attr), + COMMUNITY_LLGR_STALE)) + continue; + if (bgp_debug_neighbor_events(peer)) zlog_debug( "%pBP Long-lived set stale community (LLGR_STALE) for: %pFX", @@ -724,7 +733,6 @@ static void bgp_set_llgr_stale(struct peer *peer, afi_t afi, safi_t safi) bgp_attr_add_llgr_community(&attr); pi->attr = bgp_attr_intern(&attr); bgp_process(peer->bgp, dest, pi, afi, safi); - break; } } } From 86532b3dc25e654d17a9e236a972278432b617ea Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 7 Nov 2024 13:17:12 +0200 Subject: [PATCH 3/4] tests: Check if routes with addpath are cleared if they are stale Signed-off-by: Donatas Abraitis (cherry picked from commit 444bdc4cc04908fbda08114fd2f845b3be088e01) --- .../bgp_addpath_graceful_restart/__init__.py | 0 .../bgp_addpath_graceful_restart/r1/frr.conf | 23 +++ .../bgp_addpath_graceful_restart/r2/frr.conf | 28 ++++ .../bgp_addpath_graceful_restart/r3/frr.conf | 13 ++ .../test_bgp_addpath_graceful_restart.py | 132 ++++++++++++++++++ 5 files changed, 196 insertions(+) create mode 100644 tests/topotests/bgp_addpath_graceful_restart/__init__.py create mode 100644 tests/topotests/bgp_addpath_graceful_restart/r1/frr.conf create mode 100644 tests/topotests/bgp_addpath_graceful_restart/r2/frr.conf create mode 100644 tests/topotests/bgp_addpath_graceful_restart/r3/frr.conf create mode 100644 tests/topotests/bgp_addpath_graceful_restart/test_bgp_addpath_graceful_restart.py diff --git a/tests/topotests/bgp_addpath_graceful_restart/__init__.py b/tests/topotests/bgp_addpath_graceful_restart/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/topotests/bgp_addpath_graceful_restart/r1/frr.conf b/tests/topotests/bgp_addpath_graceful_restart/r1/frr.conf new file mode 100644 index 000000000000..0644cf63a834 --- /dev/null +++ b/tests/topotests/bgp_addpath_graceful_restart/r1/frr.conf @@ -0,0 +1,23 @@ +! +int r1-eth0 + ip address 192.168.1.1/24 +! +int r1-eth1 + ip address 192.168.2.1/24 +! +router bgp 65001 + no bgp ebgp-requires-policy + no bgp network import-check + neighbor 192.168.1.2 remote-as auto + neighbor 192.168.1.2 timers 1 3 + neighbor 192.168.1.2 timers connect 1 + neighbor 192.168.2.2 remote-as auto + neighbor 192.168.2.2 timers 1 3 + neighbor 192.168.2.2 timers connect 1 + neighbor r1-eth1 interface remote-as auto + neighbor r1-eth1 timers 1 3 + neighbor r1-eth1 timers connect 1 + address-family ipv4 unicast + network 10.0.0.1/32 + exit-address-family +! diff --git a/tests/topotests/bgp_addpath_graceful_restart/r2/frr.conf b/tests/topotests/bgp_addpath_graceful_restart/r2/frr.conf new file mode 100644 index 000000000000..ad236e0680ce --- /dev/null +++ b/tests/topotests/bgp_addpath_graceful_restart/r2/frr.conf @@ -0,0 +1,28 @@ +! +int r2-eth0 + ip address 192.168.1.2/24 +! +int r2-eth1 + ip address 192.168.2.2/24 +! +int r2-eth2 + ip address 192.168.3.2/24 +! +router bgp 65002 + bgp graceful-restart + bgp graceful-restart preserve-fw-state + bgp graceful-restart restart-time 10 + no bgp ebgp-requires-policy + neighbor 192.168.1.1 remote-as auto + neighbor 192.168.1.1 timers 1 3 + neighbor 192.168.1.1 timers connect 1 + neighbor 192.168.2.1 remote-as auto + neighbor 192.168.2.1 timers 1 3 + neighbor 192.168.2.1 timers connect 1 + neighbor 192.168.3.3 remote-as auto + neighbor 192.168.3.3 timers 1 3 + neighbor 192.168.3.3 timers connect 1 + address-family ipv4 unicast + neighbor 192.168.3.3 addpath-tx-all-paths + exit-address-family +! diff --git a/tests/topotests/bgp_addpath_graceful_restart/r3/frr.conf b/tests/topotests/bgp_addpath_graceful_restart/r3/frr.conf new file mode 100644 index 000000000000..e864003f259b --- /dev/null +++ b/tests/topotests/bgp_addpath_graceful_restart/r3/frr.conf @@ -0,0 +1,13 @@ +! +int r3-eth0 + ip address 192.168.3.3/24 +! +router bgp 65003 + bgp graceful-restart + bgp graceful-restart preserve-fw-state + bgp graceful-restart restart-time 10 + no bgp ebgp-requires-policy + neighbor 192.168.3.2 remote-as auto + neighbor 192.168.3.2 timers 1 3 + neighbor 192.168.3.2 timers connect 1 +! diff --git a/tests/topotests/bgp_addpath_graceful_restart/test_bgp_addpath_graceful_restart.py b/tests/topotests/bgp_addpath_graceful_restart/test_bgp_addpath_graceful_restart.py new file mode 100644 index 000000000000..9088c44bdb0f --- /dev/null +++ b/tests/topotests/bgp_addpath_graceful_restart/test_bgp_addpath_graceful_restart.py @@ -0,0 +1,132 @@ +#!/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 +from lib.common_config import kill_router_daemons + +pytestmark = [pytest.mark.bgpd] + + +def setup_module(mod): + topodef = {"s1": ("r1", "r2"), "s2": ("r1", "r2"), "s3": ("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_addpath_graceful_restart(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + r2 = tgen.gears["r2"] + r3 = tgen.gears["r3"] + + def _bgp_converge(): + output = json.loads(r2.vtysh_cmd("show bgp ipv4 unicast summary json")) + expected = { + "peers": { + "192.168.1.1": { + "hostname": "r1", + "remoteAs": 65001, + "localAs": 65002, + "pfxRcd": 1, + "state": "Established", + }, + "192.168.2.1": { + "hostname": "r1", + "remoteAs": 65001, + "localAs": 65002, + "pfxRcd": 1, + "state": "Established", + }, + "192.168.3.3": { + "hostname": "r3", + "remoteAs": 65003, + "localAs": 65002, + "pfxSnt": 2, + "state": "Established", + }, + } + } + 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, "Initial peering failed" + + kill_router_daemons(tgen, "r2", ["bgpd"]) + + def _bgp_check_stale_routes(): + output = json.loads(r3.vtysh_cmd("show bgp ipv4 unicast json")) + expected = { + "routes": { + "10.0.0.1/32": [ + { + "stale": True, + "valid": True, + }, + { + "stale": True, + "valid": True, + "multipath": True, + }, + ] + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_stale_routes, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Can't see stale routes" + + def _bgp_check_stale_routes_cleared(): + output = json.loads(r3.vtysh_cmd("show bgp ipv4 unicast json")) + expected = { + "routes": { + "10.0.0.1/32": None, + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_stale_routes_cleared, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Can't see stale routes" + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args)) From 0b1bb842421723e183d9cd36b794d2dd9a2aed0d Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 7 Nov 2024 14:01:39 +0200 Subject: [PATCH 4/4] tests: Check if stale routes with addpath are marked with LLGR community Signed-off-by: Donatas Abraitis (cherry picked from commit 2e5e3b4bd097dc5b45dee476d844bde398ebee00) --- tests/topotests/bgp_addpath_llgr/__init__.py | 0 tests/topotests/bgp_addpath_llgr/r1/frr.conf | 23 +++ tests/topotests/bgp_addpath_llgr/r2/frr.conf | 29 ++++ tests/topotests/bgp_addpath_llgr/r3/frr.conf | 14 ++ .../bgp_addpath_llgr/test_bgp_addpath_llgr.py | 131 ++++++++++++++++++ 5 files changed, 197 insertions(+) create mode 100644 tests/topotests/bgp_addpath_llgr/__init__.py create mode 100644 tests/topotests/bgp_addpath_llgr/r1/frr.conf create mode 100644 tests/topotests/bgp_addpath_llgr/r2/frr.conf create mode 100644 tests/topotests/bgp_addpath_llgr/r3/frr.conf create mode 100644 tests/topotests/bgp_addpath_llgr/test_bgp_addpath_llgr.py diff --git a/tests/topotests/bgp_addpath_llgr/__init__.py b/tests/topotests/bgp_addpath_llgr/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/topotests/bgp_addpath_llgr/r1/frr.conf b/tests/topotests/bgp_addpath_llgr/r1/frr.conf new file mode 100644 index 000000000000..0644cf63a834 --- /dev/null +++ b/tests/topotests/bgp_addpath_llgr/r1/frr.conf @@ -0,0 +1,23 @@ +! +int r1-eth0 + ip address 192.168.1.1/24 +! +int r1-eth1 + ip address 192.168.2.1/24 +! +router bgp 65001 + no bgp ebgp-requires-policy + no bgp network import-check + neighbor 192.168.1.2 remote-as auto + neighbor 192.168.1.2 timers 1 3 + neighbor 192.168.1.2 timers connect 1 + neighbor 192.168.2.2 remote-as auto + neighbor 192.168.2.2 timers 1 3 + neighbor 192.168.2.2 timers connect 1 + neighbor r1-eth1 interface remote-as auto + neighbor r1-eth1 timers 1 3 + neighbor r1-eth1 timers connect 1 + address-family ipv4 unicast + network 10.0.0.1/32 + exit-address-family +! diff --git a/tests/topotests/bgp_addpath_llgr/r2/frr.conf b/tests/topotests/bgp_addpath_llgr/r2/frr.conf new file mode 100644 index 000000000000..7f6fff0fda59 --- /dev/null +++ b/tests/topotests/bgp_addpath_llgr/r2/frr.conf @@ -0,0 +1,29 @@ +! +int r2-eth0 + ip address 192.168.1.2/24 +! +int r2-eth1 + ip address 192.168.2.2/24 +! +int r2-eth2 + ip address 192.168.3.2/24 +! +router bgp 65002 + bgp graceful-restart + bgp graceful-restart preserve-fw-state + bgp graceful-restart restart-time 0 + bgp long-lived-graceful-restart stale-time 10 + no bgp ebgp-requires-policy + neighbor 192.168.1.1 remote-as auto + neighbor 192.168.1.1 timers 1 3 + neighbor 192.168.1.1 timers connect 1 + neighbor 192.168.2.1 remote-as auto + neighbor 192.168.2.1 timers 1 3 + neighbor 192.168.2.1 timers connect 1 + neighbor 192.168.3.3 remote-as auto + neighbor 192.168.3.3 timers 1 3 + neighbor 192.168.3.3 timers connect 1 + address-family ipv4 unicast + neighbor 192.168.3.3 addpath-tx-all-paths + exit-address-family +! diff --git a/tests/topotests/bgp_addpath_llgr/r3/frr.conf b/tests/topotests/bgp_addpath_llgr/r3/frr.conf new file mode 100644 index 000000000000..f36492d7e009 --- /dev/null +++ b/tests/topotests/bgp_addpath_llgr/r3/frr.conf @@ -0,0 +1,14 @@ +! +int r3-eth0 + ip address 192.168.3.3/24 +! +router bgp 65003 + bgp graceful-restart + bgp graceful-restart preserve-fw-state + bgp graceful-restart restart-time 0 + bgp long-lived-graceful-restart stale-time 10 + no bgp ebgp-requires-policy + neighbor 192.168.3.2 remote-as auto + neighbor 192.168.3.2 timers 1 3 + neighbor 192.168.3.2 timers connect 1 +! diff --git a/tests/topotests/bgp_addpath_llgr/test_bgp_addpath_llgr.py b/tests/topotests/bgp_addpath_llgr/test_bgp_addpath_llgr.py new file mode 100644 index 000000000000..fa0314304b3f --- /dev/null +++ b/tests/topotests/bgp_addpath_llgr/test_bgp_addpath_llgr.py @@ -0,0 +1,131 @@ +#!/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 +from lib.common_config import kill_router_daemons + +pytestmark = [pytest.mark.bgpd] + + +def setup_module(mod): + topodef = {"s1": ("r1", "r2"), "s2": ("r1", "r2"), "s3": ("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_addpath_llgr(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + r2 = tgen.gears["r2"] + r3 = tgen.gears["r3"] + + def _bgp_converge(): + output = json.loads(r2.vtysh_cmd("show bgp ipv4 unicast summary json")) + expected = { + "peers": { + "192.168.1.1": { + "hostname": "r1", + "remoteAs": 65001, + "localAs": 65002, + "pfxRcd": 1, + "state": "Established", + }, + "192.168.2.1": { + "hostname": "r1", + "remoteAs": 65001, + "localAs": 65002, + "pfxRcd": 1, + "state": "Established", + }, + "192.168.3.3": { + "hostname": "r3", + "remoteAs": 65003, + "localAs": 65002, + "pfxSnt": 2, + "state": "Established", + }, + } + } + 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, "Initial peering failed" + + kill_router_daemons(tgen, "r2", ["bgpd"]) + + def _bgp_check_stale_llgr_routes(): + output = json.loads(r3.vtysh_cmd("show bgp ipv4 unicast 10.0.0.1/32 json")) + expected = { + "paths": [ + { + "stale": True, + "valid": True, + "community": {"string": "llgr-stale", "list": ["llgrStale"]}, + }, + { + "stale": True, + "valid": True, + "community": {"string": "llgr-stale", "list": ["llgrStale"]}, + }, + ] + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_stale_llgr_routes, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Can't see stale LLGR routes" + + def _bgp_check_stale_routes_cleared(): + output = json.loads(r3.vtysh_cmd("show bgp ipv4 unicast json")) + expected = { + "routes": { + "10.0.0.1/32": None, + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_stale_routes_cleared, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Can't see stale routes" + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args))