From 6e92e255182229ae5742dbb86abebfd574b5d413 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 11 Nov 2024 16:48:59 +0200 Subject: [PATCH 1/5] bgpd: Do not use an existing peer pointer for ALL_LIST_ELEMENTS() Use a separate `member` in this case. Signed-off-by: Donatas Abraitis --- bgpd/bgpd.c | 48 ++++++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 7e98735c141b..aa2bd5c3719c 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -5394,7 +5394,7 @@ int peer_ebgp_multihop_set(struct peer *peer, int ttl) { struct peer_group *group; struct listnode *node, *nnode; - struct peer *peer1; + struct peer *member; if (peer->sort == BGP_PEER_IBGP || peer->conf_if) return 0; @@ -5410,12 +5410,11 @@ int peer_ebgp_multihop_set(struct peer *peer, int ttl) if (group->conf->gtsm_hops != BGP_GTSM_HOPS_DISABLED) return BGP_ERR_NO_EBGP_MULTIHOP_WITH_TTLHACK; - for (ALL_LIST_ELEMENTS(group->peer, node, nnode, - peer1)) { - if (peer1->sort == BGP_PEER_IBGP) + for (ALL_LIST_ELEMENTS(group->peer, node, nnode, member)) { + if (member->sort == BGP_PEER_IBGP) continue; - if (peer1->gtsm_hops != BGP_GTSM_HOPS_DISABLED) + if (member->gtsm_hops != BGP_GTSM_HOPS_DISABLED) return BGP_ERR_NO_EBGP_MULTIHOP_WITH_TTLHACK; } } else { @@ -5442,23 +5441,21 @@ int peer_ebgp_multihop_set(struct peer *peer, int ttl) } } else { group = peer->group; - for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) { - if (peer->sort == BGP_PEER_IBGP) + for (ALL_LIST_ELEMENTS(group->peer, node, nnode, member)) { + if (member->sort == BGP_PEER_IBGP) continue; - peer->ttl = group->conf->ttl; + member->ttl = group->conf->ttl; - if (BGP_IS_VALID_STATE_FOR_NOTIF( - peer->connection->status)) - bgp_notify_send(peer->connection, - BGP_NOTIFY_CEASE, + if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status)) + bgp_notify_send(member->connection, BGP_NOTIFY_CEASE, BGP_NOTIFY_CEASE_CONFIG_CHANGE); else - bgp_session_reset(peer); + bgp_session_reset(member); /* Reconfigure BFD peer with new TTL. */ - if (peer->bfd_config) - bgp_peer_bfd_update_source(peer); + if (member->bfd_config) + bgp_peer_bfd_update_source(member); } } return 0; @@ -5466,6 +5463,7 @@ int peer_ebgp_multihop_set(struct peer *peer, int ttl) int peer_ebgp_multihop_unset(struct peer *peer) { + struct peer *member; struct peer_group *group; struct listnode *node, *nnode; int ttl; @@ -5498,25 +5496,23 @@ int peer_ebgp_multihop_unset(struct peer *peer) bgp_peer_bfd_update_source(peer); } else { group = peer->group; - for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) { - if (peer->sort == BGP_PEER_IBGP) + for (ALL_LIST_ELEMENTS(group->peer, node, nnode, member)) { + if (member->sort == BGP_PEER_IBGP) continue; - peer->ttl = BGP_DEFAULT_TTL; + member->ttl = BGP_DEFAULT_TTL; - if (peer->connection->fd >= 0) { - if (BGP_IS_VALID_STATE_FOR_NOTIF( - peer->connection->status)) - bgp_notify_send(peer->connection, - BGP_NOTIFY_CEASE, + if (member->connection->fd >= 0) { + if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status)) + bgp_notify_send(member->connection, BGP_NOTIFY_CEASE, BGP_NOTIFY_CEASE_CONFIG_CHANGE); else - bgp_session_reset(peer); + bgp_session_reset(member); } /* Reconfigure BFD peer with new TTL. */ - if (peer->bfd_config) - bgp_peer_bfd_update_source(peer); + if (member->bfd_config) + bgp_peer_bfd_update_source(member); } } return 0; From 29eafd32c58c3b6e3ecb6f715aef74a17f22382a Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 11 Nov 2024 16:49:22 +0200 Subject: [PATCH 2/5] bgpd: Do not try to uninstall BFD session if the peer is not established Having something like: ``` neighbor 192.168.1.222 ebgp-multihop 32 neighbor 192.168.1.222 update-source 192.168.1.5 neighbor 192.168.1.222 bfd ``` Won't work and the result is (empty): ``` $ show bfd peers BFD Peers: ``` bgp_stop() is called in BGP FSM multiple times (even at startup) that causes intermediate session interruption when update-source/ebgp-multihop is triggered. With this fix, the ordering does not matter and the BFD session's parameters are updated correctly. Signed-off-by: Donatas Abraitis --- bgpd/bgp_fsm.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index 58e1ffa500c3..8c9050185b32 100644 --- a/bgpd/bgp_fsm.c +++ b/bgpd/bgp_fsm.c @@ -1344,11 +1344,6 @@ enum bgp_fsm_state_progress bgp_stop(struct peer_connection *connection) peer->nsf_af_count = 0; - /* deregister peer */ - if (peer->bfd_config - && peer->last_reset == PEER_DOWN_UPDATE_SOURCE_CHANGE) - bfd_sess_uninstall(peer->bfd_config->session); - if (peer_dynamic_neighbor_no_nsf(peer) && !(CHECK_FLAG(peer->flags, PEER_FLAG_DELETE))) { if (bgp_debug_neighbor_events(peer)) @@ -1368,6 +1363,10 @@ enum bgp_fsm_state_progress bgp_stop(struct peer_connection *connection) if (peer_established(connection)) { peer->dropped++; + if (peer->bfd_config && (peer->last_reset == PEER_DOWN_UPDATE_SOURCE_CHANGE || + peer->last_reset == PEER_DOWN_MULTIHOP_CHANGE)) + bfd_sess_uninstall(peer->bfd_config->session); + /* Notify BGP conditional advertisement process */ peer->advmap_table_change = true; From 55dcbd329286cbc01495b9d4e88c2cfaadcd4fa3 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 11 Nov 2024 17:07:45 +0200 Subject: [PATCH 3/5] tests: Check if BGP BFD session is created correctly with multihop/update-source Signed-off-by: Donatas Abraitis --- tests/topotests/bgp_bfd_session/__init__.py | 0 tests/topotests/bgp_bfd_session/r1/frr.conf | 10 ++ .../bgp_bfd_session/test_bgp_bfd_session.py | 97 +++++++++++++++++++ 3 files changed, 107 insertions(+) create mode 100644 tests/topotests/bgp_bfd_session/__init__.py create mode 100644 tests/topotests/bgp_bfd_session/r1/frr.conf create mode 100644 tests/topotests/bgp_bfd_session/test_bgp_bfd_session.py diff --git a/tests/topotests/bgp_bfd_session/__init__.py b/tests/topotests/bgp_bfd_session/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/topotests/bgp_bfd_session/r1/frr.conf b/tests/topotests/bgp_bfd_session/r1/frr.conf new file mode 100644 index 000000000000..1b248dc332d7 --- /dev/null +++ b/tests/topotests/bgp_bfd_session/r1/frr.conf @@ -0,0 +1,10 @@ +! +interface r1-eth0 + ip address 10.0.0.1/24 +! +router bgp 65000 + neighbor 192.168.1.2 remote-as auto + neighbor 192.168.1.2 bfd + neighbor 192.168.1.2 ebgp-multihop 10 + neighbor 192.168.1.2 update-source 10.0.0.1 +exit diff --git a/tests/topotests/bgp_bfd_session/test_bgp_bfd_session.py b/tests/topotests/bgp_bfd_session/test_bgp_bfd_session.py new file mode 100644 index 000000000000..6884670789ed --- /dev/null +++ b/tests/topotests/bgp_bfd_session/test_bgp_bfd_session.py @@ -0,0 +1,97 @@ +#!/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, TopoRouter +from lib.common_config import step + +pytestmark = [pytest.mark.bgpd] + + +def build_topo(tgen): + r1 = tgen.add_router("r1") + + switch = tgen.add_switch("s1") + switch.add_link(r1) + + +def setup_module(mod): + tgen = Topogen(build_topo, mod.__name__) + tgen.start_topology() + + for _, (rname, router) in enumerate(tgen.routers().items(), 1): + router.load_frr_config( + os.path.join(CWD, "{}/frr.conf".format(rname)), + [ + (TopoRouter.RD_ZEBRA, None), + (TopoRouter.RD_MGMTD, None), + (TopoRouter.RD_BFD, None), + (TopoRouter.RD_BGP, None), + ], + ) + + tgen.start_router() + + +def teardown_module(mod): + tgen = get_topogen() + tgen.stop_topology() + + +def test_bgp_bfd_session(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + r1 = tgen.gears["r1"] + + def _bfd_session(): + output = json.loads(r1.vtysh_cmd("show bfd peers json")) + expected = [ + { + "multihop": True, + "peer": "192.168.1.2", + "local": "10.0.0.1", + "vrf": "default", + "minimum-ttl": 246, + "status": "down", + "diagnostic": "ok", + "remote-diagnostic": "ok", + "type": "dynamic", + } + ] + return topotest.json_cmp(output, expected) + + test_func = functools.partial(_bfd_session) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Can't see BFD session created" + + +def test_memory_leak(): + "Run the memory leak test and report results." + tgen = get_topogen() + if not tgen.is_memleak_enabled(): + pytest.skip("Memory leak test/report is disabled") + + tgen.report_memory_leaks() + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args)) From fcabeeaf796358d6766397217863d86a5474546b Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 12 Nov 2024 13:09:09 +0200 Subject: [PATCH 4/5] bgpd: Update source address for BFD session If BFD is down, we should try to detect the source automatically from the given interface. Signed-off-by: Donatas Abraitis --- bgpd/bgp_bfd.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/bgpd/bgp_bfd.c b/bgpd/bgp_bfd.c index 14ff5f2e1151..2edc840fbf15 100644 --- a/bgpd/bgp_bfd.c +++ b/bgpd/bgp_bfd.c @@ -26,6 +26,7 @@ #include "bgpd/bgp_debug.h" #include "bgpd/bgp_vty.h" #include "bgpd/bgp_packet.h" +#include "bgpd/bgp_network.h" DEFINE_MTYPE_STATIC(BGPD, BFD_CONFIG, "BFD configuration data"); @@ -142,23 +143,40 @@ void bgp_peer_config_apply(struct peer *p, struct peer_group *pg) void bgp_peer_bfd_update_source(struct peer *p) { struct bfd_session_params *session = p->bfd_config->session; - const union sockunion *source; + const union sockunion *source = NULL; bool changed = false; int family; union { struct in_addr v4; struct in6_addr v6; } src, dst; + struct interface *ifp; + union sockunion addr; /* Nothing to do for groups. */ if (CHECK_FLAG(p->sflags, PEER_STATUS_GROUP)) return; /* Figure out the correct source to use. */ - if (CHECK_FLAG(p->flags, PEER_FLAG_UPDATE_SOURCE) && p->update_source) - source = p->update_source; - else + if (CHECK_FLAG(p->flags, PEER_FLAG_UPDATE_SOURCE)) { + if (p->update_source) { + source = p->update_source; + } else if (p->update_if) { + ifp = if_lookup_by_name(p->update_if, p->bgp->vrf_id); + if (ifp) { + sockunion_init(&addr); + if (bgp_update_address(ifp, &p->connection->su, &addr)) { + if (BGP_DEBUG(bfd, BFD_LIB)) + zlog_debug("%s: can't find the source address for interface %s", + __func__, p->update_if); + } + + source = &addr; + } + } + } else { source = p->su_local; + } /* Update peer's source/destination addresses. */ bfd_sess_addresses(session, &family, &src.v6, &dst.v6); From 2c08d9f824111a9deb0766ea8b708a50971d43b8 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 12 Nov 2024 13:12:56 +0200 Subject: [PATCH 5/5] tests: Check if BFD session is created with update-source (interface) Signed-off-by: Donatas Abraitis --- tests/topotests/bgp_bfd_session/r1/frr.conf | 4 ++++ .../topotests/bgp_bfd_session/test_bgp_bfd_session.py | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/tests/topotests/bgp_bfd_session/r1/frr.conf b/tests/topotests/bgp_bfd_session/r1/frr.conf index 1b248dc332d7..a1560b09fa89 100644 --- a/tests/topotests/bgp_bfd_session/r1/frr.conf +++ b/tests/topotests/bgp_bfd_session/r1/frr.conf @@ -7,4 +7,8 @@ router bgp 65000 neighbor 192.168.1.2 bfd neighbor 192.168.1.2 ebgp-multihop 10 neighbor 192.168.1.2 update-source 10.0.0.1 + neighbor 192.168.1.3 remote-as auto + neighbor 192.168.1.3 bfd + neighbor 192.168.1.3 ebgp-multihop 20 + neighbor 192.168.1.3 update-source r1-eth0 exit diff --git a/tests/topotests/bgp_bfd_session/test_bgp_bfd_session.py b/tests/topotests/bgp_bfd_session/test_bgp_bfd_session.py index 6884670789ed..adf557af7b1b 100644 --- a/tests/topotests/bgp_bfd_session/test_bgp_bfd_session.py +++ b/tests/topotests/bgp_bfd_session/test_bgp_bfd_session.py @@ -74,6 +74,17 @@ def _bfd_session(): "diagnostic": "ok", "remote-diagnostic": "ok", "type": "dynamic", + }, + { + "multihop": True, + "peer": "192.168.1.3", + "local": "10.0.0.1", + "vrf": "default", + "minimum-ttl": 236, + "status": "down", + "diagnostic": "ok", + "remote-diagnostic": "ok", + "type": "dynamic", } ] return topotest.json_cmp(output, expected)