[dash]: ACL orchagent by Pterosaur · Pull Request #2470 · sonic-net/sonic-swss

Pterosaur · 2022-09-28T15:14:28Z

What I did
Add DASH ACL orchagent implementation.

Why I did it
DASH needs ACL function.

How I verified it

$ sudo pytest -sv --num-ports=2 test_dash_acl.py --pdb
================================ test session starts ================================
platform linux -- Python 3.8.10, pytest-4.6.2, py-1.10.0, pluggy-0.13.1 -- /usr/bin/python3
cachedir: .pytest_cache
plugins: flaky-3.7.0
collected 4 items

test_dash_acl.py::TestAcl::test_acl_flow PASSED
test_dash_acl.py::TestAcl::test_acl_group PASSED
test_dash_acl.py::TestAcl::test_acl_rule PASSED
test_dash_acl.py::test_nonflaky_dummy PASSED

============================= 4 passed in 54.83 seconds =============================

Details if related

.azure-pipelines/build-template.yml

orchagent/dash/dashaclorch.cpp

theasianpianist · 2022-11-14T22:51:46Z

orchagent/dash/dashaclorch.cpp

+            attrs.back().id = SAI_DASH_ACL_RULE_ATTR_DASH_ACL_GROUP_ID;
+            attrs.back().value.oid = acl_group->m_dash_acl_group_id;
+
+            acl_rule.m_status = m_dash_acl_rule_bulker.create_entry(&acl_rule.m_dash_acl_rule_id, static_cast<uint32_t>(attrs.size()), attrs.data());


same question as above re: bulk operations

theasianpianist · 2022-11-14T22:52:39Z

orchagent/dash/dashaclorch.cpp

+                }
+                else
+                {
+                    getAclGroup(group_id)->m_rule_count--;


Might be a good idea to add an error log here?

prsunny

You would need to maintain a ref count b/w eni and acl group to prevent deleting group while it is already bind to an ENI.

prsunny · 2022-12-20T21:43:21Z

orchagent/dash/dashaclorch.cpp

+        else
+        {
+            // Update the ACL group's attributes
+            for (const auto &attr : attrs)


ACL group update is for what scenario? Is this only to set the ip_version, say changing from ipv4 to ipv6? if so, its not a valid case. can you check if this is required?

OK, I removed the update block.

prsunny · 2022-12-20T21:44:50Z

orchagent/dash/dashaclorch.cpp

+    // The member rules of group should be removed first
+    if (acl_group->m_rule_count != 0)
+    {
+        SWSS_LOG_WARN("ACL group %s still has %d rules", key.c_str(), acl_group->m_rule_count);


Please change this to info, else this log will be continuously generated. Please address this for all task_retries.

prsunny · 2022-12-20T21:46:15Z

orchagent/dash/dashaclorch.cpp

+    }
+
+    // The member rules of group should be removed first
+    if (acl_group->m_rule_count != 0)


You should also check if this group is 'bind' to some ENI. If so, we cannot delete the group. User must first unbind the group before deleting.

Done. Add a ref count to check that.

prsunny · 2022-12-20T21:46:43Z

orchagent/dash/dashaclorch.cpp

+    auto acl_group = getAclGroup(group_id);
+    if (acl_group == nullptr)
+    {
+        SWSS_LOG_WARN("ACL group %s doesn't exist, waiting for group creating before creating rule %s", group_id.c_str(), rule_id.c_str());


As suggested, please change to INFO

prsunny · 2022-12-20T21:50:01Z

orchagent/dash/dashaclorch.cpp

+        // If the attributes don't have default value, just skip and wait for the user to set the value at the next message
+        if (!acl_rule.m_protocols)
+        {
+            const static vector<uint8_t> all_protocols = {


This is prone to error and hard to see if you missed some value. Suggest generating it through a lambda function.

prsunny · 2022-12-20T21:51:05Z

orchagent/dash/dashaclorch.cpp

+    bool update_action = false;
+    update_action |= updateValue(data, "action", acl_rule.m_action);
+    update_action |= updateValue(data, "terminating", acl_rule.m_terminating);
+    if (update_action || (acl_rule.m_action && acl_rule.m_terminating && !is_existing))


I see that is_existing is repeated all around? Please move this to a section for new create vs set.

Here is an optimization I want.
I would always only pass these updated attributes to the SAI. But how can I know which attributes are updated if I move this to another section? I have no idea only if I introduce another repeated code to check and add them one by one to the attrs.

prsunny · 2022-12-20T21:53:19Z

orchagent/dash/dashaclorch.cpp

+        }
+        else
+        {
+            // Update the ACL group's attributes


ACL Rule's attribute -> Typo

prsunny · 2022-12-20T21:54:31Z

orchagent/dash/dashaclorch.cpp

+
+    bool is_existing = acl_rule.m_dash_acl_rule_id != SAI_NULL_OBJECT_ID;
+
+    if (!is_existing)


how about ref count in this case? do we need to decrement?

In my opinion, a ACL rule can only be added to only one ACL group. So that the ref count is always one. If yes, why we need a count to record it?

prsunny · 2022-12-20T21:56:17Z

orchagent/dash/dashaclorch.cpp

+    auto eni_entry = m_dash_orch->getEni(eni);
+    if (eni_entry == nullptr)
+    {
+        SWSS_LOG_WARN("eni %s cannot be found", eni.c_str());


Change to INFO

prsunny · 2022-12-20T21:58:09Z

orchagent/dash/dashaclorch.cpp

+        attr.id = getSaiStage(direction, *(acl_group->m_ip_version), stage);
+        attr.value.oid = acl_group->m_dash_acl_group_id;
+    }
+    else


What would be a case where updateValue fails?

If the input string isn't valid or the new value equals than the old one.

Signed-off-by: Ze Gan <ganze718@gmail.com>

theasianpianist · 2022-12-30T23:10:02Z

orchagent/dash/dashaclorch.cpp

+        else
+        {
+            SWSS_LOG_ERROR(
+                "Unknown task : %s - %s",


Might be a good idea to include the actual task/key in this message as well to help with debugging

I believe all actual tasks/key have been logged in /var/log/swss/swss.rec by lib swsscommon so that we don't need repeated recording.

theasianpianist · 2022-12-30T23:10:11Z

orchagent/dash/dashaclorch.cpp

+        if (task_status == task_need_retry)
+        {
+            SWSS_LOG_DEBUG(
+                "Task %s - %s need retry",


Same as above

theasianpianist · 2022-12-30T23:10:24Z

orchagent/dash/dashaclorch.cpp

+        {
+            if (task_status != task_success)
+            {
+                SWSS_LOG_WARN("Task %s - %s fail",


Same as above

theasianpianist · 2022-12-30T23:10:30Z

orchagent/dash/dashaclorch.cpp

+            else
+            {
+                SWSS_LOG_DEBUG(
+                    "Task %s - %s success",


Same as above

Pterosaur changed the base branch from master to dash September 28, 2022 15:14

prsunny force-pushed the dash branch from f093be3 to df92fb7 Compare October 7, 2022 21:33

Pterosaur force-pushed the dash_acl branch from 6863811 to fdc0dc3 Compare October 10, 2022 17:22

Pterosaur changed the title ~~Dash acl~~ [dash]: ACL orchagent Oct 11, 2022

Pterosaur marked this pull request as ready for review October 11, 2022 09:37

Pterosaur requested a review from prsunny as a code owner October 11, 2022 09:37

Pterosaur requested review from abdosi, judyjoseph and neethajohn as code owners October 25, 2022 04:57

Pterosaur force-pushed the dash_acl branch from 2167244 to 49df256 Compare October 25, 2022 06:01

Pterosaur changed the base branch from dash to master October 25, 2022 06:04

Pterosaur changed the base branch from master to dash October 25, 2022 06:04

Pterosaur force-pushed the dash_acl branch 11 times, most recently from 0cc18b9 to c88ec82 Compare October 25, 2022 18:41

theasianpianist reviewed Oct 25, 2022

View reviewed changes

.azure-pipelines/build-template.yml Outdated Show resolved Hide resolved

theasianpianist reviewed Oct 25, 2022

View reviewed changes

orchagent/dash/dashaclorch.cpp Outdated Show resolved Hide resolved

Pterosaur force-pushed the dash_acl branch 5 times, most recently from 60901af to 2d35a7e Compare October 26, 2022 15:59

theasianpianist reviewed Nov 14, 2022

View reviewed changes

prsunny reviewed Dec 20, 2022

View reviewed changes

Pterosaur marked this pull request as draft December 22, 2022 14:07

Pterosaur force-pushed the dash_acl branch 9 times, most recently from bbf5032 to 2466354 Compare December 23, 2022 09:08

Fix comment

0f713cb

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the dash_acl branch from 2466354 to 0f713cb Compare December 23, 2022 09:10

Merge branch 'dash' into dash_acl

376752f

Pterosaur force-pushed the dash_acl branch 6 times, most recently from 6fbe409 to 22552e4 Compare December 24, 2022 15:53

Fix comment

4bd5517

Signed-off-by: Ze Gan <ganze718@gmail.com>

prsunny approved these changes Dec 29, 2022

View reviewed changes

Merge branch 'dash' into dash_acl

4874e33

theasianpianist reviewed Dec 30, 2022

View reviewed changes

orchagent/dash/dashaclorch.cpp

{

if (task_status != task_success)

{

SWSS_LOG_WARN("Task %s - %s fail",

Copy link

Contributor

theasianpianist Dec 30, 2022

Choose a reason for hiding this comment

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

Same as above

theasianpianist reviewed Dec 30, 2022

View reviewed changes

orchagent/dash/dashaclorch.cpp

else

{

SWSS_LOG_DEBUG(

"Task %s - %s success",

Copy link

Contributor

theasianpianist Dec 30, 2022

Choose a reason for hiding this comment

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

Same as above

Merge branch 'dash' into dash_acl

2c84ca3


		bool is_existing = acl_rule.m_dash_acl_rule_id != SAI_NULL_OBJECT_ID;

		if (!is_existing)

Conversation

Pterosaur commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsunny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Pterosaur commented Sep 28, 2022 •

edited

Loading