[bgpcfgd]: Support default action for "Allow prefix" feature (#6370)

* Use 20 and 30 route-map entries instead of 2 and 3 for TSA * Added support for dynamic "Allow list" default action. Co-authored-by: Pavel Shirshov <[email protected]>
sonic-net · Jan 9, 2021 · 03391f2 · 03391f2
1 parent bfec282
commit 03391f2
Show file tree

Hide file tree

Showing 14 changed files with 291 additions and 36 deletions.
diff --git a/dockers/docker-fpm-frr/TSA b/dockers/docker-fpm-frr/TSA
@@ -6,9 +6,9 @@ function check_not_installed()
   config=$(vtysh -c "show run")
   for route_map_name in $(echo "$config" | sed -ne 's/  neighbor \S* route-map \(\S*\) out/\1/p');
   do
-    echo "$config" | grep -q "route-map $route_map_name permit 2"
+    echo "$config" | grep -q "route-map $route_map_name permit 20"
     c=$((c+$?))
-    echo "$config" | grep -q "route-map $route_map_name deny 3"
+    echo "$config" | grep -q "route-map $route_map_name deny 30"
     c=$((c+$?))
   done
   return $c

diff --git a/dockers/docker-fpm-frr/TSB b/dockers/docker-fpm-frr/TSB
@@ -7,10 +7,10 @@ function check_installed()
   config=$(vtysh -c "show run")
   for route_map_name in $(echo "$config" | sed -ne 's/  neighbor \S* route-map \(\S*\) out/\1/p');
   do
-    echo "$config" | grep -q "route-map $route_map_name permit 2"
+    echo "$config" | grep -q "route-map $route_map_name permit 20"
     c=$((c+$?))
     e=$((e+1))
-    echo "$config" | grep -q "route-map $route_map_name deny 3"
+    echo "$config" | grep -q "route-map $route_map_name deny 30"
     c=$((c+$?))
     e=$((e+1))
   done

diff --git a/dockers/docker-fpm-frr/TSC b/dockers/docker-fpm-frr/TSC
@@ -6,9 +6,9 @@ function check_not_installed()
   config=$(vtysh -c "show run")
   for route_map_name in $(echo "$config" | sed -ne 's/  neighbor \S* route-map \(\S*\) out/\1/p' | egrep 'V4|V6');
   do
-    echo "$config" | grep -q "route-map $route_map_name permit 2"
+    echo "$config" | grep -q "route-map $route_map_name permit 20"
     c=$((c+$?))
-    echo "$config" | grep -q "route-map $route_map_name deny 3"
+    echo "$config" | grep -q "route-map $route_map_name deny 30"
     c=$((c+$?))
   done
   return $c
@@ -21,10 +21,10 @@ function check_installed()
   config=$(vtysh -c "show run")
   for route_map_name in $(echo "$config" | sed -ne 's/  neighbor \S* route-map \(\S*\) out/\1/p' | egrep 'V4|V6');
   do
-    echo "$config" | grep -q "route-map $route_map_name permit 2"
+    echo "$config" | grep -q "route-map $route_map_name permit 20"
     c=$((c+$?))
     e=$((e+1))
-    echo "$config" | grep -q "route-map $route_map_name deny 3"
+    echo "$config" | grep -q "route-map $route_map_name deny 30"
     c=$((c+$?))
     e=$((e+1))
   done

diff --git a/dockers/docker-fpm-frr/frr/bgpd/templates/general/policies.conf.j2 b/dockers/docker-fpm-frr/frr/bgpd/templates/general/policies.conf.j2
@@ -3,29 +3,46 @@
 !
 !
 !
-{% if constants.bgp.allow_list is defined and constants.bgp.allow_list.enabled is defined and constants.bgp.allow_list.enabled %}
-{%   if constants.bgp.allow_list.default_action is defined and constants.bgp.allow_list.default_action.strip() == 'deny' %}
+{% if constants.bgp.allow_list is defined and constants.bgp.allow_list.enabled is defined and constants.bgp.allow_list.enabled and constants.bgp.allow_list.drop_community is defined %}
+!
+!
+! please don't remove. 65535 entries are default rules
+! which works when allow_list is enabled, but new configuration
+! is not applied
+!
+{%   if allow_list_default_action == 'deny' %}
+!
 route-map ALLOW_LIST_DEPLOYMENT_ID_0_V4 permit 65535
   set community no-export additive
 !
 route-map ALLOW_LIST_DEPLOYMENT_ID_0_V6 permit 65535
   set community no-export additive
 {%   else %}
+!
 route-map ALLOW_LIST_DEPLOYMENT_ID_0_V4 permit 65535
   set community {{ constants.bgp.allow_list.drop_community }} additive
 !
 route-map ALLOW_LIST_DEPLOYMENT_ID_0_V6 permit 65535
   set community {{ constants.bgp.allow_list.drop_community }} additive
 {%   endif %}
 !
-route-map FROM_BGP_PEER_V4 permit 2
+bgp community-list standard allow_list_default_community permit no-export
+bgp community-list standard allow_list_default_community permit {{ constants.bgp.allow_list.drop_community }}
+!
+route-map FROM_BGP_PEER_V4 permit 10
   call ALLOW_LIST_DEPLOYMENT_ID_0_V4
   on-match next
 !
-route-map FROM_BGP_PEER_V6 permit 2
+route-map FROM_BGP_PEER_V4 permit 11
+  match community allow_list_default_community
+!
+route-map FROM_BGP_PEER_V6 permit 10
   call ALLOW_LIST_DEPLOYMENT_ID_0_V6
   on-match next
 !
+route-map FROM_BGP_PEER_V6 permit 11
+  match community allow_list_default_community
+!
 {% endif %}
 !
 !

diff --git a/dockers/docker-fpm-frr/frr/bgpd/tsa/bgpd.tsa.isolate.conf.j2 b/dockers/docker-fpm-frr/frr/bgpd/tsa/bgpd.tsa.isolate.conf.j2
@@ -1,5 +1,5 @@
-route-map {{ route_map_name }} permit 2
+route-map {{ route_map_name }} permit 20
   match {{ ip_protocol }} address prefix-list PL_Loopback{{ ip_version }}
   set community {{ constants.bgp.traffic_shift_community }}
-route-map {{ route_map_name }} deny 3
+route-map {{ route_map_name }} deny 30
 !
diff --git a/dockers/docker-fpm-frr/frr/bgpd/tsa/bgpd.tsa.unisolate.conf.j2 b/dockers/docker-fpm-frr/frr/bgpd/tsa/bgpd.tsa.unisolate.conf.j2
@@ -1,3 +1,3 @@
-no route-map {{ route_map_name }} permit 2
-no route-map {{ route_map_name }} deny 3
+no route-map {{ route_map_name }} permit 20
+no route-map {{ route_map_name }} deny 30
 !
diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_allow_list.py b/src/sonic-bgpcfgd/bgpcfgd/managers_allow_list.py
@@ -36,8 +36,6 @@ def __init__(self, common_objs, db, table):
             db,
             table,
         )
-        self.cfg_mgr = common_objs["cfg_mgr"]
-        self.constants = common_objs["constants"]
         self.key_re = re.compile(r"^DEPLOYMENT_ID\|\d+\|\S+$|^DEPLOYMENT_ID\|\d+$")
         self.enabled = self.__get_enabled()
         self.__load_constant_lists()
@@ -63,7 +61,8 @@ def set_handler(self, key, data):
             prefixes_v4 = str(data['prefixes_v4']).split(",")
         if "prefixes_v6" in data:
             prefixes_v6 = str(data['prefixes_v6']).split(",")
-        self.__update_policy(deployment_id, community_value, prefixes_v4, prefixes_v6)
+        default_action_community = self.__get_default_action_community(data)
+        self.__update_policy(deployment_id, community_value, prefixes_v4, prefixes_v6, default_action_community)
         return True
 
     def __set_handler_validate(self, key, data):
@@ -96,6 +95,9 @@ def __set_handler_validate(self, key, data):
         if not prefixes_v4 and not prefixes_v6:
             log_err("BGPAllowListMgr::Received BGP ALLOWED 'SET' message with no prefixes specified: %s" % str(data))
             return False
+        if "default_action" in data and data["default_action"] != "permit" and data["default_action"] != "deny":
+            log_err("BGPAllowListMgr::Received BGP ALLOWED 'SET' message with invalid 'default_action' field: '%s'" % str(data))
+            return False
         return True
 
     def del_handler(self, key):
@@ -124,13 +126,14 @@ def __del_handler_validate(self, key):
             return False
         return True
 
-    def __update_policy(self, deployment_id, community_value, prefixes_v4, prefixes_v6):
+    def __update_policy(self, deployment_id, community_value, prefixes_v4, prefixes_v6, default_action):
         """
         Update "allow list" policy with parameters
         :param deployment_id: deployment id which policy will be changed
         :param community_value: community value to match for the updated policy
         :param prefixes_v4: a list of v4 prefixes for the updated policy
         :param prefixes_v6: a list of v6 prefixes for the updated policy
+        :param default_action: the default action for the policy. should be either 'permit' or 'deny'
         """
         # update all related entries with the information
         info = deployment_id, community_value, str(prefixes_v4), str(prefixes_v6)
@@ -146,6 +149,8 @@ def __update_policy(self, deployment_id, community_value, prefixes_v4, prefixes_
         cmds += self.__update_community(names['community'], community_value)
         cmds += self.__update_allow_route_map_entry(self.V4, names['pl_v4'], names['community'], names['rm_v4'])
         cmds += self.__update_allow_route_map_entry(self.V6, names['pl_v6'], names['community'], names['rm_v6'])
+        cmds += self.__update_default_route_map_entry(names['rm_v4'], default_action)
+        cmds += self.__update_default_route_map_entry(names['rm_v6'], default_action)
         if cmds:
             self.cfg_mgr.push_list(cmds)
             peer_groups = self.__find_peer_group_by_deployment_id(deployment_id)
@@ -365,6 +370,52 @@ def __update_allow_route_map_entry(self, af, allow_address_pl_name, community_na
             cmds.append(" match community %s" % community_name)
         return cmds
 
+    def __update_default_route_map_entry(self, route_map_name, default_action_community):
+        """
+        Add or update default action rule for the route-map.
+        Default action rule is hardcoded into route-map permit 65535
+        :param route_map_name: name of the target route_map
+        :param default_action_community: community value to mark not-matched prefixes
+        """
+        info = route_map_name, default_action_community
+        log_debug("BGPAllowListMgr::__update_default_route_map_entry. rm='%s' set_community='%s'" % info)
+        current_default_action_value = self.__parse_default_action_route_map_entry(route_map_name)
+        if current_default_action_value != default_action_community:
+            return [
+                'route-map %s permit 65535' % route_map_name,
+                ' set community %s additive' % default_action_community
+            ]
+        else:
+            return []
+
+    def __parse_default_action_route_map_entry(self, route_map_name):
+        """
+        Parse default-action route-map entry
+        :param route_map_name: Name of the route-map to parse
+        :return: a community value used for default action
+        """
+        log_debug("BGPAllowListMgr::__parse_default_action_route_map_entries. rm='%s'" % route_map_name)
+        match_string = 'route-map %s permit 65535' % route_map_name
+        match_community = re.compile(r'^set community (\S+) additive$')
+        inside_route_map = False
+        community_value = ""
+        conf = self.cfg_mgr.get_text()
+        for line in conf + [""]:
+            s_line = line.strip()
+            if inside_route_map:
+                matched = match_community.match(s_line)
+                if matched:
+                    community_value = matched.group(1)
+                    break
+                else:
+                    log_err("BGPAllowListMgr::Found incomplete route-map '%s' entry. seq_no=65535" % route_map_name)
+                inside_route_map = False
+            elif s_line == match_string:
+                inside_route_map = True
+        if community_value == "":
+            log_err("BGPAllowListMgr::Default action community value is not found. route-map '%s' entry. seq_no=65535" % route_map_name)
+        return community_value
+
     def __remove_allow_route_map_entry(self, af, allow_address_pl_name, community_name, route_map_name):
         """
         Add or update a "Allow address" route-map entry with the parameters
@@ -624,3 +675,26 @@ def __af_to_family(self, af):
         :return: prefix list ip family
         """
         return 'ip' if af == self.V4 else 'ipv6'
+
+    def __get_default_action_community(self, data):
+        """
+        Determine the default action community based on the request.
+        If request doesn't contain "default_action" field - the default_action value
+        from the constants is being used
+        :param data: SET request data
+        :return: returns community value for "default_action"
+        """
+        drop_community = self.constants["bgp"]["allow_list"]["drop_community"]
+        if "default_action" in data:
+            if data["default_action"] == "deny":
+                return "no-export"
+            else: # "permit"
+                return drop_community
+        else:
+            if "default_action" in self.constants["bgp"]["allow_list"]:
+                if self.constants["bgp"]["allow_list"]["default_action"].strip() == "deny":
+                    return "no-export"
+                else:
+                    return drop_community
+            else:
+                return drop_community
diff --git a/src/sonic-bgpcfgd/tests/data/general/policies.conf/param_all.json b/src/sonic-bgpcfgd/tests/data/general/policies.conf/param_all.json
@@ -4,9 +4,9 @@
         "bgp": {
             "allow_list": {
                 "enabled": true,
-                "default_action": "permit",
                 "drop_community": "12345:12345"
             }
         }
-    }
+    },
+    "allow_list_default_action": "permit"
 }
diff --git a/src/sonic-bgpcfgd/tests/data/general/policies.conf/param_deny.json b/src/sonic-bgpcfgd/tests/data/general/policies.conf/param_deny.json
@@ -4,9 +4,9 @@
         "bgp": {
             "allow_list": {
                 "enabled": true,
-                "default_action": "deny",
                 "drop_community": "12345:12345"
             }
         }
-    }
+    },
+    "allow_list_default_action": "deny"
 }
diff --git a/src/sonic-bgpcfgd/tests/data/general/policies.conf/result_all.conf b/src/sonic-bgpcfgd/tests/data/general/policies.conf/result_all.conf
@@ -1,20 +1,33 @@
 !
 ! template: bgpd/templates/general/policies.conf.j2
 !
+! please don't remove. 65535 entries are default rules
+! which works when allow_list is enabled, but new configuration
+! is not applied
+!
 route-map ALLOW_LIST_DEPLOYMENT_ID_0_V4 permit 65535
   set community 12345:12345 additive
 !
 route-map ALLOW_LIST_DEPLOYMENT_ID_0_V6 permit 65535
   set community 12345:12345 additive
 !
-route-map FROM_BGP_PEER_V4 permit 2
+bgp community-list standard allow_list_default_community permit no-export
+bgp community-list standard allow_list_default_community permit 12345:12345
+!
+route-map FROM_BGP_PEER_V4 permit 10
   call ALLOW_LIST_DEPLOYMENT_ID_0_V4
   on-match next
 !
-route-map FROM_BGP_PEER_V6 permit 2
+route-map FROM_BGP_PEER_V4 permit 11
+  match community allow_list_default_community
+!
+route-map FROM_BGP_PEER_V6 permit 10
   call ALLOW_LIST_DEPLOYMENT_ID_0_V6
   on-match next
 !
+route-map FROM_BGP_PEER_V6 permit 11
+  match community allow_list_default_community
+!
 route-map FROM_BGP_PEER_V4 permit 100
 !
 route-map TO_BGP_PEER_V4 permit 100

diff --git a/src/sonic-bgpcfgd/tests/data/general/policies.conf/result_deny.conf b/src/sonic-bgpcfgd/tests/data/general/policies.conf/result_deny.conf
@@ -1,20 +1,33 @@
 !
 ! template: bgpd/templates/general/policies.conf.j2
 !
+! please don't remove. 65535 entries are default rules
+! which works when allow_list is enabled, but new configuration
+! is not applied
+!
 route-map ALLOW_LIST_DEPLOYMENT_ID_0_V4 permit 65535
   set community no-export additive
 !
 route-map ALLOW_LIST_DEPLOYMENT_ID_0_V6 permit 65535
   set community no-export additive
 !
-route-map FROM_BGP_PEER_V4 permit 2
+bgp community-list standard allow_list_default_community permit no-export
+bgp community-list standard allow_list_default_community permit 12345:12345
+!
+route-map FROM_BGP_PEER_V4 permit 10
   call ALLOW_LIST_DEPLOYMENT_ID_0_V4
   on-match next
 !
-route-map FROM_BGP_PEER_V6 permit 2
+route-map FROM_BGP_PEER_V4 permit 11
+  match community allow_list_default_community
+!
+route-map FROM_BGP_PEER_V6 permit 10
   call ALLOW_LIST_DEPLOYMENT_ID_0_V6
   on-match next
 !
+route-map FROM_BGP_PEER_V6 permit 11
+  match community allow_list_default_community
+!
 route-map FROM_BGP_PEER_V4 permit 100
 !
 route-map TO_BGP_PEER_V4 permit 100

diff --git a/src/sonic-bgpcfgd/tests/data/sonic-cfggen/tsa/isolate.conf b/src/sonic-bgpcfgd/tests/data/sonic-cfggen/tsa/isolate.conf
@@ -1,5 +1,5 @@
-route-map test_rm_name permit 2
+route-map test_rm_name permit 20
   match ip address prefix-list PL_LoopbackV4
   set community 12345:555
-route-map test_rm_name deny 3
+route-map test_rm_name deny 30
 !
diff --git a/src/sonic-bgpcfgd/tests/data/sonic-cfggen/tsa/unisolate.conf b/src/sonic-bgpcfgd/tests/data/sonic-cfggen/tsa/unisolate.conf
@@ -1,3 +1,3 @@
-no route-map test_rm permit 2
-no route-map test_rm deny 3
+no route-map test_rm permit 20
+no route-map test_rm deny 30
 !