Merge pull request #11484 from opensourcerouting/fix/allow_using_bgp_roles_for_peer_groups

bgpd: Make sure peer-groups/unnumbered work too with BGP role

bgpd/bgp_open.c

@@ -1152,7 +1152,7 @@ static bool bgp_role_violation(struct peer *peer)
 		return true;
 	}
 	if (remote_role == ROLE_UNDEFINED &&
-	    CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE)) {
+	    CHECK_FLAG(peer->flags, PEER_FLAG_ROLE_STRICT_MODE)) {
 		const char *err_msg =
 			"Strict mode. Please set the role on your side.";
 		bgp_notify_send_with_data(peer, BGP_NOTIFY_OPEN_ERR,

bgpd/bgp_vty.c

@@ -6449,7 +6449,7 @@ static int peer_role_set_vty(struct vty *vty, const char *ip_str,
 {
 	struct peer *peer;
 
-	peer = peer_lookup_vty(vty, ip_str);
+	peer = peer_and_group_lookup_vty(vty, ip_str);
 	if (!peer)
 		return CMD_WARNING_CONFIG_FAILED;
 	uint8_t role = get_role_by_name(role_str);
@@ -6463,7 +6463,7 @@ static int peer_role_unset_vty(struct vty *vty, const char *ip_str)
 {
 	struct peer *peer;
 
-	peer = peer_lookup_vty(vty, ip_str);
+	peer = peer_and_group_lookup_vty(vty, ip_str);
 	if (!peer)
 		return CMD_WARNING_CONFIG_FAILED;
 	return bgp_vty_return(vty, peer_role_unset(peer));
@@ -16785,13 +16785,13 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp,
 	}
 
 	/* role */
-	if (peer->local_role != ROLE_UNDEFINED) {
+	if (peergroup_flag_check(peer, PEER_FLAG_ROLE) &&
+	    peer->local_role != ROLE_UNDEFINED)
 		vty_out(vty, " neighbor %s local-role %s%s\n", addr,
 			bgp_get_name_by_role(peer->local_role),
-			CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE)
+			CHECK_FLAG(peer->flags, PEER_FLAG_ROLE_STRICT_MODE)
 				? " strict-mode"
 				: "");
-	}
 
 	/* ttl-security hops */
 	if (peer->gtsm_hops != BGP_GTSM_HOPS_DISABLED) {

bgpd/bgpd.c

@@ -2731,6 +2731,9 @@ static void peer_group2peer_config_copy(struct peer_group *group,
 		}
 	}
 
+	/* role */
+	PEER_ATTR_INHERIT(peer, group, local_role);
+
 	/* Update GR flags for the peer. */
 	bgp_peer_gr_flags_update(peer);
 
@@ -4256,7 +4259,8 @@ static const struct peer_flag_action peer_flag_action_list[] = {
 	{PEER_FLAG_UPDATE_SOURCE, 0, peer_change_none},
 	{PEER_FLAG_DISABLE_LINK_BW_ENCODING_IEEE, 0, peer_change_none},
 	{PEER_FLAG_EXTENDED_OPT_PARAMS, 0, peer_change_reset},
-	{PEER_FLAG_STRICT_MODE, 0, peer_change_reset},
+	{PEER_FLAG_ROLE_STRICT_MODE, 0, peer_change_reset},
+	{PEER_FLAG_ROLE, 0, peer_change_reset},
 	{0, 0, 0}};
 
 static const struct peer_flag_action peer_af_flag_action_list[] = {
@@ -4929,36 +4933,109 @@ int peer_ebgp_multihop_unset(struct peer *peer)
 /* Set Open Policy Role and check its correctness */
 int peer_role_set(struct peer *peer, uint8_t role, bool strict_mode)
 {
-	if (peer->sort != BGP_PEER_EBGP)
+	struct peer *member;
-		return BGP_ERR_INVALID_INTERNAL_ROLE;
+	struct listnode *node, *nnode;
 
-	if (peer->local_role == role) {
+	peer_flag_set(peer, PEER_FLAG_ROLE);
-		if (CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE) &&
+
-		    !strict_mode)
+	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-			/* TODO: Is session restart needed if it was down? */
+		if (peer->sort != BGP_PEER_EBGP)
-			UNSET_FLAG(peer->flags, PEER_FLAG_STRICT_MODE);
+			return BGP_ERR_INVALID_INTERNAL_ROLE;
-		if (!CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE) &&
+
-		    strict_mode) {
+		if (peer->local_role == role) {
-			SET_FLAG(peer->flags, PEER_FLAG_STRICT_MODE);
+			if (CHECK_FLAG(peer->flags,
-			/* Restart session to throw Role Mismatch Notification
+				       PEER_FLAG_ROLE_STRICT_MODE) &&
-			 */
+			    !strict_mode)
-			if (peer->remote_role == ROLE_UNDEFINED)
+				/* TODO: Is session restart needed if it was
-				bgp_session_reset(peer);
+				 * down?
+				 */
+				UNSET_FLAG(peer->flags,
+					   PEER_FLAG_ROLE_STRICT_MODE);
+			if (!CHECK_FLAG(peer->flags,
+					PEER_FLAG_ROLE_STRICT_MODE) &&
+			    strict_mode) {
+				SET_FLAG(peer->flags,
+					 PEER_FLAG_ROLE_STRICT_MODE);
+				/* Restart session to throw Role Mismatch
+				 * Notification
+				 */
+				if (peer->remote_role == ROLE_UNDEFINED)
+					bgp_session_reset(peer);
+			}
+		} else {
+			peer->local_role = role;
+			if (strict_mode)
+				SET_FLAG(peer->flags,
+					 PEER_FLAG_ROLE_STRICT_MODE);
+			else
+				UNSET_FLAG(peer->flags,
+					   PEER_FLAG_ROLE_STRICT_MODE);
+			bgp_session_reset(peer);
 		}
-	} else {
+
-		peer->local_role = role;
+		return CMD_SUCCESS;
-		if (strict_mode)
-			SET_FLAG(peer->flags, PEER_FLAG_STRICT_MODE);
-		else
-			UNSET_FLAG(peer->flags, PEER_FLAG_STRICT_MODE);
-		bgp_session_reset(peer);
 	}
-	return 0;
+
+	peer->local_role = role;
+	for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+		if (member->sort != BGP_PEER_EBGP)
+			return BGP_ERR_INVALID_INTERNAL_ROLE;
+
+		if (member->local_role == role) {
+			if (CHECK_FLAG(member->flags,
+				       PEER_FLAG_ROLE_STRICT_MODE) &&
+			    !strict_mode)
+				/* TODO: Is session restart needed if it was
+				 * down?
+				 */
+				UNSET_FLAG(member->flags,
+					   PEER_FLAG_ROLE_STRICT_MODE);
+			if (!CHECK_FLAG(member->flags,
+					PEER_FLAG_ROLE_STRICT_MODE) &&
+			    strict_mode) {
+				SET_FLAG(peer->flags,
+					 PEER_FLAG_ROLE_STRICT_MODE);
+				SET_FLAG(member->flags,
+					 PEER_FLAG_ROLE_STRICT_MODE);
+				/* Restart session to throw Role Mismatch
+				 * Notification
+				 */
+				if (member->remote_role == ROLE_UNDEFINED)
+					bgp_session_reset(member);
+			}
+		} else {
+			member->local_role = role;
+
+			if (strict_mode) {
+				SET_FLAG(peer->flags,
+					 PEER_FLAG_ROLE_STRICT_MODE);
+				SET_FLAG(member->flags,
+					 PEER_FLAG_ROLE_STRICT_MODE);
+			} else {
+				UNSET_FLAG(member->flags,
+					   PEER_FLAG_ROLE_STRICT_MODE);
+			}
+			bgp_session_reset(member);
+		}
+	}
+
+	return CMD_SUCCESS;
 }
 
 int peer_role_unset(struct peer *peer)
 {
-	return peer_role_set(peer, ROLE_UNDEFINED, 0);
+	struct peer *member;
+	struct listnode *node, *nnode;
+
+	peer_flag_unset(peer, PEER_FLAG_ROLE);
+
+	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP))
+		return peer_role_set(peer, ROLE_UNDEFINED, 0);
+
+	for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member))
+		peer_role_set(member, ROLE_UNDEFINED, 0);
+
+	return CMD_SUCCESS;
 }
 
 /* Neighbor description. */

bgpd/bgpd.h

@@ -1337,9 +1337,12 @@ struct peer {
 #define PEER_FLAG_EXTENDED_OPT_PARAMS (1ULL << 30)
 
 	/* BGP Open Policy flags.
-	 * Enforce using roles on both sides
+	 * Enforce using roles on both sides:
+	 * `local-role ROLE strict-mode` configured.
 	 */
-#define PEER_FLAG_STRICT_MODE               (1ULL << 31)
+#define PEER_FLAG_ROLE_STRICT_MODE (1ULL << 31)
+	/* `local-role` configured */
+#define PEER_FLAG_ROLE (1ULL << 32)
 
 	/*
 	 *GR-Disabled mode means unset PEER_FLAG_GRACEFUL_RESTART

tests/topotests/bgp_roles_capability/r1/bgpd.conf

@@ -2,18 +2,24 @@ router bgp 64501
   bgp router-id 192.168.2.1
   network 192.0.2.0/24
 ! Correct role pair
-  neighbor 192.168.2.2 remote-as 64502
+  neighbor 192.168.2.2 remote-as external
   neighbor 192.168.2.2 local-role provider
   neighbor 192.168.2.2 timers 3 10
 ! Incorrect role pair
-  neighbor 192.168.3.2 remote-as 64503
+  neighbor 192.168.3.2 remote-as external
   neighbor 192.168.3.2 local-role provider
   neighbor 192.168.3.2 timers 3 10
 ! Missed neighbor role
-  neighbor 192.168.4.2 remote-as 64504
+  neighbor 192.168.4.2 remote-as external
   neighbor 192.168.4.2 local-role provider
   neighbor 192.168.4.2 timers 3 10
 ! Missed neighbor role with strict-mode
-  neighbor 192.168.5.2 remote-as 64505
+  neighbor 192.168.5.2 remote-as external
   neighbor 192.168.5.2 local-role provider strict-mode
   neighbor 192.168.5.2 timers 3 10
+! Testing peer-groups
+  neighbor PG peer-group
+  neighbor PG remote-as external
+  neighbor PG local-role provider
+  neighbor PG timers 3 10
+  neighbor 192.168.6.2 peer-group PG

tests/topotests/bgp_roles_capability/r1/zebra.conf

@@ -11,5 +11,8 @@ interface r1-eth2
 interface r1-eth3
  ip address 192.168.5.1/24
 !
+interface r1-eth4
+ ip address 192.168.6.1/24
+!
 ip forwarding
 !

tests/topotests/bgp_roles_capability/r2/bgpd.conf

@@ -1,5 +1,5 @@
 router bgp 64502
   bgp router-id 192.168.2.2
-  neighbor 192.168.2.1 remote-as 64501
+  neighbor 192.168.2.1 remote-as external
   neighbor 192.168.2.1 local-role customer
   neighbor 192.168.2.1 timers 3 10

tests/topotests/bgp_roles_capability/r3/bgpd.conf

@@ -1,4 +1,4 @@
 router bgp 64503
-  neighbor 192.168.3.1 remote-as 64501
+  neighbor 192.168.3.1 remote-as external
   neighbor 192.168.3.1 local-role peer
   neighbor 192.168.3.1 timers 3 10

tests/topotests/bgp_roles_capability/r4/bgpd.conf

@@ -1,3 +1,3 @@
 router bgp 64504
-  neighbor 192.168.4.1 remote-as 64501
+  neighbor 192.168.4.1 remote-as external
   neighbor 192.168.4.1 timers 3 10

tests/topotests/bgp_roles_capability/r5/bgpd.conf

@@ -1,3 +1,3 @@
 router bgp 64505
-  neighbor 192.168.5.1 remote-as 64501
+  neighbor 192.168.5.1 remote-as external
   neighbor 192.168.5.1 timers 3 10

tests/topotests/bgp_roles_capability/r6/bgpd.conf

@@ -0,0 +1,4 @@
+router bgp 64506
+  neighbor 192.168.6.1 remote-as external
+  neighbor 192.168.6.1 local-role customer
+  neighbor 192.168.6.1 timers 3 10

tests/topotests/bgp_roles_capability/r6/zebra.conf

@@ -0,0 +1,6 @@
+!
+interface r6-eth0
+ ip address 192.168.6.2/24
+!
+ip forwarding
+!

tests/topotests/bgp_roles_capability/test_bgp_roles_capability.py

@@ -43,7 +43,7 @@ from lib.topolog import logger
 pytestmark = [pytest.mark.bgpd]
 
 
-topodef = {f"s{i}": ("r1", f"r{i}") for i in range(2, 6)}
+topodef = {f"s{i}": ("r1", f"r{i}") for i in range(2, 7)}
 
 
 @pytest.fixture(scope="module")
@@ -162,6 +162,24 @@ def test_role_strict_mode(tgen):
     assert success, "Session between r1 and r5 was not correctly closed"
 
 
+def test_correct_pair_peer_group(tgen):
+    # provider-customer pair (using peer-groups)
+    router = tgen.gears["r1"]
+    neighbor_ip = "192.168.6.2"
+    check_r6_established = functools.partial(
+        check_session_established, router, neighbor_ip
+    )
+    success, _ = topotest.run_and_expect(check_r6_established, True, count=20, wait=3)
+    assert success, "Session with r6 is not Established"
+
+    neighbor_status = find_neighbor_status(router, neighbor_ip)
+    assert neighbor_status["localRole"] == "provider"
+    assert neighbor_status["remoteRole"] == "customer"
+    assert (
+        neighbor_status["neighborCapabilities"].get("role") == "advertisedAndReceived"
+    )
+
+
 if __name__ == "__main__":
     args = ["-s"] + sys.argv[1:]
     sys.exit(pytest.main(args))