From: Miod Vallat Date: Thu, 4 Dec 2025 13:26:29 +0000 (+0100) Subject: Allow for multiple extend/prune in the same request as long as different RRsets. X-Git-Tag: rec-5.4.0-alpha1~21^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9017ef0b5b71214202181662af6b2822d792b8ef;p=thirdparty%2Fpdns.git Allow for multiple extend/prune in the same request as long as different RRsets. Signed-off-by: Miod Vallat --- diff --git a/pdns/ws-auth.cc b/pdns/ws-auth.cc index 0d93f1774a..0ae6e2d6e9 100644 --- a/pdns/ws-auth.cc +++ b/pdns/ws-auth.cc @@ -2476,7 +2476,7 @@ static void replaceZoneRecords(const DomainInfo& domainInfo, const ZoneName& zon // Check that two changetype values are compatible with each other. // Throws if they aren't. -static void checkChangetypeCompatibility(changeType lastOperationType, changeType thisOperationType, const std::string thisChangetype) +static void checkChangetypeCompatibility(changeType lastOperationType, changeType thisOperationType) { // DELETE and REPLACE operations are not compatible with PRUNE and // EXTEND operations. @@ -2485,11 +2485,6 @@ static void checkChangetypeCompatibility(changeType lastOperationType, changeTyp if (lastOperationOperatedOnRRsets ^ thisOperationOperatesOnRRsets) { throw ApiException("Mixing RRset operations with single-record operations is not allowed"); } - // Moreover, we currently only allow a single rrset for PRUNE and - // EXTEND. - if (thisOperationType == PRUNE || thisOperationType == EXTEND) { - throw ApiException("Only one rrset may be provided for " + thisChangetype + " changetype"); - } } // Parse the record name and type from a Json patch record. @@ -2503,13 +2498,22 @@ static void parseRecordNameAndType(const Json& rrset, DNSName& qname, QType& qty } } +// The return value of the apply* functions below +enum applyResult +{ + SUCCESS, // successful and changes performed + NOP, // successful but no changes needed + ABORT // failed horribly, don't process anything further +}; + // Apply a DELETE changetype. -static void applyDelete(const DomainInfo& domainInfo, DNSName& qname, QType& qtype) +static applyResult applyDelete(const DomainInfo& domainInfo, DNSName& qname, QType& qtype) { // Delete all matching qname/qtype RRs (and implicitly, comments). if (!domainInfo.backend->replaceRRSet(domainInfo.id, qname, qtype, {})) { throw ApiException("Hosting backend does not support editing records."); } + return SUCCESS; } // Struct gathering the SOA edition details, so as not to pass billions of @@ -2522,7 +2526,7 @@ struct soaEditSettings }; // Apply a REPLACE changetype. -static bool applyReplace(const DomainInfo& domainInfo, const ZoneName& zonename, const Json& rrset, DNSName& qname, QType& qtype, bool allowUnderscores, soaEditSettings& soa, HttpResponse* resp) +static applyResult applyReplace(const DomainInfo& domainInfo, const ZoneName& zonename, const Json& rrset, DNSName& qname, QType& qtype, bool allowUnderscores, soaEditSettings& soa, HttpResponse* resp) { bool replace_records = rrset["records"].is_array(); bool replace_comments = rrset["comments"].is_array(); @@ -2548,7 +2552,7 @@ static bool applyReplace(const DomainInfo& domainInfo, const ZoneName& zonename, } if (!checkNewRecords(resp, new_records, zonename, allowUnderscores)) { // Proper error response has been setup, no need to do anything further. - return false; + return ABORT; } } @@ -2572,10 +2576,10 @@ static bool applyReplace(const DomainInfo& domainInfo, const ZoneName& zonename, throw ApiException("Hosting backend does not support editing comments."); } } - return true; + return SUCCESS; } -static bool applyPruneOrExtend(const DomainInfo& domainInfo, const ZoneName& zonename, const Json& container, DNSName& qname, QType& qtype, bool allowUnderscores, soaEditSettings& soa, HttpResponse* resp, changeType operationType) +static applyResult applyPruneOrExtend(const DomainInfo& domainInfo, const ZoneName& zonename, const Json& container, DNSName& qname, QType& qtype, bool allowUnderscores, soaEditSettings& soa, HttpResponse* resp, changeType operationType) { if (!container["records"].is_array()) { throw ApiException("No record provided for PRUNE or EXTEND operation"); @@ -2597,7 +2601,7 @@ static bool applyPruneOrExtend(const DomainInfo& domainInfo, const ZoneName& zon if (!checkNewRecords(resp, new_records, zonename, allowUnderscores)) { // Proper error response has been setup, no need to do anything further. - return false; + return ABORT; } // Fetch the existing RRSet @@ -2625,29 +2629,22 @@ static bool applyPruneOrExtend(const DomainInfo& domainInfo, const ZoneName& zon rrset.emplace_back(new_record); } bool submitChanges = (operationType == EXTEND && !seenRecord) || (operationType == PRUNE && seenRecord); - if (submitChanges) { - if (!domainInfo.backend->replaceRRSet(domainInfo.id, qname, qtype, rrset)) { - throw ApiException("Hosting backend does not support editing records."); - } + if (!submitChanges) { + return NOP; } - else { - resp->body = ""; - resp->status = 204; // No Content, but indicate success - // This will force our caller to abort the transaction and return quickly, - // without increasing the zone serial number and flushing caches. - // This is safe to do as we do not allow more than one PRUNE or EXTEND - // operation, so there is no further zone changes to process. - return false; + if (!domainInfo.backend->replaceRRSet(domainInfo.id, qname, qtype, rrset)) { + throw ApiException("Hosting backend does not support editing records."); } } catch (const JsonException& e) { throw ApiException("Submitted record is invalid: " + string(e.what())); } - return true; + return SUCCESS; } static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInfo& domainInfo, const vector& rrsets, HttpResponse* resp) { + bool madeAnyChanges{false}; domainInfo.backend->startTransaction(zonename); try { soaEditSettings soa; @@ -2665,17 +2662,29 @@ static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInf firstPass = false; } else { - checkChangetypeCompatibility(lastOperationType, operationType, changetype); + checkChangetypeCompatibility(lastOperationType, operationType); } lastOperationType = operationType; // for next pass DNSName qname; QType qtype; parseRecordNameAndType(rrset, qname, qtype); - if (seen.count({qname, qtype, operationType}) != 0) { - throw ApiException("Duplicate RRset " + qname.toString() + " IN " + qtype.toString() + " with changetype: " + changetype); + // Since we need to fetch RRset from the backend for prune and extend + // operations, we risk operating on stale data if we allow more than + // one of these for a given RRset (even processing two prune requests + // in a row could result in the action of the first one being lost). + // So for the sake of the sentinel, use the same operation type for + // prune and extend. + { + auto operationType2 = operationType == EXTEND ? PRUNE : operationType; + if (seen.count({qname, qtype, operationType2}) != 0) { + if (operationType == EXTEND) { + changetype = "EXTEND/PRUNE"; // for the sake of the error message + } + throw ApiException("Duplicate RRset " + qname.toString() + " IN " + qtype.toString() + " with changetype: " + changetype); + } + seen.insert({qname, qtype, operationType2}); } - seen.insert({qname, qtype, operationType}); if (operationType != DELETE) { if (domainInfo.kind == DomainInfo::Consumer) { @@ -2690,47 +2699,52 @@ static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInf } } - bool abortPatch{false}; + applyResult result; switch (operationType) { case DELETE: - applyDelete(domainInfo, qname, qtype); + result = applyDelete(domainInfo, qname, qtype); break; case REPLACE: - abortPatch = !applyReplace(domainInfo, zonename, rrset, qname, qtype, allowUnderscores, soa, resp); + result = applyReplace(domainInfo, zonename, rrset, qname, qtype, allowUnderscores, soa, resp); break; case PRUNE: case EXTEND: - abortPatch = !applyPruneOrExtend(domainInfo, zonename, rrset, qname, qtype, allowUnderscores, soa, resp, operationType); + result = applyPruneOrExtend(domainInfo, zonename, rrset, qname, qtype, allowUnderscores, soa, resp, operationType); break; } - if (abortPatch) { + if (result == ABORT) { // Proper error response has been setup, no need to do anything further. domainInfo.backend->abortTransaction(); return; } + if (result == SUCCESS) { + madeAnyChanges = true; + } } - SOAData soaData; - bool zone_disabled = (!backend.getSOAUncached(zonename, soaData)); + if (madeAnyChanges) { + SOAData soaData; + bool zone_disabled = (!backend.getSOAUncached(zonename, soaData)); - // edit SOA (if needed) - if (!zone_disabled && !soa.edit_api_kind.empty() && !soa.edit_done) { - // return old serial in headers, before changing it - resp->headers["X-PDNS-Old-Serial"] = std::to_string(soaData.serial); + // edit SOA (if needed) + if (!zone_disabled && !soa.edit_api_kind.empty() && !soa.edit_done) { + // return old serial in headers, before changing it + resp->headers["X-PDNS-Old-Serial"] = std::to_string(soaData.serial); - updateZoneSerial(domainInfo, soaData, soa.edit_api_kind, soa.edit_kind); + updateZoneSerial(domainInfo, soaData, soa.edit_api_kind, soa.edit_kind); - // return new serial in headers - resp->headers["X-PDNS-New-Serial"] = std::to_string(soaData.serial); - } + // return new serial in headers + resp->headers["X-PDNS-New-Serial"] = std::to_string(soaData.serial); + } - // Rectify - DNSSECKeeper dnssecKeeper(&backend); - if (!zone_disabled && !dnssecKeeper.isPresigned(zonename) && isZoneApiRectifyEnabled(domainInfo)) { - string info; - string error_msg; - if (!dnssecKeeper.rectifyZone(zonename, error_msg, info, false)) { - throw ApiException("Failed to rectify '" + zonename.toString() + "' " + error_msg); + // Rectify + DNSSECKeeper dnssecKeeper(&backend); + if (!zone_disabled && !dnssecKeeper.isPresigned(zonename) && isZoneApiRectifyEnabled(domainInfo)) { + string info; + string error_msg; + if (!dnssecKeeper.rectifyZone(zonename, error_msg, info, false)) { + throw ApiException("Failed to rectify '" + zonename.toString() + "' " + error_msg); + } } } } @@ -2739,10 +2753,15 @@ static void patchZone(UeberBackend& backend, const ZoneName& zonename, DomainInf throw; } - domainInfo.backend->commitTransaction(); + if (madeAnyChanges) { + domainInfo.backend->commitTransaction(); - DNSSECKeeper::clearCaches(zonename); - purgeAuthCaches(zonename.operator const DNSName&().toString() + "$"); + DNSSECKeeper::clearCaches(zonename); + purgeAuthCaches(zonename.operator const DNSName&().toString() + "$"); + } + else { + domainInfo.backend->abortTransaction(); + } resp->body = ""; resp->status = 204; // No Content, but indicate success diff --git a/regression-tests.api/test_Zones.py b/regression-tests.api/test_Zones.py index 2d60c0121b..748025f4f2 100644 --- a/regression-tests.api/test_Zones.py +++ b/regression-tests.api/test_Zones.py @@ -1379,7 +1379,7 @@ $NAME$ 1D IN SOA ns1.example.org. hostmaster.example.org. ( def test_zone_rr_bogus_update_5(self): name, payload, zone = self.create_zone() - # more than one extend changeset + # duplicate rrset in extend rrset1 = { 'changetype': 'extend', 'name': 'a.'+name, @@ -1398,7 +1398,17 @@ $NAME$ 1D IN SOA ns1.example.org. hostmaster.example.org. ( data=json.dumps(payload), headers={'content-type': 'application/json'}) self.assertEqual(r.status_code, 422) - self.assert_in_json_error("Only one rrset may be provided", r.json()) + self.assert_in_json_error("Duplicate RRset", r.json()) + # duplicate rrset in extend/prune + rrset2 = rrset1 + rrset2['changetype'] = 'prune' + payload = {'rrsets': [rrset1, rrset2]} + r = self.session.patch( + self.url("/api/v1/servers/localhost/zones/" + name), + data=json.dumps(payload), + headers={'content-type': 'application/json'}) + self.assertEqual(r.status_code, 422) + self.assert_in_json_error("Duplicate RRset", r.json()) def test_zone_rr_update(self): name, payload, zone = self.create_zone()