From: Remi Gacogne Date: Mon, 27 Feb 2023 09:58:38 +0000 (+0100) Subject: dnsdist: Only increment the 'servfail-responses' metric on backend responses X-Git-Tag: dnsdist-1.8.0-rc2~8^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F12592%2Fhead;p=thirdparty%2Fpdns.git dnsdist: Only increment the 'servfail-responses' metric on backend responses Reported by phonedph1 (many thanks!). --- diff --git a/pdns/dnsdist-tcp.cc b/pdns/dnsdist-tcp.cc index 7a781f1598..11da401daf 100644 --- a/pdns/dnsdist-tcp.cc +++ b/pdns/dnsdist-tcp.cc @@ -247,10 +247,10 @@ static void handleResponseSent(std::shared_ptr& stat if (backendProtocol == dnsdist::Protocol::DoUDP) { backendProtocol = dnsdist::Protocol::DoTCP; } - ::handleResponseSent(ids, udiff, state->d_ci.remote, ds->d_config.remote, static_cast(currentResponse.d_buffer.size()), currentResponse.d_cleartextDH, backendProtocol); + ::handleResponseSent(ids, udiff, state->d_ci.remote, ds->d_config.remote, static_cast(currentResponse.d_buffer.size()), currentResponse.d_cleartextDH, backendProtocol, true); } else { const auto& ids = currentResponse.d_idstate; - ::handleResponseSent(ids, 0., state->d_ci.remote, ComboAddress(), static_cast(currentResponse.d_buffer.size()), currentResponse.d_cleartextDH, ids.protocol); + ::handleResponseSent(ids, 0., state->d_ci.remote, ComboAddress(), static_cast(currentResponse.d_buffer.size()), currentResponse.d_cleartextDH, ids.protocol, false); } currentResponse.d_buffer.clear(); diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc index 1532226c26..c141aa9c4a 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -621,12 +621,12 @@ bool sendUDPResponse(int origFD, const PacketBuffer& response, const int delayMs return true; } -void handleResponseSent(const InternalQueryState& ids, double udiff, const ComboAddress& client, const ComboAddress& backend, unsigned int size, const dnsheader& cleartextDH, dnsdist::Protocol outgoingProtocol) +void handleResponseSent(const InternalQueryState& ids, double udiff, const ComboAddress& client, const ComboAddress& backend, unsigned int size, const dnsheader& cleartextDH, dnsdist::Protocol outgoingProtocol, bool fromBackend) { - handleResponseSent(ids.qname, ids.qtype, udiff, client, backend, size, cleartextDH, outgoingProtocol, ids.protocol); + handleResponseSent(ids.qname, ids.qtype, udiff, client, backend, size, cleartextDH, outgoingProtocol, ids.protocol, fromBackend); } -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) +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) { if (g_rings.shouldRecordResponses()) { struct timespec ts; @@ -639,7 +639,9 @@ void handleResponseSent(const DNSName& qname, const QType& qtype, double udiff, ++g_stats.frontendNXDomain; break; case RCode::ServFail: - ++g_stats.servfailResponses; + if (fromBackend) { + ++g_stats.servfailResponses; + } ++g_stats.frontendServFail; break; case RCode::NoError: @@ -700,10 +702,10 @@ static void handleResponseForUDPClient(InternalQueryState& ids, PacketBuffer& re vinfolog("Got answer from %s, NOT relayed to %s (UDP) since that frontend is muted, took %f usec", ds->d_config.remote.toStringWithPort(), ids.origRemote.toStringWithPort(), udiff); } - handleResponseSent(ids, udiff, dr.ids.origRemote, ds->d_config.remote, response.size(), cleartextDH, ds->getProtocol()); + handleResponseSent(ids, udiff, dr.ids.origRemote, ds->d_config.remote, response.size(), cleartextDH, ds->getProtocol(), true); } else { - handleResponseSent(ids, 0., dr.ids.origRemote, ComboAddress(), response.size(), cleartextDH, dnsdist::Protocol::DoUDP); + handleResponseSent(ids, 0., dr.ids.origRemote, ComboAddress(), response.size(), cleartextDH, dnsdist::Protocol::DoUDP, false); } } @@ -1670,7 +1672,7 @@ static void processUDPQuery(ClientState& cs, LocalHolders& holders, const struct /* we use dest, always, because we don't want to use the listening address to send a response since it could be 0.0.0.0 */ sendUDPResponse(cs.udpFD, query, dq.ids.delayMsec, dest, remote); - handleResponseSent(dq.ids.qname, dq.ids.qtype, 0., remote, ComboAddress(), query.size(), *dh, dnsdist::Protocol::DoUDP, dnsdist::Protocol::DoUDP); + handleResponseSent(dq.ids.qname, dq.ids.qtype, 0., remote, ComboAddress(), query.size(), *dh, dnsdist::Protocol::DoUDP, dnsdist::Protocol::DoUDP, false); return; } diff --git a/pdns/dnsdist.hh b/pdns/dnsdist.hh index 8825356113..88900e1dab 100644 --- a/pdns/dnsdist.hh +++ b/pdns/dnsdist.hh @@ -1233,5 +1233,5 @@ bool assignOutgoingUDPQueryToBackend(std::shared_ptr& ds, uint1 ssize_t udpClientSendRequestToBackend(const std::shared_ptr& ss, const int sd, const PacketBuffer& request, bool healthCheck = false); 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); -void handleResponseSent(const InternalQueryState& ids, double udiff, const ComboAddress& client, const ComboAddress& backend, unsigned int size, const dnsheader& cleartextDH, dnsdist::Protocol outgoingProtocol); +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); diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc index ef31e2c6c3..8481193d87 100644 --- a/pdns/dnsdistdist/doh.cc +++ b/pdns/dnsdistdist/doh.cc @@ -492,7 +492,7 @@ public: if (backendProtocol == dnsdist::Protocol::DoUDP && du->tcp) { backendProtocol = dnsdist::Protocol::DoTCP; } - handleResponseSent(du->ids, udiff, du->ids.origRemote, du->downstream->d_config.remote, du->response.size(), cleartextDH, backendProtocol); + handleResponseSent(du->ids, udiff, du->ids.origRemote, du->downstream->d_config.remote, du->response.size(), cleartextDH, backendProtocol, true); } ++g_stats.responses; @@ -733,7 +733,7 @@ static void processDOHQuery(DOHUnitUniquePtr&& unit, bool inMainThread = false) if (du->response.size() >= sizeof(dnsheader) && du->contentType.empty()) { auto dh = reinterpret_cast(du->response.data()); - handleResponseSent(du->ids.qname, QType(du->ids.qtype), 0., du->ids.origDest, ComboAddress(), du->response.size(), *dh, dnsdist::Protocol::DoH, dnsdist::Protocol::DoH); + handleResponseSent(du->ids.qname, QType(du->ids.qtype), 0., du->ids.origDest, ComboAddress(), du->response.size(), *dh, dnsdist::Protocol::DoH, dnsdist::Protocol::DoH, false); } handleImmediateResponse(std::move(du), "DoH self-answered response"); return; @@ -1737,7 +1737,7 @@ void handleUDPResponseForDoH(DOHUnitUniquePtr&& du, PacketBuffer&& udpResponse, double udiff = du->ids.queryRealTime.udiff(); vinfolog("Got answer from %s, relayed to %s (https), took %f usec", du->downstream->d_config.remote.toStringWithPort(), du->ids.origRemote.toStringWithPort(), udiff); - handleResponseSent(du->ids, udiff, dr.ids.origRemote, du->downstream->d_config.remote, du->response.size(), cleartextDH, du->downstream->getProtocol()); + handleResponseSent(du->ids, udiff, dr.ids.origRemote, du->downstream->d_config.remote, du->response.size(), cleartextDH, du->downstream->getProtocol(), true); ++g_stats.responses; if (du->ids.cs) { diff --git a/pdns/dnsdistdist/test-dnsdisttcp_cc.cc b/pdns/dnsdistdist/test-dnsdisttcp_cc.cc index 7da8b3153b..0904441da6 100644 --- a/pdns/dnsdistdist/test-dnsdisttcp_cc.cc +++ b/pdns/dnsdistdist/test-dnsdisttcp_cc.cc @@ -59,7 +59,7 @@ uint64_t uptimeOfProcess(const std::string& str) return 0; } -void handleResponseSent(const InternalQueryState& ids, double udiff, const ComboAddress& client, const ComboAddress& backend, unsigned int size, const dnsheader& cleartextDH, dnsdist::Protocol protocol) +void handleResponseSent(const InternalQueryState& ids, double udiff, const ComboAddress& client, const ComboAddress& backend, unsigned int size, const dnsheader& cleartextDH, dnsdist::Protocol protocol, bool fromBackend) { } diff --git a/regression-tests.dnsdist/test_Metrics.py b/regression-tests.dnsdist/test_Metrics.py index 08a84daca0..5850d95425 100644 --- a/regression-tests.dnsdist/test_Metrics.py +++ b/regression-tests.dnsdist/test_Metrics.py @@ -60,6 +60,8 @@ class TestRuleMetrics(DNSDistTest): ( 'refused', dns.rcode.REFUSED ), ( 'servfail', dns.rcode.SERVFAIL ) ] + servfailBackendResponses = self.getMetric('servfail-responses') + for (name, rcode) in rcodes: qname = 'rcode-' + name + '.metrics.tests.powerdns.com.' query = dns.message.make_query(qname, 'A', 'IN') @@ -68,13 +70,21 @@ class TestRuleMetrics(DNSDistTest): expectedResponse = dns.message.make_response(query) expectedResponse.set_rcode(rcode) + ruleMetricBefore = self.getMetric('rule-' + name) + if name != 'refused': + frontendMetricBefore = self.getMetric('frontend-' + name) + for method in ("sendUDPQuery", "sendTCPQuery"): sender = getattr(self, method) (_, receivedResponse) = sender(query, response=None, useQueue=False) self.assertEqual(receivedResponse, expectedResponse) - self.assertEquals(self.getMetric('rule-' + name), 2) + self.assertEqual(self.getMetric('rule-' + name), ruleMetricBefore + 2) + if name != 'refused': + self.assertEqual(self.getMetric('frontend-' + name), frontendMetricBefore + 2) + # self-generated responses should not increase this metric + self.assertEqual(self.getMetric('servfail-responses'), servfailBackendResponses) def testCacheMetrics(self): """ Metrics: Check that metrics are correctly updated for cache misses and hits @@ -110,3 +120,34 @@ class TestRuleMetrics(DNSDistTest): self.assertEqual(self.getMetric('responses'), responsesBefore + 2) self.assertEqual(self.getMetric('cache-hits'), cacheHitsBefore + 1) self.assertEqual(self.getMetric('cache-misses'), cacheMissesBefore + 1) + + def testServFailMetrics(self): + """ + Metrics: Check that servfail metrics are correctly updated for cache misses and hits + """ + + for method in ("sendUDPQuery", "sendTCPQuery", "sendDOTQueryWrapper", "sendDOHQueryWrapper"): + qname = method + '.servfail.cache.metrics.tests.powerdns.com.' + query = dns.message.make_query(qname, 'A', 'IN') + # dnsdist set RA = RD for spoofed responses + query.flags &= ~dns.flags.RD + response = dns.message.make_response(query) + response.set_rcode(dns.rcode.SERVFAIL) + + frontendBefore = self.getMetric('frontend-servfail') + servfailBefore = self.getMetric('servfail-responses') + ruleBefore = self.getMetric('rule-servfail') + + sender = getattr(self, method) + # first time, cache miss + (receivedQuery, receivedResponse) = sender(query, response) + receivedQuery.id = query.id + self.assertEqual(query, receivedQuery) + self.assertEqual(receivedResponse, response) + # second time, hit + (_, receivedResponse) = sender(query, response=None, useQueue=False) + self.assertEqual(receivedResponse, response) + + self.assertEqual(self.getMetric('frontend-servfail'), frontendBefore + 2) + self.assertEqual(self.getMetric('servfail-responses'), servfailBefore + 1) + self.assertEqual(self.getMetric('rule-servfail'), ruleBefore)