Allow for multiple extend/prune in the same request as long as different RRsets.

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

diff --git a/pdns/ws-auth.cc b/pdns/ws-auth.cc
index 0d93f1774a9ebae28ce5abc050c6064976b8ae33..0ae6e2d6e907ca2b2e16089246f4920667ba9a4e 100644 (file)
--- 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<Json>& 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 2d60c0121b2bbc68a92f06f00f64158135bf247e..748025f4f2ba09ab967ee0f7f80677190dd39d70 100644 (file)
--- 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()