Make AddressResolver not keep duplicate IP addresses in its cache (pr…

…oject-chip#36861)

* Add FAILING unit test that address resolve does dedup of inputs

* Unit tests pass, but code is not YET complete

* Tests still pass...

* Update comments

* Restyle

* Include fix

* Restyled by clang-format

* Enforce more lookup results... this makes tests happy and also it sounds like just picking a single address by default is a bit low

* Fix casts

* Apparently adding mdns results increases RAM a lot and esp32 m5 does not have sufficient space. This is a bit concerning ...

* Fix condition that I messed up

* Update non-LL interface for the dedup logic as well

* Fix includes

* Fix typo

* moved a bit the test guard: we have 2 tests that need 3 slots and more

---------

Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Andrei Litvin <andreilitvin@google.com>

src/lib/address_resolve/AddressResolve_DefaultImpl.cpp

@@ -19,6 +19,7 @@
 
 #include <lib/address_resolve/TracingStructs.h>
 #include <tracing/macros.h>
+#include <transport/raw/PeerAddress.h>
 
 namespace chip {
 namespace AddressResolve {
@@ -128,6 +129,17 @@ NodeLookupAction NodeLookupHandle::NextAction(System::Clock::Timestamp now)
 
 bool NodeLookupResults::UpdateResults(const ResolveResult & result, const Dnssd::IPAddressSorter::IpScore newScore)
 {
+    Transport::PeerAddress addressWithAdjustedInterface = result.address;
+    if (!addressWithAdjustedInterface.GetIPAddress().IsIPv6LinkLocal())
+    {
+        // Only use the DNS-SD resolution's InterfaceID for addresses that are IPv6 LLA.
+        // For all other addresses, we should rely on the device's routing table to route messages sent.
+        // Forcing messages down an InterfaceId might fail. For example, in bridged networks like Thread,
+        // mDNS advertisements are not usually received on the same interface the peer is reachable on.
+        addressWithAdjustedInterface.SetInterface(Inet::InterfaceId::Null());
+        ChipLogDetail(Discovery, "Lookup clearing interface for non LL address");
+    }
+
     uint8_t insertAtIndex = 0;
     for (; insertAtIndex < kNodeLookupResultsLen; insertAtIndex++)
     {
@@ -138,7 +150,14 @@ bool NodeLookupResults::UpdateResults(const ResolveResult & result, const Dnssd:
         }
 
         auto & oldAddress = results[insertAtIndex].address;
-        auto oldScore     = Dnssd::IPAddressSorter::ScoreIpAddress(oldAddress.GetIPAddress(), oldAddress.GetInterface());
+
+        if (oldAddress == addressWithAdjustedInterface)
+        {
+            // this address is already in our list.
+            return false;
+        }
+
+        auto oldScore = Dnssd::IPAddressSorter::ScoreIpAddress(oldAddress.GetIPAddress(), oldAddress.GetInterface());
         if (newScore > oldScore)
         {
             // This is a score update, it will replace a previous entry.
@@ -151,6 +170,10 @@ bool NodeLookupResults::UpdateResults(const ResolveResult & result, const Dnssd:
         return false;
     }
 
+    // we are guaranteed no duplicates here:
+    // - insertAtIndex MUST be with some score that is `< newScore`, so all
+    //   addresses with a `newScore` were duplicate-checked
+
     // Move the following valid entries one level down.
     for (auto i = count; i > insertAtIndex; i--)
     {
@@ -168,17 +191,9 @@ bool NodeLookupResults::UpdateResults(const ResolveResult & result, const Dnssd:
         count++;
     }
 
-    auto & updatedResult = results[insertAtIndex];
-    updatedResult        = result;
-    if (!updatedResult.address.GetIPAddress().IsIPv6LinkLocal())
-    {
-        // Only use the DNS-SD resolution's InterfaceID for addresses that are IPv6 LLA.
-        // For all other addresses, we should rely on the device's routing table to route messages sent.
-        // Forcing messages down an InterfaceId might fail. For example, in bridged networks like Thread,
-        // mDNS advertisements are not usually received on the same interface the peer is reachable on.
-        updatedResult.address.SetInterface(Inet::InterfaceId::Null());
-        ChipLogDetail(Discovery, "Lookup clearing interface for non LL address");
-    }
+    auto & updatedResult  = results[insertAtIndex];
+    updatedResult         = result;
+    updatedResult.address = addressWithAdjustedInterface;
 
     return true;
 }

src/lib/address_resolve/tests/TestAddressResolve_DefaultImpl.cpp

@@ -18,24 +18,60 @@
 
 #include <lib/address_resolve/AddressResolve_DefaultImpl.h>
 #include <lib/core/StringBuilderAdapters.h>
+#include <lib/dnssd/IPAddressSorter.h>
+#include <lib/support/StringBuilder.h>
+#include <transport/raw/PeerAddress.h>
 
 using namespace chip;
 using namespace chip::AddressResolve;
 
+namespace pw {
+
+template <>
+StatusWithSize ToString<Transport::PeerAddress>(const Transport::PeerAddress & addr, pw::span<char> buffer)
+{
+    char buff[Transport::PeerAddress::kMaxToStringSize];
+    addr.ToString(buff);
+    return pw::string::Format(buffer, "IP<%s>", buff);
+}
+
+} // namespace pw
+
 namespace {
 
 using chip::Dnssd::IPAddressSorter::IpScore;
 using chip::Dnssd::IPAddressSorter::ScoreIpAddress;
 
 constexpr uint8_t kNumberOfAvailableSlots = CHIP_CONFIG_MDNS_RESOLVE_LOOKUP_RESULTS;
 
-Transport::PeerAddress GetAddressWithLowScore(uint16_t port = CHIP_PORT, Inet::InterfaceId interfaceId = Inet::InterfaceId::Null())
+/// Get an address that should have `kUniqueLocal` (one of the lowest) priority.
+///
+/// Since for various tests we check filling the cache with values, we allow
+/// unique address generation by varying the `idx` parameter
+///
+/// @param idx - a value to generate a unique IP address (in case we do not want dedups to happen)
+/// @param port - port in case some tests would like to vary it. Required for PeerAddress
+/// @param interfaceId - interface required for PeerAddress
+Transport::PeerAddress GetAddressWithLowScore(uint16_t idx = 4, uint16_t port = CHIP_PORT,
+                                              Inet::InterfaceId interfaceId = Inet::InterfaceId::Null())
 {
     // Unique Local - expect score "3"
     Inet::IPAddress ipAddress;
-    if (!Inet::IPAddress::FromString("fdff:aabb:ccdd:1::4", ipAddress))
+
+    auto high = static_cast<uint8_t>(idx >> 8);
+    auto low  = static_cast<uint8_t>(idx & 0xFF);
+
+    StringBuilder<64> address;
+    address.Add("fdff:aabb:ccdd:1::");
+    if (high != 0)
     {
-        ChipLogError(NotSpecified, "!!!!!!!! IP Parse failure");
+        address.AddFormat("%x:", high);
+    }
+    address.AddFormat("%x", low);
+
+    if (!Inet::IPAddress::FromString(address.c_str(), ipAddress))
+    {
+        ChipLogError(NotSpecified, "!!!!!!!! IP Parse failure for %s", address.c_str());
     }
     return Transport::PeerAddress::UDP(ipAddress, port, interfaceId);
 }
@@ -66,8 +102,60 @@ Transport::PeerAddress GetAddressWithHighScore(uint16_t port = CHIP_PORT, Inet::
     return Transport::PeerAddress::UDP(ipAddress, port, interfaceId);
 }
 
-TEST(TestAddressResolveDefaultImpl, TestLookupResult)
+#if CHIP_CONFIG_MDNS_RESOLVE_LOOKUP_RESULTS >= 3
+
+// test requires at least 3 slots: for high, medium and low
+TEST(TestAddressResolveDefaultImpl, UpdateResultsDoesNotAddDuplicatesWhenFull)
+{
+    Impl::NodeLookupResults results;
+    ASSERT_EQ(results.count, 0);
+
+    for (auto i = 0; i < kNumberOfAvailableSlots; i++)
+    {
+        ResolveResult result;
+        result.address = GetAddressWithLowScore(static_cast<uint16_t>(i + 10));
+        ASSERT_TRUE(results.UpdateResults(result, Dnssd::IPAddressSorter::IpScore::kUniqueLocal));
+    }
+    ASSERT_EQ(results.count, kNumberOfAvailableSlots);
+
+    // Adding another one should fail as there is no more room
+    ResolveResult result;
+    result.address = GetAddressWithLowScore(static_cast<uint16_t>(5));
+    ASSERT_FALSE(results.UpdateResults(result, Dnssd::IPAddressSorter::IpScore::kUniqueLocal));
+    ASSERT_EQ(results.count, kNumberOfAvailableSlots);
+
+    // however one with higher priority should work
+    result.address = GetAddressWithHighScore();
+    ASSERT_TRUE(results.UpdateResults(result, Dnssd::IPAddressSorter::IpScore::kGlobalUnicast));
+    ASSERT_EQ(results.count, kNumberOfAvailableSlots);
+
+    // however not duplicate
+    ASSERT_FALSE(results.UpdateResults(result, Dnssd::IPAddressSorter::IpScore::kGlobalUnicast));
+    ASSERT_EQ(results.count, kNumberOfAvailableSlots);
+
+    // another higher priority one
+    result.address = GetAddressWithMediumScore();
+    ASSERT_TRUE(results.UpdateResults(result, Dnssd::IPAddressSorter::IpScore::kLinkLocal));
+    ASSERT_EQ(results.count, kNumberOfAvailableSlots);
+
+    // however not duplicate
+    ASSERT_FALSE(results.UpdateResults(result, Dnssd::IPAddressSorter::IpScore::kLinkLocal));
+    ASSERT_EQ(results.count, kNumberOfAvailableSlots);
+}
+
+// test requires at least 3 slots: for high, medium and low
+TEST(TestAddressResolveDefaultImpl, UpdateResultsDoesNotAddDuplicates)
 {
+    static_assert(Impl::kNodeLookupResultsLen >= 3, "Test uses 3 address slots");
+
+    Impl::NodeLookupResults results;
+    ASSERT_EQ(results.count, 0);
+
+    // The order below is VERY explicit to test both before and after inserts
+    //   - low first
+    //   - high (to be before low)
+    //   - medium (to be after high, even though before low)
+
     ResolveResult lowResult;
     lowResult.address = GetAddressWithLowScore();
 
@@ -77,6 +165,49 @@ TEST(TestAddressResolveDefaultImpl, TestLookupResult)
     ResolveResult highResult;
     highResult.address = GetAddressWithHighScore();
 
+    results.UpdateResults(lowResult, Dnssd::IPAddressSorter::IpScore::kUniqueLocal);
+    ASSERT_EQ(results.count, 1);
+
+    // same address again. we should not actually insert it!
+    results.UpdateResults(lowResult, Dnssd::IPAddressSorter::IpScore::kUniqueLocal);
+    ASSERT_EQ(results.count, 1);
+
+    // we CAN insert a different one
+    results.UpdateResults(highResult, Dnssd::IPAddressSorter::IpScore::kGlobalUnicast);
+    ASSERT_EQ(results.count, 2);
+
+    // extra insertions of the same address should NOT make a difference
+    results.UpdateResults(lowResult, Dnssd::IPAddressSorter::IpScore::kUniqueLocal);
+    ASSERT_EQ(results.count, 2);
+    results.UpdateResults(highResult, Dnssd::IPAddressSorter::IpScore::kGlobalUnicast);
+    ASSERT_EQ(results.count, 2);
+
+    // we CAN insert a different one
+    results.UpdateResults(mediumResult, Dnssd::IPAddressSorter::IpScore::kLinkLocal);
+    ASSERT_EQ(results.count, 3);
+
+    // re-insertin any of these should not make a difference
+    results.UpdateResults(lowResult, Dnssd::IPAddressSorter::IpScore::kUniqueLocal);
+    ASSERT_EQ(results.count, 3);
+    results.UpdateResults(highResult, Dnssd::IPAddressSorter::IpScore::kGlobalUnicast);
+    ASSERT_EQ(results.count, 3);
+    results.UpdateResults(mediumResult, Dnssd::IPAddressSorter::IpScore::kLinkLocal);
+    ASSERT_EQ(results.count, 3);
+}
+
+#endif
+
+TEST(TestAddressResolveDefaultImpl, TestLookupResult)
+{
+    ResolveResult lowResult;
+    lowResult.address = GetAddressWithLowScore(static_cast<uint16_t>(1));
+
+    ResolveResult mediumResult;
+    mediumResult.address = GetAddressWithMediumScore();
+
+    ResolveResult highResult;
+    highResult.address = GetAddressWithHighScore();
+
     // Ensure test expectations regarding ordering is matched
 
     IpScore lowScore    = ScoreIpAddress(lowResult.address.GetIPAddress(), Inet::InterfaceId::Null());
@@ -115,6 +246,8 @@ TEST(TestAddressResolveDefaultImpl, TestLookupResult)
     // Fill all the possible slots.
     for (auto i = 0; i < kNumberOfAvailableSlots; i++)
     {
+        // Set up UNIQUE addresses to not apply dedup here
+        lowResult.address = GetAddressWithLowScore(static_cast<uint16_t>(i + 10));
         handle.LookupResult(lowResult);
     }
 
@@ -123,7 +256,7 @@ TEST(TestAddressResolveDefaultImpl, TestLookupResult)
     {
         EXPECT_TRUE(handle.HasLookupResult());
         outResult = handle.TakeLookupResult();
-        EXPECT_EQ(lowResult.address, outResult.address);
+        EXPECT_EQ(GetAddressWithLowScore(static_cast<uint16_t>(i + 10)), outResult.address);
     }
 
     // Check that the results has been consumed properly.
@@ -134,6 +267,7 @@ TEST(TestAddressResolveDefaultImpl, TestLookupResult)
     // Fill all the possible slots by giving it 2 times more results than the available slots.
     for (auto i = 0; i < kNumberOfAvailableSlots * 2; i++)
     {
+        lowResult.address = GetAddressWithLowScore(static_cast<uint16_t>(i + 1000));
         handle.LookupResult(lowResult);
     }
 
@@ -142,7 +276,7 @@ TEST(TestAddressResolveDefaultImpl, TestLookupResult)
     {
         EXPECT_TRUE(handle.HasLookupResult());
         outResult = handle.TakeLookupResult();
-        EXPECT_EQ(lowResult.address, outResult.address);
+        EXPECT_EQ(GetAddressWithLowScore(static_cast<uint16_t>(i + 1000)), outResult.address);
     }
 
     // Check that the results has been consumed properly.
@@ -167,6 +301,7 @@ TEST(TestAddressResolveDefaultImpl, TestLookupResult)
     // Fill all the possible slots.
     for (auto i = 0; i < kNumberOfAvailableSlots; i++)
     {
+        lowResult.address = GetAddressWithLowScore(static_cast<uint16_t>(i + 10));
         handle.LookupResult(lowResult);
     }
 
@@ -192,7 +327,8 @@ TEST(TestAddressResolveDefaultImpl, TestLookupResult)
         {
             EXPECT_TRUE(handle.HasLookupResult());
             outResult = handle.TakeLookupResult();
-            EXPECT_EQ(lowResult.address, outResult.address);
+            // - 2 because we start from 2 at the top for the high and medium slots
+            EXPECT_EQ(GetAddressWithLowScore(static_cast<uint16_t>(i + 10 - 2)), outResult.address);
         }
     }