limit community list count #17836

pguibert6WIND · 2025-01-10T16:50:09Z

Add the ability to configure a limit on the number of received community entries.

ton31337

Are you planning to implement the same for other types of communities (large, extended)?

Also... IMO we should somehow treat well-known communities to be an exception for this limit, what do you think? Imagine I have a limit 5, but my route has 6 communities and one of them is no-export or no-advertise.

ton31337 · 2025-01-11T11:08:28Z

doc/user/bgp.rst

@@ -2693,6 +2693,11 @@ The following commands can be used in route maps:
   happen only when BGP updates have completely same communities value
   specified in the community list.

+.. clicmd:: match community-limit (1-1024)


Any reason why this is limited to 1k? And why we can't use 0?

what I can do is begin at 0 and consider this limit by comparing with <= instead of <.

ton31337 · 2025-01-11T11:09:26Z

tests/topotests/bgp_comm_list_match/test_bgp_comm_list_match.py

+    router = tgen.gears["r3"]
+    router.vtysh_cmd(
+        """
+        configure terminal\n


Please remove \n chars, newlines are handled out of the box.

ton31337 · 2025-01-11T11:09:39Z

tests/topotests/bgp_comm_list_match/test_bgp_comm_list_match.py

+    router = tgen.gears["r3"]
+    router.vtysh_cmd(
+        """
+        configure terminal\n


ton31337 · 2025-01-11T11:10:16Z

yang/frr-bgp-route-map.yang

+    case community-limit {
+      when "derived-from-or-self(../frr-route-map:condition, 'frr-bgp-route-map:match-community-limit')";
+      description
+        "Match eVPN route-distinguisher";


Wrong description.

ton31337 · 2025-01-11T11:11:17Z

bgpd/bgp_routemap.c

+	}
+
+	if (argc < idx_limit) {
+		vty_out(vty, "%% BGP limit count not given.\n");


Somehow the description is not as I would expect... What BGP limit?

ton31337 · 2025-01-11T11:12:26Z

bgpd/bgp_routemap.c

+DEFPY_YANG(
+	match_community_limit, match_community_limit_cmd,
+	"[no$no] match community-limit [(1-1024)$limit]",
+	NO_STR MATCH_STR "Match BGP community limit\n"


Please use the format as any other (every new token with a new line). E.g.:

NO_STR MATCH_STR ...

ton31337 · 2025-01-11T11:13:22Z

bgpd/bgp_routemap.c

+	struct bgp_path_info *path = NULL;
+	struct community *picomm = NULL;
+	uint16_t count = 0;
+	uint16_t *limit_rule = (uint16_t *)rule;


nit: Casting not needed here.

uint16_t *limit_rule = rule;

pguibert6WIND · 2025-01-13T12:46:34Z

Are you planning to implement the same for other types of communities (large, extended)?

I plan to implement extended comm limit and as path limit.

Also... IMO we should somehow treat well-known communities to be an exception for this limit, what do you think? Imagine I have a limit 5, but my route has 6 communities and one of them is no-export or no-advertise.

I would need to know more about those specific communities. Where in frr code, do we add this community dynamically.

At first glance, I don't see any exceptions. I see the limit option as an administrative control in a domain where no device should use more than the defined limitation count. As counter example, which community should I keep if my limit is 1 and I have both no-export an no-advertise ?

ton31337 · 2025-01-13T12:49:35Z

As counter example, which community should I keep if my limit is 1 and I have both no-export an no-advertise ?

Keep both?

ton31337 · 2025-01-14T07:22:01Z

bgpd/bgp_routemap.c

+	picomm = bgp_attr_get_community(path->attr);
+	if (picomm)
+		count = picomm->size;
+	if (!count)


If the limit is 0, it means "no limit"? Or it should be reverse?

having 0 communities in the BGP update, it will always match. I simplified code and added comments.

What if I want to deny any routes with any community attached?

ton31337 · 2025-01-14T07:22:09Z

bgpd/bgp_routemap.c

@@ -1303,6 +1303,58 @@ static const struct route_map_rule_cmd route_match_evpn_rd_cmd = {
 	route_match_rd_free
 };

+/* `match community-limit' */
+
+/* Match function should return 1 if match is success else return zero. */


Wrong comment.

modified comment.

ton31337 · 2025-01-14T07:22:54Z

bgpd/bgp_routemap.c

+}
+
+/* Route map commands for community limit matching. */
+static const struct route_map_rule_cmd route_match_community_limit_cmd = {


Could you keep formatting for route_map_rule_cmd as others? (ignoring frrbot)

ton31337 · 2025-01-14T07:24:42Z

bgpd/bgp_routemap.c

@@ -5708,6 +5760,36 @@ DEFPY_YANG(
 	return nb_cli_apply_changes(vty, NULL);
 }

+DEFPY_YANG(
+	match_community_limit, match_community_limit_cmd,
+	"[no$no] match community-limit [(0-65535)$limit]",


We should do instead this way, and then checking for idx_limit is not needed.

"[no$no] match community-limit ![(0-65535)$limit]",

ton31337 · 2025-01-14T07:26:49Z

doc/user/bgp.rst

@@ -2693,6 +2693,11 @@ The following commands can be used in route maps:
   happen only when BGP updates have completely same communities value
   specified in the community list.

+.. clicmd:: match community-limit (0-65535)
+
+   This command matches BGP updates that use community list, and with a community


We should also document what 0 means explicitly. Because what is now, it means unlimited, but me as an operator understand it as "do not accept the route if it has any community attached".

whatever the meaning, I will document it. thanks for advice.

ton31337 · 2025-01-14T07:26:57Z

lib/routemap.c

@@ -1568,7 +1568,6 @@ enum rmap_compile_rets route_map_add_match(struct route_map_index *index,
 		rule->rule_str = XSTRDUP(MTYPE_ROUTE_MAP_RULE_STR, match_arg);
 	else
 		rule->rule_str = NULL;
-


Let's drop this change.

ton31337 · 2025-01-14T07:27:25Z

tests/topotests/bgp_comm_list_match/r3/bgpd.conf

@@ -11,6 +11,7 @@ router bgp 65003
  neighbor 192.168.1.2 soft-reconfiguration inbound
 exit-address-family
 !
+bgp route-map delay-timer 5


Is this needed?

no, u are right.

pguibert6WIND · 2025-01-14T08:18:13Z

As counter example, which community should I keep if my limit is 1 and I have both no-export an no-advertise ?

Keep both?

why those communities are so special ? is there a RFC that describes this?

Add a mechanism in route-map to filter out route-map which have a list of communities greater than the given number. Signed-off-by: Philippe Guibert <[email protected]>

Add a test to control the community-list count. Signed-off-by: Philippe Guibert <[email protected]>

riw777

I wonder if this should not apply to well-known communities? I suspect the result would be surprising to operators if we "randomly" choose which of the well-known communities to keep and which to discard, as these do have some pretty strong policy implications?

pguibert6WIND · 2025-01-14T15:15:45Z

I wonder if this should not apply to well-known communities? I suspect the result would be surprising to operators if we "randomly" choose which of the well-known communities to keep and which to discard, as these do have some pretty strong policy implications?

An optional keyword could be added to relax the default behavior, for all well known attributes.

pguibert6WIND · 2025-01-14T16:23:05Z

for information, some do this:
https://www.juniper.net/documentation/us/en/software/junos/routing-policy/bgp/topics/example/policy-community-count.html

riw777

looks good ... waiting on @ton31337 's comments

donaldsharp · 2025-01-15T15:36:02Z

Yes, I'm pretty happy with this now. We discussed this during the meeting yesterday. End result of blanket dropping the route when hitting the community limit is reasonable. Just waiting on Donatas's comments

frrbot bot added the bgp label Jan 10, 2025

github-actions bot added size/L master labels Jan 10, 2025

ton31337 reviewed Jan 11, 2025

View reviewed changes

pguibert6WIND force-pushed the limit_comm_list_count branch from f52fa87 to e2b0e12 Compare January 13, 2025 12:39

ton31337 reviewed Jan 14, 2025

View reviewed changes

pguibert6WIND force-pushed the limit_comm_list_count branch from e2b0e12 to b81d971 Compare January 14, 2025 08:19

pguibert6WIND added 2 commits January 14, 2025 13:59

bgpd: add 'match community-count' command to restrict comm count

f19b866

Add a mechanism in route-map to filter out route-map which have a list of communities greater than the given number. Signed-off-by: Philippe Guibert <[email protected]>

topotest: add a test to control the community-list count

bd4b8c3

Add a test to control the community-list count. Signed-off-by: Philippe Guibert <[email protected]>

pguibert6WIND force-pushed the limit_comm_list_count branch from b81d971 to bd4b8c3 Compare January 14, 2025 13:01

riw777 reviewed Jan 14, 2025

View reviewed changes

ton31337 approved these changes Jan 17, 2025

View reviewed changes

ton31337 merged commit 705e6f8 into FRRouting:master Jan 17, 2025
11 checks passed

limit community list count #17836

limit community list count #17836

Conversation

pguibert6WIND commented Jan 10, 2025

ton31337 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguibert6WIND Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguibert6WIND commented Jan 13, 2025

ton31337 commented Jan 13, 2025

Choose a reason for hiding this comment

pguibert6WIND Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguibert6WIND commented Jan 14, 2025 • edited Loading

riw777 left a comment

Choose a reason for hiding this comment

pguibert6WIND commented Jan 14, 2025

pguibert6WIND commented Jan 14, 2025

riw777 left a comment

Choose a reason for hiding this comment

donaldsharp commented Jan 15, 2025

ton31337 left a comment •

edited

Loading

pguibert6WIND Jan 13, 2025 •

edited

Loading

pguibert6WIND Jan 14, 2025 •

edited

Loading

pguibert6WIND commented Jan 14, 2025 •

edited

Loading