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 7a781f15985812d03631ef37b9db488367f62bc6..11da401daf2ba5f3e781aea19a6be815e97d8b17 100644 (file)
--- a/pdns/dnsdist-tcp.cc
+++ b/pdns/dnsdist-tcp.cc
@@ -247,10 +247,10 @@ static void handleResponseSent(std::shared_ptr<IncomingTCPConnectionState>& stat
     if (backendProtocol == dnsdist::Protocol::DoUDP) {
       backendProtocol = dnsdist::Protocol::DoTCP;
     }
-    ::handleResponseSent(ids, udiff, state->d_ci.remote, ds->d_config.remote, static_cast<unsigned int>(currentResponse.d_buffer.size()), currentResponse.d_cleartextDH, backendProtocol);
+    ::handleResponseSent(ids, udiff, state->d_ci.remote, ds->d_config.remote, static_cast<unsigned int>(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<unsigned int>(currentResponse.d_buffer.size()), currentResponse.d_cleartextDH, ids.protocol);
+    ::handleResponseSent(ids, 0., state->d_ci.remote, ComboAddress(), static_cast<unsigned int>(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 1532226c26e93b572ffea9d9728dd582ad3bf8a8..c141aa9c4a0cbf649ceb609f8cf0bfea1f66310f 100644 (file)
--- 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 882535611370b6702438fd1795a0b491b1ed9e48..88900e1dab03cf40dbfdf56c5570f06238d05156 100644 (file)
--- a/pdns/dnsdist.hh
+++ b/pdns/dnsdist.hh
@@ -1233,5 +1233,5 @@ bool assignOutgoingUDPQueryToBackend(std::shared_ptr<DownstreamState>& ds, uint1
 
 ssize_t udpClientSendRequestToBackend(const std::shared_ptr<DownstreamState>& 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 ef31e2c6c39c9a8e03fcab6d88597c3c886130af..8481193d877b901a4c32dbbd63528a154b3b93f8 100644 (file)
--- 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<const struct dnsheader*>(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 7da8b3153bdc86b96643eb76cf81d00474d4ba59..0904441da6fe2239d0bf828d6b4595ea660acd8a 100644 (file)
--- 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 08a84daca0e8364059b5c37dc69ce89ee26d0b69..5850d954258ca06fadaed616bf75680796a5b467 100644 (file)
--- 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)