[aclorch] add generic AclOrch::updateAclRule() method (#1993)

stepanblyschak · web-flow · commit da21172e06dd · 2021-11-22T18:27:25.000+02:00
- What I did
I added generic AclOrch::updateAclRule() implementation. This implementation takes a newly constructed shared_ptr&lt;AclRule&gt; provided by the user, calculates a difference between priority, matches and actions and calls sai_acl_api-&gt;set_acl_entry_attribute() for those attributes that were changed. The derivatives of AclRule can customize the behaviour of AclRule::update(const AclRule&amp; updatedRule) by polymorphic override, although in that case the derivative needs dynamic_cast the updatedRule to a concrete derivative type.
Added unit test to cover update scenario. Currently this new API is not used by any clients nor handling ACL_RULE table updates. This API will be used by PBH ACL rule update flow.

- Why I did it
To support the scenario for updating PBH ACL rule in the future.

- How I verified it
Temporary changed the ACL_RULE table update to leverage this new API and tested updates through CONFIG_DB and unit test coverage.

Signed-off-by: Stepan Blyshchak &lt;stepanb@nvidia.com&gt;
diff --git a/orchagent/Makefile.am b/orchagent/Makefile.am
@@ -64,6 +64,7 @@ orchagent_SOURCES = \
             pbh/pbhrule.cpp \
             pbhorch.cpp \
             saihelper.cpp \
+            saiattr.cpp \
             switchorch.cpp \
             pfcwdorch.cpp \
             pfcactionhandler.cpp \
diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp
diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h
@@ -19,6 +19,8 @@
 
 #include "acltable.h"
 
+#include "saiattr.h"
+
 #define RULE_PRIORITY           "PRIORITY"
 #define MATCH_IN_PORTS          "IN_PORTS"
 #define MATCH_OUT_PORTS         "OUT_PORTS"
@@ -93,6 +95,7 @@
 #define ACL_COUNTER_FLEX_COUNTER_GROUP "ACL_STAT_COUNTER"
 
 typedef map<string, sai_acl_entry_attr_t> acl_rule_attr_lookup_t;
+typedef map<string, sai_acl_range_type_t> acl_range_type_lookup_t;
 typedef map<string, sai_acl_ip_type_t> acl_ip_type_lookup_t;
 typedef map<string, sai_acl_dtel_flow_op_t> acl_dtel_flow_op_type_lookup_t;
 typedef map<string, sai_packet_action_t> acl_packet_action_lookup_t;
@@ -102,6 +105,13 @@ typedef map<sai_acl_action_type_t, set<int32_t>> acl_action_enum_values_capabili
 
 class AclOrch;
 
+struct AclRangeConfig
+{
+    sai_acl_range_type_t rangeType;
+    uint32_t min;
+    uint32_t max;
+};
+
 class AclRange
 {
 public:
@@ -130,7 +140,7 @@ class AclRule
     AclRule(AclOrch *pAclOrch, string rule, string table, acl_table_type_t type, bool createCounter = true);
     virtual bool validateAddPriority(string attr_name, string attr_value);
     virtual bool validateAddMatch(string attr_name, string attr_value);
-    virtual bool validateAddAction(string attr_name, string attr_value);
+    virtual bool validateAddAction(string attr_name, string attr_value) = 0;
     virtual bool validate() = 0;
     bool processIpType(string type, sai_uint32_t &ip_type);
     inline static void setRulePriorities(sai_uint32_t min, sai_uint32_t max)
@@ -140,8 +150,9 @@ class AclRule
     }
 
     virtual bool create();
+    virtual bool update(const AclRule& updatedRule);
     virtual bool remove();
-    virtual void update(SubjectType, void *) = 0;
+    virtual void onUpdate(SubjectType, void *) = 0;
     virtual void updateInPorts();
 
     virtual bool enableCounter();
@@ -167,16 +178,13 @@ class AclRule
         return m_counterOid;
     }
 
+    vector<sai_object_id_t> getInPorts() const;
+
     bool hasCounter() const
     {
         return getCounterOid() != SAI_NULL_OBJECT_ID;
     }
 
-    vector<sai_object_id_t> getInPorts()
-    {
-        return m_inPorts;
-    }
-
     static shared_ptr<AclRule> makeShared(acl_table_type_t type, AclOrch *acl, MirrorOrch *mirror, DTelOrch *dtel, const string& rule, const string& table, const KeyOpFieldsValuesTuple&);
     virtual ~AclRule() {}
 
@@ -187,6 +195,17 @@ class AclRule
     virtual bool removeRanges();
     virtual bool removeRule();
 
+    virtual bool updatePriority(const AclRule& updatedRule);
+    virtual bool updateMatches(const AclRule& updatedRule);
+    virtual bool updateActions(const AclRule& updatedRule);
+    virtual bool updateCounter(const AclRule& updatedRule);
+
+    virtual bool setPriority(const sai_uint32_t &value);
+    virtual bool setAction(sai_acl_entry_attr_t actionId, sai_acl_action_data_t actionData);
+    virtual bool setMatch(sai_acl_entry_attr_t matchId, sai_acl_field_data_t matchData);
+
+    virtual bool setAttribute(sai_attribute_t attr);
+
     void decreaseNextHopRefCount();
 
     bool isActionSupported(sai_acl_entry_attr_t) const;
@@ -201,13 +220,13 @@ class AclRule
     sai_object_id_t m_ruleOid;
     sai_object_id_t m_counterOid;
     uint32_t m_priority;
-    map <sai_acl_entry_attr_t, sai_attribute_value_t> m_matches;
-    map <sai_acl_entry_attr_t, sai_attribute_value_t> m_actions;
+    map <sai_acl_entry_attr_t, SaiAttrWrapper> m_actions;
+    map <sai_acl_entry_attr_t, SaiAttrWrapper> m_matches;
     string m_redirect_target_next_hop;
     string m_redirect_target_next_hop_group;
 
-    vector<sai_object_id_t> m_inPorts;
-    vector<sai_object_id_t> m_outPorts;
+    vector<AclRangeConfig> m_rangeConfig;
+    vector<AclRange*> m_ranges;
 
 private:
     bool m_createCounter;
@@ -221,7 +240,8 @@ class AclRuleL3: public AclRule
     bool validateAddAction(string attr_name, string attr_value);
     bool validateAddMatch(string attr_name, string attr_value);
     bool validate();
-    void update(SubjectType, void *);
+    void onUpdate(SubjectType, void *) override;
+
 protected:
     sai_object_id_t getRedirectObjectId(const string& redirect_param);
 };
@@ -256,11 +276,12 @@ class AclRuleMirror: public AclRule
     bool validate();
     bool createRule();
     bool removeRule();
-    void update(SubjectType, void *);
+    void onUpdate(SubjectType, void *) override;
 
     bool activate();
     bool deactivate();
 
+    bool update(const AclRule& updatedRule) override;
 protected:
     bool m_state {false};
     string m_sessionName;
@@ -275,11 +296,12 @@ class AclRuleDTelFlowWatchListEntry: public AclRule
     bool validate();
     bool createRule();
     bool removeRule();
-    void update(SubjectType, void *);
+    void onUpdate(SubjectType, void *) override;
 
     bool activate();
     bool deactivate();
 
+    bool update(const AclRule& updatedRule) override;
 protected:
     DTelOrch *m_pDTelOrch;
     string m_intSessionId;
@@ -293,8 +315,7 @@ class AclRuleDTelDropWatchListEntry: public AclRule
     AclRuleDTelDropWatchListEntry(AclOrch *m_pAclOrch, DTelOrch *m_pDTelOrch, string rule, string table, acl_table_type_t type);
     bool validateAddAction(string attr_name, string attr_value);
     bool validate();
-    void update(SubjectType, void *);
-
+    void onUpdate(SubjectType, void *) override;
 protected:
     DTelOrch *m_pDTelOrch;
 };
@@ -342,12 +363,14 @@ class AclTable
     void unlink(sai_object_id_t portOid);
     // Add or overwrite a rule into the ACL table
     bool add(shared_ptr<AclRule> newRule);
+    // Update existing ACL rule
+    bool updateRule(shared_ptr<AclRule> updatedRule);
     // Remove a rule from the ACL table
     bool remove(string rule_id);
     // Remove all rules from the ACL table
     bool clear();
     // Update table subject to changes
-    void update(SubjectType, void *);
+    void onUpdate(SubjectType, void *);
 
 public:
     string id;
@@ -403,6 +426,7 @@ class AclOrch : public Orch, public Observer
     bool updateAclTable(string table_id, AclTable &table);
     bool addAclRule(shared_ptr<AclRule> aclRule, string table_id);
     bool removeAclRule(string table_id, string rule_id);
+    bool updateAclRule(shared_ptr<AclRule> updatedAclRule);
     bool updateAclRule(string table_id, string rule_id, string attr_name, void *data, bool oper);
     bool updateAclRule(string table_id, string rule_id, bool enableCounter);
     AclRule* getAclRule(string table_id, string rule_id);
diff --git a/orchagent/pbh/pbhrule.cpp b/orchagent/pbh/pbhrule.cpp
@@ -11,15 +11,7 @@ bool AclRulePbh::validateAddPriority(const sai_uint32_t &value)
 {
     SWSS_LOG_ENTER();
 
-    if ((value < m_minPriority) || (value > m_maxPriority))
-    {
-        SWSS_LOG_ERROR("Failed to validate priority: invalid value %d", value);
-        return false;
-    }
-
-    m_priority = value;
-
-    return true;
+    return setPriority(value);
 }
 
 bool AclRulePbh::validateAddMatch(const sai_attribute_t &attr)
@@ -52,9 +44,7 @@ bool AclRulePbh::validateAddMatch(const sai_attribute_t &attr)
         return false;
     }
 
-    m_matches[attrId] = attr.value;
-
-    return true;
+    return setMatch(attrId, attr.value.aclfield);
 }
 
 bool AclRulePbh::validateAddAction(const sai_attribute_t &attr)
@@ -83,15 +73,7 @@ bool AclRulePbh::validateAddAction(const sai_attribute_t &attr)
         return false;
     }
 
-    if (!AclRule::isActionSupported(attrId))
-    {
-        SWSS_LOG_ERROR("Action %s is not supported by ASIC", attrName.c_str());
-        return false;
-    }
-
-    m_actions[attrId] = attr.value;
-
-    return true;
+    return setAction(attrId, attr.value.aclaction);
 }
 
 bool AclRulePbh::validate()
@@ -107,7 +89,12 @@ bool AclRulePbh::validate()
     return true;
 }
 
-void AclRulePbh::update(SubjectType, void *)
+void AclRulePbh::onUpdate(SubjectType, void *)
 {
     // Do nothing
 }
+
+bool AclRulePbh::validateAddAction(string attr_name, string attr_value)
+{
+    SWSS_LOG_THROW("This API should not be used on PbhRule");
+}
diff --git a/orchagent/pbh/pbhrule.h b/orchagent/pbh/pbhrule.h
@@ -11,5 +11,6 @@ class AclRulePbh: public AclRule
     bool validateAddMatch(const sai_attribute_t &attr);
     bool validateAddAction(const sai_attribute_t &attr);
     bool validate() override;
-    void update(SubjectType, void *) override;
+    void onUpdate(SubjectType, void *) override;
+    bool validateAddAction(string attr_name, string attr_value) override;
 };
diff --git a/orchagent/saiattr.cpp b/orchagent/saiattr.cpp
@@ -0,0 +1,91 @@
+#include "saiattr.h"
+
+#include <swss/logger.h>
+#include <sai_serialize.h>
+
+SaiAttrWrapper::SaiAttrWrapper(sai_object_type_t objectType, const sai_attribute_t& attr)
+{
+    auto meta = sai_metadata_get_attr_metadata(objectType, attr.id);
+    if (!meta)
+    {
+        SWSS_LOG_THROW("Failed to get attribute %d metadata", attr.id);
+    }
+
+    init(objectType, *meta, attr);
+}
+
+SaiAttrWrapper::~SaiAttrWrapper()
+{
+    if (m_meta)
+    {
+        sai_deserialize_free_attribute_value(m_meta->attrvaluetype, m_attr);
+    }
+}
+
+SaiAttrWrapper::SaiAttrWrapper(const SaiAttrWrapper& other)
+{
+    init(other.m_objectType, *other.m_meta, other.m_attr);
+}
+
+SaiAttrWrapper& SaiAttrWrapper::operator=(const SaiAttrWrapper& other)
+{
+    init(other.m_objectType, *other.m_meta, other.m_attr);
+    return *this;
+}
+
+SaiAttrWrapper::SaiAttrWrapper(SaiAttrWrapper&& other)
+{
+    swap(std::move(other));
+}
+
+SaiAttrWrapper& SaiAttrWrapper::operator=(SaiAttrWrapper&& other)
+{
+    swap(std::move(other));
+    return *this;
+}
+
+bool SaiAttrWrapper::operator<(const SaiAttrWrapper& other) const
+{
+    return m_serializedAttr < other.m_serializedAttr;
+}
+
+const sai_attribute_t& SaiAttrWrapper::getSaiAttr() const
+{
+    return m_attr;
+}
+
+std::string SaiAttrWrapper::toString() const
+{
+    return m_serializedAttr;
+}
+
+sai_attr_id_t SaiAttrWrapper::getAttrId() const
+{
+    return m_attr.id;
+}
+
+void SaiAttrWrapper::swap(SaiAttrWrapper&& other)
+{
+    m_objectType = other.m_objectType;
+    m_meta = other.m_meta;
+    m_attr = other.m_attr;
+    m_serializedAttr = other.m_serializedAttr;
+    other.m_attr = sai_attribute_t{};
+    other.m_serializedAttr.clear();
+}
+
+void SaiAttrWrapper::init(
+    sai_object_type_t objectType,
+    const sai_attr_metadata_t& meta,
+    const sai_attribute_t& attr)
+{
+    m_objectType = objectType;
+    m_attr.id = attr.id;
+    m_meta = &meta;
+
+    m_serializedAttr = sai_serialize_attr_value(*m_meta, attr);
+
+    // deserialize to actually preform a deep copy of attr
+    // and attribute value's dynamically allocated lists.
+    sai_deserialize_attr_value(m_serializedAttr, *m_meta, m_attr);
+}
diff --git a/orchagent/saiattr.h b/orchagent/saiattr.h
@@ -0,0 +1,41 @@
+#pragma once
+
+extern "C"
+{
+#include <sai.h>
+#include <saimetadata.h>
+}
+
+#include <string>
+
+class SaiAttrWrapper
+{
+public:
+    SaiAttrWrapper() = default;
+
+    SaiAttrWrapper(sai_object_type_t objectType, const sai_attribute_t& attr);
+    SaiAttrWrapper(const SaiAttrWrapper& other);
+    SaiAttrWrapper(SaiAttrWrapper&& other);
+    SaiAttrWrapper& operator=(const SaiAttrWrapper& other);
+    SaiAttrWrapper& operator=(SaiAttrWrapper&& other);
+    virtual ~SaiAttrWrapper();
+
+    bool operator<(const SaiAttrWrapper& other) const;
+
+    const sai_attribute_t& getSaiAttr() const;
+    std::string toString() const;
+    sai_attr_id_t getAttrId() const;
+
+private:
+
+    void init(
+        sai_object_type_t objectType,
+        const sai_attr_metadata_t& meta,
+        const sai_attribute_t& attr);
+    void swap(SaiAttrWrapper&& other);
+
+    sai_object_type_t m_objectType {SAI_OBJECT_TYPE_NULL};
+    const sai_attr_metadata_t* m_meta {nullptr};
+    sai_attribute_t m_attr {};
+    std::string m_serializedAttr;
+};
diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am
@@ -59,6 +59,7 @@ tests_SOURCES = aclorch_ut.cpp \
                 $(top_srcdir)/orchagent/pbh/pbhrule.cpp \
                 $(top_srcdir)/orchagent/pbhorch.cpp \
                 $(top_srcdir)/orchagent/saihelper.cpp \
+                $(top_srcdir)/orchagent/saiattr.cpp \
                 $(top_srcdir)/orchagent/switchorch.cpp \
                 $(top_srcdir)/orchagent/pfcwdorch.cpp \
                 $(top_srcdir)/orchagent/pfcactionhandler.cpp \
diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp
diff --git a/tests/mock_tests/portal.h b/tests/mock_tests/portal.h

Original file line number	Diff line number	Diff line change
`@@ -11,15 +11,7 @@ bool AclRulePbh::validateAddPriority(const sai_uint32_t &value)`
`11`	`11`	`{`
`12`	`12`	`SWSS_LOG_ENTER();`
`13`	`13`
`14`		`- if ((value < m_minPriority) \|\| (value > m_maxPriority))`
`15`		`- {`
`16`		`- SWSS_LOG_ERROR("Failed to validate priority: invalid value %d", value);`
`17`		`- return false;`
`18`		`- }`
`19`		`-`
`20`		`- m_priority = value;`
`21`		`-`
`22`		`- return true;`
	`14`	`+ return setPriority(value);`
`23`	`15`	`}`
`24`	`16`
`25`	`17`	`bool AclRulePbh::validateAddMatch(const sai_attribute_t &attr)`
`@@ -52,9 +44,7 @@ bool AclRulePbh::validateAddMatch(const sai_attribute_t &attr)`
`52`	`44`	`return false;`
`53`	`45`	`}`
`54`	`46`
`55`		`- m_matches[attrId] = attr.value;`
`56`		`-`
`57`		`- return true;`
	`47`	`+ return setMatch(attrId, attr.value.aclfield);`
`58`	`48`	`}`
`59`	`49`
`60`	`50`	`bool AclRulePbh::validateAddAction(const sai_attribute_t &attr)`
`@@ -83,15 +73,7 @@ bool AclRulePbh::validateAddAction(const sai_attribute_t &attr)`
`83`	`73`	`return false;`
`84`	`74`	`}`
`85`	`75`
`86`		`- if (!AclRule::isActionSupported(attrId))`
`87`		`- {`
`88`		`- SWSS_LOG_ERROR("Action %s is not supported by ASIC", attrName.c_str());`
`89`		`- return false;`
`90`		`- }`
`91`		`-`
`92`		`- m_actions[attrId] = attr.value;`
`93`		`-`
`94`		`- return true;`
	`76`	`+ return setAction(attrId, attr.value.aclaction);`
`95`	`77`	`}`
`96`	`78`
`97`	`79`	`bool AclRulePbh::validate()`
`@@ -107,7 +89,12 @@ bool AclRulePbh::validate()`
`107`	`89`	`return true;`
`108`	`90`	`}`
`109`	`91`
`110`		`-void AclRulePbh::update(SubjectType, void *)`
	`92`	`+void AclRulePbh::onUpdate(SubjectType, void *)`
`111`	`93`	`{`
`112`	`94`	`// Do nothing`
`113`	`95`	`}`
	`96`	`+`
	`97`	`+bool AclRulePbh::validateAddAction(string attr_name, string attr_value)`
	`98`	`+{`
	`99`	`+ SWSS_LOG_THROW("This API should not be used on PbhRule");`
	`100`	`+}`