Multiple IP address support in ResolvedNodeData structure and cache (#…

…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 <bzbarsky@apple.com>

* 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 <bzbarsky@apple.com>
Co-authored-by: Restyled.io <commits@restyled.io>

examples/chip-tool/commands/discover/Commands.h

@@ -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();
 

src/controller/CHIPDeviceController.cpp

@@ -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)

src/controller/python/chip/discovery/NodeResolution.cpp

@@ -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

src/lib/dnssd/Discovery_ImplPlatform.cpp

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

src/lib/dnssd/DnssdCache.h

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

src/lib/dnssd/Resolver.h

@@ -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
     }
 
@@ -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;

src/lib/dnssd/Resolver_ImplMinimalMdns.cpp

@@ -74,8 +74,7 @@ class PacketDataReporter : public ParserDelegate
         mDelegate(delegate),
         mDiscoveryType(discoveryType), mPacketRange(packet)
     {
-        mInterfaceId           = interfaceId;
-        mNodeData.mInterfaceId = interfaceId;
+        mInterfaceId = interfaceId;
     }
 
     // ParserDelegate implementation
@@ -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)

src/lib/dnssd/platform/Dnssd.h

@@ -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 {
@@ -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;
 };
 
 /**