Skip to content

Commit

Permalink
Multiple IP address support in ResolvedNodeData structure and cache (#…
Browse files Browse the repository at this point in the history 
…11149)

* Add multiple addresses to ResolvedNodeData

* Use ResolvedNodeData directly for cache.

Added expiry time to ResolvedNodeId, which the resolvers should
set as appropriate from the TTL.

Using ResolvedNodeData for the DNS-SD cache directly means we
don't need to copy everything over, which is becoming more
cumbersome as we add support for more addresses. Also change
test to use test time structure.

* Update src/lib/dnssd/DnssdCache.h

Co-authored-by: Boris Zbarsky <[email protected]>

* Address review comments.

* Reduce mdns cache size for platforms.

Maintaining size for darwin, linux and android, other have the
cache size called out explicitly in the platform config so the
platform maintainers can set this as appropriate.

* Restyled by whitespace

Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
  • Loading branch information
@cecille @bzbarsky-apple
@restyled-commits @pull
3 people authored and pull[bot] committed Jun 5, 2023
1 parent a21320b commit a81e179
Show file tree
Hide file tree
Showing 24 changed files with 175 additions and 130 deletions.
10 changes: 7 additions & 3 deletions examples/chip-tool/commands/discover/Commands.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,14 @@ class Resolve : public DiscoverCommand, public chip::Dnssd::ResolverDelegate
void OnNodeIdResolved(const chip::Dnssd::ResolvedNodeData & nodeData) override
{
char addrBuffer[chip::Transport::PeerAddress::kMaxToStringSize];
nodeData.mAddress.ToString(addrBuffer);
ChipLogProgress(chipTool, "NodeId Resolution: %" PRIu64 " Address: %s, Port: %" PRIu16, nodeData.mPeerId.GetNodeId(),
addrBuffer, nodeData.mPort);

ChipLogProgress(chipTool, "NodeId Resolution: %" PRIu64 " Port: %" PRIu16, nodeData.mPeerId.GetNodeId(), nodeData.mPort);
ChipLogProgress(chipTool, " Hostname: %s", nodeData.mHostName);
for (size_t i = 0; i < nodeData.mNumIPs; ++i)
{
nodeData.mAddress[i].ToString(addrBuffer);
ChipLogProgress(chipTool, " addr %zu: %s", i, addrBuffer);
}

auto retryInterval = nodeData.GetMrpRetryIntervalIdle();

Expand Down
6 changes: 4 additions & 2 deletions src/controller/CHIPDeviceController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -580,12 +580,14 @@ Transport::PeerAddress DeviceController::ToPeerAddress(const chip::Dnssd::Resolv
// 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.
if (nodeData.mAddress.IsIPv6LinkLocal())
// TODO: Right now, just use addr0, but we should really push all the addresses and interfaces to
// the device and allow it to make a proper decision about which addresses are preferred and reachable.
if (nodeData.mAddress[0].IsIPv6LinkLocal())
{
interfaceId = nodeData.mInterfaceId;
}

return Transport::PeerAddress::UDP(nodeData.mAddress, nodeData.mPort, interfaceId);
return Transport::PeerAddress::UDP(nodeData.mAddress[0], nodeData.mPort, interfaceId);
}

void DeviceController::OnNodeIdResolved(const chip::Dnssd::ResolvedNodeData & nodeData)
Expand Down
14 changes: 8 additions & 6 deletions src/controller/python/chip/discovery/NodeResolution.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,14 @@ class PythonResolverDelegate : public ResolverDelegate
{
char ipAddressBuffer[128];

mSuccessCallback( //
nodeData.mPeerId.GetCompressedFabricId(), //
nodeData.mPeerId.GetNodeId(), //
nodeData.mInterfaceId.GetPlatformInterface(), //
nodeData.mAddress.ToString(ipAddressBuffer, sizeof(ipAddressBuffer)), //
nodeData.mPort //
// TODO: For now, just provide addr 0, but this should really provide all and
// allow the caller to choose.
mSuccessCallback( //
nodeData.mPeerId.GetCompressedFabricId(), //
nodeData.mPeerId.GetNodeId(), //
nodeData.mInterfaceId.GetPlatformInterface(), //
nodeData.mAddress[0].ToString(ipAddressBuffer, sizeof(ipAddressBuffer)), //
nodeData.mPort //
);
}
else
Expand Down
44 changes: 17 additions & 27 deletions src/lib/dnssd/Discovery_ImplPlatform.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,23 +459,11 @@ CHIP_ERROR DiscoveryImplPlatform::ResolveNodeId(const PeerId & peerId, Inet::IPA
ReturnErrorOnFailure(InitImpl());

#if CHIP_CONFIG_MDNS_CACHE_SIZE > 0
Inet::IPAddress addr;
uint16_t port;
Inet::InterfaceId iface;

/* see if the entry is cached and use it.... */

if (sDnssdCache.Lookup(peerId, addr, port, iface) == CHIP_NO_ERROR)
ResolvedNodeData nodeData;
if (sDnssdCache.Lookup(peerId, nodeData) == CHIP_NO_ERROR)
{
ResolvedNodeData nodeData;

nodeData.mInterfaceId = iface;
nodeData.mPort = port;
nodeData.mAddress = addr;
nodeData.mPeerId = peerId;

mResolverDelegate->OnNodeIdResolved(nodeData);

return CHIP_NO_ERROR;
}
#endif
Expand Down Expand Up @@ -587,22 +575,16 @@ void DiscoveryImplPlatform::HandleNodeIdResolve(void * context, DnssdService * r
return;
}

#if CHIP_CONFIG_MDNS_CACHE_SIZE > 0
// TODO -- define appropriate TTL, for now use 2000 msec (rfc default)
// figure out way to use TTL value from mDNS packet in future update
error = mgr->sDnssdCache.Insert(nodeData.mPeerId, result->mAddress.Value(), result->mPort, result->mInterface,
System::Clock::Seconds16(2));

if (CHIP_NO_ERROR != error)
{
ChipLogError(Discovery, "DnssdCache insert failed with %s", chip::ErrorStr(error));
}
#endif

// TODO: Expand the results to include all the addresses.
Platform::CopyString(nodeData.mHostName, result->mHostName);
nodeData.mInterfaceId = result->mInterface;
nodeData.mAddress = result->mAddress.ValueOr({});
nodeData.mAddress[0] = result->mAddress.ValueOr({});
nodeData.mPort = result->mPort;
nodeData.mNumIPs = 1;
// TODO: Use seconds?
const System::Clock::Timestamp currentTime = System::SystemClock().GetMonotonicTimestamp();

nodeData.mExpiryTime = currentTime + System::Clock::Seconds16(result->mTtlSeconds);

for (size_t i = 0; i < result->mTextEntrySize; ++i)
{
Expand All @@ -612,6 +594,14 @@ void DiscoveryImplPlatform::HandleNodeIdResolve(void * context, DnssdService * r
}

nodeData.LogNodeIdResolved();
#if CHIP_CONFIG_MDNS_CACHE_SIZE > 0
error = mgr->sDnssdCache.Insert(nodeData);

if (CHIP_NO_ERROR != error)
{
ChipLogError(Discovery, "DnssdCache insert failed with %s", chip::ErrorStr(error));
}
#endif
mgr->mResolverDelegate->OnNodeIdResolved(nodeData);
}

Expand Down
89 changes: 36 additions & 53 deletions src/lib/dnssd/DnssdCache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
#include <inet/InetLayer.h>
#include <lib/core/CHIPError.h>
#include <lib/core/PeerId.h>
#include <system/SystemTimer.h>
#include <lib/dnssd/Resolver.h>
#include <system/TimeSource.h>

// set MDNS_LOGGING to enable logging -- sometimes used in debug/test programs -- traces the behavior
Expand All @@ -43,7 +43,7 @@ class DnssdCache
public:
DnssdCache() : elementsUsed(CACHE_SIZE)
{
for (DnssdCacheEntry & e : mLookupTable)
for (ResolvedNodeData & e : mLookupTable)
{
// each unused entry decrements the count
MarkEntryUnused(e);
Expand All @@ -55,40 +55,32 @@ class DnssdCache
// return error if cache is full
// TODO: have an eviction policy so if the cache is full, an entry may be deleted.
// One policy may be Least-time-to-live
CHIP_ERROR Insert(PeerId peerId, const Inet::IPAddress & addr, uint16_t port, Inet::InterfaceId iface,
System::Clock::Timestamp TTL)
CHIP_ERROR Insert(const ResolvedNodeData & nodeData)
{
const System::Clock::Timestamp currentTime = mTimeSource.GetMonotonicTimestamp();
const System::Clock::Timestamp currentTime = System::SystemClock().GetMonotonicTimestamp();

DnssdCacheEntry * entry;
ResolvedNodeData * entry;

entry = FindPeerId(peerId, currentTime);
entry = FindPeerId(nodeData.mPeerId, currentTime);
if (entry)
{
// update timeout if found entry
entry->expiryTime = currentTime + TTL;
entry->TTL = TTL; // in case it changes */
*entry = nodeData;
return CHIP_NO_ERROR;
}

VerifyOrReturnError(entry = findSlot(currentTime), CHIP_ERROR_TOO_MANY_KEYS);

// have a free slot for this entry
entry->peerId = peerId;
entry->ipAddr = addr;
entry->port = port;
entry->ifaceId = iface;
entry->TTL = TTL;
entry->expiryTime = currentTime + TTL;
*entry = nodeData;
elementsUsed++;

return CHIP_NO_ERROR;
}

CHIP_ERROR Delete(PeerId peerId)
{
DnssdCacheEntry * pentry;
const System::Clock::Timestamp currentTime = mTimeSource.GetMonotonicTimestamp();
ResolvedNodeData * pentry;
const System::Clock::Timestamp currentTime = System::SystemClock().GetMonotonicTimestamp();

VerifyOrReturnError(pentry = FindPeerId(peerId, currentTime), CHIP_ERROR_KEY_NOT_FOUND);

Expand All @@ -97,16 +89,14 @@ class DnssdCache
}

// given a peerId, find the parameters if its in the cache, or return error
CHIP_ERROR Lookup(PeerId peerId, Inet::IPAddress & addr, uint16_t & port, Inet::InterfaceId & iface)
CHIP_ERROR Lookup(PeerId peerId, ResolvedNodeData & nodeData)
{
DnssdCacheEntry * pentry;
const System::Clock::Timestamp currentTime = mTimeSource.GetMonotonicTimestamp();
ResolvedNodeData * pentry;
const System::Clock::Timestamp currentTime = System::SystemClock().GetMonotonicTimestamp();

VerifyOrReturnError(pentry = FindPeerId(peerId, currentTime), CHIP_ERROR_KEY_NOT_FOUND);

addr = pentry->ipAddr;
port = pentry->port;
iface = pentry->ifaceId;
nodeData = *pentry;

return CHIP_NO_ERROR;
}
Expand All @@ -117,48 +107,41 @@ class DnssdCache
int i = 0;

MdnsLogProgress(Discovery, "cache size = %d", elementsUsed);
for (DnssdCacheEntry & e : mLookupTable)
for (ResolvedNodeData & e : mLookupTable)
{
if (e.peerId == nullPeerId)
if (e.mPeerId == nullPeerId)
{
MdnsLogProgress(Discovery, "Entry %d unused", i);
}
else
{
char address[100];

e.ipAddr.ToString(address, sizeof address);
MdnsLogProgress(Discovery, "Entry %d: node %lx fabric %lx, port = %d, address = %s", i, e.peerId.GetNodeId(),
e.peerId.GetFabricId(), e.port, address);
MdnsLogProgress(Discovery, "Entry %d: node %lx fabric %lx, port = %d", i, e.mPeerId.GetNodeId(),
e.peerId.GetFabricId(), e.port);
for (size_t j = 0; j < e.mNumIPs; ++j)
{
char address[Inet::IPAddress::kMaxStringLength];
e.mAddress[i].ToString(address);
MdnsLogProgress(Discovery, " address %d: %s", j, address);
}
}
i++;
}
}

private:
struct DnssdCacheEntry
{
PeerId peerId;
Inet::IPAddress ipAddr;
uint16_t port;
Inet::InterfaceId ifaceId;
System::Clock::Timestamp TTL;
System::Clock::Timestamp expiryTime;
};
PeerId nullPeerId; // indicates a cache entry is unused
int elementsUsed; // running count of how many entries are used -- for a sanity check

DnssdCacheEntry mLookupTable[CACHE_SIZE];
Time::TimeSource<Time::Source::kSystem> mTimeSource;
ResolvedNodeData mLookupTable[CACHE_SIZE];

DnssdCacheEntry * findSlot(System::Clock::Timestamp currentTime)
ResolvedNodeData * findSlot(System::Clock::Timestamp currentTime)
{
for (DnssdCacheEntry & entry : mLookupTable)
for (ResolvedNodeData & entry : mLookupTable)
{
if (entry.peerId == nullPeerId)
if (entry.mPeerId == nullPeerId)
return &entry;

if (entry.expiryTime <= currentTime)
if (entry.mExpiryTime <= currentTime)
{
MarkEntryUnused(entry);
return &entry;
Expand All @@ -167,21 +150,21 @@ class DnssdCache
return nullptr;
}

DnssdCacheEntry * FindPeerId(PeerId peerId, System::Clock::Timestamp current_time)
ResolvedNodeData * FindPeerId(PeerId peerId, System::Clock::Timestamp current_time)
{
for (DnssdCacheEntry & entry : mLookupTable)
for (ResolvedNodeData & entry : mLookupTable)
{
if (entry.peerId == peerId)
if (entry.mPeerId == peerId)
{
if (entry.expiryTime < current_time)
if (entry.mExpiryTime < current_time)
{
MarkEntryUnused(entry);
break; // return nullptr
}
else
return &entry;
}
if (entry.peerId != nullPeerId && entry.expiryTime < current_time)
if (entry.mPeerId != nullPeerId && entry.mExpiryTime < current_time)
{
MarkEntryUnused(entry);
}
Expand All @@ -191,9 +174,9 @@ class DnssdCache
}

// have a method to mark ununused -- so its easy to change
void MarkEntryUnused(DnssdCacheEntry & pentry)
void MarkEntryUnused(ResolvedNodeData & pentry)
{
pentry.peerId = nullPeerId;
pentry.mPeerId = nullPeerId;
elementsUsed--;
}
};
Expand Down
18 changes: 13 additions & 5 deletions src/lib/dnssd/Resolver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,21 @@ constexpr uint32_t kUndefinedRetryInterval = std::numeric_limits<uint32_t>::max(

struct ResolvedNodeData
{
// TODO: use pool to allow dynamic
static constexpr int kMaxIPAddresses = 5;
void LogNodeIdResolved()
{
#if CHIP_PROGRESS_LOGGING
char addrBuffer[Inet::IPAddress::kMaxStringLength];
mAddress.ToString(addrBuffer);

// Would be nice to log the interface id, but sorting out how to do so
// across our differnet InterfaceId implementations is a pain.
ChipLogProgress(Discovery, "Node ID resolved for 0x" ChipLogFormatX64 " to [%s]:%" PRIu16,
ChipLogValueX64(mPeerId.GetNodeId()), addrBuffer, mPort);
ChipLogProgress(Discovery, "Node ID resolved for 0x" ChipLogFormatX64, ChipLogValueX64(mPeerId.GetNodeId()));
for (size_t i = 0; i < mNumIPs; ++i)
{
mAddress[i].ToString(addrBuffer);
ChipLogProgress(Discovery, " Addr %zu: [%s]:%" PRIu16, i, addrBuffer, mPort);
}
#endif // CHIP_PROGRESS_LOGGING
}

Expand All @@ -62,13 +68,15 @@ struct ResolvedNodeData
}

PeerId mPeerId;
Inet::IPAddress mAddress = Inet::IPAddress::Any;
Inet::InterfaceId mInterfaceId = Inet::InterfaceId::Null();
size_t mNumIPs = 0;
Inet::InterfaceId mInterfaceId;
Inet::IPAddress mAddress[kMaxIPAddresses];
uint16_t mPort = 0;
char mHostName[kHostNameMaxLength + 1] = {};
bool mSupportsTcp = false;
uint32_t mMrpRetryIntervalIdle = kUndefinedRetryInterval;
uint32_t mMrpRetryIntervalActive = kUndefinedRetryInterval;
System::Clock::Timestamp mExpiryTime;
};

constexpr size_t kMaxDeviceNameLen = 32;
Expand Down
8 changes: 4 additions & 4 deletions src/lib/dnssd/Resolver_ImplMinimalMdns.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@ class PacketDataReporter : public ParserDelegate
mDelegate(delegate),
mDiscoveryType(discoveryType), mPacketRange(packet)
{
mInterfaceId = interfaceId;
mNodeData.mInterfaceId = interfaceId;
mInterfaceId = interfaceId;
}

// ParserDelegate implementation
Expand Down Expand Up @@ -177,8 +176,9 @@ void PacketDataReporter::OnOperationalIPAddress(const chip::Inet::IPAddress & ad
// This code assumes that all entries in the mDNS packet relate to the
// same entity. This may not be correct if multiple servers are reported
// (if multi-admin decides to use unique ports for every ecosystem).
mNodeData.mAddress = addr;
mHasIP = true;
mNodeData.mAddress[mDiscoveredNodeData.numIPs++] = addr;
mNodeData.mInterfaceId = mInterfaceId;
mHasIP = true;
}

void PacketDataReporter::OnDiscoveredNodeIPAddress(const chip::Inet::IPAddress & addr)
Expand Down
3 changes: 3 additions & 0 deletions src/lib/dnssd/platform/Dnssd.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include <lib/core/Optional.h>
#include <lib/dnssd/Constants.h>
#include <lib/dnssd/ServiceNaming.h>
#include <system/TimeSource.h>

namespace chip {
namespace Dnssd {
Expand Down Expand Up @@ -75,6 +76,8 @@ struct DnssdService
const char ** mSubTypes;
size_t mSubTypeSize;
Optional<chip::Inet::IPAddress> mAddress;
// Time to live in seconds. Per rfc6762 section 10, because we have a hostname, our default TTL is 120 seconds
uint32_t mTtlSeconds = 120;
};

/**
Expand Down
Loading

0 comments on commit a81e179

Please sign in to comment.