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; } diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index b1f6e2d828..cd0d6def7d 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -9253,7 +9253,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; } @@ -14111,33 +14111,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( @@ -14146,33 +14141,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( 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__":