-
Notifications
You must be signed in to change notification settings - Fork 557
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.
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
[acl]: Allow ACL table bind to LAGs and VLANs. #349
Conversation
orchagent/port.cpp
Outdated
sai_object_id_t bp_list[] = { SAI_ACL_BIND_POINT_TYPE_PORT }; | ||
int32_t bp_list[] = {SAI_ACL_BIND_POINT_TYPE_PORT, | ||
SAI_ACL_BIND_POINT_TYPE_LAG, | ||
SAI_ACL_BIND_POINT_TYPE_VLAN}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depending on the port type (phy/lag/vlan), we can select the BIND_POINT_TYPE here. We do not need to add all bind point type in the SAI_ACL_TABLE_GROUP_ATTR_ACL_BIND_POINT_TYPE_LIST. It is not necessary. On some platforms, it can cost us to use more resources.
orchagent/port.cpp
Outdated
group_attr.value.objlist.count = 1; | ||
group_attr.value.objlist.list = bp_list; | ||
group_attr.value.s32list.count = sizeof(bp_list)/sizeof(*bp_list); | ||
group_attr.value.s32list.list = bp_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure it is necessary to add all bind point types when you creating acl table group for this port. Since the port has only one type, we should add appropriate port type to the bind point type list. Only 1 type is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limit to 1 bind point type depending on port type.
2ffa068
to
580b0b1
Compare
@xinliu-seattle @lguohan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you added to the existing test framework for ACLs, test cases for this enhancement?
If so, can you point to the review of those test cases?
break; | ||
default: | ||
SWSS_LOG_ERROR("Failed to process port. Incorrect port %s type %d", alias.c_str(), port.m_type); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor alignment issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oleksandrivantsiv , can you resolve the conflict here? |
Conflicts: orchagent/portsorch.cpp
580b0b1
to
ef8c574
Compare
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
What I did
Allowed to bind ACL table to LAGs and VLANs.
Denied to bind ACL table to LAG members.
Fixed issue with ACL table type and ports validation.
Why I did it
All changes are done in the scope of ACL dynamic configuration feature.
How I verified it
Manual tests
Run ACL testbed tests