From: Oliver Chen Date: Wed, 9 Apr 2025 06:58:43 +0000 (+0000) Subject: Generate timeout response packet, clang-tidy, PR comments X-Git-Tag: dnsdist-2.0.0-alpha2~75^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1fe274fa82c795c77e109397ee3079836f49522e;p=thirdparty%2Fpdns.git 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 0d8200f08c..4502c070d3 100644 --- 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 6b54d9b4f7..63a53c4e0f 100644 --- 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 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& getHTTPHeaders() - { - return stubHeaders; - } - virtual std::shared_ptr 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) { } diff --git a/pdns/dnsdistdist/dnsdist-self-answers.cc b/pdns/dnsdistdist/dnsdist-self-answers.cc index dd0ff0b119..5586b230e7 100644 --- 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 qtc = {static_cast(qtype>>8), static_cast(qtype&0xff), static_cast(qclass>>8), static_cast(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 5703db4648..2c02beb5dd 100644 --- 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& addresses, const ResponseConfig& responseConfig); bool generateAnswerFromRDataEntries(DNSQuestion& dnsQuestion, const std::vector& entries, std::optional 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 935aa04476..86e475ad2c 100644 --- a/pdns/dnsdistdist/dnsdist-settings-definitions.yml +++ b/pdns/dnsdistdist/dnsdist-settings-definitions.yml @@ -146,7 +146,7 @@ global: type: "Vec" 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 591eaf1544..c4e57b2fff 100644 --- 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& rules, InternalQueryState& ids, std::shared_ptr ds, std::shared_ptr sender) +bool handleTimeoutResponseRules(const std::vector& rules, InternalQueryState& ids, const std::shared_ptr& d_ds, const std::shared_ptr& 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 2652a65d62..278494315a 100644 --- a/pdns/dnsdistdist/dnsdist.hh +++ b/pdns/dnsdistdist/dnsdist.hh @@ -984,4 +984,4 @@ ssize_t udpClientSendRequestToBackend(const std::shared_ptr& 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& rules, InternalQueryState& ids, std::shared_ptr ds, std::shared_ptr sender); +bool handleTimeoutResponseRules(const std::vector& rules, InternalQueryState& ids, const std::shared_ptr& ds, const std::shared_ptr& sender); diff --git a/pdns/dnsdistdist/docs/reference/rules-management.rst b/pdns/dnsdistdist/docs/reference/rules-management.rst index 713236f600..f78358e745 100644 --- 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 5c7d0a8584..ebe7142554 100644 --- a/pdns/dnsdistdist/test-dnsdist_cc.cc +++ b/pdns/dnsdistdist/test-dnsdist_cc.cc @@ -65,11 +65,11 @@ bool applyRulesToResponse(const std::vector& return true; } -bool handleTimeoutResponseRules(const std::vector& rules, InternalQueryState& ids, std::shared_ptr ds, std::shared_ptr sender) +bool handleTimeoutResponseRules(const std::vector& rules, InternalQueryState& ids, const std::shared_ptr& d_ds, const std::shared_ptr& 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 a365498525..afb79ab3a9 100644 --- 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 5cf8a347a5..e75b099709 100644 --- 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