Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support both port name and alias in ACL table AttachTo attribute #13444

Merged
merged 5 commits into from
Jan 20, 2023
Merged

Support both port name and alias in ACL table AttachTo attribute #13444

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7f9db69
Support both port name and alias in ACL table AttachTo attribute
bingwang-ms Jan 19, 2023
48c5d89
All attachto string must be port name or alias
bingwang-ms Jan 19, 2023
1ee9424
Optimise logic
bingwang-ms Jan 19, 2023
85a8466
Fix LGTM
bingwang-ms Jan 19, 2023
b10bf97
Fix UT
bingwang-ms Jan 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 39 additions & 7 deletions src/sonic-config-engine/minigraph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,38 @@ def parse_dpg(dpg, hname):
is_mirror = False
is_mirror_v6 = False
is_mirror_dscp = False
use_port_alias = True

# Walk through all interface names/alias to determine whether the input string is
# port name or alias.We need this logic because there can be duplicaitons in port alias
# and port names
# The input port name/alias can be either port_name or port_alias. A mix of name and alias is not accepted
port_name_count = 0
port_alias_count = 0
total_count = 0
for member in aclattach:
member = member.strip()
if member in pcs or \
member in vlans or \
member.lower().startswith('erspan') or \
member.lower().startswith('egress_erspan') or \
member.lower().startswith('erspan_dscp'):
continue
total_count += 1
if member in port_alias_map:
port_alias_count += 1
if member in port_names_map:
port_name_count += 1
# All inputs are port alias
if port_alias_count == total_count:
use_port_alias = True
# All inputs are port name
elif port_name_count == total_count:
use_port_alias = False
# There are both port alias and port name, then port alias is preferred to keep the behavior not changed
else:
use_port_alias = True
print("Warning: The given port name for ACL " + aclname + " is inconsistent. It must be either port name or alias ", file=sys.stderr)

# TODO: Ensure that acl_intfs will only ever contain front-panel interfaces (e.g.,
# maybe we should explicity ignore management and loopback interfaces?) because we
Expand All @@ -674,15 +706,15 @@ def parse_dpg(dpg, hname):
acl_intfs.extend(vlan_member_list[member])
else:
acl_intfs.append(member)
elif (member in port_alias_map) or (member in port_names_map):
if member in port_alias_map:
acl_intf = port_alias_map[member]
else:
acl_intf = member
acl_intfs.append(acl_intf)
elif use_port_alias and (member in port_alias_map):
acl_intfs.append(port_alias_map[member])
# Give a warning if trying to attach ACL to a LAG member interface, correct way is to attach ACL to the LAG interface
if acl_intf in intfs_inpc:
if port_alias_map[member] in intfs_inpc:
print("Warning: ACL " + aclname + " is attached to a LAG member interface " + port_alias_map[member] + ", instead of LAG interface", file=sys.stderr)
elif (not use_port_alias) and (member in port_names_map):
acl_intfs.append(member)
if member in intfs_inpc:
print("Warning: ACL " + aclname + " is attached to a LAG member interface " + member + ", instead of LAG interface", file=sys.stderr)
elif member.lower().startswith('erspan') or member.lower().startswith('egress_erspan') or member.lower().startswith('erspan_dscp'):
if 'dscp' in member.lower():
is_mirror_dscp = True
Expand Down
24 changes: 22 additions & 2 deletions src/sonic-config-engine/tests/simple-sample-graph-case.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,28 @@
<DataAcls/>
<AclInterfaces>
<AclInterface>
<AttachTo>PortChannel01;fortyGigE0/8;Ethernet12</AttachTo>
<InAcl>DataAcl</InAcl>
<AttachTo>PortChannel01;Ethernet20;Ethernet24</AttachTo>
<InAcl>DataAcl_port_name</InAcl>
<Type>DataPlane</Type>
</AclInterface>
<AclInterface>
<AttachTo>PortChannel01;Ethernet6/1;Ethernet7/1</AttachTo>
<InAcl>DataAcl_port_alias</InAcl>
<Type>DataPlane</Type>
</AclInterface>
<AclInterface>
<AttachTo>PortChannel01;Ethernet0;Ethernet1;Ethernet2;Ethernet3</AttachTo>
<InAcl>DataAcl_mixed_name_alias_1</InAcl>
<Type>DataPlane</Type>
</AclInterface>
<AclInterface>
<AttachTo>PortChannel01;Ethernet1;Ethernet2;Ethernet6/1;Ethernet7/1</AttachTo>
<InAcl>DataAcl_mixed_name_alias_2</InAcl>
<Type>DataPlane</Type>
</AclInterface>
<AclInterface>
<AttachTo>Ethernet1</AttachTo>
<InAcl>DataAcl_mixed_name_alias_3</InAcl>
<Type>DataPlane</Type>
</AclInterface>
<AclInterface>
Expand Down
36 changes: 36 additions & 0 deletions src/sonic-config-engine/tests/t0-sample-port-config-duplicated-name-alias.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# name lanes alias index
Ethernet0 9 Ethernet1 1
Ethernet1 10 Ethernet2 2
Ethernet2 11 Ethernet3 3
Ethernet3 12 Ethernet4 4
Ethernet4 13,14,15,16 Ethernet6/1 6
Ethernet8 17,18,19,20 Ethernet7/1 7
Ethernet12 21,22,23,24 Ethernet8/1 8
Ethernet16 29,30,31,32 Ethernet9/1 9
Ethernet20 25,26,27,28 Ethernet10/1 10
Ethernet24 33,34,35,36 Ethernet11/1 11
Ethernet28 37,38,39,40 Ethernet12/1 12
Ethernet32 45,46,47,48 Ethernet13/1 13
Ethernet36 41,42,43,44 Ethernet14/1 14
Ethernet40 49,50,51,52 Ethernet15/1 15
Ethernet44 53,54,55,56 Ethernet16/1 16
Ethernet48 69,70,71,72 Ethernet17/1 17
Ethernet52 65,66,67,68 Ethernet18/1 18
Ethernet56 73,74,75,76 Ethernet19/1 19
Ethernet60 77,78,79,80 Ethernet20/1 20
Ethernet64 93,94,95,96 Ethernet21/1 21
Ethernet68 89,90,91,92 Ethernet22/1 22
Ethernet72 97,98,99,100 Ethernet23/1 23
Ethernet76 101,102,103,104 Ethernet24/1 24
Ethernet80 109,110,111,112 Ethernet25/1 25
Ethernet84 105,106,107,108 Ethernet26/1 26
Ethernet88 121,122,123,124 Ethernet27/1 27
Ethernet92 125,126,127,128 Ethernet28/1 28
Ethernet96 61,62,63,64 Ethernet29 29
Ethernet100 57,58,59,60 Ethernet30 30
Ethernet104 81,82,83,84 Ethernet31 31
Ethernet108 85,86,87,88 Ethernet32 32
Ethernet112 117,118,119,120 Ethernet33 33
Ethernet116 113,114,115,116 Ethernet34 34
Ethernet120 1,2,3,4 Ethernet35 35
Ethernet124 5,6,7,8 Ethernet36 36
20 changes: 17 additions & 3 deletions src/sonic-config-engine/tests/test_minigraph_case.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ def setUp(self):
self.sample_simple_device_desc = os.path.join(self.test_dir, 'simple-sample-device-desc.xml')
self.sample_simple_device_desc_ipv6_only = os.path.join(self.test_dir, 'simple-sample-device-desc-ipv6-only.xml')
self.port_config = os.path.join(self.test_dir, 't0-sample-port-config.ini')
self.port_config_duplicated_name_alias = os.path.join(self.test_dir, 't0-sample-port-config-duplicated-name-alias.ini')

def run_script(self, argument, check_stderr=False):
print('\n Running sonic-cfggen ' + ' '.join(argument))
Expand Down Expand Up @@ -471,9 +472,22 @@ def test_minigraph_acl_attach_to_ports(self):
"""
The test case is to verify ACL table can be bound to both port names and alias
"""
result = minigraph.parse_xml(self.sample_graph, port_config_file=self.port_config)
expected_dataacl_ports = ['PortChannel01','fortyGigE0/8','Ethernet12']
self.assertEqual(result['ACL_TABLE']['DATAACL']['ports'].sort(), expected_dataacl_ports.sort())
result = minigraph.parse_xml(self.sample_graph, port_config_file=self.port_config_duplicated_name_alias)
# TC1: All ports are portchannels or port names
expected_dataacl_ports = ['PortChannel01','Ethernet20','Ethernet24']
self.assertEqual(sorted(result['ACL_TABLE']['DATAACL_PORT_NAME']['ports']), sorted(expected_dataacl_ports))
# TC2: All ports are portchanels or port alias
expected_dataacl_ports = ['PortChannel01','Ethernet4','Ethernet8']
self.assertEqual(sorted(result['ACL_TABLE']['DATAACL_PORT_ALIAS']['ports']), sorted(expected_dataacl_ports))
# TC3: Duplicated values in port names and alias, but more falls in port names
expected_dataacl_ports = ['PortChannel01','Ethernet0','Ethernet1','Ethernet2','Ethernet3']
self.assertEqual(sorted(result['ACL_TABLE']['DATAACL_MIXED_NAME_ALIAS_1']['ports']), sorted(expected_dataacl_ports))
# TC4: Duplicated values in port names and alias, but more falls in port alias
expected_dataacl_ports = ['PortChannel01','Ethernet0','Ethernet1','Ethernet4','Ethernet8']
self.assertEqual(sorted(result['ACL_TABLE']['DATAACL_MIXED_NAME_ALIAS_2']['ports']), sorted(expected_dataacl_ports))
# TC5: Same count in port names and alias, port alias is preferred
expected_dataacl_ports = ['Ethernet0']
self.assertEqual(sorted(result['ACL_TABLE']['DATAACL_MIXED_NAME_ALIAS_3']['ports']), sorted(expected_dataacl_ports))

def test_parse_device_desc_xml_mgmt_interface(self):
# Regular device_desc.xml with both IPv4 and IPv6 mgmt address
Expand Down