diff --git a/dockers/docker-fpm-frr/frr/bgpd/templates/dynamic/update.conf.j2 b/dockers/docker-fpm-frr/frr/bgpd/templates/dynamic/update.conf.j2 deleted file mode 100644 index 343129dcfaf..00000000000 --- a/dockers/docker-fpm-frr/frr/bgpd/templates/dynamic/update.conf.j2 +++ /dev/null @@ -1,12 +0,0 @@ -! -! template: bgpd/templates/dynamic/update.conf.j2 -! -{% for ip_range in delete_ranges %} - no bgp listen range {{ ip_range }} peer-group {{ bgp_session['name'] }} -{% endfor %} -{% for ip_range in add_ranges %} - bgp listen range {{ ip_range }} peer-group {{ bgp_session['name'] }} -{% endfor %} -! -! end of template: bgpd/templates/dynamic/update.conf.j2 -! diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py index fcaab583f79..3af049d63fb 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py @@ -3,7 +3,6 @@ import jinja2 import netaddr -import os from .log import log_warn, log_err, log_info, log_debug, log_crit from .manager import Manager @@ -106,9 +105,6 @@ def __init__(self, common_objs, db_name, table_name, peer_type, check_neig_meta) "no shutdown": self.fabric.from_string('no neighbor {{ neighbor_addr }} shutdown'), } - if (os.path.exists(self.fabric.env.loader.searchpath[0] + "/" + base_template + "update.conf.j2")): - self.templates["update"] = self.fabric.from_file(base_template + "update.conf.j2") - deps = [ ("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"), ("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/type"), @@ -231,46 +227,10 @@ def add_peer(self, vrf, nbr, data): self.apply_op(cmd, vrf) key = (vrf, nbr) self.peers.add(key) - self.update_state_db(vrf, nbr, data, "SET") log_info("Peer '(%s|%s)' has been scheduled to be added with attributes '%s'" % print_data) return True - def update_state_db(self, vrf, nbr, data, op): - """ - Update the database with the new data - :param vrf: vrf name. Name is equal "default" for the global vrf - :param nbr: neighbor ip address (name for dynamic peer type) - :param data: associated data (will be empty for deletion case) - :param op: operation type. It can be "SET" or "DEL" - :return: True if this adding was successful, False otherwise - """ - if (vrf == "default"): - key = nbr - else: - key = vrf + "|" + nbr - # Update the peer in the STATE_DB table - try: - state_db = swsscommon.DBConnector("STATE_DB", 0) - state_peer_table = swsscommon.Table(state_db, swsscommon.STATE_BGP_PEER_CONFIGURED_TABLE_NAME) - if (op == "SET"): - state_peer_table.set(key, list(sorted(data.items()))) - log_info("Peer '(%s)' has been added to BGP_PEER_CONFIGURED_TABLE with attributes '%s'" % (key, data)) - elif (op == "DEL"): - (status, fvs) = state_peer_table.get(key) - if status == True: - state_peer_table.delete(key) - log_info("Peer '(%s)' has been deleted from BGP_PEER_CONFIGURED_TABLE" % (key)) - else: - log_warn("Peer '(%s)' not found in BGP_PEER_CONFIGURED_TABLE" % (key)) - else: - log_err("Update state DB called for Peer '(%s)' with unkown operation '%s'" % (key, op)) - return False - return True - except Exception as e: - log_err("Update of state db failed for peer '(%s)' with error: %s" % (key, str(e))) - return False - def update_peer(self, vrf, nbr, data): """ Update a peer. This is used when the peer is already in the FRR @@ -282,8 +242,6 @@ def update_peer(self, vrf, nbr, data): """ if "admin_status" in data: self.change_admin_status(vrf, nbr, data) - elif "update" in self.templates and "ip_range" in data and self.peer_type == 'dynamic': - self.change_ip_range(vrf, nbr, data) else: log_err("Peer '(%s|%s)': Can't update the peer. Only 'admin_status' attribute is supported" % (vrf, nbr)) @@ -298,14 +256,14 @@ def change_admin_status(self, vrf, nbr, data): :return: True if this adding was successful, False otherwise """ if data['admin_status'] == 'up': - self.apply_admin_status(vrf, nbr, "no shutdown", "up", data) + self.apply_admin_status(vrf, nbr, "no shutdown", "up") elif data['admin_status'] == 'down': - self.apply_admin_status(vrf, nbr, "shutdown", "down", data) + self.apply_admin_status(vrf, nbr, "shutdown", "down") else: print_data = vrf, nbr, data['admin_status'] log_err("Peer '%s|%s': Can't update the peer. It has wrong attribute value attr['admin_status'] = '%s'" % print_data) - def apply_admin_status(self, vrf, nbr, template_name, admin_state, data): + def apply_admin_status(self, vrf, nbr, template_name, admin_state): """ Render admin state template and apply the command to the FRR :param vrf: vrf name. Name is equal "default" for the global vrf @@ -317,99 +275,10 @@ def apply_admin_status(self, vrf, nbr, template_name, admin_state, data): print_data = vrf, nbr, admin_state ret_code = self.apply_op(self.templates[template_name].render(neighbor_addr=nbr), vrf) if ret_code: - self.update_state_db(vrf, nbr, data, "SET") log_info("Peer '%s|%s' admin state is set to '%s'" % print_data) else: log_err("Can't set peer '%s|%s' admin state to '%s'." % print_data) - def change_ip_range(self, vrf, nbr, data): - """ - Change ip range of a peer - :param vrf: vrf name. Name is equal "default" for the global vrf - :param nbr: neighbor ip address (name for dynamic peer type) - :param data: associated data - :return: True if this adding was successful, False otherwise - """ - if data['ip_range']: - log_info("Peer '(%s|%s)' ip range is going to be updated with range: %s" % (vrf, nbr, data['ip_range'])) - new_ip_range = data["ip_range"].split(",") - ip_ranges_to_add = new_ip_range - ip_ranges_to_del = [] - existing_ipv4_range, existing_ipv6_range = self.get_existing_ip_ranges(vrf, nbr) - if existing_ipv4_range: - for ipv4_range in existing_ipv4_range: - if ipv4_range not in new_ip_range: - ip_ranges_to_del.append(ipv4_range) - else: - ip_ranges_to_add.remove(ipv4_range) - if existing_ipv6_range: - for ipv6_range in existing_ipv6_range: - if ipv6_range not in new_ip_range: - ip_ranges_to_del.append(ipv6_range) - else: - ip_ranges_to_add.remove(ipv6_range) - if ip_ranges_to_del or ip_ranges_to_add: - log_info("Peer '(%s|%s)' ip range is going to be updated. Ranges to delete: %s Ranges to add: %s" % (vrf, nbr, ip_ranges_to_del, ip_ranges_to_add)) - self.apply_range_changes(vrf, nbr, ip_ranges_to_add, ip_ranges_to_del, data) - - def get_existing_ip_ranges(self, vrf, nbr): - """ - Get existing ip range of a peer - :param vrf: vrf name. Name is equal "default" for the global vrf - :param nbr: neighbor ip address (name for dynamic peer type) - :return: existing ipv4 and ipv6 ranges of a peer if they exist. - """ - ipv4_ranges = [] - ipv6_ranges = [] - if vrf == 'default': - command = ["vtysh", "-c", "show bgp peer-group %s json" % (nbr)] - else: - command = ["vtysh", "-c", "show bgp vrf %s peer-group %s json" % (vrf, nbr)] - try: - ret_code, out, err = run_command(command) - if ret_code == 0: - js_bgp = json.loads(out) - if nbr in js_bgp and 'dynamicRanges' in js_bgp[nbr] and 'IPv4' in js_bgp[nbr]['dynamicRanges'] and 'ranges' in js_bgp[nbr]['dynamicRanges']['IPv4']: - ipv4_ranges = js_bgp[nbr]['dynamicRanges']['IPv4']['ranges'] - log_info("Peer '(%s|%s)' already has ipV4 range: %s" % (vrf, nbr, ipv4_ranges)) - if nbr in js_bgp and 'dynamicRanges' in js_bgp[nbr] and 'IPv6' in js_bgp[nbr]['dynamicRanges'] and 'ranges' in js_bgp[nbr]['dynamicRanges']['IPv6']: - ipv6_ranges = js_bgp[nbr]['dynamicRanges']['IPv6']['ranges'] - log_info("Peer '(%s|%s)' already has ipV6 range: %s" % (vrf, nbr, ipv6_ranges)) - return ipv4_ranges, ipv6_ranges - else: - log_err("Can't read ip range of peer '%s|%s': %s" % (vrf, nbr, str(err))) - return ipv4_ranges, ipv6_ranges - except Exception as e: - log_err("Error in parsing ip range: %s" % str(e)) - return ipv4_ranges, ipv6_ranges - - def apply_range_changes(self, vrf, nbr, new_ip_range, ip_ranges_to_del, data): - """ - Apply changes of ip range of a peer - :param vrf: vrf name. Name is equal "default" for the global vrf - :param nbr: neighbor ip address (name for dynamic peer type) - :param new_ip_range: new ip range - :param ip_ranges_to_del: ip ranges to delete - """ - print_data = vrf, nbr, data - kwargs = { - 'CONFIG_DB__DEVICE_METADATA': self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME), - 'vrf': vrf, - 'bgp_session': data, - 'delete_ranges': ip_ranges_to_del, - 'add_ranges': new_ip_range - } - try: - cmd = self.templates["update"].render(**kwargs) - except jinja2.TemplateError as e: - msg = "Peer '(%s|%s)'. Error in rendering the template for 'SET' command '%s'" % print_data - log_err("%s: %s" % (msg, str(e))) - return True - if cmd is not None: - self.apply_op(cmd, vrf) - self.update_state_db(vrf, nbr, data, "SET") - log_info("Peer '(%s|%s)' ip range has been scheduled to be updated with range '%s'" % (vrf, nbr, data['ip_range'])) - def del_handler(self, key): """ 'DEL' handler for the BGP PEER tables @@ -423,7 +292,6 @@ def del_handler(self, key): cmd = self.templates["delete"].render(neighbor_addr=nbr) ret_code = self.apply_op(cmd, vrf) if ret_code: - self.update_state_db(vrf, nbr, {}, "DEL") log_info("Peer '(%s|%s)' has been removed" % (vrf, nbr)) self.peers.remove(peer_key) else: diff --git a/src/sonic-bgpcfgd/tests/test_bgp.py b/src/sonic-bgpcfgd/tests/test_bgp.py index 9c2cd417657..f52c63494a9 100644 --- a/src/sonic-bgpcfgd/tests/test_bgp.py +++ b/src/sonic-bgpcfgd/tests/test_bgp.py @@ -33,9 +33,7 @@ def constructor(constants_path, bgp_router_id="", peer_type="general", with_lo0_ return_value_map = { "['vtysh', '-H', '/dev/null', '-c', 'show bgp vrfs json']": (0, "{\"vrfs\": {\"default\": {}}}", ""), - "['vtysh', '-c', 'show bgp vrf default neighbors json']": (0, "{\"10.10.10.1\": {}, \"20.20.20.1\": {}, \"fc00:10::1\": {}, \"DynNbr1\": {}, \"DynNbr2\": {}}", ""), - "['vtysh', '-c', 'show bgp peer-group DynNbr1 json']": (0, "{\"DynNbr1\":{\"dynamicRanges\":{\"IPv4\":{\"count\":1,\"ranges\":[\"10.255.0.0/24\"]}}}}", ""), - "['vtysh', '-c', 'show bgp peer-group DynNbr2 json']": (0, "{\"DynNbr2\":{\"dynamicRanges\":{\"IPv4\":{\"count\":1,\"ranges\":[\"192.168.0.0/24\",\"192.168.1.0/24\"]}}}}", "") + "['vtysh', '-c', 'show bgp vrf default neighbors json']": (0, "{\"10.10.10.1\": {}, \"20.20.20.1\": {}, \"fc00:10::1\": {}}", "") } bgpcfgd.managers_bgp.run_command = lambda cmd: return_value_map[str(cmd)] @@ -164,66 +162,7 @@ def test_add_peer_ipv6_in_vnet(): for constant in load_constant_files(): m = constructor(constant) res = m.set_handler("Vnet-10|fc00:20::1", {'asn': '65200', 'holdtime': '180', 'keepalive': '60', 'local_addr': 'fc00:20::20', 'name': 'TOR', 'nhopself': '0', 'rrclient': '0'}) - -@patch('bgpcfgd.managers_bgp.log_info') -def test_add_dynamic_peer(mocked_log_info): - for constant in load_constant_files(): - m = constructor(constant, peer_type="dynamic") - m.check_neig_meta = False - res = m.set_handler("BGPSLBPassive", {"peer_asn": "65200", "ip_range": "10.250.0.0/27", "name": "BGPSLBPassive", "src_address": "10.250.0.1"}) - mocked_log_info.assert_called_with("Peer '(default|BGPSLBPassive)' has been scheduled to be added with attributes '{'peer_asn': '65200', 'ip_range': '10.250.0.0/27', 'name': 'BGPSLBPassive', 'src_address': '10.250.0.1'}'") - assert res, "Expect True return value" - -@patch('bgpcfgd.managers_bgp.log_info') -def test_add_dynamic_peer_ipv6(mocked_log_info): - for constant in load_constant_files(): - m = constructor(constant, peer_type="dynamic") - m.check_neig_meta = False - res = m.set_handler("BGPSLBPassive", {"peer_asn": "65200", "ip_range": "fc00:20::/64", "name": "BGPSLBPassive", "src_address": "fc00:20::1"}) - mocked_log_info.assert_called_with("Peer '(default|BGPSLBPassive)' has been scheduled to be added with attributes '{'peer_asn': '65200', 'ip_range': 'fc00:20::/64', 'name': 'BGPSLBPassive', 'src_address': 'fc00:20::1'}'") - assert res, "Expect True return value" - -@patch('bgpcfgd.managers_bgp.log_info') -@patch('bgpcfgd.managers_bgp.swsscommon.Table') -@patch('bgpcfgd.managers_bgp.swsscommon.DBConnector') -def modify_dynamic_peer_common(mock_db_conn, mock_table, mocked_log_info, peer, data, expected_cmds, expected_log): - for constant in load_constant_files(): - m = constructor(constant, peer_type="dynamic") - m.cfg_mgr.push = MagicMock(return_value = None) - m.check_neig_meta = False - swsscommon.STATE_BGP_PEER_CONFIGURED_TABLE_NAME = "BGP_PEER_CONFIGURED_TABLE" - mock_state_db_table = MagicMock() - mock_table.return_value = mock_state_db_table - res = m.set_handler(peer, data) assert res, "Expect True return value" - mock_state_db_table.set.assert_called_once_with(peer, list(sorted(data.items()))) - mocked_log_info.assert_called_with(expected_log) - m.cfg_mgr.push.assert_called_once_with(expected_cmds) - -def test_add_dynamic_peer_range(): - expected_cmds = 'router bgp 65100\n bgp suppress-fib-pending\n!\n! template: bgpd/templates/dynamic/update.conf.j2\n!\n\n\n' \ - ' bgp listen range 10.255.1.0/24 peer-group DynNbr1\n\n!\n! end of template: bgpd/templates/dynamic/update.conf.j2\n!\nexit' - data = {"peer_asn": "65200", "ip_range": "10.255.0.0/24,10.255.1.0/24", "name": "DynNbr1"} - peer = "DynNbr1" - expected_log = "Peer '(default|DynNbr1)' ip range has been scheduled to be updated with range '10.255.0.0/24,10.255.1.0/24'" - modify_dynamic_peer_common(peer=peer, data=data, expected_cmds=expected_cmds, expected_log=expected_log) - -def test_modify_dynamic_peer_range(): - expected_cmds = 'router bgp 65100\n bgp suppress-fib-pending\n!\n! template: bgpd/templates/dynamic/update.conf.j2\n!\n\n' \ - ' no bgp listen range 10.255.0.0/24 peer-group DynNbr1\n\n\n bgp listen range 10.255.0.0/26 peer-group DynNbr1\n\n!\n!' \ - ' end of template: bgpd/templates/dynamic/update.conf.j2\n!\nexit' - data = {"peer_asn": "65200", "ip_range": "10.255.0.0/26", "name": "DynNbr1"} - peer = "DynNbr1" - expected_log = "Peer '(default|DynNbr1)' ip range has been scheduled to be updated with range '10.255.0.0/26'" - modify_dynamic_peer_common(peer=peer, data=data, expected_cmds=expected_cmds, expected_log=expected_log) - -def test_delete_dynamic_peer_range(): - expected_cmds = 'router bgp 65100\n bgp suppress-fib-pending\n!\n! template: bgpd/templates/dynamic/update.conf.j2\n!\n\n' \ - ' no bgp listen range 192.168.1.0/24 peer-group DynNbr2\n\n\n!\n! end of template: bgpd/templates/dynamic/update.conf.j2\n!\nexit' - data = {"peer_asn": "65200", "ip_range": "192.168.0.0/24", "name": "DynNbr2"} - peer = "DynNbr2" - expected_log = "Peer '(default|DynNbr2)' ip range has been scheduled to be updated with range '192.168.0.0/24'" - modify_dynamic_peer_common(peer=peer, data=data, expected_cmds=expected_cmds, expected_log=expected_log) @patch('bgpcfgd.managers_bgp.log_warn') def test_add_peer_no_local_addr(mocked_log_warn):