Merge pull request #9880 from louis-oui/maximum-prefix-out

bgpd: fixes maximum prefix out

10 files changed, 252 insertions, 33 deletions

diff --git a/bgpd/bgp_conditional_adv.c b/bgpd/bgp_conditional_adv.c
index 82eb8a815e..8e2f6ff501 100644
--- a/bgpd/bgp_conditional_adv.c
+++ b/bgpd/bgp_conditional_adv.c
@@ -95,6 +95,9 @@ static void bgp_conditional_adv_routes(struct peer *peer, afi_t afi,
 	if (!subgrp)
 		return;
 
+	subgrp->pscount = 0;
+	SET_FLAG(subgrp->sflags, SUBGRP_STATUS_TABLE_REPARSING);
+
 	if (BGP_DEBUG(update, UPDATE_OUT))
 		zlog_debug("%s: %s routes to/from %s for %s", __func__,
 			   update_type == ADVERTISE ? "Advertise" : "Withdraw",
@@ -162,6 +165,7 @@ static void bgp_conditional_adv_routes(struct peer *peer, afi_t afi,
 			}
 		}
 	}
+	UNSET_FLAG(subgrp->sflags, SUBGRP_STATUS_TABLE_REPARSING);
 }
 
 /* Handler of conditional advertisement timer event.
diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 309699fdf4..4956c7238d 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -1866,6 +1866,17 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi,
 	piattr = bgp_path_info_mpath_count(pi) ? bgp_path_info_mpath_attr(pi)
 					       : pi->attr;
 
+	if (CHECK_FLAG(peer->af_flags[afi][safi], PEER_FLAG_MAX_PREFIX_OUT) &&
+	    peer->pmax_out[afi][safi] != 0 &&
+	    subgrp->pscount >= peer->pmax_out[afi][safi]) {
+		if (BGP_DEBUG(update, UPDATE_OUT) ||
+		    BGP_DEBUG(update, UPDATE_PREFIX)) {
+			zlog_debug("%s reached maximum prefix to be send (%u)",
+				   peer->host, peer->pmax_out[afi][safi]);
+		}
+		return false;
+	}
+
 #ifdef ENABLE_BGP_VNC
 	if (((afi == AFI_IP) || (afi == AFI_IP6)) && (safi == SAFI_MPLS_VPN)
 	    && ((pi->type == ZEBRA_ROUTE_BGP_DIRECT)
diff --git a/bgpd/bgp_updgrp.c b/bgpd/bgp_updgrp.c
index dd3309dad9..a65be3d128 100644
--- a/bgpd/bgp_updgrp.c
+++ b/bgpd/bgp_updgrp.c
@@ -306,6 +306,7 @@ static void *updgrp_hash_alloc(void *p)
  *       14. encoding both global and link-local nexthop?
  *       15. If peer is configured to be a lonesoul, peer ip address
  *       16. Local-as should match, if configured.
+ *       17. maximum-prefix-out
  *      )
  */
 static unsigned int updgrp_hash_key_make(const void *p)
@@ -340,6 +341,7 @@ static unsigned int updgrp_hash_key_make(const void *p)
 	key = jhash_1word(peer->v_routeadv, key);
 	key = jhash_1word(peer->change_local_as, key);
 	key = jhash_1word(peer->max_packet_size, key);
+	key = jhash_1word(peer->pmax_out[afi][safi], key);
 
 	if (peer->group)
 		key = jhash_1word(jhash(peer->group->name,
@@ -453,6 +455,9 @@ static bool updgrp_hash_cmp(const void *p1, const void *p2)
 	if (pe1->change_local_as != pe2->change_local_as)
 		return false;
 
+	if (pe1->pmax_out[afi][safi] != pe2->pmax_out[afi][safi])
+		return false;
+
 	/* flags like route reflector client */
 	if ((flags1 & PEER_UPDGRP_AF_FLAGS) != (flags2 & PEER_UPDGRP_AF_FLAGS))
 		return false;
diff --git a/bgpd/bgp_updgrp.h b/bgpd/bgp_updgrp.h
index 694636ef3d..0da6dfd19d 100644
--- a/bgpd/bgp_updgrp.h
+++ b/bgpd/bgp_updgrp.h
@@ -204,6 +204,9 @@ struct update_subgroup {
 	/* send prefix count */
 	uint32_t scount;
 
+	/* send prefix count prior to packet update */
+	uint32_t pscount;
+
 	/* announcement attribute hash */
 	struct hash *hash;
 
@@ -248,6 +251,7 @@ struct update_subgroup {
 	uint16_t sflags;
 #define SUBGRP_STATUS_DEFAULT_ORIGINATE (1 << 0)
 #define SUBGRP_STATUS_FORCE_UPDATES (1 << 1)
+#define SUBGRP_STATUS_TABLE_REPARSING (1 << 2)
 
 	uint16_t flags;
 #define SUBGRP_FLAG_NEEDS_REFRESH (1 << 0)
diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index c6ddb1c1a7..9faf318716 100644
--- a/bgpd/bgp_updgrp_adv.c
+++ b/bgpd/bgp_updgrp_adv.c
@@ -481,13 +481,18 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest,
 		dest, subgrp,
 		bgp_addpath_id_for_peer(peer, afi, safi, &path->tx_addpath));
 
-	if (!adj) {
+	if (adj) {
+		if (CHECK_FLAG(subgrp->sflags, SUBGRP_STATUS_TABLE_REPARSING))
+			subgrp->pscount++;
+	} else {
 		adj = bgp_adj_out_alloc(
 			subgrp, dest,
 			bgp_addpath_id_for_peer(peer, afi, safi,
 						&path->tx_addpath));
 		if (!adj)
 			return;
+
+		subgrp->pscount++;
 	}
 
 	/* Check if we are sending the same route. This is needed to
@@ -607,6 +612,8 @@ void bgp_adj_out_unset_subgroup(struct bgp_dest *dest,
 			/* Free allocated information.  */
 			adj_free(adj);
 		}
+		if (!CHECK_FLAG(subgrp->sflags, SUBGRP_STATUS_TABLE_REPARSING))
+			subgrp->pscount--;
 	}
 
 	subgrp->version = MAX(subgrp->version, dest->version);
@@ -669,6 +676,9 @@ void subgroup_announce_table(struct update_subgroup *subgrp,
 			  PEER_FLAG_DEFAULT_ORIGINATE))
 		subgroup_default_originate(subgrp, 0);
 
+	subgrp->pscount = 0;
+	SET_FLAG(subgrp->sflags, SUBGRP_STATUS_TABLE_REPARSING);
+
 	for (dest = bgp_table_top(table); dest; dest = bgp_route_next(dest)) {
 		const struct prefix *dest_p = bgp_dest_get_prefix(dest);
 
@@ -722,6 +732,7 @@ void subgroup_announce_table(struct update_subgroup *subgrp,
 				}
 			}
 	}
+	UNSET_FLAG(subgrp->sflags, SUBGRP_STATUS_TABLE_REPARSING);
 
 	/*
 	 * We walked through the whole table -- make sure our version number
diff --git a/bgpd/bgp_updgrp_packet.c b/bgpd/bgp_updgrp_packet.c
index 9c32c7ed1e..341c7dd78b 100644
--- a/bgpd/bgp_updgrp_packet.c
+++ b/bgpd/bgp_updgrp_packet.c
@@ -709,21 +709,6 @@ struct bpacket *subgroup_update_packet(struct update_subgroup *subgrp)
 		addpath_tx_id = adj->addpath_tx_id;
 		path = adv->pathi;
 
-		/* Check if we need to add a prefix to the packet if
-		 * maximum-prefix-out is set for the peer.
-		 */
-		if (CHECK_FLAG(peer->af_flags[afi][safi],
-			       PEER_FLAG_MAX_PREFIX_OUT)
-		    && subgrp->scount >= peer->pmax_out[afi][safi]) {
-			if (BGP_DEBUG(update, UPDATE_OUT)
-			    || BGP_DEBUG(update, UPDATE_PREFIX)) {
-				zlog_debug(
-					"%s reached maximum prefix to be send (%u)",
-					peer->host, peer->pmax_out[afi][safi]);
-			}
-			goto next;
-		}
-
 		space_remaining = STREAM_CONCAT_REMAIN(s, snlri, STREAM_SIZE(s))
 				  - BGP_MAX_PACKET_SIZE_OVERFLOW;
 		space_needed =
@@ -876,7 +861,6 @@ struct bpacket *subgroup_update_packet(struct update_subgroup *subgrp)
 			subgrp->scount++;
 
 		adj->attr = bgp_attr_intern(adv->baa->attr);
-next:
 		adv = bgp_advertise_clean_subgroup(subgrp, adj);
 	}
 
diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index e3f1abe748..5eae350102 100644
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -7666,6 +7666,7 @@ DEFUN(neighbor_maximum_prefix_out,
       "Maximum number of prefixes to be sent to this peer\n"
       "Maximum no. of prefix limit\n")
 {
+	int ret;
 	int idx_peer = 1;
 	int idx_number = 3;
 	struct peer *peer;
@@ -7679,20 +7680,21 @@ DEFUN(neighbor_maximum_prefix_out,
 
 	max = strtoul(argv[idx_number]->arg, NULL, 10);
 
-	SET_FLAG(peer->af_flags[afi][safi], PEER_FLAG_MAX_PREFIX_OUT);
-	peer->pmax_out[afi][safi] = max;
+	ret = peer_maximum_prefix_out_set(peer, afi, safi, max);
 
-	return CMD_SUCCESS;
+	return bgp_vty_return(vty, ret);
 }
 
 DEFUN(no_neighbor_maximum_prefix_out,
       no_neighbor_maximum_prefix_out_cmd,
-      "no neighbor <A.B.C.D|X:X::X:X|WORD> maximum-prefix-out",
+      "no neighbor <A.B.C.D|X:X::X:X|WORD> maximum-prefix-out [(1-4294967295)]",
       NO_STR
       NEIGHBOR_STR
       NEIGHBOR_ADDR_STR2
-      "Maximum number of prefixes to be sent to this peer\n")
+      "Maximum number of prefixes to be sent to this peer\n"
+      "Maximum no. of prefix limit\n")
 {
+	int ret;
 	int idx_peer = 2;
 	struct peer *peer;
 	afi_t afi = bgp_node_afi(vty);
@@ -7702,10 +7704,9 @@ DEFUN(no_neighbor_maximum_prefix_out,
 	if (!peer)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	UNSET_FLAG(peer->af_flags[afi][safi], PEER_FLAG_MAX_PREFIX_OUT);
-	peer->pmax_out[afi][safi] = 0;
+	ret = peer_maximum_prefix_out_unset(peer, afi, safi);
 
-	return CMD_SUCCESS;
+	return bgp_vty_return(vty, ret);
 }
 
 /* Maximum number of prefix configuration. Prefix count is different
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 72e7a936c6..bc7203fec6 100644
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -2012,6 +2012,10 @@ static void peer_group2peer_config_copy_af(struct peer_group *group,
 		PEER_ATTR_INHERIT(peer, group, pmax_restart[afi][safi]);
 	}
 
+	/* maximum-prefix-out */
+	if (!CHECK_FLAG(pflags_ovrd, PEER_FLAG_MAX_PREFIX_OUT))
+		PEER_ATTR_INHERIT(peer, group, pmax_out[afi][safi]);
+
 	/* allowas-in */
 	if (!CHECK_FLAG(pflags_ovrd, PEER_FLAG_ALLOWAS_IN))
 		PEER_ATTR_INHERIT(peer, group, allowas_in[afi][safi]);
@@ -4220,6 +4224,7 @@ static const struct peer_flag_action peer_af_flag_action_list[] = {
 	{PEER_FLAG_MAX_PREFIX, 0, peer_change_none},
 	{PEER_FLAG_MAX_PREFIX_WARNING, 0, peer_change_none},
 	{PEER_FLAG_MAX_PREFIX_FORCE, 0, peer_change_none},
+	{PEER_FLAG_MAX_PREFIX_OUT, 0, peer_change_none},
 	{PEER_FLAG_NEXTHOP_LOCAL_UNCHANGED, 0, peer_change_reset_out},
 	{PEER_FLAG_FORCE_NEXTHOP_SELF, 1, peer_change_reset_out},
 	{PEER_FLAG_REMOVE_PRIVATE_AS_ALL, 1, peer_change_reset_out},
@@ -7319,6 +7324,96 @@ int peer_maximum_prefix_unset(struct peer *peer, afi_t afi, safi_t safi)
 	return 0;
 }
 
+void peer_maximum_prefix_out_refresh_routes(struct peer *peer, afi_t afi,
+					    safi_t safi)
+{
+	update_group_adjust_peer(peer_af_find(peer, afi, safi));
+
+	if (peer_established(peer))
+		bgp_announce_route(peer, afi, safi, false);
+}
+
+int peer_maximum_prefix_out_set(struct peer *peer, afi_t afi, safi_t safi,
+				uint32_t max)
+{
+	struct peer *member;
+	struct listnode *node, *nnode;
+
+	/* Set flag on peer and peer-group member if any */
+	peer_af_flag_set(peer, afi, safi, PEER_FLAG_MAX_PREFIX_OUT);
+	/* Set configuration on peer. */
+	peer->pmax_out[afi][safi] = max;
+
+	/* Check if handling a regular peer. */
+	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
+		/* Skip peer-group mechanics for regular peers. */
+		peer_maximum_prefix_out_refresh_routes(peer, afi, safi);
+		return 0;
+	}
+
+	/*
+	 * Set flag and configuration on all peer-group members, unless they
+	 * are explicitely overriding peer-group configuration.
+	 */
+	for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+		/* Skip peers with overridden configuration. */
+		if (CHECK_FLAG(member->af_flags_override[afi][safi],
+			       PEER_FLAG_MAX_PREFIX_OUT))
+			continue;
+
+		/* Set configuration on peer-group member. */
+		member->pmax_out[afi][safi] = max;
+
+		peer_maximum_prefix_out_refresh_routes(member, afi, safi);
+	}
+	return 0;
+}
+
+int peer_maximum_prefix_out_unset(struct peer *peer, afi_t afi, safi_t safi)
+{
+	struct peer *member;
+	struct listnode *node;
+	/* Inherit configuration from peer-group if peer is member. */
+	if (peer_group_active(peer)) {
+		peer_af_flag_inherit(peer, afi, safi, PEER_FLAG_MAX_PREFIX_OUT);
+		PEER_ATTR_INHERIT(peer, peer->group, pmax_out[afi][safi]);
+
+		peer_maximum_prefix_out_refresh_routes(peer, afi, safi);
+		return 0;
+	}
+
+	/* Remove flag and configuration from peer. */
+	peer_af_flag_unset(peer, afi, safi, PEER_FLAG_MAX_PREFIX_OUT);
+	peer->pmax_out[afi][safi] = 0;
+
+	/* Check if handling a regular peer. */
+	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
+		/* Skip peer-group mechanics for regular peers. */
+		peer_maximum_prefix_out_refresh_routes(peer, afi, safi);
+		return 0;
+	}
+
+	/*
+	 * Remove flag and configuration from all peer-group members, unless
+	 * they are explicitely overriding peer-group configuration.
+	 */
+	for (ALL_LIST_ELEMENTS_RO(peer->group->peer, node, member)) {
+		/* Skip peers with overridden configuration. */
+		if (CHECK_FLAG(member->af_flags_override[afi][safi],
+			       PEER_FLAG_MAX_PREFIX_OUT))
+			continue;
+
+		/* Remove flag and configuration on peer-group member.
+		 */
+		UNSET_FLAG(member->af_flags[afi][safi],
+			   PEER_FLAG_MAX_PREFIX_OUT);
+		member->pmax_out[afi][safi] = 0;
+
+		peer_maximum_prefix_out_refresh_routes(member, afi, safi);
+	}
+	return 0;
+}
+
 int is_ebgp_multihop_configured(struct peer *peer)
 {
 	struct peer_group *group;
diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 45ccf014f8..ae6427b356 100644
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -2207,6 +2207,13 @@ extern int peer_maximum_prefix_set(struct peer *, afi_t, safi_t, uint32_t,
 				   uint8_t, int, uint16_t, bool force);
 extern int peer_maximum_prefix_unset(struct peer *, afi_t, safi_t);
 
+extern void peer_maximum_prefix_out_refresh_routes(struct peer *peer, afi_t afi,
+						   safi_t safi);
+extern int peer_maximum_prefix_out_set(struct peer *peer, afi_t afi,
+				       safi_t safi, uint32_t max);
+extern int peer_maximum_prefix_out_unset(struct peer *peer, afi_t afi,
+					 safi_t safi);
+
 extern int peer_clear(struct peer *, struct listnode **);
 extern int peer_clear_soft(struct peer *, afi_t, safi_t, enum bgp_clear_type);
 
diff --git a/tests/topotests/bgp_maximum_prefix_out/test_bgp_maximum_prefix_out.py b/tests/topotests/bgp_maximum_prefix_out/test_bgp_maximum_prefix_out.py
index d45f00f697..696d6d94ce 100644
--- a/tests/topotests/bgp_maximum_prefix_out/test_bgp_maximum_prefix_out.py
+++ b/tests/topotests/bgp_maximum_prefix_out/test_bgp_maximum_prefix_out.py
@@ -80,22 +80,119 @@ def test_bgp_maximum_prefix_out():
     if tgen.routers_have_failure():
         pytest.skip(tgen.errors)
 
-    router = tgen.gears["r2"]
-
-    def _bgp_converge(router):
+    router1 = tgen.gears["r1"]
+    router2 = tgen.gears["r2"]
+
+    # format (router to configure, command, expected received prefixes on r2)
+    tests = [
+        # test of the initial config
+        (None, 2),
+        # modifying the max-prefix-out value
+        (
+            "router bgp\n address-family ipv4\n neighbor 192.168.255.1 maximum-prefix-out 4",
+            4,
+        ),
+        # removing the max-prefix-out value
+        (
+            "router bgp\n address-family ipv4\n no neighbor 192.168.255.1 maximum-prefix-out",
+            6,
+        ),
+        # setting a max-prefix-out value
+        (
+            "router bgp\n address-family ipv4\n neighbor 192.168.255.1 maximum-prefix-out 3",
+            3,
+        ),
+        # setting a max-prefix-out value - higher than the total number of prefix
+        (
+            "router bgp\n address-family ipv4\n neighbor 192.168.255.1 maximum-prefix-out 8",
+            6,
+        ),
+        # adding a new prefix
+        ("router bgp\n int lo\n ip address 172.16.255.249/32", 7),
+        # setting a max-prefix-out value - lower than the total number of prefix
+        (
+            "router bgp\n address-family ipv4\n neighbor 192.168.255.1 maximum-prefix-out 1",
+            1,
+        ),
+        # adding a new prefix
+        ("router bgp\n int lo\n ip address 172.16.255.248/32", 1),
+        # removing the max-prefix-out value
+        (
+            "router bgp\n address-family ipv4\n no neighbor 192.168.255.1 maximum-prefix-out 1",
+            8,
+        ),
+        # test setting the existing neighbor into a peer-group with a max-prefix-out value
+        (
+            """
+                router bgp
+                 neighbor test peer-group
+                 neighbor test remote-as 65002
+                 neighbor test timers 3 10
+                 address-family ipv4
+                  neighbor test maximum-prefix-out 3
+                 !
+                 neighbor 192.168.255.1 peer-group test
+            """,
+            3,
+        ),
+        # max-prefix-out value of the neighbor must take the precedence
+        (
+            "router bgp\n address-family ipv4\n neighbor 192.168.255.1 maximum-prefix-out 4",
+            4,
+        ),
+        (
+            "router bgp\n address-family ipv4\n no neighbor 192.168.255.1 maximum-prefix-out",
+            3,
+        ),
+        (
+            """
+                router bgp
+                 no neighbor 192.168.255.1 peer-group test
+                 neighbor 192.168.255.1 remote-as 65002
+                 neighbor 192.168.255.1 timers 3 10
+            """,
+            8,
+        ),
+        (
+            "router bgp\n address-family ipv4\n neighbor 192.168.255.1 maximum-prefix-out 5",
+            5,
+        ),
+        # test setting the existing neighbor with a max-pref-out value into a peer-group with a max-pref-out value
+        ("router bgp\n neighbor 192.168.255.1 peer-group test", 5),
+        (
+            "router bgp\n address-family ipv4\n no neighbor 192.168.255.1 maximum-prefix-out 5",
+            3,
+        ),
+    ]
+
+    def _bgp_converge(router, nb_prefixes):
         output = json.loads(router.vtysh_cmd("show ip bgp neighbor 192.168.255.2 json"))
         expected = {
             "192.168.255.2": {
                 "bgpState": "Established",
-                "addressFamilyInfo": {"ipv4Unicast": {"acceptedPrefixCounter": 2}},
+                "addressFamilyInfo": {
+                    "ipv4Unicast": {"acceptedPrefixCounter": nb_prefixes}
+                },
             }
         }
         return topotest.json_cmp(output, expected)
 
-    test_func = functools.partial(_bgp_converge, router)
-    success, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
-
-    assert result is None, 'Failed bgp convergence in "{}"'.format(router)
+    for test in tests:
+        cfg, exp_prfxs = test
+        if cfg:
+            cmd = (
+                """
+              configure terminal
+               %s
+            """
+                % cfg
+            )
+            router1.vtysh_cmd(cmd)
+
+        test_func = functools.partial(_bgp_converge, router2, exp_prfxs)
+        success, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5)
+
+        assert result is None, 'Failed bgp convergence in "{}"'.format(router2)
 
 
 if __name__ == "__main__":