From 5f50359c8a828d74f0937335868512adaa598a3c Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 12 Mar 2024 21:00:50 +0200 Subject: [PATCH 1/4] bgpd: Show Addpath capability TX/RX flags unconditionally It's very annoying when testing and instead of looking for true/false, you have to check if the field exists. Signed-off-by: Donatas Abraitis --- bgpd/bgp_vty.c | 98 +++++++++++++++++++++++--------------------------- 1 file changed, 44 insertions(+), 54 deletions(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 2920405780..812f35eb89 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -14127,33 +14127,28 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, bool use_json, CHECK_FLAG( p->af_cap[afi][safi], PEER_CAP_ADDPATH_AF_TX_RCV)) { - if (CHECK_FLAG( - p->af_cap[afi] - [safi], - PEER_CAP_ADDPATH_AF_TX_ADV) && - CHECK_FLAG( - p->af_cap[afi] - [safi], - PEER_CAP_ADDPATH_AF_TX_RCV)) - json_object_boolean_true_add( - json_sub, - "txAdvertisedAndReceived"); - else if ( - CHECK_FLAG( - p->af_cap[afi] - [safi], - PEER_CAP_ADDPATH_AF_TX_ADV)) - json_object_boolean_true_add( - json_sub, - "txAdvertised"); - else if ( - CHECK_FLAG( - p->af_cap[afi] - [safi], - PEER_CAP_ADDPATH_AF_TX_RCV)) - json_object_boolean_true_add( - json_sub, - "txReceived"); + json_object_boolean_add( + json_sub, + "txAdvertisedAndReceived", + CHECK_FLAG(p->af_cap[afi] + [safi], + PEER_CAP_ADDPATH_AF_TX_ADV) && + CHECK_FLAG( + p->af_cap[afi] + [safi], + PEER_CAP_ADDPATH_AF_TX_RCV)); + + json_object_boolean_add( + json_sub, "txAdvertised", + CHECK_FLAG(p->af_cap[afi] + [safi], + PEER_CAP_ADDPATH_AF_TX_ADV)); + + json_object_boolean_add( + json_sub, "txReceived", + CHECK_FLAG(p->af_cap[afi] + [safi], + PEER_CAP_ADDPATH_AF_TX_RCV)); } if (CHECK_FLAG( @@ -14162,33 +14157,28 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, bool use_json, CHECK_FLAG( p->af_cap[afi][safi], PEER_CAP_ADDPATH_AF_RX_RCV)) { - if (CHECK_FLAG( - p->af_cap[afi] - [safi], - PEER_CAP_ADDPATH_AF_RX_ADV) && - CHECK_FLAG( - p->af_cap[afi] - [safi], - PEER_CAP_ADDPATH_AF_RX_RCV)) - json_object_boolean_true_add( - json_sub, - "rxAdvertisedAndReceived"); - else if ( - CHECK_FLAG( - p->af_cap[afi] - [safi], - PEER_CAP_ADDPATH_AF_RX_ADV)) - json_object_boolean_true_add( - json_sub, - "rxAdvertised"); - else if ( - CHECK_FLAG( - p->af_cap[afi] - [safi], - PEER_CAP_ADDPATH_AF_RX_RCV)) - json_object_boolean_true_add( - json_sub, - "rxReceived"); + json_object_boolean_add( + json_sub, + "rxAdvertisedAndReceived", + CHECK_FLAG(p->af_cap[afi] + [safi], + PEER_CAP_ADDPATH_AF_RX_ADV) && + CHECK_FLAG( + p->af_cap[afi] + [safi], + PEER_CAP_ADDPATH_AF_RX_RCV)); + + json_object_boolean_add( + json_sub, "rxAdvertised", + CHECK_FLAG(p->af_cap[afi] + [safi], + PEER_CAP_ADDPATH_AF_RX_ADV)); + + json_object_boolean_add( + json_sub, "rxReceived", + CHECK_FLAG(p->af_cap[afi] + [safi], + PEER_CAP_ADDPATH_AF_RX_RCV)); } if (CHECK_FLAG( From 56a23f056c6de342a28576c38295542adaa96d84 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 12 Mar 2024 21:14:13 +0200 Subject: [PATCH 2/4] bgpd: Set Paths Limit to 0 instead of unsetting the capability The capability should be untouched, and send 0 (unlimited) instead. Otherwise, we miss the capability and things are broken later until the session reset. Fixes: 72f0e06824c237238121b96c45845a57e5cfb59f ("bgpd: Implement Paths-Limit capability") Signed-off-by: Donatas Abraitis --- bgpd/bgp_vty.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 812f35eb89..06a90fc1b5 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -9261,7 +9261,7 @@ DEFPY (no_neighbor_addpath_paths_limit, peer->addpath_paths_limit[afi][safi].send = 0; bgp_capability_send(peer, afi, safi, CAPABILITY_CODE_PATHS_LIMIT, - CAPABILITY_ACTION_UNSET); + CAPABILITY_ACTION_SET); return ret; } From 6ff16b3439fac6c67e9fd78d63054a219cf54b93 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 12 Mar 2024 20:30:46 +0200 Subject: [PATCH 3/4] tests: Check if Paths-Limit capability is working dynamically Signed-off-by: Donatas Abraitis --- .../bgp_dynamic_capability/r1/frr.conf | 1 + .../bgp_dynamic_capability/r2/frr.conf | 1 + .../test_bgp_dynamic_capability_addpath.py | 160 ++++++++++++++---- 3 files changed, 130 insertions(+), 32 deletions(-) diff --git a/tests/topotests/bgp_dynamic_capability/r1/frr.conf b/tests/topotests/bgp_dynamic_capability/r1/frr.conf index aa5c3db9a6..c9594626f5 100644 --- a/tests/topotests/bgp_dynamic_capability/r1/frr.conf +++ b/tests/topotests/bgp_dynamic_capability/r1/frr.conf @@ -15,6 +15,7 @@ router bgp 65001 ! 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 ! 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 7f25665ed5..3cc1f1fc39 100644 --- a/tests/topotests/bgp_dynamic_capability/r2/frr.conf +++ b/tests/topotests/bgp_dynamic_capability/r2/frr.conf @@ -16,6 +16,7 @@ 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 addpath-rx-paths-limit 20 ! address-family ipv4 unicast redistribute connected 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 index 5202f51e28..7511d57e3e 100644 --- a/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_addpath.py +++ b/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_addpath.py @@ -6,7 +6,11 @@ # """ -Test if Addpath capability is adjusted dynamically. +Test if Addpath/Paths-Limit capabilities are adjusted dynamically. +T1: Enable Addpath/Paths-Limit capabilities and check if they are exchanged dynamically +T2: Disable paths limit and check if it's exchanged dynamically +T3: Disable Addpath capability RX and check if it's exchanged dynamically +T4: Disable Addpath capability and check if it's exchanged dynamically """ import os @@ -24,7 +28,6 @@ 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] @@ -47,7 +50,7 @@ def teardown_module(mod): tgen.stop_topology() -def test_bgp_dynamic_capability_addpath(): +def test_bgp_addpath_paths_limit(): tgen = get_topogen() if tgen.routers_have_failure(): @@ -56,7 +59,7 @@ def test_bgp_dynamic_capability_addpath(): r1 = tgen.gears["r1"] r2 = tgen.gears["r2"] - def _bgp_converge(): + def _converge(): output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) expected = { "192.168.1.2": { @@ -65,8 +68,19 @@ def test_bgp_dynamic_capability_addpath(): "dynamic": "advertisedAndReceived", "addPath": { "ipv4Unicast": { + "txAdvertisedAndReceived": False, "txAdvertised": True, + "txReceived": False, "rxAdvertisedAndReceived": True, + "rxAdvertised": True, + "rxReceived": True, + } + }, + "pathsLimit": { + "ipv4Unicast": { + "advertisedAndReceived": True, + "advertisedPathsLimit": 10, + "receivedPathsLimit": 20, } }, }, @@ -80,26 +94,26 @@ def test_bgp_dynamic_capability_addpath(): return topotest.json_cmp(output, expected) test_func = functools.partial( - _bgp_converge, + _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. + #### + # T1: Enable Addpath/Paths-Limit capabilities and check if they are exchanged dynamically + #### 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 + router bgp + address-family ipv4 unicast + neighbor 192.168.1.1 addpath-tx-all-paths + neighbor 192.168.1.1 addpath-rx-paths-limit 21 """ ) - def _bgp_check_if_addpath_rx_tx_and_session_not_reset(): + def _enable_addpath_paths_limit(): output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) expected = { "192.168.1.2": { @@ -109,7 +123,18 @@ def test_bgp_dynamic_capability_addpath(): "addPath": { "ipv4Unicast": { "txAdvertisedAndReceived": True, + "txAdvertised": True, + "txReceived": True, "rxAdvertisedAndReceived": True, + "rxAdvertised": True, + "rxReceived": True, + } + }, + "pathsLimit": { + "ipv4Unicast": { + "advertisedAndReceived": True, + "advertisedPathsLimit": 10, + "receivedPathsLimit": 21, } }, }, @@ -120,23 +145,76 @@ def test_bgp_dynamic_capability_addpath(): }, "messageStats": { "notificationsRecv": 0, - "capabilityRecv": 1, + "notificationsSent": 0, + "capabilityRecv": 2, }, } } return topotest.json_cmp(output, expected) test_func = functools.partial( - _bgp_check_if_addpath_rx_tx_and_session_not_reset, + _enable_addpath_paths_limit, ) _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) - assert result is None, "Session was reset after enabling Addpath capability" + assert ( + result is None + ), "Something went wrong when enabling Addpath/Paths-Limit capabilities" - step("Disable Addpath capability RX and check if it's exchanged dynamically") + ### + # T2: Disable paths limit and check if it's exchanged dynamically + ### + r2.vtysh_cmd( + """ + configure terminal + router bgp + address-family ipv4 unicast + no neighbor 192.168.1.1 addpath-rx-paths-limit + """ + ) - # 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") + def _disable_paths_limit(): + output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) + expected = { + "192.168.1.2": { + "bgpState": "Established", + "neighborCapabilities": { + "dynamic": "advertisedAndReceived", + "addPath": { + "ipv4Unicast": { + "txAdvertisedAndReceived": True, + "txAdvertised": True, + "txReceived": True, + "rxAdvertisedAndReceived": True, + "rxAdvertised": True, + "rxReceived": True, + } + }, + "pathsLimit": { + "ipv4Unicast": { + "advertisedAndReceived": True, + "advertisedPathsLimit": 10, + "receivedPathsLimit": 0, + } + }, + }, + "messageStats": { + "notificationsRecv": 0, + "notificationsSent": 0, + "capabilityRecv": 3, + }, + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _disable_paths_limit, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "Something went wrong after disabling paths limit" + + ### + # T3: Disable Addpath capability RX and check if it's exchanged dynamically + ### r2.vtysh_cmd( """ configure terminal @@ -146,7 +224,7 @@ def test_bgp_dynamic_capability_addpath(): """ ) - def _bgp_check_if_addpath_tx_and_session_not_reset(): + def _disable_addpath_rx(): output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) expected = { "192.168.1.2": { @@ -156,27 +234,39 @@ def test_bgp_dynamic_capability_addpath(): "addPath": { "ipv4Unicast": { "txAdvertisedAndReceived": True, + "txAdvertised": True, + "txReceived": True, + "rxAdvertisedAndReceived": False, "rxAdvertised": True, + "rxReceived": False, + } + }, + "pathsLimit": { + "ipv4Unicast": { + "advertisedAndReceived": True, + "advertisedPathsLimit": 10, + "receivedPathsLimit": 0, } }, }, "messageStats": { "notificationsRecv": 0, - "capabilityRecv": 1, + "notificationsSent": 0, + "capabilityRecv": 4, }, } } return topotest.json_cmp(output, expected) test_func = functools.partial( - _bgp_check_if_addpath_tx_and_session_not_reset, + _disable_addpath_rx, ) _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) - assert result is None, "Session was reset after disabling Addpath RX flags" + assert result is None, "Something went wrong 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") + ### + # T4: Disable Addpath capability and check if it's exchanged dynamically + ### r1.vtysh_cmd( """ configure terminal @@ -186,7 +276,7 @@ def test_bgp_dynamic_capability_addpath(): """ ) - def _bgp_check_if_addpath_capability_is_absent(): + def _disable_addpath(): output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) expected = { "192.168.1.2": { @@ -195,24 +285,30 @@ def test_bgp_dynamic_capability_addpath(): "dynamic": "advertisedAndReceived", "addPath": { "ipv4Unicast": { - "txAdvertisedAndReceived": None, - "txAdvertised": None, + "txAdvertisedAndReceived": False, + "txAdvertised": False, + "txReceived": True, + "rxAdvertisedAndReceived": False, "rxAdvertised": True, + "rxReceived": False, } }, }, "messageStats": { "notificationsRecv": 0, + "notificationsSent": 0, + "capabilitySent": 1, + "capabilityRecv": 4, }, } } return topotest.json_cmp(output, expected) test_func = functools.partial( - _bgp_check_if_addpath_capability_is_absent, + _disable_addpath, ) _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) - assert result is None, "Failed to disable Addpath capability" + assert result is None, "Something went wrong when disabling Addpath capability" if __name__ == "__main__": From 081f6520ff37b3fdd1cdaa8f58c53d0696d8057a Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 14 Mar 2024 09:45:18 +0200 Subject: [PATCH 4/4] bgpd: Avoid padding for bgp_paths_limit_capability struct When sending the packets over the network (dynamic capability) it reports 6 bytes instead of 5 bytes, and causes some issues between little/big endian machines. Signed-off-by: Donatas Abraitis --- bgpd/bgp_addpath.h | 2 +- bgpd/bgp_packet.c | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/bgpd/bgp_addpath.h b/bgpd/bgp_addpath.h index b19e63c946..c267ebe43e 100644 --- a/bgpd/bgp_addpath.h +++ b/bgpd/bgp_addpath.h @@ -25,7 +25,7 @@ struct bgp_paths_limit_capability { uint16_t afi; uint8_t safi; uint16_t paths_limit; -}; +} __attribute__((packed)); #define BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE 1 diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 8a36a3f4c8..78878013f8 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -3240,11 +3240,13 @@ static void bgp_dynamic_capability_paths_limit(uint8_t *pnt, int action, safi_t safi; iana_afi_t pkt_afi; iana_safi_t pkt_safi; + uint16_t paths_limit = 0; struct bgp_paths_limit_capability bpl = {}; memcpy(&bpl, data, sizeof(bpl)); pkt_afi = ntohs(bpl.afi); pkt_safi = safi_int2iana(bpl.safi); + paths_limit = ntohs(bpl.paths_limit); if (bgp_debug_neighbor_events(peer)) zlog_debug("%s OPEN has %s capability for afi/safi: %s/%s limit: %u", @@ -3252,8 +3254,7 @@ static void bgp_dynamic_capability_paths_limit(uint8_t *pnt, int action, lookup_msg(capcode_str, hdr->code, NULL), iana_afi2str(pkt_afi), - iana_safi2str(pkt_safi), - bpl.paths_limit); + iana_safi2str(pkt_safi), paths_limit); if (bgp_map_afi_safi_iana2int(pkt_afi, pkt_safi, &afi, &safi)) { @@ -3275,7 +3276,7 @@ static void bgp_dynamic_capability_paths_limit(uint8_t *pnt, int action, SET_FLAG(peer->af_cap[afi][safi], PEER_CAP_PATHS_LIMIT_AF_RCV); peer->addpath_paths_limit[afi][safi].receive = - bpl.paths_limit; + paths_limit; ignore: data += CAPABILITY_CODE_PATHS_LIMIT_LEN; }