Merge pull request #14617 from omoerbeek/rec-dedup-recs

rec: dedup records

pdns/Makefile.am

@@ -949,6 +949,7 @@ speedtest_SOURCES = \
 	nsecrecords.cc \
 	qtype.cc \
 	rcpgenerator.cc rcpgenerator.hh \
+	shuffle.cc shuffle.hh \
 	sillyrecords.cc \
 	speedtest.cc \
 	statbag.cc \

pdns/dnsparser.hh

@@ -202,22 +202,28 @@ public:
   virtual std::string getZoneRepresentation(bool noDot=false) const = 0;
   virtual ~DNSRecordContent() = default;
   virtual void toPacket(DNSPacketWriter& pw) const = 0;
-  // returns the wire format of the content, possibly including compressed pointers pointing to the owner name (unless canonic or lowerCase are set)
-  string serialize(const DNSName& qname, bool canonic=false, bool lowerCase=false) const
+  // returns the wire format of the content or the full record, possibly including compressed pointers pointing to the owner name (unless canonic or lowerCase are set)
+  [[nodiscard]] string serialize(const DNSName& qname, bool canonic = false, bool lowerCase = false, bool full = false) const
   {
     vector<uint8_t> packet;
-    DNSPacketWriter pw(packet, g_rootdnsname, 1);
-    if(canonic)
-      pw.setCanonic(true);
+    DNSPacketWriter packetWriter(packet, g_rootdnsname, QType::A);
 
-    if(lowerCase)
-      pw.setLowercase(true);
+    if (canonic) {
+      packetWriter.setCanonic(true);
+    }
+    if (lowerCase) {
+      packetWriter.setLowercase(true);
+    }
 
-    pw.startRecord(qname, this->getType());
-    this->toPacket(pw);
+    packetWriter.startRecord(qname, getType());
+    toPacket(packetWriter);
 
     string record;
-    pw.getRecordPayload(record); // needs to be called before commit()
+    if (full) {
+      packetWriter.getWireFormatContent(record); // needs to be called before commit()
+    } else {
+      packetWriter.getRecordPayload(record); // needs to be called before commit()
+    }
     return record;
   }
 

pdns/dnswriter.cc

@@ -457,6 +457,12 @@ template <typename Container> void GenericDNSPacketWriter<Container>::getRecordP
   records.assign(d_content.begin() + d_sor, d_content.end());
 }
 
+// call __before commit__
+template <typename Container> void GenericDNSPacketWriter<Container>::getWireFormatContent(string& record)
+{
+  record.assign(d_content.begin() + d_rollbackmarker, d_content.end());
+}
+
 template <typename Container> uint32_t GenericDNSPacketWriter<Container>::size() const
 {
   return d_content.size();

pdns/dnswriter.hh

@@ -138,6 +138,7 @@ public:
 
   dnsheader* getHeader();
   void getRecordPayload(string& records); // call __before commit__
+  void getWireFormatContent(string& record); // call __before commit__
 
   void setCanonic(bool val)
   {

pdns/recursordist/Makefile.am

@@ -339,6 +339,7 @@ testrunner_SOURCES = \
 	secpoll.cc \
 	settings/cxxsupport.cc \
 	sholder.hh \
+	shuffle.cc shuffle.hh \
 	sillyrecords.cc \
 	sortlist.cc sortlist.hh \
 	sstuff.hh \
@@ -380,6 +381,7 @@ testrunner_SOURCES = \
 	test-secpoll_cc.cc \
 	test-settings.cc \
 	test-sholder_hh.cc \
+	test-shuffle_cc.cc \
 	test-signers.cc \
 	test-syncres_cc.cc \
 	test-syncres_cc.hh \

pdns/recursordist/meson.build

@@ -472,6 +472,7 @@ test_sources += files(
       src_dir / 'test-rpzloader_cc.cc',
       src_dir / 'test-secpoll_cc.cc',
       src_dir / 'test-settings.cc',
+      src_dir / 'test-shuffle_cc.cc',
       src_dir / 'test-signers.cc',
       src_dir / 'test-syncres_cc.cc',
       src_dir / 'test-syncres_cc.hh',

pdns/recursordist/pdns_recursor.cc

@@ -780,22 +780,7 @@ int getFakeAAAARecords(const DNSName& qname, ComboAddress prefix, vector<DNSReco
               ret.end());
   }
   else {
-    // Remove double SOA records
-    std::set<DNSName> seenSOAs;
-    ret.erase(std::remove_if(
-                ret.begin(),
-                ret.end(),
-                [&seenSOAs](DNSRecord& record) {
-                  if (record.d_type == QType::SOA) {
-                    if (seenSOAs.count(record.d_name) > 0) {
-                      // We've had this SOA before, remove it
-                      return true;
-                    }
-                    seenSOAs.insert(record.d_name);
-                  }
-                  return false;
-                }),
-              ret.end());
+    pdns::dedupRecords(ret);
   }
   t_Counters.at(rec::Counter::dns64prefixanswers)++;
   return rcode;
@@ -1527,6 +1512,12 @@ void startDoResolve(void* arg) // NOLINT(readability-function-cognitive-complexi
       }
 
       if (!ret.empty()) {
+#ifdef notyet
+        // As dedupping is relatively expensive do not dedup in general. We do have a few cases
+        // where we call dedup explicitly, e.g. when doing NAT64 or when adding NSEC records in
+        // doCNAMECacheCheck
+        pdns::dedupRecords(ret);
+#endif
         pdns::orderAndShuffle(ret, false);
         if (auto listToSort = luaconfsLocal->sortlist.getOrderCmp(comboWriter->d_source)) {
           stable_sort(ret.begin(), ret.end(), *listToSort);

pdns/recursordist/syncres.cc

@@ -39,6 +39,7 @@
 #include "dnsseckeeper.hh"
 #include "validate-recursor.hh"
 #include "rec-taskqueue.hh"
+#include "shuffle.hh"
 
 rec::GlobalCounters g_Counters;
 thread_local rec::TCounters t_Counters(g_Counters);
@@ -2759,6 +2760,7 @@ bool SyncRes::doCNAMECacheCheck(const DNSName& qname, const QType qtype, vector<
       // so you can't trust that a real lookup will have been made.
       res = doResolve(newTarget, qtype, ret, depth + 1, beenthere, cnameContext);
       LOG(prefix << qname << ": Updating validation state for response to " << qname << " from " << context.state << " with the state from the DNAME/CNAME quest: " << cnameContext.state << endl);
+      pdns::dedupRecords(ret); // multiple NSECS could have been added, #14120
       updateValidationState(qname, context.state, cnameContext.state, prefix);
 
       return true;
@@ -4478,6 +4480,12 @@ void SyncRes::sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, con
     }
     lwr.d_records = std::move(vec);
   }
+#ifdef notyet
+  // As dedupping is relatively expensive and having dup records not really hurts as far as we have seen, do not dedup.
+  if (auto count = pdns::dedupRecords(lwr.d_records); count > 0) {
+    LOG(prefix << qname << ": Removed " << count << " duplicate records from response received from " << auth << endl);
+  }
+#endif
 }
 
 void SyncRes::rememberParentSetIfNeeded(const DNSName& domain, const vector<DNSRecord>& newRecords, unsigned int depth, const string& prefix)

pdns/recursordist/test-shuffle_cc.cc

@@ -0,0 +1,58 @@
+/*
+ * This file is part of PowerDNS or dnsdist.
+ * Copyright -- PowerDNS.COM B.V. and its contributors
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * In addition, for the avoidance of any doubt, permission is granted to
+ * link this program with OpenSSL and to (re)distribute the binaries
+ * produced as the result of such linking.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef BOOST_TEST_DYN_LINK
+#define BOOST_TEST_DYN_LINK
+#endif
+
+#define BOOST_TEST_NO_MAIN
+
+#include "config.h"
+#include <boost/test/unit_test.hpp>
+
+#include "shuffle.hh"
+#include "test-common.hh"
+
+BOOST_AUTO_TEST_SUITE(shuffle_cc)
+
+BOOST_AUTO_TEST_CASE(test_simple)
+{
+  std::vector<DNSRecord> list;
+  auto* address = &list;
+  addRecordToList(list, DNSName("foo"), QType::A, "1.2.3.4");
+  addRecordToList(list, DNSName("foo2"), QType::A, "1.2.3.4");
+  auto dups = pdns::dedupRecords(list);
+  BOOST_CHECK_EQUAL(dups, 0U);
+  BOOST_CHECK_EQUAL(list.size(), 2U);
+  addRecordToList(list, DNSName("foo"), QType::A, "1.2.3.4");
+  dups = pdns::dedupRecords(list);
+  BOOST_CHECK_EQUAL(dups, 1U);
+  BOOST_CHECK_EQUAL(list.size(), 2U);
+  addRecordToList(list, DNSName("Foo"), QType::A, "1.2.3.4");
+  addRecordToList(list, DNSName("FoO"), QType::A, "1.2.3.4", DNSResourceRecord::ADDITIONAL, 999);
+  dups = pdns::dedupRecords(list);
+  BOOST_CHECK_EQUAL(dups, 2U);
+  BOOST_CHECK_EQUAL(list.size(), 2U);
+  BOOST_CHECK_EQUAL(address, &list);
+}
+
+BOOST_AUTO_TEST_SUITE_END()

pdns/recursordist/test-syncres_cc.cc

@@ -326,7 +326,7 @@ void computeRRSIG(const DNSSECPrivateKey& dpk, const DNSName& signer, const DNSN
 
 typedef std::unordered_map<DNSName, std::pair<DNSSECPrivateKey, DSRecordContent>> testkeysset_t;
 
-bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& records, const DNSName& signer, uint32_t sigValidity, bool broken, boost::optional<uint8_t> algo, boost::optional<DNSName> wildcard, boost::optional<time_t> now)
+bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& records, const DNSName& signer, uint32_t sigValidity, std::variant<bool, int> broken, boost::optional<uint8_t> algo, boost::optional<DNSName> wildcard, boost::optional<time_t> now)
 {
   if (records.empty()) {
     return false;
@@ -368,9 +368,12 @@ bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& records, const
 
   RRSIGRecordContent rrc;
   computeRRSIG(it->second.first, signer, wildcard ? *wildcard : name, type, ttl, sigValidity, rrc, recordcontents, algo, boost::none, now);
-  if (broken) {
+  if (auto* bval = std::get_if<bool>(&broken); bval != nullptr && *bval) {
     rrc.d_signature[0] ^= 42;
   }
+  else if (auto* ival = std::get_if<int>(&broken)) {
+    rrc.d_signature[0] ^= *ival; // NOLINT(*-narrowing-conversions)
+  }
 
   DNSRecord rec;
   rec.d_type = QType::RRSIG;

pdns/recursordist/test-syncres_cc.hh

@@ -48,7 +48,7 @@ void computeRRSIG(const DNSSECPrivateKey& dpk, const DNSName& signer, const DNSN
 
 typedef std::unordered_map<DNSName, std::pair<DNSSECPrivateKey, DSRecordContent>> testkeysset_t;
 
-bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& records, const DNSName& signer, uint32_t sigValidity, bool broken = false, boost::optional<uint8_t> algo = boost::none, boost::optional<DNSName> wildcard = boost::none, boost::optional<time_t> now = boost::none);
+bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& records, const DNSName& signer, uint32_t sigValidity, std::variant<bool, int> broken = false, boost::optional<uint8_t> algo = boost::none, boost::optional<DNSName> wildcard = boost::none, boost::optional<time_t> now = boost::none);
 
 void addDNSKEY(const testkeysset_t& keys, const DNSName& signer, uint32_t ttl, std::vector<DNSRecord>& records);
 

pdns/recursordist/test-syncres_cc4.cc

@@ -1729,9 +1729,9 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_too_many_sigs)
         addRecordToLW(res, domain, QType::NS, std::string(addr), DNSResourceRecord::ANSWER, 3600);
       }
 
-      addRRSIG(keys, res->d_records, domain, 300, true, boost::none, boost::none, fixedNow);
-      addRRSIG(keys, res->d_records, domain, 300, true, boost::none, boost::none, fixedNow);
-      addRRSIG(keys, res->d_records, domain, 300, false, boost::none, boost::none, fixedNow);
+      addRRSIG(keys, res->d_records, domain, 300, 1, boost::none, boost::none, fixedNow);
+      addRRSIG(keys, res->d_records, domain, 300, 2, boost::none, boost::none, fixedNow);
+      addRRSIG(keys, res->d_records, domain, 300, 0, boost::none, boost::none, fixedNow);
 
       addRecordToLW(res, "a.root-servers.net.", QType::A, "198.41.0.4", DNSResourceRecord::ADDITIONAL, 3600);
       addRecordToLW(res, "a.root-servers.net.", QType::AAAA, "2001:503:ba3e::2:30", DNSResourceRecord::ADDITIONAL, 3600);

pdns/recursordist/test-syncres_cc5.cc

@@ -1575,6 +1575,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec3_nodata_nowildcard_duplicated_n
         addRecordToLW(res, "a.gtld-servers.com.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
         return LWResult::Result::Success;
       }
+      // The code below introduces duplicate NSEC3 records
       if (address == ComboAddress("192.0.2.1:53")) {
         setLWResult(res, 0, true, false, true);
         /* no data */
@@ -1603,7 +1604,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec3_nodata_nowildcard_duplicated_n
   int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Secure);
-  /* because we pass along the duplicated NSEC3 */
+  /* the duplicated NSEC3 have not been dedupped */
   BOOST_REQUIRE_EQUAL(ret.size(), 9U);
   BOOST_CHECK_EQUAL(queriesCount, 4U);
 
@@ -1612,7 +1613,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec3_nodata_nowildcard_duplicated_n
   res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Secure);
-  /* because we pass along the duplicated NSEC3 */
+  /* the duplicated NSEC3 have not been dedupped */
   BOOST_REQUIRE_EQUAL(ret.size(), 9U);
   BOOST_CHECK_EQUAL(queriesCount, 4U);
 }

pdns/shuffle.cc

@@ -140,3 +140,49 @@ void pdns::orderAndShuffle(vector<DNSRecord>& rrs, bool includingAdditionals)
   });
   shuffle(rrs, includingAdditionals);
 }
+
+unsigned int pdns::dedupRecords(vector<DNSRecord>& rrs)
+{
+  // This function tries to avoid unnecessary work
+  // First a vector with zero or one element does not need dedupping
+  if (rrs.size() <= 1) {
+    return 0;
+  }
+
+  // If we have a larger vector, first check if we actually have duplicates.
+  // We assume the most common case is: no
+  std::unordered_set<std::string> seen;
+  std::vector<bool> dups(rrs.size(), false);
+
+  unsigned int counter = 0;
+  unsigned int numDups = 0;
+
+  seen.reserve(rrs.size());
+  for (const auto& rec : rrs) {
+    auto key = rec.getContent()->serialize(rec.d_name, true, true, true);
+    // This ignores class, ttl and place by using constants for those
+    if (!seen.emplace(std::move(key)).second) {
+      dups[counter] = true;
+      numDups++;
+    }
+    ++counter;
+  }
+
+  if (numDups == 0) {
+    // Original is fine as-is.
+    return 0;
+  }
+
+  // We avoid calling erase, as it calls a lot of move constructors. This can hurt, especially if
+  // you call it on a large vector multiple times.
+  // So we just take the elements that are unique
+  std::vector<DNSRecord> ret;
+  ret.reserve(rrs.size() - numDups);
+  for (counter = 0; counter < rrs.size(); ++counter) {
+    if (!dups[counter]) {
+      ret.emplace_back(std::move(rrs[counter]));
+    }
+  }
+  rrs = std::move(ret);
+  return numDups;
+}

pdns/shuffle.hh

@@ -29,4 +29,5 @@ namespace pdns
 {
 void shuffle(std::vector<DNSZoneRecord>& rrs);
 void orderAndShuffle(std::vector<DNSRecord>& rrs, bool includingAdditionals);
+unsigned int dedupRecords(std::vector<DNSRecord>& rrs);
 }