From: Remi Gacogne Date: Tue, 24 Aug 2021 09:23:54 +0000 (+0200) Subject: dnsdist: Cache based on the DNS flags of the query after applying the rules X-Git-Tag: dnsdist-1.7.0-alpha1~51^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F10664%2Fhead;p=thirdparty%2Fpdns.git dnsdist: Cache based on the DNS flags of the query after applying the rules The tentative fix in dbadb4d272a3317407e6bc934f55c2d41a87c0ac actually introduced an issue, because the backend might not perfectly echo the RD and CD flags as they were in the query. We can't use the "original" (before applying rules) flags either, so we need to store the flags as they were sent to the backend to be able to correctly store them in the cache. --- diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc index 41af3461f3..cf96c599ad 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -292,11 +292,6 @@ static void restoreFlags(struct dnsheader* dh, uint16_t origFlags) *flags |= origFlags; } -static uint16_t getRDAndCDFlagsFromDNSHeader(const struct dnsheader* dh) -{ - return static_cast(dh->rd) << FLAGS_RD_OFFSET | static_cast(dh->cd) << FLAGS_CD_OFFSET; -} - static bool fixUpQueryTurnedResponse(DNSQuestion& dq, const uint16_t origFlags) { restoreFlags(dq.getHeader(), origFlags); @@ -450,9 +445,6 @@ bool processResponse(PacketBuffer& response, LocalStateHolderinsert(zeroScope ? dr.cacheKeyNoECS : dr.cacheKey, zeroScope ? boost::none : dr.subnet, cacheFlags, dr.dnssecOK, *dr.qname, dr.qtype, dr.qclass, response, receivedOverUDP, dr.getHeader()->rcode, dr.tempFailureTTL); + dr.packetCache->insert(zeroScope ? dr.cacheKeyNoECS : dr.cacheKey, zeroScope ? boost::none : dr.subnet, dr.cacheFlags, dr.dnssecOK, *dr.qname, dr.qtype, dr.qclass, response, receivedOverUDP, dr.getHeader()->rcode, dr.tempFailureTTL); } #ifdef HAVE_DNSCRYPT @@ -1273,6 +1265,9 @@ ProcessQueryResult processQuery(DNSQuestion& dq, ClientState& cs, LocalHolders& return ProcessQueryResult::Drop; } + /* save the DNS flags as sent to the backend so we can cache the answer with the right flags later */ + dq.cacheFlags = *getFlagsFromDNSHeader(dq.getHeader()); + if (dq.addXPF && selectedBackend->xpfRRCode != 0) { addXPF(dq, selectedBackend->xpfRRCode); } diff --git a/pdns/dnsdist.hh b/pdns/dnsdist.hh index 30ef1ad0ce..801f35f55f 100644 --- a/pdns/dnsdist.hh +++ b/pdns/dnsdist.hh @@ -161,6 +161,7 @@ public: const uint16_t qclass; uint16_t ecsPrefixLength; uint16_t origFlags; + uint16_t cacheFlags; /* DNS flags as sent to the backend */ const Protocol protocol; uint8_t ednsRCode{0}; bool skipCache{false}; @@ -582,7 +583,7 @@ struct IDState { IDState(): sentTime(true), tempFailureTTL(boost::none) { origDest.sin4.sin_family = 0;} IDState(const IDState& orig) = delete; - IDState(IDState&& rhs): subnet(rhs.subnet), origRemote(rhs.origRemote), origDest(rhs.origDest), hopRemote(rhs.hopRemote), hopLocal(rhs.hopLocal), qname(std::move(rhs.qname)), sentTime(rhs.sentTime), dnsCryptQuery(std::move(rhs.dnsCryptQuery)), packetCache(std::move(rhs.packetCache)), qTag(std::move(rhs.qTag)), tempFailureTTL(rhs.tempFailureTTL), cs(rhs.cs), du(std::move(rhs.du)), cacheKey(rhs.cacheKey), cacheKeyNoECS(rhs.cacheKeyNoECS), origFD(rhs.origFD), delayMsec(rhs.delayMsec), qtype(rhs.qtype), qclass(rhs.qclass), origID(rhs.origID), origFlags(rhs.origFlags), protocol(rhs.protocol), ednsAdded(rhs.ednsAdded), ecsAdded(rhs.ecsAdded), skipCache(rhs.skipCache), destHarvested(rhs.destHarvested), dnssecOK(rhs.dnssecOK), useZeroScope(rhs.useZeroScope) + IDState(IDState&& rhs): subnet(rhs.subnet), origRemote(rhs.origRemote), origDest(rhs.origDest), hopRemote(rhs.hopRemote), hopLocal(rhs.hopLocal), qname(std::move(rhs.qname)), sentTime(rhs.sentTime), dnsCryptQuery(std::move(rhs.dnsCryptQuery)), packetCache(std::move(rhs.packetCache)), qTag(std::move(rhs.qTag)), tempFailureTTL(rhs.tempFailureTTL), cs(rhs.cs), du(std::move(rhs.du)), cacheKey(rhs.cacheKey), cacheKeyNoECS(rhs.cacheKeyNoECS), origFD(rhs.origFD), delayMsec(rhs.delayMsec), qtype(rhs.qtype), qclass(rhs.qclass), origID(rhs.origID), origFlags(rhs.origFlags), cacheFlags(rhs.cacheFlags), protocol(rhs.protocol), ednsAdded(rhs.ednsAdded), ecsAdded(rhs.ecsAdded), skipCache(rhs.skipCache), destHarvested(rhs.destHarvested), dnssecOK(rhs.dnssecOK), useZeroScope(rhs.useZeroScope) { if (rhs.isInUse()) { throw std::runtime_error("Trying to move an in-use IDState"); @@ -632,6 +633,7 @@ struct IDState qclass = rhs.qclass; origID = rhs.origID; origFlags = rhs.origFlags; + cacheFlags = rhs.cacheFlags; protocol = rhs.protocol; uniqueId = std::move(rhs.uniqueId); ednsAdded = rhs.ednsAdded; @@ -738,6 +740,7 @@ struct IDState uint16_t qclass{0}; // 2 uint16_t origID{0}; // 2 uint16_t origFlags{0}; // 2 + uint16_t cacheFlags{0}; // DNS flags as sent to the backend // 2 DNSQuestion::Protocol protocol; // 1 boost::optional uniqueId{boost::none}; // 17 (placed here to reduce the space lost to padding) bool ednsAdded{false}; diff --git a/pdns/dnsdistdist/dnsdist-idstate.cc b/pdns/dnsdistdist/dnsdist-idstate.cc index a88ab219d9..f34e79de58 100644 --- a/pdns/dnsdistdist/dnsdist-idstate.cc +++ b/pdns/dnsdistdist/dnsdist-idstate.cc @@ -5,6 +5,7 @@ DNSResponse makeDNSResponseFromIDState(IDState& ids, PacketBuffer& data) { DNSResponse dr(&ids.qname, ids.qtype, ids.qclass, &ids.origDest, &ids.origRemote, data, ids.protocol, &ids.sentTime.d_start); dr.origFlags = ids.origFlags; + dr.cacheFlags = ids.cacheFlags; dr.ecsAdded = ids.ecsAdded; dr.ednsAdded = ids.ednsAdded; dr.useZeroScope = ids.useZeroScope; @@ -41,6 +42,7 @@ void setIDStateFromDNSQuestion(IDState& ids, DNSQuestion& dq, DNSName&& qname) ids.delayMsec = dq.delayMsec; ids.tempFailureTTL = dq.tempFailureTTL; ids.origFlags = dq.origFlags; + ids.cacheFlags = dq.cacheFlags; ids.cacheKey = dq.cacheKey; ids.cacheKeyNoECS = dq.cacheKeyNoECS; ids.subnet = dq.subnet; diff --git a/regression-tests.dnsdist/test_Caching.py b/regression-tests.dnsdist/test_Caching.py index f5a6d46838..ac4061afe8 100644 --- a/regression-tests.dnsdist/test_Caching.py +++ b/regression-tests.dnsdist/test_Caching.py @@ -2567,18 +2567,19 @@ class TestCachingAlteredHeader(DNSDistTest): '192.0.2.1') expectedResponse.answer.append(rrset) - (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response) - self.assertTrue(receivedQuery) - self.assertTrue(receivedResponse) - receivedQuery.id = expectedQuery.id - self.assertEqual(expectedQuery, receivedQuery) - self.assertEqual(receivedResponse, expectedResponse) + for method in ("sendUDPQuery", "sendTCPQuery"): + sender = getattr(self, method) + (receivedQuery, receivedResponse) = sender(query, response) + self.assertTrue(receivedQuery) + self.assertTrue(receivedResponse) + receivedQuery.id = expectedQuery.id + self.assertEqual(expectedQuery, receivedQuery) + self.assertEqual(receivedResponse, expectedResponse) # next query should hit the cache - (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response=None, useQueue=False) - self.assertFalse(receivedQuery) - self.assertTrue(receivedResponse) - self.assertEqual(receivedResponse, expectedResponse) + for method in ("sendUDPQuery", "sendTCPQuery"): + sender = getattr(self, method) + (receivedQuery, receivedResponse) = sender(query, response=None, useQueue=False) # same query with RD=0, should hit the cache as well query.flags &= ~dns.flags.RD @@ -2589,7 +2590,78 @@ class TestCachingAlteredHeader(DNSDistTest): dns.rdatatype.A, '192.0.2.1') expectedResponse.answer.append(rrset) - (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response=None, useQueue=False) - self.assertFalse(receivedQuery) - self.assertTrue(receivedResponse) - self.assertEqual(receivedResponse, expectedResponse) + for method in ("sendUDPQuery", "sendTCPQuery"): + sender = getattr(self, method) + (receivedQuery, receivedResponse) = sender(query, response=None, useQueue=False) + self.assertFalse(receivedQuery) + self.assertTrue(receivedResponse) + self.assertEqual(receivedResponse, expectedResponse) + +class TestCachingBackendSettingRD(DNSDistTest): + + _config_template = """ + pc = newPacketCache(100) + getPool(""):setCache(pc) + newServer{address="127.0.0.1:%d"} + """ + + def testCachingBackendSetRD(self): + """ + Cache: The backend sets RD=1 in the response even if the query had RD=0 + """ + name = 'backend-sets-rd.tests.powerdns.com.' + query = dns.message.make_query(name, 'A', 'IN') + query.flags &= ~dns.flags.RD + expectedQuery = dns.message.make_query(name, 'A', 'IN') + expectedQuery.flags &= ~dns.flags.RD + response = dns.message.make_response(query) + response.flags |= dns.flags.RD + rrset = dns.rrset.from_text(name, + 60, + dns.rdataclass.IN, + dns.rdatatype.A, + '192.0.2.1') + response.answer.append(rrset) + + expectedResponse = dns.message.make_response(query) + expectedResponse.flags &= ~dns.flags.RD + rrset = dns.rrset.from_text(name, + 60, + dns.rdataclass.IN, + dns.rdatatype.A, + '192.0.2.1') + expectedResponse.answer.append(rrset) + + for method in ("sendUDPQuery", "sendTCPQuery"): + sender = getattr(self, method) + (receivedQuery, receivedResponse) = sender(query, response) + self.assertTrue(receivedQuery) + self.assertTrue(receivedResponse) + receivedQuery.id = expectedQuery.id + self.assertEqual(expectedQuery, receivedQuery) + self.assertEqual(receivedResponse, expectedResponse) + + # exact same query should be cached + for method in ("sendUDPQuery", "sendTCPQuery"): + sender = getattr(self, method) + (receivedQuery, receivedResponse) = sender(query, response=None, useQueue=False) + self.assertFalse(receivedQuery) + self.assertTrue(receivedResponse) + self.assertEqual(receivedResponse, expectedResponse) + + # same query with RD=1, should NOT hit the cache + query.flags |= dns.flags.RD + expectedResponse = dns.message.make_response(query) + rrset = dns.rrset.from_text(name, + 60, + dns.rdataclass.IN, + dns.rdatatype.A, + '192.0.2.1') + expectedResponse.answer.append(rrset) + + for method in ("sendUDPQuery", "sendTCPQuery"): + sender = getattr(self, method) + (receivedQuery, receivedResponse) = sender(query, response) + self.assertTrue(receivedQuery) + self.assertTrue(receivedResponse) + self.assertEqual(receivedResponse, expectedResponse)