[aclorch] Fix and simplify DTel watchlist tables and entries #2155
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The significant rewrite of aclorch when adding ACL_TABLE_TYPE configuration caused a bug that prevents configuration of any DTel rules. This is due to use of an incorrect set of enum mappings while determining which type of AclRule to create. This commit changes that set of enum mappings. This also updates the list of actions in the DTel Flow and Drop watchlist tables so that they match the existing allowed actions in DTel acl rules. Signed-off-by: mickeyspiegel <mickey.spiegel@intel.com>
Since the 201911 release, the DTel drop actions have been supported in the DTel Flow Watchlist as well as the DTel Drop Watchlist. This allows for simplified operation where there is a single watchlist (DTel Flow Watchlist) that contains both flow and drop actions. The significant rewrite of aclorch when adding ACL_TABLE_TYPE configuration changed the way that the type of AclRule is determined. The current logic to distinguish between DTelFlow and DTelDrop AclRules is not correct. A fix to this logic would effectively remove the distinction between AclRules of type DTelFlow and DTelDrop, allowing for the larger set of actions supported by DTelFlow in either table. With this change in behavior, this seems like a good time to remove the DTel Drop Watchlist, leaving only the DTel Flow Watchlist that supports the superset of actions for both flow and drop. Signed-off-by: mickeyspiegel <mickey.spiegel@intel.com>
Note: Leaving definition of TABLE_TYPE_DTEL_FLOW_WATCHLIST unchanged
for backwards compatibility.
Signed-off-by: mickeyspiegel <mickey.spiegel@intel.com>
|
|
|
Changes lgtm, @bingwang-ms to review/merge |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…watchlist_tests [aclorch] Create sonic-swss tests for dtel acl
|
@bingwang-ms could u please help merge this PR |
Signed-off-by: Myron Sosyak <myronx.sosyak@intel.com>
Extend DTEL ACL table
|
@bingwang-ms can this be merged, or are we waiting for something? |
|
@bingwang-ms Test failures between successive runs are disjoint. It does not seem like any of these failures are related to this patch. |
|
@prsunny , @bingwang-ms can this PR be merged? Is there any action required from @mickeyspiegel |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@prsunny I updated to latest master, all checks are passing now. |
prsunny
approved these changes
Jun 17, 2022
|
@prsunny Thanks for merging this. |
yxieca
pushed a commit
that referenced
this pull request
Jun 20, 2022
* Fix DTel acl rule creation The significant rewrite of aclorch when adding ACL_TABLE_TYPE configuration caused a bug that prevents configuration of any DTel rules. This is due to use of an incorrect set of enum mappings while determining which type of AclRule to create.
7 tasks
jimmyzhai
added a commit
to sonic-net/sonic-buildimage
that referenced
this pull request
Jun 27, 2022
2022-06-24 93af69c: [PFC_WD] Avoid applying ZeroBuffer Profiles to ingress PG when a PFC storm is detected (sonic-net/sonic-swss#2304) 2022-06-24 37349cf: [swssconfig] Optimize performance of swssconfig (sonic-net/sonic-swss#2336) 2022-06-24 84e9b07: [fdborch] fix heap-use-after-free in clearFdbEntry() (sonic-net/sonic-swss#2353) 2022-06-24 1b8bd94: Create ACL table fails due to incorrect check for supported ACL actions #11235 (sonic-net/sonic-swss#2351) 2022-06-24 1ed0b4b: [macsec] Refactor the logic of macsec name map (sonic-net/sonic-swss#2348) 2022-06-23 f88f992: [mock_tests] Add Sflow Orch UTs (sonic-net/sonic-swss#2295) 2022-06-23 ec57bf1: [macsec] Update macsec flex counter (sonic-net/sonic-swss#2338) 2022-06-22 6e0fc85: [ACL] Support stage particular match fields (sonic-net/sonic-swss#2341) 2022-06-22 efb4530: [orchagent, DTel]: report session support to set user vrf (sonic-net/sonic-swss#2326) 2022-06-22 d82874d: Fix for "orchagent crashed when trying to delete fdb static entry with swssconfig #11046" (sonic-net/sonic-swss#2332) 2022-06-22 0c789e6: Fix qos map test in vs test (sonic-net/sonic-swss#2343) 2022-06-17 1bb5070: Enhance mock test for dynamic buffer manager for port removing and qos reload flows (sonic-net/sonic-swss#2262) 2022-06-16 700492f: [aclorch] Fix and simplify DTel watchlist tables and entries (sonic-net/sonic-swss#2155)
preetham-singh
pushed a commit
to preetham-singh/sonic-swss
that referenced
this pull request
Aug 6, 2022
…et#2155) * Fix DTel acl rule creation The significant rewrite of aclorch when adding ACL_TABLE_TYPE configuration caused a bug that prevents configuration of any DTel rules. This is due to use of an incorrect set of enum mappings while determining which type of AclRule to create.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What I did
by referencing the correct set of enum mappings for DTel actions.
DTel flow watchlist table and entries, which support the superset of
actions for both flow and drop.
(without ...Flow...) to clarify that there is only one common watchlist
for DTel, covering both flow and drop actions.
Note: Left definition of TABLE_TYPE_DTEL_FLOW_WATCHLIST unchanged
for backwards compatibility.
it matches the existing allowed actions in DTel AclRule subclasses.
Why I did it
The significant rewrite of aclorch when adding ACL_TABLE_TYPE
configuration caused two major issues that prevent the DTel
watchlist from operating correctly:
Introduced a bug that prevents configuration of any DTel rules.
This is due to a reference to an incorrect set of enum mappings,
while determining which subclass of AclRule to create.
Changed the way that the AclRule subclass is determined so that it
is no longer tied to ACL_TABLE_TYPE. The new logic assumes
that each subclass of AclRule has a disjoint set of actions that do not
overlap any of the actions in other AclRule subclasses. This assumption
does not hold for DTel which currently has two AclRule subclasses,
for flow and drop.
Since the 201911 release, the DTel drop actions have been
supported in the DTel flow watchlist as well as the DTel drop
watchlist. This allows for simplified operation where there is a
single watchlist (DTel flow watchlist) that contains both flow and
drop actions. The ACL_TABLE_TYPE changes effectively reverted
this change. A fix to this logic would remove the distinction
between AclRules of subclass DTelFlow and DTelDrop, allowing for the
larger set of actions supported by DTelFlow in either table.
With this change in behavior, this seems like a good time to remove
the DTel drop watchlist, leaving only one DTel watchlist that supports
the superset of actions for both flow and drop.
How I verified it
Manual testing, verifying that syncd sends down the corresponding
acl entries, and that these rules are programmed in the switch hardware.
We also added sonic-swss tests for DTel acls that verify the SAI calls
generated for DTel acl entries.
Details if related
Signed-off-by: mickeyspiegel mickey.spiegel@intel.com