[aclorch] Fix and simplify DTel watchlist tables and entries (#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.

Author: mickeyspiegel

orchagent/aclorch.cpp

@@ -1455,23 +1455,14 @@ shared_ptr<AclRule> AclRule::makeShared(AclOrch *acl, MirrorOrch *mirror, DTelOr
         {
             return make_shared<AclRulePacket>(acl, rule, table);
         }
-        else if (aclDTelFlowOpTypeLookup.find(action) != aclDTelFlowOpTypeLookup.cend())
+        else if (aclDTelActionLookup.find(action) != aclDTelActionLookup.cend())
         {
             if (!dtel)
             {
                 throw runtime_error("DTel feature is not enabled. Watchlists cannot be configured");
             }
 
-            if (action == ACTION_DTEL_DROP_REPORT_ENABLE ||
-                action == ACTION_DTEL_TAIL_DROP_REPORT_ENABLE ||
-                action == ACTION_DTEL_REPORT_ALL_PACKETS)
-            {
-                return make_shared<AclRuleDTelDropWatchListEntry>(acl, dtel, rule, table);
-            }
-            else
-            {
-                return make_shared<AclRuleDTelFlowWatchListEntry>(acl, dtel, rule, table);
-            }
+            return make_shared<AclRuleDTelWatchListEntry>(acl, dtel, rule, table);
         }
     }
 
@@ -2447,13 +2438,13 @@ bool AclTable::clear()
     return true;
 }
 
-AclRuleDTelFlowWatchListEntry::AclRuleDTelFlowWatchListEntry(AclOrch *aclOrch, DTelOrch *dtel, string rule, string table) :
+AclRuleDTelWatchListEntry::AclRuleDTelWatchListEntry(AclOrch *aclOrch, DTelOrch *dtel, string rule, string table) :
         AclRule(aclOrch, rule, table),
         m_pDTelOrch(dtel)
 {
 }
 
-bool AclRuleDTelFlowWatchListEntry::validateAddAction(string attr_name, string attr_val)
+bool AclRuleDTelWatchListEntry::validateAddAction(string attr_name, string attr_val)
 {
     SWSS_LOG_ENTER();
 
@@ -2535,7 +2526,7 @@ bool AclRuleDTelFlowWatchListEntry::validateAddAction(string attr_name, string a
     return setAction(aclDTelActionLookup[attr_name], actionData);
 }
 
-bool AclRuleDTelFlowWatchListEntry::validate()
+bool AclRuleDTelWatchListEntry::validate()
 {
     SWSS_LOG_ENTER();
 
@@ -2552,19 +2543,19 @@ bool AclRuleDTelFlowWatchListEntry::validate()
     return true;
 }
 
-bool AclRuleDTelFlowWatchListEntry::createRule()
+bool AclRuleDTelWatchListEntry::createRule()
 {
     SWSS_LOG_ENTER();
 
     return activate();
 }
 
-bool AclRuleDTelFlowWatchListEntry::removeRule()
+bool AclRuleDTelWatchListEntry::removeRule()
 {
     return deactivate();
 }
 
-bool AclRuleDTelFlowWatchListEntry::activate()
+bool AclRuleDTelWatchListEntry::activate()
 {
     SWSS_LOG_ENTER();
 
@@ -2581,7 +2572,7 @@ bool AclRuleDTelFlowWatchListEntry::activate()
     return AclRule::createRule();
 }
 
-bool AclRuleDTelFlowWatchListEntry::deactivate()
+bool AclRuleDTelWatchListEntry::deactivate()
 {
     SWSS_LOG_ENTER();
 
@@ -2612,7 +2603,7 @@ bool AclRuleDTelFlowWatchListEntry::deactivate()
     return true;
 }
 
-void AclRuleDTelFlowWatchListEntry::onUpdate(SubjectType type, void *cntx)
+void AclRuleDTelWatchListEntry::onUpdate(SubjectType type, void *cntx)
 {
     sai_acl_action_data_t actionData;
     sai_object_id_t session_oid = SAI_NULL_OBJECT_ID;
@@ -2673,72 +2664,19 @@ void AclRuleDTelFlowWatchListEntry::onUpdate(SubjectType type, void *cntx)
     }
 }
 
-bool AclRuleDTelFlowWatchListEntry::update(const AclRule& rule)
+bool AclRuleDTelWatchListEntry::update(const AclRule& rule)
 {
-    auto dtelDropWathcListRule = dynamic_cast<const AclRuleDTelFlowWatchListEntry*>(&rule);
-    if (!dtelDropWathcListRule)
+    auto dtelWatchListRule = dynamic_cast<const AclRuleDTelWatchListEntry*>(&rule);
+    if (!dtelWatchListRule)
     {
-        SWSS_LOG_ERROR("Cannot update DTEL flow watch list rule with a rule of a different type");
+        SWSS_LOG_ERROR("Cannot update DTEL watch list rule with a rule of a different type");
         return false;
     }
 
-    SWSS_LOG_ERROR("Updating DTEL flow watch list rule is currently not implemented");
+    SWSS_LOG_ERROR("Updating DTEL watch list rule is currently not implemented");
     return false;
 }
 
-AclRuleDTelDropWatchListEntry::AclRuleDTelDropWatchListEntry(AclOrch *aclOrch, DTelOrch *dtel, string rule, string table) :
-        AclRule(aclOrch, rule, table),
-        m_pDTelOrch(dtel)
-{
-}
-
-bool AclRuleDTelDropWatchListEntry::validateAddAction(string attr_name, string attr_val)
-{
-    SWSS_LOG_ENTER();
-
-    if (!m_pDTelOrch)
-    {
-        return false;
-    }
-
-    sai_acl_action_data_t actionData;
-    string attr_value = to_upper(attr_val);
-
-    if (attr_name != ACTION_DTEL_DROP_REPORT_ENABLE &&
-        attr_name != ACTION_DTEL_TAIL_DROP_REPORT_ENABLE &&
-        attr_name != ACTION_DTEL_REPORT_ALL_PACKETS)
-    {
-        return false;
-    }
-
-    actionData.parameter.booldata = (attr_value == DTEL_ENABLED) ? true : false;
-    actionData.enable = (attr_value == DTEL_ENABLED) ? true : false;
-
-    return setAction(aclDTelActionLookup[attr_name], actionData);
-}
-
-bool AclRuleDTelDropWatchListEntry::validate()
-{
-    SWSS_LOG_ENTER();
-
-    if (!m_pDTelOrch)
-    {
-        return false;
-    }
-
-    if ((m_rangeConfig.empty() && m_matches.empty()) || m_actions.size() == 0)
-    {
-        return false;
-    }
-
-    return true;
-}
-
-void AclRuleDTelDropWatchListEntry::onUpdate(SubjectType, void *)
-{
-    // Do nothing
-}
-
 AclRange::AclRange(sai_acl_range_type_t type, sai_object_id_t oid, int min, int max):
     m_oid(oid), m_refCnt(0), m_min(min), m_max(max), m_type(type)
 {
@@ -4619,11 +4557,10 @@ void AclOrch::createDTelWatchListTables()
 
     AclTableTypeBuilder builder;
 
-    AclTable flowWLTable(this, TABLE_TYPE_DTEL_FLOW_WATCHLIST);
-    AclTable dropWLTable(this, TABLE_TYPE_DTEL_DROP_WATCHLIST);
+    AclTable dtelWLTable(this, TABLE_TYPE_DTEL_FLOW_WATCHLIST);
 
-    flowWLTable.validateAddStage(ACL_STAGE_INGRESS);
-    flowWLTable.validateAddType(builder
+    dtelWLTable.validateAddStage(ACL_STAGE_INGRESS);
+    dtelWLTable.validateAddType(builder
         .withBindPointType(SAI_ACL_BIND_POINT_TYPE_SWITCH)
         .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ETHER_TYPE))
         .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_SRC_IP))
@@ -4635,39 +4572,35 @@ void AclOrch::createDTelWatchListTables()
         .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_INNER_ETHER_TYPE))
         .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_INNER_SRC_IP))
         .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_INNER_DST_IP))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_OUTER_VLAN_ID))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ACL_IP_TYPE))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_TCP_FLAGS))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_DSCP))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_SRC_IPV6))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_DST_IPV6))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ICMP_TYPE))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ICMP_CODE))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ICMPV6_TYPE))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ICMPV6_CODE))
+        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_IPV6_NEXT_HEADER))
         .withAction(SAI_ACL_ACTION_TYPE_ACL_DTEL_FLOW_OP)
         .withAction(SAI_ACL_ACTION_TYPE_DTEL_INT_SESSION)
-        .withAction(SAI_ACL_ACTION_TYPE_DTEL_REPORT_ALL_PACKETS)
-        .withAction(SAI_ACL_ACTION_TYPE_DTEL_FLOW_SAMPLE_PERCENT)
-        .build()
-    );
-    flowWLTable.setDescription("Dataplane Telemetry Flow Watchlist table");
-
-    dropWLTable.validateAddStage(ACL_STAGE_INGRESS);
-    dropWLTable.validateAddType(builder
-        .withBindPointType(SAI_ACL_BIND_POINT_TYPE_SWITCH)
-        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ETHER_TYPE))
-        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_SRC_IP))
-        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_DST_IP))
-        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_L4_SRC_PORT))
-        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_L4_DST_PORT))
-        .withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_IP_PROTOCOL))
         .withAction(SAI_ACL_ACTION_TYPE_DTEL_DROP_REPORT_ENABLE)
         .withAction(SAI_ACL_ACTION_TYPE_DTEL_TAIL_DROP_REPORT_ENABLE)
+        .withAction(SAI_ACL_ACTION_TYPE_DTEL_REPORT_ALL_PACKETS)
+        .withAction(SAI_ACL_ACTION_TYPE_DTEL_FLOW_SAMPLE_PERCENT)
         .build()
     );
-    dropWLTable.setDescription("Dataplane Telemetry Drop Watchlist table");
+    dtelWLTable.setDescription("Dataplane Telemetry Watchlist table");
 
-    addAclTable(flowWLTable);
-    addAclTable(dropWLTable);
+    addAclTable(dtelWLTable);
 }
 
 void AclOrch::deleteDTelWatchListTables()
 {
     SWSS_LOG_ENTER();
 
     removeAclTable(TABLE_TYPE_DTEL_FLOW_WATCHLIST);
-    removeAclTable(TABLE_TYPE_DTEL_DROP_WATCHLIST);
 }
 
 void AclOrch::registerFlexCounter(const AclRule& rule)

orchagent/aclorch.h

@@ -339,10 +339,10 @@ class AclRuleMirror: public AclRule
     MirrorOrch *m_pMirrorOrch {nullptr};
 };
 
-class AclRuleDTelFlowWatchListEntry: public AclRule
+class AclRuleDTelWatchListEntry: public AclRule
 {
 public:
-    AclRuleDTelFlowWatchListEntry(AclOrch *m_pAclOrch, DTelOrch *m_pDTelOrch, string rule, string table);
+    AclRuleDTelWatchListEntry(AclOrch *m_pAclOrch, DTelOrch *m_pDTelOrch, string rule, string table);
     bool validateAddAction(string attr_name, string attr_value);
     bool validate();
     bool createRule();
@@ -360,17 +360,6 @@ class AclRuleDTelFlowWatchListEntry: public AclRule
     bool INT_session_valid;
 };
 
-class AclRuleDTelDropWatchListEntry: public AclRule
-{
-public:
-    AclRuleDTelDropWatchListEntry(AclOrch *m_pAclOrch, DTelOrch *m_pDTelOrch, string rule, string table);
-    bool validateAddAction(string attr_name, string attr_value);
-    bool validate();
-    void onUpdate(SubjectType, void *) override;
-protected:
-    DTelOrch *m_pDTelOrch;
-};
-
 class AclTable
 {
 public:

orchagent/acltable.h

@@ -31,7 +31,6 @@ extern "C" {
 #define TABLE_TYPE_PFCWD                "PFCWD"
 #define TABLE_TYPE_CTRLPLANE            "CTRLPLANE"
 #define TABLE_TYPE_DTEL_FLOW_WATCHLIST  "DTEL_FLOW_WATCHLIST"
-#define TABLE_TYPE_DTEL_DROP_WATCHLIST  "DTEL_DROP_WATCHLIST"
 #define TABLE_TYPE_MCLAG                "MCLAG"
 #define TABLE_TYPE_MUX                  "MUX"
 #define TABLE_TYPE_DROP                 "DROP"

tests/test_dtel.py

@@ -211,7 +211,111 @@ def test_DtelQueueReportAttribs(self, dvs, testlog):
                     assert False
 
         tbl._del("Ethernet0|0")
-    
+
+    def test_DtelFlowWatchlist(self, dvs, testlog):
+        self.db = swsscommon.DBConnector(4, dvs.redis_sock, 0)
+        self.adb = swsscommon.DBConnector(1, dvs.redis_sock, 0)
+        self.table = "DTEL_FLOW_WATCHLIST"
+
+        fields_1=[("PRIORITY", "30"),
+                 ("ETHER_TYPE", "0x800"),
+                 ("L4_DST_PORT", "1674"),
+                 ("FLOW_OP", "POSTCARD"),
+                 ("REPORT_ALL_PACKETS", "FALSE"),
+                 ("DROP_REPORT_ENABLE", "TRUE"),
+                 ("TAIL_DROP_REPORT_ENABLE", "TRUE")]
+        fields_2=[("PRIORITY", "40"),
+                 ("ETHER_TYPE", "0x800"),
+                 ("L4_DST_PORT", "1674"),
+                 ("FLOW_OP", "POSTCARD"),
+                 ("REPORT_ALL_PACKETS", "TRUE"),
+                 ("DROP_REPORT_ENABLE", "FALSE"),
+                 ("TAIL_DROP_REPORT_ENABLE", "FALSE")]
+        fields_3=[("PRIORITY", "50"),
+                 ("ETHER_TYPE", "0x800"),
+                 ("L4_DST_PORT", "1674"),
+                 ("FLOW_OP", "POSTCARD"),
+                 ("REPORT_ALL_PACKETS", "TRUE")]
+        fields_4=[("PRIORITY", "60"),
+                 ("ETHER_TYPE", "0x800"),
+                 ("L4_DST_PORT", "1674"),
+                 ("REPORT_ALL_PACKETS", "TRUE"),
+                 ("DROP_REPORT_ENABLE", "TRUE"),
+                 ("TAIL_DROP_REPORT_ENABLE", "TRUE")]
+        fields_5=[("PRIORITY", "70"),
+                 ("ETHER_TYPE", "0x800"),
+                 ("L4_DST_PORT", "1674"),
+                 ("FLOW_OP", "NOP"),
+                 ("REPORT_ALL_PACKETS", "FALSE"),
+                 ("DROP_REPORT_ENABLE", "TRUE"),
+                 ("TAIL_DROP_REPORT_ENABLE", "TRUE")]
+        listfield = [fields_1, fields_2, fields_3, fields_4, fields_5]
+
+        for field in listfield:
+            k = listfield.index(field)
+            rule = "RULE-" + str(k)
+            self._create_dtel_acl_rule(self.table, rule, field)
+            self._check_dtel_acl_rule(dvs, rule)
+            self._remove_dtel_acl_rule(self.table, rule)
+
+    def _create_dtel_acl_rule(self, table, rule, field):
+        tbl = swsscommon.Table(self.db, "ACL_RULE")
+        fvs = swsscommon.FieldValuePairs(field)
+        tbl.set(table + "|" + rule, fvs)
+        time.sleep(1)
+
+    def _remove_dtel_acl_rule(self, table, rule):
+        tbl = swsscommon.Table(self.db, "ACL_RULE")
+        tbl._del(table + "|" + rule)
+        time.sleep(1)
+
+    def _check_dtel_acl_rule(self, dvs, rule):
+        time.sleep(1)
+        atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY")
+        keys = atbl.getKeys()
+        acl_entry = [k for k in keys if k not in dvs.asicdb.default_acl_entries]
+        assert len(acl_entry) != 0
+        (status, fvs) = atbl.get(acl_entry[0])
+        value = dict(fvs)
+        assert status
+
+        if rule == "RULE-0":
+            assert value["SAI_ACL_ENTRY_ATTR_PRIORITY"] == "30"
+            assert value["SAI_ACL_ENTRY_ATTR_FIELD_ETHER_TYPE"] == "2048&mask:0xffff"
+            assert value["SAI_ACL_ENTRY_ATTR_FIELD_L4_DST_PORT"] == "1674&mask:0xffff"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_ACL_DTEL_FLOW_OP"] == "SAI_ACL_DTEL_FLOW_OP_POSTCARD"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_REPORT_ALL_PACKETS"] == "disabled"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_DROP_REPORT_ENABLE"] == "true"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_TAIL_DROP_REPORT_ENABLE"] == "true"
+        elif rule == "RULE-1":
+            assert value["SAI_ACL_ENTRY_ATTR_PRIORITY"] == "40"
+            assert value["SAI_ACL_ENTRY_ATTR_FIELD_ETHER_TYPE"] == "2048&mask:0xffff"
+            assert value["SAI_ACL_ENTRY_ATTR_FIELD_L4_DST_PORT"] == "1674&mask:0xffff"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_ACL_DTEL_FLOW_OP"] == "SAI_ACL_DTEL_FLOW_OP_POSTCARD"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_REPORT_ALL_PACKETS"] == "true"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_DROP_REPORT_ENABLE"] == "disabled"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_TAIL_DROP_REPORT_ENABLE"] == "disabled"
+        elif rule == "RULE-2":
+            assert value["SAI_ACL_ENTRY_ATTR_PRIORITY"] == "50"
+            assert value["SAI_ACL_ENTRY_ATTR_FIELD_ETHER_TYPE"] == "2048&mask:0xffff"
+            assert value["SAI_ACL_ENTRY_ATTR_FIELD_L4_DST_PORT"] == "1674&mask:0xffff"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_ACL_DTEL_FLOW_OP"] == "SAI_ACL_DTEL_FLOW_OP_POSTCARD"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_REPORT_ALL_PACKETS"] == "true"
+        elif rule == "RULE-3":
+            assert value["SAI_ACL_ENTRY_ATTR_PRIORITY"] == "60"
+            assert value["SAI_ACL_ENTRY_ATTR_FIELD_ETHER_TYPE"] == "2048&mask:0xffff"
+            assert value["SAI_ACL_ENTRY_ATTR_FIELD_L4_DST_PORT"] == "1674&mask:0xffff"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_REPORT_ALL_PACKETS"] == "true"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_DROP_REPORT_ENABLE"] == "true"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_TAIL_DROP_REPORT_ENABLE"] == "true"
+        elif rule == "RULE-4":
+            assert value["SAI_ACL_ENTRY_ATTR_PRIORITY"] == "70"
+            assert value["SAI_ACL_ENTRY_ATTR_FIELD_ETHER_TYPE"] == "2048&mask:0xffff"
+            assert value["SAI_ACL_ENTRY_ATTR_FIELD_L4_DST_PORT"] == "1674&mask:0xffff"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_ACL_DTEL_FLOW_OP"] == "SAI_ACL_DTEL_FLOW_OP_NOP"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_REPORT_ALL_PACKETS"] == "disabled"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_DROP_REPORT_ENABLE"] == "true"
+            assert value["SAI_ACL_ENTRY_ATTR_ACTION_DTEL_TAIL_DROP_REPORT_ENABLE"] == "true"
 
     def test_DtelEventAttribs(self, dvs, testlog):