From d3d4d1c37aa73552138912bf464b6b0276b37512 Mon Sep 17 00:00:00 2001 From: bingwang Date: Mon, 19 Dec 2022 07:36:58 +0000 Subject: [PATCH 1/2] Support port name in ACL table AttachTo attribute --- src/sonic-config-engine/minigraph.py | 13 ++++++++++--- .../tests/simple-sample-graph-case.xml | 2 +- .../tests/test_minigraph_case.py | 8 ++++++++ 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/sonic-config-engine/minigraph.py b/src/sonic-config-engine/minigraph.py index fd8cc06ca181..45def06146a5 100644 --- a/src/sonic-config-engine/minigraph.py +++ b/src/sonic-config-engine/minigraph.py @@ -622,10 +622,14 @@ def parse_dpg(dpg, hname): acl_intfs.extend(vlans[member]['members']) else: acl_intfs.append(member) - elif member in port_alias_map: - acl_intfs.append(port_alias_map[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) # Give a warning if trying to attach ACL to a LAG member interface, correct way is to attach ACL to the LAG interface - if port_alias_map[member] in intfs_inpc: + if acl_intf 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 member.lower().startswith('erspan') or member.lower().startswith('egress_erspan') or member.lower().startswith('erspan_dscp'): if 'dscp' in member.lower(): @@ -1254,6 +1258,8 @@ def parse_xml(filename, platform=None, port_config_file=None, asic_name=None, hw docker_routing_config_mode = child.text (ports, alias_map, alias_asic_map) = get_port_config(hwsku=hwsku, platform=platform, port_config_file=port_config_file, asic_name=asic_name, hwsku_config_file=hwsku_config_file) + + port_names_map.update(ports) port_alias_map.update(alias_map) port_alias_asic_map.update(alias_asic_map) @@ -1803,6 +1809,7 @@ def parse_asic_meta_get_devices(root): return local_devices +port_names_map = {} port_alias_map = {} port_alias_asic_map = {} diff --git a/src/sonic-config-engine/tests/simple-sample-graph-case.xml b/src/sonic-config-engine/tests/simple-sample-graph-case.xml index acaab2c4c7e3..decea3cba185 100644 --- a/src/sonic-config-engine/tests/simple-sample-graph-case.xml +++ b/src/sonic-config-engine/tests/simple-sample-graph-case.xml @@ -180,7 +180,7 @@ - PortChannel01 + PortChannel01;fortyGigE0/8;Ethernet12 DataAcl DataPlane diff --git a/src/sonic-config-engine/tests/test_minigraph_case.py b/src/sonic-config-engine/tests/test_minigraph_case.py index 2f7a74bf3ed0..5b250d304f76 100644 --- a/src/sonic-config-engine/tests/test_minigraph_case.py +++ b/src/sonic-config-engine/tests/test_minigraph_case.py @@ -456,6 +456,14 @@ def test_minigraph_mirror_dscp(self): expected_ports.sort() ) + 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()) + def test_parse_device_desc_xml_mgmt_interface(self): # Regular device_desc.xml with both IPv4 and IPv6 mgmt address result = minigraph.parse_device_desc_xml(self.sample_simple_device_desc) From c184b46afd1aee67a071ea73a7bb29a2ec7545f6 Mon Sep 17 00:00:00 2001 From: bingwang-ms <66248323+bingwang-ms@users.noreply.github.com> Date: Fri, 20 Jan 2023 10:11:39 -0800 Subject: [PATCH 2/2] Support both port name and alias in ACL table `AttachTo` attribute (#13444) Why I did it This PR is an enhancement of PR #13105 Because the input string of AttachTo for ACL table can appear in both port name group and port alias group, I added a logic to determine whether the string should be port name or port alias If all the input strings belong to port name group, then we treat all of them as port name If all the input strings belong to port alias, then we treat all of them as port alias If all the input string belongs to both port alias group and port name group, we prefer port alias. The behavior is as before. How I did it Walk through all port names/alias in the input to make a decision. How to verify it Verified by adding UT. --- src/sonic-config-engine/minigraph.py | 48 +- .../simple-sample-graph-case-acl-test.xml | 974 ++++++++++++++++++ .../tests/simple-sample-graph-case.xml | 2 +- ...mple-port-config-duplicated-name-alias.ini | 36 + .../tests/test_minigraph_case.py | 21 +- 5 files changed, 1070 insertions(+), 11 deletions(-) create mode 100644 src/sonic-config-engine/tests/simple-sample-graph-case-acl-test.xml create mode 100644 src/sonic-config-engine/tests/t0-sample-port-config-duplicated-name-alias.ini diff --git a/src/sonic-config-engine/minigraph.py b/src/sonic-config-engine/minigraph.py index 45def06146a5..9d8dd6e3c00f 100644 --- a/src/sonic-config-engine/minigraph.py +++ b/src/sonic-config-engine/minigraph.py @@ -605,6 +605,40 @@ 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 + # For CTRLPLANE ACL, both counters are 0 + if (port_alias_count != 0) and (port_name_count != 0): + 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 @@ -622,15 +656,15 @@ def parse_dpg(dpg, hname): acl_intfs.extend(vlans[member]['members']) 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 diff --git a/src/sonic-config-engine/tests/simple-sample-graph-case-acl-test.xml b/src/sonic-config-engine/tests/simple-sample-graph-case-acl-test.xml new file mode 100644 index 000000000000..13dbbd17a234 --- /dev/null +++ b/src/sonic-config-engine/tests/simple-sample-graph-case-acl-test.xml @@ -0,0 +1,974 @@ + + + + + + false + switch-t0 + 10.0.0.56 + ARISTA01T1 + 10.0.0.57 + 1 + 180 + 60 + + + switch-t0 + FC00::71 + ARISTA01T1 + FC00::72 + 1 + 180 + 60 + + + false + switch-t0 + 10.0.0.58 + ARISTA02T1 + 10.0.0.59 + 1 + 180 + 60 + + + switch-t0 + FC00::75 + ARISTA02T1 + FC00::76 + 1 + 180 + 60 + + + + + 65100 + switch-t0 + + +
10.0.0.57
+ + + + + +
10.0.0.59
+ + + + + + + + + 64600 + ARISTA01T1 + + + + 64600 + ARISTA02T1 + + + + 64600 + ARISTA03T1 + + + + 64600 + ARISTA04T1 + + + + + + + + + + HostIP + Loopback0 + + 10.1.0.32/32 + + 10.1.0.32/32 + + + HostIP1 + Loopback0 + + FC00:1::32/128 + + FC00:1::32/128 + + + + + HostIP + eth0 + + 10.0.0.100/24 + + 10.0.0.100/24 + + + + + + + switch-t0 + + + PortChannel01 + fortyGigE0/4 + + + + + + + + + ab1 + fortyGigE0/8 + 192.0.0.1;192.0.0.2 + fc02:2000::1;fc02:2000::2 + 1000 + 1000 + 192.168.0.0/27 + 00:aa:bb:cc:dd:ee + + + ab2 + fortyGigE0/4 + 192.0.0.1 + fc02:2000::3;fc02:2000::4 + 2000 + 2000 + + + + + + + PortChannel01 + 10.0.0.56/31 + + + + PortChannel01 + FC00::71/126 + + + + fortyGigE0/0 + 10.0.0.58/31 + + + + fortyGigE0/0 + FC00::75/126 + + + + ab1 + 192.168.0.1/27 + + + + + + PortChannel01;Ethernet20;Ethernet24 + DataAcl_port_name + DataPlane + + + PortChannel01;Ethernet6/1;Ethernet7/1 + DataAcl_port_alias + DataPlane + + + PortChannel01;Ethernet0;Ethernet1;Ethernet2;Ethernet3 + DataAcl_mixed_name_alias_1 + DataPlane + + + PortChannel01;Ethernet1;Ethernet2;Ethernet6/1;Ethernet7/1 + DataAcl_mixed_name_alias_2 + DataPlane + + + Ethernet1 + DataAcl_mixed_name_alias_3 + DataPlane + + + SNMP + SNMP_ACL + SNMP + + + ERSPAN_DSCP + Everflow_dscp + Everflow_dscp + + + + + + + + + + LoopbackInterface + HostIP + Loopback0 + + 10.10.10.2/32 + + 10.10.10.2/32 + + + LoopbackInterface + HostIP1 + Loopback0 + + fe80::0002/128 + + fe80::0002/128 + + + LoopbackInterface + SoCHostIP0 + server2SOC + + 10.10.10.3/32 + + 10.10.10.3/32 + + + LoopbackInterface + SoCHostIP1 + server2SOC + + fe80::0003/128 + + fe80::0003/128 + + + + + + + + server2 + + + + + + + + + + + + + + + DeviceSerialLink + 9600 + switch-t0 + console + true + switch-t1 + 1 + + + DeviceSerialLink + 9600 + switch-t0 + 1 + true + managed_device + console + + + DeviceInterfaceLink + 10000 + switch-t0 + fortyGigE0/0 + switch-01t1 + port1 + + + DeviceInterfaceLink + 10000 + switch-t0 + fortyGigE0/12 + switch-02t1 + port1 + + + DeviceInterfaceLink + 25000 + switch-t0 + fortyGigE0/4 + server1 + port1 + + + DeviceInterfaceLink + 40000 + switch-t0 + fortyGigE0/8 + server2 + port1 + + + LogicalLink + 10000 + false + switch-t0 + fortyGigE0/4 + true + server1-SC + L + true + + + LogicalLink + 0 + false + switch-t0 + MuxTunnel0 + false + switch2-t0 + MuxTunnel0 + true + + + + + ToRRouter +
+ 26.1.1.10/32 +
+ switch-t0 + Force10-S6000 + AAA00PrdStr00 + + +
+ 25.1.1.10/32 +
+ + 10.7.0.196/26 + + switch2-t0 + Force10-S6000 + + + switch-01t1 +
+ 10.1.0.186/32 +
+ 2 + + + 10.7.0.196/26 + + Force10-S6000 + + + SmartCable +
+ 0.0.0.0/0 +
+ + ::/0 + + + 0.0.0.0/0 + + + ::/0 + + + server1-SC + smartcable-sku + + + Server +
+ 10.10.10.1/32 +
+ + fe80::0001/80 + + + 10.0.0.1/32 + + server1 + server-sku + + + Server +
+ 10.10.10.2/32 +
+ + fe80::0002/128 + + + 10.0.0.2/32 + + server2 + server-sku + + + + + + + + + + GeminiPeeringLink + + True + + + UpperTOR + + switch-t0 + + + LowerTOR + + switch2-t0 + + + switch2-t0:MuxTunnel0;switch-t0:MuxTunnel0 + + + + + + AutoNegotiation + + True + + + switch-01t1:port1;switch-t0:fortyGigE0/0 + + + + + + AutoNegotiation + + True + + + switch-02t1:port1;switch-t0:fortyGigE0/12 + + + + + + AutoNegotiation + + True + + + server1:port1;switch-t0:fortyGigE0/4 + + + + + + AutoNegotiation + + True + + + server2:port1;switch-t0:fortyGigE0/8 + + + + + + + switch-t0 + + + DeploymentId + + 1 + + + ErspanDestinationIpv4 + + 10.0.100.1 + + + NtpResources + + 10.0.10.1;10.0.10.2 + + + + SnmpResources + + 10.0.10.3;10.0.10.4 + + + + SyslogResources + + 10.0.10.5;10.0.10.6 + + + + TacacsServer + + 10.0.10.7;10.0.10.8 + + + KubernetesEnabled + + 0 + + + KubernetesServerIp + + 10.10.10.10 + + + ResourceType + + Storage + + + RedundancyType + + Mixed + + + + + + + + + + + DeviceInterface + + true + 1 + fortyGigE0/0 + + false + 0 + 0 + 10000 + + + DeviceInterface + + true + 1 + fortyGigE0/4 + + false + 0 + 0 + 25000 + + + DeviceInterface + + true + 1 + fortyGigE0/8 + + false + 0 + 0 + 40000 + Interface description + + + DeviceInterface + + true + 1 + fortyGigE0/12 + + false + 0 + 0 + 100000 + Interface description + + + DeviceInterface + + true + 1 + fortyGigE0/16 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/20 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/24 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/28 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/32 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/36 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/40 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/44 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/48 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/52 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/56 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/60 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/64 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/68 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/72 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/76 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/80 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/84 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/88 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/92 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/96 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/100 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/104 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/108 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/112 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/116 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/120 + + false + 0 + 0 + 100000 + + + DeviceInterface + + true + 1 + fortyGigE0/124 + + false + 0 + 0 + 100000 + + + true + 0 + Force10-S6000 + + + DeviceInterface + + true + 1 + eth0 + false + eth0 + 1000 + + + + + switch-t0 + Force10-S6000 + diff --git a/src/sonic-config-engine/tests/simple-sample-graph-case.xml b/src/sonic-config-engine/tests/simple-sample-graph-case.xml index decea3cba185..9e15e1a0c486 100644 --- a/src/sonic-config-engine/tests/simple-sample-graph-case.xml +++ b/src/sonic-config-engine/tests/simple-sample-graph-case.xml @@ -180,7 +180,7 @@ - PortChannel01;fortyGigE0/8;Ethernet12 + PortChannel01;Ethernet0;Ethernet12 DataAcl DataPlane diff --git a/src/sonic-config-engine/tests/t0-sample-port-config-duplicated-name-alias.ini b/src/sonic-config-engine/tests/t0-sample-port-config-duplicated-name-alias.ini new file mode 100644 index 000000000000..dafdb570e9cc --- /dev/null +++ b/src/sonic-config-engine/tests/t0-sample-port-config-duplicated-name-alias.ini @@ -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 diff --git a/src/sonic-config-engine/tests/test_minigraph_case.py b/src/sonic-config-engine/tests/test_minigraph_case.py index 5b250d304f76..bd2151459fad 100644 --- a/src/sonic-config-engine/tests/test_minigraph_case.py +++ b/src/sonic-config-engine/tests/test_minigraph_case.py @@ -460,9 +460,24 @@ 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()) + sample_graph = os.path.join(self.test_dir,'simple-sample-graph-case-acl-test.xml') + port_config_duplicated_name_alias = os.path.join(self.test_dir, 't0-sample-port-config-duplicated-name-alias.ini') + result = minigraph.parse_xml(sample_graph, port_config_file=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 all fall 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 all fall 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