From cd85b697a37637870ad1f4841eec77f5b307e4ba Mon Sep 17 00:00:00 2001 From: bingwang Date: Tue, 15 Mar 2022 03:58:20 -0700 Subject: [PATCH 01/13] Update acl-loader to support dynamic ACL Signed-off-by: bingwang --- acl_loader/main.py | 171 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 162 insertions(+), 9 deletions(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index ada7162154..7c39cf326f 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -12,6 +12,8 @@ from sonic_py_common import multi_asic from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector from utilities_common.general import load_db_config +import time + def info(msg): click.echo(click.style("Info: ", fg='cyan') + click.style(str(msg), fg='green')) @@ -81,6 +83,9 @@ class AclLoader(object): ACL_STAGE_CAPABILITY_TABLE = "ACL_STAGE_CAPABILITY_TABLE" ACL_ACTIONS_CAPABILITY_FIELD = "action_list" ACL_ACTION_CAPABILITY_FIELD = "ACL_ACTION" + DEFAULT_RULE = "DEFAULT_RULE" + + STATE_DB_ACL_TTL_TABLE = "ACL_TTL_TABLE" min_priority = 1 max_priority = 10000 @@ -116,6 +121,7 @@ def __init__(self): self.tables_db_info = {} self.rules_db_info = {} self.rules_info = {} + self.rules_ttl = {} # Load database config files load_db_config() @@ -340,15 +346,16 @@ def parse_acl_json(filename): raise AclLoaderException("Invalid input file %s" % filename) return yang_acl - def load_rules_from_file(self, filename): + def load_rules_from_file(self, filename, is_dynamic=False): """ Load file with ACL rules configuration in openconfig ACL format. Convert rules to Config DB schema. :param filename: File in openconfig ACL format + :param is_dynamic: A bool, indicates whether the rule is dynamic or not :return: """ self.yang_acl = AclLoader.parse_acl_json(filename) - self.convert_rules() + self.convert_rules(is_dynamic) def convert_action(self, table_name, rule_idx, rule): rule_props = {} @@ -583,6 +590,62 @@ def convert_input_interface(self, table_name, rule_idx, rule): return rule_props + def is_db_rule_dynamic(self, key): + try: + data = self.configdb.get_entry(self.ACL_RULE, key) + if 'is_dynamic' in data and data['is_dynamic'].lower() == 'true': + return True + return False + except: + return False + + def is_dynamic_rule(self, rule): + return rule.dynamic_acl.config.ttl + + def convert_dynamic_acl_config(self, key, rule): + rule_props = {} + # The ttl value is not written into ACL_RULE table as we don't want + # update notificaton when TTL is changed + if self.is_dynamic_rule(rule): + rule_props['is_dynamic'] = 'True' + self.rules_ttl[key] = int(rule.dynamic_acl.config.ttl) + + return rule_props + + def add_state_db_ttl_entry(self, table_name, entry_name, ttl): + """ + Insert entry to STATE_DB for dynamic ACL + """ + key_name = self.STATE_DB_ACL_TTL_TABLE + '|' + table_name + '|' + entry_name + try: + timenow = int(time.time()) + values = { + "ttl": str(ttl), + "creation_time": str(timenow), + "expiration_time": str(timenow + ttl) + } + self.statedb.hmset(self.statedb.STATE_DB, key_name, values) + info("Added STATE_DB entry for rule {} ttl = {}".format(entry_name, ttl)) + except Exception as e: + raise AclLoaderException("Failed to add STATE_DB entry for {}; error is {}".format(entry_name, str(e))) + + def remove_state_db_ttl_entry(self, table_name, entry_name): + """ + Insert entry to STATE_DB for dynamic ACL + """ + key_name = self.STATE_DB_ACL_TTL_TABLE + '|' + table_name + '|' + entry_name + try: + + self.statedb.delete(self.statedb.STATE_DB, key_name) + info("Removed STATE_DB entry for rule {}".format(entry_name)) + except Exception as e: + raise AclLoaderException("Failed to remove STATE_DB entry for {}; error is {}".format(entry_name, str(e))) + + def handle_dynamic_acl_config(self, table_name, rule): + if not rule.dynamic_acl.config.ttl: + return + + def validate_rule_fields(self, rule_props): protocol = rule_props.get("IP_PROTOCOL") @@ -596,16 +659,24 @@ def validate_rule_fields(self, rule_props): if ("ICMPV6_TYPE" in rule_props or "ICMPV6_CODE" in rule_props) and protocol != 58: raise AclLoaderException("IP_PROTOCOL={} is not ICMPV6, but ICMPV6 fields were provided".format(protocol)) - def convert_rule_to_db_schema(self, table_name, rule): + def convert_rule_to_db_schema(self, table_name, rule, rule_name, is_dynamic): """ Convert rules format from openconfig ACL to Config DB schema :param table_name: ACL table name to which rule belong :param rule: ACL rule in openconfig format + :param rule_name: The name of ACL rule, can be None + :param is_dynamic: A bool, indicates whether the rule is dynamic or not :return: dict with Config DB schema """ - rule_idx = int(rule.config.sequence_id) rule_props = {} - rule_data = {(table_name, "RULE_" + str(rule_idx)): rule_props} + if is_dynamic and not self.is_dynamic_rule(rule): + warning("Attempting to insert a dynamic ACL rule without TTL, refused") + return {} + rule_idx = int(rule.config.sequence_id) + if not rule_name: + rule_name = "RULE_" + str(rule_idx) + rule_key = (table_name, rule_name) + rule_data = {rule_key: rule_props} rule_props["PRIORITY"] = str(self.max_priority - rule_idx) @@ -621,6 +692,7 @@ def convert_rule_to_db_schema(self, table_name, rule): deep_update(rule_props, self.convert_icmp(table_name, rule_idx, rule)) deep_update(rule_props, self.convert_transport(table_name, rule_idx, rule)) deep_update(rule_props, self.convert_input_interface(table_name, rule_idx, rule)) + deep_update(rule_props, self.convert_dynamic_acl_config(rule_key, rule)) self.validate_rule_fields(rule_props) @@ -633,7 +705,7 @@ def deny_rule(self, table_name): :return: dict with Config DB schema """ rule_props = {} - rule_data = {(table_name, "DEFAULT_RULE"): rule_props} + rule_data = {(table_name, self.DEFAULT_RULE): rule_props} rule_props["PRIORITY"] = str(self.min_priority) rule_props["PACKET_ACTION"] = "DROP" if self.is_table_ipv6(table_name): @@ -642,9 +714,10 @@ def deny_rule(self, table_name): rule_props["ETHER_TYPE"] = str(self.ethertype_map["ETHERTYPE_IPV4"]) return rule_data - def convert_rules(self): + def convert_rules(self, is_dynamic): """ Convert rules in openconfig ACL format to Config DB schema + :param is_dynamic: A bool, indicates whether the rule is dynamic or not :return: """ for acl_set_name in self.yang_acl.acl.acl_sets.acl_set: @@ -661,7 +734,13 @@ def convert_rules(self): for acl_entry_name in acl_set.acl_entries.acl_entry: acl_entry = acl_set.acl_entries.acl_entry[acl_entry_name] try: - rule = self.convert_rule_to_db_schema(table_name, acl_entry) + acl_name_to_use = None + if is_dynamic: + acl_name_to_use = "DYNAMIC_RULE_" + acl_entry_name + rule = self.convert_rule_to_db_schema(table_name=table_name, + rule=acl_entry, + rule_name=acl_name_to_use, + is_dynamic=is_dynamic) deep_update(self.rules_info, rule) except AclLoaderException as ex: error("Error processing rule %s: %s. Skipped." % (acl_entry_name, ex)) @@ -765,6 +844,43 @@ 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 update_dynamic_acl_rules(self): + """ + Perform update of dynamic ACL rule. + Renew the TTL of ACL rule if existing, or insert new dynamic ACL rule + :return: + """ + for key, entry in self.rules_info.items(): + if key[1] == self.DEFAULT_RULE: + continue + if key in self.rules_db_info: + if not self.is_db_rule_dynamic(key): + warning("Attempting to overwrite a non-dynamic rule {}".format(key)) + continue + else: + self.configdb.mod_entry(self.ACL_RULE, key, entry) + info("Added a new dynamic ACL rule {}".format(key)) + + self.add_state_db_ttl_entry(table_name=key[0], entry_name=key[1], ttl=self.rules_ttl[key]) + + def remove_dynamic_acl_rules(self): + """ + Remove dynamic ACL rule specified by input json file. + 1. Remove entry from CONFIG_DB (must be dynamic) + 2. Remove TTL entry from STATE_DB + :return: + """ + for key, entry in self.rules_info.items(): + if key[1] == self.DEFAULT_RULE: + continue + if key in self.rules_db_info: + if not self.is_db_rule_dynamic(key): + warning("Attempting to remove a non-dynamic rule {}".format(key)) + continue + self.configdb.mod_entry(self.ACL_RULE, key, None) + info("Removed a dynamic ACL rule {}".format(key)) + self.remove_state_db_ttl_entry(table_name=key[0], entry_name=key[1]) + def delete(self, table=None, rule=None): """ :param table: @@ -899,7 +1015,7 @@ def pop_action(val): return action def pop_matches(val): - matches = list(sorted(["%s: %s" % (k, val[k]) for k in val])) + matches = list(sorted(["%s: %s" % (k, val[k]) for k in val if k != "is_dynamic"])) if len(matches) == 0: matches.append("N/A") return matches @@ -1065,6 +1181,26 @@ def incremental(ctx, filename, session_name, mirror_stage, max_priority): acl_loader.load_rules_from_file(filename) acl_loader.incremental_update() +@update.command() +@click.argument('filename', type=click.Path(exists=True)) +@click.option('--table_name', type=click.STRING, required=False) +@click.option('--max_priority', type=click.INT, required=False) +@click.pass_context +def dynamic(ctx, filename, table_name, max_priority): + """ + Create dynamic ACL rule, or renew the TTL of an existing dynamic ACL rule + If a table_name is provided, the operation will be restricted in the specified table. + """ + acl_loader = ctx.obj["acl_loader"] + + if table_name: + acl_loader.set_table_name(table_name) + + if max_priority: + acl_loader.set_max_priority(max_priority) + + acl_loader.load_rules_from_file(filename, True) + acl_loader.update_dynamic_acl_rules() @cli.command() @click.argument('table', required=False) @@ -1079,6 +1215,23 @@ def delete(ctx, table, rule): acl_loader.delete(table, rule) +@cli.command() +@click.argument('filename', type=click.Path(exists=True)) +@click.option('--table_name', type=click.STRING, required=False) +@click.pass_context +def delete_dynamic(ctx, filename, table_name): + """ + Delete dynamic ACL rules. + If a table_name is provided, the operation will be restricted in the specified table. + """ + acl_loader = ctx.obj["acl_loader"] + + if table_name: + acl_loader.set_table_name(table_name) + + acl_loader.load_rules_from_file(filename, True) + acl_loader.remove_dynamic_acl_rules() + if __name__ == "__main__": try: cli() From eafc0f88f299496441413c34ffa620a9d6d33de3 Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Mon, 5 Sep 2022 19:27:10 -0700 Subject: [PATCH 02/13] Update according to 0905 desgin --- acl_loader/main.py | 87 +++++++++++++--------------------------------- 1 file changed, 25 insertions(+), 62 deletions(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index 7c39cf326f..55e537b39a 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -10,6 +10,7 @@ import pyangbind.lib.pybindJSON as pybindJSON from natsort import natsorted from sonic_py_common import multi_asic +from swsscommon import swsscommon from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector from utilities_common.general import load_db_config import time @@ -84,8 +85,7 @@ class AclLoader(object): ACL_ACTIONS_CAPABILITY_FIELD = "action_list" ACL_ACTION_CAPABILITY_FIELD = "ACL_ACTION" DEFAULT_RULE = "DEFAULT_RULE" - - STATE_DB_ACL_TTL_TABLE = "ACL_TTL_TABLE" + CONFIG_DB_DYNAMIC_ACL_RULE_TABLE = "DYNAMIC_ACL_RULE_TABLE" min_priority = 1 max_priority = 10000 @@ -612,35 +612,22 @@ def convert_dynamic_acl_config(self, key, rule): return rule_props - def add_state_db_ttl_entry(self, table_name, entry_name, ttl): + def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry, ttl): """ - Insert entry to STATE_DB for dynamic ACL + Insert entry to CONFIG_DB for dynamic ACL """ - key_name = self.STATE_DB_ACL_TTL_TABLE + '|' + table_name + '|' + entry_name + key = table + '|' + rule try: timenow = int(time.time()) - values = { - "ttl": str(ttl), - "creation_time": str(timenow), - "expiration_time": str(timenow + ttl) - } - self.statedb.hmset(self.statedb.STATE_DB, key_name, values) - info("Added STATE_DB entry for rule {} ttl = {}".format(entry_name, ttl)) + entry['ttl'] = str(ttl) + entry['creation_time'] = str(timenow) + entry['expiration_time'] = str(timenow + ttl) + self.configdb.mod_entry(self.CONFIG_DB_DYNAMIC_ACL_RULE_TABLE, key, entry) + info("Added CONFIG_DB entry for rule {}".format(rule)) + info(str(entry)) except Exception as e: - raise AclLoaderException("Failed to add STATE_DB entry for {}; error is {}".format(entry_name, str(e))) + raise AclLoaderException("Failed to add CONFIG_DB entry for {}; error is {}".format(rule, str(e))) - def remove_state_db_ttl_entry(self, table_name, entry_name): - """ - Insert entry to STATE_DB for dynamic ACL - """ - key_name = self.STATE_DB_ACL_TTL_TABLE + '|' + table_name + '|' + entry_name - try: - - self.statedb.delete(self.statedb.STATE_DB, key_name) - info("Removed STATE_DB entry for rule {}".format(entry_name)) - except Exception as e: - raise AclLoaderException("Failed to remove STATE_DB entry for {}; error is {}".format(entry_name, str(e))) - def handle_dynamic_acl_config(self, table_name, rule): if not rule.dynamic_acl.config.ttl: return @@ -853,33 +840,16 @@ def update_dynamic_acl_rules(self): for key, entry in self.rules_info.items(): if key[1] == self.DEFAULT_RULE: continue - if key in self.rules_db_info: - if not self.is_db_rule_dynamic(key): - warning("Attempting to overwrite a non-dynamic rule {}".format(key)) - continue - else: - self.configdb.mod_entry(self.ACL_RULE, key, entry) - info("Added a new dynamic ACL rule {}".format(key)) - - self.add_state_db_ttl_entry(table_name=key[0], entry_name=key[1], ttl=self.rules_ttl[key]) + self.add_config_db_dynamic_acl_rule_entry(table=key[0], rule=key[1], entry=entry, ttl=self.rules_ttl[key]) - def remove_dynamic_acl_rules(self): + def remove_dynamic_acl_rules(self, table=None, rule=None): """ - Remove dynamic ACL rule specified by input json file. - 1. Remove entry from CONFIG_DB (must be dynamic) - 2. Remove TTL entry from STATE_DB + Remove entry from CONFIG_DB (must be dynamic) :return: """ - for key, entry in self.rules_info.items(): - if key[1] == self.DEFAULT_RULE: - continue - if key in self.rules_db_info: - if not self.is_db_rule_dynamic(key): - warning("Attempting to remove a non-dynamic rule {}".format(key)) - continue - self.configdb.mod_entry(self.ACL_RULE, key, None) - info("Removed a dynamic ACL rule {}".format(key)) - self.remove_state_db_ttl_entry(table_name=key[0], entry_name=key[1]) + key = str(table) + '|' + "DYNAMIC_RULE_" + str(rule) + self.configdb.set_entry(self.CONFIG_DB_DYNAMIC_ACL_RULE_TABLE, key, None) + info("Removed a dynamic ACL rule {}".format(key)) def delete(self, table=None, rule=None): """ @@ -1186,10 +1156,9 @@ def incremental(ctx, filename, session_name, mirror_stage, max_priority): @click.option('--table_name', type=click.STRING, required=False) @click.option('--max_priority', type=click.INT, required=False) @click.pass_context -def dynamic(ctx, filename, table_name, max_priority): +def time_based_acl(ctx, filename, table_name, max_priority): """ - Create dynamic ACL rule, or renew the TTL of an existing dynamic ACL rule - If a table_name is provided, the operation will be restricted in the specified table. + Create time-based ACL rule, or renew the TTL of an existing ACL rule. """ acl_loader = ctx.obj["acl_loader"] @@ -1214,23 +1183,17 @@ def delete(ctx, table, rule): acl_loader.delete(table, rule) - @cli.command() -@click.argument('filename', type=click.Path(exists=True)) -@click.option('--table_name', type=click.STRING, required=False) +@click.argument('table', type=click.STRING, required=True) +@click.argument('rule', type=click.STRING, required=True) @click.pass_context -def delete_dynamic(ctx, filename, table_name): +def delete_time_based_acl(ctx, table, rule): """ - Delete dynamic ACL rules. - If a table_name is provided, the operation will be restricted in the specified table. + Delete time-based ACL rules. """ acl_loader = ctx.obj["acl_loader"] - if table_name: - acl_loader.set_table_name(table_name) - - acl_loader.load_rules_from_file(filename, True) - acl_loader.remove_dynamic_acl_rules() + acl_loader.remove_dynamic_acl_rules(table, rule) if __name__ == "__main__": try: From 0006a606242d14ef18f8bc5b3a61a90c84237457 Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Tue, 6 Sep 2022 18:45:11 -0700 Subject: [PATCH 03/13] fix typo --- acl_loader/main.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index 55e537b39a..4b7690945f 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -85,7 +85,7 @@ class AclLoader(object): ACL_ACTIONS_CAPABILITY_FIELD = "action_list" ACL_ACTION_CAPABILITY_FIELD = "ACL_ACTION" DEFAULT_RULE = "DEFAULT_RULE" - CONFIG_DB_DYNAMIC_ACL_RULE_TABLE = "DYNAMIC_ACL_RULE_TABLE" + DYNAMIC_ACL_RULE_TABLE = "DYNAMIC_ACL_RULE" min_priority = 1 max_priority = 10000 @@ -622,7 +622,7 @@ def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry, ttl): entry['ttl'] = str(ttl) entry['creation_time'] = str(timenow) entry['expiration_time'] = str(timenow + ttl) - self.configdb.mod_entry(self.CONFIG_DB_DYNAMIC_ACL_RULE_TABLE, key, entry) + self.configdb.mod_entry(self.DYNAMIC_ACL_RULE_TABLE, key, entry) info("Added CONFIG_DB entry for rule {}".format(rule)) info(str(entry)) except Exception as e: @@ -848,7 +848,7 @@ def remove_dynamic_acl_rules(self, table=None, rule=None): :return: """ key = str(table) + '|' + "DYNAMIC_RULE_" + str(rule) - self.configdb.set_entry(self.CONFIG_DB_DYNAMIC_ACL_RULE_TABLE, key, None) + self.configdb.set_entry(self.DYNAMIC_ACL_RULE_TABLE, key, None) info("Removed a dynamic ACL rule {}".format(key)) def delete(self, table=None, rule=None): From 2371472a0e340ab035f34a48461e18b7902142d4 Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Tue, 6 Sep 2022 19:59:29 -0700 Subject: [PATCH 04/13] Fix PR merge alerts --- acl_loader/main.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index 4b7690945f..a533e8fd80 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -10,7 +10,6 @@ import pyangbind.lib.pybindJSON as pybindJSON from natsort import natsorted from sonic_py_common import multi_asic -from swsscommon import swsscommon from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector from utilities_common.general import load_db_config import time @@ -596,7 +595,7 @@ def is_db_rule_dynamic(self, key): if 'is_dynamic' in data and data['is_dynamic'].lower() == 'true': return True return False - except: + except BaseException as e: return False def is_dynamic_rule(self, rule): From fe80fc46ece9192c192c12e98bb44e6b20e8ac85 Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Tue, 6 Sep 2022 20:16:30 -0700 Subject: [PATCH 05/13] Fix PR alert --- acl_loader/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index a533e8fd80..7d31ddee27 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -595,7 +595,7 @@ def is_db_rule_dynamic(self, key): if 'is_dynamic' in data and data['is_dynamic'].lower() == 'true': return True return False - except BaseException as e: + except Exception as e: return False def is_dynamic_rule(self, rule): From b35c3267cd0d6f929c2487a16ab23745ebce2d86 Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Thu, 8 Sep 2022 01:01:10 -0700 Subject: [PATCH 06/13] Modify according to PR review comment --- acl_loader/main.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index 7d31ddee27..6c0f876b92 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -656,7 +656,10 @@ def convert_rule_to_db_schema(self, table_name, rule, rule_name, is_dynamic): """ rule_props = {} if is_dynamic and not self.is_dynamic_rule(rule): - warning("Attempting to insert a dynamic ACL rule without TTL, refused") + warning("Attempting to insert time-based ACL rule without TTL, refused") + return {} + if not is_dynamic and self.is_dynamic_rule(rule): + warning("Attempting to insert time-based ACL rule to non-time-based ACL table, refused") return {} rule_idx = int(rule.config.sequence_id) if not rule_name: @@ -722,7 +725,7 @@ def convert_rules(self, is_dynamic): try: acl_name_to_use = None if is_dynamic: - acl_name_to_use = "DYNAMIC_RULE_" + acl_entry_name + acl_name_to_use = "TIME_BASED_RULE_" + acl_entry_name rule = self.convert_rule_to_db_schema(table_name=table_name, rule=acl_entry, rule_name=acl_name_to_use, @@ -846,7 +849,7 @@ def remove_dynamic_acl_rules(self, table=None, rule=None): Remove entry from CONFIG_DB (must be dynamic) :return: """ - key = str(table) + '|' + "DYNAMIC_RULE_" + str(rule) + key = str(table) + '|' + "TIME_BASED_RULE_" + str(rule) self.configdb.set_entry(self.DYNAMIC_ACL_RULE_TABLE, key, None) info("Removed a dynamic ACL rule {}".format(key)) From 4460dac667cd6b685e97c486dfccd1c31a3d8c8e Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Sat, 10 Sep 2022 03:04:56 -0700 Subject: [PATCH 07/13] Modify according to PR review comment --- acl_loader/main.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index 6c0f876b92..a8f7cfdaca 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -84,7 +84,7 @@ class AclLoader(object): ACL_ACTIONS_CAPABILITY_FIELD = "action_list" ACL_ACTION_CAPABILITY_FIELD = "ACL_ACTION" DEFAULT_RULE = "DEFAULT_RULE" - DYNAMIC_ACL_RULE_TABLE = "DYNAMIC_ACL_RULE" + TIME_BASED_ACL_RULE_TABLE = "TIME_BASED_ACL_RULE" min_priority = 1 max_priority = 10000 @@ -621,7 +621,7 @@ def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry, ttl): entry['ttl'] = str(ttl) entry['creation_time'] = str(timenow) entry['expiration_time'] = str(timenow + ttl) - self.configdb.mod_entry(self.DYNAMIC_ACL_RULE_TABLE, key, entry) + self.configdb.mod_entry(self.TIME_BASED_ACL_RULE_TABLE, key, entry) info("Added CONFIG_DB entry for rule {}".format(rule)) info(str(entry)) except Exception as e: @@ -850,7 +850,7 @@ def remove_dynamic_acl_rules(self, table=None, rule=None): :return: """ key = str(table) + '|' + "TIME_BASED_RULE_" + str(rule) - self.configdb.set_entry(self.DYNAMIC_ACL_RULE_TABLE, key, None) + self.configdb.set_entry(self.TIME_BASED_ACL_RULE_TABLE, key, None) info("Removed a dynamic ACL rule {}".format(key)) def delete(self, table=None, rule=None): From bec2e4ba7d4e8b57a893d17ae9654718e7226e87 Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Sat, 10 Sep 2022 03:26:27 -0700 Subject: [PATCH 08/13] Modify according to PR review comment --- acl_loader/main.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index a8f7cfdaca..ec77e87b0d 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -618,7 +618,8 @@ def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry, ttl): key = table + '|' + rule try: timenow = int(time.time()) - entry['ttl'] = str(ttl) + del entry['is_dynamic'] + #entry['ttl'] = str(ttl) entry['creation_time'] = str(timenow) entry['expiration_time'] = str(timenow + ttl) self.configdb.mod_entry(self.TIME_BASED_ACL_RULE_TABLE, key, entry) From 803a1d08dc7e18f61736b24434a5b180cb229448 Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Mon, 12 Sep 2022 19:48:08 -0700 Subject: [PATCH 09/13] Format info output --- acl_loader/main.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index ec77e87b0d..94200c04d3 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -618,12 +618,11 @@ def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry, ttl): key = table + '|' + rule try: timenow = int(time.time()) - del entry['is_dynamic'] - #entry['ttl'] = str(ttl) + entry.pop('is_dynamic', None) entry['creation_time'] = str(timenow) entry['expiration_time'] = str(timenow + ttl) self.configdb.mod_entry(self.TIME_BASED_ACL_RULE_TABLE, key, entry) - info("Added CONFIG_DB entry for rule {}".format(rule)) + info("Added CONFIG_DB entry: time-based ACL rule {}".format(key)) info(str(entry)) except Exception as e: raise AclLoaderException("Failed to add CONFIG_DB entry for {}; error is {}".format(rule, str(e))) @@ -852,7 +851,7 @@ def remove_dynamic_acl_rules(self, table=None, rule=None): """ key = str(table) + '|' + "TIME_BASED_RULE_" + str(rule) self.configdb.set_entry(self.TIME_BASED_ACL_RULE_TABLE, key, None) - info("Removed a dynamic ACL rule {}".format(key)) + info("Removed CONFIG_DB entry: time-based ACL rule {}".format(key)) def delete(self, table=None, rule=None): """ From 9ab1f48fb8ee4d3a106287e4f058160d86ce5bec Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Wed, 14 Sep 2022 19:59:21 -0700 Subject: [PATCH 10/13] Update acl-loader according to HLD --- acl_loader/main.py | 79 +++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 40 deletions(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index 94200c04d3..06734cb472 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -345,16 +345,16 @@ def parse_acl_json(filename): raise AclLoaderException("Invalid input file %s" % filename) return yang_acl - def load_rules_from_file(self, filename, is_dynamic=False): + def load_rules_from_file(self, filename, is_time_based=False): """ Load file with ACL rules configuration in openconfig ACL format. Convert rules to Config DB schema. :param filename: File in openconfig ACL format - :param is_dynamic: A bool, indicates whether the rule is dynamic or not + :param is_time_based: A bool, indicates whether the rule is dynamic or not :return: """ self.yang_acl = AclLoader.parse_acl_json(filename) - self.convert_rules(is_dynamic) + self.convert_rules(is_time_based) def convert_action(self, table_name, rule_idx, rule): rule_props = {} @@ -592,45 +592,37 @@ def convert_input_interface(self, table_name, rule_idx, rule): def is_db_rule_dynamic(self, key): try: data = self.configdb.get_entry(self.ACL_RULE, key) - if 'is_dynamic' in data and data['is_dynamic'].lower() == 'true': + if 'is_time_based' in data and data['is_time_based'].lower() == 'true': return True return False except Exception as e: return False - def is_dynamic_rule(self, rule): - return rule.dynamic_acl.config.ttl + def is_time_based_rule(self, rule): + return rule.time_range.config.start_time def convert_dynamic_acl_config(self, key, rule): rule_props = {} - # The ttl value is not written into ACL_RULE table as we don't want - # update notificaton when TTL is changed - if self.is_dynamic_rule(rule): - rule_props['is_dynamic'] = 'True' - self.rules_ttl[key] = int(rule.dynamic_acl.config.ttl) + + if self.is_time_based_rule(rule): + rule_props['is_time_based'] = 'True' + rule_props['start_time'] = rule.time_range.config.start_time + rule_props['end_time'] = rule.time_range.config.end_time return rule_props - def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry, ttl): + def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry): """ Insert entry to CONFIG_DB for dynamic ACL """ key = table + '|' + rule try: - timenow = int(time.time()) - entry.pop('is_dynamic', None) - entry['creation_time'] = str(timenow) - entry['expiration_time'] = str(timenow + ttl) + entry.pop('is_time_based', None) self.configdb.mod_entry(self.TIME_BASED_ACL_RULE_TABLE, key, entry) info("Added CONFIG_DB entry: time-based ACL rule {}".format(key)) info(str(entry)) except Exception as e: raise AclLoaderException("Failed to add CONFIG_DB entry for {}; error is {}".format(rule, str(e))) - - def handle_dynamic_acl_config(self, table_name, rule): - if not rule.dynamic_acl.config.ttl: - return - def validate_rule_fields(self, rule_props): protocol = rule_props.get("IP_PROTOCOL") @@ -645,20 +637,20 @@ def validate_rule_fields(self, rule_props): if ("ICMPV6_TYPE" in rule_props or "ICMPV6_CODE" in rule_props) and protocol != 58: raise AclLoaderException("IP_PROTOCOL={} is not ICMPV6, but ICMPV6 fields were provided".format(protocol)) - def convert_rule_to_db_schema(self, table_name, rule, rule_name, is_dynamic): + def convert_rule_to_db_schema(self, table_name, rule, rule_name, is_time_based): """ Convert rules format from openconfig ACL to Config DB schema :param table_name: ACL table name to which rule belong :param rule: ACL rule in openconfig format :param rule_name: The name of ACL rule, can be None - :param is_dynamic: A bool, indicates whether the rule is dynamic or not + :param is_time_based: A bool, indicates whether the rule is dynamic or not :return: dict with Config DB schema """ rule_props = {} - if is_dynamic and not self.is_dynamic_rule(rule): + if is_time_based and not self.is_time_based_rule(rule): warning("Attempting to insert time-based ACL rule without TTL, refused") return {} - if not is_dynamic and self.is_dynamic_rule(rule): + if not is_time_based and self.is_time_based_rule(rule): warning("Attempting to insert time-based ACL rule to non-time-based ACL table, refused") return {} rule_idx = int(rule.config.sequence_id) @@ -703,10 +695,10 @@ def deny_rule(self, table_name): rule_props["ETHER_TYPE"] = str(self.ethertype_map["ETHERTYPE_IPV4"]) return rule_data - def convert_rules(self, is_dynamic): + def convert_rules(self, is_time_based): """ Convert rules in openconfig ACL format to Config DB schema - :param is_dynamic: A bool, indicates whether the rule is dynamic or not + :param is_time_based: A bool, indicates whether the rule is dynamic or not :return: """ for acl_set_name in self.yang_acl.acl.acl_sets.acl_set: @@ -724,12 +716,12 @@ def convert_rules(self, is_dynamic): acl_entry = acl_set.acl_entries.acl_entry[acl_entry_name] try: acl_name_to_use = None - if is_dynamic: + if is_time_based: acl_name_to_use = "TIME_BASED_RULE_" + acl_entry_name rule = self.convert_rule_to_db_schema(table_name=table_name, rule=acl_entry, rule_name=acl_name_to_use, - is_dynamic=is_dynamic) + is_time_based=is_time_based) deep_update(self.rules_info, rule) except AclLoaderException as ex: error("Error processing rule %s: %s. Skipped." % (acl_entry_name, ex)) @@ -833,7 +825,7 @@ 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 update_dynamic_acl_rules(self): + def update_time_based_acl_rules(self): """ Perform update of dynamic ACL rule. Renew the TTL of ACL rule if existing, or insert new dynamic ACL rule @@ -842,16 +834,23 @@ def update_dynamic_acl_rules(self): for key, entry in self.rules_info.items(): if key[1] == self.DEFAULT_RULE: continue - self.add_config_db_dynamic_acl_rule_entry(table=key[0], rule=key[1], entry=entry, ttl=self.rules_ttl[key]) + self.add_config_db_dynamic_acl_rule_entry(table=key[0], rule=key[1], entry=entry) - def remove_dynamic_acl_rules(self, table=None, rule=None): + def remove_time_based_acl_rules(self, table=None, rule=None): """ Remove entry from CONFIG_DB (must be dynamic) :return: """ - key = str(table) + '|' + "TIME_BASED_RULE_" + str(rule) - self.configdb.set_entry(self.TIME_BASED_ACL_RULE_TABLE, key, None) - info("Removed CONFIG_DB entry: time-based ACL rule {}".format(key)) + if not rule: + keys = self.configdb.get_table(self.TIME_BASED_ACL_RULE_TABLE) + for key in keys: + if table == key[0]: + self.configdb.set_entry(self.TIME_BASED_ACL_RULE_TABLE, key, None) + info("Removed CONFIG_DB entry: time-based ACL rule {}".format(key[0]+'|'+key[1])) + else: + key = str(table) + '|' + "TIME_BASED_RULE_" + str(rule) + self.configdb.set_entry(self.TIME_BASED_ACL_RULE_TABLE, key, None) + info("Removed CONFIG_DB entry: time-based ACL rule {}".format(key)) def delete(self, table=None, rule=None): """ @@ -987,7 +986,7 @@ def pop_action(val): return action def pop_matches(val): - matches = list(sorted(["%s: %s" % (k, val[k]) for k in val if k != "is_dynamic"])) + matches = list(sorted(["%s: %s" % (k, val[k]) for k in val if k != "is_time_based"])) if len(matches) == 0: matches.append("N/A") return matches @@ -1160,7 +1159,7 @@ def incremental(ctx, filename, session_name, mirror_stage, max_priority): @click.pass_context def time_based_acl(ctx, filename, table_name, max_priority): """ - Create time-based ACL rule, or renew the TTL of an existing ACL rule. + Create time-based ACL rule, or renew the timestamp of an existing ACL rule. """ acl_loader = ctx.obj["acl_loader"] @@ -1171,7 +1170,7 @@ def time_based_acl(ctx, filename, table_name, max_priority): acl_loader.set_max_priority(max_priority) acl_loader.load_rules_from_file(filename, True) - acl_loader.update_dynamic_acl_rules() + acl_loader.update_time_based_acl_rules() @cli.command() @click.argument('table', required=False) @@ -1187,7 +1186,7 @@ def delete(ctx, table, rule): @cli.command() @click.argument('table', type=click.STRING, required=True) -@click.argument('rule', type=click.STRING, required=True) +@click.argument('rule', type=click.STRING, required=False) @click.pass_context def delete_time_based_acl(ctx, table, rule): """ @@ -1195,7 +1194,7 @@ def delete_time_based_acl(ctx, table, rule): """ acl_loader = ctx.obj["acl_loader"] - acl_loader.remove_dynamic_acl_rules(table, rule) + acl_loader.remove_time_based_acl_rules(table, rule) if __name__ == "__main__": try: From b850485512a91039b68742ec2866547b904c9136 Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Wed, 14 Sep 2022 20:13:16 -0700 Subject: [PATCH 11/13] Remove unused import --- acl_loader/main.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index 06734cb472..55c0fba47c 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -12,8 +12,6 @@ from sonic_py_common import multi_asic from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector from utilities_common.general import load_db_config -import time - def info(msg): click.echo(click.style("Info: ", fg='cyan') + click.style(str(msg), fg='green')) From b939ebc16e96ce39c42314472f35cc38944f7f5b Mon Sep 17 00:00:00 2001 From: Shiyan Wang Date: Wed, 14 Sep 2022 23:36:59 -0700 Subject: [PATCH 12/13] Remove unused variables --- acl_loader/main.py | 1 - 1 file changed, 1 deletion(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index 55c0fba47c..ee57171232 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -118,7 +118,6 @@ def __init__(self): self.tables_db_info = {} self.rules_db_info = {} self.rules_info = {} - self.rules_ttl = {} # Load database config files load_db_config() From 57c9f7a5f4b7e5a0dda420bf4c37671e57c6a46b Mon Sep 17 00:00:00 2001 From: Shiyan Date: Fri, 4 Nov 2022 10:12:02 +0800 Subject: [PATCH 13/13] Update according to review comments --- acl_loader/main.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/acl_loader/main.py b/acl_loader/main.py index ee57171232..c53462861c 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -347,7 +347,7 @@ def load_rules_from_file(self, filename, is_time_based=False): Load file with ACL rules configuration in openconfig ACL format. Convert rules to Config DB schema. :param filename: File in openconfig ACL format - :param is_time_based: A bool, indicates whether the rule is dynamic or not + :param is_time_based: A bool, indicates whether the rule is time-based or not :return: """ self.yang_acl = AclLoader.parse_acl_json(filename) @@ -586,7 +586,7 @@ def convert_input_interface(self, table_name, rule_idx, rule): return rule_props - def is_db_rule_dynamic(self, key): + def is_db_rule_time_based(self, key): try: data = self.configdb.get_entry(self.ACL_RULE, key) if 'is_time_based' in data and data['is_time_based'].lower() == 'true': @@ -596,9 +596,9 @@ def is_db_rule_dynamic(self, key): return False def is_time_based_rule(self, rule): - return rule.time_range.config.start_time + return rule.time_range.config.end_time - def convert_dynamic_acl_config(self, key, rule): + def convert_time_based_acl_config(self, key, rule): rule_props = {} if self.is_time_based_rule(rule): @@ -608,9 +608,9 @@ def convert_dynamic_acl_config(self, key, rule): return rule_props - def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry): + def add_config_db_time_based_acl_rule_entry(self, table, rule, entry): """ - Insert entry to CONFIG_DB for dynamic ACL + Insert entry to CONFIG_DB for time-based ACL """ key = table + '|' + rule try: @@ -640,12 +640,12 @@ def convert_rule_to_db_schema(self, table_name, rule, rule_name, is_time_based): :param table_name: ACL table name to which rule belong :param rule: ACL rule in openconfig format :param rule_name: The name of ACL rule, can be None - :param is_time_based: A bool, indicates whether the rule is dynamic or not + :param is_time_based: A bool, indicates whether the rule is time-based or not :return: dict with Config DB schema """ rule_props = {} if is_time_based and not self.is_time_based_rule(rule): - warning("Attempting to insert time-based ACL rule without TTL, refused") + warning("Attempting to insert time-based ACL rule without timestamps, refused") return {} if not is_time_based and self.is_time_based_rule(rule): warning("Attempting to insert time-based ACL rule to non-time-based ACL table, refused") @@ -670,7 +670,7 @@ def convert_rule_to_db_schema(self, table_name, rule, rule_name, is_time_based): deep_update(rule_props, self.convert_icmp(table_name, rule_idx, rule)) deep_update(rule_props, self.convert_transport(table_name, rule_idx, rule)) deep_update(rule_props, self.convert_input_interface(table_name, rule_idx, rule)) - deep_update(rule_props, self.convert_dynamic_acl_config(rule_key, rule)) + deep_update(rule_props, self.convert_time_based_acl_config(rule_key, rule)) self.validate_rule_fields(rule_props) @@ -695,7 +695,7 @@ def deny_rule(self, table_name): def convert_rules(self, is_time_based): """ Convert rules in openconfig ACL format to Config DB schema - :param is_time_based: A bool, indicates whether the rule is dynamic or not + :param is_time_based: A bool, indicates whether the rule is time-based or not :return: """ for acl_set_name in self.yang_acl.acl.acl_sets.acl_set: @@ -824,18 +824,18 @@ def incremental_update(self): def update_time_based_acl_rules(self): """ - Perform update of dynamic ACL rule. - Renew the TTL of ACL rule if existing, or insert new dynamic ACL rule + Perform update of time-based ACL rule. + Update the start_time and end_time if existing, or insert new time-based ACL rule :return: """ for key, entry in self.rules_info.items(): if key[1] == self.DEFAULT_RULE: continue - self.add_config_db_dynamic_acl_rule_entry(table=key[0], rule=key[1], entry=entry) + self.add_config_db_time_based_acl_rule_entry(table=key[0], rule=key[1], entry=entry) def remove_time_based_acl_rules(self, table=None, rule=None): """ - Remove entry from CONFIG_DB (must be dynamic) + Remove entry from CONFIG_DB (must be time-based) :return: """ if not rule: