Skip to content

Commit

Permalink
Handle learning duplicate IPs on different VRFs (#3165) (#3179)
Browse files Browse the repository at this point in the history 
- 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 <[email protected]>
Co-authored-by: Prince Sunny <[email protected]>
  • Loading branch information
@theasianpianist @prsunny
theasianpianist and prsunny authored Jun 3, 2024
1 parent 5075e66 commit 5a998b6
Show file tree
Hide file tree
Showing 3 changed files with 149 additions and 47 deletions.
30 changes: 27 additions & 3 deletions orchagent/neighorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<VRFOrch*>()->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;
}
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions tests/mock_tests/mock_orch_test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
158 changes: 114 additions & 44 deletions tests/mock_tests/neighorch_ut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include "mock_sai_api.h"
#include "mock_orch_test.h"


EXTERN_MOCK_FNS

namespace neighorch_test
Expand All @@ -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());
}
Expand All @@ -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" },
Expand All @@ -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" },
Expand All @@ -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<Orch *>(gPortsOrch)->doTask();

gVrfOrch->addExistingData(&vrf_table);
static_cast<Orch *>(gVrfOrch)->doTask();

gIntfsOrch->addExistingData(&intf_table);
static_cast<Orch *>(gIntfsOrch)->doTask();

m_TunnelDecapOrch->addExistingData(&tunnel_table);
static_cast<Orch *>(m_TunnelDecapOrch)->doTask();

m_MuxOrch->addExistingData(&peer_switch_table);
static_cast<Orch *>(m_MuxOrch)->doTask();

m_MuxOrch->addExistingData(&mux_cable_table);
static_cast<Orch *>(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<Orch *>(gFdbOrch)->doTask();

SetAndAssertMuxState(ACTIVE_INTERFACE, ACTIVE_STATE);
SetAndAssertMuxState(STANDBY_INTERFACE, STANDBY_STATE);
}

void PostSetUp() override
Expand All @@ -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);
Expand All @@ -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);
}
}

0 comments on commit 5a998b6

Please sign in to comment.