[ACL][VLAN] Add support for ingress and egress ACLs on router interfaces/SVI #1218

jleveque · 2020-03-06T00:56:35Z

Support attaching ingress and egress ACLs to router interfaces/switch virtual interfaces

…ces/SVI

prsunny · 2020-03-06T01:23:00Z

orchagent/portsorch.cpp

-                bind_type = SAI_ACL_BIND_POINT_TYPE_VLAN;
+                if (port.m_rif_id)
+                {
+                    bind_type = SAI_ACL_BIND_POINT_TYPE_ROUTER_INTERFACE;


Why is RIF accounted only for Vlan. I mean Port and Lag (cases above) can also have corresponding RIF.

This PR is only meant to support VLAN RIFs.

prsunny · 2020-03-06T02:22:57Z

orchagent/portsorch.cpp

@@ -479,7 +480,8 @@ bool PortsOrch::getPort(sai_object_id_t id, Port &port)
            }
            break;
        case Port::VLAN:
-            if (portIter.second.m_vlan_info.vlan_oid == id)
+            if (portIter.second.m_vlan_info.vlan_oid == id ||


I don't see a new getPort usage in this PR. So where is it called with rif_id as id? I think this change may not be required

getPort() is called in getAclBindPortId() to get the port information.

getAclBindPortId() invokes getPort(alias, port) with alias. Its not this function.

prsunny · 2020-03-06T02:29:38Z

orchagent/portsorch.cpp

-                bind_type = SAI_ACL_BIND_POINT_TYPE_VLAN;
+                if (port.m_rif_id)
+                {
+                    bind_type = SAI_ACL_BIND_POINT_TYPE_ROUTER_INTERFACE;


With this change, if it is a Vlan router interface, it is always BIND_TYPE_ROUTER_INTERFACE. This is a behavioral change from existing flow and not sure if it breaks for those who use VLAN binding currently (In most cases, Vlan is a RIF). Do we have a case where the bind_type can be TYPE_VLAN even if it has a RIF association? @daall

This is also concerning because there are platforms that have full (e.g. ingress and egress) support for VLAN ACLs but not RIF ACLs. This change could break those devices that you're describing.

daall

We need to have some discussion and figure out how to address 1) the table bind points and 2) the behavior for binding interfaces that are both VLANs and RIFs.

daall · 2020-03-30T16:50:23Z

orchagent/aclorch.cpp

@@ -1295,7 +1295,7 @@ bool AclTable::create()
    }
    else
    {
-        bpoint_list = { SAI_ACL_BIND_POINT_TYPE_PORT, SAI_ACL_BIND_POINT_TYPE_LAG };
+        bpoint_list = { SAI_ACL_BIND_POINT_TYPE_PORT, SAI_ACL_BIND_POINT_TYPE_LAG, SAI_ACL_BIND_POINT_TYPE_ROUTER_INTERFACE };


This will break certain platforms. Mellanox, as an example, doesn't allow you to create tables with both RIF bindings and LAG/PORT bindings. @prsunny @lguohan FYI

daall · 2020-03-30T16:53:39Z

orchagent/portsorch.cpp

+            }
+            else
+            {
+                port_id = port.m_vlan_info.vlan_oid;


Same issue as this.

daall · 2020-03-30T16:57:37Z

orchagent/portsorch.cpp

-                bind_type = SAI_ACL_BIND_POINT_TYPE_VLAN;
+                if (port.m_rif_id)
+                {
+                    bind_type = SAI_ACL_BIND_POINT_TYPE_ROUTER_INTERFACE;


This is also concerning because there are platforms that have full (e.g. ingress and egress) support for VLAN ACLs but not RIF ACLs. This change could break those devices that you're describing.

daall · 2020-03-30T16:59:25Z

orchagent/portsorch.cpp

-                sai_attribute_t vlan_attr;
-                vlan_attr.id = ingress ? SAI_VLAN_ATTR_INGRESS_ACL : SAI_VLAN_ATTR_EGRESS_ACL;
-                vlan_attr.value.oid = group_oid;
+                if(port.m_rif_id)


Same issue as this.

daall · 2020-03-30T16:59:39Z

orchagent/portsorch.cpp

@@ -3747,7 +3781,14 @@ bool PortsOrch::removeAclTableGroup(const Port &p)
            bind_type = SAI_ACL_BIND_POINT_TYPE_LAG;
            break;
        case Port::VLAN:
-            bind_type = SAI_ACL_BIND_POINT_TYPE_VLAN;
+            if (p.m_rif_id)


Same issue as this.

wendani · 2020-04-22T04:55:17Z

orchagent/aclorch.cpp

@@ -1295,7 +1295,7 @@ bool AclTable::create()
    }
    else
    {
-        bpoint_list = { SAI_ACL_BIND_POINT_TYPE_PORT, SAI_ACL_BIND_POINT_TYPE_LAG };
+        bpoint_list = { SAI_ACL_BIND_POINT_TYPE_PORT, SAI_ACL_BIND_POINT_TYPE_LAG, SAI_ACL_BIND_POINT_TYPE_ROUTER_INTERFACE };


It is better to be conscious of what BIND_POINT_TYPE is indeed needed according to the input field, as BIND_POINT_TYPE is translated into match field/qualifiers allocation in the hardware that will increase the possibility of using double-wide mode TCAM

wendani · 2020-04-22T06:09:38Z

orchagent/portsorch.cpp

@@ -910,7 +919,14 @@ bool PortsOrch::createBindAclTableGroup(sai_object_id_t id, sai_object_id_t &gro
                bind_type = SAI_ACL_BIND_POINT_TYPE_LAG;
                break;
            case Port::VLAN:
-                bind_type = SAI_ACL_BIND_POINT_TYPE_VLAN;
+                if (port.m_rif_id)


Vlan interface may not be created when vlan Port object is created. You may need to introduce a structure similar to AclTable::pendingPortSet in AclTable object to hold those rifs that are pending creation, and be an observer to rif creation event, upon which a rif is ready for ACL binding.

) * Simply remove the check * Add unit test for consutil show command * Fix a known typo (found by adding unittest) Signed-off-by: Jing Kan [email protected]

What I did: Moved the SAI header to v1.8.1. 7cd3a7ed84db3fc9cec13496a5339b6fe1888bb7 (HEAD, tag: v1.8.1, origin/v1.8) Update SAI version to V1.8.1 (sonic-net#1218) 5913e4cdd0c9c7ae859baa2e18086327b39a94da Fix error when compiling Broadcom SAI with v1.8.0 (sonic-net#1216) 5a98bc3c7e86c01f3cf702054f9af7c7c5ca6daf (HEAD, tag: v1.8.0, origin/master, origin/HEAD, master) Update version to 1.8.0 (sonic-net#1207) b3244ceceb45184ffe37da55bb9a98ef126050ce saineighbor.h: Updated SAI_NEIGHBOR_ENTRY_ATTR_ENCAP_INDEX and deprecated SAI_NEIGHBOR_ENTRY_ATTR_ENCAP_IMPOSE_INDEX (sonic-net#1202) 8731ca6e09c7ba99b0b009e5821d80598e216756 Add source/dest/double NAPT entry available attributes (sonic-net#1194) f053d899feb9517f2db43ee462589a30572b5ed1 Add switch attributes for hash offset configuration. (sonic-net#1195) 13e5cd6940f9a0da1878d00f08e5941e09f16e7f PRBS RX State Data Type (sonic-net#1179) 9755845a06525a3c17f03e7b936a70783e8ef068 Packet header based VRF classification (sonic-net#1185) 2369ecb59fff1a5cae948d41eea06bf8b71330b2 SAI versioning (sonic-net#1183) 744279839c176e68b19734657975e3f5ec6f1a32 Replaced SAI_SWITCH_ATTR_MACSEC_OBJECT_ID with SAI_SWITCH_ATTR_MACSEC_OBJECT_LIST (sonic-net#1199) 584c724864fe565357e82d097ddcc7363bddefac [CI] Set up CI&PR with Azure Pipelines (sonic-net#1200) 08192237963174cc60edae9b4812a39c43b291fd Add attribute to query available packet DMA pool size (sonic-net#1198) f092ef1e3ce695fc3f9552721025695312b961a2 Add IPv6 flow label hash attribute. (sonic-net#1192) cbc9562bb7a8f2c3a79702b99be55f3b3afa6957 Override VRF (sonic-net#1186) 1eb35afdb2146baf40e6c2b8f2f8bfe99075eaee Add SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO format for GB MDIO sysfs access (sonic-net#1171) b2d4c9a57c7f00b2632c35ca5eb3dd6480f7916a Switch scoped tunnel attributes (sonic-net#1173) 96adc95bf8316e1905143d9ecd21f32a43e80d7f Enhancements for MPLS support (sonic-net#1181) 3dcf1f2028da4060b345ad78e8a0c87d225bf5d0 Support for ACL extensions in metadata (sonic-net#1178) 24076be95b871e8f82ecaeb908cad951dc68896c [meta] Add support for allow empty list tag (sonic-net#1190) a2b3344cdde0bf5a4f8e98e1c676a658c0c615b0 spell check fixes (sonic-net#1189) bf4271bab6e8884bd96050bcba44e3134adaaec3 Do not call sai_metadata_sai get APIs before checking if they are allocated (sonic-net#1182) 5d5784dc3dbfc713c92ae7d2c472680b837bb002 [macsec]: Separate XPN configuration attribute from read-only attribute (sonic-net#1169) 6d5a9bf5ad17cb82621cabbe2449524320930606 [macsec]: add SAI_MACSEC_ATTR_SUPPORTED_CIPHER_SUITE_LIST (sonic-net#1172) e72c8f3a0cc543cb228554be82c97a63db917740 [meta] Print each tool version in Makefile (sonic-net#1177) 8f19677da88c7494d563ef7c5acb0529ecbd0b6e [meta] Add check for START, END and RANBE_BASE enums (sonic-net#1175) 24ad7906f145930b2e25682b6248909289d39e72 [meta] Create sai_switch_pointers_t struct (sonic-net#1174) 4f5f84df3fcd0e146707df41d3e2837c48f7c760 Tunnel loopback packet action as resource (sonic-net#1163) 8a0e82c57aa0e22e696158735516904e7dc14052 [meta] Add create only oid attribute check on switch object (sonic-net#1170) 14cf50772e478551920963ecf11f4fd019a0c106 Remove obsolete stub folder (sonic-net#1168) f14f406340e4f5f1b1d674f6fdd5fd861a54c877 [meta] Use safer calloc for integer overflow check (sonic-net#1166) Also this PR include changes of this sonic-net#815 SAI commit b2d4c9a57c7f00b2632c35ca5eb3dd6480f7916a Switch scoped tunnel attributes (sonic-net#1173) needed change in sai_redis_switch.cpp and sai_vs_switch.cpp for compilation. How I verify: Verify Build is fine of libsairedis*.deb, syncd*.deb, swss*.deb Co-authored-by: Ann Pokora <[email protected]>

[ACL][VLAN] Add support for ingress and egress ACLs on router interfa…

c2de0e7

…ces/SVI

jleveque added Enhancement ➕ request_for_201911_branch labels Mar 6, 2020

jleveque requested review from lguohan and daall March 6, 2020 00:56

jleveque self-assigned this Mar 6, 2020

prsunny reviewed Mar 6, 2020

View reviewed changes

jleveque mentioned this pull request Mar 18, 2020

[minigraph.py] Add support for 'OutAcl' keyword and attaching ACLs to VLAN interfaces sonic-net/sonic-buildimage#4229

Merged

daall suggested changes Mar 30, 2020

View reviewed changes

wendani reviewed Apr 22, 2020

View reviewed changes

wendani mentioned this pull request Dec 9, 2020

[Sub-ports] ACL doesn't support sub-ports as port type #1533

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ACL][VLAN] Add support for ingress and egress ACLs on router interfaces/SVI #1218

[ACL][VLAN] Add support for ingress and egress ACLs on router interfaces/SVI #1218

jleveque commented Mar 6, 2020

prsunny Mar 6, 2020

jleveque Mar 6, 2020

prsunny Mar 6, 2020

anirudhnr Mar 10, 2020

prsunny Mar 10, 2020

prsunny Mar 6, 2020

daall Mar 30, 2020

daall left a comment

daall Mar 30, 2020

daall Mar 30, 2020

daall Mar 30, 2020

daall Mar 30, 2020

daall Mar 30, 2020

wendani Apr 22, 2020

wendani Apr 22, 2020

[ACL][VLAN] Add support for ingress and egress ACLs on router interfaces/SVI #1218

[ACL][VLAN] Add support for ingress and egress ACLs on router interfaces/SVI #1218

Conversation

jleveque commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment