From d1cfd730601e5063d126ca1e78be5695fe909a77 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 14 Jan 2025 18:56:48 +0200 Subject: [PATCH 1/2] tests: Check if ENHE capability can be handled dynamically Signed-off-by: Donatas Abraitis --- .../bgp_dynamic_capability/r1/frr.conf | 9 + .../bgp_dynamic_capability/r2/frr.conf | 12 + .../test_bgp_dynamic_capability_enhe.py | 215 ++++++++++++++++++ 3 files changed, 236 insertions(+) create mode 100644 tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_enhe.py diff --git a/tests/topotests/bgp_dynamic_capability/r1/frr.conf b/tests/topotests/bgp_dynamic_capability/r1/frr.conf index c9594626f5..d91913e15e 100644 --- a/tests/topotests/bgp_dynamic_capability/r1/frr.conf +++ b/tests/topotests/bgp_dynamic_capability/r1/frr.conf @@ -3,6 +3,7 @@ ! int r1-eth0 ip address 192.168.1.1/24 + ipv6 address 2001:db8::1/64 ! router bgp 65001 no bgp ebgp-requires-policy @@ -12,11 +13,19 @@ router bgp 65001 neighbor 192.168.1.2 timers 1 3 neighbor 192.168.1.2 timers connect 1 neighbor 192.168.1.2 capability dynamic + neighbor 2001:db8::2 remote-as external + neighbor 2001:db8::2 timers 1 3 + neighbor 2001:db8::2 timers connect 1 + neighbor 2001:db8::2 capability dynamic ! address-family ipv4 unicast neighbor 192.168.1.2 addpath-tx-all-paths neighbor 192.168.1.2 addpath-rx-paths-limit 10 exit-address-family + ! + address-family ipv6 unicast + neighbor 2001:db8::2 activate + exit-address-family ! ip prefix-list r2 seq 5 permit 10.10.10.10/32 ! diff --git a/tests/topotests/bgp_dynamic_capability/r2/frr.conf b/tests/topotests/bgp_dynamic_capability/r2/frr.conf index 3cc1f1fc39..621e9381e3 100644 --- a/tests/topotests/bgp_dynamic_capability/r2/frr.conf +++ b/tests/topotests/bgp_dynamic_capability/r2/frr.conf @@ -7,6 +7,7 @@ int lo ! int r2-eth0 ip address 192.168.1.2/24 + ipv6 address 2001:db8::2/64 ! router bgp 65002 bgp graceful-restart @@ -16,9 +17,20 @@ router bgp 65002 neighbor 192.168.1.1 timers 1 3 neighbor 192.168.1.1 timers connect 1 neighbor 192.168.1.1 capability dynamic + neighbor 192.168.1.1 capability extended-nexthop neighbor 192.168.1.1 addpath-rx-paths-limit 20 + neighbor 2001:db8::1 remote-as external + neighbor 2001:db8::1 timers 1 3 + neighbor 2001:db8::1 timers connect 1 + neighbor 2001:db8::1 capability dynamic + neighbor 2001:db8::1 capability extended-nexthop ! address-family ipv4 unicast redistribute connected exit-address-family + ! + address-family ipv6 unicast + redistribute connected + neighbor 2001:db8::1 activate + exit-address-family ! diff --git a/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_enhe.py b/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_enhe.py new file mode 100644 index 0000000000..aa0508e88f --- /dev/null +++ b/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_enhe.py @@ -0,0 +1,215 @@ +#!/usr/bin/env python +# SPDX-License-Identifier: ISC + +# Copyright (c) 2024 by +# Donatas Abraitis +# + +""" +Test if extended nexthop capability is exchanged dynamically. +""" + +import os +import sys +import json +import pytest +import functools +import time + +pytestmark = [pytest.mark.bgpd] + +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 step + + +def setup_module(mod): + topodef = {"s1": ("r1", "r2")} + 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_dynamic_capability_enhe(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + r1 = tgen.gears["r1"] + r2 = tgen.gears["r2"] + + def _bgp_converge(): + output = json.loads(r1.vtysh_cmd("show bgp neighbor 2001:db8::2 json")) + expected = { + "2001:db8::2": { + "bgpState": "Established", + "localRole": "undefined", + "remoteRole": "undefined", + "neighborCapabilities": { + "dynamic": "advertisedAndReceived", + "extendedNexthop": "received", + }, + } + } + 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_nexthop(): + output = json.loads(r1.vtysh_cmd("show ip route 10.10.10.10/32 json")) + expected = { + "10.10.10.10/32": [ + { + "protocol": "bgp", + "selected": True, + "installed": True, + "nexthops": [ + { + "fib": True, + "ip": "192.168.1.2", + "afi": "ipv4", + "interfaceName": "r1-eth0", + "active": True, + }, + { + "duplicate": True, + "ip": "192.168.1.2", + "afi": "ipv4", + "interfaceName": "r1-eth0", + "active": True, + }, + ], + } + ] + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_nexthop, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Can't see 10.10.10.10/32 with IPv4 only nexthops" + + step("Enable ENHE capability") + + # Clear message stats to check if we receive a notification or not after we + # change the role. + r1.vtysh_cmd("clear bgp 2001:db8::2 message-stats") + r1.vtysh_cmd( + """ + configure terminal + router bgp + neighbor 2001:db8::2 capability extended-nexthop + """ + ) + + def _bgp_check_if_session_not_reset(): + output = json.loads(r2.vtysh_cmd("show bgp neighbor 2001:db8::1 json")) + expected = { + "2001:db8::1": { + "bgpState": "Established", + "neighborCapabilities": { + "dynamic": "advertisedAndReceived", + "extendedNexthop": "advertisedAndReceived", + "extendedNexthopFamililesByPeer": { + "ipv4Unicast": "recieved", + }, + }, + "messageStats": { + "notificationsRecv": 0, + "capabilityRecv": 1, + }, + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_if_session_not_reset, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Session was reset after setting ENHE capability" + + def _bgp_check_if_session_not_reset(): + output = json.loads(r2.vtysh_cmd("show bgp neighbor 2001:db8::1 json")) + expected = { + "2001:db8::1": { + "bgpState": "Established", + "neighborCapabilities": { + "dynamic": "advertisedAndReceived", + "extendedNexthop": "advertisedAndReceived", + "extendedNexthopFamililesByPeer": { + "ipv4Unicast": "recieved", + }, + }, + "messageStats": { + "notificationsRecv": 0, + "capabilityRecv": 1, + }, + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_if_session_not_reset, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Session was reset after setting ENHE capability" + + def _bgp_check_nexthop_enhe(): + output = json.loads(r1.vtysh_cmd("show ip route 10.10.10.10/32 json")) + expected = { + "10.10.10.10/32": [ + { + "protocol": "bgp", + "selected": True, + "installed": True, + "nexthops": [ + { + "fib": True, + "ip": "192.168.1.2", + "afi": "ipv4", + "interfaceName": "r1-eth0", + "active": True, + }, + { + "fib": True, + "afi": "ipv6", + "interfaceName": "r1-eth0", + "active": True, + }, + ], + } + ] + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_nexthop_enhe, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Can't see 10.10.10.10/32 with IPv4 only nexthops" + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args)) From d60320c6d27e476f9e351bc5f1470197fa46623b Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 14 Jan 2025 18:57:45 +0200 Subject: [PATCH 2/2] bgpd: Handle ENHE capability via dynamic capability FRR supports dynamic capability which is useful to exchange the capabilities without tearing down the session. ENHE capability was missed to be included handling via dynamic capability. Let's add it too. This was missed and asked in Slack that it would be useful. Signed-off-by: Donatas Abraitis --- bgpd/bgp_packet.c | 110 +++++++++++++++++++++++++++++++++++++++++++++- bgpd/bgp_packet.h | 6 +++ bgpd/bgp_vty.c | 17 +++++-- bgpd/bgpd.c | 4 ++ 4 files changed, 131 insertions(+), 6 deletions(-) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index c5e390b045..ca2e8de041 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1620,9 +1620,38 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi, case CAPABILITY_CODE_AS4: case CAPABILITY_CODE_DYNAMIC: case CAPABILITY_CODE_ENHANCED_RR: - case CAPABILITY_CODE_ENHE: case CAPABILITY_CODE_EXT_MESSAGE: break; + case CAPABILITY_CODE_ENHE: + FOREACH_AFI_SAFI (afi, safi) { + if (!peer->afc[afi][safi]) + continue; + + bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi, &pkt_safi); + + if (CHECK_FLAG(peer->flags, PEER_FLAG_CAPABILITY_ENHE) && + peer->connection->su.sa.sa_family == AF_INET6 && afi == AFI_IP && + (safi == SAFI_UNICAST || safi == SAFI_MPLS_VPN || + safi == SAFI_LABELED_UNICAST)) { + stream_putc(s, action); + stream_putc(s, CAPABILITY_CODE_ENHE); + stream_putc(s, CAPABILITY_CODE_ENHE_LEN); + stream_putw(s, pkt_afi); + stream_putw(s, pkt_safi); + stream_putw(s, afi_int2iana(AFI_IP6)); + + COND_FLAG(peer->af_cap[AFI_IP][safi], PEER_CAP_ENHE_AF_ADV, + action == CAPABILITY_ACTION_SET); + + if (CHECK_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_RCV)) + COND_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_NEGO, + action == CAPABILITY_ACTION_SET); + } + } + COND_FLAG(peer->cap, PEER_CAP_ENHE_ADV, action == CAPABILITY_ACTION_SET); + update_group_adjust_peer_afs(peer); + bgp_announce_route_all(peer); + break; case CAPABILITY_CODE_ROLE: stream_putc(s, action); stream_putc(s, CAPABILITY_CODE_ROLE); @@ -3321,6 +3350,81 @@ ignore: } } +static void bgp_dynamic_capability_enhe(uint8_t *pnt, int action, struct capability_header *hdr, + struct peer *peer) +{ + uint8_t *data = pnt + 3; + uint8_t *end = data + hdr->length; + size_t len = end - data; + + if (data + CAPABILITY_CODE_ENHE_LEN > end) { + flog_warn(EC_BGP_CAPABILITY_INVALID_LENGTH, + "Extended NH: Received invalid length %zu, less than %d", len, + CAPABILITY_CODE_ENHE_LEN); + return; + } + + if (action == CAPABILITY_ACTION_SET) { + if (hdr->length % CAPABILITY_CODE_ENHE_LEN) { + flog_warn(EC_BGP_CAPABILITY_INVALID_LENGTH, + "Extended NH: Received invalid length %d, non-multiple of %d", + hdr->length, CAPABILITY_CODE_ENHE_LEN); + return; + } + + while (data + CAPABILITY_CODE_ENHE_LEN <= end) { + afi_t afi; + safi_t safi; + afi_t nh_afi; + struct bgp_enhe_capability bec = {}; + + memcpy(&bec, data, sizeof(bec)); + afi = ntohs(bec.afi); + safi = ntohs(bec.safi); + nh_afi = afi_iana2int(ntohs(bec.nh_afi)); + + /* RFC 5549 specifies use of this capability only for IPv4 AFI, + * with the Nexthop AFI being IPv6. A future spec may introduce + * other possibilities, so we ignore other values with a log. + * Also, only SAFI_UNICAST and SAFI_LABELED_UNICAST are currently + * supported (and expected). + */ + if (afi != AFI_IP || nh_afi != AFI_IP6 || + !(safi == SAFI_UNICAST || safi == SAFI_MPLS_VPN || + safi == SAFI_LABELED_UNICAST)) { + flog_warn(EC_BGP_CAPABILITY_INVALID_DATA, + "%s Unexpected afi/safi/next-hop afi: %s/%s/%u in Extended Next-hop capability, ignoring", + peer->host, afi2str(afi), safi2str(safi), nh_afi); + goto ignore; + } + + SET_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_RCV); + + if (CHECK_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_ADV)) + SET_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_NEGO); + +ignore: + data += CAPABILITY_CODE_ENHE_LEN; + } + + SET_FLAG(peer->cap, PEER_CAP_ENHE_RCV); + update_group_adjust_peer_afs(peer); + bgp_announce_route_all(peer); + } else { + afi_t afi; + safi_t safi; + + UNSET_FLAG(peer->cap, PEER_CAP_ENHE_RCV); + + FOREACH_AFI_SAFI (afi, safi) { + UNSET_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_RCV); + + if (CHECK_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_ADV)) + UNSET_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_NEGO); + } + } +} + static void bgp_dynamic_capability_orf(uint8_t *pnt, int action, struct capability_header *hdr, struct peer *peer) @@ -3943,9 +4047,11 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt, case CAPABILITY_CODE_AS4: case CAPABILITY_CODE_DYNAMIC: case CAPABILITY_CODE_ENHANCED_RR: - case CAPABILITY_CODE_ENHE: case CAPABILITY_CODE_EXT_MESSAGE: break; + case CAPABILITY_CODE_ENHE: + bgp_dynamic_capability_enhe(pnt, action, hdr, peer); + break; case CAPABILITY_CODE_ROLE: bgp_dynamic_capability_role(pnt, action, peer); break; diff --git a/bgpd/bgp_packet.h b/bgpd/bgp_packet.h index c266b17266..866b8f617d 100644 --- a/bgpd/bgp_packet.h +++ b/bgpd/bgp_packet.h @@ -8,6 +8,12 @@ #include "hook.h" +struct bgp_enhe_capability { + uint16_t afi; + uint16_t safi; + uint16_t nh_afi; +}; + DECLARE_HOOK(bgp_packet_dump, (struct peer *peer, uint8_t type, bgp_size_t size, struct stream *s), diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 56b06106f3..30b633416b 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -5977,13 +5977,17 @@ DEFUN (neighbor_capability_enhe, { int idx_peer = 1; struct peer *peer; + int ret; peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg); if (peer && peer->conf_if) return CMD_SUCCESS; - return peer_flag_set_vty(vty, argv[idx_peer]->arg, - PEER_FLAG_CAPABILITY_ENHE); + ret = peer_flag_set_vty(vty, argv[idx_peer]->arg, PEER_FLAG_CAPABILITY_ENHE); + + bgp_capability_send(peer, AFI_IP, SAFI_UNICAST, CAPABILITY_CODE_ENHE, CAPABILITY_ACTION_SET); + + return ret; } DEFUN (no_neighbor_capability_enhe, @@ -5997,6 +6001,7 @@ DEFUN (no_neighbor_capability_enhe, { int idx_peer = 2; struct peer *peer; + int ret; peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg); if (peer && peer->conf_if) { @@ -6006,8 +6011,12 @@ DEFUN (no_neighbor_capability_enhe, return CMD_WARNING_CONFIG_FAILED; } - return peer_flag_unset_vty(vty, argv[idx_peer]->arg, - PEER_FLAG_CAPABILITY_ENHE); + ret = peer_flag_unset_vty(vty, argv[idx_peer]->arg, PEER_FLAG_CAPABILITY_ENHE); + + bgp_capability_send(peer, AFI_IP, SAFI_UNICAST, CAPABILITY_CODE_ENHE, + CAPABILITY_ACTION_UNSET); + + return ret; } /* neighbor capability software-version */ diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 02333db1c2..977980dc49 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -4953,6 +4953,10 @@ static void peer_flag_modify_action(struct peer *peer, uint64_t flag) peer->v_start = BGP_INIT_START_TIMER; BGP_EVENT_ADD(peer->connection, BGP_Stop); } + } else if (CHECK_FLAG(peer->cap, PEER_CAP_DYNAMIC_RCV) && + CHECK_FLAG(peer->cap, PEER_CAP_DYNAMIC_ADV) && + flag == PEER_FLAG_CAPABILITY_ENHE) { + peer->last_reset = PEER_DOWN_CAPABILITY_CHANGE; } else if (!peer_notify_config_change(peer->connection)) bgp_session_reset(peer); }