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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion orchagent/aclorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

attr.id = SAI_ACL_TABLE_ATTR_ACL_BIND_POINT_TYPE_LIST;
Expand Down
67 changes: 54 additions & 13 deletions orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ extern sai_vlan_api_t *sai_vlan_api;
extern sai_lag_api_t *sai_lag_api;
extern sai_hostif_api_t* sai_hostif_api;
extern sai_acl_api_t* sai_acl_api;
extern sai_router_interface_api_t *sai_router_intfs_api;
extern sai_queue_api_t *sai_queue_api;
extern sai_object_id_t gSwitchId;
extern IntfsOrch *gIntfsOrch;
Expand Down Expand Up @@ -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 ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

portIter.second.m_rif_id == id)
{
port = portIter.second;
return true;
Expand Down Expand Up @@ -546,7 +548,14 @@ bool PortsOrch::getAclBindPortId(string alias, sai_object_id_t &port_id)
port_id = port.m_lag_id;
break;
case Port::VLAN:
port_id = port.m_vlan_info.vlan_oid;
if (port.m_rif_id)
{
port_id = port.m_rif_id;
}
else
{
port_id = port.m_vlan_info.vlan_oid;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as this.

}
break;
default:
SWSS_LOG_ERROR("Failed to process port. Incorrect port %s type %d", alias.c_str(), port.m_type);
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
bind_type = SAI_ACL_BIND_POINT_TYPE_ROUTER_INTERFACE;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is only meant to support VLAN RIFs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
else
{
bind_type = SAI_ACL_BIND_POINT_TYPE_VLAN;
}
break;
default:
SWSS_LOG_ERROR("Failed to bind ACL table to port %s with unknown type %d",
Expand Down Expand Up @@ -992,17 +1008,35 @@ bool PortsOrch::createBindAclTableGroup(sai_object_id_t id, sai_object_id_t &gro
}
case Port::VLAN:
{
// Bind this ACL group to VLAN
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as this.

{
// Bind this ACL group to RIF
sai_attribute_t rif_attr;
rif_attr.id = ingress ? SAI_ROUTER_INTERFACE_ATTR_INGRESS_ACL : SAI_ROUTER_INTERFACE_ATTR_EGRESS_ACL;
rif_attr.value.oid = group_oid;

status = sai_vlan_api->set_vlan_attribute(port.m_vlan_info.vlan_oid, &vlan_attr);
if (status != SAI_STATUS_SUCCESS)
status = sai_router_intfs_api->set_router_interface_attribute(port.m_rif_id, &rif_attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to bind RIF %s to ACL table group %lx, rv:%d",
port.m_alias.c_str(), group_oid, status);
return false;
}
}
else
{
SWSS_LOG_ERROR("Failed to bind VLAN %s to ACL table group %" PRIx64 ", rv:%d",
port.m_alias.c_str(), group_oid, status);
return false;
// Bind this ACL group to VLAN
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;

status = sai_vlan_api->set_vlan_attribute(port.m_vlan_info.vlan_oid, &vlan_attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to bind VLAN %s to ACL table group %lx, rv:%d",
port.m_alias.c_str(), group_oid, status);
return false;
}
}
break;
}
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as this.

{
bind_type = SAI_ACL_BIND_POINT_TYPE_ROUTER_INTERFACE;
}
else
{
bind_type = SAI_ACL_BIND_POINT_TYPE_VLAN;
}
break;
default:
// Dealing with port, lag and vlan for now.
Expand Down