[acl] Expand VLAN into VLAN members when creating an ACL table (#1475)

Signed-off-by: Danny Allen <daall@microsoft.com>

config/main.py

@@ -3291,37 +3291,85 @@ def get_acl_bound_ports():
 
     return list(ports)
 
-#
-# 'table' subcommand ('config acl add table ...')
-#
 
-@add.command()
-@click.argument("table_name", metavar="<table_name>")
-@click.argument("table_type", metavar="<table_type>")
-@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:
         table_info["policy_desc"] = description
     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="<table_name>")
+@click.argument("table_type", metavar="<table_type>")
+@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)
 
 #

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] <table_name> <table_type> [-d <description>] [-p <ports>] [-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
 

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

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"
     },

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