From f2ed3e9c0d9ba4a4bcc0190102e4af2c987a05f9 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Wed, 26 Jun 2024 18:35:30 -0700 Subject: [PATCH] [dns-client] switch to separate SRV/TXT queries on response timeout (#10444) This commit updates `Dns::Client` so that when resolving a service using `kServiceModeSrvTxtOptimize`, it switches to single-question query mode and sends separate parallel SRV and TXT queries upon the first response timeout. This is in addition to the existing behavior of switching to separate queries upon receiving a response with an error rcode from the server. The `test_dns_client` unit test is also updated to validate this scenario. New `TestMode` configurations are added to server to control its behavior, allowing it to either reject multi-question queries (by sending a "FormatError" rcode) or ignore them (sending no response). --- src/core/net/dns_client.cpp | 14 ++++++++-- src/core/net/dnssd_server.cpp | 17 ++++++++---- src/core/net/dnssd_server.hpp | 7 ++--- tests/unit/test_dns_client.cpp | 48 +++++++++++++++++++++++++++++++--- 4 files changed, 73 insertions(+), 13 deletions(-) diff --git a/src/core/net/dns_client.cpp b/src/core/net/dns_client.cpp index 1b1b77ee5..49a0d4d5a 100644 --- a/src/core/net/dns_client.cpp +++ b/src/core/net/dns_client.cpp @@ -1520,7 +1520,17 @@ void Client::HandleTimer(void) break; } - IgnoreError(SendQuery(*query, info, /* aUpdateTimer */ false)); +#if OPENTHREAD_CONFIG_DNS_CLIENT_SERVICE_DISCOVERY_ENABLE + if (ReplaceWithSeparateSrvTxtQueries(*query) == kErrorNone) + { + LogInfo("Switching to separate SRV/TXT on response timeout"); + info.ReadFrom(*query); + } + else +#endif + { + IgnoreError(SendQuery(*query, info, /* aUpdateTimer */ false)); + } } nextTime.UpdateIfEarlier(info.mRetransmissionTime); @@ -1534,7 +1544,7 @@ void Client::HandleTimer(void) } } - mTimer.FireAt(nextTime); + mTimer.FireAtIfEarlier(nextTime); #if OPENTHREAD_CONFIG_DNS_CLIENT_OVER_TCP_ENABLE if (!hasTcpQuery && mTcpState != kTcpUninitialized) diff --git a/src/core/net/dnssd_server.cpp b/src/core/net/dnssd_server.cpp index 7d512efc8..35c6cb793 100644 --- a/src/core/net/dnssd_server.cpp +++ b/src/core/net/dnssd_server.cpp @@ -163,7 +163,8 @@ exit: void Server::ProcessQuery(Request &aRequest) { - ResponseCode rcode = Header::kResponseSuccess; + ResponseCode rcode = Header::kResponseSuccess; + bool shouldRespond = true; Response response(GetInstance()); #if OPENTHREAD_CONFIG_DNS_UPSTREAM_QUERY_ENABLE @@ -193,7 +194,7 @@ void Server::ProcessQuery(Request &aRequest) SuccessOrExit(rcode); #endif - SuccessOrExit(rcode = aRequest.ParseQuestions(mTestMode)); + SuccessOrExit(rcode = aRequest.ParseQuestions(mTestMode, shouldRespond)); SuccessOrExit(rcode = response.AddQuestionsFrom(aRequest)); #if OT_SHOULD_LOG_AT(OT_LOG_LEVEL_INFO) @@ -225,7 +226,10 @@ exit: response.SetResponseCode(rcode); } - response.Send(*aRequest.mMessageInfo); + if (shouldRespond) + { + response.Send(*aRequest.mMessageInfo); + } } Server::Response::Response(Instance &aInstance) @@ -296,7 +300,7 @@ exit: return; } -Server::ResponseCode Server::Request::ParseQuestions(uint8_t aTestMode) +Server::ResponseCode Server::Request::ParseQuestions(uint8_t aTestMode, bool &aShouldRespond) { // Parse header and questions from a `Request` query message and // determine the `QueryType`. @@ -306,6 +310,8 @@ Server::ResponseCode Server::Request::ParseQuestions(uint8_t aTestMode) uint16_t questionCount = mHeader.GetQuestionCount(); Question question; + aShouldRespond = true; + VerifyOrExit(mHeader.GetQueryType() == Header::kQueryTypeStandard, rcode = Header::kResponseNotImplemented); VerifyOrExit(!mHeader.IsTruncationFlagSet()); @@ -335,7 +341,8 @@ Server::ResponseCode Server::Request::ParseQuestions(uint8_t aTestMode) if (questionCount > 1) { - VerifyOrExit(!(aTestMode & kTestModeSingleQuestionOnly)); + VerifyOrExit(!(aTestMode & kTestModeRejectMultiQuestionQuery)); + VerifyOrExit(!(aTestMode & kTestModeIgnoreMultiQuestionQuery), aShouldRespond = false); VerifyOrExit(questionCount == 2); diff --git a/src/core/net/dnssd_server.hpp b/src/core/net/dnssd_server.hpp index ea8f7e7d7..f584e3b12 100644 --- a/src/core/net/dnssd_server.hpp +++ b/src/core/net/dnssd_server.hpp @@ -291,8 +291,9 @@ public: */ enum TestModeFlags : uint8_t { - kTestModeSingleQuestionOnly = 1 << 0, ///< Allow single question in query, send `FormatError` otherwise. - kTestModeEmptyAdditionalSection = 1 << 1, ///< Do not include any RR in additional section. + kTestModeRejectMultiQuestionQuery = 1 << 0, ///< Send `FormatError` for a query with multiple questions. + kTestModeIgnoreMultiQuestionQuery = 1 << 1, ///< Ignore a query with multiple questions (send no response). + kTestModeEmptyAdditionalSection = 1 << 2, ///< Do not include any RR in additional section. }; static constexpr uint8_t kTestModeDisabled = 0; ///< Test mode is disabled (no flags). @@ -346,7 +347,7 @@ private: struct Request { - ResponseCode ParseQuestions(uint8_t aTestMode); + ResponseCode ParseQuestions(uint8_t aTestMode, bool &aShouldRespond); const Message *mMessage; const Ip6::MessageInfo *mMessageInfo; diff --git a/tests/unit/test_dns_client.cpp b/tests/unit/test_dns_client.cpp index 176654c0f..e91376689 100644 --- a/tests/unit/test_dns_client.cpp +++ b/tests/unit/test_dns_client.cpp @@ -671,8 +671,8 @@ void TestDnsClient(void) Log("- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - "); - Log("Set TestMode on server to only accept single question"); - dnsServer->SetTestMode(Dns::ServiceDiscovery::Server::kTestModeSingleQuestionOnly); + Log("Set TestMode on server to reject multi-question queries and send error"); + dnsServer->SetTestMode(Dns::ServiceDiscovery::Server::kTestModeRejectMultiQuestionQuery); Log("ResolveService(%s,%s) with ServiceMode %s", kInstance1Label, kService1FullName, ServiceModeToString(Dns::Client::QueryConfig::kServiceModeSrvTxtOptimize)); @@ -706,6 +706,48 @@ void TestDnsClient(void) dnsServer->SetTestMode(Dns::ServiceDiscovery::Server::kTestModeDisabled); + Log("- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - "); + + Log("Set TestMode on server to ignore multi-question queries (send no response)"); + dnsServer->SetTestMode(Dns::ServiceDiscovery::Server::kTestModeIgnoreMultiQuestionQuery); + + Log("ResolveService(%s,%s) with ServiceMode %s", kInstance1Label, kService1FullName, + ServiceModeToString(Dns::Client::QueryConfig::kServiceModeSrvTxtOptimize)); + + queryConfig.Clear(); + queryConfig.mServiceMode = static_cast(Dns::Client::QueryConfig::kServiceModeSrvTxtOptimize); + + sResolveServiceInfo.Reset(); + SuccessOrQuit( + dnsClient->ResolveService(kInstance1Label, kService1FullName, ServiceCallback, sInstance, &queryConfig)); + + AdvanceTime(10 * 1000); // Wait longer than client response timeout. + + VerifyOrQuit(sResolveServiceInfo.mCallbackCount == 1); + SuccessOrQuit(sResolveServiceInfo.mError); + + // Use `kServiceModeSrvTxt` and check that server does ignore two questions. + + Log("ResolveService(%s,%s) with ServiceMode %s", kInstance1Label, kService1FullName, + ServiceModeToString(Dns::Client::QueryConfig::kServiceModeSrvTxt)); + + queryConfig.Clear(); + queryConfig.mServiceMode = static_cast(Dns::Client::QueryConfig::kServiceModeSrvTxt); + + sResolveServiceInfo.Reset(); + SuccessOrQuit( + dnsClient->ResolveService(kInstance1Label, kService1FullName, ServiceCallback, sInstance, &queryConfig)); + + // Wait for the client to time out after exhausting all retry attempts, and + // ensure that a `kErrorResponseTimeout` error is reported. + + AdvanceTime(45 * 1000); + + VerifyOrQuit(sResolveServiceInfo.mCallbackCount == 1); + VerifyOrQuit(sResolveServiceInfo.mError == kErrorResponseTimeout); + + dnsServer->SetTestMode(Dns::ServiceDiscovery::Server::kTestModeDisabled); + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - // Validate DNS Client `ResolveService()` using all service modes // when sever does not provide any RR in the addition data section. @@ -832,7 +874,7 @@ void TestDnsClient(void) Log("- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - "); Log("Set TestMode on server to not include any RR in additional section AND to only accept single question"); dnsServer->SetTestMode(Dns::ServiceDiscovery::Server::kTestModeEmptyAdditionalSection + - Dns::ServiceDiscovery::Server::kTestModeSingleQuestionOnly); + Dns::ServiceDiscovery::Server::kTestModeRejectMultiQuestionQuery); Log("ResolveServiceAndHostAddress(%s,%s) with ServiceMode: %s", kInstance1Label, kService1FullName, ServiceModeToString(Dns::Client::QueryConfig::kServiceModeSrvTxtOptimize));