Merge pull request #17410 from opensourcerouting/fix/bgpd_ebgp_multihop_set_unset

BGP BFD session things

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");
 
@@ -151,23 +152,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)
+	if (CHECK_FLAG(p->flags, PEER_FLAG_UPDATE_SOURCE)) {
-		source = p->update_source;
+		if (p->update_source) {
-	else
+			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);

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;
 

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,
+			for (ALL_LIST_ELEMENTS(group->peer, node, nnode, member)) {
-					       peer1)) {
+				if (member->sort == BGP_PEER_IBGP)
-				if (peer1->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)) {
+		for (ALL_LIST_ELEMENTS(group->peer, node, nnode, member)) {
-			if (peer->sort == BGP_PEER_IBGP)
+			if (member->sort == BGP_PEER_IBGP)
 				continue;
 
-			peer->ttl = group->conf->ttl;
+			member->ttl = group->conf->ttl;
 
-			if (BGP_IS_VALID_STATE_FOR_NOTIF(
+			if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))
-				    peer->connection->status))
+				bgp_notify_send(member->connection, BGP_NOTIFY_CEASE,
-				bgp_notify_send(peer->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)
+			if (member->bfd_config)
-				bgp_peer_bfd_update_source(peer);
+				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)) {
+		for (ALL_LIST_ELEMENTS(group->peer, node, nnode, member)) {
-			if (peer->sort == BGP_PEER_IBGP)
+			if (member->sort == BGP_PEER_IBGP)
 				continue;
 
-			peer->ttl = BGP_DEFAULT_TTL;
+			member->ttl = BGP_DEFAULT_TTL;
 
-			if (peer->connection->fd >= 0) {
+			if (member->connection->fd >= 0) {
-				if (BGP_IS_VALID_STATE_FOR_NOTIF(
+				if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))
-					    peer->connection->status))
+					bgp_notify_send(member->connection, BGP_NOTIFY_CEASE,
-					bgp_notify_send(peer->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)
+			if (member->bfd_config)
-				bgp_peer_bfd_update_source(peer);
+				bgp_peer_bfd_update_source(member);
 		}
 	}
 	return 0;

tests/topotests/bgp_bfd_session/r1/frr.conf

@@ -0,0 +1,14 @@
+!
+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
+ 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

tests/topotests/bgp_bfd_session/test_bgp_bfd_session.py

@@ -0,0 +1,108 @@
+#!/usr/bin/env python
+# SPDX-License-Identifier: ISC
+
+#
+# Copyright (c) 2024 by
+# Donatas Abraitis <donatas@opensourcerouting.org>
+#
+
+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",
+            },
+            {
+                "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)
+
+    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))