diff --git a/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp b/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp index 2fc4e5a526fb65..4b60bbf6e59f28 100644 --- a/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp +++ b/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp @@ -19,6 +19,7 @@ #include #include +#include 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; } diff --git a/src/lib/address_resolve/tests/TestAddressResolve_DefaultImpl.cpp b/src/lib/address_resolve/tests/TestAddressResolve_DefaultImpl.cpp index 266939fa3f5d3b..7099414bce05a1 100644 --- a/src/lib/address_resolve/tests/TestAddressResolve_DefaultImpl.cpp +++ b/src/lib/address_resolve/tests/TestAddressResolve_DefaultImpl.cpp @@ -18,10 +18,25 @@ #include #include +#include +#include +#include using namespace chip; using namespace chip::AddressResolve; +namespace pw { + +template <> +StatusWithSize ToString(const Transport::PeerAddress & addr, pw::span 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; @@ -29,13 +44,34 @@ 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(idx >> 8); + auto low = static_cast(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(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(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(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(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(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(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(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(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(i + 10 - 2)), outResult.address); } }