From b677eff6f9d0ba397dcff59bf42a36aba06a49b2 Mon Sep 17 00:00:00 2001 From: shi-su Date: Mon, 5 Apr 2021 00:26:45 +0000 Subject: [PATCH 1/5] Add bgpcfgd support for static routes --- src/sonic-bgpcfgd/bgpcfgd/main.py | 3 + .../bgpcfgd/manager_static_route.py | 173 ++++++++++ src/sonic-bgpcfgd/tests/test_static_route.py | 307 ++++++++++++++++++ 3 files changed, 483 insertions(+) create mode 100644 src/sonic-bgpcfgd/bgpcfgd/manager_static_route.py create mode 100644 src/sonic-bgpcfgd/tests/test_static_route.py diff --git a/src/sonic-bgpcfgd/bgpcfgd/main.py b/src/sonic-bgpcfgd/bgpcfgd/main.py index 360b54dc11a..0194c249e3b 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/main.py +++ b/src/sonic-bgpcfgd/bgpcfgd/main.py @@ -15,6 +15,7 @@ from .managers_db import BGPDataBaseMgr from .managers_intf import InterfaceMgr from .managers_setsrc import ZebraSetSrc +from .manager_static_route import StaticRouteMgr from .runner import Runner, signal_handler from .template import TemplateFabric from .utils import read_constants @@ -53,6 +54,8 @@ def do_work(): BGPAllowListMgr(common_objs, "CONFIG_DB", "BGP_ALLOWED_PREFIXES"), # BBR Manager BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR"), + # Static Route Managers + StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE"), ] runner = Runner(common_objs['cfg_mgr']) for mgr in managers: diff --git a/src/sonic-bgpcfgd/bgpcfgd/manager_static_route.py b/src/sonic-bgpcfgd/bgpcfgd/manager_static_route.py new file mode 100644 index 00000000000..2437dcb55c9 --- /dev/null +++ b/src/sonic-bgpcfgd/bgpcfgd/manager_static_route.py @@ -0,0 +1,173 @@ +import traceback +from .log import log_crit, log_err, log_debug +from .manager import Manager +from .template import TemplateFabric +import socket + +class StaticRouteMgr(Manager): + """ This class updates static routes when STATIC_ROUTE table 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(StaticRouteMgr, self).__init__( + common_objs, + [], + db, + table, + ) + + self.static_routes = {} + + OP_DELETE = 'DELETE' + OP_ADD = 'ADD' + + def set_handler(self, key, data): + vrf, ip_prefix = self.split_key(key) + is_ipv6 = TemplateFabric.is_ipv6(ip_prefix) + + arg_list = lambda v: v.split(',') if len(v.strip()) != 0 else None + bkh_list = arg_list(data['blackhole']) if 'blackhole' in data else None + nh_list = arg_list(data['nexthop']) if 'nexthop' in data else None + intf_list = arg_list(data['ifname']) if 'ifname' in data else None + dist_list = arg_list(data['distance']) if 'distance' in data else None + nh_vrf_list = arg_list(data['nexthop-vrf']) if 'nexthop-vrf' in data else None + + try: + ip_nh_set = IpNextHopSet(is_ipv6, bkh_list, nh_list, intf_list, dist_list, nh_vrf_list) + cur_nh_set = self.static_routes.get(vrf, {}).get(ip_prefix, IpNextHopSet(is_ipv6)) + cmd_list = self.static_route_commands(ip_nh_set, cur_nh_set, ip_prefix, vrf) + except Exception as exc: + log_crit("Got an exception %s: Traceback: %s" % (str(exc), traceback.format_exc())) + return False + + if cmd_list: + self.cfg_mgr.push_list(cmd_list) + log_debug("Static route {} is scheduled for updates".format(key)) + else: + log_debug("Nothing to update for static route {}".format(key)) + + self.static_routes.setdefault(vrf, {})[ip_prefix] = ip_nh_set + + return True + + + def del_handler(self, key): + vrf, ip_prefix = self.split_key(key) + is_ipv6 = TemplateFabric.is_ipv6(ip_prefix) + + ip_nh_set = IpNextHopSet(is_ipv6) + cur_nh_set = self.static_routes.get(vrf, {}).get(ip_prefix, IpNextHopSet(is_ipv6)) + cmd_list = self.static_route_commands(ip_nh_set, cur_nh_set, ip_prefix, vrf) + + if cmd_list: + self.cfg_mgr.push_list(cmd_list) + log_debug("Static route {} is scheduled for updates".format(key)) + else: + log_debug("Nothing to update for static route {}".format(key)) + + self.static_routes.setdefault(vrf, {}).pop(ip_prefix, None) + + @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 static_route_commands(self, ip_nh_set, cur_nh_set, ip_prefix, vrf): + diff_set = ip_nh_set.symmetric_difference(cur_nh_set) + + op_cmd_list = {} + for ip_nh in diff_set: + if ip_nh in cur_nh_set: + op = self.OP_DELETE + else: + op = self.OP_ADD + + op_cmds = op_cmd_list.setdefault(op, []) + op_cmds.append(self.generate_command(op, ip_nh, ip_prefix, vrf)) + + cmd_list = op_cmd_list.get(self.OP_DELETE, []) + cmd_list += op_cmd_list.get(self.OP_ADD, []) + + return cmd_list + + def generate_command(self, op, ip_nh, ip_prefix, vrf): + return '{}{} route {}{}{}'.format( + 'no ' if op == self.OP_DELETE else '', + 'ipv6' if ip_nh.af == socket.AF_INET6 else 'ip', + ip_prefix, + ip_nh, + ' vrf {}'.format(vrf) if vrf != 'default' else '' + ) + +class IpNextHop: + def __init__(self, af_id, blackhole, dst_ip, if_name, dist, vrf): + zero_ip = lambda af: '0.0.0.0' if af == socket.AF_INET else '::' + self.af = af_id + self.blackhole = 'false' if blackhole is None or blackhole == '' else blackhole + self.distance = 0 if dist is None else int(dist) # TODO check 0 or 1 + if self.blackhole == 'true': + dst_ip = if_name = vrf = None + self.ip = zero_ip(af_id) if dst_ip is None else dst_ip + self.interface = '' if if_name is None else if_name + self.nh_vrf = '' if vrf is None else vrf + if self.blackhole != 'true' and self.is_zero_ip() and len(self.interface.strip()) == 0: + log_err('Mandatory attribute not found for nexthop') + raise ValueError + def __eq__(self, other): + return (self.af == other.af and self.blackhole == other.blackhole and + self.ip == other.ip and self.interface == other.interface and + self.distance == other.distance and self.nh_vrf == other.nh_vrf) + def __ne__(self, other): + return (self.af != other.af or self.blackhole != other.blackhole or + self.ip != other.ip or self.interface != other.interface or + self.distance != other.distance or self.nh_vrf != other.nh_vrf) + def __hash__(self): + return hash((self.af, self.blackhole, self.ip, self.interface, self.distance, self.nh_vrf)) + def is_zero_ip(self): + return sum([x for x in socket.inet_pton(self.af, self.ip)]) == 0 + def __format__(self, format): + ret_val = '' + if self.blackhole == 'true': + ret_val += ' blackhole' + if not (self.ip is None or self.is_zero_ip()): + ret_val += ' %s' % self.ip + if not (self.interface is None or self.interface == ''): + ret_val += ' %s' % self.interface + if not (self.distance is None or self.distance == 0): + ret_val += ' %d' % self.distance + if not (self.nh_vrf is None or self.nh_vrf == ''): + ret_val += ' nexthop-vrf %s' % self.nh_vrf + return ret_val + +class IpNextHopSet(set): + def __init__(self, is_ipv6, bkh_list = None, ip_list = None, intf_list = None, dist_list = None, vrf_list = None): + super(IpNextHopSet, self).__init__() + af = socket.AF_INET6 if is_ipv6 else socket.AF_INET + if bkh_list is None and ip_list is None and intf_list is None: + # empty set, for delete case + return + nums = {len(x) for x in [bkh_list, ip_list, intf_list, dist_list, vrf_list] if x is not None} + if len(nums) != 1: + log_err("Lists of next-hop attribute have different sizes: %s" % nums) + for x in [bkh_list, ip_list, intf_list, dist_list, vrf_list]: + log_debug("List: %s" % x) + raise ValueError + nh_cnt = nums.pop() + item = lambda lst, i: lst[i] if lst is not None else None + for idx in range(nh_cnt): + try: + self.add(IpNextHop(af, item(bkh_list, idx), item(ip_list, idx), item(intf_list, idx), + item(dist_list, idx), item(vrf_list, idx), )) + except ValueError: + continue diff --git a/src/sonic-bgpcfgd/tests/test_static_route.py b/src/sonic-bgpcfgd/tests/test_static_route.py new file mode 100644 index 00000000000..3dc9e052702 --- /dev/null +++ b/src/sonic-bgpcfgd/tests/test_static_route.py @@ -0,0 +1,307 @@ +from unittest.mock import MagicMock, patch + +from bgpcfgd.directory import Directory +from bgpcfgd.template import TemplateFabric +from bgpcfgd.manager_static_route import StaticRouteMgr +from collections import Counter + +def constructor(): + cfg_mgr = MagicMock() + + common_objs = { + 'directory': Directory(), + 'cfg_mgr': cfg_mgr, + 'tf': TemplateFabric(), + 'constants': {}, + } + + mgr = StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE") + assert len(mgr.static_routes) == 0 + + 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 Counter(cmds) == Counter(expected_cmds) # check if commands are expected (regardless of the order) + max_del_idx = -1 + min_set_idx = len(cmds) + for idx in range(len(cmds)): + if cmds[idx].startswith('no') and idx > max_del_idx: + max_del_idx = idx + if not cmds[idx].startswith('no') and idx < min_set_idx: + min_set_idx = idx + assert max_del_idx < min_set_idx, "DEL command comes after SET command" # DEL commands should be done first + 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(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("10.1.0.0/24", { + "nexthop": "10.0.0.57", + }), + True, + [ + "ip route 10.1.0.0/24 10.0.0.57" + ] + ) + +def test_set_nhvrf(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("default|10.1.1.0/24", { + "nexthop": "10.0.0.57", + "ifname": "PortChannel0001", + "distance": "10", + "nexthop-vrf": "nh_vrf", + "blackhole": "false", + }), + True, + [ + "ip route 10.1.1.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf" + ] + ) + +def test_set_blackhole(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("default|10.1.2.0/24", { + "nexthop": "10.0.0.57", + "ifname": "PortChannel0001", + "distance": "10", + "nexthop-vrf": "nh_vrf", + "blackhole": "true", + }), + True, + [ + "ip route 10.1.2.0/24 blackhole 10" + ] + ) + +def test_set_vrf(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57", + "ifname": "PortChannel0001", + "distance": "10", + "nexthop-vrf": "nh_vrf", + "blackhole": "false", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED" + ] + ) + +def test_set_ipv6(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("default|fc00:10::/64", { + "nexthop": "fc00::72", + "ifname": "PortChannel0001", + "distance": "10", + "nexthop-vrf": "", + "blackhole": "false", + }), + True, + [ + "ipv6 route fc00:10::/64 fc00::72 PortChannel0001 10" + ] + ) + +def test_set_del(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61", + "ifname": "PortChannel0001,PortChannel0002,PortChannel0003", + "distance": "10,20,30", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED" + ] + ) + set_del_test( + mgr, + "DEL", + ("vrfRED|10.1.3.0/24",), + True, + [ + "no ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "no ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED", + "no ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED" + ] + ) + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61", + "ifname": "PortChannel0001,PortChannel0002,PortChannel0003", + "distance": "10,20,30", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED" + ] + ) + +def test_set_same_route(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61", + "ifname": "PortChannel0001,PortChannel0002,PortChannel0003", + "distance": "10,20,30", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED" + ] + ) + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61", + "ifname": "PortChannel0001,PortChannel0002,PortChannel0003", + "distance": "40,50,60", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "no ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "no ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED", + "no ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 40 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 50 vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 60 nexthop-vrf default vrf vrfRED" + ] + ) + +@patch('bgpcfgd.manager_static_route.log_debug') +def test_set_no_action(mocked_log_debug): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("default|10.1.1.0/24", { + "nexthop": "10.0.0.57", + "ifname": "PortChannel0001", + "blackhole": "true", + }), + True, + [ + "ip route 10.1.1.0/24 blackhole" + ] + ) + + set_del_test( + mgr, + "SET", + ("default|10.1.1.0/24", { + "nexthop": "10.0.0.59", + "ifname": "PortChannel0002", + "blackhole": "true", + }), + True, + [] + ) + mocked_log_debug.assert_called_with("Nothing to update for static route default|10.1.1.0/24") + +@patch('bgpcfgd.manager_static_route.log_debug') +def test_del_no_action(mocked_log_debug): + mgr = constructor() + set_del_test( + mgr, + "DEL", + ("default|10.1.1.0/24",), + True, + [] + ) + mocked_log_debug.assert_called_with("Nothing to update for static route default|10.1.1.0/24") + +def test_set_invalid_arg(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("default|10.1.1.0/24", { + "nexthop": "10.0.0.57, 10.0.0.59", + "ifname": "PortChannel0001", + }), + False, + [] + ) + +@patch('bgpcfgd.manager_static_route.log_err') +def test_set_invalid_blackhole(mocked_log_err): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("default|10.1.1.0/24", { + "nexthop": "", + "ifname": "", + "blackhole": "false", + }), + True, + [] + ) + mocked_log_err.assert_called_with("Mandatory attribute not found for nexthop") + +def test_set_invalid_ipaddr(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("10.1.0.0/24", { + "nexthop": "invalid_ipaddress", + }), + False, + [] + ) From c31b3630993ed75b75844b42d030451c65dcbc36 Mon Sep 17 00:00:00 2001 From: shi-su Date: Tue, 6 Apr 2021 19:28:02 +0000 Subject: [PATCH 2/5] Address review comments and fix for dst_ip is empty --- src/sonic-bgpcfgd/bgpcfgd/main.py | 2 +- ..._static_route.py => managers_static_rt.py} | 4 +- ...test_static_route.py => test_static_rt.py} | 86 ++++++++++++++++++- 3 files changed, 85 insertions(+), 7 deletions(-) rename src/sonic-bgpcfgd/bgpcfgd/{manager_static_route.py => managers_static_rt.py} (98%) rename src/sonic-bgpcfgd/tests/{test_static_route.py => test_static_rt.py} (76%) diff --git a/src/sonic-bgpcfgd/bgpcfgd/main.py b/src/sonic-bgpcfgd/bgpcfgd/main.py index 0194c249e3b..64d2edbed17 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/main.py +++ b/src/sonic-bgpcfgd/bgpcfgd/main.py @@ -15,7 +15,7 @@ from .managers_db import BGPDataBaseMgr from .managers_intf import InterfaceMgr from .managers_setsrc import ZebraSetSrc -from .manager_static_route import StaticRouteMgr +from .managers_static_rt import StaticRouteMgr from .runner import Runner, signal_handler from .template import TemplateFabric from .utils import read_constants diff --git a/src/sonic-bgpcfgd/bgpcfgd/manager_static_route.py b/src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py similarity index 98% rename from src/sonic-bgpcfgd/bgpcfgd/manager_static_route.py rename to src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py index 2437dcb55c9..f6dd77a520d 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/manager_static_route.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py @@ -115,10 +115,10 @@ def __init__(self, af_id, blackhole, dst_ip, if_name, dist, vrf): zero_ip = lambda af: '0.0.0.0' if af == socket.AF_INET else '::' self.af = af_id self.blackhole = 'false' if blackhole is None or blackhole == '' else blackhole - self.distance = 0 if dist is None else int(dist) # TODO check 0 or 1 + self.distance = 0 if dist is None else int(dist) if self.blackhole == 'true': dst_ip = if_name = vrf = None - self.ip = zero_ip(af_id) if dst_ip is None else dst_ip + self.ip = zero_ip(af_id) if dst_ip is None or dst_ip == '' else dst_ip self.interface = '' if if_name is None else if_name self.nh_vrf = '' if vrf is None else vrf if self.blackhole != 'true' and self.is_zero_ip() and len(self.interface.strip()) == 0: diff --git a/src/sonic-bgpcfgd/tests/test_static_route.py b/src/sonic-bgpcfgd/tests/test_static_rt.py similarity index 76% rename from src/sonic-bgpcfgd/tests/test_static_route.py rename to src/sonic-bgpcfgd/tests/test_static_rt.py index 3dc9e052702..df5ecc0c109 100644 --- a/src/sonic-bgpcfgd/tests/test_static_route.py +++ b/src/sonic-bgpcfgd/tests/test_static_rt.py @@ -2,7 +2,7 @@ from bgpcfgd.directory import Directory from bgpcfgd.template import TemplateFabric -from bgpcfgd.manager_static_route import StaticRouteMgr +from bgpcfgd.managers_static_rt import StaticRouteMgr from collections import Counter def constructor(): @@ -135,6 +135,84 @@ def test_set_ipv6(): ] ) +def test_set_nh_only(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61", + "distance": "10,20,30", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.57 10 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.59 20 vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.61 30 nexthop-vrf default vrf vrfRED" + ] + ) + +def test_set_ifname_only(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "ifname": "PortChannel0001,PortChannel0002,PortChannel0003", + "distance": "10,20,30", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 10.1.3.0/24 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 PortChannel0002 20 vrf vrfRED", + "ip route 10.1.3.0/24 PortChannel0003 30 nexthop-vrf default vrf vrfRED" + ] + ) + +def test_set_with_empty_ifname(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61", + "ifname": "PortChannel0001,,PortChannel0003", + "distance": "10,20,30", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.59 20 vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED" + ] + ) + +def test_set_with_empty_nh(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,,", + "ifname": "PortChannel0001,PortChannel0002,PortChannel0003", + "distance": "10,20,30", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 PortChannel0002 20 vrf vrfRED", + "ip route 10.1.3.0/24 PortChannel0003 30 nexthop-vrf default vrf vrfRED" + ] + ) + def test_set_del(): mgr = constructor() set_del_test( @@ -223,7 +301,7 @@ def test_set_same_route(): ] ) -@patch('bgpcfgd.manager_static_route.log_debug') +@patch('bgpcfgd.managers_static_rt.log_debug') def test_set_no_action(mocked_log_debug): mgr = constructor() set_del_test( @@ -253,7 +331,7 @@ def test_set_no_action(mocked_log_debug): ) mocked_log_debug.assert_called_with("Nothing to update for static route default|10.1.1.0/24") -@patch('bgpcfgd.manager_static_route.log_debug') +@patch('bgpcfgd.managers_static_rt.log_debug') def test_del_no_action(mocked_log_debug): mgr = constructor() set_del_test( @@ -278,7 +356,7 @@ def test_set_invalid_arg(): [] ) -@patch('bgpcfgd.manager_static_route.log_err') +@patch('bgpcfgd.managers_static_rt.log_err') def test_set_invalid_blackhole(mocked_log_err): mgr = constructor() set_del_test( From ce12e83d508fd694b373eca1a4db72a0dac3e7f0 Mon Sep 17 00:00:00 2001 From: shi-su Date: Tue, 6 Apr 2021 21:56:18 +0000 Subject: [PATCH 3/5] remove a space --- src/sonic-bgpcfgd/tests/test_static_rt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-bgpcfgd/tests/test_static_rt.py b/src/sonic-bgpcfgd/tests/test_static_rt.py index df5ecc0c109..d9ed78b434a 100644 --- a/src/sonic-bgpcfgd/tests/test_static_rt.py +++ b/src/sonic-bgpcfgd/tests/test_static_rt.py @@ -349,7 +349,7 @@ def test_set_invalid_arg(): mgr, "SET", ("default|10.1.1.0/24", { - "nexthop": "10.0.0.57, 10.0.0.59", + "nexthop": "10.0.0.57,10.0.0.59", "ifname": "PortChannel0001", }), False, From 2640144d58436868537a162f0d10912a3f5ca994 Mon Sep 17 00:00:00 2001 From: shi-su Date: Tue, 6 Apr 2021 22:23:40 +0000 Subject: [PATCH 4/5] Add test case for adding a nexthop --- src/sonic-bgpcfgd/tests/test_static_rt.py | 35 +++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/sonic-bgpcfgd/tests/test_static_rt.py b/src/sonic-bgpcfgd/tests/test_static_rt.py index d9ed78b434a..239d9271912 100644 --- a/src/sonic-bgpcfgd/tests/test_static_rt.py +++ b/src/sonic-bgpcfgd/tests/test_static_rt.py @@ -301,6 +301,41 @@ def test_set_same_route(): ] ) +def test_set_add_nh(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61", + "ifname": "PortChannel0001,PortChannel0002,PortChannel0003", + "distance": "10,20,30", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED" + ] + ) + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61,10.0.0.63", + "ifname": "PortChannel0001,PortChannel0002,PortChannel0003,PortChannel0004", + "distance": "10,20,30,30", + "nexthop-vrf": "nh_vrf,,default,", + "blackhole": "false,false,false,", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.63 PortChannel0004 30 vrf vrfRED", + ] + ) + @patch('bgpcfgd.managers_static_rt.log_debug') def test_set_no_action(mocked_log_debug): mgr = constructor() From f8e1a1e0afb44a5290f16b2fd63c6e7b44c444c0 Mon Sep 17 00:00:00 2001 From: shi-su Date: Wed, 7 Apr 2021 00:39:01 +0000 Subject: [PATCH 5/5] add test case for del nexthop and ethernet interfaces --- src/sonic-bgpcfgd/tests/test_static_rt.py | 69 ++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/src/sonic-bgpcfgd/tests/test_static_rt.py b/src/sonic-bgpcfgd/tests/test_static_rt.py index 239d9271912..c6a3e40fab4 100644 --- a/src/sonic-bgpcfgd/tests/test_static_rt.py +++ b/src/sonic-bgpcfgd/tests/test_static_rt.py @@ -301,7 +301,7 @@ def test_set_same_route(): ] ) -def test_set_add_nh(): +def test_set_add_del_nh(): mgr = constructor() set_del_test( mgr, @@ -335,6 +335,73 @@ def test_set_add_nh(): "ip route 10.1.3.0/24 10.0.0.63 PortChannel0004 30 vrf vrfRED", ] ) + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59", + "ifname": "PortChannel0001,PortChannel0002", + "distance": "10,20", + "nexthop-vrf": "nh_vrf,", + "blackhole": "false,false", + }), + True, + [ + "no ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED", + "no ip route 10.1.3.0/24 10.0.0.63 PortChannel0004 30 vrf vrfRED", + ] + ) + +def test_set_add_del_nh_ethernet(): + mgr = constructor() + set_del_test( + mgr, + "SET", + ("default|20.1.3.0/24", { + "nexthop": "20.0.0.57,20.0.0.59,20.0.0.61", + "ifname": "Ethernet4,Ethernet8,Ethernet12", + "distance": "10,20,30", + "nexthop-vrf": "default,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 20.1.3.0/24 20.0.0.57 Ethernet4 10 nexthop-vrf default", + "ip route 20.1.3.0/24 20.0.0.59 Ethernet8 20", + "ip route 20.1.3.0/24 20.0.0.61 Ethernet12 30 nexthop-vrf default" + ] + ) + set_del_test( + mgr, + "SET", + ("default|20.1.3.0/24", { + "nexthop": "20.0.0.57,20.0.0.59,20.0.0.61,20.0.0.63", + "ifname": "Ethernet4,Ethernet8,Ethernet12,Ethernet16", + "distance": "10,20,30,30", + "nexthop-vrf": "default,,default,", + "blackhole": "false,false,false,", + }), + True, + [ + "ip route 20.1.3.0/24 20.0.0.63 Ethernet16 30", + ] + ) + set_del_test( + mgr, + "SET", + ("default|20.1.3.0/24", { + "nexthop": "20.0.0.57,20.0.0.59", + "ifname": "Ethernet4,Ethernet8", + "distance": "10,20", + "nexthop-vrf": "default,", + "blackhole": "false,false", + }), + True, + [ + "no ip route 20.1.3.0/24 20.0.0.61 Ethernet12 30 nexthop-vrf default", + "no ip route 20.1.3.0/24 20.0.0.63 Ethernet16 30", + ] + ) @patch('bgpcfgd.managers_static_rt.log_debug') def test_set_no_action(mocked_log_debug):