bgpd: Add patches for RFC9234 implementation

This commit fixes some issues that were noted by the reviewer

Signed-off-by: Eugene Bogomazov <eb@qrator.net>

bgpd/bgp_fsm.c

@@ -243,7 +243,7 @@ static struct peer *peer_xfer_conn(struct peer *from_peer)
 	peer->v_delayopen = from_peer->v_delayopen;
 	peer->v_gr_restart = from_peer->v_gr_restart;
 	peer->cap = from_peer->cap;
-	peer->neighbor_role = from_peer->neighbor_role;
+	peer->remote_role = from_peer->remote_role;
 	status = peer->status;
 	pstatus = peer->ostatus;
 	last_evt = peer->last_event;
@@ -1528,7 +1528,7 @@ int bgp_stop(struct peer *peer)
 	peer->cap = 0;
 
 	/* Resetting neighbor role to the default value */
-	peer->neighbor_role = ROLE_UNDEFINE;
+	peer->remote_role = ROLE_UNDEFINED;
 
 	FOREACH_AFI_SAFI (afi, safi) {
 		/* Reset all negotiated variables */

bgpd/bgp_open.c

@@ -900,7 +900,7 @@ static int bgp_capability_role(struct peer *peer, struct capability_header *hdr)
 	}
 	uint8_t role = stream_getc(BGP_INPUT(peer));
 
-	peer->neighbor_role = role;
+	peer->remote_role = role;
 	return 0;
 }
 
@@ -1138,19 +1138,20 @@ static bool strict_capability_same(struct peer *peer)
 static bool bgp_role_violation(struct peer *peer)
 {
 	uint8_t local_role = peer->local_role;
-	uint8_t neigh_role = peer->neighbor_role;
+	uint8_t remote_role = peer->remote_role;
 
-	if (local_role != ROLE_UNDEFINE && neigh_role != ROLE_UNDEFINE &&
+	if (local_role != ROLE_UNDEFINED && remote_role != ROLE_UNDEFINED &&
-	    !((local_role == ROLE_PEER && neigh_role == ROLE_PEER) ||
+	    !((local_role == ROLE_PEER && remote_role == ROLE_PEER) ||
-	      (local_role == ROLE_PROVIDER && neigh_role == ROLE_CUSTOMER) ||
+	      (local_role == ROLE_PROVIDER && remote_role == ROLE_CUSTOMER) ||
-	      (local_role == ROLE_CUSTOMER && neigh_role == ROLE_PROVIDER) ||
+	      (local_role == ROLE_CUSTOMER && remote_role == ROLE_PROVIDER) ||
-	      (local_role == ROLE_RS_SERVER && neigh_role == ROLE_RS_CLIENT) ||
+	      (local_role == ROLE_RS_SERVER && remote_role == ROLE_RS_CLIENT) ||
-	      (local_role == ROLE_RS_CLIENT && neigh_role == ROLE_RS_SERVER))) {
+	      (local_role == ROLE_RS_CLIENT &&
+	       remote_role == ROLE_RS_SERVER))) {
 		bgp_notify_send(peer, BGP_NOTIFY_OPEN_ERR,
 				BGP_NOTIFY_OPEN_ROLE_MISMATCH);
 		return true;
 	}
-	if (neigh_role == ROLE_UNDEFINE &&
+	if (remote_role == ROLE_UNDEFINED &&
 	    CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE)) {
 		const char *err_msg =
 			"Strict mode. Please set the role on your side.";
@@ -1729,7 +1730,7 @@ uint16_t bgp_open_capability(struct stream *s, struct peer *peer,
 	stream_putc(s, CAPABILITY_CODE_EXT_MESSAGE_LEN);
 
 	/* Role*/
-	if (peer->local_role != ROLE_UNDEFINE) {
+	if (peer->local_role != ROLE_UNDEFINED) {
 		SET_FLAG(peer->cap, PEER_CAP_ROLE_ADV);
 		stream_putc(s, BGP_OPEN_OPT_CAP);
 		stream_putc(s, CAPABILITY_CODE_ROLE_LEN + 2);

bgpd/bgp_vty.c

@@ -927,7 +927,7 @@ int bgp_vty_return(struct vty *vty, enum bgp_create_error_code ret)
 		str = "Invalid role name";
 		break;
 	case BGP_ERR_INVALID_INTERNAL_ROLE:
-		str = "Extrenal roles can be set only on eBGP session";
+		str = "External roles can be set only on eBGP session";
 		break;
 	}
 	if (str) {
@@ -6423,11 +6423,11 @@ static uint8_t get_role_by_name(const char *role_str)
 		return ROLE_RS_SERVER;
 	if (strncmp(role_str, "rs-client", 4) == 0)
 		return ROLE_RS_CLIENT;
-	return ROLE_UNDEFINE;
+	return ROLE_UNDEFINED;
 }
 
 static int peer_role_set_vty(struct vty *vty, const char *ip_str,
-			     const char *role_str, int strict_mode)
+			     const char *role_str, bool strict_mode)
 {
 	struct peer *peer;
 
@@ -6436,7 +6436,7 @@ static int peer_role_set_vty(struct vty *vty, const char *ip_str,
 		return CMD_WARNING_CONFIG_FAILED;
 	uint8_t role = get_role_by_name(role_str);
 
-	if (role == ROLE_UNDEFINE)
+	if (role == ROLE_UNDEFINED)
 		return bgp_vty_return(vty, BGP_ERR_INVALID_ROLE_NAME);
 	return bgp_vty_return(vty, peer_role_set(peer, role, strict_mode));
 }
@@ -6451,7 +6451,7 @@ static int peer_role_unset_vty(struct vty *vty, const char *ip_str)
 	return bgp_vty_return(vty, peer_role_unset(peer));
 }
 
-DEFUN(neighbor_role,
+DEFPY(neighbor_role,
       neighbor_role_cmd,
       "neighbor <A.B.C.D|X:X::X:X|WORD> local-role <provider|rs-server|rs-client|customer|peer>",
       NEIGHBOR_STR
@@ -6463,10 +6463,10 @@ DEFUN(neighbor_role,
 	int idx_role = 3;
 
 	return peer_role_set_vty(vty, argv[idx_peer]->arg, argv[idx_role]->arg,
-				 0);
+				 false);
 }
 
-DEFUN(neighbor_role_strict,
+DEFPY(neighbor_role_strict,
       neighbor_role_strict_cmd,
       "neighbor <A.B.C.D|X:X::X:X|WORD> local-role <provider|rs-server|rs-client|customer|peer> strict-mode",
       NEIGHBOR_STR
@@ -6479,18 +6479,18 @@ DEFUN(neighbor_role_strict,
 	int idx_role = 3;
 
 	return peer_role_set_vty(vty, argv[idx_peer]->arg, argv[idx_role]->arg,
-				 1);
+				 true);
 }
 
-DEFUN(no_neighbor_role,
+DEFPY(no_neighbor_role,
       no_neighbor_role_cmd,
       "no neighbor <A.B.C.D|X:X::X:X|WORD> local-role <provider|rs-server|rs-client|customer|peer> [strict-mode]",
       NO_STR
       NEIGHBOR_STR
       NEIGHBOR_ADDR_STR2
-      "Unset session role\n"
+      "Set session role\n"
       ROLE_STR
-      "Was used additional restriction on peer\n")
+      "Use additional restriction on peer\n")
 {
 	int idx_peer = 2;
 
@@ -12571,14 +12571,14 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, bool use_json,
 	/* Roles */
 	if (use_json) {
 		json_object_string_add(json_neigh, "localRole",
-				       get_name_by_role(p->local_role));
+				       bgp_get_name_by_role(p->local_role));
-		json_object_string_add(json_neigh, "neighRole",
+		json_object_string_add(json_neigh, "remoteRole",
-				       get_name_by_role(p->neighbor_role));
+				       bgp_get_name_by_role(p->remote_role));
 	} else {
 		vty_out(vty, "  Local Role: %s\n",
-			get_name_by_role(p->local_role));
+			bgp_get_name_by_role(p->local_role));
-		vty_out(vty, "  Neighbor Role: %s\n",
+		vty_out(vty, "  Remote Role: %s\n",
-			get_name_by_role(p->neighbor_role));
+			bgp_get_name_by_role(p->remote_role));
 	}
 
 
@@ -16794,9 +16794,9 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp,
 	}
 
 	/* role */
-	if (peer->local_role != ROLE_UNDEFINE) {
+	if (peer->local_role != ROLE_UNDEFINED) {
 		vty_out(vty, " neighbor %s local-role %s%s\n", addr,
-			get_name_by_role(peer->local_role),
+			bgp_get_name_by_role(peer->local_role),
 			CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE)
 				? " strict-mode"
 				: "");

bgpd/bgpd.c

@@ -1374,8 +1374,8 @@ struct peer *peer_new(struct bgp *bgp)
 	peer->cur_event = peer->last_event = peer->last_major_event = 0;
 	peer->bgp = bgp_lock(bgp);
 	peer = peer_lock(peer); /* initial reference */
-	peer->local_role = ROLE_UNDEFINE;
+	peer->local_role = ROLE_UNDEFINED;
-	peer->neighbor_role = ROLE_UNDEFINE;
+	peer->remote_role = ROLE_UNDEFINED;
 	peer->password = NULL;
 	peer->max_packet_size = BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE;
 
@@ -1999,12 +1999,12 @@ int peer_remote_as(struct bgp *bgp, union sockunion *su, const char *conf_if,
 	return 0;
 }
 
-const char *get_name_by_role(uint8_t role)
+const char *bgp_get_name_by_role(uint8_t role)
 {
 	static const char *const bgp_role_names[] = {
 		"provider", "rs-server", "rs-client", "customer", "peer"};
-	if (role == ROLE_UNDEFINE)
+	if (role == ROLE_UNDEFINED)
-		return "undefine";
+		return "undefined";
 	if (role <= 5)
 		return bgp_role_names[role];
 	return "unknown";
@@ -4247,6 +4247,7 @@ 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},
 	{0, 0, 0}};
 
 static const struct peer_flag_action peer_af_flag_action_list[] = {
@@ -4917,7 +4918,7 @@ 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, int strict_mode)
+int peer_role_set(struct peer *peer, uint8_t role, bool strict_mode)
 {
 	if (peer->local_role == role) {
 		if (CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE) &&
@@ -4929,7 +4930,7 @@ int peer_role_set(struct peer *peer, uint8_t role, int strict_mode)
 			SET_FLAG(peer->flags, PEER_FLAG_STRICT_MODE);
 			/* Restart session to throw Role Mismatch Notification
 			 */
-			if (peer->neighbor_role == ROLE_UNDEFINE)
+			if (peer->remote_role == ROLE_UNDEFINED)
 				bgp_session_reset(peer);
 		}
 	} else {
@@ -4950,7 +4951,7 @@ int peer_role_set(struct peer *peer, uint8_t role, int strict_mode)
 
 int peer_role_unset(struct peer *peer)
 {
-	return peer_role_set(peer, ROLE_UNDEFINE, 0);
+	return peer_role_set(peer, ROLE_UNDEFINED, 0);
 }
 
 /* Neighbor description. */

bgpd/bgpd.h

@@ -1169,13 +1169,13 @@ struct peer {
 
 	/* Roles in bgp session */
 	uint8_t local_role;
-	uint8_t neighbor_role;
+	uint8_t remote_role;
 #define ROLE_PROVIDER                       0
 #define ROLE_RS_SERVER                      1
 #define ROLE_RS_CLIENT                      2
 #define ROLE_CUSTOMER                       3
 #define ROLE_PEER                           4
-#define ROLE_UNDEFINE                     255
+#define ROLE_UNDEFINED                    255
 
 #define ROLE_NAME_MAX_LEN                  20
 
@@ -2179,7 +2179,7 @@ extern int peer_ebgp_multihop_set(struct peer *, int);
 extern int peer_ebgp_multihop_unset(struct peer *);
 extern int is_ebgp_multihop_configured(struct peer *peer);
 
-extern int peer_role_set(struct peer *peer, uint8_t role, int strict_mode);
+extern int peer_role_set(struct peer *peer, uint8_t role, bool strict_mode);
 extern int peer_role_unset(struct peer *peer);
 
 extern void peer_description_set(struct peer *, const char *);
@@ -2281,7 +2281,7 @@ extern void peer_tx_shutdown_message_set(struct peer *, const char *msg);
 extern void peer_tx_shutdown_message_unset(struct peer *);
 
 extern void bgp_route_map_update_timer(struct thread *thread);
-extern const char *get_name_by_role(uint8_t role);
+extern const char *bgp_get_name_by_role(uint8_t role);
 
 extern void bgp_route_map_terminate(void);
 

doc/user/bgp.rst

@@ -2653,7 +2653,12 @@ prevention, detection and mitigation.
 
 To enable its mechanics, you must set your local role to reflect your type of
 peering relationship with your neighbor. Possible values of ``LOCAL-ROLE`` are:
-<provider|rs-server|rs-client|customer|peer>.
+
+- provider
+- rs-server
+- rs-client
+- customer
+- peer
 
 The local Role value is negotiated with the new BGP Role capability with a
 built-in check of the corresponding value. In case of mismatch the new OPEN
@@ -2673,9 +2678,9 @@ The correct Role pairs are:
        Role: advertised and received
 
 If strict-mode is set BGP session won't become established until BGP neighbor
-set local Role on its side. This configuratoin parameter is defined in
+set local Role on its side. This configuration parameter is defined in
 :rfc:`9234` and used to enforce corresponding configuration at your
-conter-part side. Default value - disabled.
+counter-part side. Default value - disabled.
 
 Routes that sent from provider, rs-server, or peer local-role (or if received
 by customer, rs-clinet, or peer local-role) will be marked with a new
@@ -2685,7 +2690,7 @@ Routes with this attribute can only be sent to your neighbor if your
 local-role is provider or rs-server. Routes with this attribute can be
 received only if your local-role is customer or rs-client.
 
-In case of peer-peer relaitonship routes can be received only if
+In case of peer-peer relationship routes can be received only if
 OTC value is equal to your neighbor AS number.
 
 All these rules with OTC help to detect and mitigate route leaks and
@@ -2696,7 +2701,7 @@ happened automatically if local-role is set.
    This command set your local-role to ``LOCAL-ROLE``:
    <provider|rs-server|rs-client|customer|peer>.
 
-   This role help to detect and prevent route leaks.
+   This role helps to detect and prevent route leaks.
 
    If ``strict-mode`` is set, your neighbor must send you Capability with the
    value of his role (by setting local-role on his side). Otherwise, a Role

tests/topotests/bgp_roles_capability/r1/bgpd.conf

@@ -4,12 +4,16 @@ router bgp 64501
 ! Correct role pair
   neighbor 192.168.2.2 remote-as 64502
   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 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 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 local-role provider strict-mode
+  neighbor 192.168.5.2 timers 3 10

tests/topotests/bgp_roles_capability/r2/bgpd.conf

@@ -2,3 +2,4 @@ router bgp 64502
   bgp router-id 192.168.2.2
   neighbor 192.168.2.1 remote-as 64501
   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,3 +1,4 @@
 router bgp 64503
   neighbor 192.168.3.1 remote-as 64501
   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,2 +1,3 @@
 router bgp 64504
   neighbor 192.168.4.1 remote-as 64501
+  neighbor 192.168.4.1 timers 3 10

tests/topotests/bgp_roles_capability/r5/bgpd.conf

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

tests/topotests/bgp_roles_capability/test_bgp_roles_capability.py

@@ -82,7 +82,7 @@ def test_correct_pair(tgen):
         tgen.gears["r1"].vtysh_cmd(f"show bgp neighbors {neighbor_ip} json")
     )[neighbor_ip]
     assert neighbor_status["localRole"] == "provider"
-    assert neighbor_status["neighRole"] == "customer"
+    assert neighbor_status["remoteRole"] == "customer"
     assert neighbor_status["bgpState"] == "Established"
     assert (
         neighbor_status["neighborCapabilities"].get("role") == "advertisedAndReceived"
@@ -99,31 +99,31 @@ def test_role_pair_mismatch(tgen):
 
 
 def test_single_role_advertising(tgen):
-    # provider-undefine pair; we set role
+    # provider-undefined pair; we set role
     neighbor_ip = "192.168.4.2"
     neighbor_status = json.loads(
         tgen.gears["r1"].vtysh_cmd(f"show bgp neighbors {neighbor_ip} json")
     )[neighbor_ip]
     assert neighbor_status["localRole"] == "provider"
-    assert neighbor_status["neighRole"] == "undefine"
+    assert neighbor_status["remoteRole"] == "undefined"
     assert neighbor_status["bgpState"] == "Established"
     assert neighbor_status["neighborCapabilities"].get("role") == "advertised"
 
 
 def test_single_role_receiving(tgen):
-    # provider-undefine pair; we receive role
+    # provider-undefined pair; we receive role
     neighbor_ip = "192.168.4.1"
     neighbor_status = json.loads(
         tgen.gears["r4"].vtysh_cmd(f"show bgp neighbors {neighbor_ip} json")
     )[neighbor_ip]
-    assert neighbor_status["localRole"] == "undefine"
+    assert neighbor_status["localRole"] == "undefined"
-    assert neighbor_status["neighRole"] == "provider"
+    assert neighbor_status["remoteRole"] == "provider"
     assert neighbor_status["bgpState"] == "Established"
     assert neighbor_status["neighborCapabilities"].get("role") == "received"
 
 
 def test_role_strict_mode(tgen):
-    # provider-undefine pair bur strict-mode was set
+    # provider-undefined pair with strict-mode
     neighbor_ip = "192.168.5.2"
     neighbor_status = json.loads(
         tgen.gears["r1"].vtysh_cmd(f"show bgp neighbors {neighbor_ip} json")

tests/topotests/bgp_roles_filtering/test_bgp_roles_filtering.py

@@ -60,7 +60,7 @@ def tgen(request):
     BGP_CONVERGENCE = verify_bgp_convergence_from_running_config(tgen)
     assert BGP_CONVERGENCE, f"setup_module :Failed \n Error: {BGP_CONVERGENCE}"
     # Todo: What is the indented way to wait for convergence without json?!
-    time.sleep(3)
+    time.sleep(5)
     yield tgen
     tgen.stop_topology()