From 28b64ecb56a15eebba3106900487c0b843eb03b8 Mon Sep 17 00:00:00 2001 From: lguohan Date: Sun, 28 Mar 2021 22:20:30 -0700 Subject: [PATCH] [acl-loader]: do not add default deny rule for egress acl (#1531) * [acl-loader]: do not add default deny rule for egress acl Signed-off-by: Guohan Lu --- acl_loader/main.py | 50 ++++++++------ tests/acl_input/acl_egress.json | 110 +++++++++++++++++++++++++++++++ tests/acl_loader_test.py | 22 +++++++ tests/aclshow_test.py | 2 +- tests/mock_tables/config_db.json | 26 +++++--- 5 files changed, 180 insertions(+), 30 deletions(-) create mode 100644 tests/acl_input/acl_egress.json diff --git a/acl_loader/main.py b/acl_loader/main.py index 91cb750c3318..bb09e20487a4 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -132,27 +132,27 @@ def __init__(self): # Global namespace will be used for Control plane ACL which are via IPTables. # Per ASIC namespace will be used for Data and Everflow ACL's. # Global Configdb will have all ACL information for both Ctrl and Data/Evereflow ACL's - # and will be used as souurce of truth for ACL modification to config DB which will be done to both Global DB and + # and will be used as souurce of truth for ACL modification to config DB which will be done to both Global DB and # front asic namespace - + self.per_npu_configdb = {} # State DB are used for to get mirror Session monitor port. # For multi-npu platforms each asic namespace can have different monitor port # dependinding on which route to session destination ip. So for multi-npu - # platforms we get state db for all front asic namespace in addition to - + # platforms we get state db for all front asic namespace in addition to + self.per_npu_statedb = {} # Getting all front asic namespace and correspding config and state DB connector - + namespaces = device_info.get_all_namespaces() for front_asic_namespaces in namespaces['front_ns']: self.per_npu_configdb[front_asic_namespaces] = ConfigDBConnector(use_unix_socket_path=True, namespace=front_asic_namespaces) self.per_npu_configdb[front_asic_namespaces].connect() self.per_npu_statedb[front_asic_namespaces] = SonicV2Connector(use_unix_socket_path=True, namespace=front_asic_namespaces) self.per_npu_statedb[front_asic_namespaces].connect(self.per_npu_statedb[front_asic_namespaces].STATE_DB) - + self.read_tables_info() self.read_rules_info() self.read_sessions_info() @@ -183,8 +183,8 @@ def read_policers_info(self): Read POLICER table from configuration database :return: """ - - # For multi-npu platforms we will read from any one of front asic namespace + + # For multi-npu platforms we will read from any one of front asic namespace # config db as the information should be same across all config db if self.per_npu_configdb: namespace_configdb = list(self.per_npu_configdb.values())[0] @@ -201,7 +201,7 @@ def read_sessions_info(self): :return: """ - # For multi-npu platforms we will read from any one of front asic namespace + # For multi-npu platforms we will read from any one of front asic namespace # config db as the information should be same across all config db if self.per_npu_configdb: namespace_configdb = list(self.per_npu_configdb.values())[0] @@ -210,8 +210,8 @@ def read_sessions_info(self): self.sessions_db_info = self.configdb.get_table(self.CFG_MIRROR_SESSION_TABLE) for key in self.sessions_db_info: if self.per_npu_statedb: - # For multi-npu platforms we will read from all front asic name space - # statedb as the monitor port will be differnt for each asic + # For multi-npu platforms we will read from all front asic name space + # statedb as the monitor port will be differnt for each asic # and it's status also might be different (ideally should not happen) # We will store them as dict of 'asic' : value self.sessions_db_info[key]["status"] = {} @@ -283,6 +283,14 @@ def set_max_priority(self, priority): def is_table_valid(self, tname): return self.tables_db_info.get(tname) + def is_table_egress(self, tname): + """ + Check if ACL table stage is egress + :param tname: ACL table name + :return: True if table type is Egress + """ + return self.tables_db_info[tname].get("stage", Stage.INGRESS).upper() == Stage.EGRESS + def is_table_mirror(self, tname): """ Check if ACL table type is ACL_TABLE_TYPE_MIRROR or ACL_TABLE_TYPE_MIRRORV6 @@ -377,12 +385,12 @@ def validate_actions(self, table_name, action_props): # check if per npu state db is there then read using first state db # else read from global statedb if self.per_npu_statedb: - # For multi-npu we will read using anyone statedb connector for front asic namespace. - # Same information should be there in all state DB's + # For multi-npu we will read using anyone statedb connector for front asic namespace. + # Same information should be there in all state DB's # as it is static information about switch capability namespace_statedb = list(self.per_npu_statedb.values())[0] capability = namespace_statedb.get_all(self.statedb.STATE_DB, "{}|switch".format(self.SWITCH_CAPABILITY_TABLE)) - else: + else: capability = self.statedb.get_all(self.statedb.STATE_DB, "{}|switch".format(self.SWITCH_CAPABILITY_TABLE)) for action_key in dict(action_props): key = "{}|{}".format(self.ACL_ACTIONS_CAPABILITY_FIELD, stage.upper()) @@ -636,7 +644,7 @@ def convert_rules(self): except AclLoaderException as ex: error("Error processing rule %s: %s. Skipped." % (acl_entry_name, ex)) - if not self.is_table_mirror(table_name): + if not self.is_table_mirror(table_name) and not self.is_table_egress(table_name): deep_update(self.rules_info, self.deny_rule(table_name)) def full_update(self): @@ -705,7 +713,7 @@ def incremental_update(self): # Add all new dataplane rules for key in new_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. + # Program for per-asic namespace corresponding to front asic also if present. for namespace_configdb in self.per_npu_configdb.values(): namespace_configdb.mod_entry(self.ACL_RULE, key, self.rules_info[key]) @@ -715,14 +723,14 @@ def incremental_update(self): for key in added_controlplane_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. + # 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 removed_controlplane_rules: self.configdb.mod_entry(self.ACL_RULE, key, None) - # Program for per-asic namespace corresponding to front asic also if present. + # 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, None) @@ -730,7 +738,7 @@ def incremental_update(self): for key in existing_controlplane_rules: if cmp(self.rules_info[key], self.rules_db_info[key]) != 0: self.configdb.set_entry(self.ACL_RULE, key, self.rules_info[key]) - # Program for per-asic namespace corresponding to front asic also if present. + # 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]) @@ -745,10 +753,10 @@ def delete(self, table=None, rule=None): if not table or table == key[0]: if not rule or rule == key[1]: self.configdb.set_entry(self.ACL_RULE, key, None) - # Program for per-asic namespace corresponding to front asic also if present. + # Program for per-asic namespace corresponding to front asic also if present. for namespace_configdb in self.per_npu_configdb.values(): namespace_configdb.set_entry(self.ACL_RULE, key, None) - + def show_table(self, table_name): """ Show ACL table configuration. diff --git a/tests/acl_input/acl_egress.json b/tests/acl_input/acl_egress.json new file mode 100644 index 000000000000..ed0f5900787f --- /dev/null +++ b/tests/acl_input/acl_egress.json @@ -0,0 +1,110 @@ +{ + "acl": { + "acl-sets": { + "acl-set": { + "DATAACL_3": { + "acl-entries": { + "acl-entry": { + "1": { + "config": { + "sequence-id": 1 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "ip": { + "config": { + "protocol": "IP_ICMP", + "source-ip-address": "20.0.0.2/32", + "destination-ip-address": "30.0.0.3/32" + } + }, + "icmp": { + "config": { + "type": "3", + "code": "0" + } + } + }, + "2": { + "config": { + "sequence-id": 2 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "l2": { + "config": { + "vlan-id": "369" + } + }, + "ip": { + "config": { + "protocol": "IP_TCP", + "source-ip-address": "20.0.0.2/32", + "destination-ip-address": "30.0.0.3/32" + } + } + } + } + } + }, + "DATAACL_4": { + "acl-entries": { + "acl-entry": { + "1": { + "config": { + "sequence-id": 1 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "ip": { + "config": { + "protocol": "IP_ICMP", + "source-ip-address": "::1/128", + "destination-ip-address": "::1/128" + } + }, + "icmp": { + "config": { + "type": "1", + "code": "0" + } + } + }, + "2": { + "config": { + "sequence-id": 100 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "ip": { + "config": { + "protocol": "IP_ICMP", + "source-ip-address": "::1/128", + "destination-ip-address": "::1/128" + } + }, + "icmp": { + "config": { + "type": "128" + } + } + } + } + } + } + } + } + } +} diff --git a/tests/acl_loader_test.py b/tests/acl_loader_test.py index e2ee414a5037..ee5ba65ed1eb 100644 --- a/tests/acl_loader_test.py +++ b/tests/acl_loader_test.py @@ -118,6 +118,28 @@ def test_icmpv6_translation(self, acl_loader): "PRIORITY": "9900" } + def test_ingress_default_deny_rule(self, acl_loader): + acl_loader.rules_info = {} + acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/acl1.json')) + print(acl_loader.rules_info) + assert acl_loader.rules_info[('DATAACL', 'DEFAULT_RULE')] == { + 'PRIORITY': '1', + 'PACKET_ACTION': 'DROP', + 'ETHER_TYPE': '2048' + } + assert acl_loader.rules_info[('DATAACL_2', 'DEFAULT_RULE')] == { + 'PRIORITY': '1', + 'PACKET_ACTION': 'DROP', + 'IP_TYPE': 'IPV6ANY' + } + + def test_egress_no_default_deny_rule(self, acl_loader): + acl_loader.rules_info = {} + acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/acl_egress.json')) + print(acl_loader.rules_info) + assert ('DATAACL_3', 'DEFAULT_RULE') not in acl_loader.rules_info + assert ('DATAACL_4', 'DEFAULT_RULE') not in acl_loader.rules_info + def test_icmp_type_lower_bound(self, acl_loader): with pytest.raises(ValueError): acl_loader.rules_info = {} diff --git a/tests/aclshow_test.py b/tests/aclshow_test.py index a2c122ddd7f3..b2371e972398 100644 --- a/tests/aclshow_test.py +++ b/tests/aclshow_test.py @@ -78,7 +78,7 @@ # Expected output for aclshow -r RULE_4,RULE_6 -vv rule4_rule6_verbose_output = '' + \ """Reading ACL info... -Total number of ACL Tables: 6 +Total number of ACL Tables: 8 Total number of ACL Rules: 11 RULE NAME TABLE NAME PRIO PACKETS COUNT BYTES COUNT diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index 051c49004d6e..f8ceebffbf38 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -405,6 +405,18 @@ "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet64,Ethernet68,Ethernet72,Ethernet76,Ethernet80,Ethernet84,Ethernet88,Ethernet92,Ethernet96,Ethernet100,Ethernet104,Ethernet108,Ethernet112,Ethernet116,Ethernet120,Ethernet124", "type": "L3V6" }, + "ACL_TABLE|DATAACL_3": { + "policy_desc": "DATAACL_3", + "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet64,Ethernet68,Ethernet72,Ethernet76,Ethernet80,Ethernet84,Ethernet88,Ethernet92,Ethernet96,Ethernet100,Ethernet104,Ethernet108,Ethernet112,Ethernet116,Ethernet120,Ethernet124", + "type": "L3", + "stage": "egress" + }, + "ACL_TABLE|DATAACL_4": { + "policy_desc": "DATAACL_4", + "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet64,Ethernet68,Ethernet72,Ethernet76,Ethernet80,Ethernet84,Ethernet88,Ethernet92,Ethernet96,Ethernet100,Ethernet104,Ethernet108,Ethernet112,Ethernet116,Ethernet120,Ethernet124", + "type": "L3V6", + "stage": "egress" + }, "ACL_TABLE|EVERFLOW": { "policy_desc": "EVERFLOW", "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet100,Ethernet104,Ethernet92,Ethernet96,Ethernet84,Ethernet88,Ethernet76,Ethernet80,Ethernet108,Ethernet112,Ethernet64,Ethernet120,Ethernet116,Ethernet124,Ethernet72,Ethernet68", @@ -1489,9 +1501,9 @@ "name": "T2-Peer", "local_addr": "20.1.1.1", "nhopself": "0", - "admin_status": "up", - "holdtime": "10", - "asn": "65200", + "admin_status": "up", + "holdtime": "10", + "asn": "65200", "keepalive": "3" }, "BGP_NEIGHBOR|30.1.1.5": { @@ -1499,11 +1511,9 @@ "name": "T0-Peer", "local_addr": "30.1.1.1", "nhopself": "0", - "admin_status": "up", - "holdtime": "10", - "asn": "65200", + "admin_status": "up", + "holdtime": "10", + "asn": "65200", "keepalive": "3" } - - }