From 2a94de8af2678407859473cdb700d0fe2eb908cc Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 20 Nov 2024 09:18:39 -0500 Subject: [PATCH 1/3] bgpd: bgp_connect should return an `enum connect_result` This function when it is run by bgp_start is expected to return a `enum connect_result`. But instead the function returns a variety of values that are not really being checked for. Consolidate to a correct choice. Signed-off-by: Donald Sharp --- bgpd/bgp_fsm.c | 2 +- bgpd/bgp_network.c | 10 +++++----- bgpd/bgp_network.h | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index 8c9050185b32..4ac8201f749c 100644 --- a/bgpd/bgp_fsm.c +++ b/bgpd/bgp_fsm.c @@ -1826,7 +1826,7 @@ static void bgp_connect_in_progress_update_connection(struct peer *peer) static enum bgp_fsm_state_progress bgp_start(struct peer_connection *connection) { struct peer *peer = connection->peer; - int status; + enum connect_result status; bgp_peer_conf_if_to_su_update(connection); diff --git a/bgpd/bgp_network.c b/bgpd/bgp_network.c index de57d91806e1..844f6b9af2f1 100644 --- a/bgpd/bgp_network.c +++ b/bgpd/bgp_network.c @@ -762,7 +762,7 @@ static int bgp_update_source(struct peer_connection *connection) } /* BGP try to connect to the peer. */ -int bgp_connect(struct peer_connection *connection) +enum connect_result bgp_connect(struct peer_connection *connection) { struct peer *peer = connection->peer; @@ -773,7 +773,7 @@ int bgp_connect(struct peer_connection *connection) if (peer->conf_if && BGP_CONNECTION_SU_UNSPEC(connection)) { if (bgp_debug_neighbor_events(peer)) zlog_debug("Peer address not learnt: Returning from connect"); - return 0; + return connect_error; } frr_with_privs(&bgpd_privs) { /* Make socket for the peer. */ @@ -787,7 +787,7 @@ int bgp_connect(struct peer_connection *connection) zlog_debug("%s: Failure to create socket for connection to %s, error received: %s(%d)", __func__, peer->host, safe_strerror(errno), errno); - return -1; + return connect_error; } set_nonblocking(connection->fd); @@ -808,7 +808,7 @@ int bgp_connect(struct peer_connection *connection) __func__, peer->host, safe_strerror(errno), errno); - return -1; + return connect_error; } sockopt_reuseaddr(connection->fd); @@ -844,7 +844,7 @@ int bgp_connect(struct peer_connection *connection) /* If the peer is passive mode, force to move to Active mode. */ if (CHECK_FLAG(peer->flags, PEER_FLAG_PASSIVE)) { BGP_EVENT_ADD(connection, TCP_connection_open_failed); - return BGP_FSM_SUCCESS; + return connect_error; } if (peer->conf_if || peer->ifname) diff --git a/bgpd/bgp_network.h b/bgpd/bgp_network.h index ceb6b6f002e3..61ca19a34da5 100644 --- a/bgpd/bgp_network.h +++ b/bgpd/bgp_network.h @@ -21,7 +21,7 @@ extern int bgp_socket(struct bgp *bgp, unsigned short port, const char *address); extern void bgp_close_vrf_socket(struct bgp *bgp); extern void bgp_close(void); -extern int bgp_connect(struct peer_connection *connection); +extern enum connect_result bgp_connect(struct peer_connection *connection); extern int bgp_getsockname(struct peer *peer); extern void bgp_updatesockname(struct peer *peer); From 6a945b41049d36dc3cd41c6ff3d1c03a34d1d49f Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 20 Nov 2024 09:22:46 -0500 Subject: [PATCH 2/3] tests: zebra_fec_nexthop_resolution improve a) timers are really large preventing convergence in 30 seconds b) The same configuration does not need to be initiated 60 times when things are not working properly. Once is enough. Signed-off-by: Donald Sharp --- .../topotests/zebra_fec_nexthop_resolution/r1/bgpd.conf | 2 ++ .../topotests/zebra_fec_nexthop_resolution/r2/bgpd.conf | 1 + .../topotests/zebra_fec_nexthop_resolution/r3/bgpd.conf | 1 + .../topotests/zebra_fec_nexthop_resolution/r4/bgpd.conf | 1 + .../topotests/zebra_fec_nexthop_resolution/r5/bgpd.conf | 1 + .../topotests/zebra_fec_nexthop_resolution/r7/bgpd.conf | 2 ++ .../test_zebra_fec_nexthop_resolution.py | 9 +++++---- 7 files changed, 13 insertions(+), 4 deletions(-) diff --git a/tests/topotests/zebra_fec_nexthop_resolution/r1/bgpd.conf b/tests/topotests/zebra_fec_nexthop_resolution/r1/bgpd.conf index 9d28957d99eb..ccfec19e9b78 100644 --- a/tests/topotests/zebra_fec_nexthop_resolution/r1/bgpd.conf +++ b/tests/topotests/zebra_fec_nexthop_resolution/r1/bgpd.conf @@ -1,5 +1,6 @@ ! router bgp 65500 + timers bgp 3 9 bgp router-id 192.0.2.1 neighbor 192.0.2.3 remote-as 65500 neighbor 192.0.2.3 update-source lo @@ -7,6 +8,7 @@ router bgp 65500 neighbor 192.0.2.7 ttl-security hops 10 neighbor 192.0.2.7 disable-connected-check neighbor 192.0.2.7 update-source lo + neighbor 192.0.2.7 timers connect 5 ! address-family ipv4 unicast network 192.0.2.1/32 diff --git a/tests/topotests/zebra_fec_nexthop_resolution/r2/bgpd.conf b/tests/topotests/zebra_fec_nexthop_resolution/r2/bgpd.conf index 46d2c9a01d67..e02e7a4b29ac 100644 --- a/tests/topotests/zebra_fec_nexthop_resolution/r2/bgpd.conf +++ b/tests/topotests/zebra_fec_nexthop_resolution/r2/bgpd.conf @@ -1,4 +1,5 @@ router bgp 65500 + timers bgp 3 9 bgp router-id 192.0.2.2 neighbor 192.0.2.1 remote-as 65500 neighbor 192.0.2.1 update-source lo diff --git a/tests/topotests/zebra_fec_nexthop_resolution/r3/bgpd.conf b/tests/topotests/zebra_fec_nexthop_resolution/r3/bgpd.conf index 060777e7fe33..f2b22d7b3801 100644 --- a/tests/topotests/zebra_fec_nexthop_resolution/r3/bgpd.conf +++ b/tests/topotests/zebra_fec_nexthop_resolution/r3/bgpd.conf @@ -1,4 +1,5 @@ router bgp 65500 + timers bgp 3 9 bgp router-id 192.0.2.3 neighbor 192.0.2.1 remote-as 65500 neighbor 192.0.2.1 update-source lo diff --git a/tests/topotests/zebra_fec_nexthop_resolution/r4/bgpd.conf b/tests/topotests/zebra_fec_nexthop_resolution/r4/bgpd.conf index dc052da86347..d0f2f468bf29 100644 --- a/tests/topotests/zebra_fec_nexthop_resolution/r4/bgpd.conf +++ b/tests/topotests/zebra_fec_nexthop_resolution/r4/bgpd.conf @@ -1,5 +1,6 @@ ! router bgp 65500 + timers bgp 3 9 bgp router-id 192.0.2.4 neighbor 192.0.2.1 remote-as 65500 neighbor 192.0.2.1 ttl-security hops 10 diff --git a/tests/topotests/zebra_fec_nexthop_resolution/r5/bgpd.conf b/tests/topotests/zebra_fec_nexthop_resolution/r5/bgpd.conf index 1c73154e27bf..e2401eb1f902 100644 --- a/tests/topotests/zebra_fec_nexthop_resolution/r5/bgpd.conf +++ b/tests/topotests/zebra_fec_nexthop_resolution/r5/bgpd.conf @@ -1,4 +1,5 @@ router bgp 65500 + timers bgp 3 9 bgp router-id 192.0.2.5 neighbor 192.0.2.3 remote-as 65500 neighbor 192.0.2.3 update-source lo diff --git a/tests/topotests/zebra_fec_nexthop_resolution/r7/bgpd.conf b/tests/topotests/zebra_fec_nexthop_resolution/r7/bgpd.conf index eeda9d9cfa9d..325124e9f89d 100644 --- a/tests/topotests/zebra_fec_nexthop_resolution/r7/bgpd.conf +++ b/tests/topotests/zebra_fec_nexthop_resolution/r7/bgpd.conf @@ -1,10 +1,12 @@ ! router bgp 65500 + timers bgp 3 9 bgp router-id 192.0.2.7 neighbor 192.0.2.1 remote-as 65500 neighbor 192.0.2.1 ttl-security hops 10 neighbor 192.0.2.1 disable-connected-check neighbor 192.0.2.1 update-source lo + neighbor 192.0.2.1 timers connect 5 neighbor 192.0.2.5 remote-as 65500 neighbor 192.0.2.5 update-source lo ! diff --git a/tests/topotests/zebra_fec_nexthop_resolution/test_zebra_fec_nexthop_resolution.py b/tests/topotests/zebra_fec_nexthop_resolution/test_zebra_fec_nexthop_resolution.py index 984ff3c18526..e42070b4d6ed 100644 --- a/tests/topotests/zebra_fec_nexthop_resolution/test_zebra_fec_nexthop_resolution.py +++ b/tests/topotests/zebra_fec_nexthop_resolution/test_zebra_fec_nexthop_resolution.py @@ -156,16 +156,17 @@ def test_zebra_fec_nexthop_resolution_bgp(): def _check_bgp_session(): r1 = tgen.gears["r1"] - tgen.gears["r3"].vtysh_cmd("config \n no mpls fec nexthop-resolution \n end") - tgen.gears["r3"].vtysh_cmd("config \n mpls fec nexthop-resolution \n end") - tgen.gears["r5"].vtysh_cmd("config \n no mpls fec nexthop-resolution \n end") - tgen.gears["r5"].vtysh_cmd("config \n mpls fec nexthop-resolution \n end") output = json.loads(r1.vtysh_cmd("show bgp summary json")) if output["ipv4Unicast"]["peers"]["192.0.2.7"]["state"] == "Established": return None return False + tgen.gears["r3"].vtysh_cmd("config \n no mpls fec nexthop-resolution \n end") + tgen.gears["r3"].vtysh_cmd("config \n mpls fec nexthop-resolution \n end") + tgen.gears["r5"].vtysh_cmd("config \n no mpls fec nexthop-resolution \n end") + tgen.gears["r5"].vtysh_cmd("config \n mpls fec nexthop-resolution \n end") + test_func1 = functools.partial(_check_bgp_session) _, result1 = topotest.run_and_expect(test_func1, None, count=60, wait=0.5) assert result1 is None, "Failed to verify the fec_nexthop_resolution: bgp session" From c1c6298dd3d7fbcc5b19c1e3692aac9eb2e6cade Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 20 Nov 2024 16:07:34 -0500 Subject: [PATCH 3/3] bgpd: Allow bfd to work if peer known but interface address not yet If bgp is coming up and bgp has not received the interface address yet but bgp has knowledge about a bfd peering, allow it to set the peering data appropriately. Signed-off-by: Donald Sharp --- bgpd/bgp_bfd.c | 6 +++++- bgpd/bgp_nexthop.c | 28 +++++++++++++++++----------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/bgpd/bgp_bfd.c b/bgpd/bgp_bfd.c index a331585d3291..50b00d21b195 100644 --- a/bgpd/bgp_bfd.c +++ b/bgpd/bgp_bfd.c @@ -151,7 +151,7 @@ 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; + struct bfd_session_params *session; const union sockunion *source = NULL; bool changed = false; int family; @@ -162,6 +162,10 @@ void bgp_peer_bfd_update_source(struct peer *p) struct interface *ifp; union sockunion addr; + if (!p->bfd_config) + return; + + session = p->bfd_config->session; /* Nothing to do for groups. */ if (CHECK_FLAG(p->sflags, PEER_STATUS_GROUP)) return; diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index 357d5292dabb..bf0f3b15cfde 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -32,6 +32,7 @@ #include "bgpd/bgp_vty.h" #include "bgpd/bgp_rd.h" #include "bgpd/bgp_mplsvpn.h" +#include "bgpd/bgp_bfd.h" DEFINE_MTYPE_STATIC(BGPD, MARTIAN_STRING, "BGP Martian Addr Intf String"); @@ -409,17 +410,6 @@ void bgp_connected_add(struct bgp *bgp, struct connected *ifc) bgp_dest_set_bgp_connected_ref_info(dest, bc); } - for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) { - if (peer->conf_if && - (strcmp(peer->conf_if, ifc->ifp->name) == 0) && - !peer_established(peer->connection) && - !CHECK_FLAG(peer->flags, PEER_FLAG_IFPEER_V6ONLY)) { - connection = peer->connection; - if (peer_active(peer)) - BGP_EVENT_ADD(connection, BGP_Stop); - BGP_EVENT_ADD(connection, BGP_Start); - } - } } else if (addr->family == AF_INET6) { apply_mask_ipv6((struct prefix_ipv6 *)&p); @@ -443,6 +433,22 @@ void bgp_connected_add(struct bgp *bgp, struct connected *ifc) bgp_dest_set_bgp_connected_ref_info(dest, bc); } } + + /* + * Iterate over all the peers and attempt to set the bfd session + * data and if it's a bgp unnumbered get her flowing if necessary + */ + for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) { + bgp_peer_bfd_update_source(peer); + if (peer->conf_if && (strcmp(peer->conf_if, ifc->ifp->name) == 0) && + !peer_established(peer->connection) && + !CHECK_FLAG(peer->flags, PEER_FLAG_IFPEER_V6ONLY)) { + connection = peer->connection; + if (peer_active(peer)) + BGP_EVENT_ADD(connection, BGP_Stop); + BGP_EVENT_ADD(connection, BGP_Start); + } + } } void bgp_connected_delete(struct bgp *bgp, struct connected *ifc)