From 85595c8e2e0223a96a779d69632c9552f4bc4a40 Mon Sep 17 00:00:00 2001 From: Vivek R Date: Thu, 26 May 2022 09:08:39 -0700 Subject: [PATCH] [202111] [portchannel] Added ACL/PBH binding checks to the port before getting added to portchannel (#2186) - What I did Backport #2151 to 202111 The change to handle it in oA is already added. When this check is not performed when adding the config, the portchannel configuration will be inconsistent b/w Kernel and ASIC - How I did it Utilize the match infra to implement methods to check for ACL/PBH bindings to a port - How to verify it Unit tests Signed-off-by: Vivek Reddy Karri --- config/main.py | 22 ++++++++++- dump/match_helper.py | 17 ++++++++ dump/match_infra.py | 4 ++ tests/mock_tables/config_db.json | 4 ++ tests/portchannel_test.py | 22 +++++++++++ utilities_common/db.py | 8 ++-- utilities_common/helper.py | 66 ++++++++++++++++++++++++++++++++ 7 files changed, 137 insertions(+), 6 deletions(-) create mode 100644 utilities_common/helper.py diff --git a/config/main.py b/config/main.py index 03b4c155d2..03eca117cf 100644 --- a/config/main.py +++ b/config/main.py @@ -28,6 +28,7 @@ from utilities_common import bgp_util import utilities_common.cli as clicommon from utilities_common.general import load_db_config +from utilities_common.helper import get_port_pbh_binding, get_port_acl_binding from .utils import log @@ -1733,14 +1734,15 @@ def synchronous_mode(sync_mode): @click.option('-n', '--namespace', help='Namespace name', required=True if multi_asic.is_multi_asic() else False, type=click.Choice(multi_asic.get_namespace_list())) @click.pass_context -def portchannel(ctx, namespace): +@clicommon.pass_db +def portchannel(db, ctx, namespace): # Set namespace to default_namespace if it is None. if namespace is None: namespace = DEFAULT_NAMESPACE config_db = ConfigDBConnector(use_unix_socket_path=True, namespace=str(namespace)) config_db.connect() - ctx.obj = {'db': config_db, 'namespace': str(namespace)} + ctx.obj = {'db': config_db, 'namespace': str(namespace), 'db_wrap': db} @portchannel.command('add') @click.argument('portchannel_name', metavar='', required=True) @@ -1870,6 +1872,22 @@ def add_portchannel_member(ctx, portchannel_name, port_name): if port_tpid != DEFAULT_TPID: ctx.fail("Port TPID of {}: {} is not at default 0x8100".format(port_name, port_tpid)) + # Don't allow a port to be a member of portchannel if already has ACL bindings + try: + acl_bindings = get_port_acl_binding(ctx.obj['db_wrap'], port_name, ctx.obj['namespace']) + if acl_bindings: + ctx.fail("Port {} is already bound to following ACL_TABLES: {}".format(port_name, acl_bindings)) + except Exception as e: + ctx.fail(str(e)) + + # Don't allow a port to be a member of portchannel if already has PBH bindings + try: + pbh_bindings = get_port_pbh_binding(ctx.obj['db_wrap'], port_name, DEFAULT_NAMESPACE) + if pbh_bindings: + ctx.fail("Port {} is already bound to following PBH_TABLES: {}".format(port_name, pbh_bindings)) + except Exception as e: + ctx.fail(str(e)) + db.set_entry('PORTCHANNEL_MEMBER', (portchannel_name, port_name), {'NULL': 'NULL'}) diff --git a/dump/match_helper.py b/dump/match_helper.py index 9493a83458..aa0d7f9b19 100644 --- a/dump/match_helper.py +++ b/dump/match_helper.py @@ -1,6 +1,23 @@ from dump.match_infra import MatchRequest from dump.helper import handle_multiple_keys_matched_error +# Return dict helper methods + +def check_error(ret): + """ Check if the match request failed """ + if ret["error"]: + return True, ret["error"] + else: + return False, "" + +def get_matched_keys(ret): + """ Return Matched Keys """ + failed, err_str = check_error(ret) + if not failed: + return ret["keys"], "" + else: + return [], err_str + # Port Helper Methods def fetch_port_oid(match_engine, port_name, ns): diff --git a/dump/match_infra.py b/dump/match_infra.py index c74fdc7c1e..a28c7f9bf8 100644 --- a/dump/match_infra.py +++ b/dump/match_infra.py @@ -250,6 +250,10 @@ def clear(self, namespace=None): elif namespace in self.cache: del self.cache[namespace] + def fill(self, ns, conn, connected_to): + """ Update internal cache """ + self.cache[ns] = {'conn': conn, 'connected_to': set(connected_to)} + class MatchEngine: """ diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index ca40ee45a8..546624f48c 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -532,6 +532,10 @@ "services@": "SSH", "type": "CTRLPLANE" }, + "PBH_TABLE|pbh_table1": { + "description": "NVGRE", + "interface_list@": "Ethernet8,Ethernet60" + }, "VLAN|Vlan1000": { "dhcp_servers@": "192.0.0.1,192.0.0.2,192.0.0.3,192.0.0.4", "vlanid": "1000" diff --git a/tests/portchannel_test.py b/tests/portchannel_test.py index ebf77e86b5..11e0e89e52 100644 --- a/tests/portchannel_test.py +++ b/tests/portchannel_test.py @@ -157,6 +157,28 @@ def test_delete_portchannel_member_which_is_member_of_another_po(self): assert result.exit_code != 0 assert "Error: Ethernet116 is not a member of portchannel PortChannel1001" in result.output + def test_add_portchannel_member_with_acl_bindngs(self): + runner = CliRunner() + db = Db() + obj = {'db':db.cfgdb, 'db_wrap':db, 'namespace':''} + + result = runner.invoke(config.config.commands["portchannel"].commands["member"].commands["add"], ["PortChannel0002", "Ethernet100"], obj=obj) + print(result.exit_code) + print(result.output) + assert result.exit_code != 0 + assert "Error: Port Ethernet100 is already bound to following ACL_TABLES:" in result.output + + def test_add_portchannel_member_with_pbh_bindngs(self): + runner = CliRunner() + db = Db() + obj = {'db':db.cfgdb, 'db_wrap':db, 'namespace':''} + + result = runner.invoke(config.config.commands["portchannel"].commands["member"].commands["add"], ["PortChannel0002", "Ethernet60"], obj=obj) + print(result.exit_code) + print(result.output) + assert result.exit_code != 0 + assert "Error: Port Ethernet60 is already bound to following PBH_TABLES:" in result.output + @classmethod def teardown_class(cls): os.environ['UTILITIES_UNIT_TESTING'] = "0" diff --git a/utilities_common/db.py b/utilities_common/db.py index 136e1fc91e..091a821ff1 100644 --- a/utilities_common/db.py +++ b/utilities_common/db.py @@ -15,15 +15,15 @@ def __init__(self): self.db = SonicV2Connector(host="127.0.0.1") # Skip connecting to chassis databases in line cards - db_list = list(self.db.get_db_list()) + self.db_list = list(self.db.get_db_list()) if not device_info.is_supervisor(): try: - db_list.remove('CHASSIS_APP_DB') - db_list.remove('CHASSIS_STATE_DB') + self.db_list.remove('CHASSIS_APP_DB') + self.db_list.remove('CHASSIS_STATE_DB') except Exception: pass - for db_id in db_list: + for db_id in self.db_list: self.db.connect(db_id) self.cfgdb_clients[constants.DEFAULT_NAMESPACE] = self.cfgdb diff --git a/utilities_common/helper.py b/utilities_common/helper.py new file mode 100644 index 0000000000..f7a71cec36 --- /dev/null +++ b/utilities_common/helper.py @@ -0,0 +1,66 @@ +from dump.match_infra import MatchEngine, MatchRequest, ConnectionPool +from dump.match_helper import get_matched_keys +from .db import Db + +def get_port_acl_binding(db_wrap, port, ns): + """ + Verify if the port is not bound to any ACL Table + + Args: + db_wrap: utilities_common.Db() object + port: Iface name + ns: namespace + + Returns: + list: ACL_TABLE names if found, + otherwise empty + """ + ACL = "ACL_TABLE" # Table to look for port bindings + if not isinstance(db_wrap, Db): + raise Exception("db_wrap object is not of type utilities_common.Db") + + conn_pool = ConnectionPool() + conn_pool.fill(ns, db_wrap.db_clients[ns], db_wrap.db_list) + m_engine = MatchEngine(conn_pool) + req = MatchRequest(db="CONFIG_DB", + table=ACL, + key_pattern="*", + field="ports@", + value=port, + ns=ns, + match_entire_list=False) + ret = m_engine.fetch(req) + acl_tables, _ = get_matched_keys(ret) + return acl_tables + + +def get_port_pbh_binding(db_wrap, port, ns): + """ + Verify if the port is not bound to any PBH Table + + Args: + db_wrap: Db() object + port: Iface name + ns: namespace + + Returns: + list: PBH_TABLE names if found, + otherwise empty + """ + PBH = "PBH_TABLE" # Table to look for port bindings + if not isinstance(db_wrap, Db): + raise Exception("db_wrap object is not of type utilities_common.Db") + + conn_pool = ConnectionPool() + conn_pool.fill(ns, db_wrap.db_clients[ns], db_wrap.db_list) + m_engine = MatchEngine(conn_pool) + req = MatchRequest(db="CONFIG_DB", + table=PBH, + key_pattern="*", + field="interface_list@", + value=port, + ns=ns, + match_entire_list=False) + ret = m_engine.fetch(req) + pbh_tables, _ = get_matched_keys(ret) + return pbh_tables