From 5a998b6eea8af87c2d59d1e751d09e824867b454 Mon Sep 17 00:00:00 2001 From: Lawrence Lee Date: Mon, 3 Jun 2024 09:36:55 -0700 Subject: [PATCH] Handle learning duplicate IPs on different VRFs (#3165) (#3179) - What I did Fixes sonic-net/sonic-buildimage#18890 If we try to learn an existing neighbor on a different VLAN in the same VRF, delete the old neighbor entry before creating the new one. For all other scenarios, proceed with neighbor learning normally. - Why I did it Allow learning the same IP in two different VRFs - How I verified it Run the C++ unit tests Signed-off-by: Lawrence Lee Co-authored-by: Prince Sunny --- orchagent/neighorch.cpp | 30 +++++- tests/mock_tests/mock_orch_test.h | 8 ++ tests/mock_tests/neighorch_ut.cpp | 158 +++++++++++++++++++++--------- 3 files changed, 149 insertions(+), 47 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 4008fb81fb..7aa98addc3 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -945,12 +945,36 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress NeighborEntry temp_entry = { ip_address, vlan_port }; if (m_syncdNeighbors.find(temp_entry) != m_syncdNeighbors.end()) { - SWSS_LOG_NOTICE("Neighbor %s on %s already exists, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str()); - if (!removeNeighbor(temp_entry)) + // Neighbor already exists on another VLAN. If they belong to the same VRF, delete the old neighbor + Port existing_vlan, new_vlan; + if (!gPortsOrch->getPort(vlan_port, new_vlan)) { - SWSS_LOG_ERROR("Failed to remove neighbor %s on %s", ip_address.to_string().c_str(), vlan_port.c_str()); + SWSS_LOG_ERROR("Failed to get port for %s", vlan_port.c_str()); return false; } + if (!gPortsOrch->getPort(alias, existing_vlan)) + { + SWSS_LOG_ERROR("Failed to get port for %s", alias.c_str()); + return false; + } + if (existing_vlan.m_vr_id == new_vlan.m_vr_id) + { + std::string vrf_name = gDirectory.get()->getVRFname(existing_vlan.m_vr_id); + if (vrf_name.empty()) + { + SWSS_LOG_NOTICE("Neighbor %s already learned on %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str()); + } + else + { + SWSS_LOG_NOTICE("Neighbor %s already learned on %s in VRF %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str(), vrf_name.c_str()); + } + + if (!removeNeighbor(temp_entry)) + { + SWSS_LOG_ERROR("Failed to remove neighbor %s on %s", ip_address.to_string().c_str(), vlan_port.c_str()); + return false; + } + } } } diff --git a/tests/mock_tests/mock_orch_test.h b/tests/mock_tests/mock_orch_test.h index eefda42057..3508dd8c10 100644 --- a/tests/mock_tests/mock_orch_test.h +++ b/tests/mock_tests/mock_orch_test.h @@ -19,16 +19,24 @@ namespace mock_orch_test static const string PEER_IPV4_ADDRESS = "1.1.1.1"; static const string ACTIVE_INTERFACE = "Ethernet4"; static const string STANDBY_INTERFACE = "Ethernet8"; + static const string ETHERNET0 = "Ethernet0"; + static const string ETHERNET4 = "Ethernet4"; + static const string ETHERNET8 = "Ethernet8"; + static const string ETHERNET12 = "Ethernet12"; static const string ACTIVE_STATE = "active"; static const string STANDBY_STATE = "standby"; static const string STATE = "state"; static const string VLAN_1000 = "Vlan1000"; static const string VLAN_2000 = "Vlan2000"; + static const string VLAN_3000 = "Vlan3000"; + static const string VLAN_4000 = "Vlan4000"; static const string SERVER_IP1 = "192.168.0.2"; static const string SERVER_IP2 = "192.168.0.3"; static const string MAC1 = "62:f9:65:10:2f:01"; static const string MAC2 = "62:f9:65:10:2f:02"; static const string MAC3 = "62:f9:65:10:2f:03"; + static const string MAC4 = "62:f9:65:10:2f:04"; + static const string MAC5 = "62:f9:65:10:2f:05"; class MockOrchTest: public ::testing::Test { diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index 03957436a6..13e4ead4b0 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -9,7 +9,6 @@ #include "mock_sai_api.h" #include "mock_orch_test.h" - EXTERN_MOCK_FNS namespace neighorch_test @@ -21,15 +20,18 @@ namespace neighorch_test using ::testing::Throw; static const string TEST_IP = "10.10.10.10"; - static const NeighborEntry VLAN1000_NEIGH = NeighborEntry(TEST_IP, VLAN_1000); + static const string VRF_3000 = "Vrf3000"; + static const NeighborEntry VLAN1000_NEIGH = NeighborEntry(TEST_IP, VLAN_1000); static const NeighborEntry VLAN2000_NEIGH = NeighborEntry(TEST_IP, VLAN_2000); + static const NeighborEntry VLAN3000_NEIGH = NeighborEntry(TEST_IP, VLAN_3000); + static const NeighborEntry VLAN4000_NEIGH = NeighborEntry(TEST_IP, VLAN_4000); - class NeighOrchTest: public MockOrchTest + class NeighOrchTest : public MockOrchTest { protected: void SetAndAssertMuxState(std::string interface, std::string state) { - MuxCable* muxCable = m_MuxOrch->getMuxCable(interface); + MuxCable *muxCable = m_MuxOrch->getMuxCable(interface); muxCable->setState(state); EXPECT_EQ(state, muxCable->getState()); } @@ -46,34 +48,49 @@ namespace neighorch_test void ApplyInitialConfigs() { - Table peer_switch_table = Table(m_config_db.get(), CFG_PEER_SWITCH_TABLE_NAME); - Table tunnel_table = Table(m_app_db.get(), APP_TUNNEL_DECAP_TABLE_NAME); - Table mux_cable_table = Table(m_config_db.get(), CFG_MUX_CABLE_TABLE_NAME); Table port_table = Table(m_app_db.get(), APP_PORT_TABLE_NAME); Table vlan_table = Table(m_app_db.get(), APP_VLAN_TABLE_NAME); Table vlan_member_table = Table(m_app_db.get(), APP_VLAN_MEMBER_TABLE_NAME); Table neigh_table = Table(m_app_db.get(), APP_NEIGH_TABLE_NAME); Table intf_table = Table(m_app_db.get(), APP_INTF_TABLE_NAME); Table fdb_table = Table(m_app_db.get(), APP_FDB_TABLE_NAME); + Table vrf_table = Table(m_app_db.get(), APP_VRF_TABLE_NAME); auto ports = ut_helper::getInitialSaiPorts(); - port_table.set(ACTIVE_INTERFACE, ports[ACTIVE_INTERFACE]); - port_table.set(STANDBY_INTERFACE, ports[STANDBY_INTERFACE]); + port_table.set(ETHERNET0, ports[ETHERNET0]); + port_table.set(ETHERNET4, ports[ETHERNET4]); + port_table.set(ETHERNET8, ports[ETHERNET8]); port_table.set("PortConfigDone", { { "count", to_string(1) } }); port_table.set("PortInitDone", { {} }); + vrf_table.set(VRF_3000, { {"NULL", "NULL"} }); + vlan_table.set(VLAN_1000, { { "admin_status", "up" }, { "mtu", "9100" }, { "mac", "00:aa:bb:cc:dd:ee" } }); - vlan_table.set(VLAN_2000, { { "admin_status", "up"}, + vlan_table.set(VLAN_2000, { { "admin_status", "up" }, { "mtu", "9100" }, { "mac", "aa:11:bb:22:cc:33" } }); + vlan_table.set(VLAN_3000, { { "admin_status", "up" }, + { "mtu", "9100" }, + { "mac", "99:ff:88:ee:77:dd" } }); + vlan_table.set(VLAN_4000, { { "admin_status", "up" }, + { "mtu", "9100" }, + { "mac", "99:ff:88:ee:77:dd" } }); + vlan_member_table.set( + VLAN_1000 + vlan_member_table.getTableNameSeparator() + ETHERNET0, + { { "tagging_mode", "untagged" } }); + + vlan_member_table.set( + VLAN_2000 + vlan_member_table.getTableNameSeparator() + ETHERNET4, + { { "tagging_mode", "untagged" } }); + vlan_member_table.set( - VLAN_1000 + vlan_member_table.getTableNameSeparator() + ACTIVE_INTERFACE, + VLAN_3000 + vlan_member_table.getTableNameSeparator() + ETHERNET8, { { "tagging_mode", "untagged" } }); vlan_member_table.set( - VLAN_2000 + vlan_member_table.getTableNameSeparator() + STANDBY_INTERFACE, + VLAN_4000 + vlan_member_table.getTableNameSeparator() + ETHERNET12, { { "tagging_mode", "untagged" } }); intf_table.set(VLAN_1000, { { "grat_arp", "enabled" }, @@ -84,6 +101,16 @@ namespace neighorch_test { "proxy_arp", "enabled" }, { "mac_addr", "00:00:00:00:00:00" } }); + intf_table.set(VLAN_3000, { { "grat_arp", "enabled" }, + { "proxy_arp", "enabled" }, + { "vrf_name", VRF_3000 }, + { "mac_addr", "00:00:00:00:00:00" } }); + + intf_table.set(VLAN_4000, { { "grat_arp", "enabled" }, + { "proxy_arp", "enabled" }, + { "vrf_name", VRF_3000 }, + { "mac_addr", "00:00:00:00:00:00" } }); + intf_table.set( VLAN_1000 + neigh_table.getTableNameSeparator() + "192.168.0.1/24", { { "scope", "global" }, @@ -95,60 +122,56 @@ namespace neighorch_test { "scope", "global" }, { "family", "IPv4" }, }); - tunnel_table.set(MUX_TUNNEL, { { "dscp_mode", "uniform" }, - { "dst_ip", "2.2.2.2" }, - { "ecn_mode", "copy_from_outer" }, - { "encap_ecn_mode", "standard" }, - { "ttl_mode", "pipe" }, - { "tunnel_type", "IPINIP" } }); - - peer_switch_table.set(PEER_SWITCH_HOSTNAME, { { "address_ipv4", PEER_IPV4_ADDRESS } }); - - mux_cable_table.set(ACTIVE_INTERFACE, { { "server_ipv4", SERVER_IP1 + "/32" }, - { "server_ipv6", "a::a/128" }, - { "state", "auto" } }); + intf_table.set( + VLAN_3000 + neigh_table.getTableNameSeparator() + "192.168.3.1/24", { + { "scope", "global" }, + { "family", "IPv4" }, + }); - mux_cable_table.set(STANDBY_INTERFACE, { { "server_ipv4", SERVER_IP2+ "/32" }, - { "server_ipv6", "a::b/128" }, - { "state", "auto" } }); + intf_table.set( + VLAN_4000 + neigh_table.getTableNameSeparator() + "192.168.3.1/24", { + { "scope", "global" }, + { "family", "IPv4" }, + }); gPortsOrch->addExistingData(&port_table); gPortsOrch->addExistingData(&vlan_table); gPortsOrch->addExistingData(&vlan_member_table); static_cast(gPortsOrch)->doTask(); + gVrfOrch->addExistingData(&vrf_table); + static_cast(gVrfOrch)->doTask(); + gIntfsOrch->addExistingData(&intf_table); static_cast(gIntfsOrch)->doTask(); - m_TunnelDecapOrch->addExistingData(&tunnel_table); - static_cast(m_TunnelDecapOrch)->doTask(); - - m_MuxOrch->addExistingData(&peer_switch_table); - static_cast(m_MuxOrch)->doTask(); - - m_MuxOrch->addExistingData(&mux_cable_table); - static_cast(m_MuxOrch)->doTask(); - fdb_table.set( VLAN_1000 + fdb_table.getTableNameSeparator() + MAC1, { { "type", "dynamic" }, - { "port", ACTIVE_INTERFACE } }); + { "port", ETHERNET0 } }); fdb_table.set( VLAN_2000 + fdb_table.getTableNameSeparator() + MAC2, { { "type", "dynamic" }, - { "port", STANDBY_INTERFACE} }); + { "port", ETHERNET4 } }); fdb_table.set( VLAN_1000 + fdb_table.getTableNameSeparator() + MAC3, { { "type", "dynamic" }, - { "port", ACTIVE_INTERFACE} }); + { "port", ETHERNET0 } }); + + fdb_table.set( + VLAN_3000 + fdb_table.getTableNameSeparator() + MAC4, + { { "type", "dynamic" }, + { "port", ETHERNET8 } }); + + fdb_table.set( + VLAN_4000 + fdb_table.getTableNameSeparator() + MAC5, + { { "type", "dynamic" }, + { "port", ETHERNET12 } }); gFdbOrch->addExistingData(&fdb_table); static_cast(gFdbOrch)->doTask(); - - SetAndAssertMuxState(ACTIVE_INTERFACE, ACTIVE_STATE); - SetAndAssertMuxState(STANDBY_INTERFACE, STANDBY_STATE); } void PostSetUp() override @@ -163,18 +186,19 @@ namespace neighorch_test } }; - TEST_F(NeighOrchTest, MultiVlanIpLearning) + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighbor) { - EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); LearnNeighbor(VLAN_1000, TEST_IP, MAC1); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 1); EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry); + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); LearnNeighbor(VLAN_2000, TEST_IP, MAC2); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 0); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN2000_NEIGH), 1); + EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry); EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); LearnNeighbor(VLAN_1000, TEST_IP, MAC3); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 1); @@ -195,4 +219,50 @@ namespace neighorch_test ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 1); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN2000_NEIGH), 0); } + + TEST_F(NeighOrchTest, MultiVlanDifferentVrfDuplicateNeighbor) + { + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); + LearnNeighbor(VLAN_1000, TEST_IP, MAC1); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 1); + + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); + EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry).Times(0); + LearnNeighbor(VLAN_3000, TEST_IP, MAC4); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 1); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN3000_NEIGH), 1); + } + + TEST_F(NeighOrchTest, MultiVlanSameVrfDuplicateNeighbor) + { + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); + LearnNeighbor(VLAN_3000, TEST_IP, MAC4); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN3000_NEIGH), 1); + + EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry); + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); + LearnNeighbor(VLAN_4000, TEST_IP, MAC5); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN3000_NEIGH), 0); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN4000_NEIGH), 1); + } + + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighborMissingExistingVlanPort) + { + LearnNeighbor(VLAN_1000, TEST_IP, MAC1); + + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry).Times(0); + EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry).Times(0); + gPortsOrch->m_portList.erase(VLAN_1000); + LearnNeighbor(VLAN_2000, TEST_IP, MAC2); + } + + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighborMissingNewVlanPort) + { + LearnNeighbor(VLAN_1000, TEST_IP, MAC1); + + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry).Times(0); + EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry).Times(0); + gPortsOrch->m_portList.erase(VLAN_2000); + LearnNeighbor(VLAN_2000, TEST_IP, MAC2); + } }