From 64604dbf20f0d1fe0b3063b105a88447de606005 Mon Sep 17 00:00:00 2001 From: Danny Allen Date: Wed, 3 Mar 2021 17:24:22 -0800 Subject: [PATCH] [acl] Expand VLAN into VLAN members when creating an ACL table (#1475) Signed-off-by: Danny Allen --- config/main.py | 74 +++++++++++++++++++++++----- doc/Command-Reference.md | 29 +++++++++++ tests/acl_config_test.py | 83 ++++++++++++++++++++++++++++++++ tests/mock_tables/config_db.json | 3 ++ tests/vlan_test.py | 14 ++++++ 5 files changed, 190 insertions(+), 13 deletions(-) create mode 100644 tests/acl_config_test.py diff --git a/config/main.py b/config/main.py index b8fbaab6e2b2..f2a204b93ffc 100644 --- a/config/main.py +++ b/config/main.py @@ -3291,23 +3291,33 @@ def get_acl_bound_ports(): return list(ports) -# -# 'table' subcommand ('config acl add table ...') -# -@add.command() -@click.argument("table_name", metavar="") -@click.argument("table_type", metavar="") -@click.option("-d", "--description") -@click.option("-p", "--ports") -@click.option("-s", "--stage", type=click.Choice(["ingress", "egress"]), default="ingress") -def table(table_name, table_type, description, ports, stage): +def expand_vlan_ports(port_name): """ - Add ACL table + Expands a given VLAN interface into its member ports. + + If the provided interface is a VLAN, then this method will return its member ports. + + If the provided interface is not a VLAN, then this method will return a list with only + the provided interface in it. """ config_db = ConfigDBConnector() config_db.connect() + if port_name not in config_db.get_keys("VLAN"): + return [port_name] + + vlan_members = config_db.get_keys("VLAN_MEMBER") + + members = [member for vlan, member in vlan_members if port_name == vlan] + + if not members: + raise ValueError("Cannot bind empty VLAN {}".format(port_name)) + + return members + + +def parse_acl_table_info(table_name, table_type, description, ports, stage): table_info = {"type": table_type} if description: @@ -3315,13 +3325,51 @@ def table(table_name, table_type, description, ports, stage): else: table_info["policy_desc"] = table_name + if not ports and ports != None: + raise ValueError("Cannot bind empty list of ports") + + port_list = [] + valid_acl_ports = get_acl_bound_ports() if ports: - table_info["ports@"] = ports + for port in ports.split(","): + port_list += expand_vlan_ports(port) + port_list = set(port_list) else: - table_info["ports@"] = ",".join(get_acl_bound_ports()) + port_list = valid_acl_ports + + for port in port_list: + if port not in valid_acl_ports: + raise ValueError("Cannot bind ACL to specified port {}".format(port)) + + table_info["ports@"] = ",".join(port_list) table_info["stage"] = stage + return table_info + +# +# 'table' subcommand ('config acl add table ...') +# + +@add.command() +@click.argument("table_name", metavar="") +@click.argument("table_type", metavar="") +@click.option("-d", "--description") +@click.option("-p", "--ports") +@click.option("-s", "--stage", type=click.Choice(["ingress", "egress"]), default="ingress") +@click.pass_context +def table(ctx, table_name, table_type, description, ports, stage): + """ + Add ACL table + """ + config_db = ConfigDBConnector() + config_db.connect() + + try: + table_info = parse_acl_table_info(table_name, table_type, description, ports, stage) + except ValueError as e: + ctx.fail("Failed to parse ACL table config: exception={}".format(e)) + config_db.set_entry("ACL_TABLE", table_name, table_info) # diff --git a/doc/Command-Reference.md b/doc/Command-Reference.md index 08237329bd84..172e103273f6 100644 --- a/doc/Command-Reference.md +++ b/doc/Command-Reference.md @@ -1371,6 +1371,35 @@ When the optional argument "max_priority" is specified, each rule’s priority Go Back To [Beginning of the document](#) or [Beginning of this section](#acl) +**config acl add table** + +This command is used to create new ACL tables. + +- Usage: + ``` + config acl add table [OPTIONS] [-d ] [-p ] [-s (ingress | egress)] + ``` + +- Parameters: + - table_name: The name of the ACL table to create. + - table_type: The type of ACL table to create (e.g. "L3", "L3V6", "MIRROR") + - description: A description of the table for the user. (default is the table_name) + - ports: A comma-separated list of ports/interfaces to add to the table. The behavior is as follows: + - Physical ports will be bound as physical ports + - Portchannels will be bound as portchannels - passing a portchannel member is invalid + - VLANs will be expanded into their members (e.g. "Vlan1000" will become "Ethernet0,Ethernet2,Ethernet4...") + - stage: The stage this ACL table will be applied to, either ingress or egress. (default is ingress) + +- Examples: + ``` + admin@sonic:~$ sudo config acl add table EXAMPLE L3 -p Ethernet0,Ethernet4 -s ingress + ``` + ``` + admin@sonic:~$ sudo config acl add table EXAMPLE_2 L3V6 -p Vlan1000,PortChannel0001,Ethernet128 -s egress + ``` + +Go Back To [Beginning of the document](#) or [Beginning of this section](#acl) + ## ARP & NDP diff --git a/tests/acl_config_test.py b/tests/acl_config_test.py new file mode 100644 index 000000000000..63f92b787be1 --- /dev/null +++ b/tests/acl_config_test.py @@ -0,0 +1,83 @@ +import pytest + +import config.main as config + +from click.testing import CliRunner +from config.main import expand_vlan_ports, parse_acl_table_info + + +class TestConfigAcl(object): + def test_expand_vlan(self): + assert set(expand_vlan_ports("Vlan1000")) == {"Ethernet4", "Ethernet8", "Ethernet12", "Ethernet16"} + + def test_expand_lag(self): + assert set(expand_vlan_ports("PortChannel1001")) == {"PortChannel1001"} + + def test_expand_physical_interface(self): + assert set(expand_vlan_ports("Ethernet4")) == {"Ethernet4"} + + def test_expand_empty_vlan(self): + with pytest.raises(ValueError): + expand_vlan_ports("Vlan3000") + + def test_parse_table_with_vlan_expansion(self): + table_info = parse_acl_table_info("TEST", "L3", None, "Vlan1000", "egress") + assert table_info["type"] == "L3" + assert table_info["policy_desc"] == "TEST" + assert table_info["stage"] == "egress" + + port_list = table_info["ports@"].split(",") + assert set(port_list) == {"Ethernet4", "Ethernet8", "Ethernet12", "Ethernet16"} + + def test_parse_table_with_vlan_and_duplicates(self): + table_info = parse_acl_table_info("TEST", "L3", None, "Ethernet4,Vlan1000", "egress") + assert table_info["type"] == "L3" + assert table_info["policy_desc"] == "TEST" + assert table_info["stage"] == "egress" + + # Since Ethernet4 is a member of Vlan1000 we should not include it twice in the output + port_list = table_info["ports@"].split(",") + assert len(port_list) == 4 + assert set(port_list) == {"Ethernet4", "Ethernet8", "Ethernet12", "Ethernet16"} + + def test_parse_table_with_empty_vlan(self): + with pytest.raises(ValueError): + parse_acl_table_info("TEST", "L3", None, "Ethernet4,Vlan3000", "egress") + + def test_parse_table_with_invalid_ports(self): + with pytest.raises(ValueError): + parse_acl_table_info("TEST", "L3", None, "Ethernet200", "egress") + + def test_parse_table_with_empty_ports(self): + with pytest.raises(ValueError): + parse_acl_table_info("TEST", "L3", None, "", "egress") + + def test_acl_add_table_nonexistent_port(self): + runner = CliRunner() + + result = runner.invoke( + config.config.commands["acl"].commands["add"].commands["table"], + ["TEST", "L3", "-p", "Ethernet200"]) + + assert result.exit_code != 0 + assert "Cannot bind ACL to specified port Ethernet200" in result.output + + def test_acl_add_table_empty_string_port_list(self): + runner = CliRunner() + + result = runner.invoke( + config.config.commands["acl"].commands["add"].commands["table"], + ["TEST", "L3", "-p", ""]) + + assert result.exit_code != 0 + assert "Cannot bind empty list of ports" in result.output + + def test_acl_add_table_empty_vlan(self): + runner = CliRunner() + + result = runner.invoke( + config.config.commands["acl"].commands["add"].commands["table"], + ["TEST", "L3", "-p", "Vlan3000"]) + + assert result.exit_code != 0 + assert "Cannot bind empty VLAN Vlan3000" in result.output diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index f9a7459680bc..a8545a5a80de 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -434,6 +434,9 @@ "dhcp_servers@": "192.0.0.1,192.0.0.2,192.0.0.3,192.0.0.4", "vlanid": "2000" }, + "VLAN|Vlan3000": { + "vlanid": "3000" + }, "VLAN_INTERFACE|Vlan1000": { "NULL": "NULL" }, diff --git a/tests/vlan_test.py b/tests/vlan_test.py index f17abac43113..8be9db1e2e0f 100644 --- a/tests/vlan_test.py +++ b/tests/vlan_test.py @@ -22,6 +22,8 @@ | | | | | 192.0.0.3 | | | | | | | 192.0.0.4 | | +-----------+-----------------+------------+----------------+-----------------------+-------------+ +| 3000 | | | | | disabled | ++-----------+-----------------+------------+----------------+-----------------------+-------------+ """ show_vlan_brief_in_alias_mode_output="""\ @@ -38,6 +40,8 @@ | | | | | 192.0.0.3 | | | | | | | 192.0.0.4 | | +-----------+-----------------+---------+----------------+-----------------------+-------------+ +| 3000 | | | | | disabled | ++-----------+-----------------+---------+----------------+-----------------------+-------------+ """ show_vlan_brief_empty_output="""\ @@ -49,6 +53,8 @@ | | | | | 192.0.0.3 | | | | | | | 192.0.0.4 | | +-----------+-----------------+------------+----------------+-----------------------+-------------+ +| 3000 | | | | | disabled | ++-----------+-----------------+------------+----------------+-----------------------+-------------+ """ show_vlan_brief_with_portchannel_output="""\ @@ -66,6 +72,8 @@ | | | | | 192.0.0.3 | | | | | | | 192.0.0.4 | | +-----------+-----------------+-----------------+----------------+-----------------------+-------------+ +| 3000 | | | | | disabled | ++-----------+-----------------+-----------------+----------------+-----------------------+-------------+ """ show_vlan_config_output="""\ @@ -115,6 +123,8 @@ | | | | | 192.0.0.3 | | | | | | | 192.0.0.4 | | +-----------+-----------------+------------+----------------+-----------------------+-------------+ +| 3000 | | | | | disabled | ++-----------+-----------------+------------+----------------+-----------------------+-------------+ """ config_add_del_vlan_and_vlan_member_output="""\ @@ -133,6 +143,8 @@ | | | | | 192.0.0.3 | | | | | | | 192.0.0.4 | | +-----------+-----------------+------------+----------------+-----------------------+-------------+ +| 3000 | | | | | disabled | ++-----------+-----------------+------------+----------------+-----------------------+-------------+ """ config_add_del_vlan_and_vlan_member_in_alias_mode_output="""\ @@ -151,6 +163,8 @@ | | | | | 192.0.0.3 | | | | | | | 192.0.0.4 | | +-----------+-----------------+---------+----------------+-----------------------+-------------+ +| 3000 | | | | | disabled | ++-----------+-----------------+---------+----------------+-----------------------+-------------+ """ class TestVlan(object): @classmethod