From 580c7a562c4a01e371b6779cd4ca89e13ab62560 Mon Sep 17 00:00:00 2001 From: ArthiGovindaraj Date: Thu, 24 Nov 2022 04:07:06 -0600 Subject: [PATCH 1/4] sonic-utilities : support for addind/deleting rules in openconfig format using acl-loader * Support for adding rules using json file in dataplane acl using "config acl add rule * Support for deleting rule using table_name rule_name using "config acl delete rule " --- acl_loader/main.py | 55 ++++++++++++++++++++++++++++++++++++++++++++++ config/main.py | 32 +++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/acl_loader/main.py b/acl_loader/main.py index c50efec032..ae5df7a356 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -766,6 +766,50 @@ def incremental_update(self): for namespace_configdb in self.per_npu_configdb.values(): namespace_configdb.set_entry(self.ACL_RULE, key, self.rules_info[key]) + def add_rule(self): + """ + Perform update/addition of new rules only. Get existing rules from + Config DB. Compare with rules specified in file and perform corresponding + modifications. Do not delete the existing rules. Handles only dataplane rules + :return: + """ + + # TODO: Alternate command for adding rules in dataplane ACLs instead of removing + # all the rules and populating like in full/incremental. + new_rules = set(self.rules_info.keys()) + new_dataplane_rules = set() + current_rules = set(self.rules_db_info.keys()) + current_dataplane_rules = set() + + for key in new_rules: + table_name = key[0] + if self.tables_db_info[table_name]['type'].upper() != self.ACL_TABLE_TYPE_CTRLPLANE: + new_dataplane_rules.add(key) + + for key in current_rules: + table_name = key[0] + if self.tables_db_info[table_name]['type'].upper() != self.ACL_TABLE_TYPE_CTRLPLANE: + current_dataplane_rules.add(key) + + added_dataplane_rules = new_dataplane_rules.difference(current_dataplane_rules) + removed_dataplane_rules = current_dataplane_rules.difference(new_dataplane_rules) + existing_dataplane_rules = new_rules.intersection(current_dataplane_rules) + + for key in added_dataplane_rules: + self.configdb.mod_entry(self.ACL_RULE, key, self.rules_info[key]) + # Program for per-asic namespace corresponding to front asic also if present. + # For control plane ACL it's not needed but to keep all db in sync program everywhere + for namespace_configdb in self.per_npu_configdb.values(): + namespace_configdb.mod_entry(self.ACL_RULE, key, self.rules_info[key]) + + for key in existing_dataplane_rules: + if not operator.eq(self.rules_info[key], self.rules_db_info[key]): + self.configdb.set_entry(self.ACL_RULE, key, self.rules_info[key]) + # Program for per-asic namespace corresponding to front asic also if present. + # For control plane ACL it's not needed but to keep all db in sync program everywhere + for namespace_configdb in self.per_npu_configdb.values(): + namespace_configdb.set_entry(self.ACL_RULE, key, self.rules_info[key]) + def delete(self, table=None, rule=None): """ :param table: @@ -1068,6 +1112,17 @@ def incremental(ctx, filename, session_name, mirror_stage, max_priority): acl_loader.load_rules_from_file(filename) acl_loader.incremental_update() +@cli.command() +@click.argument('filename', type=click.Path(exists=True)) +@click.pass_context +def add(ctx, filename): + """ + Add ACL rules. + """ + acl_loader = ctx.obj["acl_loader"] + + acl_loader.load_rules_from_file(filename) + acl_loader.add_rule() @cli.command() @click.argument('table', required=False) diff --git a/config/main.py b/config/main.py index 004b40bdb4..bc80f3bfd5 100644 --- a/config/main.py +++ b/config/main.py @@ -5579,6 +5579,20 @@ def table(ctx, table_name, table_type, description, ports, stage): config_db.set_entry("ACL_TABLE", table_name, table_info) +# +# 'add' subcommand +# + +@add.command() +@click.argument('file_name', required=True) +def rule(file_name): + """ + Add ACL rule + """ + log.log_info("'config acl add rule {}' executing...".format(file_name)) + command = "acl-loader add {}".format(file_name) + clicommon.run_command(command) + # # 'remove' subgroup ('config acl remove ...') # @@ -5600,10 +5614,28 @@ def table(table_name): """ Remove ACL table """ + log.log_info("'config acl remove table {}' executing...".format(table_name)) + command = "acl-loader delete {}".format(table_name) + clicommon.run_command(command) + config_db = ConfigDBConnector() config_db.connect() config_db.set_entry("ACL_TABLE", table_name, None) +# +# 'delete' subcommand +# + +@remove.command() +@click.argument("table_name", required=True, metavar="") +@click.argument("rule_name", required=True, metavar="") +def rule(table_name, rule_name): + """ + Remove ACL rule + """ + log.log_info("'config acl remove rule {} {}' executing...".format(table_name, rule_name)) + command = "acl-loader delete {} {}".format(table_name, rule_name) + clicommon.run_command(command) # # 'acl update' group From b160a7f0b279fda83a4a632a27b68be9be4454f6 Mon Sep 17 00:00:00 2001 From: ArthiGovindaraj Date: Thu, 24 Nov 2022 05:08:48 -0600 Subject: [PATCH 2/4] Removed unused variable --- acl_loader/main.py | 1 - 1 file changed, 1 deletion(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index ae5df7a356..41b30e116b 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -792,7 +792,6 @@ def add_rule(self): current_dataplane_rules.add(key) added_dataplane_rules = new_dataplane_rules.difference(current_dataplane_rules) - removed_dataplane_rules = current_dataplane_rules.difference(new_dataplane_rules) existing_dataplane_rules = new_rules.intersection(current_dataplane_rules) for key in added_dataplane_rules: From d3793fee81e73ffeff367417207ae1cc36a665a3 Mon Sep 17 00:00:00 2001 From: ArthiGovindaraj Date: Tue, 29 Nov 2022 07:11:23 -0600 Subject: [PATCH 3/4] Updated unit test cases --- tests/acl_input/acl_add_1.json | 47 +++++++++++++++++++++++++ tests/acl_input/acl_add_2.json | 46 ++++++++++++++++++++++++ tests/acl_loader_test.py | 64 ++++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+) create mode 100644 tests/acl_input/acl_add_1.json create mode 100644 tests/acl_input/acl_add_2.json diff --git a/tests/acl_input/acl_add_1.json b/tests/acl_input/acl_add_1.json new file mode 100644 index 0000000000..a2b1096f4b --- /dev/null +++ b/tests/acl_input/acl_add_1.json @@ -0,0 +1,47 @@ +{ + "acl": { + "acl-sets": { + "acl-set": { + "DATAACL_ADD_DEL": { + "acl-entries": { + "acl-entry": { + "1": { + "config": { + "sequence-id": 1 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "ip": { + "config": { + "source-ip-address": "20.0.0.2/32", + "destination-ip-address": "30.0.0.3/32" + } + } + }, + "2": { + "config": { + "sequence-id": 2 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "ip": { + "config": { + "source-ip-address": "21.0.0.2/32", + "destination-ip-address": "31.0.0.3/32" + } + } + } + + } + } + } + } + } + } +} diff --git a/tests/acl_input/acl_add_2.json b/tests/acl_input/acl_add_2.json new file mode 100644 index 0000000000..1c175b6e65 --- /dev/null +++ b/tests/acl_input/acl_add_2.json @@ -0,0 +1,46 @@ +{ + "acl": { + "acl-sets": { + "acl-set": { + "DATAACL_ADD_DEL": { + "acl-entries": { + "acl-entry": { + "1": { + "config": { + "sequence-id": 1 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "ip": { + "config": { + "source-ip-address": "30.0.0.2/32", + "destination-ip-address": "40.0.0.3/32" + } + } + }, + "3": { + "config": { + "sequence-id": 3 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "ip": { + "config": { + "source-ip-address": "31.0.0.2/32", + "destination-ip-address": "41.0.0.3/32" + } + } + } + } + } + } + } + } + } +} diff --git a/tests/acl_loader_test.py b/tests/acl_loader_test.py index 20b7283319..23ed2f4dce 100644 --- a/tests/acl_loader_test.py +++ b/tests/acl_loader_test.py @@ -2,6 +2,8 @@ import os import pytest from unittest import mock +from sonic_py_common import multi_asic +from swsssdk import ConfigDBConnector test_path = os.path.dirname(os.path.abspath(__file__)) modules_path = os.path.dirname(test_path) @@ -217,3 +219,65 @@ def test_incremental_update(self, acl_loader): acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/incremental_2.json')) acl_loader.incremental_update() assert acl_loader.rules_info[(('NTP_ACL', 'RULE_1'))]["PACKET_ACTION"] == "DROP" + + def test_add_rule(self, acl_loader): + acl_loader.rules_info = {} + acl_loader.tables_db_info['DATAACL_ADD_DEL'] = { + "policy_desc": "DATAACL_ADD_DEL", + "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023", + "stage": "INGRESS", + "type": "L3" + } + acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/acl_add_1.json')) + assert acl_loader.rules_info[('DATAACL_ADD_DEL', 'RULE_1')] + assert acl_loader.rules_info[('DATAACL_ADD_DEL', 'RULE_1')] == { + 'SRC_IP': '20.0.0.2/32', + 'DST_IP': '30.0.0.3/32', + 'ETHER_TYPE': '2048', + 'PACKET_ACTION': 'FORWARD', + 'PRIORITY': '9999' + } + assert acl_loader.rules_info[('DATAACL_ADD_DEL', 'RULE_2')] + assert acl_loader.rules_info[('DATAACL_ADD_DEL', 'RULE_2')] == { + 'SRC_IP': '21.0.0.2/32', + 'DST_IP': '31.0.0.3/32', + 'ETHER_TYPE': '2048', + 'PACKET_ACTION': 'FORWARD', + 'PRIORITY': '9998' + } + + acl_loader.configdb.mod_entry = mock.MagicMock(return_value=True) + acl_loader.configdb.set_entry = mock.MagicMock(return_value=True) + acl_loader.per_npu_configdb = {} + namespaces = multi_asic.get_front_end_namespaces() + for namespace in namespaces: + asic_id = multi_asic.get_asic_index_from_namespace(namespace) + # replace these with correct macros + acl_loader.per_npu_configdb[asic_id] = ConfigDBConnector(use_unix_socket_path=True, namespace=namespace) + acl_loader.per_npu_configdb[asic_id].connect() + + acl_loader.rules_db_info = acl_loader.rules_info + + acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/acl_add_2.json')) + acl_loader.add_rule() + print(acl_loader.rules_info) + assert acl_loader.rules_info[('DATAACL_ADD_DEL', 'RULE_1')] + assert acl_loader.rules_info[('DATAACL_ADD_DEL', 'RULE_2')] + assert acl_loader.rules_info[("DATAACL_ADD_DEL", "RULE_1")] == { + "SRC_IP": "30.0.0.2/32", + "DST_IP": "40.0.0.3/32", + "ETHER_TYPE": "2048", + "PACKET_ACTION": "FORWARD", + "PRIORITY": "9999" + } + assert acl_loader.rules_info[('DATAACL_ADD_DEL', 'RULE_1')] + assert acl_loader.rules_info[('DATAACL_ADD_DEL', 'RULE_2')] + assert acl_loader.rules_info[('DATAACL_ADD_DEL', 'RULE_3')] + assert acl_loader.rules_info[('DATAACL_ADD_DEL', 'RULE_3')] == { + 'SRC_IP': '31.0.0.2/32', + 'DST_IP': '41.0.0.3/32', + 'ETHER_TYPE': '2048', + 'PACKET_ACTION': 'FORWARD', + 'PRIORITY': '9997' + } + From f2819f0534f3f19d4849d20de5ea45cc489a24b9 Mon Sep 17 00:00:00 2001 From: ArthiGovindaraj Date: Wed, 30 Nov 2022 02:04:47 -0600 Subject: [PATCH 4/4] Coverage test cases updated --- tests/acl_config_test.py | 34 ++++++++++++++++++++++++++++++++++ tests/acl_loader_test.py | 2 -- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/tests/acl_config_test.py b/tests/acl_config_test.py index ff397e760d..a1e378a9be 100644 --- a/tests/acl_config_test.py +++ b/tests/acl_config_test.py @@ -1,6 +1,8 @@ import pytest import config.main as config +from unittest import mock + from click.testing import CliRunner from config.main import expand_vlan_ports, parse_acl_table_info @@ -79,3 +81,35 @@ def test_acl_add_table_empty_vlan(self): assert result.exit_code != 0 assert "Cannot bind empty VLAN Vlan3000" in result.output + + def test_acl_add_del_rule(self): + runner = CliRunner() + + result = runner.invoke( + config.config.commands["acl"].commands["add"].commands["table"], + ["DATAACL_ADD_DEL", "L3", "-p", "Ethernet0"]) + + assert result.exit_code == 0 + + with mock.patch('utilities_common.cli.run_command') as mock_run_command: + result = runner.invoke( + config.config.commands["acl"].commands["add"].commands["rule"], + ["tests/acl_input/acl_add_1.json"]) + + assert result.exit_code == 0 + + with mock.patch('utilities_common.cli.run_command') as mock_run_command: + result = runner.invoke( + config.config.commands["acl"].commands["remove"].commands["rule"], + ["DATAACL_ADD_DEL", "RULE_1"]) + + assert result.exit_code == 0 + + with mock.patch('utilities_common.cli.run_command') as mock_run_command: + result = runner.invoke( + config.config.commands["acl"].commands["remove"].commands["table"], + ["DATAACL_ADD_DEL"]) + + assert result.exit_code == 0 + + diff --git a/tests/acl_loader_test.py b/tests/acl_loader_test.py index 23ed2f4dce..02552b9df3 100644 --- a/tests/acl_loader_test.py +++ b/tests/acl_loader_test.py @@ -246,8 +246,6 @@ def test_add_rule(self, acl_loader): 'PRIORITY': '9998' } - acl_loader.configdb.mod_entry = mock.MagicMock(return_value=True) - acl_loader.configdb.set_entry = mock.MagicMock(return_value=True) acl_loader.per_npu_configdb = {} namespaces = multi_asic.get_front_end_namespaces() for namespace in namespaces: