[dash] add ACL group bind check for rule create/update (#2974)

* add a check that prevents adding/editing rules in the bound group
* return error instead of retry for binding an empty group

orchagent/dash/dashaclgroupmgr.cpp

@@ -492,12 +492,6 @@ task_process_status DashAclGroupMgr::updateRule(const string& group_id, const st
 {
     SWSS_LOG_ENTER();
 
-    if (isBound(group_id))
-    {
-        SWSS_LOG_INFO("Failed to update dash ACL rule %s:%s, ACL group is bound to the ENI", group_id.c_str(), rule_id.c_str());
-        return task_failed;
-    }
-
     if (ruleExists(group_id, rule_id))
     {
         removeRule(group_id, rule_id);
@@ -601,15 +595,15 @@ task_process_status DashAclGroupMgr::bind(const string& group_id, const string&
     if (group_it == m_groups_table.end())
     {
         SWSS_LOG_INFO("Failed to bind ACL group %s to ENI %s. ACL group does not exist", group_id.c_str(), eni_id.c_str());
-        return task_need_retry;
+        return task_failed;
     }
 
     auto& group = group_it->second;
 
     if (group.m_dash_acl_rule_table.empty())
     {
-        SWSS_LOG_INFO("ACL group %s has no rules attached. Waiting for ACL rules creation", group_id.c_str());
-        return task_need_retry;
+        SWSS_LOG_INFO("Failed to bind ACL group %s to ENI %s. ACL group has no rules attached.", group_id.c_str(), eni_id.c_str());
+        return task_failed;
     }
 
     auto eni = m_dash_orch->getEni(eni_id);

orchagent/dash/dashaclgroupmgr.h

@@ -93,6 +93,7 @@ class DashAclGroupMgr
     task_process_status create(const std::string& group_id, DashAclGroup& group);
     task_process_status remove(const std::string& group_id);
     bool exists(const std::string& group_id) const;
+    bool isBound(const std::string& group_id);
 
     void onUpdate(const std::string& group_id, const std::string& tag_id,const DashTag& tag);
 
@@ -114,7 +115,6 @@ class DashAclGroupMgr
 
     void bind(const DashAclGroup& group, const EniEntry& eni, DashAclDirection direction, DashAclStage stage);
     void unbind(const DashAclGroup& group, const EniEntry& eni, DashAclDirection direction, DashAclStage stage);
-    bool isBound(const std::string &group_id);
     bool isBound(const DashAclGroup& group);
     void attachTags(const std::string &group_id, const std::unordered_set<std::string>& tags);
     void detachTags(const std::string &group_id, const std::unordered_set<std::string>& tags);

orchagent/dash/dashaclorch.cpp

@@ -272,6 +272,12 @@ task_process_status DashAclOrch::taskUpdateDashAclRule(
         return task_failed;
     }
 
+    if (m_group_mgr.isBound(group_id))
+    {
+        SWSS_LOG_INFO("Failed to set dash ACL rule %s:%s, ACL group is bound to the ENI", group_id.c_str(), rule_id.c_str());
+        return task_failed;
+    }
+
     if (m_group_mgr.ruleExists(group_id, rule_id))
     {
         return m_group_mgr.updateRule(group_id, rule_id, rule);

tests/test_dash_acl.py

@@ -429,12 +429,51 @@ def test_empty_acl_group_binding(self, ctx):
                             src_port=[PortRange(0,1)], dst_port=[PortRange(0,1)])
 
         # Now that the group contains a rule, expect binding to occur
+        ctx.bind_acl_out(self.eni_name, ACL_STAGE_1, v4_group_id = ACL_GROUP_1)
         ctx.asic_eni_table.wait_for_field_match(key=eni_key, expected_fields={sai_stage: acl_group_key})
 
         # Unbinding should occur immediately
         ctx.unbind_acl_out(self.eni_name, ACL_STAGE_1)
         ctx.asic_eni_table.wait_for_field_match(key=eni_key, expected_fields={sai_stage: SAI_NULL_OID})
 
+    def test_acl_rule_after_group_bind(self, ctx):
+        eni_key = ctx.asic_eni_table.get_keys()[0]
+        sai_stage = get_sai_stage(outbound=False, v4=True, stage_num=ACL_STAGE_1)
+
+        ctx.create_acl_group(ACL_GROUP_1, IpVersion.IP_VERSION_IPV4)
+        acl_group_key = ctx.asic_dash_acl_group_table.wait_for_n_keys(num_keys=1)[0]
+        ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_1,
+                            priority=1, action=Action.ACTION_PERMIT, terminating=False,
+                            src_addr=["192.168.0.1/32", "192.168.1.2/30"], dst_addr=["192.168.0.1/32", "192.168.1.2/30"],
+                            src_port=[PortRange(0,1)], dst_port=[PortRange(0,1)])
+        ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=1)
+
+        self.bind_acl_group(ctx, ACL_STAGE_1, ACL_GROUP_1, acl_group_key)
+
+        # The new rule should not be created since the group is bound
+        ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_2,
+                            priority=2, action=Action.ACTION_PERMIT, terminating=False,
+                            src_addr=["192.168.0.1/32", "192.168.1.2/30"], dst_addr=["192.168.0.1/32", "192.168.1.2/30"],
+                            src_port=[PortRange(0,1)], dst_port=[PortRange(0,1)])
+        time.sleep(3)
+        ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=1)
+
+        # Unbinding the group
+        ctx.unbind_acl_in(self.eni_name, ACL_STAGE_1)
+        ctx.asic_eni_table.wait_for_field_match(key=eni_key, expected_fields={sai_stage: SAI_NULL_OID})
+
+        # Now the rule can be created since the group is no longer bound
+        ctx.create_acl_rule(ACL_GROUP_1, ACL_RULE_2,
+                            priority=2, action=Action.ACTION_PERMIT, terminating=False,
+                            src_addr=["192.168.0.1/32", "192.168.1.2/30"], dst_addr=["192.168.0.1/32", "192.168.1.2/30"],
+                            src_port=[PortRange(0,1)], dst_port=[PortRange(0,1)])
+        ctx.asic_dash_acl_rule_table.wait_for_n_keys(num_keys=2)
+
+        # cleanup
+        ctx.remove_acl_rule(ACL_GROUP_1, ACL_RULE_1)
+        ctx.remove_acl_rule(ACL_GROUP_1, ACL_RULE_2)
+        ctx.remove_acl_group(ACL_GROUP_1)
+
     def test_acl_group_binding(self, ctx):
         eni_key = ctx.asic_eni_table.get_keys()[0]
         sai_stage = get_sai_stage(outbound=False, v4=True, stage_num=ACL_STAGE_2)