From 8bf9ea06118218148fddee0f9b5b4cdf4144adf4 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 3 Aug 2023 16:35:46 +0300 Subject: [PATCH 1/4] tests: Check if we can handle software version capability dynamicaly Signed-off-by: Donatas Abraitis --- .../bgp_dynamic_capability/__init__.py | 0 .../bgp_dynamic_capability/r1/bgpd.conf | 10 ++ .../bgp_dynamic_capability/r1/zebra.conf | 4 + .../bgp_dynamic_capability/r2/bgpd.conf | 10 ++ .../bgp_dynamic_capability/r2/zebra.conf | 4 + ...bgp_dynamic_capability_software_version.py | 153 ++++++++++++++++++ 6 files changed, 181 insertions(+) create mode 100644 tests/topotests/bgp_dynamic_capability/__init__.py create mode 100644 tests/topotests/bgp_dynamic_capability/r1/bgpd.conf create mode 100644 tests/topotests/bgp_dynamic_capability/r1/zebra.conf create mode 100644 tests/topotests/bgp_dynamic_capability/r2/bgpd.conf create mode 100644 tests/topotests/bgp_dynamic_capability/r2/zebra.conf create mode 100644 tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_software_version.py diff --git a/tests/topotests/bgp_dynamic_capability/__init__.py b/tests/topotests/bgp_dynamic_capability/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/topotests/bgp_dynamic_capability/r1/bgpd.conf b/tests/topotests/bgp_dynamic_capability/r1/bgpd.conf new file mode 100644 index 0000000000..113936df4c --- /dev/null +++ b/tests/topotests/bgp_dynamic_capability/r1/bgpd.conf @@ -0,0 +1,10 @@ +! +!debug bgp neighbor +! +router bgp 65001 + no bgp ebgp-requires-policy + neighbor 192.168.1.2 remote-as external + neighbor 192.168.1.2 timers 1 3 + neighbor 192.168.1.2 timers connect 1 + neighbor 192.168.1.2 capability dynamic +! diff --git a/tests/topotests/bgp_dynamic_capability/r1/zebra.conf b/tests/topotests/bgp_dynamic_capability/r1/zebra.conf new file mode 100644 index 0000000000..b29940f46a --- /dev/null +++ b/tests/topotests/bgp_dynamic_capability/r1/zebra.conf @@ -0,0 +1,4 @@ +! +int r1-eth0 + ip address 192.168.1.1/24 +! diff --git a/tests/topotests/bgp_dynamic_capability/r2/bgpd.conf b/tests/topotests/bgp_dynamic_capability/r2/bgpd.conf new file mode 100644 index 0000000000..587b241a90 --- /dev/null +++ b/tests/topotests/bgp_dynamic_capability/r2/bgpd.conf @@ -0,0 +1,10 @@ +! +!debug bgp neighbor +! +router bgp 65002 + no bgp ebgp-requires-policy + neighbor 192.168.1.1 remote-as external + neighbor 192.168.1.1 timers 1 3 + neighbor 192.168.1.1 timers connect 1 + neighbor 192.168.1.1 capability dynamic +! diff --git a/tests/topotests/bgp_dynamic_capability/r2/zebra.conf b/tests/topotests/bgp_dynamic_capability/r2/zebra.conf new file mode 100644 index 0000000000..cffe827363 --- /dev/null +++ b/tests/topotests/bgp_dynamic_capability/r2/zebra.conf @@ -0,0 +1,4 @@ +! +int r2-eth0 + ip address 192.168.1.2/24 +! diff --git a/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_software_version.py b/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_software_version.py new file mode 100644 index 0000000000..a375993af4 --- /dev/null +++ b/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_software_version.py @@ -0,0 +1,153 @@ +#!/usr/bin/env python +# SPDX-License-Identifier: ISC + +# Copyright (c) 2023 by +# Donatas Abraitis +# + +""" +Test if software version capability is exchanged 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 i, (rname, router) in enumerate(router_list.items(), 1): + router.load_config( + TopoRouter.RD_ZEBRA, os.path.join(CWD, "{}/zebra.conf".format(rname)) + ) + router.load_config( + TopoRouter.RD_BGP, os.path.join(CWD, "{}/bgpd.conf".format(rname)) + ) + + tgen.start_router() + + +def teardown_module(mod): + tgen = get_topogen() + tgen.stop_topology() + + +def test_bgp_dynamic_capability(): + 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", + "softwareVersion": { + "advertisedSoftwareVersion": None, + "receivedSoftwareVersion": None, + }, + }, + "connectionsEstablished": 1, + "connectionsDropped": 0, + } + } + 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 software version capability and check if it's exchanged dynamically") + + r1.vtysh_cmd( + """ + configure terminal + router bgp + neighbor 192.168.1.2 capability software-version + """ + ) + + r2.vtysh_cmd( + """ + configure terminal + router bgp + neighbor 192.168.1.1 capability software-version + """ + ) + + def _bgp_check_if_session_not_reset(): + def _bgp_software_version(): + try: + versions = output["192.168.1.2"]["neighborCapabilities"][ + "softwareVersion" + ] + adv = versions["advertisedSoftwareVersion"] + rcv = versions["receivedSoftwareVersion"] + + if not adv and not rcv: + return "" + + pattern = "FRRouting/\\d.+" + if re.search(pattern, adv) and re.search(pattern, rcv): + return adv, rcv + except: + return "" + + output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) + adv, rcv = _bgp_software_version() + expected = { + "192.168.1.2": { + "bgpState": "Established", + "neighborCapabilities": { + "dynamic": "advertisedAndReceived", + "softwareVersion": { + "advertisedSoftwareVersion": adv, + "receivedSoftwareVersion": rcv, + }, + }, + "connectionsEstablished": 1, + "connectionsDropped": 0, + } + } + 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 enabling software version capability" + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args)) From bf11a9eb252d7802871d3315e768068fb146a292 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 3 Aug 2023 16:37:54 +0300 Subject: [PATCH 2/4] bgpd: Handle software version capability dynamicaly We have dynamic capability support, but it handles only MP capability. With this change, we can enable software version capability dynamicaly, without resetting the session. Signed-off-by: Donatas Abraitis --- bgpd/bgp_packet.c | 98 +++++++++++++++++++++++++++++++++++++---------- bgpd/bgp_vty.c | 20 ++++++++-- bgpd/bgpd.c | 2 +- 3 files changed, 95 insertions(+), 25 deletions(-) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 2d1fc103bc..bb2e1dcfc9 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1199,6 +1199,8 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi, struct stream *s; iana_afi_t pkt_afi = IANA_AFI_IPV4; iana_safi_t pkt_safi = IANA_SAFI_UNICAST; + unsigned long cap_len; + uint16_t len; /* Convert AFI, SAFI to values for packet. */ bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi, &pkt_safi); @@ -1209,7 +1211,41 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi, bgp_packet_set_marker(s, BGP_MSG_CAPABILITY); /* Encode MP_EXT capability. */ - if (capability_code == CAPABILITY_CODE_MP) { + switch (capability_code) { + case CAPABILITY_CODE_SOFT_VERSION: + SET_FLAG(peer->cap, PEER_CAP_SOFT_VERSION_ADV); + stream_putc(s, action); + stream_putc(s, CAPABILITY_CODE_SOFT_VERSION); + cap_len = stream_get_endp(s); + stream_putc(s, 0); /* Capability Length */ + + /* The Capability Length SHOULD be no greater than 64. + * This is the limit to allow other capabilities as much + * space as they require. + */ + const char *soft_version = cmd_software_version_get(); + + len = strlen(soft_version); + if (len > BGP_MAX_SOFT_VERSION) + len = BGP_MAX_SOFT_VERSION; + + stream_putc(s, len); + stream_put(s, soft_version, len); + + /* Software Version capability Len. */ + len = stream_get_endp(s) - cap_len - 1; + stream_putc_at(s, cap_len, len); + + if (bgp_debug_neighbor_events(peer)) + zlog_debug("%pBP sending CAPABILITY has %s Software Version for afi/safi: %s/%s", + peer, + action == CAPABILITY_ACTION_SET + ? "Advertising" + : "Removing", + iana_afi2str(pkt_afi), + iana_safi2str(pkt_safi)); + break; + case CAPABILITY_CODE_MP: stream_putc(s, action); stream_putc(s, CAPABILITY_CODE_MP); stream_putc(s, CAPABILITY_CODE_MP_LEN); @@ -1224,6 +1260,9 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi, action == CAPABILITY_ACTION_SET ? "Advertising" : "Removing", iana_afi2str(pkt_afi), iana_safi2str(pkt_safi)); + break; + default: + break; } /* Set packet size. */ @@ -2698,6 +2737,7 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt, afi_t afi; iana_safi_t pkt_safi; safi_t safi; + char soft_version[BGP_MAX_SOFT_VERSION + 1] = {}; end = pnt + length; @@ -2728,14 +2768,6 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt, "%s CAPABILITY has action: %d, code: %u, length %u", peer->host, action, hdr->code, hdr->length); - if (hdr->length < sizeof(struct capability_mp_data)) { - zlog_info( - "%pBP Capability structure is not properly filled out, expected at least %zu bytes but header length specified is %d", - peer, sizeof(struct capability_mp_data), - hdr->length); - return BGP_Stop; - } - /* Capability length check. */ if ((pnt + hdr->length + 3) > end) { zlog_info("%s Capability length error", peer->host); @@ -2744,20 +2776,42 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt, return BGP_Stop; } - /* Fetch structure to the byte stream. */ - memcpy(&mpc, pnt + 3, sizeof(struct capability_mp_data)); - pnt += hdr->length + 3; + /* Ignore capability when override-capability is set. */ + if (CHECK_FLAG(peer->flags, PEER_FLAG_OVERRIDE_CAPABILITY)) + continue; - /* We know MP Capability Code. */ - if (hdr->code == CAPABILITY_CODE_MP) { + switch (hdr->code) { + case CAPABILITY_CODE_SOFT_VERSION: + if (action == CAPABILITY_ACTION_SET) { + SET_FLAG(peer->cap, PEER_CAP_SOFT_VERSION_RCV); + + memcpy(&soft_version, pnt + 3, hdr->length); + soft_version[hdr->length] = '\0'; + + XFREE(MTYPE_BGP_SOFT_VERSION, + peer->soft_version); + peer->soft_version = + XSTRDUP(MTYPE_BGP_SOFT_VERSION, + soft_version); + } else { + UNSET_FLAG(peer->cap, PEER_CAP_SOFT_VERSION_RCV); + XFREE(MTYPE_BGP_SOFT_VERSION, + peer->soft_version); + } + break; + case CAPABILITY_CODE_MP: + if (hdr->length < sizeof(struct capability_mp_data)) { + zlog_info("%pBP Capability structure is not properly filled out, expected at least %zu bytes but header length specified is %d", + peer, + sizeof(struct capability_mp_data), + hdr->length); + return BGP_Stop; + } + + memcpy(&mpc, pnt + 3, sizeof(struct capability_mp_data)); pkt_afi = ntohs(mpc.afi); pkt_safi = mpc.safi; - /* Ignore capability when override-capability is set. */ - if (CHECK_FLAG(peer->flags, - PEER_FLAG_OVERRIDE_CAPABILITY)) - continue; - /* Convert AFI, SAFI to internal values. */ if (bgp_map_afi_safi_iana2int(pkt_afi, pkt_safi, &afi, &safi)) { @@ -2797,12 +2851,16 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt, else return BGP_Stop; } - } else { + break; + default: flog_warn( EC_BGP_UNRECOGNIZED_CAPABILITY, "%s unrecognized capability code: %d - ignored", peer->host, hdr->code); + break; } + + pnt += hdr->length + 3; } /* No FSM action necessary */ diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 4f773f21a5..7464e99d48 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -5725,17 +5725,29 @@ DEFPY(neighbor_capability_software_version, "Advertise Software Version capability to the peer\n") { struct peer *peer; + int ret; peer = peer_and_group_lookup_vty(vty, neighbor); if (peer && peer->conf_if) return CMD_SUCCESS; if (no) - return peer_flag_unset_vty(vty, neighbor, - PEER_FLAG_CAPABILITY_SOFT_VERSION); + ret = peer_flag_unset_vty(vty, neighbor, + PEER_FLAG_CAPABILITY_SOFT_VERSION); else - return peer_flag_set_vty(vty, neighbor, - PEER_FLAG_CAPABILITY_SOFT_VERSION); + ret = peer_flag_set_vty(vty, neighbor, + PEER_FLAG_CAPABILITY_SOFT_VERSION); + + if (peer_established(peer)) { + if (CHECK_FLAG(peer->cap, PEER_CAP_DYNAMIC_RCV) && + CHECK_FLAG(peer->cap, PEER_CAP_DYNAMIC_ADV)) + bgp_capability_send(peer, AFI_IP, SAFI_UNICAST, + CAPABILITY_CODE_SOFT_VERSION, + no ? CAPABILITY_ACTION_UNSET + : CAPABILITY_ACTION_SET); + } + + return ret; } static int peer_af_flag_modify_vty(struct vty *vty, const char *peer_str, diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 84890da8c1..b72e75d12e 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -4449,7 +4449,7 @@ static const struct peer_flag_action peer_flag_action_list[] = { {PEER_FLAG_PORT, 0, peer_change_reset}, {PEER_FLAG_AIGP, 0, peer_change_none}, {PEER_FLAG_GRACEFUL_SHUTDOWN, 0, peer_change_none}, - {PEER_FLAG_CAPABILITY_SOFT_VERSION, 0, peer_change_reset}, + {PEER_FLAG_CAPABILITY_SOFT_VERSION, 0, peer_change_none}, {0, 0, 0}}; static const struct peer_flag_action peer_af_flag_action_list[] = { From f3279abe137e2e7083391c19901de5151eb959e2 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 3 Aug 2023 16:58:40 +0300 Subject: [PATCH 3/4] bgpd: Add all other capabilities for dynamic handling (placeholders) Gonna be covered later with further PRs. Now adding them to avoid compiler errors due to uncovered switch/cases. Signed-off-by: Donatas Abraitis --- bgpd/bgp_packet.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index bb2e1dcfc9..7c2c6f616b 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1261,6 +1261,19 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi, : "Removing", iana_afi2str(pkt_afi), iana_safi2str(pkt_safi)); break; + case CAPABILITY_CODE_REFRESH: + case CAPABILITY_CODE_ORF: + case CAPABILITY_CODE_RESTART: + case CAPABILITY_CODE_AS4: + case CAPABILITY_CODE_DYNAMIC: + case CAPABILITY_CODE_ADDPATH: + case CAPABILITY_CODE_ENHANCED_RR: + case CAPABILITY_CODE_LLGR: + case CAPABILITY_CODE_FQDN: + case CAPABILITY_CODE_ENHE: + case CAPABILITY_CODE_EXT_MESSAGE: + case CAPABILITY_CODE_ROLE: + break; default: break; } @@ -2852,6 +2865,19 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt, return BGP_Stop; } break; + case CAPABILITY_CODE_REFRESH: + case CAPABILITY_CODE_ORF: + case CAPABILITY_CODE_RESTART: + case CAPABILITY_CODE_AS4: + case CAPABILITY_CODE_DYNAMIC: + case CAPABILITY_CODE_ADDPATH: + case CAPABILITY_CODE_ENHANCED_RR: + case CAPABILITY_CODE_LLGR: + case CAPABILITY_CODE_FQDN: + case CAPABILITY_CODE_ENHE: + case CAPABILITY_CODE_EXT_MESSAGE: + case CAPABILITY_CODE_ROLE: + break; default: flog_warn( EC_BGP_UNRECOGNIZED_CAPABILITY, From 7636bcc76565a5791a3acf4b6aa2605e2221e700 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 3 Aug 2023 22:00:27 +0300 Subject: [PATCH 4/4] bgpd: Check if we have such a peer before handling software capability Do not pass NULL for peer_established(), just in case. Signed-off-by: Donatas Abraitis --- bgpd/bgp_vty.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 7464e99d48..591e0c3969 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -5728,8 +5728,8 @@ DEFPY(neighbor_capability_software_version, int ret; peer = peer_and_group_lookup_vty(vty, neighbor); - if (peer && peer->conf_if) - return CMD_SUCCESS; + if (!peer) + return CMD_WARNING_CONFIG_FAILED; if (no) ret = peer_flag_unset_vty(vty, neighbor,