Be sure to blame the new record when two records conflict.

Signed-off-by: Miod Vallat <miod.vallat@powerdns.com>

diff --git a/pdns/check-zone.cc b/pdns/check-zone.cc
index 6823879b950a130827113364e89798fbbe31739b..d26f5a404d941052c71e6456ed2909923789d497 100644 (file)
--- a/pdns/check-zone.cc
+++ b/pdns/check-zone.cc
@@ -28,7 +28,7 @@
 namespace Check
 {
 
-void checkRRSet(vector<DNSResourceRecord>& records, const ZoneName& zone, vector<pair<DNSResourceRecord, string>>& errors)
+void checkRRSet(const vector<DNSResourceRecord>& oldrrs, vector<DNSResourceRecord>& allrrs, const ZoneName& zone, vector<pair<DNSResourceRecord, string>>& errors)
 {
   // QTypes that MUST NOT have multiple records of the same type in a given RRset.
   static const std::set<uint16_t> onlyOneEntryTypes = {QType::CNAME, QType::DNAME, QType::SOA};
@@ -37,14 +37,14 @@ void checkRRSet(vector<DNSResourceRecord>& records, const ZoneName& zone, vector
   // QTypes that are NOT allowed at apex.
   static const std::set<uint16_t> nonApexTypes = {QType::DS};
 
-  sort(records.begin(), records.end(),
+  sort(allrrs.begin(), allrrs.end(),
        [](const DNSResourceRecord& rec_a, const DNSResourceRecord& rec_b) -> bool {
          /* we need _strict_ weak ordering */
          return std::tie(rec_a.qname, rec_a.qtype, rec_a.content) < std::tie(rec_b.qname, rec_b.qtype, rec_b.content);
        });
 
   DNSResourceRecord previous;
-  for (const auto& rec : records) {
+  for (const auto& rec : allrrs) {
     if (previous.qname == rec.qname) {
       if (previous.qtype == rec.qtype) {
         if (onlyOneEntryTypes.count(rec.qtype.getCode()) != 0) {
@@ -57,7 +57,18 @@ void checkRRSet(vector<DNSResourceRecord>& records, const ZoneName& zone, vector
       else {
         if (QType::exclusiveEntryTypes.count(rec.qtype.getCode()) != 0
             || QType::exclusiveEntryTypes.count(previous.qtype.getCode()) != 0) {
-          errors.emplace_back(std::make_pair(rec, std::string{"conflicts with existing "} + previous.qtype.toString() + " RRset of the same name"));
+          // The `rec' record can't be added because of `previous'. However
+          // `rec' might be one of the existing records, and `previous' the
+          // added one. Or they might both be new records.
+          // We thus check if `rec' appears in the existing records in
+          // order to decide which record to blame in order to make the error
+          // message as less confusing as possible.
+          if (std::find(oldrrs.begin(), oldrrs.end(), rec) != oldrrs.end()) {
+            errors.emplace_back(std::make_pair(previous, std::string{"conflicts with existing "} + rec.qtype.toString() + " RRset of the same name"));
+          }
+          else {
+            errors.emplace_back(std::make_pair(rec, std::string{"conflicts with existing "} + previous.qtype.toString() + " RRset of the same name"));
+          }
         }
       }
     }

diff --git a/pdns/check-zone.hh b/pdns/check-zone.hh
index 415b7ee943431427e3666c7433c7f0450b4463d0..89b4577c5eead6384452e8886a3f8d49a8f44b3f 100644 (file)
--- a/pdns/check-zone.hh
+++ b/pdns/check-zone.hh
@@ -23,13 +23,14 @@
 namespace Check
 {
 
-// Returns the list of errors found for records which violate RRset constraints.
+// Returns the list of errors found for new records which violate RRset
+// constraints.
 // NOTE: sorts records in-place.
 //
 //  Constraints being checked:
 //   *) no exact duplicates
 //   *) no duplicates for QTypes that can only be present once per RRset
 //   *) hostnames are hostnames
-void checkRRSet(vector<DNSResourceRecord>& records, const ZoneName& zone, vector<pair<DNSResourceRecord, string>>& errors);
+void checkRRSet(const vector<DNSResourceRecord>& oldrrs, vector<DNSResourceRecord>& allrrs, const ZoneName& zone, vector<pair<DNSResourceRecord, string>>& errors);
 
 } // namespace Check

diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc
index f0e74d6a8d7b44f1e575c59f4d4ddc2fb7aba433..56d36d9cbbce28d8aea00efd6929d52e626ae5ea 100644 (file)
--- a/pdns/pdnsutil.cc
+++ b/pdns/pdnsutil.cc
@@ -2510,7 +2510,6 @@ static int addOrReplaceRecord(bool isAdd, const vector<string>& cmds)
   rr.auth = true;
   rr.domain_id = di.id;
   rr.qname = name;
-  DNSResourceRecord oldrr;
 
   unsigned int contentStart = 3;
   if(cmds.size() > 4) {
@@ -2540,10 +2539,11 @@ static int addOrReplaceRecord(bool isAdd, const vector<string>& cmds)
 
   di.backend->startTransaction(zone, UnknownDomainID);
 
+  DNSResourceRecord oldrr;
+  vector<DNSResourceRecord> oldrrs;
   if (isAdd) {
     // the 'add' case; preserve existing records, making sure to discard
     // would-be new records which contents are identical to the existing ones.
-    vector<DNSResourceRecord> oldrrs;
     di.backend->lookup(QType(QType::ANY), rr.qname, static_cast<int>(di.id));
     while (di.backend->get(oldrr)) {
       oldrrs.push_back(oldrr);
@@ -2554,12 +2554,12 @@ static int addOrReplaceRecord(bool isAdd, const vector<string>& cmds)
         }
       }
     }
-    oldrrs.insert(oldrrs.end(), newrrs.begin(), newrrs.end());
-    newrrs = std::move(oldrrs);
+    newrrs.insert(newrrs.end(), oldrrs.begin(), oldrrs.end());
   }
 
   std::vector<std::pair<DNSResourceRecord, string>> errors;
-  Check::checkRRSet(newrrs, zone, errors);
+  Check::checkRRSet(oldrrs, newrrs, zone, errors);
+  oldrrs.clear(); // no longer needed
   if (!errors.empty()) {
     for (const auto& error : errors) {
       const auto [rec, why] = error;

diff --git a/pdns/ws-auth.cc b/pdns/ws-auth.cc
index 432768306927434aa33d134a1bdbab9c2a0191c5..046c0a2a2a20c3e8a379b67d9534f5a672a890dd 100644 (file)
--- a/pdns/ws-auth.cc
+++ b/pdns/ws-auth.cc
@@ -1661,7 +1661,7 @@ static bool checkNewRecords(HttpResponse* resp, vector<DNSResourceRecord>& recor
 {
   std::vector<std::pair<DNSResourceRecord, string>> errors;
 
-  Check::checkRRSet(records, zone, errors);
+  Check::checkRRSet({}, records, zone, errors);
   if (errors.empty()) {
     return true;
   }

diff --git a/regression-tests/tests/pdnsutil-zone-handling/expected_result b/regression-tests/tests/pdnsutil-zone-handling/expected_result
index 93025fe5ead7161bbc9c5a65f3bdb7c539465352..fb7955a4b4039d3ee54489d313cf0aada4c38a86 100644 (file)
--- a/regression-tests/tests/pdnsutil-zone-handling/expected_result
+++ b/regression-tests/tests/pdnsutil-zone-handling/expected_result
@@ -5,7 +5,7 @@ host.bug.less. 3600 IN A 127.0.0.1
 Ignoring duplicate record content "127.0.0.2"
 New rrset:
 host2.bug.less. 3600 IN A 127.0.0.2
-RRset cname.bug.less. IN CNAME: conflicts with existing A RRset of the same name
+RRset cname.bug.less. IN A: conflicts with existing CNAME RRset of the same name
 RRset host.bug.less. IN CNAME: conflicts with existing A RRset of the same name
 New rrset:
 host2.bug.less. 3600 IN A 127.0.0.2

diff --git a/regression-tests/tests/pdnsutil-zone-handling/expected_result.lmdb b/regression-tests/tests/pdnsutil-zone-handling/expected_result.lmdb
index 3c87ac01efa25615f53a459aeca14b9656780453..2034da891dbd7de181d8dbfc7a08a65d480ed0a3 100644 (file)
--- a/regression-tests/tests/pdnsutil-zone-handling/expected_result.lmdb
+++ b/regression-tests/tests/pdnsutil-zone-handling/expected_result.lmdb
@@ -5,7 +5,7 @@ host.bug.less. 3600 IN A 127.0.0.1
 Ignoring duplicate record content "127.0.0.2"
 New rrset:
 host2.bug.less. 3600 IN A 127.0.0.2
-RRset cname.bug.less. IN CNAME: conflicts with existing A RRset of the same name
+RRset cname.bug.less. IN A: conflicts with existing CNAME RRset of the same name
 RRset host.bug.less. IN CNAME: conflicts with existing A RRset of the same name
 New rrset:
 host2.bug.less. 3600 IN A 127.0.0.2