diff --git a/dockers/docker-fpm-frr/frr/bgpd/templates/general/peer-group.conf.j2 b/dockers/docker-fpm-frr/frr/bgpd/templates/general/peer-group.conf.j2 index 5790d47a5a8a..111b5ceb5f9f 100644 --- a/dockers/docker-fpm-frr/frr/bgpd/templates/general/peer-group.conf.j2 +++ b/dockers/docker-fpm-frr/frr/bgpd/templates/general/peer-group.conf.j2 @@ -9,6 +9,10 @@ {% if CONFIG_DB__DEVICE_METADATA['localhost']['type'] == 'ToRRouter' %} neighbor PEER_V4 allowas-in 1 neighbor PEER_V4_INT allowas-in 1 +{% elif CONFIG_DB__DEVICE_METADATA['localhost']['type'] == 'LeafRouter' %} +{% if CONFIG_DB__BGP_BBR['status'] == 'enabled' %} + neighbor PEER_V4 allowas-in 1 +{% endif %} {% endif %} {% if CONFIG_DB__DEVICE_METADATA['localhost']['sub_role'] == 'BackEnd' %} neighbor PEER_V4_INT route-reflector-client @@ -24,6 +28,10 @@ {% if CONFIG_DB__DEVICE_METADATA['localhost']['type'] == 'ToRRouter' %} neighbor PEER_V6 allowas-in 1 neighbor PEER_V6_INT allowas-in 1 +{% elif CONFIG_DB__DEVICE_METADATA['localhost']['type'] == 'LeafRouter' %} +{% if CONFIG_DB__BGP_BBR['status'] == 'enabled' %} + neighbor PEER_V6 allowas-in 1 +{% endif %} {% endif %} {% if CONFIG_DB__DEVICE_METADATA['localhost']['sub_role'] == 'BackEnd' %} neighbor PEER_V6_INT route-reflector-client diff --git a/files/image_config/constants/constants.yml b/files/image_config/constants/constants.yml index a142bb653df7..d3b70d4a27ec 100644 --- a/files/image_config/constants/constants.yml +++ b/files/image_config/constants/constants.yml @@ -29,10 +29,17 @@ constants: v6: - "deny 0::/0 le 59" - "deny 0::/0 ge 65" + bbr: + enabled: true peers: general: # peer_type db_table: "BGP_NEIGHBOR" template_dir: "general" + bbr: + PEER_V4: + - ipv4 + PEER_V6: + - ipv6 monitors: # peer_type enabled: true db_table: "BGP_MONITORS" diff --git a/src/sonic-bgpcfgd/bgpcfgd/main.py b/src/sonic-bgpcfgd/bgpcfgd/main.py index bef2f733ad57..7bbb6fb768eb 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/main.py +++ b/src/sonic-bgpcfgd/bgpcfgd/main.py @@ -10,6 +10,7 @@ from .directory import Directory from .log import log_notice, log_crit from .managers_allow_list import BGPAllowListMgr +from .managers_bbr import BBRMgr from .managers_bgp import BGPPeerMgrBase from .managers_db import BGPDataBaseMgr from .managers_intf import InterfaceMgr @@ -47,6 +48,8 @@ def do_work(): BGPPeerMgrBase(common_objs, "CONFIG_DB", "BGP_PEER_RANGE", "dynamic", False), # AllowList Managers BGPAllowListMgr(common_objs, "CONFIG_DB", "BGP_ALLOWED_PREFIXES"), + # BBR Manager + BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR"), ] runner = Runner() for mgr in managers: diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py new file mode 100644 index 000000000000..5c03eabf4bbf --- /dev/null +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py @@ -0,0 +1,120 @@ +from swsscommon import swsscommon + +from .log import log_err, log_info, log_crit +from .manager import Manager +from .utils import run_command + + +class BBRMgr(Manager): + """ This class initialize "BBR" feature for """ + 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(BBRMgr, self).__init__( + common_objs, + [("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], + db, + table, + ) + self.enabled = False + self.bbr_enabled_pgs = {} + self.directory.put(self.db_name, self.table_name, 'status', "disabled") + self.__init() + + def set_handler(self, key, data): + """ Implementation of 'SET' command for this class """ + if not self.enabled: + log_info("BBRMgr::BBR is disabled. Drop the request") + return True + if not self.__set_validation(key, data): + return True + cmds = self.__set_prepare_config(data['status']) + rv = self.cfg_mgr.push_list(cmds) + if not rv: + log_crit("BBRMgr::can't apply configuration") + return True + self.__restart_peers() + return True + + def del_handler(self, key): + """ Implementation of 'DEL' command for this class """ + log_err("The '%s' table shouldn't be removed from the db" % self.table_name) + + def __init(self): + """ Initialize BBRMgr. Extracted from constructor """ + if not 'bgp' in self.constants: + log_err("BBRMgr::Disabled: 'bgp' key is not found in constants") + return + if 'bbr' in self.constants['bgp'] and \ + 'enabled' in self.constants['bgp']['bbr'] and \ + self.constants['bgp']['bbr']['enabled']: + self.bbr_enabled_pgs = self.__read_pgs() + if self.bbr_enabled_pgs: + self.enabled = True + self.directory.put(self.db_name, self.table_name, 'status', "enabled") + log_info("BBRMgr::Initialized and enabled") + else: + log_info("BBRMgr::Disabled: no BBR enabled peers") + else: + log_info("BBRMgr::Disabled: not enabled in the constants") + + def __read_pgs(self): + """ + Read peer-group bbr settings from constants file + :return: return bbr information from constant peer-group settings + """ + if 'peers' not in self.constants['bgp']: + log_info("BBRMgr::no 'peers' was found in constants") + return {} + res = {} + for peer_name, value in self.constants['bgp']['peers'].items(): + if 'bbr' not in value: + continue + for pg_name, pg_afs in value['bbr'].items(): + res[pg_name] = pg_afs + return res + + def __set_validation(self, key, data): + """ Validate set-command arguments + :param key: key of 'set' command + :param data: data of 'set' command + :return: True is the parameters are valid, False otherwise + """ + if key != 'all': + log_err("Invalid key '%s' for table '%s'. Only key value 'all' is supported" % (key, self.table_name)) + return False + if 'status' not in data: + log_err("Invalid value '%s' for table '%s', key '%s'. Key 'status' in data is expected" % (data, self.table_name, key)) + return False + if data['status'] != "enabled" and data['status'] != "disabled": + log_err("Invalid value '%s' for table '%s', key '%s'. Only 'enabled' and 'disabled' are supported" % (data, self.table_name, key)) + return False + return True + + def __set_prepare_config(self, status): + """ + Generate FFR configuration to apply changes + :param status: either "enabled" or "disabled" + :return: list of commands prepared for FRR + """ + bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] + cmds = ["router bgp %s" % bgp_asn] + prefix_of_commands = "" if status == "enabled" else "no " + for af in ["ipv4", "ipv6"]: + cmds.append(" address-family %s" % af) + for pg_name in sorted(self.bbr_enabled_pgs.keys()): + if af in self.bbr_enabled_pgs[pg_name]: + cmds.append(" %sneighbor %s allowas-in 1" % (prefix_of_commands, pg_name)) + return cmds + + def __restart_peers(self): + """ Restart peer-groups which support BBR """ + for peer_group in sorted(self.bbr_enabled_pgs.keys()): + rc, out, err = run_command(["vtysh", "-c", "clear bgp peer-group %s soft in" % peer_group]) + if rc != 0: + log_value = peer_group, rc, out, err + log_crit("BBRMgr::Can't restart bgp peer-group '%s'. rc='%d', out='%s', err='%s'" % log_value) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py index fc7e04f7e7b4..b1545dae9288 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py @@ -187,6 +187,7 @@ def add_peer(self, vrf, nbr, data): kwargs = { 'CONFIG_DB__DEVICE_METADATA': self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME), + 'CONFIG_DB__BGP_BBR': self.directory.get_slot('CONFIG_DB', 'BGP_BBR'), 'constants': self.constants, 'bgp_asn': bgp_asn, 'vrf': vrf, diff --git a/src/sonic-bgpcfgd/tests/data/general/peer-group.conf/param_all.json b/src/sonic-bgpcfgd/tests/data/general/peer-group.conf/param_all.json index 293ccc7990dc..0820dd4050f9 100644 --- a/src/sonic-bgpcfgd/tests/data/general/peer-group.conf/param_all.json +++ b/src/sonic-bgpcfgd/tests/data/general/peer-group.conf/param_all.json @@ -3,5 +3,8 @@ "localhost": { "type": "ToRRouter" } - } -} \ No newline at end of file + }, + "CONFIG_DB__BGP_BBR": { + "status": "enabled" + } +} diff --git a/src/sonic-bgpcfgd/tests/data/general/peer-group.conf/param_base.json b/src/sonic-bgpcfgd/tests/data/general/peer-group.conf/param_base.json index 046ffb1a6417..443b5739201c 100644 --- a/src/sonic-bgpcfgd/tests/data/general/peer-group.conf/param_base.json +++ b/src/sonic-bgpcfgd/tests/data/general/peer-group.conf/param_base.json @@ -4,5 +4,8 @@ "type": "LeafRouter", "sub_role": "BackEnd" } + }, + "CONFIG_DB__BGP_BBR": { + "status": "disabled" } -} \ No newline at end of file +} diff --git a/src/sonic-bgpcfgd/tests/swsscommon_test.py b/src/sonic-bgpcfgd/tests/swsscommon_test.py new file mode 100644 index 000000000000..196c9bcb8924 --- /dev/null +++ b/src/sonic-bgpcfgd/tests/swsscommon_test.py @@ -0,0 +1,4 @@ +from mock import MagicMock + + +swsscommon = MagicMock(CFG_DEVICE_METADATA_TABLE_NAME = "DEVICE_METADATA") diff --git a/src/sonic-bgpcfgd/tests/test_bbr.py b/src/sonic-bgpcfgd/tests/test_bbr.py new file mode 100644 index 000000000000..5f95d12acf60 --- /dev/null +++ b/src/sonic-bgpcfgd/tests/test_bbr.py @@ -0,0 +1,319 @@ +from bgpcfgd.directory import Directory +from bgpcfgd.template import TemplateFabric +from mock import MagicMock, patch +from copy import deepcopy +import swsscommon_test +import bgpcfgd + +with patch.dict("sys.modules", swsscommon=swsscommon_test): + from bgpcfgd.managers_bbr import BBRMgr + +global_constants = { + "bgp": { + "allow_list": { + "enabled": True, + "default_pl_rules": { + "v4": [ "deny 0.0.0.0/0 le 17" ], + "v6": [ + "deny 0::/0 le 59", + "deny 0::/0 ge 65" + ] + } + } + } +} + +#@patch('bgpcfgd.managers_bbr.log_info') +#@patch('bgpcfgd.managers_bbr.log_err') +#@patch('bgpcfgd.managers_bbr.log_crit') +def test_constructor():#m1, m2, m3): + cfg_mgr = MagicMock() + common_objs = { + 'directory': Directory(), + 'cfg_mgr': cfg_mgr, + 'tf': TemplateFabric(), + 'constants': {}, + } + m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR") + assert not m.enabled + assert len(m.bbr_enabled_pgs) == 0 + assert m.directory.get("CONFIG_DB", "BGP_BBR", "status") == "disabled" + +@patch('bgpcfgd.managers_bbr.log_info') +@patch('bgpcfgd.managers_bbr.log_crit') +def set_handler_common(key, value, + is_enabled, is_valid, has_no_push_cmd_errors, + mocked_log_crit, mocked_log_info): + cfg_mgr = MagicMock() + common_objs = { + 'directory': Directory(), + 'cfg_mgr': cfg_mgr, + 'tf': TemplateFabric(), + 'constants': global_constants, + } + m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR") + m.enabled = is_enabled + prepare_config_return_value = [ + ["vtysh", "-c", "clear bgp peer-group PEER_V4 soft in"], + ["vtysh", "-c", "clear bgp peer-group PEER_V6 soft in"] + ] + m._BBRMgr__set_prepare_config = MagicMock(return_value = prepare_config_return_value) + m.cfg_mgr.push_list = MagicMock(return_value = has_no_push_cmd_errors) + m._BBRMgr__restart_peers = MagicMock() + res = m.set_handler(key, value) + assert res, "Returns always True" + if not is_enabled: + mocked_log_info.assert_called_with('BBRMgr::BBR is disabled. Drop the request') + else: + if is_valid: + m._BBRMgr__set_prepare_config.assert_called_once_with(value["status"]) + m.cfg_mgr.push_list.assert_called_once_with(prepare_config_return_value) + if has_no_push_cmd_errors: + m._BBRMgr__restart_peers.assert_called_once() + else: + mocked_log_crit.assert_called_with("BBRMgr::can't apply configuration") + m._BBRMgr__restart_peers.assert_not_called() + else: + m._BBRMgr__set_prepare_config.assert_not_called() + m.cfg_mgr.push_list.assert_not_called() + m._BBRMgr__restart_peers.assert_not_called() + +def test_set_handler_1(): + set_handler_common("anything", {}, False, False, True) + +def test_set_handler_2(): + set_handler_common("anything", {}, True, False, True) + +def test_set_handler_3(): + set_handler_common("all", {"status": "enabled"}, True, True, True) + +def test_set_handler_4(): + set_handler_common("all", {"status": "enabled"}, True, True, False) + +@patch('bgpcfgd.managers_bbr.log_err') +def test_del_handler(mocked_log_err): + cfg_mgr = MagicMock() + common_objs = { + 'directory': Directory(), + 'cfg_mgr': cfg_mgr, + 'tf': TemplateFabric(), + 'constants': global_constants, + } + m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR") + m.del_handler("anything") + mocked_log_err.assert_called_with("The 'BGP_BBR' table shouldn't be removed from the db") + +@patch('bgpcfgd.managers_bbr.log_info') +@patch('bgpcfgd.managers_bbr.log_err') +def __init_common(constants, + expected_log_info, expected_log_err, expected_bbr_enabled_pgs, expected_status, + mocked_log_err, mocked_log_info): + cfg_mgr = MagicMock() + common_objs = { + 'directory': Directory(), + 'cfg_mgr': cfg_mgr, + 'tf': TemplateFabric(), + 'constants': constants, + } + m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR") + m._BBRMgr__init() + assert m.bbr_enabled_pgs == expected_bbr_enabled_pgs + assert m.directory.get("CONFIG_DB", "BGP_BBR", "status") == expected_status + if expected_status == "enabled": + assert m.enabled + else: + assert not m.enabled + if expected_log_err is not None: + mocked_log_err.assert_called_with(expected_log_err) + if expected_log_info is not None: + mocked_log_info.assert_called_with(expected_log_info) + +def test___init_1(): + __init_common({}, None, "BBRMgr::Disabled: 'bgp' key is not found in constants", {}, "disabled") + +def test___init_2(): + constants = deepcopy(global_constants) + __init_common(constants, "BBRMgr::Disabled: not enabled in the constants", None, {}, "disabled") + +def test___init_3(): + constants = deepcopy(global_constants) + constants["bgp"]["bbr"] = { "123" : False } + __init_common(constants, "BBRMgr::Disabled: not enabled in the constants", None, {}, "disabled") + +def test___init_4(): + constants = deepcopy(global_constants) + constants["bgp"]["bbr"] = { "enabled" : False } + __init_common(constants, "BBRMgr::Disabled: not enabled in the constants", None, {}, "disabled") + +def test___init_5(): + constants = deepcopy(global_constants) + constants["bgp"]["bbr"] = { "enabled" : True } + __init_common(constants, "BBRMgr::Disabled: no BBR enabled peers", None, {}, "disabled") + +def test___init_6(): + expected_bbr_entries = { + "PEER_V4": ["ipv4"], + "PEER_V6": ["ipv6"], + } + constants = deepcopy(global_constants) + constants["bgp"]["bbr"] = { "enabled" : True } + constants["bgp"]["peers"] = { + "general": { + "bbr": expected_bbr_entries, + } + } + __init_common(constants, 'BBRMgr::Initialized and enabled', None, expected_bbr_entries, "enabled") + +@patch('bgpcfgd.managers_bbr.log_info') +def read_pgs_common(constants, expected_log_info, expected_bbr_enabled_pgs, mocked_log_info): + cfg_mgr = MagicMock() + common_objs = { + 'directory': Directory(), + 'cfg_mgr': cfg_mgr, + 'tf': TemplateFabric(), + 'constants': constants, + } + m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR") + res = m._BBRMgr__read_pgs() + assert res == expected_bbr_enabled_pgs + if expected_log_info is not None: + mocked_log_info.assert_called_with(expected_log_info) + +def test___read_pgs_no_configuration(): + read_pgs_common(global_constants, "BBRMgr::no 'peers' was found in constants", {}) + +def test___read_pgs_parse_configuration(): + expected_bbr_entries = { + "PEER_V4": ["ipv4", "ipv6"], + "PEER_V6": ["ipv6"], + } + constants = deepcopy(global_constants) + constants["bgp"]["peers"] = { + "general": { + "bbr": expected_bbr_entries, + }, + "dynamic": { + "123": { + "PEER_V8": ["ipv10", "ipv20"], + } + } + } + read_pgs_common(constants, None, expected_bbr_entries) + +@patch('bgpcfgd.managers_bbr.log_err') +def __set_validation_common(key, data, expected_log_err, expected_result, mocked_log_err): + cfg_mgr = MagicMock() + common_objs = { + 'directory': Directory(), + 'cfg_mgr': cfg_mgr, + 'tf': TemplateFabric(), + 'constants': global_constants, + } + m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR") + res = m._BBRMgr__set_validation(key, data) + assert res == expected_result + if expected_log_err is not None: + mocked_log_err.assert_called_with(expected_log_err) + +def test___set_validation_1(): + __set_validation_common("all1", {}, "Invalid key 'all1' for table 'BGP_BBR'. Only key value 'all' is supported", False) + +def test___set_validation_2(): + __set_validation_common("all", {"stat": "enabled"}, "Invalid value '{'stat': 'enabled'}' for table 'BGP_BBR', key 'all'. Key 'status' in data is expected", False) + +def test___set_validation_3(): + __set_validation_common("all", {"status": "enabled1"}, "Invalid value '{'status': 'enabled1'}' for table 'BGP_BBR', key 'all'. Only 'enabled' and 'disabled' are supported", False) + +def test___set_validation_4(): + __set_validation_common("all", {"status": "enabled"}, None, True) + +def test___set_validation_5(): + __set_validation_common("all", {"status": "disabled"}, None, True) + +def __set_prepare_config_common(status, bbr_enabled_pgs, expected_cmds): + cfg_mgr = MagicMock() + common_objs = { + 'directory': Directory(), + 'cfg_mgr': cfg_mgr, + 'tf': TemplateFabric(), + 'constants': global_constants, + } + m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR") + m.directory.data = {"CONFIG_DB__DEVICE_METADATA": + { + "localhost": { + "bgp_asn": "65500" + } + } + } + m.bbr_enabled_pgs = bbr_enabled_pgs + cmds = m._BBRMgr__set_prepare_config(status) + assert cmds == expected_cmds + +def test___set_prepare_config_enabled(): + __set_prepare_config_common("enabled", { + "PEER_V4": ["ipv4", "ipv6"], + "PEER_V6": ["ipv6"], + }, [ + 'router bgp 65500', + ' address-family ipv4', + ' neighbor PEER_V4 allowas-in 1', + ' address-family ipv6', + ' neighbor PEER_V4 allowas-in 1', + ' neighbor PEER_V6 allowas-in 1', + ]) + +def test___set_prepare_config_disabled(): + __set_prepare_config_common("disabled", { + "PEER_V4": ["ipv4", "ipv6"], + "PEER_V6": ["ipv6"], + }, [ + 'router bgp 65500', + ' address-family ipv4', + ' no neighbor PEER_V4 allowas-in 1', + ' address-family ipv6', + ' no neighbor PEER_V4 allowas-in 1', + ' no neighbor PEER_V6 allowas-in 1', + ]) + +@patch('bgpcfgd.managers_bbr.log_crit') +def __restart_peers_common(run_command_results, run_command_expects, last_log_crit_message, mocked_log_crit): + cfg_mgr = MagicMock() + common_objs = { + 'directory': Directory(), + 'cfg_mgr': cfg_mgr, + 'tf': TemplateFabric(), + 'constants': global_constants, + } + m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR") + m.bbr_enabled_pgs = { + "PEER_V4": ["ipv4", "ipv6"], + "PEER_V6": ["ipv6"], + } + def run_command_mock(cmd): + assert cmd == run_command_expects[run_command_mock.run] + res = run_command_results[run_command_mock.run] + run_command_mock.run += 1 + return res + run_command_mock.run = 0 + bgpcfgd.managers_bbr.run_command = run_command_mock + #lambda cmd: (0, "", "") + m._BBRMgr__restart_peers() + if last_log_crit_message is not None: + mocked_log_crit.assert_called_with(last_log_crit_message) + +def test___restart_peers_1(): + __restart_peers_common([(0, "", ""), (0, "", "")], + [ + ["vtysh", "-c", "clear bgp peer-group PEER_V4 soft in"], + ["vtysh", "-c", "clear bgp peer-group PEER_V6 soft in"] + ], + None) + +def test___restart_peers_2(): + __restart_peers_common([(1, "out1", "err1"), (0, "", "")], + [ + ["vtysh", "-c", "clear bgp peer-group PEER_V4 soft in"], + ["vtysh", "-c", "clear bgp peer-group PEER_V6 soft in"] + ], + "BBRMgr::Can't restart bgp peer-group 'PEER_V4'. rc='1', out='out1', err='err1'")