From c2a58bfec29e7c23b27e533c4c11e50a3c4c9481 Mon Sep 17 00:00:00 2001 From: Miod Vallat Date: Wed, 10 Sep 2025 10:11:02 +0200 Subject: [PATCH] Be sure to blame the new record when two records conflict. Signed-off-by: Miod Vallat --- pdns/check-zone.cc | 19 +++++++++++++++---- pdns/check-zone.hh | 5 +++-- pdns/pdnsutil.cc | 10 +++++----- pdns/ws-auth.cc | 2 +- .../pdnsutil-zone-handling/expected_result | 2 +- .../expected_result.lmdb | 2 +- 6 files changed, 26 insertions(+), 14 deletions(-) diff --git a/pdns/check-zone.cc b/pdns/check-zone.cc index 6823879b9..d26f5a404 100644 --- a/pdns/check-zone.cc +++ b/pdns/check-zone.cc @@ -28,7 +28,7 @@ namespace Check { -void checkRRSet(vector& records, const ZoneName& zone, vector>& errors) +void checkRRSet(const vector& oldrrs, vector& allrrs, const ZoneName& zone, vector>& errors) { // QTypes that MUST NOT have multiple records of the same type in a given RRset. static const std::set onlyOneEntryTypes = {QType::CNAME, QType::DNAME, QType::SOA}; @@ -37,14 +37,14 @@ void checkRRSet(vector& records, const ZoneName& zone, vector // QTypes that are NOT allowed at apex. static const std::set 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& 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 415b7ee94..89b4577c5 100644 --- 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& records, const ZoneName& zone, vector>& errors); +void checkRRSet(const vector& oldrrs, vector& allrrs, const ZoneName& zone, vector>& errors); } // namespace Check diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index f0e74d6a8..56d36d9cb 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -2510,7 +2510,6 @@ static int addOrReplaceRecord(bool isAdd, const vector& 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& cmds) di.backend->startTransaction(zone, UnknownDomainID); + DNSResourceRecord oldrr; + vector 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 oldrrs; di.backend->lookup(QType(QType::ANY), rr.qname, static_cast(di.id)); while (di.backend->get(oldrr)) { oldrrs.push_back(oldrr); @@ -2554,12 +2554,12 @@ static int addOrReplaceRecord(bool isAdd, const vector& cmds) } } } - oldrrs.insert(oldrrs.end(), newrrs.begin(), newrrs.end()); - newrrs = std::move(oldrrs); + newrrs.insert(newrrs.end(), oldrrs.begin(), oldrrs.end()); } std::vector> 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 432768306..046c0a2a2 100644 --- a/pdns/ws-auth.cc +++ b/pdns/ws-auth.cc @@ -1661,7 +1661,7 @@ static bool checkNewRecords(HttpResponse* resp, vector& recor { std::vector> 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 93025fe5e..fb7955a4b 100644 --- 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 3c87ac01e..2034da891 100644 --- 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 -- 2.47.3