[ACL] Add default action_list for default ACL table type (sonic-net#2298)

What I did
This PR is derived from sonic-net#2205
Fix sonic-net#10425

We were seeing ACL table creation failure on some platform because action_list is mandatory, while the action_list is not provided by aclorch.

Apr  1 01:24:11.702608 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table DATAACL is mandatory
Apr  1 01:24:11.702608 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table DATAACL, invalid configuration
Apr  1 01:24:11.702741 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table EVERFLOW is mandatory
Apr  1 01:24:11.702741 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table EVERFLOW, invalid configuration
Apr  1 01:24:11.702926 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table EVERFLOWV6 is mandatory
Apr  1 01:24:11.702926 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table EVERFLOWV6, invalid configuration
This PR fixed the issue by adding default action_list to the default ACL table type if not present.

Why I did it
Fix the ACL table creation issue.

How I verified it
Verified by running test_acl and test_everflow on Broadcom TD3 platform

Signed-off-by: bingwang <[email protected]>
Co-authored-by: syuan <[email protected]>

Author: bingwang-ms

orchagent/aclorch.cpp

@@ -153,6 +153,176 @@ static const acl_capabilities_t defaultAclActionsSupported =
     }
 };
 
+static acl_table_action_list_lookup_t defaultAclActionList = 
+{
+    {
+        // L3
+        TABLE_TYPE_L3,
+        {
+            {
+                ACL_STAGE_INGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION,
+                    SAI_ACL_ACTION_TYPE_REDIRECT
+                }
+            },
+            {
+                ACL_STAGE_EGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION,
+                    SAI_ACL_ACTION_TYPE_REDIRECT
+                }
+            }
+        }
+    },
+    {
+        // L3V6
+        TABLE_TYPE_L3V6,
+        {
+            {
+                ACL_STAGE_INGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION,
+                    SAI_ACL_ACTION_TYPE_REDIRECT
+                }
+            },
+            {
+                ACL_STAGE_EGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION,
+                    SAI_ACL_ACTION_TYPE_REDIRECT
+                }
+            }
+        }
+    },
+    {
+        // MIRROR
+        TABLE_TYPE_MIRROR,
+        {
+            {
+                ACL_STAGE_INGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_MIRROR_INGRESS
+                }
+            },
+            {
+                ACL_STAGE_EGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_MIRROR_EGRESS
+                }
+            }
+        }
+    },
+    {
+        // MIRRORV6
+        TABLE_TYPE_MIRRORV6,
+        {
+            {
+                ACL_STAGE_INGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_MIRROR_INGRESS
+                }
+            },
+            {
+                ACL_STAGE_EGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_MIRROR_EGRESS
+                }
+            }
+        }
+    },
+    {
+        // MIRROR_DSCP
+        TABLE_TYPE_MIRROR_DSCP,
+        {
+            {
+                ACL_STAGE_INGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_MIRROR_INGRESS
+                }
+            },
+            {
+                ACL_STAGE_EGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_MIRROR_EGRESS
+                }
+            }
+        }
+    },
+    {
+        // TABLE_TYPE_PFCWD
+        TABLE_TYPE_PFCWD,
+        {
+            {
+                ACL_STAGE_INGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION
+                }
+            },
+            {
+                ACL_STAGE_EGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION
+                }
+            }
+        }
+    },
+    {
+        // MCLAG
+        TABLE_TYPE_MCLAG,
+        {
+            {
+                ACL_STAGE_INGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION
+                }
+            },
+            {
+                ACL_STAGE_EGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION
+                }
+            }
+        }
+    },
+    {
+        // MUX
+        TABLE_TYPE_MUX,
+        {
+            {
+                ACL_STAGE_INGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION
+                }
+            },
+            {
+                ACL_STAGE_EGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION
+                }
+            }
+        }
+    },
+    {
+        // DROP
+        TABLE_TYPE_DROP,
+        {
+            {
+                ACL_STAGE_INGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION
+                }
+            },
+            {
+                ACL_STAGE_EGRESS,
+                {
+                    SAI_ACL_ACTION_TYPE_PACKET_ACTION
+                }
+            }
+        }
+    }
+};
+
 static acl_ip_type_lookup_t aclIpTypeLookup =
 {
     { IP_TYPE_ANY,         SAI_ACL_IP_TYPE_ANY },
@@ -301,6 +471,12 @@ const set<sai_acl_action_type_t>& AclTableType::getActions() const
     return m_aclAcitons;
 }
 
+bool AclTableType::addAction(sai_acl_action_type_t action)
+{
+    m_aclAcitons.insert(action);
+    return true;
+}
+
 AclTableTypeBuilder& AclTableTypeBuilder::withName(string name)
 {
     m_tableType.m_name = name;
@@ -1808,6 +1984,51 @@ AclTable::AclTable(AclOrch *pAclOrch) noexcept : m_pAclOrch(pAclOrch)
 
 }
 
+bool AclTable::addMandatoryActions()
+{
+    SWSS_LOG_ENTER();
+
+    if (stage == ACL_STAGE_UNKNOWN)
+    {
+        return false;
+    }
+
+    if (!m_pAclOrch->isAclActionListMandatoryOnTableCreation(stage))
+    {
+        // No op if action list is not mandatory on table creation.
+        return true;
+    }
+    if (!type.getActions().empty())
+    {
+        // No change if action_list is provided
+        return true;
+    }
+
+    sai_acl_action_type_t acl_action = SAI_ACL_ACTION_TYPE_COUNTER;
+    if (m_pAclOrch->isAclActionSupported(stage, acl_action))
+    {
+        SWSS_LOG_INFO("Add counter acl action");
+        type.addAction(acl_action);
+    }
+
+    if (defaultAclActionList.count(type.getName()) != 0)
+    {
+        // Add the default action list
+        for (auto action : defaultAclActionList[type.getName()][stage])
+        {
+            if (m_pAclOrch->isAclActionSupported(stage, acl_action))
+            {
+                SWSS_LOG_INFO("Added default action for table type %s stage %s",
+                                    type.getName().c_str(),
+                                    ((stage == ACL_STAGE_INGRESS)? "INGRESS":"EGRESS"));
+                type.addAction(action);
+            }
+        }
+    }
+
+    return true;
+}
+
 bool AclTable::validateAddType(const AclTableType &tableType)
 {
     SWSS_LOG_ENTER();
@@ -3949,6 +4170,8 @@ void AclOrch::doAclTableTask(Consumer &consumer)
 
             newTable.validateAddType(*tableType);
 
+            newTable.addMandatoryActions();
+
             // validate and create/update ACL Table
             if (bAllAttributesOk && newTable.validate())
             {

orchagent/aclorch.h

@@ -113,6 +113,9 @@ typedef tuple<sai_acl_range_type_t, int, int> acl_range_properties_t;
 typedef map<acl_stage_type_t, AclActionCapabilities> acl_capabilities_t;
 typedef map<sai_acl_action_type_t, set<int32_t>> acl_action_enum_values_capabilities_t;
 
+typedef map<acl_stage_type_t, set<sai_acl_action_type_t> > acl_stage_action_list_t;
+typedef map<string, acl_stage_action_list_t> acl_table_action_list_lookup_t;
+
 class AclRule;
 
 class AclTableMatchInterface
@@ -156,6 +159,8 @@ class AclTableType
     const set<sai_acl_range_type_t>& getRangeTypes() const;
     const set<sai_acl_action_type_t>& getActions() const;
 
+    bool addAction(sai_acl_action_type_t action);
+
 private:
     friend class AclTableTypeBuilder;
 
@@ -387,6 +392,9 @@ class AclTable
     bool validate();
     bool create();
 
+    // Add actions to ACL table if mandatory action list is required on table creation.
+    bool addMandatoryActions();
+
     // validate AclRule match attribute against rule and table configuration
     bool validateAclRuleMatch(sai_acl_entry_attr_t matchId, const AclRule& rule) const;
     // validate AclRule action attribute against rule and table configuration

tests/mock_tests/aclorch_ut.cpp

@@ -1755,4 +1755,96 @@ namespace aclorch_test
         // try to delete non existing acl rule
         ASSERT_TRUE(orch->m_aclOrch->removeAclRule(tableId, ruleId));
     }
+
+    sai_switch_api_t *old_sai_switch_api;
+
+    // The following function is used to override SAI API get_switch_attribute to request passing
+    // mandatory ACL actions to SAI when creating mirror ACL table.
+    sai_status_t getSwitchAttribute(_In_ sai_object_id_t switch_id,_In_ uint32_t attr_count,
+                                    _Inout_ sai_attribute_t *attr_list)
+    {
+        if (attr_count == 1)
+        {
+            switch(attr_list[0].id)
+            {
+            case SAI_SWITCH_ATTR_MAX_ACL_ACTION_COUNT:
+                attr_list[0].value.u32 = 2;
+                return SAI_STATUS_SUCCESS;
+            case SAI_SWITCH_ATTR_ACL_STAGE_INGRESS:
+            case SAI_SWITCH_ATTR_ACL_STAGE_EGRESS:
+                attr_list[0].value.aclcapability.action_list.count = 2;
+                attr_list[0].value.aclcapability.action_list.list[0]= SAI_ACL_ACTION_TYPE_COUNTER;
+                attr_list[0].value.aclcapability.action_list.list[1]=
+                    attr_list[0].id == SAI_SWITCH_ATTR_ACL_STAGE_INGRESS ?
+                        SAI_ACL_ACTION_TYPE_MIRROR_INGRESS : SAI_ACL_ACTION_TYPE_MIRROR_EGRESS;
+                attr_list[0].value.aclcapability.is_action_list_mandatory = true;
+                return SAI_STATUS_SUCCESS;
+            }
+        }
+        return old_sai_switch_api->get_switch_attribute(switch_id, attr_count, attr_list);
+    }
+
+    TEST_F(AclOrchTest, AclTableCreationWithMandatoryActions)
+    {
+        // Override SAI API get_switch_attribute to request passing mandatory ACL actions to SAI
+        // when creating mirror ACL table.
+        old_sai_switch_api = sai_switch_api;
+        sai_switch_api_t new_sai_switch_api = *sai_switch_api;
+        sai_switch_api = &new_sai_switch_api;
+        sai_switch_api->get_switch_attribute = getSwitchAttribute;
+
+        // Set platform env to enable support of MIRRORV6 ACL table.
+        bool unset_platform_env = false;
+        if (!getenv("platform"))
+        {
+            setenv("platform", VS_PLATFORM_SUBSTRING, 0);
+            unset_platform_env = true;
+        }
+
+        auto orch = createAclOrch();
+
+        for (const auto &acl_table_type : { TABLE_TYPE_MIRROR, TABLE_TYPE_MIRRORV6, TABLE_TYPE_MIRROR_DSCP })
+        {
+            for (const auto &acl_table_stage : { STAGE_INGRESS, STAGE_EGRESS })
+            {
+                // Create ACL table.
+                string acl_table_id = "mirror_acl_table";
+                auto kvfAclTable = deque<KeyOpFieldsValuesTuple>(
+                    { { acl_table_id,
+                        SET_COMMAND,
+                        { { ACL_TABLE_DESCRIPTION, acl_table_type },
+                          { ACL_TABLE_TYPE, acl_table_type },
+                          { ACL_TABLE_STAGE, acl_table_stage },
+                          { ACL_TABLE_PORTS, "1,2" } } } });
+                orch->doAclTableTask(kvfAclTable);
+                auto acl_table = orch->getAclTable(acl_table_id);
+                ASSERT_NE(acl_table, nullptr);
+
+                // Verify mandaotry ACL actions has been added.
+                auto acl_actions = acl_table->type.getActions();
+                ASSERT_NE(acl_actions.find(SAI_ACL_ACTION_TYPE_COUNTER), acl_actions.end());
+                sai_acl_action_type_t action = strcmp(acl_table_stage, STAGE_INGRESS) == 0 ?
+                    SAI_ACL_ACTION_TYPE_MIRROR_INGRESS : SAI_ACL_ACTION_TYPE_MIRROR_EGRESS;
+                ASSERT_NE(acl_actions.find(action), acl_actions.end());
+
+                // Delete ACL table.
+                kvfAclTable = deque<KeyOpFieldsValuesTuple>(
+                    { { acl_table_id,
+                        DEL_COMMAND,
+                        {} } });
+                orch->doAclTableTask(kvfAclTable);
+                acl_table = orch->getAclTable(acl_table_id);
+                ASSERT_EQ(acl_table, nullptr);
+            }
+        }
+
+        // Unset platform env.
+        if (unset_platform_env)
+        {
+            unsetenv("platform");
+        }
+
+        // Restore sai_switch_api.
+        sai_switch_api = old_sai_switch_api;
+    }
 } // namespace nsAclOrchTest