From 6cc60e303f3766df0863b820d531cfe4fdf27805 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 29 Aug 2023 10:02:14 +0300 Subject: [PATCH 1/5] bgpd: Handle Graceful-Restart capability with dynamic capability Graceful-Restart restart time is exchanged using OPEN messages. In order to reduce restart time before doing an actual graceful restart, it might be useful to increase the time, but this is not possible without resetting the session. With this change, it's possible to send dynamic capability with a new value, and GR will respect a new reset time value. Signed-off-by: Donatas Abraitis --- bgpd/bgp_packet.c | 153 +++++++++++++++++++++++++++++++++++++++++++++- bgpd/bgp_vty.c | 23 +++++++ 2 files changed, 174 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 74291383dadc..97a68626e44a 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1201,6 +1201,7 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi, iana_safi_t pkt_safi = IANA_SAFI_UNICAST; unsigned long cap_len; uint16_t len; + uint32_t gr_restart_time; if (!peer_established(peer)) return; @@ -1268,9 +1269,64 @@ 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_RESTART: + if (!CHECK_FLAG(peer->flags, PEER_FLAG_GRACEFUL_RESTART) && + !CHECK_FLAG(peer->flags, PEER_FLAG_GRACEFUL_RESTART_HELPER)) + return; + + SET_FLAG(peer->cap, PEER_CAP_RESTART_ADV); + stream_putc(s, action); + stream_putc(s, CAPABILITY_CODE_RESTART); + cap_len = stream_get_endp(s); + stream_putc(s, 0); + gr_restart_time = peer->bgp->restart_time; + + if (peer->bgp->t_startup) { + SET_FLAG(gr_restart_time, GRACEFUL_RESTART_R_BIT); + SET_FLAG(peer->cap, PEER_CAP_GRACEFUL_RESTART_R_BIT_ADV); + } + + if (CHECK_FLAG(peer->bgp->flags, + BGP_FLAG_GRACEFUL_NOTIFICATION)) { + SET_FLAG(gr_restart_time, GRACEFUL_RESTART_N_BIT); + SET_FLAG(peer->cap, PEER_CAP_GRACEFUL_RESTART_N_BIT_ADV); + } + + stream_putw(s, gr_restart_time); + + if (CHECK_FLAG(peer->flags, PEER_FLAG_GRACEFUL_RESTART)) { + FOREACH_AFI_SAFI (afi, safi) { + if (!peer->afc[afi][safi]) + continue; + + bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi, + &pkt_safi); + stream_putw(s, pkt_afi); + stream_putc(s, pkt_safi); + if (CHECK_FLAG(peer->bgp->flags, + BGP_FLAG_GR_PRESERVE_FWD)) + stream_putc(s, GRACEFUL_RESTART_F_BIT); + else + stream_putc(s, 0); + } + } + + /* 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 Graceful-Restart CAP for afi/safi: %s/%s", + peer, + action == CAPABILITY_ACTION_SET + ? "Advertising" + : "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: @@ -2753,6 +2809,88 @@ static int bgp_route_refresh_receive(struct peer *peer, bgp_size_t size) return BGP_PACKET_NOOP; } +static void bgp_dynamic_capability_graceful_restart(uint8_t *pnt, int action, + struct capability_header *hdr, + struct peer *peer) +{ +#define GRACEFUL_RESTART_CAPABILITY_PER_AFI_SAFI_SIZE 4 + uint16_t gr_restart_flag_time; + uint8_t *data = pnt + 3; + uint8_t *end = pnt + hdr->length; + + if (action == CAPABILITY_ACTION_SET) { + SET_FLAG(peer->cap, PEER_CAP_RESTART_RCV); + ptr_get_be16(data, &gr_restart_flag_time); + data += sizeof(gr_restart_flag_time); + + if (CHECK_FLAG(gr_restart_flag_time, GRACEFUL_RESTART_R_BIT)) + SET_FLAG(peer->cap, PEER_CAP_GRACEFUL_RESTART_R_BIT_RCV); + else + UNSET_FLAG(peer->cap, + PEER_CAP_GRACEFUL_RESTART_R_BIT_RCV); + + if (CHECK_FLAG(gr_restart_flag_time, GRACEFUL_RESTART_N_BIT)) + SET_FLAG(peer->cap, PEER_CAP_GRACEFUL_RESTART_N_BIT_RCV); + else + UNSET_FLAG(peer->cap, + PEER_CAP_GRACEFUL_RESTART_N_BIT_RCV); + + UNSET_FLAG(gr_restart_flag_time, 0xF000); + peer->v_gr_restart = gr_restart_flag_time; + + while (data + GRACEFUL_RESTART_CAPABILITY_PER_AFI_SAFI_SIZE <= + end) { + afi_t afi; + safi_t safi; + iana_afi_t pkt_afi; + iana_safi_t pkt_safi; + struct graceful_restart_af graf; + + memcpy(&graf, data, sizeof(graf)); + pkt_afi = ntohs(graf.afi); + pkt_safi = safi_int2iana(graf.safi); + + /* Convert AFI, SAFI to internal values, check. */ + 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 Graceful Restart capability for this AFI/SAFI", + peer->host, + iana_afi2str(pkt_afi), + iana_safi2str(pkt_safi)); + } 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 Graceful Restart capability", + peer->host, + iana_afi2str(pkt_afi), + iana_safi2str(pkt_safi)); + } else { + if (bgp_debug_neighbor_events(peer)) + zlog_debug("%s Address family %s is%spreserved", + peer->host, + get_afi_safi_str(afi, safi, + false), + CHECK_FLAG(peer->af_cap[afi] + [safi], + PEER_CAP_RESTART_AF_PRESERVE_RCV) + ? " " + : " not "); + + SET_FLAG(peer->af_cap[afi][safi], + PEER_CAP_RESTART_AF_RCV); + if (CHECK_FLAG(graf.flag, + GRACEFUL_RESTART_F_BIT)) + SET_FLAG(peer->af_cap[afi][safi], + PEER_CAP_RESTART_AF_PRESERVE_RCV); + } + + data += GRACEFUL_RESTART_CAPABILITY_PER_AFI_SAFI_SIZE; + } + } else { + UNSET_FLAG(peer->cap, PEER_CAP_RESTART_RCV); + } +} + /** * Parse BGP CAPABILITY message for peer. * @@ -2886,9 +3024,20 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt, return BGP_Stop; } break; + case CAPABILITY_CODE_RESTART: + if ((hdr->length - 2) % 4) { + zlog_err("%pBP: Received invalid Graceful-Restart capability length %d", + peer, hdr->length); + bgp_notify_send(peer, BGP_NOTIFY_CEASE, + BGP_NOTIFY_SUBCODE_UNSPECIFIC); + return BGP_Stop; + } + + bgp_dynamic_capability_graceful_restart(pnt, action, + hdr, peer); + 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: diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index f4e901ef6ce5..f44b14f8cc20 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -3066,9 +3066,17 @@ DEFUN (bgp_graceful_restart_restart_time, VTY_DECLVAR_CONTEXT(bgp, bgp); int idx_number = 3; uint32_t restart; + struct listnode *node, *nnode; + struct peer *peer; restart = strtoul(argv[idx_number]->arg, NULL, 10); bgp->restart_time = restart; + + for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) + bgp_capability_send(peer, AFI_IP, SAFI_UNICAST, + CAPABILITY_CODE_RESTART, + CAPABILITY_ACTION_SET); + return CMD_SUCCESS; } @@ -3119,8 +3127,16 @@ DEFUN (no_bgp_graceful_restart_restart_time, "Delay value (seconds)\n") { VTY_DECLVAR_CONTEXT(bgp, bgp); + struct listnode *node, *nnode; + struct peer *peer; bgp->restart_time = BGP_DEFAULT_RESTART_TIME; + + for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) + bgp_capability_send(peer, AFI_IP, SAFI_UNICAST, + CAPABILITY_CODE_RESTART, + CAPABILITY_ACTION_UNSET); + return CMD_SUCCESS; } @@ -3175,12 +3191,19 @@ DEFPY (bgp_graceful_restart_notification, "Indicate Graceful Restart support for BGP NOTIFICATION messages\n") { VTY_DECLVAR_CONTEXT(bgp, bgp); + struct listnode *node, *nnode; + struct peer *peer; if (no) UNSET_FLAG(bgp->flags, BGP_FLAG_GRACEFUL_NOTIFICATION); else SET_FLAG(bgp->flags, BGP_FLAG_GRACEFUL_NOTIFICATION); + for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) + bgp_capability_send(peer, AFI_IP, SAFI_UNICAST, + CAPABILITY_CODE_RESTART, + CAPABILITY_ACTION_SET); + return CMD_SUCCESS; } From d64a4ec490d87d21d6b78b30f7b2a96ef8e0f4fa Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 29 Aug 2023 12:40:02 +0300 Subject: [PATCH 2/5] doc: GR restart time, notifcation flag can be changed via BGP dynamic cap Signed-off-by: Donatas Abraitis --- doc/user/bgp.rst | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/doc/user/bgp.rst b/doc/user/bgp.rst index 651aa36d0eea..ba6c16056084 100644 --- a/doc/user/bgp.rst +++ b/doc/user/bgp.rst @@ -840,7 +840,10 @@ The following functionality is provided by graceful restart: 1. The feature allows the restarting router to indicate to the helping peer the routes it can preserve in its forwarding plane during control plane restart by sending graceful restart capability in the OPEN message sent during - session establishment. + session establishment. Graceful restart notification flag and/or restart + time can also be changed during the dynamic BGP capabilities. If using + dynamic capabilities, no session reset is required, thus it's very useful + to increase restart time before doing a software upgrade or so. 2. The feature allows helping router to advertise to all other peers the routes received from the restarting router which are preserved in the forwarding plane of the restarting router during control plane restart. @@ -2160,8 +2163,8 @@ Using AS Path in Route Map Remove some AS numbers from the AS_PATH of the BGP path's NLRI. Removed AS numbers are conformant with the regex defined in as-path access-list WORD. The no form of this command removes this set operation from the route-map. - - + + .. _bgp-communities-attribute: Communities Attribute From 23fa9b4107e415113525987a66b1677dcafac743 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 29 Aug 2023 13:41:52 +0300 Subject: [PATCH 3/5] tests: Check if GR settings can be changed via BGP dynamic capabilities restart-time and/or notification support. Signed-off-by: Donatas Abraitis --- .../bgp_dynamic_capability/r1/bgpd.conf | 1 + .../bgp_dynamic_capability/r2/bgpd.conf | 1 + ...bgp_dynamic_capability_graceful_restart.py | 175 ++++++++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_graceful_restart.py diff --git a/tests/topotests/bgp_dynamic_capability/r1/bgpd.conf b/tests/topotests/bgp_dynamic_capability/r1/bgpd.conf index 113936df4c0f..ad2fe4cd0d67 100644 --- a/tests/topotests/bgp_dynamic_capability/r1/bgpd.conf +++ b/tests/topotests/bgp_dynamic_capability/r1/bgpd.conf @@ -3,6 +3,7 @@ ! router bgp 65001 no bgp ebgp-requires-policy + bgp graceful-restart neighbor 192.168.1.2 remote-as external neighbor 192.168.1.2 timers 1 3 neighbor 192.168.1.2 timers connect 1 diff --git a/tests/topotests/bgp_dynamic_capability/r2/bgpd.conf b/tests/topotests/bgp_dynamic_capability/r2/bgpd.conf index 587b241a90cf..9b7292163ffe 100644 --- a/tests/topotests/bgp_dynamic_capability/r2/bgpd.conf +++ b/tests/topotests/bgp_dynamic_capability/r2/bgpd.conf @@ -2,6 +2,7 @@ !debug bgp neighbor ! router bgp 65002 + bgp graceful-restart no bgp ebgp-requires-policy neighbor 192.168.1.1 remote-as external neighbor 192.168.1.1 timers 1 3 diff --git a/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_graceful_restart.py b/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_graceful_restart.py new file mode 100644 index 000000000000..a7feb3c580f4 --- /dev/null +++ b/tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_graceful_restart.py @@ -0,0 +1,175 @@ +#!/usr/bin/env python +# SPDX-License-Identifier: ISC + +# Copyright (c) 2023 by +# Donatas Abraitis +# + +""" +Test if BGP graceful restart capability's restart time and notification +flag are 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_graceful_restart(): + 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", + "gracefulRestart": "advertisedAndReceived", + }, + "gracefulRestartInfo": { + "nBit": True, + "timers": { + "receivedRestartTimer": 120, + }, + }, + "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("Change Graceful-Restart restart-time, and check if it's changed dynamically") + + r2.vtysh_cmd( + """ + configure terminal + router bgp + bgp graceful-restart restart-time 123 + """ + ) + + def _bgp_check_if_session_not_reset_after_changing_restart_time(): + output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) + expected = { + "192.168.1.2": { + "bgpState": "Established", + "neighborCapabilities": { + "dynamic": "advertisedAndReceived", + "gracefulRestart": "advertisedAndReceived", + }, + "gracefulRestartInfo": { + "nBit": True, + "timers": { + "receivedRestartTimer": 123, + }, + }, + "connectionsEstablished": 1, + "connectionsDropped": 0, + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_if_session_not_reset_after_changing_restart_time, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert ( + result is None + ), "Session was reset after changing Graceful-Restart restart-time" + + step( + "Disable Graceful-Restart notification support, and check if it's changed dynamically" + ) + + r2.vtysh_cmd( + """ + configure terminal + router bgp + no bgp graceful-restart notification + """ + ) + + def _bgp_check_if_session_not_reset_after_changing_notification(): + output = json.loads(r1.vtysh_cmd("show bgp neighbor json")) + expected = { + "192.168.1.2": { + "bgpState": "Established", + "neighborCapabilities": { + "dynamic": "advertisedAndReceived", + "gracefulRestart": "advertisedAndReceived", + }, + "gracefulRestartInfo": { + "nBit": False, + "timers": { + "receivedRestartTimer": 123, + }, + }, + "connectionsEstablished": 1, + "connectionsDropped": 0, + } + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_if_session_not_reset_after_changing_notification, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert ( + result is None + ), "Session was reset after changing Graceful-Restart notification support" + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args)) From 7d5873cdc46b51196f12181dae6069a15fb87d67 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Wed, 30 Aug 2023 17:29:11 +0300 Subject: [PATCH 4/5] bgpd: Make sure we have enough data to read restart time and flags for GR cap Just a safety check to avoid out of bound reading. Signed-off-by: Donatas Abraitis --- bgpd/bgp_packet.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 97a68626e44a..f59fd0a615a2 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -2817,8 +2817,15 @@ static void bgp_dynamic_capability_graceful_restart(uint8_t *pnt, int action, uint16_t gr_restart_flag_time; uint8_t *data = pnt + 3; uint8_t *end = pnt + hdr->length; + size_t len = end - data; if (action == CAPABILITY_ACTION_SET) { + if (len < sizeof(gr_restart_flag_time)) { + zlog_err("%pBP: Received invalid Graceful-Restart capability length %d", + peer, hdr->length); + return; + } + SET_FLAG(peer->cap, PEER_CAP_RESTART_RCV); ptr_get_be16(data, &gr_restart_flag_time); data += sizeof(gr_restart_flag_time); From 14e34520dd171e2cf53137c64809ada9596884f4 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Wed, 30 Aug 2023 17:30:27 +0300 Subject: [PATCH 5/5] bgpd: Print a hostname also for GR logs under dynamic capability Just to be consistent with other zlog_ stuff for dynamic capabilities. Signed-off-by: Donatas Abraitis --- bgpd/bgp_packet.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index f59fd0a615a2..0b9cfc8d0f5a 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -2861,20 +2861,18 @@ static void bgp_dynamic_capability_graceful_restart(uint8_t *pnt, int action, 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 Graceful Restart capability for this AFI/SAFI", - peer->host, - iana_afi2str(pkt_afi), + zlog_debug("%pBP: Addr-family %s/%s(afi/safi) not supported. Ignore the Graceful Restart capability for this AFI/SAFI", + peer, iana_afi2str(pkt_afi), iana_safi2str(pkt_safi)); } 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 Graceful Restart capability", - peer->host, - iana_afi2str(pkt_afi), + zlog_debug("%pBP: Addr-family %s/%s(afi/safi) not enabled. Ignore the Graceful Restart capability", + peer, iana_afi2str(pkt_afi), iana_safi2str(pkt_safi)); } else { if (bgp_debug_neighbor_events(peer)) - zlog_debug("%s Address family %s is%spreserved", - peer->host, + zlog_debug("%pBP: Address family %s is%spreserved", + peer, get_afi_safi_str(afi, safi, false), CHECK_FLAG(peer->af_cap[afi]