Try to set up CASE session over all discovered ips instead of only th… (

#23969)

* Try to set up CASE session over all discovered ips instead of only the 'best' one that has been discovered

* Add a macro to configure of many resolve results to store

* Add the necessary build system changes to add some tests for AddressResolve_DefaultImpl

* Add some tests

src/BUILD.gn

@@ -47,6 +47,7 @@ if (chip_build_tests) {
       "${chip_root}/src/access/tests",
       "${chip_root}/src/crypto/tests",
       "${chip_root}/src/inet/tests",
+      "${chip_root}/src/lib/address_resolve/tests",
       "${chip_root}/src/lib/asn1/tests",
       "${chip_root}/src/lib/core/tests",
       "${chip_root}/src/messaging/tests",

src/app/OperationalSessionSetup.cpp

@@ -306,6 +306,15 @@ void OperationalSessionSetup::OnSessionEstablishmentError(CHIP_ERROR error)
     VerifyOrReturn(mState != State::Uninitialized && mState != State::NeedsAddress,
                    ChipLogError(Controller, "HandleCASEConnectionFailure was called while the device was not initialized"));
 
+    if (CHIP_ERROR_TIMEOUT == error)
+    {
+        if (CHIP_NO_ERROR == Resolver::Instance().TryNextResult(mAddressLookupHandle))
+        {
+            MoveToState(State::ResolvingAddress);
+            return;
+        }
+    }
+
     DequeueConnectionCallbacks(error);
     // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks.
 }

src/lib/address_resolve/AddressResolve.h

@@ -204,6 +204,21 @@ class Resolver
     ///     in progress)
     virtual CHIP_ERROR LookupNode(const NodeLookupRequest & request, Impl::NodeLookupHandle & handle) = 0;
 
+    /// Inform the Lookup handle that the previous node lookup was not sufficient
+    /// for the purpose of the caller (e.g establishing a session fails with the
+    /// result of the previous lookup), and that more data is needed.
+    ///
+    /// If this returns CHIP_NO_ERROR, the following is expected:
+    ///   - The listener OnNodeAddressResolved will be called with the additional data.
+    ///   - handle must NOT be destroyed while the lookup is in progress (it
+    ///     is part of an internal 'lookup list')
+    ///   - handle must NOT be reused (the lookup is done on a per-node basis
+    ///     and maintains lookup data internally while the operation is still
+    ///     in progress)
+    ///
+    /// If no additional data is available at the time of the request, it returns CHIP_ERROR_WELL_EMPTY.
+    virtual CHIP_ERROR TryNextResult(Impl::NodeLookupHandle & handle) = 0;
+
     /// Stops an active lookup request.
     ///
     /// Caller controlls weather the `fail` callback of the handle is invoked or not by using

src/lib/address_resolve/AddressResolve_DefaultImpl.cpp

@@ -30,41 +30,27 @@ void NodeLookupHandle::ResetForLookup(System::Clock::Timestamp now, const NodeLo
 {
     mRequestStartTime = now;
     mRequest          = request;
-    mBestResult       = ResolveResult();
-    mBestAddressScore = Dnssd::IPAddressSorter::IpScore::kInvalid;
+    mResults          = NodeLookupResults();
 }
 
 void NodeLookupHandle::LookupResult(const ResolveResult & result)
 {
+    auto score = Dnssd::IPAddressSorter::ScoreIpAddress(result.address.GetIPAddress(), result.address.GetInterface());
+    [[maybe_unused]] bool success = mResults.UpdateResults(result, score);
+
 #if CHIP_PROGRESS_LOGGING
     char addr_string[Transport::PeerAddress::kMaxToStringSize];
     result.address.ToString(addr_string);
-#endif
 
-    auto newScore = Dnssd::IPAddressSorter::ScoreIpAddress(result.address.GetIPAddress(), result.address.GetInterface());
-    if (to_underlying(newScore) > to_underlying(mBestAddressScore))
+    if (success)
     {
-        mBestResult       = result;
-        mBestAddressScore = newScore;
-
-        if (!mBestResult.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.
-            mBestResult.address.SetInterface(Inet::InterfaceId::Null());
-            ChipLogDetail(Discovery, "Lookup clearing interface for non LL address");
-        }
-
-#if CHIP_PROGRESS_LOGGING
-        ChipLogProgress(Discovery, "%s: new best score: %u", addr_string, to_underlying(mBestAddressScore));
+        ChipLogProgress(Discovery, "%s: new best score: %u", addr_string, to_underlying(score));
     }
     else
     {
-        ChipLogProgress(Discovery, "%s: score has not improved: %u", addr_string, to_underlying(newScore));
-#endif
+        ChipLogProgress(Discovery, "%s: score has not improved: %u", addr_string, to_underlying(score));
     }
+#endif
 }
 
 System::Clock::Timeout NodeLookupHandle::NextEventTimeout(System::Clock::Timestamp now)
@@ -98,9 +84,10 @@ NodeLookupAction NodeLookupHandle::NextAction(System::Clock::Timestamp now)
     }
 
     // Minimal time to search reached. If any IP available, ready to return it.
-    if (mBestAddressScore != Dnssd::IPAddressSorter::IpScore::kInvalid)
+    if (HasLookupResult())
     {
-        return NodeLookupAction::Success(mBestResult);
+        auto result = TakeLookupResult();
+        return NodeLookupAction::Success(result);
     }
 
     // Give up if the maximum search time has been reached
@@ -112,6 +99,63 @@ NodeLookupAction NodeLookupHandle::NextAction(System::Clock::Timestamp now)
     return NodeLookupAction::KeepSearching();
 }
 
+bool NodeLookupResults::UpdateResults(const ResolveResult & result, const Dnssd::IPAddressSorter::IpScore newScore)
+{
+    uint8_t insertAtIndex = 0;
+    for (; insertAtIndex < kNodeLookupResultsLen; insertAtIndex++)
+    {
+        if (insertAtIndex >= count)
+        {
+            // This is a new entry.
+            break;
+        }
+
+        auto & oldAddress = results[insertAtIndex].address;
+        auto oldScore     = Dnssd::IPAddressSorter::ScoreIpAddress(oldAddress.GetIPAddress(), oldAddress.GetInterface());
+        if (newScore > oldScore)
+        {
+            // This is a score update, it will replace a previous entry.
+            break;
+        }
+    }
+
+    if (insertAtIndex == kNodeLookupResultsLen)
+    {
+        return false;
+    }
+
+    // Move the following valid entries one level down.
+    for (auto i = count; i > insertAtIndex; i--)
+    {
+        if (i >= kNodeLookupResultsLen)
+        {
+            continue;
+        }
+
+        results[i] = results[i - 1];
+    }
+
+    // If the number of valid entries is less than the size of the array there is an additional entry.
+    if (count < kNodeLookupResultsLen)
+    {
+        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");
+    }
+
+    return true;
+}
+
 CHIP_ERROR Resolver::LookupNode(const NodeLookupRequest & request, Impl::NodeLookupHandle & handle)
 {
     VerifyOrReturnError(mSystemLayer != nullptr, CHIP_ERROR_INCORRECT_STATE);
@@ -123,6 +167,24 @@ CHIP_ERROR Resolver::LookupNode(const NodeLookupRequest & request, Impl::NodeLoo
     return CHIP_NO_ERROR;
 }
 
+CHIP_ERROR Resolver::TryNextResult(Impl::NodeLookupHandle & handle)
+{
+    VerifyOrReturnError(mSystemLayer != nullptr, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(!mActiveLookups.Contains(&handle), CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(handle.HasLookupResult(), CHIP_ERROR_WELL_EMPTY);
+
+    return mSystemLayer->ScheduleWork(&OnTryNextResult, static_cast<void *>(&handle));
+}
+
+void Resolver::OnTryNextResult(System::Layer * layer, void * context)
+{
+    auto handle   = static_cast<Impl::NodeLookupHandle *>(context);
+    auto listener = handle->GetListener();
+    auto peerId   = handle->GetRequest().GetPeerId();
+    auto result   = handle->TakeLookupResult();
+    listener->OnNodeAddressResolved(peerId, result);
+}
+
 CHIP_ERROR Resolver::CancelLookup(Impl::NodeLookupHandle & handle, FailureCallback cancel_method)
 {
     VerifyOrReturnError(handle.IsActive(), CHIP_ERROR_INVALID_ARGUMENT);

src/lib/address_resolve/AddressResolve_DefaultImpl.h

@@ -26,13 +26,44 @@ namespace chip {
 namespace AddressResolve {
 namespace Impl {
 
+constexpr uint8_t kNodeLookupResultsLen = CHIP_CONFIG_MDNS_RESOLVE_LOOKUP_RESULTS;
+
 enum class NodeLookupResult
 {
     kKeepSearching, // keep the current search active
     kLookupError,   // final status: error
     kLookupSuccess, // final status: success
 };
 
+struct NodeLookupResults
+{
+    ResolveResult results[kNodeLookupResultsLen] = {};
+    uint8_t count                                = 0; // number of valid ResolveResult
+    uint8_t consumed                             = 0; // number of already read ResolveResult
+
+    bool UpdateResults(const ResolveResult & result, Dnssd::IPAddressSorter::IpScore score);
+
+    bool HasValidResult() const { return count > consumed; }
+
+    ResolveResult ConsumeResult()
+    {
+        VerifyOrDie(HasValidResult());
+        return results[consumed++];
+    }
+
+#if CHIP_DETAIL_LOGGING
+    void LogDetail() const
+    {
+        for (unsigned i = 0; i < count; i++)
+        {
+            char addr_string[Transport::PeerAddress::kMaxToStringSize];
+            results[i].address.ToString(addr_string);
+            ChipLogDetail(Discovery, "\t#%d: %s", i + 1, addr_string);
+        }
+    }
+#endif // CHIP_DETAIL_LOGGING
+};
+
 /// Action to take when some resolve data
 /// has been received by an active lookup
 class NodeLookupAction
@@ -92,7 +123,7 @@ class NodeLookupAction
 /// An implementation of a node lookup handle
 ///
 /// Keeps track of time requests as well as the current
-/// "best" IP address found.
+/// "best" IPs addresses found.
 class NodeLookupHandle : public NodeLookupHandleBase
 {
 public:
@@ -116,15 +147,20 @@ class NodeLookupHandle : public NodeLookupHandleBase
     /// any active searches)
     NodeLookupAction NextAction(System::Clock::Timestamp now);
 
+    /// Does the node have any valid lookup result?
+    bool HasLookupResult() const { return mResults.HasValidResult(); }
+
+    /// Return the next valid lookup result.
+    ResolveResult TakeLookupResult() { return mResults.ConsumeResult(); }
+
     /// Return when the next timer (min or max lookup time) is required to
     /// be triggered for this lookup handle
     System::Clock::Timeout NextEventTimeout(System::Clock::Timestamp now);
 
 private:
-    System::Clock::Timestamp mRequestStartTime;
+    NodeLookupResults mResults;
     NodeLookupRequest mRequest; // active request to process
-    AddressResolve::ResolveResult mBestResult;
-    Dnssd::IPAddressSorter::IpScore mBestAddressScore;
+    System::Clock::Timestamp mRequestStartTime;
 };
 
 class Resolver : public ::chip::AddressResolve::Resolver, public Dnssd::OperationalResolveDelegate
@@ -136,6 +172,7 @@ class Resolver : public ::chip::AddressResolve::Resolver, public Dnssd::Operatio
 
     CHIP_ERROR Init(System::Layer * systemLayer) override;
     CHIP_ERROR LookupNode(const NodeLookupRequest & request, Impl::NodeLookupHandle & handle) override;
+    CHIP_ERROR TryNextResult(Impl::NodeLookupHandle & handle) override;
     CHIP_ERROR CancelLookup(Impl::NodeLookupHandle & handle, FailureCallback cancel_method) override;
     void Shutdown() override;
 
@@ -146,6 +183,7 @@ class Resolver : public ::chip::AddressResolve::Resolver, public Dnssd::Operatio
 
 private:
     static void OnResolveTimer(System::Layer * layer, void * context) { static_cast<Resolver *>(context)->HandleTimer(); }
+    static void OnTryNextResult(System::Layer * layer, void * context);
 
     /// Timer on lookup node events: min and max search times.
     void HandleTimer();

src/lib/address_resolve/BUILD.gn

@@ -15,16 +15,7 @@
 import("//build_overrides/build.gni")
 import("//build_overrides/chip.gni")
 
-declare_args() {
-  # Define as "default" to include the built in resolution strategy in
-  # CHIP (i.e. sources within this directory).
-  #
-  # Define as "custom" to only have relevant type definitions included
-  # in which case the application is expected to provide a
-  #    CHIP_ADDRESS_RESOLVE_IMPL_INCLUDE_HEADER
-  # include to use
-  chip_address_resolve_strategy = "default"
-}
+import("address_resolve.gni")
 
 config("default_address_resolve_config") {
   defines = [ "CHIP_ADDRESS_RESOLVE_IMPL_INCLUDE_HEADER=<lib/address_resolve/AddressResolve_DefaultImpl.h>" ]

src/lib/address_resolve/address_resolve.gni

@@ -0,0 +1,24 @@
+# Copyright (c) 2022 Project CHIP Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+declare_args() {
+  # Define as "default" to include the built in resolution strategy in
+  # CHIP (i.e. sources within this directory).
+  #
+  # Define as "custom" to only have relevant type definitions included
+  # in which case the application is expected to provide a
+  #    CHIP_ADDRESS_RESOLVE_IMPL_INCLUDE_HEADER
+  # include to use
+  chip_address_resolve_strategy = "default"
+}

src/lib/address_resolve/tests/BUILD.gn

@@ -0,0 +1,36 @@
+# Copyright (c) 2022 Project CHIP Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import("//build_overrides/build.gni")
+import("//build_overrides/chip.gni")
+import("//build_overrides/nlunit_test.gni")
+
+import("${chip_root}/src/lib/address_resolve/address_resolve.gni")
+
+import("${chip_root}/build/chip/chip_test_suite.gni")
+
+chip_test_suite("tests") {
+  output_name = "libAddressResolveTests"
+
+  if (chip_address_resolve_strategy == "default") {
+    test_sources = [ "TestAddressResolve_DefaultImpl.cpp" ]
+  }
+
+  public_deps = [
+    "${chip_root}/src/lib/address_resolve",
+    "${chip_root}/src/lib/support:testing",
+    "${chip_root}/src/protocols",
+    "${nlunit_test_root}:nlunit-test",
+  ]
+}