Skip to content

Commit

Permalink
[acl-loader]: do not add default deny rule for egress acl (#1531)
Browse files Browse the repository at this point in the history 
* [acl-loader]: do not add default deny rule for egress acl

Signed-off-by: Guohan Lu <lguohan@gmail.com>
  • Loading branch information
@lguohan
lguohan committed Mar 29, 2021
1 parent 4d89510 commit 28b64ec
Show file tree
Hide file tree
Showing 5 changed files with 180 additions and 30 deletions.
50 changes: 29 additions & 21 deletions acl_loader/main.py
View file
Expand Up @@ -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()
Expand Down Expand Up @@ -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]
Expand All @@ -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]
Expand All @@ -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"] = {}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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])

Expand All @@ -715,22 +723,22 @@ 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)

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])
Expand All @@ -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.
Expand Down
110 changes: 110 additions & 0 deletions tests/acl_input/acl_egress.json
View file
@@ -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"
}
}
}
}
}
}
}
}
}
}
22 changes: 22 additions & 0 deletions tests/acl_loader_test.py
View file
Expand Up @@ -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 = {}
Expand Down
2 changes: 1 addition & 1 deletion tests/aclshow_test.py
View file
Expand Up @@ -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
Expand Down
26 changes: 18 additions & 8 deletions tests/mock_tables/config_db.json
View file
Expand Up @@ -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",
Expand Down Expand Up @@ -1489,21 +1501,19 @@
"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": {
"rrclient": "0",
"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"
}


}

0 comments on commit 28b64ec

Please sign in to comment.