From 40a23b9b4b878492af94b4cf79280e86c47cf189 Mon Sep 17 00:00:00 2001 From: Lawrence Lee Date: Fri, 20 Jan 2023 12:49:28 -0800 Subject: [PATCH] [dash]: Don't attempt to bind empty ACL groups (#2613) * [dash]: Don't attempt to bind empty ACL groups Signed-off-by: Lawrence Lee --- orchagent/dash/dashaclorch.cpp | 7 + tests/test_dash_acl.py | 318 +++++++++++++++++++++------------ 2 files changed, 213 insertions(+), 112 deletions(-) diff --git a/orchagent/dash/dashaclorch.cpp b/orchagent/dash/dashaclorch.cpp index b90b1ab50ec..15afc617c30 100644 --- a/orchagent/dash/dashaclorch.cpp +++ b/orchagent/dash/dashaclorch.cpp @@ -761,6 +761,13 @@ task_process_status DashAclOrch::bindAclToEni(DashAclTable &acl_table, const str return task_need_retry; } + if (acl_group->m_rule_count <= 0) + { + SWSS_LOG_INFO("acl group %s contains 0 rules, waiting for rule creation", acl.m_acl_group_id->c_str()); + acl.m_acl_group_id.reset(); + return task_need_retry; + } + attr.id = getSaiStage(direction, *(acl_group->m_ip_version), stage); attr.value.oid = acl_group->m_dash_acl_group_id; } diff --git a/tests/test_dash_acl.py b/tests/test_dash_acl.py index a94082f5fff..143eea2ad7a 100644 --- a/tests/test_dash_acl.py +++ b/tests/test_dash_acl.py @@ -1,38 +1,84 @@ from swsscommon import swsscommon -import typing +from dvslib.dvs_database import DVSDatabase +from typing import Union import time +import pytest + DVS_ENV = ["HWSKU=Nvidia-MBF2H536C"] +ACL_GROUP_1 = "acl_group_1" +ACL_GROUP_2 = "acl_group_2" +ACL_RULE_1 = "1" +ACL_RULE_2 = "2" +ACL_RULE_3 = "3" +ACL_STAGE_1 = "1" +ACL_STAGE_2 = "2" + +SAI_NULL_OID = "oid:0x0" + def to_string(value): if isinstance(value, bool): return "true" if value else "false" return str(value) +def get_sai_stage(outbound, v4, stage_num): + direction = "OUTBOUND" if outbound else "INBOUND" + ip_version = "V4" if v4 else "V6" + return "SAI_ENI_ATTR_{}_{}_STAGE{}_DASH_ACL_GROUP_ID".format(direction, ip_version, stage_num) + + class ProduceStateTable(object): def __init__(self, database, table_name: str): self.table = swsscommon.ProducerStateTable( database.db_connection, table_name) + self.keys = set() - def __setitem__(self, key: str, pairs: typing.Union[dict, list, tuple]): + def __setitem__(self, key: str, pairs: Union[dict, list, tuple]): pairs_str = [] if isinstance(pairs, dict): pairs = pairs.items() for k, v in pairs: pairs_str.append((to_string(k), to_string(v))) self.table.set(key, pairs_str) + self.keys.add(key) def __delitem__(self, key: str): self.table.delete(str(key)) + self.keys.discard(key) + + def get_keys(self): + return self.keys class Table(object): - def __init__(self, database, table_name: str): + def __init__(self, database: DVSDatabase, table_name: str): self.table_name = table_name + self.db = database self.table = swsscommon.Table(database.db_connection, self.table_name) + # Overload verification methods in DVSDatabase so we can use them per-table + # All methods from DVSDatabase that do not start with '_' are overloaded + # See the DVSDatabase class for info about the use of each method + # For each `func` in DVSDatabase, equivalent to: + # def func(self, **kwargs): + # return self.db.func(table_name=self.table_name, **kwargs) + # This means that we can call e.g. + # table_object.wait_for_n_keys(num_keys=1) + # instead of + # dvs.get_asic_db().wait_for_n_keys(table_name="ASIC_STATE:SAI_EXAMPLE_TABLE", num_keys=1) + overload_methods = [ + attr for attr in dir(DVSDatabase) + if not attr.startswith('_') and callable(getattr(DVSDatabase, attr)) + ] + for method_name in overload_methods: + setattr( + self, method_name, lambda method_name=method_name, + **kwargs: getattr(self.db, method_name)(table_name=self.table_name, **kwargs) + ) + def __getitem__(self, key: str): exists, result = self.table.get(str(key)) if not exists: @@ -40,31 +86,56 @@ def __getitem__(self, key: str): else: return dict(result) - def get_keys(self): - return self.table.getKeys() +APPL_DB_TABLE_LIST = [ + swsscommon.APP_DASH_ACL_IN_TABLE_NAME, + swsscommon.APP_DASH_ACL_OUT_TABLE_NAME, + swsscommon.APP_DASH_ACL_GROUP_TABLE_NAME, + swsscommon.APP_DASH_ACL_RULE_TABLE_NAME, + swsscommon.APP_DASH_ENI_TABLE_NAME, + swsscommon.APP_DASH_VNET_TABLE_NAME, + swsscommon.APP_DASH_APPLIANCE_TABLE_NAME +] + +# TODO: At some point, orchagent will be update to write to some DB to indicate that it's finished +# processing updates for a given table. Once this is implemented, we can remove all the `sleep` +# statements in these tests and instead proactively check for the finished signal from orchagent class DashAcl(object): def __init__(self, dvs): self.dvs = dvs - self.app_dash_acl_in_table = ProduceStateTable( - self.dvs.get_app_db(), swsscommon.APP_DASH_ACL_IN_TABLE_NAME) - self.app_dash_acl_out_table = ProduceStateTable( - self.dvs.get_app_db(), swsscommon.APP_DASH_ACL_OUT_TABLE_NAME) - self.app_dash_acl_group_table = ProduceStateTable( - self.dvs.get_app_db(), swsscommon.APP_DASH_ACL_GROUP_TABLE_NAME) - self.app_dash_acl_rule_table = ProduceStateTable( - self.dvs.get_app_db(), swsscommon.APP_DASH_ACL_RULE_TABLE_NAME) - self.app_dash_eni_table = ProduceStateTable( - self.dvs.get_app_db(), swsscommon.APP_DASH_ENI_TABLE_NAME) - self.app_dash_vnet_table = ProduceStateTable( - self.dvs.get_app_db(), swsscommon.APP_DASH_VNET_TABLE_NAME) + self.app_db_tables = [] + + for table in APPL_DB_TABLE_LIST: + pst = ProduceStateTable( + self.dvs.get_app_db(), table + ) + table_variable_name = "app_{}".format(table.lower()) + # Based on swsscommon convention for table names, assume + # e.g. swsscommon.APP_DASH_ENI_TABLE_NAME == "DASH_ENI_TABLE", therefore + # the ProducerStateTable object for swsscommon.APP_DASH_ENI_TABLE_NAME + # will be accessible as `self.app_dash_eni_table` + setattr(self, table_variable_name, pst) + self.app_db_tables.append(pst) + self.asic_dash_acl_rule_table = Table( self.dvs.get_asic_db(), "ASIC_STATE:SAI_OBJECT_TYPE_DASH_ACL_RULE") self.asic_dash_acl_group_table = Table( self.dvs.get_asic_db(), "ASIC_STATE:SAI_OBJECT_TYPE_DASH_ACL_GROUP") self.asic_eni_table = Table( self.dvs.get_asic_db(), "ASIC_STATE:SAI_OBJECT_TYPE_ENI") + self.asic_vip_table = Table( + self.dvs.get_asic_db(), "ASIC_STATE:SAI_OBJECT_TYPE_VIP_ENTRY") + self.asic_vnet_table = Table( + self.dvs.get_asic_db(), "ASIC_STATE:SAI_OBJECT_TYPE_VNET") + + self.asic_db_tables = [ + self.asic_dash_acl_group_table, + self.asic_dash_acl_rule_table, + self.asic_eni_table, + self.asic_vip_table, + self.asic_vnet_table + ] def create_acl_rule(self, group_id, rule_id, attr_maps: dict): self.app_dash_acl_rule_table[str( @@ -79,6 +150,12 @@ def create_acl_group(self, group_id, attr_maps: dict): def remove_acl_group(self, group_id): del self.app_dash_acl_group_table[str(group_id)] + def create_appliance(self, name, attr_maps: dict): + self.app_dash_appliance_table[str(name)] = attr_maps + + def remove_appliance(self, name): + del self.app_dash_appliance_table[str(name)] + def create_eni(self, eni, attr_maps: dict): self.app_dash_eni_table[str(eni)] = attr_maps @@ -107,46 +184,45 @@ def unbind_acl_out(self, eni, stage): class TestAcl(object): - def create_ctx(self, dvs): + @pytest.fixture + def ctx(self, dvs): self.vnet_name = "vnet1" self.eni_name = "eth0" + self.appliance_name = "default_app" + self.vm_vni = "4321" + self.appliance_sip = "10.20.30.40" self.vni = "1" - self.mac_address = "00:00:00:00:00:00" - ctx = DashAcl(dvs) - ctx.create_vnet(self.vnet_name, {"vni": self.vni}) - ctx.create_eni(self.eni_name, {"vnet": self.vnet_name, - "mac_address": self.mac_address}) - return ctx - - def destroy_ctx(self, ctx): - time.sleep(1) - # Because the Acl group, ENI and VNET has been referred by each others, - # And their implementation can only support synchronous deletion, So we need to wait for a while. - ctx.remove_eni(self.eni_name) - time.sleep(1) - ctx.remove_vnet(self.vnet_name) - time.sleep(1) - - def test_acl_flow(self, dvs): - ctx = self.create_ctx(dvs) - acl_group1 = "1" - acl_rule1 = "1" - acl_rule2 = "2" - acl_rule3 = "3" - stage1 = "1" - - ctx.create_acl_group(acl_group1, {"ip_version": "ipv4"}) - ctx.create_acl_rule(acl_group1, acl_rule1, {"priority": "1", "action": "allow", "terminating": "false", + self.mac_address = "01:23:45:67:89:ab" + self.underlay_ip = "1.1.1.1" + + acl_context = DashAcl(dvs) + acl_context.create_appliance(self.appliance_name, {"sip": self.appliance_sip, "vm_vni": self.vm_vni}) + acl_context.create_vnet(self.vnet_name, {"vni": self.vni}) + acl_context.create_eni(self.eni_name, {"vnet": self.vnet_name, + "mac_address": self.mac_address, "underlay_ip": self.underlay_ip}) + + acl_context.asic_vip_table.wait_for_n_keys(num_keys=1) + acl_context.asic_vnet_table.wait_for_n_keys(num_keys=1) + acl_context.asic_eni_table.wait_for_n_keys(num_keys=1) + + yield acl_context + + # Manually cleanup by deleting all remaining APPL_DB keys + for table in acl_context.app_db_tables: + keys = table.get_keys() + for key in list(keys): + del table[key] + + for table in acl_context.asic_db_tables: + table.wait_for_n_keys(num_keys=0) + + def test_acl_flow(self, ctx): + ctx.create_acl_group(ACL_GROUP_1, {"ip_version": "ipv4"}) + ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_1, {"priority": "1", "action": "allow", "terminating": "false", "src_addr": "192.168.0.1/32,192.168.1.2/30", "dst_addr": "192.168.0.1/32,192.168.1.2/30", "src_port": "0-1", "dst_port": "0-1"}) - ctx.bind_acl_in(self.eni_name, stage1, acl_group1) - time.sleep(3) - rule_ids = ctx.asic_dash_acl_rule_table.get_keys() - assert rule_ids - rule1_id = rule_ids[0] - group_ids = ctx.asic_dash_acl_group_table.get_keys() - assert group_ids - group1_id = group_ids[0] + rule1_id= ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=1)[0] + group1_id= ctx.asic_dash_acl_group_table.wait_for_n_keys(num_keys=1)[0] rule1_attr = ctx.asic_dash_acl_rule_table[rule1_id] assert rule1_attr["SAI_DASH_ACL_RULE_ATTR_PRIORITY"] == "1" assert rule1_attr["SAI_DASH_ACL_RULE_ATTR_ACTION"] == "SAI_DASH_ACL_RULE_ACTION_PERMIT_AND_CONTINUE" @@ -160,90 +236,108 @@ def test_acl_flow(self, dvs): assert group1_attr["SAI_DASH_ACL_GROUP_ATTR_IP_ADDR_FAMILY"] == "SAI_IP_ADDR_FAMILY_IPV4" # Create multiple rules - ctx.create_acl_rule(acl_group1, acl_rule2, {"priority": "2", "action": "allow", "terminating": "false", + ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_2, {"priority": "2", "action": "allow", "terminating": "false", "src_addr": "192.168.0.1/32,192.168.1.2/30", "dst_addr": "192.168.0.1/32,192.168.1.2/30", "src_port": "0-1", "dst_port": "0-1"}) - ctx.create_acl_rule(acl_group1, acl_rule3, {"priority": "2", "action": "allow", "terminating": "false", + ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_3, {"priority": "2", "action": "allow", "terminating": "false", "src_addr": "192.168.0.1/32,192.168.1.2/30", "dst_addr": "192.168.0.1/32,192.168.1.2/30", "src_port": "0-1", "dst_port": "0-1"}) - time.sleep(3) - rule_ids = ctx.asic_dash_acl_rule_table.get_keys() - assert len(rule_ids) == 3 - ctx.unbind_acl_in(self.eni_name, stage1) - ctx.remove_acl_rule(acl_group1, acl_rule1) - ctx.remove_acl_rule(acl_group1, acl_rule2) - ctx.remove_acl_rule(acl_group1, acl_rule3) - ctx.remove_acl_group(acl_group1) - time.sleep(3) - assert len(ctx.asic_dash_acl_rule_table.get_keys()) == 0 - assert len(ctx.asic_dash_acl_group_table.get_keys()) == 0 - self.destroy_ctx(ctx) - - def test_acl_group(self, dvs): - ctx = self.create_ctx(dvs) - acl_group1 = "1" - acl_rule1 = "1" - - ctx.create_acl_group(acl_group1, {"ip_version": "ipv6"}) - ctx.create_acl_rule(acl_group1, acl_rule1, {"priority": "1", "action": "allow", "terminating": "false", + ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=3) + ctx.unbind_acl_in(self.eni_name, ACL_STAGE_1) + ctx.remove_acl_rule(ACL_GROUP_1, ACL_RULE_1) + ctx.remove_acl_rule(ACL_GROUP_1, ACL_RULE_2) + ctx.remove_acl_rule(ACL_GROUP_1, ACL_RULE_3) + ctx.remove_acl_group(ACL_GROUP_1) + ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=0) + ctx.asic_dash_acl_group_table.wait_for_n_keys(num_keys=0) + + def test_acl_group(self, ctx): + ctx.create_acl_group(ACL_GROUP_1, {"ip_version": "ipv6"}) + ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_1, {"priority": "1", "action": "allow", "terminating": "false", "src_addr": "192.168.0.1/32,192.168.1.2/30", "dst_addr": "192.168.0.1/32,192.168.1.2/30", "src_port": "0-1", "dst_port": "0-1"}) - time.sleep(3) + ctx.asic_dash_acl_group_table.wait_for_n_keys(num_keys=1) # Remove group before removing its rule - ctx.remove_acl_group(acl_group1) + ctx.remove_acl_group(ACL_GROUP_1) + # Wait a few seconds to make sure no changes are made + # since group still contains a rule time.sleep(3) - assert len(ctx.asic_dash_acl_group_table.get_keys()) == 1 - ctx.remove_acl_rule(acl_group1, acl_rule1) + ctx.asic_dash_acl_group_table.wait_for_n_keys(num_keys=1) + + ctx.remove_acl_rule(ACL_GROUP_1, ACL_RULE_1) + ctx.asic_dash_acl_group_table.wait_for_n_keys(num_keys=0) + + def test_empty_acl_group_binding(self, ctx): + """ + Verifies behavior when binding ACL groups + """ + eni_key = ctx.asic_eni_table.get_keys()[0] + sai_stage = get_sai_stage(outbound=False, v4=True, stage_num=ACL_STAGE_1) + + ctx.create_acl_group(ACL_GROUP_1, {"ip_version": "ipv4"}) + acl_group_key = ctx.asic_dash_acl_group_table.wait_for_n_keys(num_keys=1)[0] + ctx.bind_acl_in(self.eni_name, ACL_STAGE_1, ACL_GROUP_1) time.sleep(3) - assert len(ctx.asic_dash_acl_group_table.get_keys()) == 0 + # Binding should not happen yet because the ACL group is empty + assert sai_stage not in ctx.asic_eni_table[eni_key] + + ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_1, {"priority": "1", "action": "allow", "terminating": "false", + "src_addr": "192.168.0.1/32,192.168.1.2/30", "dst_addr": "192.168.0.1/32,192.168.1.2/30", "src_port": "0-1", "dst_port": "0-1"}) + # Now that the group contains a rule, expect binding to occur + ctx.asic_eni_table.wait_for_field_match(key=eni_key, expected_fields={sai_stage: acl_group_key}) - self.destroy_ctx(ctx) + # Unbinding should occur immediately + ctx.unbind_acl_in(self.eni_name, ACL_STAGE_1) + ctx.asic_eni_table.wait_for_field_match(key=eni_key, expected_fields={sai_stage: SAI_NULL_OID}) - def test_acl_rule(self, dvs): - ctx = self.create_ctx(dvs) - acl_group1 = "1" - acl_rule1 = "1" - acl_rule2 = "2" + def test_acl_group_binding(self, ctx): + eni_key = ctx.asic_eni_table.get_keys()[0] + sai_stage = get_sai_stage(outbound=False, v4=True, stage_num=ACL_STAGE_2) + + ctx.create_acl_group(ACL_GROUP_2, {"ip_version": "ipv4"}) + acl_group_key = ctx.asic_dash_acl_group_table.wait_for_n_keys(num_keys=1)[0] + ctx.create_acl_rule(ACL_GROUP_2, ACL_RULE_1, {"priority": "1", "action": "allow", "terminating": "false", + "src_addr": "192.168.0.1/32,192.168.1.2/30", "dst_addr": "192.168.0.1/32,192.168.1.2/30", "src_port": "0-1", "dst_port": "0-1"}) + ctx.bind_acl_in(self.eni_name, ACL_STAGE_2, ACL_GROUP_2) + # Binding should occurr immediately since we added a rule to the group prior to binding + ctx.asic_eni_table.wait_for_field_match(key=eni_key, expected_fields={sai_stage: acl_group_key}) + ctx.unbind_acl_in(self.eni_name, ACL_STAGE_2) + ctx.asic_eni_table.wait_for_field_match(key=eni_key, expected_fields={sai_stage: SAI_NULL_OID}) + + def test_acl_rule(self, ctx): # Create acl rule before acl group - ctx.create_acl_rule(acl_group1, acl_rule1, {"priority": "1", "action": "allow", "terminating": "false", + ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_1, {"priority": "1", "action": "allow", "terminating": "false", "src_addr": "192.168.0.1/32,192.168.1.2/30", "dst_addr": "192.168.0.1/32,192.168.1.2/30", "src_port": "0-1", "dst_port": "0-1"}) time.sleep(3) - rule_ids = ctx.asic_dash_acl_rule_table.get_keys() - assert len(rule_ids) == 0 - ctx.create_acl_group(acl_group1, {"ip_version": "ipv4"}) - time.sleep(3) - rule_ids = ctx.asic_dash_acl_rule_table.get_keys() - assert len(rule_ids) == 1 + ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=0) + ctx.create_acl_group(ACL_GROUP_1, {"ip_version": "ipv4"}) - # Create acl without a invalid acl group + ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=1) + + # Create acl rule with nonexistent acl group, which should never get programmed to ASIC_DB ctx.create_acl_rule("0", "0", {"priority": "1", "action": "allow", "terminating": "false", "src_addr": "192.168.0.1/32,192.168.1.2/30", "dst_addr": "192.168.0.1/32,192.168.1.2/30", "src_port": "0-1", "dst_port": "0-1"}) time.sleep(3) - rule_ids = ctx.asic_dash_acl_rule_table.get_keys() - assert len(rule_ids) == 1 + ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=1) # Create acl with invalid attribute - ctx.create_acl_rule(acl_group1, acl_rule2, {"priority": "abc"}) + ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_2, {"priority": "abc"}) time.sleep(3) - rule_ids = ctx.asic_dash_acl_rule_table.get_keys() - assert len(rule_ids) == 1 + ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=1) - # Create acl with multiple step - ctx.create_acl_rule(acl_group1, acl_rule2, {"priority": "1", "action": "allow", "terminating": "false"}) + # Create acl without some mandatory attributes at first + ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_2, {"priority": "1", "action": "allow", "terminating": "false"}) time.sleep(3) - rule_ids = ctx.asic_dash_acl_rule_table.get_keys() - assert len(rule_ids) == 1 - ctx.create_acl_rule(acl_group1, acl_rule2, {"src_addr": "", "dst_addr": "192.168.0.1/32,192.168.1.2/30", "src_port": "", "dst_port": "0-1"}) + ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=1) + # Expect the rule to be created only after all the mandatory attributes are added + ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_2, {"src_addr": "", "dst_addr": "192.168.0.1/32,192.168.1.2/30", "src_port": "", "dst_port": "0-1"}) time.sleep(3) - rule_ids = ctx.asic_dash_acl_rule_table.get_keys() - assert len(rule_ids) == 2 + ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=2) - ctx.remove_acl_rule(acl_group1, acl_rule1) - ctx.remove_acl_rule(acl_group1, acl_rule2) - ctx.remove_acl_group(acl_group1) - time.sleep(3) - assert len(ctx.asic_dash_acl_rule_table.get_keys()) == 0 - assert len(ctx.asic_dash_acl_group_table.get_keys()) == 0 - self.destroy_ctx(ctx) + ctx.remove_acl_rule(ACL_GROUP_1, ACL_RULE_1) + ctx.remove_acl_rule(ACL_GROUP_1, ACL_RULE_2) + ctx.remove_acl_group(ACL_GROUP_1) + ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=0) + ctx.asic_dash_acl_group_table.wait_for_n_keys(num_keys=0) # Add Dummy always-pass test at end as workaroud