From 543dbf240604e7c15b119a00851259cc180bf381 Mon Sep 17 00:00:00 2001 From: stormliang Date: Tue, 5 Apr 2022 16:15:17 +0000 Subject: [PATCH 01/13] support router advertise with community id --- .../bgpcfgd/managers_advertise_rt.py | 29 +++-- src/sonic-bgpcfgd/bgpcfgd/managers_rm.py | 115 ++++++++++++++++++ 2 files changed, 133 insertions(+), 11 deletions(-) create mode 100644 src/sonic-bgpcfgd/bgpcfgd/managers_rm.py diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index 352b89f7286c..5d37163d9848 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -29,7 +29,7 @@ def __init__(self, common_objs, db, table): def set_handler(self, key, data): vrf, ip_prefix = self.split_key(key) - self.add_route_advertisement(vrf, ip_prefix) + self.add_route_advertisement(vrf, ip_prefix, data) return True @@ -39,27 +39,27 @@ def del_handler(self, key): self.remove_route_advertisement(vrf, ip_prefix) - def add_route_advertisement(self, vrf, ip_prefix): + def add_route_advertisement(self, vrf, ip_prefix, data): if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): - if not self.advertised_routes.get(vrf, set()): + if not self.advertised_routes.get(vrf, dict()): self.bgp_network_import_check_commands(vrf, self.OP_ADD) - self.advertise_route_commands(ip_prefix, vrf, self.OP_ADD) + self.advertise_route_commands(ip_prefix, vrf, self.OP_ADD, data) - self.advertised_routes.setdefault(vrf, set()).add(ip_prefix) + self.advertised_routes.setdefault(vrf, dict()).update({ip_prefix:data}) def remove_route_advertisement(self, vrf, ip_prefix): - self.advertised_routes.setdefault(vrf, set()).discard(ip_prefix) - if not self.advertised_routes.get(vrf, set()): + self.advertised_routes.setdefault(vrf, dict()).pop(ip_prefix) + if not self.advertised_routes.get(vrf, dict()): self.advertised_routes.pop(vrf, None) if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): - if not self.advertised_routes.get(vrf, set()): + if not self.advertised_routes.get(vrf, dict()): self.bgp_network_import_check_commands(vrf, self.OP_DELETE) self.advertise_route_commands(ip_prefix, vrf, self.OP_DELETE) - def advertise_route_commands(self, ip_prefix, vrf, op): + def advertise_route_commands(self, ip_prefix, vrf, op, data = None): is_ipv6 = TemplateFabric.is_ipv6(ip_prefix) bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] @@ -70,7 +70,14 @@ def advertise_route_commands(self, ip_prefix, vrf, op): cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) cmd_list.append(" address-family %s unicast" % ("ipv6" if is_ipv6 else "ipv4")) - cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix)) + ''' + For set operation, need to check if data is same or not, + need to check if it is ok by overwriting existing value or need to follow no/add sequence + ''' + if data and 'route-map' in data: + cmd_list.append(" network %s route-map %s" % (ip_prefix, data['route-map'])) + else: + cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix)) self.cfg_mgr.push_list(cmd_list) @@ -92,7 +99,7 @@ def on_bgp_asn_change(self): for vrf, ip_prefixes in self.advertised_routes.items(): self.bgp_network_import_check_commands(vrf, self.OP_ADD) for ip_prefix in ip_prefixes: - self.add_route_advertisement(vrf, ip_prefix) + self.add_route_advertisement(vrf, ip_prefix, ip_prefixes[ip_prefix]) @staticmethod diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py new file mode 100644 index 000000000000..5d37163d9848 --- /dev/null +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py @@ -0,0 +1,115 @@ +from .manager import Manager +from .template import TemplateFabric +from swsscommon import swsscommon + + +class AdvertiseRouteMgr(Manager): + """ This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated """ + def __init__(self, common_objs, db, table): + """ + Initialize the object + :param common_objs: common object dictionary + :param db: name of the db + :param table: name of the table in the db + """ + super(AdvertiseRouteMgr, self).__init__( + common_objs, + [], + db, + table, + ) + + self.directory.subscribe([("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], self.on_bgp_asn_change) + self.advertised_routes = dict() + + + OP_DELETE = 'DELETE' + OP_ADD = 'ADD' + + + def set_handler(self, key, data): + vrf, ip_prefix = self.split_key(key) + self.add_route_advertisement(vrf, ip_prefix, data) + + return True + + + def del_handler(self, key): + vrf, ip_prefix = self.split_key(key) + self.remove_route_advertisement(vrf, ip_prefix) + + + def add_route_advertisement(self, vrf, ip_prefix, data): + if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): + if not self.advertised_routes.get(vrf, dict()): + self.bgp_network_import_check_commands(vrf, self.OP_ADD) + self.advertise_route_commands(ip_prefix, vrf, self.OP_ADD, data) + + self.advertised_routes.setdefault(vrf, dict()).update({ip_prefix:data}) + + + def remove_route_advertisement(self, vrf, ip_prefix): + self.advertised_routes.setdefault(vrf, dict()).pop(ip_prefix) + if not self.advertised_routes.get(vrf, dict()): + self.advertised_routes.pop(vrf, None) + + if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): + if not self.advertised_routes.get(vrf, dict()): + self.bgp_network_import_check_commands(vrf, self.OP_DELETE) + self.advertise_route_commands(ip_prefix, vrf, self.OP_DELETE) + + + def advertise_route_commands(self, ip_prefix, vrf, op, data = None): + is_ipv6 = TemplateFabric.is_ipv6(ip_prefix) + bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] + + cmd_list = [] + if vrf == 'default': + cmd_list.append("router bgp %s" % bgp_asn) + else: + cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) + + cmd_list.append(" address-family %s unicast" % ("ipv6" if is_ipv6 else "ipv4")) + ''' + For set operation, need to check if data is same or not, + need to check if it is ok by overwriting existing value or need to follow no/add sequence + ''' + if data and 'route-map' in data: + cmd_list.append(" network %s route-map %s" % (ip_prefix, data['route-map'])) + else: + cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix)) + + self.cfg_mgr.push_list(cmd_list) + + + def bgp_network_import_check_commands(self, vrf, op): + bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] + cmd_list = [] + if vrf == 'default': + cmd_list.append("router bgp %s" % bgp_asn) + else: + cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) + cmd_list.append(" %sbgp network import-check" % ('' if op == self.OP_DELETE else 'no ')) + + self.cfg_mgr.push_list(cmd_list) + + + def on_bgp_asn_change(self): + if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): + for vrf, ip_prefixes in self.advertised_routes.items(): + self.bgp_network_import_check_commands(vrf, self.OP_ADD) + for ip_prefix in ip_prefixes: + self.add_route_advertisement(vrf, ip_prefix, ip_prefixes[ip_prefix]) + + + @staticmethod + def split_key(key): + """ + Split key into vrf name and prefix. + :param key: key to split + :return: vrf name extracted from the key, ip prefix extracted from the key + """ + if '|' not in key: + return 'default', key + else: + return tuple(key.split('|', 1)) From ea3cc7a03f994d1cfa2e0ca1f65a638b787cf49c Mon Sep 17 00:00:00 2001 From: stormliang Date: Wed, 6 Apr 2022 16:12:05 +0000 Subject: [PATCH 02/13] work at 7th Apr --- src/sonic-bgpcfgd/bgpcfgd/main.py | 2 + src/sonic-bgpcfgd/bgpcfgd/managers_rm.py | 121 +++++++---------------- 2 files changed, 38 insertions(+), 85 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/main.py b/src/sonic-bgpcfgd/bgpcfgd/main.py index 28359dd62eb8..5c06546701cc 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/main.py +++ b/src/sonic-bgpcfgd/bgpcfgd/main.py @@ -17,6 +17,7 @@ from .managers_intf import InterfaceMgr from .managers_setsrc import ZebraSetSrc from .managers_static_rt import StaticRouteMgr +from .managers_rm import RouteMapMgr from .runner import Runner, signal_handler from .template import TemplateFabric from .utils import read_constants @@ -62,6 +63,7 @@ def do_work(): StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE"), # Route Advertisement Managers AdvertiseRouteMgr(common_objs, "STATE_DB", swsscommon.STATE_ADVERTISE_NETWORK_TABLE_NAME), + RouteMapMgr(common_objs, "STATE_DB", swsscommon.STATE_ROUTE_MAP_TABLE_NAME), ] runner = Runner(common_objs['cfg_mgr']) for mgr in managers: diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py index 5d37163d9848..9e7e9927fc41 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py @@ -1,10 +1,7 @@ from .manager import Manager -from .template import TemplateFabric -from swsscommon import swsscommon - -class AdvertiseRouteMgr(Manager): - """ This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated """ +class RouteMapMgr(Manager): + """ This class add route-map when ROUTE_MAP_TABLE in STATE_DB is updated """ def __init__(self, common_objs, db, table): """ Initialize the object @@ -12,104 +9,58 @@ def __init__(self, common_objs, db, table): :param db: name of the db :param table: name of the table in the db """ - super(AdvertiseRouteMgr, self).__init__( + super(RouteMapMgr, self).__init__( common_objs, [], db, table, ) - - self.directory.subscribe([("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], self.on_bgp_asn_change) - self.advertised_routes = dict() - - - OP_DELETE = 'DELETE' - OP_ADD = 'ADD' + VALID_RM = ['VXLAN_OV_ECMP_RM'] def set_handler(self, key, data): - vrf, ip_prefix = self.split_key(key) - self.add_route_advertisement(vrf, ip_prefix, data) - + '''Only need a name as the key, and community id as the data''' + if not self._set_handler_validate(key, data): + return True + + self._update_rm(key, data) return True def del_handler(self, key): - vrf, ip_prefix = self.split_key(key) - self.remove_route_advertisement(vrf, ip_prefix) - - - def add_route_advertisement(self, vrf, ip_prefix, data): - if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): - if not self.advertised_routes.get(vrf, dict()): - self.bgp_network_import_check_commands(vrf, self.OP_ADD) - self.advertise_route_commands(ip_prefix, vrf, self.OP_ADD, data) - - self.advertised_routes.setdefault(vrf, dict()).update({ip_prefix:data}) - + if not self._del_handler_validate(key): + return + self._remove_rm(key) - def remove_route_advertisement(self, vrf, ip_prefix): - self.advertised_routes.setdefault(vrf, dict()).pop(ip_prefix) - if not self.advertised_routes.get(vrf, dict()): - self.advertised_routes.pop(vrf, None) - if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): - if not self.advertised_routes.get(vrf, dict()): - self.bgp_network_import_check_commands(vrf, self.OP_DELETE) - self.advertise_route_commands(ip_prefix, vrf, self.OP_DELETE) + def _remove_rm(self, rm): + cmds = ['no route-map %s permit 100' % rm] + self.cfg_mgr.push_list(cmds) - def advertise_route_commands(self, ip_prefix, vrf, op, data = None): - is_ipv6 = TemplateFabric.is_ipv6(ip_prefix) - bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] - - cmd_list = [] - if vrf == 'default': - cmd_list.append("router bgp %s" % bgp_asn) - else: - cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) - - cmd_list.append(" address-family %s unicast" % ("ipv6" if is_ipv6 else "ipv4")) - ''' - For set operation, need to check if data is same or not, - need to check if it is ok by overwriting existing value or need to follow no/add sequence - ''' - if data and 'route-map' in data: - cmd_list.append(" network %s route-map %s" % (ip_prefix, data['route-map'])) - else: - cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix)) - - self.cfg_mgr.push_list(cmd_list) - - - def bgp_network_import_check_commands(self, vrf, op): - bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] - cmd_list = [] - if vrf == 'default': - cmd_list.append("router bgp %s" % bgp_asn) - else: - cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) - cmd_list.append(" %sbgp network import-check" % ('' if op == self.OP_DELETE else 'no ')) + def _set_handler_validate(self, key, data): + if key != self.VALID_RM: + return False + + if data: + community_ids = data.split(':') + if len(community_ids) == 2: + if community_ids[0] in range(0, 65535) \ + and community_ids[1] in (0, 65535): + return True - self.cfg_mgr.push_list(cmd_list) + return False - def on_bgp_asn_change(self): - if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): - for vrf, ip_prefixes in self.advertised_routes.items(): - self.bgp_network_import_check_commands(vrf, self.OP_ADD) - for ip_prefix in ip_prefixes: - self.add_route_advertisement(vrf, ip_prefix, ip_prefixes[ip_prefix]) + def _del_handler_validate(self, key): + if key in self.VALID_RM: + return False + return True - @staticmethod - def split_key(key): - """ - Split key into vrf name and prefix. - :param key: key to split - :return: vrf name extracted from the key, ip prefix extracted from the key - """ - if '|' not in key: - return 'default', key - else: - return tuple(key.split('|', 1)) + def _update_rm(self, rm, community): + cmds = [ + 'route-map %s permit 100' % rm, + ' set community %s' % community + ] + self.cfg_mgr.push_list(cmds) From 3e19a2c93608c85f5ba99ff73f74c1c34be07955 Mon Sep 17 00:00:00 2001 From: stormliang Date: Mon, 11 Apr 2022 15:14:55 +0000 Subject: [PATCH 03/13] work on rm --- .../bgpcfgd/managers_advertise_rt.py | 43 +++++++++++-- src/sonic-bgpcfgd/bgpcfgd/managers_rm.py | 29 ++++++--- src/sonic-bgpcfgd/tests/test_advertise_rt.py | 64 ++++++++++++++++++- src/sonic-bgpcfgd/tests/test_rm.py | 61 ++++++++++++++++++ 4 files changed, 181 insertions(+), 16 deletions(-) create mode 100644 src/sonic-bgpcfgd/tests/test_rm.py diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index 5d37163d9848..6de47b3ef491 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -1,7 +1,9 @@ from .manager import Manager from .template import TemplateFabric from swsscommon import swsscommon - +from .managers_rm import ROUTE_MAPS +import ipaddress +from .log import log_debug, log_info, log_err, log_warn class AdvertiseRouteMgr(Manager): """ This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated """ @@ -28,6 +30,8 @@ def __init__(self, common_objs, db, table): def set_handler(self, key, data): + if not self._set_handler_validate(key, data): + return True vrf, ip_prefix = self.split_key(key) self.add_route_advertisement(vrf, ip_prefix, data) @@ -35,9 +39,36 @@ def set_handler(self, key, data): def del_handler(self, key): + if not self._del_handler_validate(key): + return True vrf, ip_prefix = self.split_key(key) self.remove_route_advertisement(vrf, ip_prefix) + def _ip_addr_validate(self, key): + if key: + try: + _, address = self.split_key(key) + ipaddress.ip_address(address) + except ValueError: + log_err("BGPAdvertiseRouteMgr:: No valid ip prefix for advertised route %s" % key) + return False + else: + return False + return True + + + def _set_handler_validate(self, key, data): + if data: + if 'rm_name' not in data or data['rm_name'] not in ROUTE_MAPS: + log_err("BGPAdvertiseRouteMgr:: No valid rm_name for advertised route %s" % data) + return False + + return self._ip_addr_validate(key) + + + def _del_handler_validate(self, key): + return self._ip_addr_validate(key) + def add_route_advertisement(self, vrf, ip_prefix, data): if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): @@ -74,13 +105,17 @@ def advertise_route_commands(self, ip_prefix, vrf, op, data = None): For set operation, need to check if data is same or not, need to check if it is ok by overwriting existing value or need to follow no/add sequence ''' - if data and 'route-map' in data: - cmd_list.append(" network %s route-map %s" % (ip_prefix, data['route-map'])) + if data and 'rm_name' in data: + cmd_list.append(" network %s route-map %s" % (ip_prefix, data['rm_name'])) + log_info("BGPAdvertiseRouteMgr:: update bgp %s network %s with route-map %" % + (bgp_asn, vrf + '|' + ip_prefix, data['rm_name'])) else: cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix)) + log_info("BGPAdvertiseRouteMgr:: %sbgp %s network %s" % + ('Remove ' if op == self.OP_DELETE else 'Update ', bgp_asn, vrf + '|' + ip_prefix)) self.cfg_mgr.push_list(cmd_list) - + log_info("BGPAdvertiseRouteMgr::Done") def bgp_network_import_check_commands(self, vrf, op): bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py index 9e7e9927fc41..e8ca3186f640 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py @@ -1,4 +1,7 @@ from .manager import Manager +from .log import log_info, log_err + +ROUTE_MAPS = ['VXLAN_OV_ECMP_RM'] class RouteMapMgr(Manager): """ This class add route-map when ROUTE_MAP_TABLE in STATE_DB is updated """ @@ -16,7 +19,6 @@ def __init__(self, common_objs, db, table): table, ) - VALID_RM = ['VXLAN_OV_ECMP_RM'] def set_handler(self, key, data): '''Only need a name as the key, and community id as the data''' @@ -35,25 +37,30 @@ def del_handler(self, key): def _remove_rm(self, rm): cmds = ['no route-map %s permit 100' % rm] + log_info("BGPRouteMapMgr:: remove route-map %s" % (rm)) self.cfg_mgr.push_list(cmds) + log_info("BGPRouteMapMgr::Done") def _set_handler_validate(self, key, data): - if key != self.VALID_RM: + if key not in ROUTE_MAPS: + log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key) return False - if data: - community_ids = data.split(':') - if len(community_ids) == 2: - if community_ids[0] in range(0, 65535) \ - and community_ids[1] in (0, 65535): - return True + if not data: + log_err("BGPRouteMapMgr:: data is None") + return False + community_ids = data.split(':') + if len(community_ids) != 2 or community_ids[0] not in range(0, 65535) or community_ids[1] not in (0, 65535): + log_err("BGPRouteMapMgr:: data is not valid community id %s" % data) + return False - return False + return True def _del_handler_validate(self, key): - if key in self.VALID_RM: + if key not in ROUTE_MAPS: + log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key) return False return True @@ -63,4 +70,6 @@ def _update_rm(self, rm, community): 'route-map %s permit 100' % rm, ' set community %s' % community ] + log_info("BGPRouteMapMgr:: update route-map %s community %s" % (rm, community)) self.cfg_mgr.push_list(cmds) + log_info("BGPRouteMapMgr::Done") diff --git a/src/sonic-bgpcfgd/tests/test_advertise_rt.py b/src/sonic-bgpcfgd/tests/test_advertise_rt.py index 26f7b6617650..238a7d2f3dd5 100644 --- a/src/sonic-bgpcfgd/tests/test_advertise_rt.py +++ b/src/sonic-bgpcfgd/tests/test_advertise_rt.py @@ -158,7 +158,9 @@ def test_set_del_bgp_asn_change(): set_del_test( mgr, "SET", - ("vrfRED|10.3.0.0/24", {}), + ("vrfRED|10.3.0.0/24", { + "rm_name": "VXLAN_OV_ECMP_RM" + }), True, [] ) @@ -170,7 +172,7 @@ def test_set_del_bgp_asn_change(): " no bgp network import-check"], ["router bgp 65100 vrf vrfRED", " address-family ipv4 unicast", - " network 10.3.0.0/24"] + " network 10.3.0.0/24 route-map VXLAN_OV_ECMP_RM"] ] def push_list(cmds): test_set_del_bgp_asn_change.push_list_called = True @@ -183,3 +185,61 @@ def push_list(cmds): mgr.directory.put("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost", {"bgp_asn": "65100"}) assert test_set_del_bgp_asn_change.push_list_called + +def test_set_del_with_community(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("10.1.0.0/24", { + "rm_name": "VXLAN_OV_ECMP_RM" + }), + True, + [ + ["router bgp 65100", + " no bgp network import-check"], + ["router bgp 65100", + " address-family ipv4 unicast", + " network 10.1.0.0/24 route-map VXLAN_OV_ECMP_RM"] + ] + ) + + set_del_test( + mgr, + "SET", + ("fc00:10::/64", { + "rm_name": "VXLAN_OV_ECMP_RM" + }), + True, + [ + ["router bgp 65100", + " address-family ipv6 unicast", + " network fc00:10::/64 route-map VXLAN_OV_ECMP_RM"] + ] + ) + + set_del_test( + mgr, + "DEL", + ("10.1.0.0/24",), + True, + [ + ["router bgp 65100", + " address-family ipv4 unicast", + " no network 10.1.0.0/24"] + ] + ) + + set_del_test( + mgr, + "DEL", + ("fc00:10::/64",), + True, + [ + ["router bgp 65100", + " bgp network import-check"], + ["router bgp 65100", + " address-family ipv6 unicast", + " no network fc00:10::/64"] + ] + ) \ No newline at end of file diff --git a/src/sonic-bgpcfgd/tests/test_rm.py b/src/sonic-bgpcfgd/tests/test_rm.py new file mode 100644 index 000000000000..8af9bba07c7a --- /dev/null +++ b/src/sonic-bgpcfgd/tests/test_rm.py @@ -0,0 +1,61 @@ +from unittest.mock import MagicMock + +from bgpcfgd.managers_rm import RouteMapMgr +from swsscommon import swsscommon + +def constructor(): + cfg_mgr = MagicMock() + + common_objs = { + 'cfg_mgr': cfg_mgr, + 'constants': {}, + } + + mgr = RouteMapMgr(common_objs, "STATE_DB", swsscommon.STATE_ROUTE_MAP_TABLE_NAME) + return mgr + +def set_del_test(mgr, op, args, expected_ret, expected_cmds): + set_del_test.push_list_called = False + def push_list(cmds): + set_del_test.push_list_called = True + assert cmds in expected_cmds + return True + mgr.cfg_mgr.push_list = push_list + + if op == "SET": + ret = mgr.set_handler(*args) + assert ret == expected_ret + elif op == "DEL": + mgr.del_handler(*args) + else: + assert False, "Wrong operation" + + if expected_cmds: + assert set_del_test.push_list_called, "cfg_mgr.push_list wasn't called" + else: + assert not set_del_test.push_list_called, "cfg_mgr.push_list was called" + +def test_set_del(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("VXLAN_OV_ECMP_RM", { + "community_id": "1234:1234" + }), + True, + [ + ["route-map VXLAN_OV_ECMP_RM permit 100", + " set community 1234:1234"] + ] + ) + + set_del_test( + mgr, + "DEL", + ("VXLAN_OV_ECMP_RM",), + True, + [ + ["no route-map VXLAN_OV_ECMP_RM permit 100"] + ] + ) From 70e69022356dbd18c78547d5e550c98c0fd0ed6e Mon Sep 17 00:00:00 2001 From: stormliang Date: Wed, 20 Apr 2022 16:28:58 +0000 Subject: [PATCH 04/13] fix ut issues --- .../bgpcfgd/managers_advertise_rt.py | 22 ++++++++++++++----- src/sonic-bgpcfgd/bgpcfgd/managers_rm.py | 15 ++++++++----- src/sonic-bgpcfgd/tests/test_rm.py | 5 +++-- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index 6de47b3ef491..32a449fe0250 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -5,6 +5,7 @@ import ipaddress from .log import log_debug, log_info, log_err, log_warn + class AdvertiseRouteMgr(Manager): """ This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated """ def __init__(self, common_objs, db, table): @@ -30,6 +31,7 @@ def __init__(self, common_objs, db, table): def set_handler(self, key, data): + log_info("AdvertiseRouteMgr:: set handler") if not self._set_handler_validate(key, data): return True vrf, ip_prefix = self.split_key(key) @@ -39,6 +41,7 @@ def set_handler(self, key, data): def del_handler(self, key): + log_info("AdvertiseRouteMgr:: del handler") if not self._del_handler_validate(key): return True vrf, ip_prefix = self.split_key(key) @@ -46,11 +49,21 @@ def del_handler(self, key): def _ip_addr_validate(self, key): if key: + _, ip_prefix = self.split_key(key) + ip_prefix = ip_prefix.split('/') + if len(ip_prefix) != 2: + log_err("BGPAdvertiseRouteMgr:: No valid ip prefix for advertised route %s" % key) + return False try: - _, address = self.split_key(key) - ipaddress.ip_address(address) + ip = ipaddress.ip_address(ip_prefix[0]) + if ip.version == 4 and int(ip_prefix[1]) not in range(0, 33): + log_err("BGPAdvertiseRouteMgr:: ipv4 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key)) + return False + if ip.version == 6 and int(ip_prefix[1]) not in range(0, 129): + log_err("BGPAdvertiseRouteMgr:: ipv6 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key)) + return False except ValueError: - log_err("BGPAdvertiseRouteMgr:: No valid ip prefix for advertised route %s" % key) + log_err("BGPAdvertiseRouteMgr:: No valid ip %s for advertised route %s" % (ip_prefix[0], key)) return False else: return False @@ -93,7 +106,6 @@ def remove_route_advertisement(self, vrf, ip_prefix): def advertise_route_commands(self, ip_prefix, vrf, op, data = None): is_ipv6 = TemplateFabric.is_ipv6(ip_prefix) bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] - cmd_list = [] if vrf == 'default': cmd_list.append("router bgp %s" % bgp_asn) @@ -107,7 +119,7 @@ def advertise_route_commands(self, ip_prefix, vrf, op, data = None): ''' if data and 'rm_name' in data: cmd_list.append(" network %s route-map %s" % (ip_prefix, data['rm_name'])) - log_info("BGPAdvertiseRouteMgr:: update bgp %s network %s with route-map %" % + log_info("BGPAdvertiseRouteMgr:: update bgp %s network %s with route-map %s" % (bgp_asn, vrf + '|' + ip_prefix, data['rm_name'])) else: cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix)) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py index e8ca3186f640..067d3ee3e01b 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py @@ -3,6 +3,7 @@ ROUTE_MAPS = ['VXLAN_OV_ECMP_RM'] + class RouteMapMgr(Manager): """ This class add route-map when ROUTE_MAP_TABLE in STATE_DB is updated """ def __init__(self, common_objs, db, table): @@ -21,6 +22,7 @@ def __init__(self, common_objs, db, table): def set_handler(self, key, data): + log_info("BGPRouteMapMgr:: set handler") '''Only need a name as the key, and community id as the data''' if not self._set_handler_validate(key, data): return True @@ -30,6 +32,7 @@ def set_handler(self, key, data): def del_handler(self, key): + log_info("BGPRouteMapMgr:: del handler") if not self._del_handler_validate(key): return self._remove_rm(key) @@ -50,9 +53,9 @@ def _set_handler_validate(self, key, data): if not data: log_err("BGPRouteMapMgr:: data is None") return False - community_ids = data.split(':') - if len(community_ids) != 2 or community_ids[0] not in range(0, 65535) or community_ids[1] not in (0, 65535): - log_err("BGPRouteMapMgr:: data is not valid community id %s" % data) + community_ids = data['community_id'].split(':') + if len(community_ids) != 2 or int(community_ids[0]) not in range(0, 65536) or int(community_ids[1]) not in range(0, 65536): + log_err("BGPRouteMapMgr:: data %s does not include valid community id %s" % (data, community_ids)) return False return True @@ -65,11 +68,11 @@ def _del_handler_validate(self, key): return True - def _update_rm(self, rm, community): + def _update_rm(self, rm, data): cmds = [ 'route-map %s permit 100' % rm, - ' set community %s' % community + ' set community %s' % data['community_id'] ] - log_info("BGPRouteMapMgr:: update route-map %s community %s" % (rm, community)) + log_info("BGPRouteMapMgr:: update route-map %s community %s" % (rm, data['community_id'])) self.cfg_mgr.push_list(cmds) log_info("BGPRouteMapMgr::Done") diff --git a/src/sonic-bgpcfgd/tests/test_rm.py b/src/sonic-bgpcfgd/tests/test_rm.py index 8af9bba07c7a..6a6ec1fb5a41 100644 --- a/src/sonic-bgpcfgd/tests/test_rm.py +++ b/src/sonic-bgpcfgd/tests/test_rm.py @@ -1,5 +1,5 @@ from unittest.mock import MagicMock - +from bgpcfgd.directory import Directory from bgpcfgd.managers_rm import RouteMapMgr from swsscommon import swsscommon @@ -7,11 +7,12 @@ def constructor(): cfg_mgr = MagicMock() common_objs = { + 'directory': Directory(), 'cfg_mgr': cfg_mgr, 'constants': {}, } - mgr = RouteMapMgr(common_objs, "STATE_DB", swsscommon.STATE_ROUTE_MAP_TABLE_NAME) + mgr = RouteMapMgr(common_objs, "STATE_DB", "STATE_ROUTE_MAP_TABLE")#swsscommon.STATE_ROUTE_MAP_TABLE_NAME) return mgr def set_del_test(mgr, op, args, expected_ret, expected_cmds): From 504d564b55480c9986d1ea6d3eb5f65786110b95 Mon Sep 17 00:00:00 2001 From: stormliang Date: Mon, 25 Apr 2022 03:23:03 +0000 Subject: [PATCH 05/13] address comments --- src/sonic-bgpcfgd/bgpcfgd/main.py | 2 +- .../bgpcfgd/managers_advertise_rt.py | 12 +++++------ src/sonic-bgpcfgd/bgpcfgd/managers_rm.py | 20 +++++++++++-------- src/sonic-bgpcfgd/tests/test_advertise_rt.py | 12 +++++------ src/sonic-bgpcfgd/tests/test_rm.py | 10 +++++----- 5 files changed, 30 insertions(+), 26 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/main.py b/src/sonic-bgpcfgd/bgpcfgd/main.py index 5c06546701cc..d9fb1cf1a885 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/main.py +++ b/src/sonic-bgpcfgd/bgpcfgd/main.py @@ -63,7 +63,7 @@ def do_work(): StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE"), # Route Advertisement Managers AdvertiseRouteMgr(common_objs, "STATE_DB", swsscommon.STATE_ADVERTISE_NETWORK_TABLE_NAME), - RouteMapMgr(common_objs, "STATE_DB", swsscommon.STATE_ROUTE_MAP_TABLE_NAME), + RouteMapMgr(common_objs, "APPL_DB", "BGP_PROFILE_TABLE")#swsscommon.APP_BGP_PROFILE_TABLE_NAME), ] runner = Runner(common_objs['cfg_mgr']) for mgr in managers: diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index 32a449fe0250..1a0fe7878ca0 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -3,7 +3,7 @@ from swsscommon import swsscommon from .managers_rm import ROUTE_MAPS import ipaddress -from .log import log_debug, log_info, log_err, log_warn +from .log import log_info, log_err class AdvertiseRouteMgr(Manager): @@ -72,8 +72,8 @@ def _ip_addr_validate(self, key): def _set_handler_validate(self, key, data): if data: - if 'rm_name' not in data or data['rm_name'] not in ROUTE_MAPS: - log_err("BGPAdvertiseRouteMgr:: No valid rm_name for advertised route %s" % data) + if 'profile' not in data or data['profile'] not in ROUTE_MAPS: + log_err("BGPAdvertiseRouteMgr:: No valid profile for advertised route %s" % data) return False return self._ip_addr_validate(key) @@ -117,10 +117,10 @@ def advertise_route_commands(self, ip_prefix, vrf, op, data = None): For set operation, need to check if data is same or not, need to check if it is ok by overwriting existing value or need to follow no/add sequence ''' - if data and 'rm_name' in data: - cmd_list.append(" network %s route-map %s" % (ip_prefix, data['rm_name'])) + if data and 'profile' in data: + cmd_list.append(" network %s route-map %s" % (ip_prefix, "%s_RM" % data['profile'])) log_info("BGPAdvertiseRouteMgr:: update bgp %s network %s with route-map %s" % - (bgp_asn, vrf + '|' + ip_prefix, data['rm_name'])) + (bgp_asn, vrf + '|' + ip_prefix, "%s_RM" % data['profile'])) else: cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix)) log_info("BGPAdvertiseRouteMgr:: %sbgp %s network %s" % diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py index 067d3ee3e01b..6f1b5ef49b95 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py @@ -1,11 +1,11 @@ from .manager import Manager from .log import log_info, log_err -ROUTE_MAPS = ['VXLAN_OV_ECMP_RM'] +ROUTE_MAPS = ['FROM_SDN_SLB_ROUTES'] class RouteMapMgr(Manager): - """ This class add route-map when ROUTE_MAP_TABLE in STATE_DB is updated """ + """ This class add route-map when ROUTE_MAP_TABLE in APPL_DB is updated """ def __init__(self, common_objs, db, table): """ Initialize the object @@ -39,8 +39,8 @@ def del_handler(self, key): def _remove_rm(self, rm): - cmds = ['no route-map %s permit 100' % rm] - log_info("BGPRouteMapMgr:: remove route-map %s" % (rm)) + cmds = ['no route-map %s permit 100' % ("%s_RM" % rm)] + log_info("BGPRouteMapMgr:: remove route-map %s" % ("%s_RM" % rm)) self.cfg_mgr.push_list(cmds) log_info("BGPRouteMapMgr::Done") @@ -54,8 +54,12 @@ def _set_handler_validate(self, key, data): log_err("BGPRouteMapMgr:: data is None") return False community_ids = data['community_id'].split(':') - if len(community_ids) != 2 or int(community_ids[0]) not in range(0, 65536) or int(community_ids[1]) not in range(0, 65536): - log_err("BGPRouteMapMgr:: data %s does not include valid community id %s" % (data, community_ids)) + try: + if len(community_ids) != 2 or int(community_ids[0]) not in range(0, 65536) or int(community_ids[1]) not in range(0, 65536): + log_err("BGPRouteMapMgr:: data %s doesn't include valid community id %s" % (data, community_ids)) + return False + except ValueError: + log_err("BGPRouteMapMgr:: data %s doesn't include valid community id %s" % (data, community_ids)) return False return True @@ -70,9 +74,9 @@ def _del_handler_validate(self, key): def _update_rm(self, rm, data): cmds = [ - 'route-map %s permit 100' % rm, + 'route-map %s permit 100' % ("%s_RM" % rm), ' set community %s' % data['community_id'] ] - log_info("BGPRouteMapMgr:: update route-map %s community %s" % (rm, data['community_id'])) + log_info("BGPRouteMapMgr:: update route-map %s community %s" % ("%s_RM" % rm, data['community_id'])) self.cfg_mgr.push_list(cmds) log_info("BGPRouteMapMgr::Done") diff --git a/src/sonic-bgpcfgd/tests/test_advertise_rt.py b/src/sonic-bgpcfgd/tests/test_advertise_rt.py index 238a7d2f3dd5..4adce7878c5b 100644 --- a/src/sonic-bgpcfgd/tests/test_advertise_rt.py +++ b/src/sonic-bgpcfgd/tests/test_advertise_rt.py @@ -159,7 +159,7 @@ def test_set_del_bgp_asn_change(): mgr, "SET", ("vrfRED|10.3.0.0/24", { - "rm_name": "VXLAN_OV_ECMP_RM" + "profile": "FROM_SDN_SLB_ROUTES" }), True, [] @@ -172,7 +172,7 @@ def test_set_del_bgp_asn_change(): " no bgp network import-check"], ["router bgp 65100 vrf vrfRED", " address-family ipv4 unicast", - " network 10.3.0.0/24 route-map VXLAN_OV_ECMP_RM"] + " network 10.3.0.0/24 route-map FROM_SDN_SLB_ROUTES_RM"] ] def push_list(cmds): test_set_del_bgp_asn_change.push_list_called = True @@ -192,7 +192,7 @@ def test_set_del_with_community(): mgr, "SET", ("10.1.0.0/24", { - "rm_name": "VXLAN_OV_ECMP_RM" + "profile": "FROM_SDN_SLB_ROUTES" }), True, [ @@ -200,7 +200,7 @@ def test_set_del_with_community(): " no bgp network import-check"], ["router bgp 65100", " address-family ipv4 unicast", - " network 10.1.0.0/24 route-map VXLAN_OV_ECMP_RM"] + " network 10.1.0.0/24 route-map FROM_SDN_SLB_ROUTES_RM"] ] ) @@ -208,13 +208,13 @@ def test_set_del_with_community(): mgr, "SET", ("fc00:10::/64", { - "rm_name": "VXLAN_OV_ECMP_RM" + "profile": "FROM_SDN_SLB_ROUTES" }), True, [ ["router bgp 65100", " address-family ipv6 unicast", - " network fc00:10::/64 route-map VXLAN_OV_ECMP_RM"] + " network fc00:10::/64 route-map FROM_SDN_SLB_ROUTES_RM"] ] ) diff --git a/src/sonic-bgpcfgd/tests/test_rm.py b/src/sonic-bgpcfgd/tests/test_rm.py index 6a6ec1fb5a41..fe89055d27f4 100644 --- a/src/sonic-bgpcfgd/tests/test_rm.py +++ b/src/sonic-bgpcfgd/tests/test_rm.py @@ -12,7 +12,7 @@ def constructor(): 'constants': {}, } - mgr = RouteMapMgr(common_objs, "STATE_DB", "STATE_ROUTE_MAP_TABLE")#swsscommon.STATE_ROUTE_MAP_TABLE_NAME) + mgr = RouteMapMgr(common_objs, "APPL_DB", "BGP_PROFILE_TABLE") return mgr def set_del_test(mgr, op, args, expected_ret, expected_cmds): @@ -41,12 +41,12 @@ def test_set_del(): set_del_test( mgr, "SET", - ("VXLAN_OV_ECMP_RM", { + ("FROM_SDN_SLB_ROUTES", { "community_id": "1234:1234" }), True, [ - ["route-map VXLAN_OV_ECMP_RM permit 100", + ["route-map FROM_SDN_SLB_ROUTES_RM permit 100", " set community 1234:1234"] ] ) @@ -54,9 +54,9 @@ def test_set_del(): set_del_test( mgr, "DEL", - ("VXLAN_OV_ECMP_RM",), + ("FROM_SDN_SLB_ROUTES",), True, [ - ["no route-map VXLAN_OV_ECMP_RM permit 100"] + ["no route-map FROM_SDN_SLB_ROUTES_RM permit 100"] ] ) From 2bd261151da28ef3eac1e300e49f1d4c90a417ce Mon Sep 17 00:00:00 2001 From: stormliang Date: Tue, 26 Apr 2022 10:34:53 +0000 Subject: [PATCH 06/13] refine comments --- src/sonic-bgpcfgd/bgpcfgd/main.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/main.py b/src/sonic-bgpcfgd/bgpcfgd/main.py index d9fb1cf1a885..48f46714d8c0 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/main.py +++ b/src/sonic-bgpcfgd/bgpcfgd/main.py @@ -63,7 +63,8 @@ def do_work(): StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE"), # Route Advertisement Managers AdvertiseRouteMgr(common_objs, "STATE_DB", swsscommon.STATE_ADVERTISE_NETWORK_TABLE_NAME), - RouteMapMgr(common_objs, "APPL_DB", "BGP_PROFILE_TABLE")#swsscommon.APP_BGP_PROFILE_TABLE_NAME), + # TBD, Change to schema name from swsscommon (swsscommon.APP_BGP_PROFILE_TABLE_NAME) when the submodule is advanced. + RouteMapMgr(common_objs, "APPL_DB", "BGP_PROFILE_TABLE"), ] runner = Runner(common_objs['cfg_mgr']) for mgr in managers: From 6956eb73b6d76b64db8ad1e539c742a961fd3418 Mon Sep 17 00:00:00 2001 From: stormliang Date: Tue, 26 Apr 2022 13:37:29 +0000 Subject: [PATCH 07/13] fix issues --- src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py | 14 +++++++------- src/sonic-bgpcfgd/bgpcfgd/managers_rm.py | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index 1a0fe7878ca0..d7d9342526ce 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -93,7 +93,10 @@ def add_route_advertisement(self, vrf, ip_prefix, data): def remove_route_advertisement(self, vrf, ip_prefix): - self.advertised_routes.setdefault(vrf, dict()).pop(ip_prefix) + if ip_prefix not in self.advertised_routes.get(vrf, dict()): + log_info("BGPAdvertiseRouteMgr:: %s|%s does not exist" % (vrf, ip_prefix)) + return + self.advertised_routes.get(vrf, dict()).pop(ip_prefix) if not self.advertised_routes.get(vrf, dict()): self.advertised_routes.pop(vrf, None) @@ -113,13 +116,10 @@ def advertise_route_commands(self, ip_prefix, vrf, op, data = None): cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) cmd_list.append(" address-family %s unicast" % ("ipv6" if is_ipv6 else "ipv4")) - ''' - For set operation, need to check if data is same or not, - need to check if it is ok by overwriting existing value or need to follow no/add sequence - ''' - if data and 'profile' in data: + + if data and 'profile' in data and data['profile'] != '': cmd_list.append(" network %s route-map %s" % (ip_prefix, "%s_RM" % data['profile'])) - log_info("BGPAdvertiseRouteMgr:: update bgp %s network %s with route-map %s" % + log_info("BGPAdvertiseRouteMgr:: Update bgp %s network %s with route-map %s" % (bgp_asn, vrf + '|' + ip_prefix, "%s_RM" % data['profile'])) else: cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix)) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py index 6f1b5ef49b95..714f88be015a 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py @@ -1,7 +1,7 @@ from .manager import Manager from .log import log_info, log_err -ROUTE_MAPS = ['FROM_SDN_SLB_ROUTES'] +ROUTE_MAPS = ['FROM_SDN_SLB_ROUTES', ''] class RouteMapMgr(Manager): From 361053c080f2bbd9eb8d9e5317e1fd76954a7e71 Mon Sep 17 00:00:00 2001 From: stormliang Date: Thu, 28 Apr 2022 11:46:12 +0000 Subject: [PATCH 08/13] to take empty data as valid input --- src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index d7d9342526ce..b0996d9e3fae 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -72,10 +72,12 @@ def _ip_addr_validate(self, key): def _set_handler_validate(self, key, data): if data: - if 'profile' not in data or data['profile'] not in ROUTE_MAPS: + if 'profile' in data and data['profile'] not in ROUTE_MAPS: log_err("BGPAdvertiseRouteMgr:: No valid profile for advertised route %s" % data) return False - + elif data != {'':''}: + log_err("BGPAdvertiseRouteMgr:: Invalid data for advertised route %s" % data) + return False return self._ip_addr_validate(key) From ec8f3f39213aea4c10e3c74c4210557782648ce6 Mon Sep 17 00:00:00 2001 From: stormliang Date: Thu, 28 Apr 2022 12:50:05 +0000 Subject: [PATCH 09/13] refine code and format --- .../bgpcfgd/managers_advertise_rt.py | 84 ++++++++++--------- src/sonic-bgpcfgd/bgpcfgd/managers_rm.py | 36 ++++---- 2 files changed, 62 insertions(+), 58 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index b0996d9e3fae..e9c86d82d323 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -7,7 +7,8 @@ class AdvertiseRouteMgr(Manager): - """ This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated """ + """This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated""" + def __init__(self, common_objs, db, table): """ Initialize the object @@ -21,14 +22,17 @@ def __init__(self, common_objs, db, table): db, table, ) - - self.directory.subscribe([("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], self.on_bgp_asn_change) - self.advertised_routes = dict() + self.directory.subscribe( + [ + ("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"), + ], + self.on_bgp_asn_change, + ) + self.advertised_routes = dict() - OP_DELETE = 'DELETE' - OP_ADD = 'ADD' - + OP_DELETE = "DELETE" + OP_ADD = "ADD" def set_handler(self, key, data): log_info("AdvertiseRouteMgr:: set handler") @@ -39,28 +43,31 @@ def set_handler(self, key, data): return True - def del_handler(self, key): log_info("AdvertiseRouteMgr:: del handler") if not self._del_handler_validate(key): - return True + return vrf, ip_prefix = self.split_key(key) self.remove_route_advertisement(vrf, ip_prefix) def _ip_addr_validate(self, key): if key: _, ip_prefix = self.split_key(key) - ip_prefix = ip_prefix.split('/') + ip_prefix = ip_prefix.split("/") if len(ip_prefix) != 2: log_err("BGPAdvertiseRouteMgr:: No valid ip prefix for advertised route %s" % key) return False try: ip = ipaddress.ip_address(ip_prefix[0]) if ip.version == 4 and int(ip_prefix[1]) not in range(0, 33): - log_err("BGPAdvertiseRouteMgr:: ipv4 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key)) + log_err( + "BGPAdvertiseRouteMgr:: ipv4 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key) + ) return False if ip.version == 6 and int(ip_prefix[1]) not in range(0, 129): - log_err("BGPAdvertiseRouteMgr:: ipv6 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key)) + log_err( + "BGPAdvertiseRouteMgr:: ipv6 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key) + ) return False except ValueError: log_err("BGPAdvertiseRouteMgr:: No valid ip %s for advertised route %s" % (ip_prefix[0], key)) @@ -69,30 +76,26 @@ def _ip_addr_validate(self, key): return False return True - def _set_handler_validate(self, key, data): if data: - if 'profile' in data and data['profile'] not in ROUTE_MAPS: + if "profile" in data and data["profile"] not in ROUTE_MAPS: log_err("BGPAdvertiseRouteMgr:: No valid profile for advertised route %s" % data) return False - elif data != {'':''}: + elif data != {"": ""}: log_err("BGPAdvertiseRouteMgr:: Invalid data for advertised route %s" % data) return False return self._ip_addr_validate(key) - def _del_handler_validate(self, key): return self._ip_addr_validate(key) - def add_route_advertisement(self, vrf, ip_prefix, data): if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): if not self.advertised_routes.get(vrf, dict()): self.bgp_network_import_check_commands(vrf, self.OP_ADD) self.advertise_route_commands(ip_prefix, vrf, self.OP_ADD, data) - self.advertised_routes.setdefault(vrf, dict()).update({ip_prefix:data}) - + self.advertised_routes.setdefault(vrf, dict()).update({ip_prefix: data}) def remove_route_advertisement(self, vrf, ip_prefix): if ip_prefix not in self.advertised_routes.get(vrf, dict()): @@ -107,42 +110,48 @@ def remove_route_advertisement(self, vrf, ip_prefix): self.bgp_network_import_check_commands(vrf, self.OP_DELETE) self.advertise_route_commands(ip_prefix, vrf, self.OP_DELETE) - - def advertise_route_commands(self, ip_prefix, vrf, op, data = None): + def advertise_route_commands(self, ip_prefix, vrf, op, data=None): is_ipv6 = TemplateFabric.is_ipv6(ip_prefix) - bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] + bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"][ + "bgp_asn" + ] cmd_list = [] - if vrf == 'default': + if vrf == "default": cmd_list.append("router bgp %s" % bgp_asn) else: cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) cmd_list.append(" address-family %s unicast" % ("ipv6" if is_ipv6 else "ipv4")) - if data and 'profile' in data and data['profile'] != '': - cmd_list.append(" network %s route-map %s" % (ip_prefix, "%s_RM" % data['profile'])) - log_info("BGPAdvertiseRouteMgr:: Update bgp %s network %s with route-map %s" % - (bgp_asn, vrf + '|' + ip_prefix, "%s_RM" % data['profile'])) + if data and "profile" in data: + cmd_list.append(" network %s route-map %s" % (ip_prefix, "%s_RM" % data["profile"])) + log_info( + "BGPAdvertiseRouteMgr:: Update bgp %s network %s with route-map %s" + % (bgp_asn, vrf + "|" + ip_prefix, "%s_RM" % data["profile"]) + ) else: - cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix)) - log_info("BGPAdvertiseRouteMgr:: %sbgp %s network %s" % - ('Remove ' if op == self.OP_DELETE else 'Update ', bgp_asn, vrf + '|' + ip_prefix)) + cmd_list.append(" %snetwork %s" % ("no " if op == self.OP_DELETE else "", ip_prefix)) + log_info( + "BGPAdvertiseRouteMgr:: %sbgp %s network %s" + % ("Remove " if op == self.OP_DELETE else "Update ", bgp_asn, vrf + "|" + ip_prefix) + ) self.cfg_mgr.push_list(cmd_list) log_info("BGPAdvertiseRouteMgr::Done") def bgp_network_import_check_commands(self, vrf, op): - bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] + bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"][ + "bgp_asn" + ] cmd_list = [] - if vrf == 'default': + if vrf == "default": cmd_list.append("router bgp %s" % bgp_asn) else: cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) - cmd_list.append(" %sbgp network import-check" % ('' if op == self.OP_DELETE else 'no ')) + cmd_list.append(" %sbgp network import-check" % ("" if op == self.OP_DELETE else "no ")) self.cfg_mgr.push_list(cmd_list) - def on_bgp_asn_change(self): if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): for vrf, ip_prefixes in self.advertised_routes.items(): @@ -150,7 +159,6 @@ def on_bgp_asn_change(self): for ip_prefix in ip_prefixes: self.add_route_advertisement(vrf, ip_prefix, ip_prefixes[ip_prefix]) - @staticmethod def split_key(key): """ @@ -158,7 +166,7 @@ def split_key(key): :param key: key to split :return: vrf name extracted from the key, ip prefix extracted from the key """ - if '|' not in key: - return 'default', key + if "|" not in key: + return "default", key else: - return tuple(key.split('|', 1)) + return tuple(key.split("|", 1)) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py index 714f88be015a..0b4c309e682a 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py @@ -1,11 +1,12 @@ from .manager import Manager from .log import log_info, log_err -ROUTE_MAPS = ['FROM_SDN_SLB_ROUTES', ''] +ROUTE_MAPS = ["FROM_SDN_SLB_ROUTES", ""] class RouteMapMgr(Manager): - """ This class add route-map when ROUTE_MAP_TABLE in APPL_DB is updated """ + """This class add route-map when BGP_PROFILE_TABLE in APPL_DB is updated""" + def __init__(self, common_objs, db, table): """ Initialize the object @@ -20,63 +21,58 @@ def __init__(self, common_objs, db, table): table, ) - def set_handler(self, key, data): log_info("BGPRouteMapMgr:: set handler") - '''Only need a name as the key, and community id as the data''' + """Only need a name as the key, and community id as the data""" if not self._set_handler_validate(key, data): return True - + self._update_rm(key, data) return True - def del_handler(self, key): log_info("BGPRouteMapMgr:: del handler") if not self._del_handler_validate(key): return self._remove_rm(key) - def _remove_rm(self, rm): - cmds = ['no route-map %s permit 100' % ("%s_RM" % rm)] + cmds = ["no route-map %s permit 100" % ("%s_RM" % rm)] log_info("BGPRouteMapMgr:: remove route-map %s" % ("%s_RM" % rm)) self.cfg_mgr.push_list(cmds) log_info("BGPRouteMapMgr::Done") - def _set_handler_validate(self, key, data): if key not in ROUTE_MAPS: log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key) return False - + if not data: log_err("BGPRouteMapMgr:: data is None") return False - community_ids = data['community_id'].split(':') + community_ids = data["community_id"].split(":") try: - if len(community_ids) != 2 or int(community_ids[0]) not in range(0, 65536) or int(community_ids[1]) not in range(0, 65536): + if ( + len(community_ids) != 2 + or int(community_ids[0]) not in range(0, 65536) + or int(community_ids[1]) not in range(0, 65536) + ): log_err("BGPRouteMapMgr:: data %s doesn't include valid community id %s" % (data, community_ids)) return False except ValueError: - log_err("BGPRouteMapMgr:: data %s doesn't include valid community id %s" % (data, community_ids)) + log_err("BGPRouteMapMgr:: data %s includes illegal input" % (data)) return False return True - def _del_handler_validate(self, key): if key not in ROUTE_MAPS: log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key) return False return True - def _update_rm(self, rm, data): - cmds = [ - 'route-map %s permit 100' % ("%s_RM" % rm), - ' set community %s' % data['community_id'] - ] - log_info("BGPRouteMapMgr:: update route-map %s community %s" % ("%s_RM" % rm, data['community_id'])) + cmds = ["route-map %s permit 100" % ("%s_RM" % rm), " set community %s" % data["community_id"]] + log_info("BGPRouteMapMgr:: update route-map %s community %s" % ("%s_RM" % rm, data["community_id"])) self.cfg_mgr.push_list(cmds) log_info("BGPRouteMapMgr::Done") From bb2911e5ec99a4723c5b151f0e80c33fda26625e Mon Sep 17 00:00:00 2001 From: stormliang Date: Fri, 29 Apr 2022 12:01:46 +0000 Subject: [PATCH 10/13] refine code --- src/sonic-bgpcfgd/bgpcfgd/managers_rm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py index 0b4c309e682a..b8eb611b4aa2 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py @@ -1,7 +1,7 @@ from .manager import Manager from .log import log_info, log_err -ROUTE_MAPS = ["FROM_SDN_SLB_ROUTES", ""] +ROUTE_MAPS = ["FROM_SDN_SLB_ROUTES"] class RouteMapMgr(Manager): From ec15a2658be58f9b282fae6b07d483c4a54f6bea Mon Sep 17 00:00:00 2001 From: stormliang Date: Fri, 29 Apr 2022 12:30:22 +0000 Subject: [PATCH 11/13] fix ut bug --- src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py | 12 +++++------- src/sonic-bgpcfgd/tests/test_advertise_rt.py | 8 ++++---- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index e9c86d82d323..10c87b171b6f 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -78,13 +78,11 @@ def _ip_addr_validate(self, key): def _set_handler_validate(self, key, data): if data: - if "profile" in data and data["profile"] not in ROUTE_MAPS: - log_err("BGPAdvertiseRouteMgr:: No valid profile for advertised route %s" % data) - return False - elif data != {"": ""}: - log_err("BGPAdvertiseRouteMgr:: Invalid data for advertised route %s" % data) - return False - return self._ip_addr_validate(key) + if ("profile" in data and data["profile"] in ROUTE_MAPS) or data == {"":""}: + return self._ip_addr_validate(key) + + log_err("BGPAdvertiseRouteMgr:: Invalid data %s for advertised route %s" % (key, data)) + return False def _del_handler_validate(self, key): return self._ip_addr_validate(key) diff --git a/src/sonic-bgpcfgd/tests/test_advertise_rt.py b/src/sonic-bgpcfgd/tests/test_advertise_rt.py index 4adce7878c5b..751540600006 100644 --- a/src/sonic-bgpcfgd/tests/test_advertise_rt.py +++ b/src/sonic-bgpcfgd/tests/test_advertise_rt.py @@ -48,7 +48,7 @@ def test_set_del(): set_del_test( mgr, "SET", - ("10.1.0.0/24", {}), + ("10.1.0.0/24", {"":""}), True, [ ["router bgp 65100", @@ -62,7 +62,7 @@ def test_set_del(): set_del_test( mgr, "SET", - ("fc00:10::/64", {}), + ("fc00:10::/64", {"":""}), True, [ ["router bgp 65100", @@ -103,7 +103,7 @@ def test_set_del_vrf(): set_del_test( mgr, "SET", - ("vrfRED|10.2.0.0/24", {}), + ("vrfRED|10.2.0.0/24", {"":""}), True, [ ["router bgp 65100 vrf vrfRED", @@ -117,7 +117,7 @@ def test_set_del_vrf(): set_del_test( mgr, "SET", - ("vrfRED|fc00:20::/64", {}), + ("vrfRED|fc00:20::/64", {"":""}), True, [ ["router bgp 65100 vrf vrfRED", From f7ffa6c3ce085830075e750a0de0715d30c307f2 Mon Sep 17 00:00:00 2001 From: stormliang Date: Fri, 29 Apr 2022 12:50:13 +0000 Subject: [PATCH 12/13] fix bug --- src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index 10c87b171b6f..891b9e786592 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -81,7 +81,7 @@ def _set_handler_validate(self, key, data): if ("profile" in data and data["profile"] in ROUTE_MAPS) or data == {"":""}: return self._ip_addr_validate(key) - log_err("BGPAdvertiseRouteMgr:: Invalid data %s for advertised route %s" % (key, data)) + log_err("BGPAdvertiseRouteMgr:: Invalid data %s for advertised route %s" % (data, key)) return False def _del_handler_validate(self, key): From fb605a1774e7b75e105532670429c4221c33203d Mon Sep 17 00:00:00 2001 From: stormliang Date: Sat, 14 May 2022 04:24:34 +0000 Subject: [PATCH 13/13] address comments --- src/sonic-bgpcfgd/bgpcfgd/main.py | 3 +- .../bgpcfgd/managers_advertise_rt.py | 71 +++++-------------- src/sonic-bgpcfgd/bgpcfgd/managers_rm.py | 30 ++++---- 3 files changed, 34 insertions(+), 70 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/main.py b/src/sonic-bgpcfgd/bgpcfgd/main.py index 48f46714d8c0..7b4291b4d4a3 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/main.py +++ b/src/sonic-bgpcfgd/bgpcfgd/main.py @@ -63,8 +63,7 @@ def do_work(): StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE"), # Route Advertisement Managers AdvertiseRouteMgr(common_objs, "STATE_DB", swsscommon.STATE_ADVERTISE_NETWORK_TABLE_NAME), - # TBD, Change to schema name from swsscommon (swsscommon.APP_BGP_PROFILE_TABLE_NAME) when the submodule is advanced. - RouteMapMgr(common_objs, "APPL_DB", "BGP_PROFILE_TABLE"), + RouteMapMgr(common_objs, "APPL_DB", swsscommon.APP_BGP_PROFILE_TABLE_NAME), ] runner = Runner(common_objs['cfg_mgr']) for mgr in managers: diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index 891b9e786592..68c48b044f61 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -3,11 +3,11 @@ from swsscommon import swsscommon from .managers_rm import ROUTE_MAPS import ipaddress -from .log import log_info, log_err +from .log import log_info, log_err, log_debug class AdvertiseRouteMgr(Manager): - """This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated""" + """ This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated """ def __init__(self, common_objs, db, table): """ @@ -23,20 +23,16 @@ def __init__(self, common_objs, db, table): table, ) - self.directory.subscribe( - [ - ("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"), - ], - self.on_bgp_asn_change, - ) + self.directory.subscribe([("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], self.on_bgp_asn_change) self.advertised_routes = dict() + OP_DELETE = "DELETE" OP_ADD = "ADD" def set_handler(self, key, data): - log_info("AdvertiseRouteMgr:: set handler") - if not self._set_handler_validate(key, data): + log_debug("AdvertiseRouteMgr:: set handler") + if not self.__set_handler_validate(key, data): return True vrf, ip_prefix = self.split_key(key) self.add_route_advertisement(vrf, ip_prefix, data) @@ -44,49 +40,21 @@ def set_handler(self, key, data): return True def del_handler(self, key): - log_info("AdvertiseRouteMgr:: del handler") - if not self._del_handler_validate(key): - return + log_debug("AdvertiseRouteMgr:: del handler") vrf, ip_prefix = self.split_key(key) self.remove_route_advertisement(vrf, ip_prefix) - def _ip_addr_validate(self, key): - if key: - _, ip_prefix = self.split_key(key) - ip_prefix = ip_prefix.split("/") - if len(ip_prefix) != 2: - log_err("BGPAdvertiseRouteMgr:: No valid ip prefix for advertised route %s" % key) - return False - try: - ip = ipaddress.ip_address(ip_prefix[0]) - if ip.version == 4 and int(ip_prefix[1]) not in range(0, 33): - log_err( - "BGPAdvertiseRouteMgr:: ipv4 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key) - ) - return False - if ip.version == 6 and int(ip_prefix[1]) not in range(0, 129): - log_err( - "BGPAdvertiseRouteMgr:: ipv6 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key) - ) - return False - except ValueError: - log_err("BGPAdvertiseRouteMgr:: No valid ip %s for advertised route %s" % (ip_prefix[0], key)) - return False - else: - return False - return True - - def _set_handler_validate(self, key, data): + def __set_handler_validate(self, key, data): if data: if ("profile" in data and data["profile"] in ROUTE_MAPS) or data == {"":""}: - return self._ip_addr_validate(key) + """ + APP which config the data should be responsible to pass a valid IP prefix + """ + return True log_err("BGPAdvertiseRouteMgr:: Invalid data %s for advertised route %s" % (data, key)) return False - def _del_handler_validate(self, key): - return self._ip_addr_validate(key) - def add_route_advertisement(self, vrf, ip_prefix, data): if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): if not self.advertised_routes.get(vrf, dict()): @@ -110,9 +78,8 @@ def remove_route_advertisement(self, vrf, ip_prefix): def advertise_route_commands(self, ip_prefix, vrf, op, data=None): is_ipv6 = TemplateFabric.is_ipv6(ip_prefix) - bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"][ - "bgp_asn" - ] + bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] + cmd_list = [] if vrf == "default": cmd_list.append("router bgp %s" % bgp_asn) @@ -123,24 +90,22 @@ def advertise_route_commands(self, ip_prefix, vrf, op, data=None): if data and "profile" in data: cmd_list.append(" network %s route-map %s" % (ip_prefix, "%s_RM" % data["profile"])) - log_info( + log_debug( "BGPAdvertiseRouteMgr:: Update bgp %s network %s with route-map %s" % (bgp_asn, vrf + "|" + ip_prefix, "%s_RM" % data["profile"]) ) else: cmd_list.append(" %snetwork %s" % ("no " if op == self.OP_DELETE else "", ip_prefix)) - log_info( + log_debug( "BGPAdvertiseRouteMgr:: %sbgp %s network %s" % ("Remove " if op == self.OP_DELETE else "Update ", bgp_asn, vrf + "|" + ip_prefix) ) self.cfg_mgr.push_list(cmd_list) - log_info("BGPAdvertiseRouteMgr::Done") + log_debug("BGPAdvertiseRouteMgr::Done") def bgp_network_import_check_commands(self, vrf, op): - bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"][ - "bgp_asn" - ] + bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] cmd_list = [] if vrf == "default": cmd_list.append("router bgp %s" % bgp_asn) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py index b8eb611b4aa2..08609c68f9a6 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py @@ -1,5 +1,5 @@ from .manager import Manager -from .log import log_info, log_err +from .log import log_err, log_debug ROUTE_MAPS = ["FROM_SDN_SLB_ROUTES"] @@ -22,27 +22,27 @@ def __init__(self, common_objs, db, table): ) def set_handler(self, key, data): - log_info("BGPRouteMapMgr:: set handler") + log_debug("BGPRouteMapMgr:: set handler") """Only need a name as the key, and community id as the data""" - if not self._set_handler_validate(key, data): + if not self.__set_handler_validate(key, data): return True - self._update_rm(key, data) + self.__update_rm(key, data) return True def del_handler(self, key): - log_info("BGPRouteMapMgr:: del handler") - if not self._del_handler_validate(key): + log_debug("BGPRouteMapMgr:: del handler") + if not self.__del_handler_validate(key): return - self._remove_rm(key) + self.__remove_rm(key) - def _remove_rm(self, rm): + def __remove_rm(self, rm): cmds = ["no route-map %s permit 100" % ("%s_RM" % rm)] - log_info("BGPRouteMapMgr:: remove route-map %s" % ("%s_RM" % rm)) + log_debug("BGPRouteMapMgr:: remove route-map %s" % ("%s_RM" % rm)) self.cfg_mgr.push_list(cmds) - log_info("BGPRouteMapMgr::Done") + log_debug("BGPRouteMapMgr::Done") - def _set_handler_validate(self, key, data): + def __set_handler_validate(self, key, data): if key not in ROUTE_MAPS: log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key) return False @@ -65,14 +65,14 @@ def _set_handler_validate(self, key, data): return True - def _del_handler_validate(self, key): + def __del_handler_validate(self, key): if key not in ROUTE_MAPS: log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key) return False return True - def _update_rm(self, rm, data): + def __update_rm(self, rm, data): cmds = ["route-map %s permit 100" % ("%s_RM" % rm), " set community %s" % data["community_id"]] - log_info("BGPRouteMapMgr:: update route-map %s community %s" % ("%s_RM" % rm, data["community_id"])) + log_debug("BGPRouteMapMgr:: update route-map %s community %s" % ("%s_RM" % rm, data["community_id"])) self.cfg_mgr.push_list(cmds) - log_info("BGPRouteMapMgr::Done") + log_debug("BGPRouteMapMgr::Done")