Merge pull request #7891 from Habbie/gsql-transactions

auth API, pdnsutil: improve backend transaction correctness

pdns/backends/gsql/gsqlbackend.cc

@@ -1349,6 +1349,10 @@ bool GSQLBackend::replaceRRSet(uint32_t domain_id, const DNSName& qname, const Q
   try {
     reconnectIfNeeded();
 
+    if (!d_inTransaction) {
+      throw PDNSException("replaceRRSet called outside of transaction");
+    }
+
     if (qt != QType::ANY) {
       d_DeleteRRSetQuery_stmt->
         bind("domain_id", domain_id)->
@@ -1495,6 +1499,9 @@ bool GSQLBackend::startTransaction(const DNSName &domain, int domain_id)
   try {
     reconnectIfNeeded();
 
+    if (inTransaction()) {
+      throw PDNSException("Attempted to start transaction while one was already active (domain '" + domain.toLogString() + "')");
+    }
     d_db->startTransaction();
     d_inTransaction = true;
     if(domain_id >= 0) {
@@ -1611,6 +1618,10 @@ bool GSQLBackend::replaceComments(const uint32_t domain_id, const DNSName& qname
   try {
     reconnectIfNeeded();
 
+    if (!d_inTransaction) {
+      throw PDNSException("replaceComments called outside of transaction");
+    }
+
     d_DeleteCommentRRsetQuery_stmt->
       bind("domain_id",domain_id)->
       bind("qname", qname)->

pdns/pdnsutil.cc

@@ -157,11 +157,11 @@ void loadMainConfig(const std::string& configdir)
   UeberBackend::go();
 }
 
-bool rectifyZone(DNSSECKeeper& dk, const DNSName& zone, bool quiet = false)
+bool rectifyZone(DNSSECKeeper& dk, const DNSName& zone, bool quiet = false, bool rectifyTransaction = true)
 {
   string output;
   string error;
-  bool ret = dk.rectifyZone(zone, error, output, true);
+  bool ret = dk.rectifyZone(zone, error, output, rectifyTransaction);
   if (!quiet || !ret) {
     // When quiet, only print output if there was an error
     if (!output.empty()) {
@@ -859,9 +859,10 @@ int clearZone(DNSSECKeeper& dk, const DNSName &zone) {
   return EXIT_SUCCESS;
 }
 
-int editZone(DNSSECKeeper& dk, const DNSName &zone) {
+int editZone(const DNSName &zone) {
   UeberBackend B;
   DomainInfo di;
+  DNSSECKeeper dk(&B);
 
   if (! B.getDomainInfo(zone, di)) {
     cerr<<"Domain '"<<zone<<"' not found!"<<endl;
@@ -1004,6 +1005,7 @@ int editZone(DNSSECKeeper& dk, const DNSName &zone) {
   else if(changed.empty() || c!='a')
     goto reAsk2;
 
+  di.backend->startTransaction(zone, -1);
   for(const auto& change : changed) {
     vector<DNSResourceRecord> vrr;
     for(const DNSRecord& rr : grouped[change.first]) {
@@ -1013,7 +1015,8 @@ int editZone(DNSSECKeeper& dk, const DNSName &zone) {
     }
     di.backend->replaceRRSet(di.id, change.first.first, QType(change.first.second), vrr);
   }
-  rectifyZone(dk, zone);
+  rectifyZone(dk, zone, false, false);
+  di.backend->commitTransaction();
   return EXIT_SUCCESS;
 }
 
@@ -1190,6 +1193,9 @@ int addOrReplaceRecord(bool addOrReplace, const vector<string>& cmds) {
   rr.domain_id = di.id;
   rr.qname = name;
   DNSResourceRecord oldrr;
+
+  di.backend->startTransaction(zone, -1);
+
   if(addOrReplace) { // the 'add' case
     di.backend->lookup(rr.qtype, rr.qname, 0, di.id);
 
@@ -1245,6 +1251,7 @@ int addOrReplaceRecord(bool addOrReplace, const vector<string>& cmds) {
   di.backend->replaceRRSet(di.id, name, rr.qtype, newrrs);
   // need to be explicit to bypass the ueberbackend cache!
   di.backend->lookup(rr.qtype, name, 0, di.id);
+  di.backend->commitTransaction();
   cout<<"New rrset:"<<endl;
   while(di.backend->get(rr)) {
     cout<<rr.qname.toString()<<" "<<rr.ttl<<" IN "<<rr.qtype.getName()<<" "<<rr.content<<endl;
@@ -1270,7 +1277,9 @@ int deleteRRSet(const std::string& zone_, const std::string& name_, const std::s
     name=DNSName(name_)+zone;
 
   QType qt(QType::chartocode(type_.c_str()));
+  di.backend->startTransaction(zone, -1);
   di.backend->replaceRRSet(di.id, name, qt, vector<DNSResourceRecord>());
+  di.backend->commitTransaction();
   return EXIT_SUCCESS;
 }
 
@@ -2388,7 +2397,7 @@ try
     if(cmds[1]==".")
       cmds[1].clear();
 
-    exit(editZone(dk, DNSName(cmds[1])));
+    exit(editZone(DNSName(cmds[1])));
   }
   else if(cmds[0] == "clear-zone") {
     if(cmds.size() != 2) {

pdns/ssqlite3.cc

@@ -46,6 +46,11 @@ int pdns_sqlite3_clear_bindings(sqlite3_stmt *pStmt){
   return rc;
 }
 
+static string SSQLite3ErrorString(sqlite3 *db)
+{
+  return string(sqlite3_errmsg(db)+string(" (")+std::to_string(sqlite3_extended_errcode(db))+string(")"));
+}
+
 class SSQLite3Statement: public SSqlStatement
 {
 public:
@@ -86,8 +91,8 @@ class SSQLite3Statement: public SSqlStatement
       // failed.
       releaseStatement();
       if (d_rc == SQLITE_CANTOPEN)
-        throw SSqlException(string("CANTOPEN error in sqlite3, often caused by unwritable sqlite3 db *directory*: ")+string(sqlite3_errmsg(d_db->db())));
-      throw SSqlException(string("Error while retrieving SQLite query results: ")+string(sqlite3_errmsg(d_db->db())));
+        throw SSqlException(string("CANTOPEN error in sqlite3, often caused by unwritable sqlite3 db *directory*: ")+SSQLite3ErrorString(d_db->db()));
+      throw SSqlException(string("Error while retrieving SQLite query results: ")+SSQLite3ErrorString(d_db->db()));
     }
     if(d_dolog) 
       g_log<<Logger::Warning<< "Query "<<((long)(void*)this)<<": "<<d_dtime.udiffNoReset()<<" usec to execute"<<endl;
@@ -164,7 +169,7 @@ class SSQLite3Statement: public SSqlStatement
 #endif
     {
       releaseStatement();
-      throw SSqlException(string("Unable to compile SQLite statement : '")+d_query+"': "+sqlite3_errmsg(d_db->db()));
+      throw SSqlException(string("Unable to compile SQLite statement : '")+d_query+"': "+SSQLite3ErrorString(d_db->db()));
     }
     if (pTail && strlen(pTail)>0)
       g_log<<Logger::Warning<<"Sqlite3 command partially processed. Unprocessed part: "<<pTail<<endl;

pdns/ws-auth.cc

@@ -618,7 +618,7 @@ static void throwUnableToSecure(const DNSName& zonename) {
       + "capable backends are loaded, or because the backends have DNSSEC disabled. Check your configuration.");
 }
 
-static void updateDomainSettingsFromDocument(UeberBackend& B, const DomainInfo& di, const DNSName& zonename, const Json document) {
+static void updateDomainSettingsFromDocument(UeberBackend& B, const DomainInfo& di, const DNSName& zonename, const Json document, bool rectifyTransaction=true) {
   vector<string> zonemaster;
   bool shouldRectify = false;
   for(auto value : document["masters"].array_items()) {
@@ -743,7 +743,7 @@ static void updateDomainSettingsFromDocument(UeberBackend& B, const DomainInfo&
     if (api_rectify == "1") {
       string info;
       string error_msg;
-      if (!dk.rectifyZone(zonename, error_msg, info, true)) {
+      if (!dk.rectifyZone(zonename, error_msg, info, rectifyTransaction)) {
         throw ApiException("Failed to rectify '" + zonename.toString() + "' " + error_msg);
       }
     }
@@ -1642,13 +1642,13 @@ static void apiServerZones(HttpRequest* req, HttpResponse* resp) {
     if(!B.getDomainInfo(zonename, di))
       throw ApiException("Creating domain '"+zonename.toString()+"' failed: lookup of domain ID failed");
 
+    di.backend->startTransaction(zonename, di.id);
+
     // updateDomainSettingsFromDocument does NOT fill out the default we've established above.
     if (!soa_edit_api_kind.empty()) {
       di.backend->setDomainMetadataOne(zonename, "SOA-EDIT-API", soa_edit_api_kind);
     }
 
-    di.backend->startTransaction(zonename, di.id);
-
     for(auto rr : new_records) {
       rr.domain_id = di.id;
       di.backend->feedRecord(rr, DNSName());
@@ -1658,7 +1658,7 @@ static void apiServerZones(HttpRequest* req, HttpResponse* resp) {
       di.backend->feedComment(c);
     }
 
-    updateDomainSettingsFromDocument(B, di, zonename, document);
+    updateDomainSettingsFromDocument(B, di, zonename, document, false);
 
     di.backend->commitTransaction();
 
@@ -1714,7 +1714,9 @@ static void apiServerZoneDetail(HttpRequest* req, HttpResponse* resp) {
   if(req->method == "PUT") {
     // update domain settings
 
-    updateDomainSettingsFromDocument(B, di, zonename, req->json());
+    di.backend->startTransaction(zonename, -1);
+    updateDomainSettingsFromDocument(B, di, zonename, req->json(), false);
+    di.backend->commitTransaction();
 
     resp->body = "";
     resp->status = 204; // No Content, but indicate success