From 700492f3e19aa143ce5dbdcbbd166b857ddfc66d Mon Sep 17 00:00:00 2001 From: Mickey Spiegel Date: Thu, 16 Jun 2022 17:51:22 -0700 Subject: [PATCH] [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. --- orchagent/aclorch.cpp | 133 +++++++++++------------------------------- orchagent/aclorch.h | 15 +---- orchagent/acltable.h | 1 - tests/test_dtel.py | 106 ++++++++++++++++++++++++++++++++- 4 files changed, 140 insertions(+), 115 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 73aa02dac936..24166a9c548a 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -1455,23 +1455,14 @@ shared_ptr AclRule::makeShared(AclOrch *acl, MirrorOrch *mirror, DTelOr { return make_shared(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(acl, dtel, rule, table); - } - else - { - return make_shared(acl, dtel, rule, table); - } + return make_shared(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(&rule); - if (!dtelDropWathcListRule) + auto dtelWatchListRule = dynamic_cast(&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(SAI_ACL_TABLE_ATTR_FIELD_ETHER_TYPE)) .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_SRC_IP)) @@ -4635,31 +4572,28 @@ void AclOrch::createDTelWatchListTables() .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_INNER_ETHER_TYPE)) .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_INNER_SRC_IP)) .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_INNER_DST_IP)) + .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_OUTER_VLAN_ID)) + .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_ACL_IP_TYPE)) + .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_TCP_FLAGS)) + .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_DSCP)) + .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_SRC_IPV6)) + .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_DST_IPV6)) + .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_ICMP_TYPE)) + .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_ICMP_CODE)) + .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_ICMPV6_TYPE)) + .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_ICMPV6_CODE)) + .withMatch(make_shared(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(SAI_ACL_TABLE_ATTR_FIELD_ETHER_TYPE)) - .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_SRC_IP)) - .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_DST_IP)) - .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_L4_SRC_PORT)) - .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_L4_DST_PORT)) - .withMatch(make_shared(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() @@ -4667,7 +4601,6 @@ void AclOrch::deleteDTelWatchListTables() SWSS_LOG_ENTER(); removeAclTable(TABLE_TYPE_DTEL_FLOW_WATCHLIST); - removeAclTable(TABLE_TYPE_DTEL_DROP_WATCHLIST); } void AclOrch::registerFlexCounter(const AclRule& rule) diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 02631d934e73..ee17ba4a1f66 100644 --- a/orchagent/aclorch.h +++ b/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: diff --git a/orchagent/acltable.h b/orchagent/acltable.h index 3ec7f1a757e0..2d91a84b9863 100644 --- a/orchagent/acltable.h +++ b/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" diff --git a/tests/test_dtel.py b/tests/test_dtel.py index b45ba13972b5..c8e86d6b7d3f 100644 --- a/tests/test_dtel.py +++ b/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):