Add DONOTDROP packet action. #1349
Conversation
Ashish1805
force-pushed
the
Ashish1805-patch-1
branch
2 times, most recently
from
65b26b7 to
a0b3eeb
Compare
| SAI_PACKET_ACTION_TRANSIT, | ||
|
|
||
| /** Do not drop the packet. */ | ||
| SAI_PACKET_ACTION_DONOTDROP |
There was a problem hiding this comment.
whats the difference with SAI_PACKET_ACTION_FORWARD?
There was a problem hiding this comment.
Our intention here is to get an acl packet action which can help override ACL DROP action of another ACL based on acl priority groups of these ACLs.
For example:
Lets say we have an ACL in a table T1 (with ACL group priority = X) that has ACL action = DROP and we have another ACL in table Table T2 (With ACL group priority = Y) with ACL action = REDIRECT and a packet is hitting both the ACLs.
Since, ACL actions REDIRECT and DROP are conflicting actions on a packet, action resolution will happen. Irrespective of acl group priority of Table T1 and T2, DROP action will take precedence and packet will be dropped.
We want packet to not get dropped rather be redirected (which means we want to override the ACL DROP action of ACL in Table T1 by ACL REDIRECT action of ACL in Table T2) when Table T2's ACL group priority is higher than Table T1's ACL group priority, i.e. Y > X. To achieve this we want to add packet action SAI_PACKET_ACTION_DONOTDROP.
Using this (ACL action = REDIRECT + packet action = SAI_PACKET_ACTION_DONOTDROP), Acl in Table2 can override the ACL DROP action of ACL action in Table 1 given acl group priority of Table2 > acl group priority of Table1.
As per my understanding, SAI_PACKET_ACTION_FORWARD packet action helps forward the packets using forwarding info from L2/L3 table if packets are not being dropped - but it cannot help override ACL DROP action itself irrespective of acl group priorities of ACL tables.
There was a problem hiding this comment.
@kcudnik would you take a look at whether the documentation addresses your question?
There was a problem hiding this comment.
explanation is very informative, thanks
|
@JaiOCP , @marian-pritsak - would you please help review this? Thanks. |
Ashish1805
force-pushed
the
Ashish1805-patch-1
branch
from
84d3562 to
103a86f
Compare
|
I updated the PR with a markdown doc to explain how the DONOTDROP packet action will be used. Thank you. |
|
@marian-pritsak Can you please take a look if changes look good to you? Thank you. |
|
|
||
| | ACL in table T1 | ACL in table T2 | | ||
| |-------------------------------------|---------------------------------------| | ||
| | Table priority group: P1 | Table priority group: P2 | |
There was a problem hiding this comment.
Can you please elaborate on "Table priority group"? Currently, there is no priority attribute for ACL Group table in SAI.
There was a problem hiding this comment.
Yes. It is good to clarify if you meant SAI_ACL_TABLE_GROUP_MEMBER_ATTR_PRIORITY.
There was a problem hiding this comment.
But SAI_ACL_TABLE_GROUP_MEMBER_ATTR_PRIORITY is applicable only for the sequential ACL groups. For a sequential ACL group, only the highest priority table is applied so if the highest priority table has a redirect action, it will apply regardless of any conflicting actions in the lower priority tables. So, I'm bit unclear how this new packet action type will be used.
There was a problem hiding this comment.
done. Thank you.
There was a problem hiding this comment.
@ashutosh-agrawal would you take a look at whether the documentation addresses your question? Thank you.
| | Table priority group: P1 | Table priority group: P2 | | ||
| |Action: neither DROP nor SAI_PACKET_ACTION_DONOTDROP | Action: DROP | | ||
|
|
||
|
|
There was a problem hiding this comment.
We need to define the behavior for enums that are "Combination of Packet Actions" like SAI_PACKET_ACTION_TRAP. For example, can you explain the below:
| ACL in table T1 | ACL in table T2 |
|---|---|
| Table priority group: P1 | Table priority group: P2 |
| Action: SAI_PACKET_ACTION_DONOTDROP | Action: TRAP |
If P1 > P2: action is “Copy to CPU?”
If P2 > P1: action is “Trap?”
There was a problem hiding this comment.
Added behavior of DONOTDROP with TRAP and DENY. Thank you.
There was a problem hiding this comment.
@rck-innovium would you take a look at whether the documentation addresses your question? Thank you.
|
|
||
| | ACL in table T1 | ACL in table T2 | | ||
| |-------------------------------------|---------------------------------------| | ||
| | Table priority group: P1 | Table priority group: P2 | |
There was a problem hiding this comment.
Yes. It is good to clarify if you meant SAI_ACL_TABLE_GROUP_MEMBER_ATTR_PRIORITY.
| Goal of this packet action attribute (SAI_PACKET_ACTION_DONOTDROP) is to resolve the conflicts of ACL actions when a packet hits more than one ACLs such that when this packet action is used with an ACL, it serves as an action that prevents the possible drop from another ACL that is in a lower priority table. | ||
|
|
||
| This new packet action attribute is in line with the current SAI architecture - actions in higher priority groups take precedence. If an ACL with DROP action is in a higher priority group than this ACL then, DROP action is expected to be honored. Few cases are explained with examples in next section. | ||
|
|
There was a problem hiding this comment.
What is the behavior if the highest priority entry in ACL stage 1 (say ingress) marks as DO_NOT_DROP, and another entry in ACL Stage 2 (say egress) marks the packet as drop?
In order to avoid ambiguities, we should spell out that DO NOT DROP overrides the DROP action set by:
-
ACL tables with lower priority in this ACL Table Group, AND
-
ACL tables in preceding stages
In other words, it would not override the drop action set in subsequent ACL Table stages.
There was a problem hiding this comment.
As per my understanding, ACL action resolution is done per-stage and an ingress stage DONOTDROP action should not have have impact on VFP or EFP actions.
There was a problem hiding this comment.
Updated the doc with comment on DONOTDROP action in one stage will not override the drop action is subsequent ACL stages. Thank you.
Ashish1805
force-pushed
the
Ashish1805-patch-1
branch
from
103a86f to
91a795f
Compare
|
LGTM with the assumption that Ashish will take care of a few typos which were identified in the meeting today. |
This will help resolve the conflicts of ACL actions when a packet hits more than one ACLs such that when this packet action is used with an ACL, it serves as an action that prevents the possible drop from another ACL that is in a lower priority table. Signed-off-by: Ashish Singh <[email protected]>
Ashish1805
force-pushed
the
Ashish1805-patch-1
branch
from
91a795f to
859d393
Compare
|
Updated the doc to incorporate the changes suggested. PTAL when you get a chance. Thank you. |
|
Hi, All the comments have been addressed - not sure anyone has any other comments/feedback. Can we go ahead and merge/commit it? Thank you. |
This will help resolve the action conflict between ACLs in different priority groups where lower priority group ACL action is DROP and higher priority group ACL action is conflicting with DROP action, say REDIRECT, and requirement is to override the DROP action.