Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[portchannel] Added ACL/PBH binding checks to the port before getting added to portchannel #2151

Merged
merged 9 commits into from
May 18, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions config/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
from utilities_common.intf_filter import parse_interface_in_filter
from utilities_common import bgp_util
import utilities_common.cli as clicommon
from utilities_common.helper import get_port_pbh_binding, get_port_acl_binding
from utilities_common.general import load_db_config, load_module_from_source

from .utils import log
Expand Down Expand Up @@ -1802,14 +1803,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='<portchannel_name>', required=True)
Expand Down Expand Up @@ -1939,6 +1941,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'})

Expand Down
17 changes: 17 additions & 0 deletions dump/match_helper.py
Original file line number Diff line number Diff line change
@@ -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):
Expand Down
4 changes: 4 additions & 0 deletions dump/match_infra.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
"""
Expand Down
4 changes: 4 additions & 0 deletions tests/mock_tables/config_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
22 changes: 22 additions & 0 deletions tests/portchannel_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
4 changes: 2 additions & 2 deletions utilities_common/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -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')
bingwang-ms marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
66 changes: 66 additions & 0 deletions utilities_common/helper.py
Original file line number Diff line number Diff line change
@@ -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