From 54a499bf4669f24782abe7f58da6553820df9d3e Mon Sep 17 00:00:00 2001
From: Sai Rama Mohan Reddy S <63886014+ram25794@users.noreply.github.com>
Date: Fri, 10 Jan 2025 13:23:07 -0800
Subject: [PATCH] Fix VRF update handling for loopback interfaces in IntfsOrch
 (#3461)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

What I did

Addressed the scenario where an interface exists in m_syncdIntfses, but the incoming VRF does not match the updated VRF for the loopback interface. The solution involves updating the loopback’s VRF ID, decrementing the reference count for the old VRF, and incrementing the reference count for the new VRF

Why I did it

Sometimes DEL is missing in the orchagent as mentioned in the issue (#3339), leading to a scenario where, when a loopback interface is added to a non-default VRF, an ip2me route is incorrectly added to the default VRF. This issue occurs because the IntfsOrch is not handling the case where the interface exists in m_syncdIntfses, but the VRF does not match the updated VRF configuration.
---
 orchagent/intfsorch.cpp           | 13 +++++++
 tests/mock_tests/intfsorch_ut.cpp | 58 +++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp
index 87c2206e90..d75375bfc1 100644
--- a/orchagent/intfsorch.cpp
+++ b/orchagent/intfsorch.cpp
@@ -846,6 +846,19 @@ void IntfsOrch::doTask(Consumer &consumer)
                         m_syncdIntfses[alias] = intfs_entry;
                         m_vrfOrch->increaseVrfRefCount(vrf_id);
                     }
+                    else if (m_syncdIntfses[alias].vrf_id != vrf_id)
+                    {
+                        if (m_syncdIntfses[alias].ip_addresses.size() == 0)
+                        {
+                            m_vrfOrch->decreaseVrfRefCount(m_syncdIntfses[alias].vrf_id);
+                            m_vrfOrch->increaseVrfRefCount(vrf_id);
+                            m_syncdIntfses[alias].vrf_id = vrf_id;
+                        }
+                        else
+                        {
+                            SWSS_LOG_ERROR("Failed to set interface '%s' to VRF ID '%d' because it has IP addresses associated with it.", alias.c_str(), vrf_id);
+                        }
+                    }
                 }
                 else
                 {
diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp
index c2d65362ae..b7c2bc7397 100644
--- a/tests/mock_tests/intfsorch_ut.cpp
+++ b/tests/mock_tests/intfsorch_ut.cpp
@@ -330,5 +330,63 @@ namespace intfsorch_test
         static_cast<Orch *>(gIntfsOrch)->doTask();
         ASSERT_EQ(current_create_count + 1, create_rif_count);
         ASSERT_EQ(current_remove_count + 1, remove_rif_count);
+    };
+
+    TEST_F(IntfsOrchTest, IntfsOrchVrfUpdate)
+    {
+        //create a new vrf
+        std::deque<KeyOpFieldsValuesTuple> entries;
+        entries.push_back({"Vrf-Blue", "SET", { {"NULL", "NULL"}}});
+        auto consumer = dynamic_cast<Consumer *>(gVrfOrch->getExecutor(APP_VRF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gVrfOrch)->doTask(); 
+        ASSERT_TRUE(gVrfOrch->isVRFexists("Vrf-Blue"));
+        auto new_vrf_reference_count = gVrfOrch->getVrfRefCount("Vrf-Blue");
+        ASSERT_EQ(new_vrf_reference_count, 0);
+
+        // create an interface
+        entries.clear();
+        entries.push_back({"Loopback2", "SET", {}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        IntfsTable m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
+        ASSERT_EQ(m_syncdIntfses["Loopback2"].vrf_id, gVirtualRouterId);
+
+        // change vrf and check if it worked
+        entries.clear();
+        entries.push_back({"Loopback2", "SET", { {"vrf_name", "Vrf-Blue"}}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        auto new_vrf_updated_reference_count = gVrfOrch->getVrfRefCount("Vrf-Blue");
+        ASSERT_EQ(new_vrf_reference_count + 1, new_vrf_updated_reference_count);
+        m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
+        ASSERT_EQ(m_syncdIntfses["Loopback2"].vrf_id, gVrfOrch->getVRFid("Vrf-Blue"));
+
+        // create an interface
+        entries.clear();
+        entries.push_back({"Loopback3", "SET", {}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
+        ASSERT_EQ(m_syncdIntfses["Loopback3"].vrf_id, gVirtualRouterId);
+
+        // Add IP address to the interface
+        entries.clear();
+        entries.push_back({"Loopback3:3.3.3.3/32", "SET", {{"scope", "global"},{"family", "IPv4"}}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+
+        // change vrf and check it doesn't affect the interface due to existing IP
+        entries.clear();
+        entries.push_back({"Loopback3", "SET", { {"vrf_name", "Vrf-Blue"}}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
+        ASSERT_EQ(m_syncdIntfses["Loopback3"].vrf_id, gVirtualRouterId);    
     }
 }
\ No newline at end of file