Generate timeout response packet, clang-tidy, PR comments

Generate a valid packet for timeout response rules so that other
actions that requires packet buffer would be happy. Fix a few
clang-tidy issues, address a few PR comments, i.e. revert changes
that fixed in other commit.

diff --git a/pdns/dnsdistdist/dnsdist-configuration-yaml.cc b/pdns/dnsdistdist/dnsdist-configuration-yaml.cc
index 0d8200f08cab16d7af8ecabc4ef16cf09be80faa..4502c070d3db6cda50c655d1183f9344e1871bb9 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-configuration-yaml.cc
+++ b/pdns/dnsdistdist/dnsdist-configuration-yaml.cc
@@ -510,42 +510,42 @@ static void loadRulesConfiguration(const dnsdist::rust::settings::GlobalConfigur
   dnsdist::configuration::updateRuntimeConfiguration([&globalConfig](dnsdist::configuration::RuntimeConfiguration& config) {
     for (const auto& rule : globalConfig.query_rules) {
       boost::uuids::uuid ruleUniqueID = rule.uuid.empty() ? getUniqueID() : getUniqueID(std::string(rule.uuid));
-      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::RuleChain::Rules, std::move(rule.selector.selector->d_rule), rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
+      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::RuleChain::Rules, rule.selector.selector->d_rule, rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
     }
 
     for (const auto& rule : globalConfig.cache_miss_rules) {
       boost::uuids::uuid ruleUniqueID = rule.uuid.empty() ? getUniqueID() : getUniqueID(std::string(rule.uuid));
-      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::RuleChain::CacheMissRules, std::move(rule.selector.selector->d_rule), rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
+      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::RuleChain::CacheMissRules, rule.selector.selector->d_rule, rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
     }
 
     for (const auto& rule : globalConfig.response_rules) {
       boost::uuids::uuid ruleUniqueID = rule.uuid.empty() ? getUniqueID() : getUniqueID(std::string(rule.uuid));
-      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::ResponseRules, std::move(rule.selector.selector->d_rule), rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
+      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::ResponseRules, rule.selector.selector->d_rule, rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
     }
 
     for (const auto& rule : globalConfig.cache_hit_response_rules) {
       boost::uuids::uuid ruleUniqueID = rule.uuid.empty() ? getUniqueID() : getUniqueID(std::string(rule.uuid));
-      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::CacheHitResponseRules, std::move(rule.selector.selector->d_rule), rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
+      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::CacheHitResponseRules, rule.selector.selector->d_rule, rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
     }
 
     for (const auto& rule : globalConfig.cache_inserted_response_rules) {
       boost::uuids::uuid ruleUniqueID = rule.uuid.empty() ? getUniqueID() : getUniqueID(std::string(rule.uuid));
-      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::CacheInsertedResponseRules, std::move(rule.selector.selector->d_rule), rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
+      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::CacheInsertedResponseRules, rule.selector.selector->d_rule, rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
     }
 
     for (const auto& rule : globalConfig.self_answered_response_rules) {
       boost::uuids::uuid ruleUniqueID = rule.uuid.empty() ? getUniqueID() : getUniqueID(std::string(rule.uuid));
-      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::SelfAnsweredResponseRules, std::move(rule.selector.selector->d_rule), rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
+      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::SelfAnsweredResponseRules, rule.selector.selector->d_rule, rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
     }
 
     for (const auto& rule : globalConfig.xfr_response_rules) {
       boost::uuids::uuid ruleUniqueID = rule.uuid.empty() ? getUniqueID() : getUniqueID(std::string(rule.uuid));
-      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::XFRResponseRules, std::move(rule.selector.selector->d_rule), rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
+      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::XFRResponseRules, rule.selector.selector->d_rule, rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
     }
 
     for (const auto& rule : globalConfig.timeout_response_rules) {
       boost::uuids::uuid ruleUniqueID = rule.uuid.empty() ? getUniqueID() : getUniqueID(std::string(rule.uuid));
-      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::TimeoutResponseRules, std::move(rule.selector.selector->d_rule), rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
+      dnsdist::rules::add(config.d_ruleChains, dnsdist::rules::ResponseRuleChain::TimeoutResponseRules, rule.selector.selector->d_rule, rule.action.action->d_action, std::string(rule.name), ruleUniqueID, 0);
     }
   });
 }

diff --git a/pdns/dnsdistdist/dnsdist-doh-common.hh b/pdns/dnsdistdist/dnsdist-doh-common.hh
index 6b54d9b4f7bbc37c38ce3d4a91304f7eb7fd04f0..63a53c4e0f9f8c2fa814abe9dc1a45ae7687f555 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-doh-common.hh
+++ b/pdns/dnsdistdist/dnsdist-doh-common.hh
@@ -207,50 +207,15 @@ class TCPQuerySender;
 #ifndef HAVE_DNS_OVER_HTTPS
 struct DOHUnitInterface
 {
-  std::string stubStr;
-  std::unordered_map<std::string, std::string> stubHeaders;
-
   virtual ~DOHUnitInterface()
   {
   }
 
-  virtual std::string getHTTPPath()
-  {
-    return std::string();
-  }
-
-  virtual std::string getHTTPQueryString()
-  {
-    return std::string();
-  }
-
-  virtual const std::string& getHTTPHost()
-  {
-    return stubStr;
-  }
-
-  virtual const std::string& getHTTPScheme()
-  {
-    return stubStr;
-  }
-
-  virtual const std::unordered_map<std::string, std::string>& getHTTPHeaders()
-  {
-    return stubHeaders;
-  }
-
   virtual std::shared_ptr<TCPQuerySender> getQuerySender() const
   {
     return nullptr;
   }
 
-  virtual void setHTTPResponse(uint16_t statusCode, PacketBuffer&& body, const std::string& contentType = "")
-  {
-    (void)statusCode;
-    (void)body;
-    (void)contentType;
-  }
-
   static void handleTimeout(std::unique_ptr<DOHUnitInterface>)
   {
   }

diff --git a/pdns/dnsdistdist/dnsdist-self-answers.cc b/pdns/dnsdistdist/dnsdist-self-answers.cc
index dd0ff0b119ec7c7b2e6a779324fbb56790f67220..5586b230e7b75340a939bcc14cb424d19283cebd 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-self-answers.cc
+++ b/pdns/dnsdistdist/dnsdist-self-answers.cc
@@ -240,4 +240,28 @@ bool generateAnswerFromRawPacket(DNSQuestion& dnsQuestion, const PacketBuffer& p
   return true;
 }
 
+bool generateAnswerForTimeoutQuery(const PacketBuffer& query, PacketBuffer& answer, const DNSName& dnsQName, uint16_t qtype, uint16_t qclass)
+{
+  auto& qname = dnsQName.getStorage();
+  if (query.size() < sizeof(dnsheader) || qname.length() <= 1) {
+    return false;
+  }
+  std::vector<uint8_t> qtc = {static_cast<uint8_t>(qtype>>8), static_cast<uint8_t>(qtype&0xff), static_cast<uint8_t>(qclass>>8), static_cast<uint8_t>(qclass&0xff)};
+
+  answer.resize(sizeof(dnsheader) + qname.length() + 4);
+  memcpy(&answer.at(0), &query.at(0), sizeof(dnsheader));
+  memcpy(&answer.at(sizeof(dnsheader)), qname.c_str(), qname.length());
+  memcpy(&answer.at(sizeof(dnsheader)+qname.length()), &qtc.at(0), 4);
+
+  dnsdist::PacketMangling::editDNSHeaderFromPacket(answer, [](dnsheader& header) {
+    header.qr = true;
+    header.qdcount = htons(1);
+    header.ancount = 0;
+    header.nscount = 0;
+    header.arcount = 0;
+    return true;
+  });
+  return true;
+}
+
 }

diff --git a/pdns/dnsdistdist/dnsdist-self-answers.hh b/pdns/dnsdistdist/dnsdist-self-answers.hh
index 5703db46488d7baa4985c3bca926569d5b8ef614..2c02beb5dd5f5ef3922a8e5ab6ee30e7ead40cfd 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-self-answers.hh
+++ b/pdns/dnsdistdist/dnsdist-self-answers.hh
@@ -29,4 +29,5 @@ bool generateAnswerFromCNAME(DNSQuestion& dnsQuestion, const DNSName& cname, con
 bool generateAnswerFromIPAddresses(DNSQuestion& dnsQuestion, const std::vector<ComboAddress>& addresses, const ResponseConfig& responseConfig);
 bool generateAnswerFromRDataEntries(DNSQuestion& dnsQuestion, const std::vector<std::string>& entries, std::optional<uint16_t> typeForAny, const ResponseConfig& responseConfig);
 bool generateAnswerFromRawPacket(DNSQuestion& dnsQuestion, const PacketBuffer& packet);
+bool generateAnswerForTimeoutQuery(const PacketBuffer& query, PacketBuffer& answer, const DNSName& dnsQName, uint16_t qtype, uint16_t qclass);
 }

diff --git a/pdns/dnsdistdist/dnsdist-settings-definitions.yml b/pdns/dnsdistdist/dnsdist-settings-definitions.yml
index 935aa04476b3db1d3f0ca651658e4cb598d2b28b..86e475ad2caec0e6a021969f08a469dcf8e2ef2a 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-settings-definitions.yml
+++ b/pdns/dnsdistdist/dnsdist-settings-definitions.yml
@@ -146,7 +146,7 @@ global:
       type: "Vec<ResponseRuleConfiguration>"
       default: true
       skip-serde: true
-      description: "List of rules executed when a timeout event occurred"
+      description: "List of rules executed when a timeout event triggered from timer expiration or I/O error"
 
 metrics:
   description: "Metrics-related settings"

diff --git a/pdns/dnsdistdist/dnsdist.cc b/pdns/dnsdistdist/dnsdist.cc
index 591eaf1544b20420d74354e2aa36ef73ee58dead..c4e57b2fff7969d29a598324b8eda9fb87986b5d 100644 (file)
--- a/pdns/dnsdistdist/dnsdist.cc
+++ b/pdns/dnsdistdist/dnsdist.cc
@@ -1565,10 +1565,18 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dnsQuestion, std::shared_
   return ProcessQueryResult::Drop;
 }
 
-bool handleTimeoutResponseRules(const std::vector<dnsdist::rules::ResponseRuleAction>& rules, InternalQueryState& ids, std::shared_ptr<DownstreamState> ds, std::shared_ptr<TCPQuerySender> sender)
+bool handleTimeoutResponseRules(const std::vector<dnsdist::rules::ResponseRuleAction>& rules, InternalQueryState& ids, const std::shared_ptr<DownstreamState>& d_ds, const std::shared_ptr<TCPQuerySender>& sender)
 {
-  PacketBuffer empty;
-  DNSResponse dnsResponse(ids, empty, ds);
+  if (!ids.d_packet || ids.d_packet->size() < sizeof(struct dnsheader)) {
+    return false;
+  }
+
+  PacketBuffer answer;
+  if (!dnsdist::self_answers::generateAnswerForTimeoutQuery(*ids.d_packet, answer, ids.qname, ids.qtype, ids.qclass)) {
+    return false;
+  }
+
+  DNSResponse dnsResponse(ids, answer, d_ds);
   auto protocol = dnsResponse.getProtocol();
 
   vinfolog("Handling timeout response rules for incoming protocol = %s", protocol.toString());

diff --git a/pdns/dnsdistdist/dnsdist.hh b/pdns/dnsdistdist/dnsdist.hh
index 2652a65d629b4dd326b055ad3aec750153dd9749..278494315aeded3f61c9afda4b93581c2b9ea40c 100644 (file)
--- a/pdns/dnsdistdist/dnsdist.hh
+++ b/pdns/dnsdistdist/dnsdist.hh
@@ -984,4 +984,4 @@ ssize_t udpClientSendRequestToBackend(const std::shared_ptr<DownstreamState>& ba
 bool sendUDPResponse(int origFD, const PacketBuffer& response, const int delayMsec, const ComboAddress& origDest, const ComboAddress& origRemote);
 void handleResponseSent(const DNSName& qname, const QType& qtype, double udiff, const ComboAddress& client, const ComboAddress& backend, unsigned int size, const dnsheader& cleartextDH, dnsdist::Protocol outgoingProtocol, dnsdist::Protocol incomingProtocol, bool fromBackend);
 void handleResponseSent(const InternalQueryState& ids, double udiff, const ComboAddress& client, const ComboAddress& backend, unsigned int size, const dnsheader& cleartextDH, dnsdist::Protocol outgoingProtocol, bool fromBackend);
-bool handleTimeoutResponseRules(const std::vector<dnsdist::rules::ResponseRuleAction>& rules, InternalQueryState& ids, std::shared_ptr<DownstreamState> ds, std::shared_ptr<TCPQuerySender> sender);
+bool handleTimeoutResponseRules(const std::vector<dnsdist::rules::ResponseRuleAction>& rules, InternalQueryState& ids, const std::shared_ptr<DownstreamState>& ds, const std::shared_ptr<TCPQuerySender>& sender);

diff --git a/pdns/dnsdistdist/docs/reference/rules-management.rst b/pdns/dnsdistdist/docs/reference/rules-management.rst
index 713236f60065e03a6b7d1679aeaad18782b264b5..f78358e745900b1c398cdbd2e13e2366ebfbcd25 100644 (file)
--- a/pdns/dnsdistdist/docs/reference/rules-management.rst
+++ b/pdns/dnsdistdist/docs/reference/rules-management.rst
@@ -520,7 +520,7 @@ For Rules related to timed out queries:
 
   .. versionadded:: 2.0.0
 
-  Add a Rule and Action for timeout responses to the existing rules.
+  Add a Rule and Action for timeout triggered from timer expiration or I/O error.
 
   :param DNSrule rule: A :class:`DNSRule`, e.g. an :func:`AllRule`, or a compounded bunch of rules using e.g. :func:`AndRule`.
   :param action: The action to take

diff --git a/pdns/dnsdistdist/test-dnsdist_cc.cc b/pdns/dnsdistdist/test-dnsdist_cc.cc
index 5c7d0a85847cd73fc30ca7405e15e751376bc5c0..ebe7142554934f1953cc13940d64df3a2c0b2f74 100644 (file)
--- a/pdns/dnsdistdist/test-dnsdist_cc.cc
+++ b/pdns/dnsdistdist/test-dnsdist_cc.cc
@@ -65,11 +65,11 @@ bool applyRulesToResponse(const std::vector<dnsdist::rules::ResponseRuleAction>&
   return true;
 }
 
-bool handleTimeoutResponseRules(const std::vector<dnsdist::rules::ResponseRuleAction>& rules, InternalQueryState& ids, std::shared_ptr<DownstreamState> ds, std::shared_ptr<TCPQuerySender> sender)
+bool handleTimeoutResponseRules(const std::vector<dnsdist::rules::ResponseRuleAction>& rules, InternalQueryState& ids, const std::shared_ptr<DownstreamState>& d_ds, const std::shared_ptr<TCPQuerySender>& sender)
 {
   (void)rules;
   (void)ids;
-  (void)ds;
+  (void)d_ds;
   (void)sender;
   return false;
 }

diff --git a/regression-tests.dnsdist/dnsdisttests.py b/regression-tests.dnsdist/dnsdisttests.py
index a365498525c04fdcaa9b9e3d5b30f64086e3bf59..afb79ab3a9a864baaa263814970921033cb78523 100644 (file)
--- a/regression-tests.dnsdist/dnsdisttests.py
+++ b/regression-tests.dnsdist/dnsdisttests.py
@@ -63,7 +63,7 @@ def pickAvailablePort():
     workerPorts[workerID] = port
     return port
 
-class DropAction(object):
+class ResponderDropAction(object):
     """
     An object to indicate a drop action shall be taken
     """
@@ -357,7 +357,7 @@ class DNSDistTest(AssertEqualDNSMessageMixin, unittest.TestCase):
 
             if not wire:
               continue
-            elif isinstance(wire, DropAction):
+            elif isinstance(wire, ResponderDropAction):
               continue
 
             sock.sendto(wire, addr)
@@ -400,7 +400,7 @@ class DNSDistTest(AssertEqualDNSMessageMixin, unittest.TestCase):
       if not wire:
         conn.close()
         return
-      elif isinstance(wire, DropAction):
+      elif isinstance(wire, ResponderDropAction):
         return
 
       wireLen = struct.pack("!H", len(wire))
@@ -566,7 +566,7 @@ class DNSDistTest(AssertEqualDNSMessageMixin, unittest.TestCase):
                             conn.close()
                             conn = None
                             break
-                        elif isinstance(wire, DropAction):
+                        elif isinstance(wire, ResponderDropAction):
                             break
 
                         headers = [

diff --git a/regression-tests.dnsdist/test_TimeoutResponse.py b/regression-tests.dnsdist/test_TimeoutResponse.py
index 5cf8a347a51d56e154878eb439e15007e699cc89..e75b09970988b7fbf028a458553e9bf2adcca372 100644 (file)
--- a/regression-tests.dnsdist/test_TimeoutResponse.py
+++ b/regression-tests.dnsdist/test_TimeoutResponse.py
@@ -2,7 +2,7 @@
 import ssl
 import threading
 import dns
-from dnsdisttests import DNSDistTest, pickAvailablePort, DropAction
+from dnsdisttests import DNSDistTest, pickAvailablePort, ResponderDropAction
 
 _common_config = """
     addDOHLocal("127.0.0.1:%d", "server.chain", "server.key", {'/dns-query'}, {library='nghttp2'})
@@ -28,7 +28,7 @@ _common_config = """
 """
 
 def timeoutResponseCallback(request):
-    return DropAction()
+    return ResponderDropAction()
 
 def normalResponseCallback(request):
     response = dns.message.make_response(request)
@@ -62,8 +62,8 @@ class TestTimeoutBackendUdpTcp(DNSDistTest):
     _dohBaseURL = ("https://%s:%d/" % (_serverName, _doh3ServerPort))
 
     _config_template = """
-    newServer{address="127.0.0.1:%d",pool='restarted',udpTimeout=2,tcpRecvTimeout=2}:setUp()
-    newServer{address="127.0.0.1:%d",pool='',udpTimeout=2,tcpRecvTimeout=2}:setUp()
+    newServer{address="127.0.0.1:%d",pool='restarted',udpTimeout=1,tcpRecvTimeout=1}:setUp()
+    newServer{address="127.0.0.1:%d",pool='',udpTimeout=1,tcpRecvTimeout=1}:setUp()
     """ + _common_config
     _config_params = ['_testNormalServerPort', '_testTimeoutServerPort', '_dohWithNGHTTP2ServerPort', '_doqServerPort', '_doh3ServerPort', '_tlsServerPort']
     _verboseMode = True
@@ -102,15 +102,15 @@ class TestTimeoutBackendUdpTcp(DNSDistTest):
 
         for method in ("sendUDPQuery", "sendTCPQuery", "sendDOQQueryWrapper", "sendDOH3QueryWrapper", "sendDOTQueryWrapper", "sendDOHWithNGHTTP2QueryWrapper"):
             sender = getattr(self, method)
-            (_, receivedResponse) = sender(query, response=None, useQueue=False, timeout=6)
+            (_, receivedResponse) = sender(query, response=None, useQueue=False, timeout=3)
             self.assertTrue(receivedResponse)
             self.assertEqual(receivedResponse, expectedResponse)
 
 class TestTimeoutBackendDOH(TestTimeoutBackendUdpTcp):
 
     _config_template = """
-    newServer{address="127.0.0.1:%d",pool='restarted',udpTimeout=2,tcpRecvTimeout=2,tls='openssl',validateCertificates=true,caStore='ca.pem',subjectName='powerdns.com',dohPath='/dns-query'}:setUp()
-    newServer{address="127.0.0.1:%d",pool='',udpTimeout=2,tcpRecvTimeout=2,tls='openssl',validateCertificates=true,caStore='ca.pem',subjectName='powerdns.com',dohPath='/dns-query'}:setUp()
+    newServer{address="127.0.0.1:%d",pool='restarted',udpTimeout=1,tcpRecvTimeout=1,tls='openssl',validateCertificates=true,caStore='ca.pem',subjectName='powerdns.com',dohPath='/dns-query'}:setUp()
+    newServer{address="127.0.0.1:%d",pool='',udpTimeout=1,tcpRecvTimeout=1,tls='openssl',validateCertificates=true,caStore='ca.pem',subjectName='powerdns.com',dohPath='/dns-query'}:setUp()
     """ + _common_config
 
     @classmethod
@@ -132,8 +132,8 @@ class TestTimeoutBackendDOH(TestTimeoutBackendUdpTcp):
 class TestTimeoutBackendDOT(TestTimeoutBackendUdpTcp):
 
     _config_template = """
-    newServer{address="127.0.0.1:%d",pool='restarted',udpTimeout=2,tcpRecvTimeout=2,tls='openssl',validateCertificates=true,caStore='ca.pem',subjectName='powerdns.com'}:setUp()
-    newServer{address="127.0.0.1:%d",pool='',udpTimeout=2,tcpRecvTimeout=2,tls='openssl',validateCertificates=true,caStore='ca.pem',subjectName='powerdns.com'}:setUp()
+    newServer{address="127.0.0.1:%d",pool='restarted',udpTimeout=1,tcpRecvTimeout=1,tls='openssl',validateCertificates=true,caStore='ca.pem',subjectName='powerdns.com'}:setUp()
+    newServer{address="127.0.0.1:%d",pool='',udpTimeout=1,tcpRecvTimeout=1,tls='openssl',validateCertificates=true,caStore='ca.pem',subjectName='powerdns.com'}:setUp()
     """ + _common_config
 
     @classmethod