Merge pull request #17855 from opensourcerouting/fix/bgp_enhe_capability_via_dynamic_capability

bgpd: Handle ENHE capability via dynamic capability

bgpd/bgp_packet.c

@@ -1620,9 +1620,38 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi,
 	case CAPABILITY_CODE_AS4:
 	case CAPABILITY_CODE_DYNAMIC:
 	case CAPABILITY_CODE_ENHANCED_RR:
-	case CAPABILITY_CODE_ENHE:
 	case CAPABILITY_CODE_EXT_MESSAGE:
 		break;
+	case CAPABILITY_CODE_ENHE:
+		FOREACH_AFI_SAFI (afi, safi) {
+			if (!peer->afc[afi][safi])
+				continue;
+
+			bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi, &pkt_safi);
+
+			if (CHECK_FLAG(peer->flags, PEER_FLAG_CAPABILITY_ENHE) &&
+			    peer->connection->su.sa.sa_family == AF_INET6 && afi == AFI_IP &&
+			    (safi == SAFI_UNICAST || safi == SAFI_MPLS_VPN ||
+			     safi == SAFI_LABELED_UNICAST)) {
+				stream_putc(s, action);
+				stream_putc(s, CAPABILITY_CODE_ENHE);
+				stream_putc(s, CAPABILITY_CODE_ENHE_LEN);
+				stream_putw(s, pkt_afi);
+				stream_putw(s, pkt_safi);
+				stream_putw(s, afi_int2iana(AFI_IP6));
+
+				COND_FLAG(peer->af_cap[AFI_IP][safi], PEER_CAP_ENHE_AF_ADV,
+					  action == CAPABILITY_ACTION_SET);
+
+				if (CHECK_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_RCV))
+					COND_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_NEGO,
+						  action == CAPABILITY_ACTION_SET);
+			}
+		}
+		COND_FLAG(peer->cap, PEER_CAP_ENHE_ADV, action == CAPABILITY_ACTION_SET);
+		update_group_adjust_peer_afs(peer);
+		bgp_announce_route_all(peer);
+		break;
 	case CAPABILITY_CODE_ROLE:
 		stream_putc(s, action);
 		stream_putc(s, CAPABILITY_CODE_ROLE);
@@ -3321,6 +3350,81 @@ ignore:
 	}
 }
 
+static void bgp_dynamic_capability_enhe(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;
+
+	if (data + CAPABILITY_CODE_ENHE_LEN > end) {
+		flog_warn(EC_BGP_CAPABILITY_INVALID_LENGTH,
+			  "Extended NH: Received invalid length %zu, less than %d", len,
+			  CAPABILITY_CODE_ENHE_LEN);
+		return;
+	}
+
+	if (action == CAPABILITY_ACTION_SET) {
+		if (hdr->length % CAPABILITY_CODE_ENHE_LEN) {
+			flog_warn(EC_BGP_CAPABILITY_INVALID_LENGTH,
+				  "Extended NH: Received invalid length %d, non-multiple of %d",
+				  hdr->length, CAPABILITY_CODE_ENHE_LEN);
+			return;
+		}
+
+		while (data + CAPABILITY_CODE_ENHE_LEN <= end) {
+			afi_t afi;
+			safi_t safi;
+			afi_t nh_afi;
+			struct bgp_enhe_capability bec = {};
+
+			memcpy(&bec, data, sizeof(bec));
+			afi = ntohs(bec.afi);
+			safi = ntohs(bec.safi);
+			nh_afi = afi_iana2int(ntohs(bec.nh_afi));
+
+			/* RFC 5549 specifies use of this capability only for IPv4 AFI,
+			 * with the Nexthop AFI being IPv6. A future spec may introduce
+			 * other possibilities, so we ignore other values with a log.
+			 * Also, only SAFI_UNICAST and SAFI_LABELED_UNICAST are currently
+			 * supported (and expected).
+			 */
+			if (afi != AFI_IP || nh_afi != AFI_IP6 ||
+			    !(safi == SAFI_UNICAST || safi == SAFI_MPLS_VPN ||
+			      safi == SAFI_LABELED_UNICAST)) {
+				flog_warn(EC_BGP_CAPABILITY_INVALID_DATA,
+					  "%s Unexpected afi/safi/next-hop afi: %s/%s/%u in Extended Next-hop capability, ignoring",
+					  peer->host, afi2str(afi), safi2str(safi), nh_afi);
+				goto ignore;
+			}
+
+			SET_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_RCV);
+
+			if (CHECK_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_ADV))
+				SET_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_NEGO);
+
+ignore:
+			data += CAPABILITY_CODE_ENHE_LEN;
+		}
+
+		SET_FLAG(peer->cap, PEER_CAP_ENHE_RCV);
+		update_group_adjust_peer_afs(peer);
+		bgp_announce_route_all(peer);
+	} else {
+		afi_t afi;
+		safi_t safi;
+
+		UNSET_FLAG(peer->cap, PEER_CAP_ENHE_RCV);
+
+		FOREACH_AFI_SAFI (afi, safi) {
+			UNSET_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_RCV);
+
+			if (CHECK_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_ADV))
+				UNSET_FLAG(peer->af_cap[afi][safi], PEER_CAP_ENHE_AF_NEGO);
+		}
+	}
+}
+
 static void bgp_dynamic_capability_orf(uint8_t *pnt, int action,
 				       struct capability_header *hdr,
 				       struct peer *peer)
@@ -3943,9 +4047,11 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt,
 		case CAPABILITY_CODE_AS4:
 		case CAPABILITY_CODE_DYNAMIC:
 		case CAPABILITY_CODE_ENHANCED_RR:
-		case CAPABILITY_CODE_ENHE:
 		case CAPABILITY_CODE_EXT_MESSAGE:
 			break;
+		case CAPABILITY_CODE_ENHE:
+			bgp_dynamic_capability_enhe(pnt, action, hdr, peer);
+			break;
 		case CAPABILITY_CODE_ROLE:
 			bgp_dynamic_capability_role(pnt, action, peer);
 			break;

bgpd/bgp_packet.h

@@ -8,6 +8,12 @@
 
 #include "hook.h"
 
+struct bgp_enhe_capability {
+	uint16_t afi;
+	uint16_t safi;
+	uint16_t nh_afi;
+};
+
 DECLARE_HOOK(bgp_packet_dump,
 		(struct peer *peer, uint8_t type, bgp_size_t size,
 			struct stream *s),

bgpd/bgp_vty.c

@@ -5977,13 +5977,17 @@ DEFUN (neighbor_capability_enhe,
 {
 	int idx_peer = 1;
 	struct peer *peer;
+	int ret;
 
 	peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg);
 	if (peer && peer->conf_if)
 		return CMD_SUCCESS;
 
-	return peer_flag_set_vty(vty, argv[idx_peer]->arg,
+	ret = peer_flag_set_vty(vty, argv[idx_peer]->arg, PEER_FLAG_CAPABILITY_ENHE);
-				 PEER_FLAG_CAPABILITY_ENHE);
+
+	bgp_capability_send(peer, AFI_IP, SAFI_UNICAST, CAPABILITY_CODE_ENHE, CAPABILITY_ACTION_SET);
+
+	return ret;
 }
 
 DEFUN (no_neighbor_capability_enhe,
@@ -5997,6 +6001,7 @@ DEFUN (no_neighbor_capability_enhe,
 {
 	int idx_peer = 2;
 	struct peer *peer;
+	int ret;
 
 	peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg);
 	if (peer && peer->conf_if) {
@@ -6006,8 +6011,12 @@ DEFUN (no_neighbor_capability_enhe,
 		return CMD_WARNING_CONFIG_FAILED;
 	}
 
-	return peer_flag_unset_vty(vty, argv[idx_peer]->arg,
+	ret = peer_flag_unset_vty(vty, argv[idx_peer]->arg, PEER_FLAG_CAPABILITY_ENHE);
-				   PEER_FLAG_CAPABILITY_ENHE);
+
+	bgp_capability_send(peer, AFI_IP, SAFI_UNICAST, CAPABILITY_CODE_ENHE,
+			    CAPABILITY_ACTION_UNSET);
+
+	return ret;
 }
 
 /* neighbor capability software-version */

bgpd/bgpd.c

@@ -4953,6 +4953,10 @@ static void peer_flag_modify_action(struct peer *peer, uint64_t flag)
 			peer->v_start = BGP_INIT_START_TIMER;
 			BGP_EVENT_ADD(peer->connection, BGP_Stop);
 		}
+	} else if (CHECK_FLAG(peer->cap, PEER_CAP_DYNAMIC_RCV) &&
+		   CHECK_FLAG(peer->cap, PEER_CAP_DYNAMIC_ADV) &&
+		   flag == PEER_FLAG_CAPABILITY_ENHE) {
+		peer->last_reset = PEER_DOWN_CAPABILITY_CHANGE;
 	} else if (!peer_notify_config_change(peer->connection))
 		bgp_session_reset(peer);
 }

tests/topotests/bgp_dynamic_capability/r1/frr.conf

@@ -3,6 +3,7 @@
 !
 int r1-eth0
  ip address 192.168.1.1/24
+ ipv6 address 2001:db8::1/64
 !
 router bgp 65001
  no bgp ebgp-requires-policy
@@ -12,11 +13,19 @@ router bgp 65001
  neighbor 192.168.1.2 timers 1 3
  neighbor 192.168.1.2 timers connect 1
  neighbor 192.168.1.2 capability dynamic
+ neighbor 2001:db8::2 remote-as external
+ neighbor 2001:db8::2 timers 1 3
+ neighbor 2001:db8::2 timers connect 1
+ neighbor 2001:db8::2 capability dynamic
  !
  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
  !
+ address-family ipv6 unicast
+  neighbor 2001:db8::2 activate
+ exit-address-family
+!
 ip prefix-list r2 seq 5 permit 10.10.10.10/32
 !

tests/topotests/bgp_dynamic_capability/r2/frr.conf

@@ -7,6 +7,7 @@ int lo
 !
 int r2-eth0
  ip address 192.168.1.2/24
+ ipv6 address 2001:db8::2/64
 !
 router bgp 65002
  bgp graceful-restart
@@ -16,9 +17,20 @@ 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 capability extended-nexthop
  neighbor 192.168.1.1 addpath-rx-paths-limit 20
+ neighbor 2001:db8::1 remote-as external
+ neighbor 2001:db8::1 timers 1 3
+ neighbor 2001:db8::1 timers connect 1
+ neighbor 2001:db8::1 capability dynamic
+ neighbor 2001:db8::1 capability extended-nexthop
  !
  address-family ipv4 unicast
   redistribute connected
  exit-address-family
  !
+ address-family ipv6 unicast
+  redistribute connected
+  neighbor 2001:db8::1 activate
+ exit-address-family
+!

tests/topotests/bgp_dynamic_capability/test_bgp_dynamic_capability_enhe.py

@@ -0,0 +1,215 @@
+#!/usr/bin/env python
+# SPDX-License-Identifier: ISC
+
+# Copyright (c) 2024 by
+# Donatas Abraitis <donatas@opensourcerouting.org>
+#
+
+"""
+Test if extended nexthop capability is exchanged dynamically.
+"""
+
+import os
+import sys
+import json
+import pytest
+import functools
+import time
+
+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, get_topogen
+from lib.common_config import step
+
+
+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_enhe():
+    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 2001:db8::2 json"))
+        expected = {
+            "2001:db8::2": {
+                "bgpState": "Established",
+                "localRole": "undefined",
+                "remoteRole": "undefined",
+                "neighborCapabilities": {
+                    "dynamic": "advertisedAndReceived",
+                    "extendedNexthop": "received",
+                },
+            }
+        }
+        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"
+
+    def _bgp_check_nexthop():
+        output = json.loads(r1.vtysh_cmd("show ip route 10.10.10.10/32 json"))
+        expected = {
+            "10.10.10.10/32": [
+                {
+                    "protocol": "bgp",
+                    "selected": True,
+                    "installed": True,
+                    "nexthops": [
+                        {
+                            "fib": True,
+                            "ip": "192.168.1.2",
+                            "afi": "ipv4",
+                            "interfaceName": "r1-eth0",
+                            "active": True,
+                        },
+                        {
+                            "duplicate": True,
+                            "ip": "192.168.1.2",
+                            "afi": "ipv4",
+                            "interfaceName": "r1-eth0",
+                            "active": True,
+                        },
+                    ],
+                }
+            ]
+        }
+        return topotest.json_cmp(output, expected)
+
+    test_func = functools.partial(
+        _bgp_check_nexthop,
+    )
+    _, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
+    assert result is None, "Can't see 10.10.10.10/32 with IPv4 only nexthops"
+
+    step("Enable ENHE capability")
+
+    # Clear message stats to check if we receive a notification or not after we
+    # change the role.
+    r1.vtysh_cmd("clear bgp 2001:db8::2 message-stats")
+    r1.vtysh_cmd(
+        """
+    configure terminal
+    router bgp
+      neighbor 2001:db8::2 capability extended-nexthop
+    """
+    )
+
+    def _bgp_check_if_session_not_reset():
+        output = json.loads(r2.vtysh_cmd("show bgp neighbor 2001:db8::1 json"))
+        expected = {
+            "2001:db8::1": {
+                "bgpState": "Established",
+                "neighborCapabilities": {
+                    "dynamic": "advertisedAndReceived",
+                    "extendedNexthop": "advertisedAndReceived",
+                    "extendedNexthopFamililesByPeer": {
+                        "ipv4Unicast": "recieved",
+                    },
+                },
+                "messageStats": {
+                    "notificationsRecv": 0,
+                    "capabilityRecv": 1,
+                },
+            }
+        }
+        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 setting ENHE capability"
+
+    def _bgp_check_if_session_not_reset():
+        output = json.loads(r2.vtysh_cmd("show bgp neighbor 2001:db8::1 json"))
+        expected = {
+            "2001:db8::1": {
+                "bgpState": "Established",
+                "neighborCapabilities": {
+                    "dynamic": "advertisedAndReceived",
+                    "extendedNexthop": "advertisedAndReceived",
+                    "extendedNexthopFamililesByPeer": {
+                        "ipv4Unicast": "recieved",
+                    },
+                },
+                "messageStats": {
+                    "notificationsRecv": 0,
+                    "capabilityRecv": 1,
+                },
+            }
+        }
+        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 setting ENHE capability"
+
+    def _bgp_check_nexthop_enhe():
+        output = json.loads(r1.vtysh_cmd("show ip route 10.10.10.10/32 json"))
+        expected = {
+            "10.10.10.10/32": [
+                {
+                    "protocol": "bgp",
+                    "selected": True,
+                    "installed": True,
+                    "nexthops": [
+                        {
+                            "fib": True,
+                            "ip": "192.168.1.2",
+                            "afi": "ipv4",
+                            "interfaceName": "r1-eth0",
+                            "active": True,
+                        },
+                        {
+                            "fib": True,
+                            "afi": "ipv6",
+                            "interfaceName": "r1-eth0",
+                            "active": True,
+                        },
+                    ],
+                }
+            ]
+        }
+        return topotest.json_cmp(output, expected)
+
+    test_func = functools.partial(
+        _bgp_check_nexthop_enhe,
+    )
+    _, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
+    assert result is None, "Can't see 10.10.10.10/32 with IPv4 only nexthops"
+
+
+if __name__ == "__main__":
+    args = ["-s"] + sys.argv[1:]
+    sys.exit(pytest.main(args))