From 9fd6596e7bd3b47d062a0f5d7d3b12fa059893c8 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Wed, 14 Aug 2024 12:41:18 -0700 Subject: [PATCH] [dns-client] track limited query (one question) servers (#10403) This commit updates `Dns::Client` to remember servers/resolvers that are known to have trouble with multiple-question queries. This information is learned from earlier interactions with the server. When `ResolveService()` is requested, if the user explicitly requests "optimize" service mode, the request is honored. Otherwise, if "optimize" service mode is chosen from the default configuration and the DNS server is known to have trouble with multiple-question queries, "separate" service mode is used instead. This commit also updates the `test_dns_client.cpp` unit test to validate the newly added behavior. --- src/core/net/dns_client.cpp | 65 +++++++++++++++++++++++++++++++ src/core/net/dns_client.hpp | 7 +++- src/core/net/dnssd_server.hpp | 21 ++++++++++ tests/unit/test_dns_client.cpp | 71 +++++++++++++++++++++++++++++----- 4 files changed, 154 insertions(+), 10 deletions(-) diff --git a/src/core/net/dns_client.cpp b/src/core/net/dns_client.cpp index 49a0d4d5a..1d41c87c6 100644 --- a/src/core/net/dns_client.cpp +++ b/src/core/net/dns_client.cpp @@ -791,6 +791,8 @@ void Client::Stop(void) IgnoreError(mEndpoint.Deinitialize()); } #endif + + mLimitedQueryServers.Clear(); } #if OPENTHREAD_CONFIG_DNS_CLIENT_OVER_TCP_ENABLE @@ -938,6 +940,8 @@ Error Client::Resolve(const char *aInstanceLabel, info.mConfig.SetFrom(aConfig, mDefaultConfig); info.mShouldResolveHostAddr = aShouldResolveHostAddr; + CheckAndUpdateServiceMode(info.mConfig, aConfig); + switch (info.mConfig.GetServiceMode()) { case QueryConfig::kServiceModeSrvTxtSeparate: @@ -1402,6 +1406,11 @@ Error Client::ParseResponse(const Message &aResponseMessage, Query *&aQuery, Err aResponseError = Header::ResponseCodeToError(header.GetResponseCode()); + if ((aResponseError == kErrorNone) && (info.mQueryType == kServiceQuerySrvTxt)) + { + RecordServerAsCapableOfMultiQuestions(info.mConfig.GetServerSockAddr().GetAddress()); + } + exit: return error; } @@ -1591,6 +1600,60 @@ exit: #if OPENTHREAD_CONFIG_DNS_CLIENT_SERVICE_DISCOVERY_ENABLE +void Client::CheckAndUpdateServiceMode(QueryConfig &aConfig, const QueryConfig *aRequestConfig) const +{ + // If the user explicitly requested "optimize" mode, we honor that + // request. Otherwise, if "optimize" is chosen from the default + // config, we check if the DNS server is known to have trouble + // with multiple-question queries. If so, we switch to "separate" + // mode. + + if ((aRequestConfig != nullptr) && (aRequestConfig->GetServiceMode() == QueryConfig::kServiceModeSrvTxtOptimize)) + { + ExitNow(); + } + + VerifyOrExit(aConfig.GetServiceMode() == QueryConfig::kServiceModeSrvTxtOptimize); + + if (mLimitedQueryServers.Contains(aConfig.GetServerSockAddr().GetAddress())) + { + aConfig.SetServiceMode(QueryConfig::kServiceModeSrvTxtSeparate); + } + +exit: + return; +} + +void Client::RecordServerAsLimitedToSingleQuestion(const Ip6::Address &aServerAddress) +{ + VerifyOrExit(!aServerAddress.IsUnspecified()); + + VerifyOrExit(!mLimitedQueryServers.Contains(aServerAddress)); + + if (mLimitedQueryServers.IsFull()) + { + uint8_t randomIndex = Random::NonCrypto::GetUint8InRange(0, mLimitedQueryServers.GetMaxSize()); + + mLimitedQueryServers.Remove(mLimitedQueryServers[randomIndex]); + } + + IgnoreError(mLimitedQueryServers.PushBack(aServerAddress)); + +exit: + return; +} + +void Client::RecordServerAsCapableOfMultiQuestions(const Ip6::Address &aServerAddress) +{ + Ip6::Address *entry = mLimitedQueryServers.Find(aServerAddress); + + VerifyOrExit(entry != nullptr); + mLimitedQueryServers.Remove(*entry); + +exit: + return; +} + Error Client::ReplaceWithSeparateSrvTxtQueries(Query &aQuery) { Error error = kErrorFailed; @@ -1602,6 +1665,8 @@ Error Client::ReplaceWithSeparateSrvTxtQueries(Query &aQuery) VerifyOrExit(info.mQueryType == kServiceQuerySrvTxt); VerifyOrExit(info.mConfig.GetServiceMode() == QueryConfig::kServiceModeSrvTxtOptimize); + RecordServerAsLimitedToSingleQuestion(info.mConfig.GetServerSockAddr().GetAddress()); + secondQuery = aQuery.Clone(); VerifyOrExit(secondQuery != nullptr); diff --git a/src/core/net/dns_client.hpp b/src/core/net/dns_client.hpp index 9ac6a3868..5a86058d1 100644 --- a/src/core/net/dns_client.hpp +++ b/src/core/net/dns_client.hpp @@ -770,7 +770,8 @@ public: #endif // OPENTHREAD_CONFIG_DNS_CLIENT_SERVICE_DISCOVERY_ENABLE private: - static constexpr uint16_t kMaxCnameAliasNameChanges = 40; + static constexpr uint16_t kMaxCnameAliasNameChanges = 40; + static constexpr uint8_t kLimitedQueryServersArraySize = 3; enum QueryType : uint8_t { @@ -858,6 +859,9 @@ private: void *aContext, const QueryConfig *aConfig, bool aShouldResolveHostAddr); + void CheckAndUpdateServiceMode(QueryConfig &aConfig, const QueryConfig *aRequestConfig) const; + void RecordServerAsLimitedToSingleQuestion(const Ip6::Address &aServerAddress); + void RecordServerAsCapableOfMultiQuestions(const Ip6::Address &aServerAddress); Error ReplaceWithSeparateSrvTxtQueries(Query &aQuery); void ResolveHostAddressIfNeeded(Query &aQuery, const Message &aResponseMessage); #endif @@ -925,6 +929,7 @@ private: #if OPENTHREAD_CONFIG_DNS_CLIENT_DEFAULT_SERVER_ADDRESS_AUTO_SET_ENABLE bool mUserDidSetDefaultAddress; #endif + Array mLimitedQueryServers; }; } // namespace Dns diff --git a/src/core/net/dnssd_server.hpp b/src/core/net/dnssd_server.hpp index 4174ac5ae..e20e71f4e 100644 --- a/src/core/net/dnssd_server.hpp +++ b/src/core/net/dnssd_server.hpp @@ -49,6 +49,7 @@ #include "border_router/infra_if.hpp" #include "common/as_core_type.hpp" #include "common/callback.hpp" +#include "common/equatable.hpp" #include "common/message.hpp" #include "common/non_copyable.hpp" #include "common/owned_ptr.hpp" @@ -96,6 +97,26 @@ public: */ class Counters : public otDnssdCounters, public Clearable { + public: + /** + * Returns the total number of processed queries (successful or failed responses). + * + * @return The total number of queries. + * + */ + uint32_t GetTotalQueries(void) const { return mSuccessResponse + GetTotalFailedQueries(); } + + /** + * Returns the total number of failed queries (any error response code). + * + * @return The total number of failed queries. + * + */ + uint32_t GetTotalFailedQueries(void) const + { + return mServerFailureResponse + mFormatErrorResponse + mNameErrorResponse + mNotImplementedResponse + + mOtherResponse; + } }; #if OPENTHREAD_CONFIG_DNS_UPSTREAM_QUERY_ENABLE diff --git a/tests/unit/test_dns_client.cpp b/tests/unit/test_dns_client.cpp index e91376689..b4947f585 100644 --- a/tests/unit/test_dns_client.cpp +++ b/tests/unit/test_dns_client.cpp @@ -497,15 +497,17 @@ void TestDnsClient(void) Dns::Client::QueryConfig::kServiceModeSrvTxtOptimize, }; - Array addresses; - Srp::Server *srpServer; - Srp::Client *srpClient; - Srp::Client::Service service1; - Srp::Client::Service service2; - Dns::Client *dnsClient; - Dns::Client::QueryConfig queryConfig; - Dns::ServiceDiscovery::Server *dnsServer; - uint16_t heapAllocations; + Array addresses; + Srp::Server *srpServer; + Srp::Client *srpClient; + Srp::Client::Service service1; + Srp::Client::Service service2; + Dns::Client *dnsClient; + Dns::Client::QueryConfig queryConfig; + Dns::ServiceDiscovery::Server *dnsServer; + Dns::ServiceDiscovery::Server::Counters oldServerCounters; + Dns::ServiceDiscovery::Server::Counters newServerCounters; + uint16_t heapAllocations; Log("--------------------------------------------------------------------------------------------"); Log("TestDnsClient"); @@ -882,6 +884,8 @@ void TestDnsClient(void) queryConfig.Clear(); queryConfig.mServiceMode = static_cast(Dns::Client::QueryConfig::kServiceModeSrvTxtOptimize); + oldServerCounters = dnsServer->GetCounters(); + sResolveServiceInfo.Reset(); SuccessOrQuit(dnsClient->ResolveServiceAndHostAddress(kInstance1Label, kService1FullName, ServiceCallback, sInstance, &queryConfig)); @@ -907,6 +911,55 @@ void TestDnsClient(void) VerifyOrQuit(addresses.Contains(sResolveServiceInfo.mHostAddresses[index])); } + newServerCounters = dnsServer->GetCounters(); + + Log("Validate (using server counter) that client first tried to query SRV/TXT together and failed"); + Log("and then send separate queries (for SRV, TXT and AAAA)"); + Log(" Total : %2u -> %2u", oldServerCounters.GetTotalQueries(), newServerCounters.GetTotalQueries()); + Log(" Failed: %2u -> %2u", oldServerCounters.GetTotalFailedQueries(), newServerCounters.GetTotalFailedQueries()); + + VerifyOrQuit(newServerCounters.GetTotalFailedQueries() == 1 + oldServerCounters.GetTotalFailedQueries()); + VerifyOrQuit(newServerCounters.GetTotalQueries() == 4 + oldServerCounters.GetTotalQueries()); + + Log("- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - "); + Log("Resolve service again now using `kServiceModeSrvTxtOptimize` as default config"); + Log("Client should already know that server is not capable of handling multi-question query"); + + queryConfig.Clear(); + queryConfig.mServiceMode = static_cast(Dns::Client::QueryConfig::kServiceModeSrvTxtOptimize); + + dnsClient->SetDefaultConfig(queryConfig); + + Log("ResolveService(%s,%s)", kInstance1Label, kService1FullName); + + oldServerCounters = dnsServer->GetCounters(); + + sResolveServiceInfo.Reset(); + SuccessOrQuit(dnsClient->ResolveService(kInstance1Label, kService1FullName, ServiceCallback, sInstance, nullptr)); + + AdvanceTime(100); + + VerifyOrQuit(sResolveServiceInfo.mCallbackCount == 1); + SuccessOrQuit(sResolveServiceInfo.mError); + + VerifyOrQuit(sResolveServiceInfo.mInfo.mTtl != 0); + VerifyOrQuit(sResolveServiceInfo.mInfo.mPort == service1.mPort); + VerifyOrQuit(sResolveServiceInfo.mInfo.mWeight == service1.mWeight); + VerifyOrQuit(strcmp(sResolveServiceInfo.mInfo.mHostNameBuffer, kHostFullName) == 0); + + VerifyOrQuit(sResolveServiceInfo.mInfo.mTxtDataTtl != 0); + VerifyOrQuit(sResolveServiceInfo.mInfo.mTxtDataSize != 0); + + newServerCounters = dnsServer->GetCounters(); + + Log("Client should already know that server is not capable of handling multi-question query"); + Log("Check server counters to validate that client did send separate queries for TXT and SRV"); + Log(" Total : %2u -> %2u", oldServerCounters.GetTotalQueries(), newServerCounters.GetTotalQueries()); + Log(" Failed: %2u -> %2u", oldServerCounters.GetTotalFailedQueries(), newServerCounters.GetTotalFailedQueries()); + + VerifyOrQuit(newServerCounters.GetTotalFailedQueries() == oldServerCounters.GetTotalFailedQueries()); + VerifyOrQuit(newServerCounters.GetTotalQueries() == 2 + oldServerCounters.GetTotalQueries()); + dnsServer->SetTestMode(Dns::ServiceDiscovery::Server::kTestModeDisabled); Log("- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - ");