More predictive order for what IP address is chosen for device connec…

…tions (use link local as last resort) (#14760)

* Mark logging of resolved node id as const just in case we want to log in other places

* Prioritize IP addresses to use for communication

* Update to manual sorting (non-optimized) and fixed signed/unsigned type for ip address counts

* Fix wrong comment

src/controller/python/ChipCommissionableNodeController-ScriptBinding.cpp

@@ -115,7 +115,7 @@ void pychip_CommissionableNodeController_PrintDiscoveredCommissioners(
             ChipLogProgress(Discovery, "\tMrp Interval active\tNot present");
         }
         ChipLogProgress(Discovery, "\tSupports TCP\t\t%d", dnsSdInfo->supportsTcp);
-        for (int j = 0; j < dnsSdInfo->numIPs; ++j)
+        for (unsigned j = 0; j < dnsSdInfo->numIPs; ++j)
         {
             char buf[chip::Inet::IPAddress::kMaxStringLength];
             dnsSdInfo->ipAddress[j].ToString(buf);

src/controller/python/ChipDeviceController-ScriptBinding.cpp

@@ -483,7 +483,7 @@ void pychip_DeviceController_PrintDiscoveredDevices(chip::Controller::DeviceComm
             ChipLogProgress(Discovery, "\tMrp Interval active\tNot present");
         }
         ChipLogProgress(Discovery, "\tSupports TCP\t\t%d", dnsSdInfo->supportsTcp);
-        for (int j = 0; j < dnsSdInfo->numIPs; ++j)
+        for (unsigned j = 0; j < dnsSdInfo->numIPs; ++j)
         {
             char buf[chip::Inet::IPAddress::kMaxStringLength];
             dnsSdInfo->ipAddress[j].ToString(buf);

src/lib/dnssd/Discovery_ImplPlatform.cpp

@@ -119,6 +119,7 @@ static void HandleNodeIdResolve(void * context, DnssdService * result, CHIP_ERRO
     }
 
     nodeData.LogNodeIdResolved();
+    nodeData.PrioritizeAddresses();
 #if CHIP_CONFIG_MDNS_CACHE_SIZE > 0
     LogErrorOnFailure(sDnssdCache.Insert(nodeData));
 #endif

src/lib/dnssd/Resolver.h

@@ -19,6 +19,7 @@
 
 #include <cstdint>
 #include <limits>
+#include <utility>
 
 #include "lib/support/logging/CHIPLogging.h"
 #include <inet/IPAddress.h>
@@ -37,23 +38,69 @@ namespace Dnssd {
 struct ResolvedNodeData
 {
     // TODO: use pool to allow dynamic
-    static constexpr int kMaxIPAddresses = 5;
-    void LogNodeIdResolved()
+    static constexpr unsigned kMaxIPAddresses = 5;
+
+    static bool IsIpLess(const Inet::IPAddress & a, const Inet::IPAddress & b)
+    {
+        // Link-local last
+        if (a.IsIPv6LinkLocal() && !b.IsIPv6LinkLocal())
+        {
+            return false;
+        }
+        if (!a.IsIPv6LinkLocal() && b.IsIPv6LinkLocal())
+        {
+            return true;
+        }
+
+        // IPv6 before IPv4
+        if (a.IsIPv6() && !b.IsIPv6())
+        {
+            return false;
+        }
+        if (!a.IsIPv6() && b.IsIPv6())
+        {
+            return true;
+        }
+
+        // no ordering, do not care
+        return false;
+    }
+
+    void LogNodeIdResolved() const
     {
 #if CHIP_PROGRESS_LOGGING
         char addrBuffer[Inet::IPAddress::kMaxStringLength];
 
         // 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, ChipLogValueX64(mPeerId.GetNodeId()));
-        for (size_t i = 0; i < mNumIPs; ++i)
+        for (unsigned i = 0; i < mNumIPs; ++i)
         {
             mAddress[i].ToString(addrBuffer);
-            ChipLogProgress(Discovery, "    Addr %zu: [%s]:%" PRIu16, i, addrBuffer, mPort);
+            ChipLogProgress(Discovery, "    Addr %u: [%s]:%" PRIu16, i, addrBuffer, mPort);
         }
 #endif // CHIP_PROGRESS_LOGGING
     }
 
+    /// Sorts IP addresses in a consistent order. Specifically places
+    /// Link-local IPv6 addresses at the end (e.g. mDNS reflector services in Unify will
+    /// return link-local addresses that will not work) and prioritizes global IPv6 addresses
+    /// before IPv4 ones.
+    void PrioritizeAddresses()
+    {
+        // Slow sort, however we have maximum kMaxIPAddreses, so this is good enough for now
+        for (unsigned i = 0; i + 1 < mNumIPs; i++)
+        {
+            for (unsigned j = i + 1; i < mNumIPs; i++)
+            {
+                if (IsIpLess(mAddress[j], mAddress[i]))
+                {
+                    std::swap(mAddress[i], mAddress[j]);
+                }
+            }
+        }
+    }
+
     ReliableMessageProtocolConfig GetMRPConfig() const
     {
         const ReliableMessageProtocolConfig defaultConfig = GetLocalMRPConfig();
@@ -95,7 +142,7 @@ constexpr size_t kMaxPairingInstructionLen = 128;
 struct DiscoveredNodeData
 {
     // TODO(cecille): is 4 OK? IPv6 LL, GUA, ULA, IPv4?
-    static constexpr int kMaxIPAddresses = 5;
+    static constexpr unsigned kMaxIPAddresses = 5;
     char hostName[kHostNameMaxLength + 1];
     char instanceName[Commission::kInstanceNameMaxLength + 1];
     uint16_t longDiscriminator;
@@ -113,7 +160,7 @@ struct DiscoveredNodeData
     Optional<System::Clock::Milliseconds32> mrpRetryIntervalIdle;
     Optional<System::Clock::Milliseconds32> mrpRetryIntervalActive;
     uint16_t port;
-    int numIPs;
+    unsigned numIPs;
     Inet::InterfaceId interfaceId[kMaxIPAddresses];
     Inet::IPAddress ipAddress[kMaxIPAddresses];
 
@@ -136,7 +183,7 @@ struct DiscoveredNodeData
         mrpRetryIntervalActive = NullOptional;
         numIPs                 = 0;
         port                   = 0;
-        for (int i = 0; i < kMaxIPAddresses; ++i)
+        for (unsigned i = 0; i < kMaxIPAddresses; ++i)
         {
             ipAddress[i] = chip::Inet::IPAddress::Any;
         }
@@ -206,7 +253,7 @@ struct DiscoveredNodeData
         {
             ChipLogDetail(Discovery, "\tInstance Name: %s", instanceName);
         }
-        for (int j = 0; j < numIPs; j++)
+        for (unsigned j = 0; j < numIPs; j++)
         {
 #if CHIP_DETAIL_LOGGING
             char buf[Inet::IPAddress::kMaxStringLength];

src/lib/dnssd/Resolver_ImplMinimalMdns.cpp

@@ -331,6 +331,7 @@ void PacketDataReporter::OnComplete(ActiveResolveAttempts & activeAttempts)
         activeAttempts.Complete(mNodeData.mPeerId);
 
         mNodeData.LogNodeIdResolved();
+        mNodeData.PrioritizeAddresses();
         mDelegate->OnNodeIdResolved(mNodeData);
     }
 }

src/protocols/user_directed_commissioning/UserDirectedCommissioningServer.cpp

@@ -129,7 +129,7 @@ void UserDirectedCommissioningServer::OnCommissionableNodeFound(const Dnssd::Dis
 
         // currently IPv6 addresses do not work for some reason
         bool foundV4 = false;
-        for (int i = 0; i < nodeData.numIPs; ++i)
+        for (unsigned i = 0; i < nodeData.numIPs; ++i)
         {
             if (nodeData.ipAddress[i].IsIPv4())
             {