Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions dockers/docker-fpm-frr/TSA
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions dockers/docker-fpm-frr/TSB
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions dockers/docker-fpm-frr/TSC
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavel-shirshov in my understanding we should this only "no-export" case. This is because if there is no allow list prefix configured we should tag them with drop community and continue to process other route-map. Otherwise we can break existing behaviour where we don't have prefix list by default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove the second line here, the later routing policy could remove the mark from the prefix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavel-shirshov i think we have to keep processing other route-map and can't break here . Because otherwise how we will permit default route from T0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

T0 will not send any default route, otherwise how do we choose between mulityple T0 for default routes?
Only T2 layer will send us default routes

!
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 %}
!
!
Expand Down
Original file line number Diff line number Diff line change
@@ -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
!
Original file line number Diff line number Diff line change
@@ -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
!
82 changes: 78 additions & 4 deletions src/sonic-bgpcfgd/bgpcfgd/managers_allow_list.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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):
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
"bgp": {
"allow_list": {
"enabled": true,
"default_action": "permit",
"drop_community": "12345:12345"
}
}
}
},
"allow_list_default_action": "permit"
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
"bgp": {
"allow_list": {
"enabled": true,
"default_action": "deny",
"drop_community": "12345:12345"
}
}
}
},
"allow_list_default_action": "deny"
}
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/sonic-bgpcfgd/tests/data/sonic-cfggen/tsa/isolate.conf
Original file line number Diff line number Diff line change
@@ -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
!
Original file line number Diff line number Diff line change
@@ -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
!
Loading