From 0052e0978675e2cd7cf0ef41a27f07d60d1800e8 Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 15 Nov 2024 15:57:20 -0800 Subject: [PATCH] [ACL] Add Tunnel Next Hop redirect support Signed-off-by: Vivek Reddy --- orchagent/aclorch.cpp | 87 +++++++- orchagent/aclorch.h | 24 +++ orchagent/vxlanorch.cpp | 6 +- tests/mock_tests/Makefile.am | 1 + tests/mock_tests/aclorch_rule_ut.cpp | 306 +++++++++++++++++++++++++++ tests/mock_tests/check.h | 96 +++++++-- 6 files changed, 489 insertions(+), 31 deletions(-) create mode 100644 tests/mock_tests/aclorch_rule_ut.cpp diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 570d40825c..38982c945c 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -11,6 +11,7 @@ #include "timer.h" #include "crmorch.h" #include "sai_serialize.h" +#include "directory.h" using namespace std; using namespace swss; @@ -30,6 +31,7 @@ extern PortsOrch* gPortsOrch; extern CrmOrch *gCrmOrch; extern SwitchOrch *gSwitchOrch; extern string gMySwitchType; +extern Directory gDirectory; #define MIN_VLAN_ID 1 // 0 is a reserved VLAN ID #define MAX_VLAN_ID 4095 // 4096 is a reserved VLAN ID @@ -617,6 +619,58 @@ bool AclTableRangeMatch::validateAclRuleMatch(const AclRule& rule) const return true; } +void AclRule::TunnelNH::load(const std::string& target) +{ + parse(target); + + VxlanTunnelOrch* vxlan_orch = gDirectory.get(); + if (!vxlan_orch) + { + throw std::logic_error("VxlanTunnelOrch couldn't be retrieved"); + } + + tunn_ = vxlan_orch->getVxlanTunnel(tunnel_name); + if (!tunn_) + { + throw std::logic_error("Invalid Tunnel object"); + } + + nh = tunn_->getNextHop(endpoint_ip, mac, 0); +} + +void AclRule::TunnelNH::parse(const std::string& target) +{ + /* Supported Format: endpoint_ip@tunnel_name */ + auto at_pos = target.find('@'); + if (at_pos == std::string::npos) + { + throw std::logic_error("Invalid format for Tunnel Next Hop"); + } + + endpoint_ip = swss::IpAddress(target.substr(0, at_pos)); + tunnel_name = target.substr(at_pos + 1); +} + +void AclRule::TunnelNH::incRefCount() +{ + if (tunn_ && nh != SAI_NULL_OBJECT_ID) + { + tunn_->incNextHopRefCount(endpoint_ip, mac, vni); + } +} +void AclRule::TunnelNH::decRefCount() +{ + if (tunn_ && nh != SAI_NULL_OBJECT_ID) + { + tunn_->decNextHopRefCount(endpoint_ip, mac, vni); + } +} + +void AclRule::TunnelNH::clear() +{ + nh = SAI_NULL_OBJECT_ID; +} + string AclTableType::getName() const { return m_name; @@ -1308,6 +1362,11 @@ void AclRule::decreaseNextHopRefCount() } m_redirect_target_next_hop_group.clear(); } + if (m_redirect_target_tun_nh.get_nh_oid() != SAI_NULL_OBJECT_ID) + { + m_redirect_target_tun_nh.decRefCount(); + m_redirect_target_tun_nh.clear(); + } return; } @@ -2001,21 +2060,35 @@ sai_object_id_t AclRulePacket::getRedirectObjectId(const string& redirect_value) try { NextHopKey nh(target); - if (!m_pAclOrch->m_neighOrch->hasNextHop(nh)) + if (m_pAclOrch->m_neighOrch->hasNextHop(nh)) { - SWSS_LOG_ERROR("ACL Redirect action target next hop ip: '%s' doesn't exist on the switch", nh.to_string().c_str()); - return SAI_NULL_OBJECT_ID; + m_redirect_target_next_hop = target; + m_pAclOrch->m_neighOrch->increaseNextHopRefCount(nh); + return m_pAclOrch->m_neighOrch->getNextHopId(nh); } - - m_redirect_target_next_hop = target; - m_pAclOrch->m_neighOrch->increaseNextHopRefCount(nh); - return m_pAclOrch->m_neighOrch->getNextHopId(nh); } catch (...) { // no error, just try next variant } + // Try to parse if this is a tunnel nexthop. + try + { + m_redirect_target_tun_nh.load(target); + auto tunn_nh_oid = m_redirect_target_tun_nh.get_nh_oid(); + if (SAI_NULL_OBJECT_ID != tunn_nh_oid) + { + SWSS_LOG_INFO("Tunnel Next Hop Found: oid:%ld, target: %s", tunn_nh_oid, target.c_str()); + m_redirect_target_tun_nh.incRefCount(); + return tunn_nh_oid; + } + } + catch (std::logic_error& e) + { + // no error, just try next variant + } + // try to parse nh group the set of try { diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 9e27be2ac0..e086e1285d 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -15,6 +15,7 @@ #include "mirrororch.h" #include "dtelorch.h" #include "observer.h" +#include "vxlanorch.h" #include "flex_counter_manager.h" #include "acltable.h" @@ -286,6 +287,28 @@ class AclTable; class AclRule { public: + struct TunnelNH + { + TunnelNH() = default; + ~TunnelNH() = default; + + void load(const std::string& target); + void parse(const std::string& target); + + sai_object_id_t get_nh_oid() { return nh; } + + void clear(); + void incRefCount(); + void decRefCount(); + + std::string tunnel_name; + swss::IpAddress endpoint_ip; + swss::MacAddress mac; + uint32_t vni = 0; + VxlanTunnel* tunn_ = nullptr; + sai_object_id_t nh = SAI_NULL_OBJECT_ID; + }; + AclRule(AclOrch *pAclOrch, string rule, string table, bool createCounter = true); virtual bool validateAddPriority(string attr_name, string attr_value); virtual bool validateAddMatch(string attr_name, string attr_value); @@ -359,6 +382,7 @@ class AclRule map m_matches; string m_redirect_target_next_hop; string m_redirect_target_next_hop_group; + AclRule::TunnelNH m_redirect_target_tun_nh; vector m_rangeConfig; vector m_ranges; diff --git a/orchagent/vxlanorch.cpp b/orchagent/vxlanorch.cpp index 05a2d3e603..19b5674f0b 100644 --- a/orchagent/vxlanorch.cpp +++ b/orchagent/vxlanorch.cpp @@ -596,10 +596,10 @@ bool VxlanTunnel::removeNextHop(IpAddress& ipAddr, MacAddress macAddress, uint32 ipAddr.to_string().c_str(), macAddress.to_string().c_str(), vni, nh_tunnels_[key].ref_count); - //Decrement ref count if already exists - nh_tunnels_[key].ref_count --; - if (!nh_tunnels_[key].ref_count) + int curr_ref = nh_tunnels_[key].ref_count; + + if (!--curr_ref) { if (sai_next_hop_api->remove_next_hop(nh_tunnels_[key].nh_id) != SAI_STATUS_SUCCESS) { diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 126f4e88c5..3f4752ef74 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -26,6 +26,7 @@ LDADD_GTEST = -L/usr/src/gtest tests_INCLUDES = -I $(FLEX_CTR_DIR) -I $(DEBUG_CTR_DIR) -I $(top_srcdir)/lib -I$(top_srcdir)/cfgmgr -I$(top_srcdir)/orchagent -I$(P4_ORCH_DIR)/tests -I$(DASH_ORCH_DIR) -I$(top_srcdir)/warmrestart tests_SOURCES = aclorch_ut.cpp \ + aclorch_rule_ut.cpp \ portsorch_ut.cpp \ routeorch_ut.cpp \ qosorch_ut.cpp \ diff --git a/tests/mock_tests/aclorch_rule_ut.cpp b/tests/mock_tests/aclorch_rule_ut.cpp new file mode 100644 index 0000000000..50b141539e --- /dev/null +++ b/tests/mock_tests/aclorch_rule_ut.cpp @@ -0,0 +1,306 @@ +#include "ut_helper.h" +#include "mock_orchagent_main.h" +#include "mock_sai_api.h" +#include "mock_orch_test.h" +#include "check.h" + +EXTERN_MOCK_FNS + +/* + This test provides a framework to mock create_acl_entry & remove_acl_entry API's +*/ +namespace aclorch_rule_test +{ + DEFINE_SAI_GENERIC_API_MOCK(acl, acl_entry); + /* To mock Redirect Action functionality */ + DEFINE_SAI_GENERIC_API_MOCK(next_hop, next_hop); + + using namespace ::testing; + using namespace std; + using namespace saimeta; + using namespace swss; + using namespace mock_orch_test; + + struct SaiMockState + { + /* Add extra attributes on demand */ + vector create_attrs; + sai_status_t create_status = SAI_STATUS_SUCCESS; + sai_status_t remove_status = SAI_STATUS_SUCCESS; + sai_object_id_t remove_oid; + sai_object_id_t create_oid; + + sai_status_t handleCreate(sai_object_id_t *sai, sai_object_id_t switch_id, uint32_t attr_count, const sai_attribute_t *attr_list) + { + *sai = create_oid; + create_attrs.clear(); + for (uint32_t i = 0; i < attr_count; ++i) + { + create_attrs.emplace_back(attr_list[i]); + } + return create_status; + } + + sai_status_t handleRemove(sai_object_id_t oid) + { + EXPECT_EQ(oid, remove_oid); + return remove_status; + } + }; + + struct AclOrchRuleTest : public MockOrchTest + { + unique_ptr aclMockState; + + void PostSetUp() override + { + INIT_SAI_API_MOCK(acl); + INIT_SAI_API_MOCK(next_hop); + MockSaiApis(); + + aclMockState = make_unique(); + /* Port init done is a pre-req for Aclorch */ + auto consumer = unique_ptr(new Consumer( + new swss::ConsumerStateTable(m_app_db.get(), APP_PORT_TABLE_NAME, 1, 1), gPortsOrch, APP_PORT_TABLE_NAME)); + consumer->addToSync({ { "PortInitDone", EMPTY_PREFIX, { { "", "" } } } }); + static_cast(gPortsOrch)->doTask(*consumer.get()); + } + + void PreTearDown() override + { + aclMockState.reset(); + RestoreSaiApis(); + } + + void doAclTableTypeTask(const deque &entries) + { + auto consumer = unique_ptr(new Consumer( + new swss::ConsumerStateTable(m_config_db.get(), CFG_ACL_TABLE_TYPE_TABLE_NAME, 1, 1), + gAclOrch, CFG_ACL_TABLE_TYPE_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gAclOrch)->doTask(*consumer); + } + + void doAclTableTask(const deque &entries) + { + auto consumer = unique_ptr(new Consumer( + new swss::ConsumerStateTable(m_config_db.get(), CFG_ACL_TABLE_TABLE_NAME, 1, 1), + gAclOrch, CFG_ACL_TABLE_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gAclOrch)->doTask(*consumer); + } + + void doAclRuleTask(const deque &entries) + { + auto consumer = unique_ptr(new Consumer( + new swss::ConsumerStateTable(m_config_db.get(), CFG_ACL_RULE_TABLE_NAME, 1, 1), + gAclOrch, CFG_ACL_RULE_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gAclOrch)->doTask(*consumer); + } + }; + + struct AclRedirectActionTest : public AclOrchRuleTest + { + string acl_table_type = "TEST_ACL_TABLE_TYPE"; + string acl_table = "TEST_ACL_TABLE"; + string acl_rule = "TEST_ACL_RULE"; + + string mock_tunnel_name = "tunnel0"; + string mock_invalid_tunnel_name = "tunnel1"; + string mock_src_ip = "20.0.0.1"; + string mock_nh_ip_str = "20.0.0.3"; + string mock_invalid_nh_ip_str = "20.0.0.4"; + sai_object_id_t nh_oid = 0x400000000064d; + + void PostSetUp() override + { + AclOrchRuleTest::PostSetUp(); + + /* Create a tunnel */ + auto consumer = unique_ptr(new Consumer( + new swss::ConsumerStateTable(m_app_db.get(), APP_VXLAN_TUNNEL_TABLE_NAME, 1, 1), + m_VxlanTunnelOrch, APP_VXLAN_TUNNEL_TABLE_NAME)); + + consumer->addToSync( + deque( + { + { + mock_tunnel_name, + SET_COMMAND, + { + { "src_ip", mock_src_ip } + } + } + } + )); + static_cast(m_VxlanTunnelOrch)->doTask(*consumer.get()); + } + + void PreTearDown() override + { + AclOrchRuleTest::PreTearDown(); + + /* Delete the Tunnel Object */ + auto consumer = unique_ptr(new Consumer( + new swss::ConsumerStateTable(m_app_db.get(), APP_VXLAN_TUNNEL_TABLE_NAME, 1, 1), + m_VxlanTunnelOrch, APP_VXLAN_TUNNEL_TABLE_NAME)); + + consumer->addToSync( + deque( + { + { + mock_tunnel_name, + DEL_COMMAND, + { } + } + } + )); + static_cast(m_VxlanTunnelOrch)->doTask(*consumer.get()); + } + + void createTunnelNH(string ip) + { + IpAddress mock_nh_ip(ip); + ASSERT_EQ(m_VxlanTunnelOrch->createNextHopTunnel(mock_tunnel_name, mock_nh_ip, MacAddress()), nh_oid); + } + + void populateAclTale() + { + /* Create a Table type and Table */ + doAclTableTypeTask({ + { + acl_table_type, + SET_COMMAND, + { + { ACL_TABLE_TYPE_MATCHES, MATCH_DST_IP }, + { ACL_TABLE_TYPE_ACTIONS, ACTION_REDIRECT_ACTION } + } + } + }); + doAclTableTask({ + { + acl_table, + SET_COMMAND, + { + { ACL_TABLE_TYPE, acl_table_type }, + { ACL_TABLE_STAGE, STAGE_INGRESS }, + } + } + }); + } + + void addTunnelNhRule(string ip, string tunnel_name) + { + /* Create a rule */ + doAclRuleTask({ + { + acl_table + "|" + acl_rule, + SET_COMMAND, + { + { RULE_PRIORITY, "9999" }, + { MATCH_DST_IP, "10.0.0.1/24" }, + { ACTION_REDIRECT_ACTION, ip + "@" + tunnel_name } + } + } + }); + } + + void delTunnelNhRule() + { + doAclRuleTask( + { + { + acl_table + "|" + acl_rule, + DEL_COMMAND, + { } + } + }); + } + + void setMockState() + { + aclMockState->create_status = SAI_STATUS_SUCCESS; + aclMockState->remove_status = SAI_STATUS_SUCCESS; + aclMockState->create_oid = nh_oid; + aclMockState->remove_oid = nh_oid; + } + }; + + TEST_F(AclRedirectActionTest, TunnelNH) + { + EXPECT_CALL(*mock_sai_next_hop_api, create_next_hop).WillOnce(DoAll(SetArgPointee<0>(nh_oid), + Return(SAI_STATUS_SUCCESS) + )); + EXPECT_CALL(*mock_sai_acl_api, create_acl_entry).WillOnce(testing::Invoke(aclMockState.get(), &SaiMockState::handleCreate)); + EXPECT_CALL(*mock_sai_acl_api, remove_acl_entry).WillOnce(testing::Invoke(aclMockState.get(), &SaiMockState::handleRemove)); + setMockState(); + populateAclTale(); + createTunnelNH(mock_nh_ip_str); + addTunnelNhRule(mock_nh_ip_str, mock_tunnel_name); + + SaiAttributeList attr_list(SAI_OBJECT_TYPE_ACL_ENTRY, vector({ + { "SAI_ACL_ENTRY_ATTR_TABLE_ID", sai_serialize_object_id(gAclOrch->getTableById(acl_table)) }, + { "SAI_ACL_ENTRY_ATTR_PRIORITY", "9999" }, + { "SAI_ACL_ENTRY_ATTR_ADMIN_STATE", "true" }, + { "SAI_ACL_ENTRY_ATTR_ACTION_COUNTER", "oid:0xfffffffffff"}, + { "SAI_ACL_ENTRY_ATTR_FIELD_DST_IP", "10.0.0.1&mask:255.255.255.0"}, + { "SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT", sai_serialize_object_id(nh_oid) } + }), false); + vector skip_list = {false, false, false, true, false, false}; /* skip checking counter */ + + /* Verify SAI attributes and if the rule is created */ + ASSERT_TRUE(Check::AttrListSubset(SAI_OBJECT_TYPE_ACL_ENTRY, aclMockState->create_attrs, attr_list, skip_list)); + ASSERT_TRUE(gAclOrch->getAclRule(acl_table, acl_rule)); + delTunnelNhRule(); + ASSERT_FALSE(gAclOrch->getAclRule(acl_table, acl_rule)); + } + + TEST_F(AclRedirectActionTest, TunnelNHRefCnt) + { + EXPECT_CALL(*mock_sai_next_hop_api, create_next_hop).WillOnce(DoAll(SetArgPointee<0>(nh_oid), + Return(SAI_STATUS_SUCCESS) + )); + EXPECT_CALL(*mock_sai_acl_api, create_acl_entry).WillOnce(testing::Invoke(aclMockState.get(), &SaiMockState::handleCreate)); + setMockState(); + populateAclTale(); + createTunnelNH(mock_nh_ip_str); + addTunnelNhRule(mock_nh_ip_str, mock_tunnel_name); + + IpAddress mock_nh_ip(mock_nh_ip_str); + /* deleting Tunnel NH oid before ACL Rule will not work */ + EXPECT_CALL(*mock_sai_next_hop_api, remove_next_hop).Times(0); + ASSERT_TRUE(m_VxlanTunnelOrch->removeNextHopTunnel(mock_tunnel_name, mock_nh_ip, MacAddress())); + + /* Remove ACL Rule */ + EXPECT_CALL(*mock_sai_acl_api, remove_acl_entry).WillOnce(testing::Invoke(aclMockState.get(), &SaiMockState::handleRemove)); + delTunnelNhRule(); + ASSERT_FALSE(gAclOrch->getAclRule(acl_table, acl_rule)); + + /* Removing next_hop should proceed */ + EXPECT_CALL(*mock_sai_next_hop_api, remove_next_hop).WillOnce(Return(SAI_STATUS_SUCCESS)); + ASSERT_TRUE(m_VxlanTunnelOrch->removeNextHopTunnel(mock_tunnel_name, mock_nh_ip, MacAddress())); + } + + TEST_F(AclRedirectActionTest, TunnelNHInvalidTunnel) + { + EXPECT_CALL(*mock_sai_acl_api, create_acl_entry).Times(0); + setMockState(); + populateAclTale(); + addTunnelNhRule(mock_nh_ip_str, mock_invalid_tunnel_name); + ASSERT_FALSE(gAclOrch->getAclRule(acl_table, acl_rule)); + } + + TEST_F(AclRedirectActionTest, TunnelNHInvalidNextHop) + { + EXPECT_CALL(*mock_sai_next_hop_api, create_next_hop).WillOnce(DoAll(SetArgPointee<0>(nh_oid), + Return(SAI_STATUS_SUCCESS) + )); + EXPECT_CALL(*mock_sai_acl_api, create_acl_entry).Times(0); + setMockState(); + populateAclTale(); + createTunnelNH(mock_nh_ip_str); + addTunnelNhRule(mock_invalid_nh_ip_str, mock_tunnel_name); + ASSERT_FALSE(gAclOrch->getAclRule(acl_table, acl_rule)); + } +} diff --git a/tests/mock_tests/check.h b/tests/mock_tests/check.h index d1b095562d..a13f2abd3c 100644 --- a/tests/mock_tests/check.h +++ b/tests/mock_tests/check.h @@ -42,40 +42,94 @@ struct Check std::cerr << "Expected: " << meta->attridname << "\n"; } } - continue; } - const int MAX_BUF_SIZE = 0x4000; - std::string act_str; - std::string exp_str; + const sai_attribute_t* act = &act_attr_list[i]; + const sai_attribute_t* exp = &exp_attr_list.get_attr_list()[i]; + if (!Check::AttrValue(objecttype, id, act, exp)) + { + return false; + } + } + + return true; + } + + static bool AttrValue(sai_object_type_t objecttype, sai_attr_id_t id, const sai_attribute_t* act, const sai_attribute_t* exp) + { + auto meta = sai_metadata_get_attr_metadata(objecttype, id); + assert(meta != nullptr); + + const int MAX_BUF_SIZE = 0x4000; + std::vector act_buf(MAX_BUF_SIZE); + std::vector exp_buf(MAX_BUF_SIZE); + + act_buf.reserve(MAX_BUF_SIZE); + exp_buf.reserve(MAX_BUF_SIZE); + + auto act_len = sai_serialize_attribute_value(act_buf.data(), meta, &act->value); + auto exp_len = sai_serialize_attribute_value(exp_buf.data(), meta, &exp->value); - act_str.reserve(MAX_BUF_SIZE); - exp_str.reserve(MAX_BUF_SIZE); + assert(act_len < act_str.size()); + assert(act_len < exp_str.size()); - auto act_len = sai_serialize_attribute_value(&act_str[0], meta, &act_attr_list[i].value); - auto exp_len = sai_serialize_attribute_value(&exp_str[0], meta, &exp_attr_list.get_attr_list()[i].value); + act_buf.resize(act_len); + exp_buf.resize(exp_len); - assert(act_len < act_str.size()); - assert(act_len < exp_str.size()); + std::string act_str(act_buf.begin(), act_buf.end()); + std::string exp_str(exp_buf.begin(), exp_buf.end()); + + if (act_len != exp_len) + { + std::cerr << "AttrValue length failed\n"; + std::cerr << "Actual: " << act_len << "," << act_str << "\n"; + std::cerr << "Expected: " << exp_len << "," << exp_str << "\n"; + return false; + } - if (act_len != exp_len) + if (act_str != exp_str) + { + std::cerr << "AttrValue string failed\n"; + std::cerr << "Actual: " << act_str << "\n"; + std::cerr << "Expected: " << exp_str << "\n"; + return false; + } + return true; + } + + static bool AttrListSubset(sai_object_type_t objecttype, const std::vector &act_attr_list, + saimeta::SaiAttributeList &exp_attr_list, const std::vector skip_check) + { + /* + Size of attributes should be equal and in the same order. + If the validation has to be skipped for certain attributes populate the skip_check. + */ + if (act_attr_list.size() != exp_attr_list.get_attr_count()) + { + std::cerr << "AttrListSubset size mismatch\n"; + return false; + } + if (act_attr_list.size() != skip_check.size()) + { + std::cerr << "AttrListSubset size mismatch\n"; + return false; + } + + for (uint32_t i = 0; i < exp_attr_list.get_attr_count(); ++i) + { + if (skip_check[i]) { - std::cerr << "AttrListEq failed\n"; - std::cerr << "Actual: " << act_str << "\n"; - std::cerr << "Expected: " << exp_str << "\n"; - return false; + continue; } - - if (act_str != exp_str) + sai_attr_id_t id = exp_attr_list.get_attr_list()[i].id; + const sai_attribute_t* act = &act_attr_list[i]; + const sai_attribute_t* exp = &exp_attr_list.get_attr_list()[i]; + if (!Check::AttrValue(objecttype, id, act, exp)) { - std::cerr << "AttrListEq failed\n"; - std::cerr << "Actual: " << act_str << "\n"; - std::cerr << "Expected: " << exp_str << "\n"; return false; } } - return true; } };