Skip to content

Commit

Permalink
Make AddressResolver not keep duplicate IP addresses in its cache (#3…
Browse files Browse the repository at this point in the history 
…6861)

* 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>
  • Loading branch information
@andy31415 @restyled-commits @andreilitvin
3 people authored Dec 17, 2024
1 parent dca8f9b commit 2c6c421
Show file tree
Hide file tree
Showing 2 changed files with 170 additions and 19 deletions.
39 changes: 27 additions & 12 deletions src/lib/address_resolve/AddressResolve_DefaultImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <lib/address_resolve/TracingStructs.h>
#include <tracing/macros.h>
#include <transport/raw/PeerAddress.h>

namespace chip {
namespace AddressResolve {
Expand Down Expand Up @@ -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++)
{
Expand All @@ -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.
Expand All @@ -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--)
{
Expand All @@ -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;
}
Expand Down
150 changes: 143 additions & 7 deletions src/lib/address_resolve/tests/TestAddressResolve_DefaultImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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();

Expand All @@ -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());
Expand Down Expand Up @@ -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);
}

Expand All @@ -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.
Expand All @@ -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);
}

Expand All @@ -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.
Expand All @@ -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);
}

Expand All @@ -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);
}
}

Expand Down

0 comments on commit 2c6c421

Please sign in to comment.