From: Donatas Abraitis Date: Sat, 25 Jan 2025 18:28:26 +0000 (+0200) Subject: Revert "bgpd: Handle Addpath capability using dynamic capabilities" X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=513d333ca8cbbb8094962c46deb2904f53871cbd;p=matthieu%2Ffrr.git Revert "bgpd: Handle Addpath capability using dynamic capabilities" This reverts commit 05cf9d03b345393b8d63ffe9345c42debd8362b6. TL;DR; Handling BGP AddPath capability is not trivial (possible) dynamically. When the sender is AddPath-capable and sends NLRIs encoded with AddPath ID, and at the same time the receiver sends AddPath capability "disable-addpath-rx" (flag update) via dynamic capabilities, both peers are out of sync about the AddPath state. The receiver thinks already he's not AddPath-capable anymore, hence it tries to parse NLRIs as non-AddPath, while they are actually encoded as AddPath. AddPath capability itself does not provide (in RFC) any mechanism on backward compatible way to handle NLRIs if they come mixed (AddPath + non-AddPath). This explains why we have failures in our CI periodically. Signed-off-by: Donatas Abraitis --- diff --git a/bgpd/bgp_addpath.c b/bgpd/bgp_addpath.c index f391c13847..de4b4a48af 100644 --- a/bgpd/bgp_addpath.c +++ b/bgpd/bgp_addpath.c @@ -10,8 +10,6 @@ #include "bgp_addpath.h" #include "bgp_route.h" -#include "bgp_open.h" -#include "bgp_packet.h" static const struct bgp_addpath_strategy_names strat_names[BGP_ADDPATH_MAX] = { { @@ -361,31 +359,6 @@ void bgp_addpath_type_changed(struct bgp *bgp) } } -int bgp_addpath_capability_action(enum bgp_addpath_strat addpath_type, - uint8_t paths) -{ - int action = CAPABILITY_ACTION_UNSET; - - switch (addpath_type) { - case BGP_ADDPATH_ALL: - case BGP_ADDPATH_BEST_PER_AS: - action = CAPABILITY_ACTION_SET; - break; - case BGP_ADDPATH_BEST_SELECTED: - if (paths) - action = CAPABILITY_ACTION_SET; - else - action = CAPABILITY_ACTION_UNSET; - break; - case BGP_ADDPATH_NONE: - case BGP_ADDPATH_MAX: - action = CAPABILITY_ACTION_UNSET; - break; - } - - return action; -} - /* * Change the addpath type assigned to a peer, or peer group. In addition to * adjusting the counts, peer sessions will be reset as needed to make the @@ -400,7 +373,6 @@ void bgp_addpath_set_peer_type(struct peer *peer, afi_t afi, safi_t safi, struct listnode *node, *nnode; struct peer *tmp_peer; struct peer_group *group; - int action = bgp_addpath_capability_action(addpath_type, paths); if (safi == SAFI_LABELED_UNICAST) safi = SAFI_UNICAST; @@ -458,12 +430,9 @@ void bgp_addpath_set_peer_type(struct peer *peer, afi_t afi, safi_t safi, } } } else { - if (!CHECK_FLAG(peer->cap, PEER_CAP_DYNAMIC_RCV) && - !CHECK_FLAG(peer->cap, PEER_CAP_DYNAMIC_ADV)) - peer_change_action(peer, afi, safi, peer_change_reset); + peer_change_action(peer, afi, safi, peer_change_reset); } - bgp_capability_send(peer, afi, safi, CAPABILITY_CODE_ADDPATH, action); } /* diff --git a/bgpd/bgp_addpath.h b/bgpd/bgp_addpath.h index d562000e30..2b91d450bc 100644 --- a/bgpd/bgp_addpath.h +++ b/bgpd/bgp_addpath.h @@ -15,12 +15,6 @@ #include "bgpd/bgp_table.h" #include "lib/json.h" -struct bgp_addpath_capability { - uint16_t afi; - uint8_t safi; - uint8_t flags; -}; - #define BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE 1 void bgp_addpath_init_bgp_data(struct bgp_addpath_bgp_data *d); @@ -63,6 +57,4 @@ void bgp_addpath_update_ids(struct bgp *bgp, struct bgp_dest *dest, afi_t afi, safi_t safi); void bgp_addpath_type_changed(struct bgp *bgp); -extern int bgp_addpath_capability_action(enum bgp_addpath_strat addpath_type, - uint8_t paths); #endif diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index e72494857c..d33e9995af 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1212,8 +1212,6 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi, unsigned long cap_len; uint16_t len; uint32_t gr_restart_time; - uint8_t addpath_afi_safi_count = 0; - bool adv_addpath_tx = false; unsigned long number_of_orfs_p; uint8_t number_of_orfs = 0; const char *capability = lookup_msg(capcode_str, capability_code, @@ -1378,87 +1376,6 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi, COND_FLAG(peer->cap, PEER_CAP_LLGR_ADV, action == CAPABILITY_ACTION_SET); break; - case CAPABILITY_CODE_ADDPATH: - FOREACH_AFI_SAFI (afi, safi) { - if (peer->afc[afi][safi]) { - addpath_afi_safi_count++; - - /* Only advertise addpath TX if a feature that - * will use it is - * configured */ - if (peer->addpath_type[afi][safi] != - BGP_ADDPATH_NONE) - adv_addpath_tx = true; - - /* If we have enabled labeled unicast, we MUST check - * against unicast SAFI because addpath IDs are - * allocated under unicast SAFI, the same as the RIB - * is managed in unicast SAFI. - */ - if (safi == SAFI_LABELED_UNICAST) - if (peer->addpath_type[afi][SAFI_UNICAST] != - BGP_ADDPATH_NONE) - adv_addpath_tx = true; - } - } - - stream_putc(s, action); - stream_putc(s, CAPABILITY_CODE_ADDPATH); - stream_putc(s, CAPABILITY_CODE_ADDPATH_LEN * - addpath_afi_safi_count); - - FOREACH_AFI_SAFI (afi, safi) { - if (peer->afc[afi][safi]) { - bool adv_addpath_rx = - !CHECK_FLAG(peer->af_flags[afi][safi], - PEER_FLAG_DISABLE_ADDPATH_RX); - uint8_t flags = 0; - - /* Convert AFI, SAFI to values for packet. */ - bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi, - &pkt_safi); - - stream_putw(s, pkt_afi); - stream_putc(s, pkt_safi); - - if (adv_addpath_rx) { - SET_FLAG(flags, BGP_ADDPATH_RX); - SET_FLAG(peer->af_cap[afi][safi], - PEER_CAP_ADDPATH_AF_RX_ADV); - } else { - UNSET_FLAG(peer->af_cap[afi][safi], - PEER_CAP_ADDPATH_AF_RX_ADV); - } - - if (adv_addpath_tx) { - SET_FLAG(flags, BGP_ADDPATH_TX); - SET_FLAG(peer->af_cap[afi][safi], - PEER_CAP_ADDPATH_AF_TX_ADV); - if (safi == SAFI_LABELED_UNICAST) - SET_FLAG(peer->af_cap[afi] - [SAFI_UNICAST], - PEER_CAP_ADDPATH_AF_TX_ADV); - } else { - UNSET_FLAG(peer->af_cap[afi][safi], - PEER_CAP_ADDPATH_AF_TX_ADV); - } - - stream_putc(s, flags); - } - } - - if (bgp_debug_neighbor_events(peer)) - zlog_debug("%pBP sending CAPABILITY has %s %s for afi/safi: %s/%s", - peer, - action == CAPABILITY_ACTION_SET - ? "Advertising" - : "Removing", - capability, iana_afi2str(pkt_afi), - iana_safi2str(pkt_safi)); - - COND_FLAG(peer->cap, PEER_CAP_ADDPATH_ADV, - action == CAPABILITY_ACTION_SET); - break; case CAPABILITY_CODE_ORF: /* Convert AFI, SAFI to values for packet. */ bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi, &pkt_safi); @@ -1571,6 +1488,7 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi, case CAPABILITY_CODE_REFRESH: case CAPABILITY_CODE_AS4: case CAPABILITY_CODE_DYNAMIC: + case CAPABILITY_CODE_ADDPATH: case CAPABILITY_CODE_ENHANCED_RR: case CAPABILITY_CODE_ENHE: case CAPABILITY_CODE_EXT_MESSAGE: @@ -3063,108 +2981,6 @@ static int bgp_route_refresh_receive(struct peer_connection *connection, return BGP_PACKET_NOOP; } -static void bgp_dynamic_capability_addpath(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; - afi_t afi; - safi_t safi; - - if (action == CAPABILITY_ACTION_SET) { - if (len % CAPABILITY_CODE_ADDPATH_LEN) { - flog_warn(EC_BGP_CAPABILITY_INVALID_LENGTH, - "Add Path: Received invalid length %zu, non-multiple of 4", - len); - return; - } - - SET_FLAG(peer->cap, PEER_CAP_ADDPATH_RCV); - - while (data + CAPABILITY_CODE_ADDPATH_LEN <= end) { - afi_t afi; - safi_t safi; - iana_afi_t pkt_afi; - iana_safi_t pkt_safi; - struct bgp_addpath_capability bac; - - memcpy(&bac, data, sizeof(bac)); - pkt_afi = ntohs(bac.afi); - pkt_safi = safi_int2iana(bac.safi); - - /* If any other value (other than 1-3) is received, - * then the capability SHOULD be treated as not - * understood and ignored. - */ - if (!bac.flags || bac.flags > 3) { - flog_warn(EC_BGP_CAPABILITY_INVALID_LENGTH, - "Add Path: Received invalid send/receive value %u in Add Path capability", - bac.flags); - goto ignore; - } - - if (bgp_debug_neighbor_events(peer)) - zlog_debug("%s OPEN has %s capability for afi/safi: %s/%s%s%s", - peer->host, - lookup_msg(capcode_str, hdr->code, - NULL), - iana_afi2str(pkt_afi), - iana_safi2str(pkt_safi), - (bac.flags & BGP_ADDPATH_RX) - ? ", receive" - : "", - (bac.flags & BGP_ADDPATH_TX) - ? ", transmit" - : ""); - - if (bgp_map_afi_safi_iana2int(pkt_afi, pkt_safi, &afi, - &safi)) { - if (bgp_debug_neighbor_events(peer)) - zlog_debug("%s Addr-family %s/%s(afi/safi) not supported. Ignore the Addpath Attribute for this AFI/SAFI", - peer->host, - iana_afi2str(pkt_afi), - iana_safi2str(pkt_safi)); - goto ignore; - } else if (!peer->afc[afi][safi]) { - if (bgp_debug_neighbor_events(peer)) - zlog_debug("%s Addr-family %s/%s(afi/safi) not enabled. Ignore the AddPath capability for this AFI/SAFI", - peer->host, - iana_afi2str(pkt_afi), - iana_safi2str(pkt_safi)); - goto ignore; - } - - if (CHECK_FLAG(bac.flags, BGP_ADDPATH_RX)) - SET_FLAG(peer->af_cap[afi][safi], - PEER_CAP_ADDPATH_AF_RX_RCV); - else - UNSET_FLAG(peer->af_cap[afi][safi], - PEER_CAP_ADDPATH_AF_RX_RCV); - - if (CHECK_FLAG(bac.flags, BGP_ADDPATH_TX)) - SET_FLAG(peer->af_cap[afi][safi], - PEER_CAP_ADDPATH_AF_TX_RCV); - else - UNSET_FLAG(peer->af_cap[afi][safi], - PEER_CAP_ADDPATH_AF_TX_RCV); - -ignore: - data += CAPABILITY_CODE_ADDPATH_LEN; - } - } else { - FOREACH_AFI_SAFI (afi, safi) { - UNSET_FLAG(peer->af_cap[afi][safi], - PEER_CAP_ADDPATH_AF_RX_RCV); - UNSET_FLAG(peer->af_cap[afi][safi], - PEER_CAP_ADDPATH_AF_TX_RCV); - } - - UNSET_FLAG(peer->cap, PEER_CAP_ADDPATH_RCV); - } -} - static void bgp_dynamic_capability_orf(uint8_t *pnt, int action, struct capability_header *hdr, struct peer *peer) @@ -3717,9 +3533,6 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt, case CAPABILITY_CODE_LLGR: bgp_dynamic_capability_llgr(pnt, action, hdr, peer); break; - case CAPABILITY_CODE_ADDPATH: - bgp_dynamic_capability_addpath(pnt, action, hdr, peer); - break; case CAPABILITY_CODE_ORF: bgp_dynamic_capability_orf(pnt, action, hdr, peer); break; @@ -3729,6 +3542,7 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt, case CAPABILITY_CODE_REFRESH: case CAPABILITY_CODE_AS4: case CAPABILITY_CODE_DYNAMIC: + case CAPABILITY_CODE_ADDPATH: case CAPABILITY_CODE_ENHANCED_RR: case CAPABILITY_CODE_ENHE: case CAPABILITY_CODE_EXT_MESSAGE: diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 8e27da54d8..8f6203f34c 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -9006,21 +9006,12 @@ DEFUN(neighbor_disable_addpath_rx, struct peer *peer; afi_t afi = bgp_node_afi(vty); safi_t safi = bgp_node_safi(vty); - int ret; - int action; peer = peer_and_group_lookup_vty(vty, peer_str); if (!peer) return CMD_WARNING_CONFIG_FAILED; - action = bgp_addpath_capability_action(peer->addpath_type[afi][safi], 0); - - ret = peer_af_flag_set_vty(vty, peer_str, afi, safi, - PEER_FLAG_DISABLE_ADDPATH_RX); - - bgp_capability_send(peer, afi, safi, CAPABILITY_CODE_ADDPATH, action); - - return ret; + return peer_af_flag_set_vty(vty, peer_str, afi, safi, PEER_FLAG_DISABLE_ADDPATH_RX); } DEFUN(no_neighbor_disable_addpath_rx, @@ -9035,21 +9026,12 @@ DEFUN(no_neighbor_disable_addpath_rx, struct peer *peer; afi_t afi = bgp_node_afi(vty); safi_t safi = bgp_node_safi(vty); - int ret; - int action; peer = peer_and_group_lookup_vty(vty, peer_str); if (!peer) return CMD_WARNING_CONFIG_FAILED; - action = bgp_addpath_capability_action(peer->addpath_type[afi][safi], 0); - - ret = peer_af_flag_unset_vty(vty, peer_str, afi, safi, - PEER_FLAG_DISABLE_ADDPATH_RX); - - bgp_capability_send(peer, afi, safi, CAPABILITY_CODE_ADDPATH, action); - - return ret; + return peer_af_flag_unset_vty(vty, peer_str, afi, safi, PEER_FLAG_DISABLE_ADDPATH_RX); } DEFUN (neighbor_addpath_tx_all_paths, @@ -9061,15 +9043,12 @@ DEFUN (neighbor_addpath_tx_all_paths, { int idx_peer = 1; struct peer *peer; - afi_t afi = bgp_node_afi(vty); - safi_t safi = bgp_node_safi(vty); peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg); if (!peer) return CMD_WARNING_CONFIG_FAILED; - bgp_addpath_set_peer_type(peer, afi, safi, BGP_ADDPATH_ALL, 0); - + bgp_addpath_set_peer_type(peer, bgp_node_afi(vty), bgp_node_safi(vty), BGP_ADDPATH_ALL, 0); return CMD_SUCCESS; } @@ -9089,20 +9068,18 @@ DEFUN (no_neighbor_addpath_tx_all_paths, { int idx_peer = 2; struct peer *peer; - afi_t afi = bgp_node_afi(vty); - safi_t safi = bgp_node_safi(vty); peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg); if (!peer) return CMD_WARNING_CONFIG_FAILED; - if (peer->addpath_type[afi][safi] != BGP_ADDPATH_ALL) { + if (peer->addpath_type[bgp_node_afi(vty)][bgp_node_safi(vty)] != BGP_ADDPATH_ALL) { vty_out(vty, "%% Peer not currently configured to transmit all paths."); return CMD_WARNING_CONFIG_FAILED; } - bgp_addpath_set_peer_type(peer, afi, safi, BGP_ADDPATH_NONE, 0); + bgp_addpath_set_peer_type(peer, bgp_node_afi(vty), bgp_node_safi(vty), BGP_ADDPATH_NONE, 0); return CMD_SUCCESS; } diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 852667fdba..d854367163 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -4632,7 +4632,7 @@ static const struct peer_flag_action peer_af_flag_action_list[] = { {PEER_FLAG_AS_OVERRIDE, 1, peer_change_reset_out}, {PEER_FLAG_REMOVE_PRIVATE_AS_ALL_REPLACE, 1, peer_change_reset_out}, {PEER_FLAG_WEIGHT, 0, peer_change_reset_in}, - {PEER_FLAG_DISABLE_ADDPATH_RX, 0, peer_change_none}, + {PEER_FLAG_DISABLE_ADDPATH_RX, 0, peer_change_reset}, {PEER_FLAG_SOO, 0, peer_change_reset}, {PEER_FLAG_ACCEPT_OWN, 0, peer_change_reset}, {PEER_FLAG_SEND_EXT_COMMUNITY_RPKI, 1, peer_change_reset_out}, diff --git a/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_addpath.py b/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_addpath.py deleted file mode 100644 index 5202f51e28..0000000000 --- a/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_addpath.py +++ /dev/null @@ -1,220 +0,0 @@ -#!/usr/bin/env python -# SPDX-License-Identifier: ISC - -# Copyright (c) 2023 by -# Donatas Abraitis -# - -""" -Test if Addpath capability is adjusted dynamically. -""" - -import os -import re -import sys -import json -import pytest -import functools - -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, TopoRouter, get_topogen -from lib.common_config import step - -pytestmark = [pytest.mark.bgpd] - - -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_addpath(): - 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 json")) - expected = { - "192.168.1.2": { - "bgpState": "Established", - "neighborCapabilities": { - "dynamic": "advertisedAndReceived", - "addPath": { - "ipv4Unicast": { - "txAdvertised": True, - "rxAdvertisedAndReceived": True, - } - }, - }, - "addressFamilyInfo": { - "ipv4Unicast": { - "acceptedPrefixCounter": 3, - } - }, - } - } - 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" - - step("Enable Addpath capability and check if it's exchanged dynamically") - - # Clear message stats to check if we receive a notification or not after we - # change the settings fo LLGR. - r1.vtysh_cmd("clear bgp 192.168.1.2 message-stats") - r2.vtysh_cmd( - """ - configure terminal - router bgp - address-family ipv4 unicast - neighbor 192.168.1.1 addpath-tx-all-paths - """ - ) - - def _bgp_check_if_addpath_rx_tx_and_session_not_reset(): - output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) - expected = { - "192.168.1.2": { - "bgpState": "Established", - "neighborCapabilities": { - "dynamic": "advertisedAndReceived", - "addPath": { - "ipv4Unicast": { - "txAdvertisedAndReceived": True, - "rxAdvertisedAndReceived": True, - } - }, - }, - "addressFamilyInfo": { - "ipv4Unicast": { - "acceptedPrefixCounter": 3, - } - }, - "messageStats": { - "notificationsRecv": 0, - "capabilityRecv": 1, - }, - } - } - return topotest.json_cmp(output, expected) - - test_func = functools.partial( - _bgp_check_if_addpath_rx_tx_and_session_not_reset, - ) - _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) - assert result is None, "Session was reset after enabling Addpath capability" - - step("Disable Addpath capability RX and check if it's exchanged dynamically") - - # Clear message stats to check if we receive a notification or not after we - # disable addpath-rx. - r1.vtysh_cmd("clear bgp 192.168.1.2 message-stats") - r2.vtysh_cmd( - """ - configure terminal - router bgp - address-family ipv4 unicast - neighbor 192.168.1.1 disable-addpath-rx - """ - ) - - def _bgp_check_if_addpath_tx_and_session_not_reset(): - output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) - expected = { - "192.168.1.2": { - "bgpState": "Established", - "neighborCapabilities": { - "dynamic": "advertisedAndReceived", - "addPath": { - "ipv4Unicast": { - "txAdvertisedAndReceived": True, - "rxAdvertised": True, - } - }, - }, - "messageStats": { - "notificationsRecv": 0, - "capabilityRecv": 1, - }, - } - } - return topotest.json_cmp(output, expected) - - test_func = functools.partial( - _bgp_check_if_addpath_tx_and_session_not_reset, - ) - _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) - assert result is None, "Session was reset after disabling Addpath RX flags" - - # Clear message stats to check if we receive a notification or not after we - # disable Addpath capability. - r1.vtysh_cmd("clear bgp 192.168.1.2 message-stats") - r1.vtysh_cmd( - """ - configure terminal - router bgp - address-family ipv4 unicast - no neighbor 192.168.1.2 addpath-tx-all-paths - """ - ) - - def _bgp_check_if_addpath_capability_is_absent(): - output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) - expected = { - "192.168.1.2": { - "bgpState": "Established", - "neighborCapabilities": { - "dynamic": "advertisedAndReceived", - "addPath": { - "ipv4Unicast": { - "txAdvertisedAndReceived": None, - "txAdvertised": None, - "rxAdvertised": True, - } - }, - }, - "messageStats": { - "notificationsRecv": 0, - }, - } - } - return topotest.json_cmp(output, expected) - - test_func = functools.partial( - _bgp_check_if_addpath_capability_is_absent, - ) - _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) - assert result is None, "Failed to disable Addpath capability" - - -if __name__ == "__main__": - args = ["-s"] + sys.argv[1:] - sys.exit(pytest.main(args))