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>

diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index 044e72cc1e017be570a918e745c7957285639e03..b034437a18d84ef92d3054e57faa5c3e09c1893f 100644 (file)
--- a/bgpd/bgp_fsm.c
+++ b/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 */

diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c
index d8dd71788be4998bf6b39242765325c1f1e0439b..28cacd6086eb379567e83bfb5ccaead449636b3f 100644 (file)
--- a/bgpd/bgp_open.c
+++ b/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;
-
-       if (local_role != ROLE_UNDEFINE && neigh_role != ROLE_UNDEFINE &&
-           !((local_role == ROLE_PEER && neigh_role == ROLE_PEER) ||
-             (local_role == ROLE_PROVIDER && neigh_role == ROLE_CUSTOMER) ||
-             (local_role == ROLE_CUSTOMER && neigh_role == ROLE_PROVIDER) ||
-             (local_role == ROLE_RS_SERVER && neigh_role == ROLE_RS_CLIENT) ||
-             (local_role == ROLE_RS_CLIENT && neigh_role == ROLE_RS_SERVER))) {
+       uint8_t remote_role = peer->remote_role;
+
+       if (local_role != ROLE_UNDEFINED && remote_role != ROLE_UNDEFINED &&
+           !((local_role == ROLE_PEER && remote_role == ROLE_PEER) ||
+             (local_role == ROLE_PROVIDER && remote_role == ROLE_CUSTOMER) ||
+             (local_role == ROLE_CUSTOMER && remote_role == ROLE_PROVIDER) ||
+             (local_role == ROLE_RS_SERVER && remote_role == ROLE_RS_CLIENT) ||
+             (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);

diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index 03b66c7d45d5a5f39e872a1a72f6a528e7884513..baa894d67d71962d54b5d9875f332453d319f284 100644 (file)
--- a/bgpd/bgp_vty.c
+++ b/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));
-               json_object_string_add(json_neigh, "neighRole",
-                                      get_name_by_role(p->neighbor_role));
+                                      bgp_get_name_by_role(p->local_role));
+               json_object_string_add(json_neigh, "remoteRole",
+                                      bgp_get_name_by_role(p->remote_role));
        } else {
                vty_out(vty, "  Local Role: %s\n",
-                       get_name_by_role(p->local_role));
-               vty_out(vty, "  Neighbor Role: %s\n",
-                       get_name_by_role(p->neighbor_role));
+                       bgp_get_name_by_role(p->local_role));
+               vty_out(vty, "  Remote Role: %s\n",
+                       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"
                                : "");

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 4bf89a0375922c88948ce8db6856481afa25d114..7cfe5654ac934b05385d6c9978a37a9f205026b7 100644 (file)
--- a/bgpd/bgpd.c
+++ b/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->neighbor_role = ROLE_UNDEFINE;
+       peer->local_role = ROLE_UNDEFINED;
+       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)
-               return "undefine";
+       if (role == ROLE_UNDEFINED)
+               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. */

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index b30a3059b99d73fa5a429fef4b1893dd45801031..26ac7a4c417473e069a1d7cca6718cf408d2b965 100644 (file)
--- a/bgpd/bgpd.h
+++ b/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);
 

diff --git a/doc/user/bgp.rst b/doc/user/bgp.rst
index 76af844b37a1ad058c65619123a4d2fe1cecfa37..ee5c389bca657b66facdb2fa2f0ac32e79e19171 100644 (file)
--- a/doc/user/bgp.rst
+++ b/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

diff --git a/tests/topotests/bgp_roles_capability/r1/bgpd.conf b/tests/topotests/bgp_roles_capability/r1/bgpd.conf
index 4f152de9602f073e0864375051ebba569cedafbe..4ec83093dcd4ea4646df46c8e4b5070a7887b715 100644 (file)
--- a/tests/topotests/bgp_roles_capability/r1/bgpd.conf
+++ b/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

diff --git a/tests/topotests/bgp_roles_capability/r2/bgpd.conf b/tests/topotests/bgp_roles_capability/r2/bgpd.conf
index efca633daa121355c45c11cbe822e8d74a09f89e..e741aa16ce2b0464d307662064844bdcb919c644 100644 (file)
--- a/tests/topotests/bgp_roles_capability/r2/bgpd.conf
+++ b/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

diff --git a/tests/topotests/bgp_roles_capability/r3/bgpd.conf b/tests/topotests/bgp_roles_capability/r3/bgpd.conf
index 201c0afb2ba8f77b12b40e0aec6a9403153162e2..d1404260e66a2f6d2e1545e3e27a8d0b82bec67e 100644 (file)
--- a/tests/topotests/bgp_roles_capability/r3/bgpd.conf
+++ b/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

diff --git a/tests/topotests/bgp_roles_capability/r4/bgpd.conf b/tests/topotests/bgp_roles_capability/r4/bgpd.conf
index 30b97bb3a457fb7588d550d349b73112f174b44e..e6a0a9222aeabaf08e0dc19dcfc4759d0b1a5b38 100644 (file)
--- a/tests/topotests/bgp_roles_capability/r4/bgpd.conf
+++ b/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

diff --git a/tests/topotests/bgp_roles_capability/r5/bgpd.conf b/tests/topotests/bgp_roles_capability/r5/bgpd.conf
index b4bf73fa3dba8cb4c3333f9c0342e177adfc4423..eeeed7ed3046415ba5e6908dcafd8e1395789449 100644 (file)
--- a/tests/topotests/bgp_roles_capability/r5/bgpd.conf
+++ b/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

diff --git a/tests/topotests/bgp_roles_capability/test_bgp_roles_capability.py b/tests/topotests/bgp_roles_capability/test_bgp_roles_capability.py
index 55fc0972c18b29ce4b4d881fc0cba9ccc03092a1..28219da07b3110c54dac1e1745bdb1fe1ed56d3d 100644 (file)
--- a/tests/topotests/bgp_roles_capability/test_bgp_roles_capability.py
+++ b/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["neighRole"] == "provider"
+    assert neighbor_status["localRole"] == "undefined"
+    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")

diff --git a/tests/topotests/bgp_roles_filtering/test_bgp_roles_filtering.py b/tests/topotests/bgp_roles_filtering/test_bgp_roles_filtering.py
index 77116f474b1c4e5381800ea17fd022362494e604..c739509b508d4afc33cadc68f356fc1d47962a1a 100644 (file)
--- a/tests/topotests/bgp_roles_filtering/test_bgp_roles_filtering.py
+++ b/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()