dnsdist: Faster cache-lookups for DNS over HTTPS queries

For DoH queries we do two cache lookups:
- a first one for responses received over UDP, that can be truncated,
  which we do not want here as the client likely would not what to do
  with that
- then on a miss, a second one for responses received over TCP

We do not want the first lookup to override the initial query in
case of a hit for a TC=1 response, so we used to do a copy of the
query (allocation + copy).
This code move the TC=1 check to the cache itself so we do not have
to do that copy, speeding things up.

diff --git a/pdns/dnsdist-cache.cc b/pdns/dnsdist-cache.cc
index 7222a29162b0e47d897330802fcb62ef8c162673..ec936be2b1f43d9ab8521268d323de1beb8793b5 100644 (file)
--- a/pdns/dnsdist-cache.cc
+++ b/pdns/dnsdist-cache.cc
@@ -192,7 +192,7 @@ void DNSDistPacketCache::insert(uint32_t key, const boost::optional<Netmask>& su
   }
 }
 
-bool DNSDistPacketCache::get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut, boost::optional<Netmask>& subnet, bool dnssecOK, bool receivedOverUDP, uint32_t allowExpired, bool skipAging)
+bool DNSDistPacketCache::get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut, boost::optional<Netmask>& subnet, bool dnssecOK, bool receivedOverUDP, uint32_t allowExpired, bool skipAging, bool truncatedOK)
 {
   const auto& dnsQName = dq.qname->getStorage();
   uint32_t key = getKey(dnsQName, dq.qname->wirelength(), dq.getData(), receivedOverUDP);
@@ -245,6 +245,14 @@ bool DNSDistPacketCache::get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut
       return false;
     }
 
+    if (!truncatedOK) {
+      dnsheader dh;
+      memcpy(&dh, value.value.data(), sizeof(dh));
+      if (dh.tc != 0) {
+        return false;
+      }
+    }
+
     response.resize(value.len);
     memcpy(&response.at(0), &queryId, sizeof(queryId));
     memcpy(&response.at(sizeof(queryId)), &value.value.at(sizeof(queryId)), sizeof(dnsheader) - sizeof(queryId));

diff --git a/pdns/dnsdist-cache.hh b/pdns/dnsdist-cache.hh
index 23b321375c765faae21ecb1caf366f2aea38c40e..d7fcca8e527c3ac32ed58374550fa292d69eb4e8 100644 (file)
--- a/pdns/dnsdist-cache.hh
+++ b/pdns/dnsdist-cache.hh
@@ -38,7 +38,7 @@ public:
   DNSDistPacketCache(size_t maxEntries, uint32_t maxTTL=86400, uint32_t minTTL=0, uint32_t tempFailureTTL=60, uint32_t maxNegativeTTL=3600, uint32_t staleTTL=60, bool dontAge=false, uint32_t shards=1, bool deferrableInsertLock=true, bool parseECS=false);
 
   void insert(uint32_t key, const boost::optional<Netmask>& subnet, uint16_t queryFlags, bool dnssecOK, const DNSName& qname, uint16_t qtype, uint16_t qclass, const PacketBuffer& response, bool receivedOverUDP, uint8_t rcode, boost::optional<uint32_t> tempFailureTTL);
-  bool get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut, boost::optional<Netmask>& subnet, bool dnssecOK, bool receivedOverUDP, uint32_t allowExpired = 0, bool skipAging = false);
+  bool get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut, boost::optional<Netmask>& subnet, bool dnssecOK, bool receivedOverUDP, uint32_t allowExpired = 0, bool skipAging = false, bool truncatedOK = true);
   size_t purgeExpired(size_t upTo, const time_t now);
   size_t expunge(size_t upTo=0);
   size_t expungeByName(const DNSName& name, uint16_t qtype=QType::ANY, bool suffixMatch=false);

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index dcbfc9e1c62f04494c647eab28144c677f70c499..82a9073200dfcde89af232a02848623e546221f6 100644 (file)
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -1327,18 +1327,14 @@ ProcessQueryResult processQuery(DNSQuestion& dq, ClientState& cs, LocalHolders&
         return ProcessQueryResult::SendAnswer;
       }
       else if (dq.protocol == dnsdist::Protocol::DoH) {
-        /* do a second-lookup for UDP responses */
-        /* we need to do a copy to be able to restore the query on a TC=1 cached answer */
+        /* do a second-lookup for UDP responses, but we do not want TC=1 answers */
         PacketBuffer initialQuery(dq.getData());
-        if (dq.packetCache->get(dq, dq.getHeader()->id, &dq.cacheKeyUDP, dq.subnet, dq.dnssecOK, true, allowExpired)) {
-          if (dq.getHeader()->tc == 0) {
-            if (!prepareOutgoingResponse(holders, cs, dq, true)) {
-              return ProcessQueryResult::Drop;
-            }
-
-            return ProcessQueryResult::SendAnswer;
+        if (dq.packetCache->get(dq, dq.getHeader()->id, &dq.cacheKeyUDP, dq.subnet, dq.dnssecOK, true, allowExpired, false)) {
+          if (!prepareOutgoingResponse(holders, cs, dq, true)) {
+            return ProcessQueryResult::Drop;
           }
-          dq.getMutableData() = std::move(initialQuery);
+
+          return ProcessQueryResult::SendAnswer;
         }
       }
 

diff --git a/pdns/test-dnsdistpacketcache_cc.cc b/pdns/test-dnsdistpacketcache_cc.cc
index 4be01c2fcf1a272e1edd21cbcb4f1ac8d24a41e9..55a757cac1ef972930161575ffa9bc7575ba9ebc 100644 (file)
--- a/pdns/test-dnsdistpacketcache_cc.cc
+++ b/pdns/test-dnsdistpacketcache_cc.cc
@@ -156,7 +156,6 @@ BOOST_AUTO_TEST_CASE(test_PacketCacheSharded) {
       pwR.startRecord(a, QType::AAAA, 7200, QClass::IN, DNSResourceRecord::ANSWER);
       ComboAddress v6("2001:db8::1");
       pwR.xfrIP6(std::string(reinterpret_cast<const char*>(v6.sin6.sin6_addr.s6_addr), 16));
-      pwR.xfr32BitInt(0x01020304);
       pwR.commit();
 
       uint32_t key = 0;
@@ -250,7 +249,6 @@ BOOST_AUTO_TEST_CASE(test_PacketCacheTCP) {
     pwR.startRecord(a, QType::AAAA, 7200, QClass::IN, DNSResourceRecord::ANSWER);
     ComboAddress v6("2001:db8::1");
     pwR.xfrIP6(std::string(reinterpret_cast<const char*>(v6.sin6.sin6_addr.s6_addr), 16));
-    pwR.xfr32BitInt(0x01020304);
     pwR.commit();
 
     {
@@ -443,6 +441,59 @@ BOOST_AUTO_TEST_CASE(test_PacketCacheNXDomainTTL) {
   }
 }
 
+BOOST_AUTO_TEST_CASE(test_PacketCacheTruncated) {
+  const size_t maxEntries = 150000;
+  DNSDistPacketCache PC(maxEntries, /* maxTTL */ 86400, /* minTTL */ 1, /* tempFailureTTL */ 60, /* maxNegativeTTL */ 1);
+
+  struct timespec queryTime;
+  gettime(&queryTime);  // does not have to be accurate ("realTime") in tests
+
+  ComboAddress remote;
+  bool dnssecOK = false;
+
+  try {
+    DNSName name("truncated");
+    PacketBuffer query;
+    GenericDNSPacketWriter<PacketBuffer> pwQ(query, name, QType::A, QClass::IN, 0);
+    pwQ.getHeader()->rd = 1;
+
+    PacketBuffer response;
+    GenericDNSPacketWriter<PacketBuffer> pwR(response, name, QType::A, QClass::IN, 0);
+    pwR.getHeader()->rd = 1;
+    pwR.getHeader()->ra = 0;
+    pwR.getHeader()->qr = 1;
+    pwR.getHeader()->tc = 1;
+    pwR.getHeader()->rcode = RCode::NoError;
+    pwR.getHeader()->id = pwQ.getHeader()->id;
+    pwR.commit();
+    pwR.startRecord(name, QType::A, 7200, QClass::IN, DNSResourceRecord::ANSWER);
+    pwR.xfr32BitInt(0x01020304);
+    pwR.commit();
+
+    uint32_t key = 0;
+    boost::optional<Netmask> subnet;
+    DNSQuestion dq(&name, QType::A, QClass::IN, &remote, &remote, query, dnsdist::Protocol::DoUDP, &queryTime);
+    bool found = PC.get(dq, 0, &key, subnet, dnssecOK, receivedOverUDP);
+    BOOST_CHECK_EQUAL(found, false);
+    BOOST_CHECK(!subnet);
+
+    PC.insert(key, subnet, *(getFlagsFromDNSHeader(dq.getHeader())), dnssecOK, name, QType::A, QClass::IN, response, receivedOverUDP, RCode::NXDomain, boost::none);
+
+    bool allowTruncated = true;
+    found = PC.get(dq, pwR.getHeader()->id, &key, subnet, dnssecOK, receivedOverUDP, 0, true, allowTruncated);
+    BOOST_CHECK_EQUAL(found, true);
+    BOOST_CHECK(!subnet);
+
+    allowTruncated = false;
+    found = PC.get(dq, pwR.getHeader()->id, &key, subnet, dnssecOK, receivedOverUDP, 0, true, allowTruncated);
+    BOOST_CHECK_EQUAL(found, false);
+}
+  catch(const PDNSException& e) {
+    cerr<<"Had error: "<<e.reason<<endl;
+    throw;
+  }
+}
+
 static DNSDistPacketCache g_PC(500000);
 
 static void threadMangler(unsigned int offset)