[pdnsutil] dedup in add-record

pdns/pdnsutil.cc

@@ -1592,7 +1592,7 @@ static int createZone(const DNSName &zone, const DNSName& nsname) {
 }
 
 // add-record ZONE name type [ttl] "content" ["content"]
-static int addOrReplaceRecord(bool addOrReplace, const vector<string>& cmds) {
+static int addOrReplaceRecord(bool isAdd, const vector<string>& cmds) {
   DNSResourceRecord rr;
   vector<DNSResourceRecord> newrrs;
   DNSName zone(cmds.at(1));
@@ -1602,73 +1602,104 @@ static int addOrReplaceRecord(bool addOrReplace, const vector<string>& cmds) {
   else
     name = DNSName(cmds.at(2)) + zone;
 
-  rr.qtype = DNSRecordContent::TypeToNumber(cmds.at(3));
-  rr.ttl = ::arg().asNum("default-ttl");
-
   UtilBackend B; //NOLINT(readability-identifier-length)
   DomainInfo di;
   if(!B.getDomainInfo(zone, di)) {
     cerr << "Zone '" << zone << "' does not exist" << endl;
     return EXIT_FAILURE;
   }
+
+  rr.qtype = DNSRecordContent::TypeToNumber(cmds.at(3));
+  rr.ttl = ::arg().asNum("default-ttl");
   rr.auth = true;
   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, di.id);
-
-    while(di.backend->get(oldrr))
-      newrrs.push_back(oldrr);
-  }
-
   unsigned int contentStart = 4;
   if(cmds.size() > 5) {
-    rr.ttl = atoi(cmds.at(4).c_str());
-    if (std::to_string(rr.ttl) == cmds.at(4)) {
+    uint32_t ttl = atoi(cmds.at(4).c_str());
+    if (std::to_string(ttl) == cmds.at(4)) {
+      rr.ttl = ttl;
       contentStart++;
     }
-    else {
-      rr.ttl = ::arg().asNum("default-ttl");
-    }
   }
 
-  di.backend->lookup(QType(QType::ANY), rr.qname, di.id);
-  bool found=false;
-  if(rr.qtype.getCode() == QType::CNAME) { // this will save us SO many questions
+  di.backend->startTransaction(zone, -1);
 
-    while(di.backend->get(oldrr)) {
-      if(addOrReplace || oldrr.qtype.getCode() != QType::CNAME) // the replace case is ok if we replace one CNAME by the other
-        found=true;
+  // Enforce that CNAME records can not be mixed with any other.
+  // If we add a CNAME: there should be no existing records except for one
+  // possible previous CNAME, which will get replaced.
+  // If we add something else: there should be no existing CNAME record.
+  bool reject{false};
+  if (rr.qtype.getCode() == QType::CNAME) {
+    di.backend->lookup(QType(QType::ANY), rr.qname, static_cast<int>(di.id));
+    while (di.backend->get(oldrr)) {
+      if (oldrr.qtype.getCode() == QType::CNAME) {
+        if (not isAdd) {
+          // Replacement operation: ok to replace CNAME with another
+          continue;
+        }
+      }
+      reject = true;
     }
-    if(found) {
-      cerr<<"Attempting to add CNAME to "<<rr.qname<<" which already had existing records"<<endl;
+    if (reject) {
+      cerr<<"Attempting to add CNAME to "<<rr.qname<<" which already has existing records"<<endl;
       return EXIT_FAILURE;
     }
   }
   else {
-    while(di.backend->get(oldrr)) {
-      if(oldrr.qtype.getCode() == QType::CNAME)
-        found=true;
-    }
-    if(found) {
-      cerr<<"Attempting to add record to "<<rr.qname<<" which already had a CNAME record"<<endl;
+    di.backend->lookup(QType(QType::CNAME), rr.qname, static_cast<int>(di.id));
+    // TODO: It would be nice to have a way to complete a lookup and release its
+    // resources without having to exhaust its results - here one successful
+    // get() is all we need to decide to reject the operation. I'm looking at
+    // you, lmdb backend.
+    while (di.backend->get(oldrr)) {
+      reject = true;
+    }
+    if (reject) {
+      cerr<<"Attempting to add record to "<<rr.qname<<" which already has a CNAME record"<<endl;
       return EXIT_FAILURE;
     }
   }
 
-  if(!addOrReplace) {
-    cout<<"Current records for "<<rr.qname<<" IN "<<rr.qtype.toString()<<" will be replaced"<<endl;
-  }
+  // Synthesize the new records.
   for(auto i = contentStart ; i < cmds.size() ; ++i) {
     rr.content = DNSRecordContent::make(rr.qtype.getCode(), QClass::IN, cmds.at(i))->getZoneRepresentation(true);
 
-    newrrs.push_back(rr);
+    bool skip{false};
+    for (const auto &record: newrrs) {
+      if (rr.content == record.content) {
+        cout<<R"(Ignoring duplicate record content ")"<<rr.content<<R"(")"<<endl;
+        skip = true;
+        break;
+      }
+    }
+    if (!skip) {
+      newrrs.push_back(rr);
+    }
   }
 
+  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(rr.qtype, rr.qname, static_cast<int>(di.id));
+    while (di.backend->get(oldrr)) {
+      oldrrs.push_back(oldrr);
+      for (auto iter = newrrs.begin(); iter != newrrs.end(); ++iter) {
+        if (iter->content == oldrr.content) {
+          newrrs.erase(iter);
+          break;
+        }
+      }
+    }
+    oldrrs.insert(oldrrs.end(), newrrs.begin(), newrrs.end());
+    newrrs = std::move(oldrrs);
+  }
+  else {
+    cout<<"All existing records for "<<rr.qname<<" IN "<<rr.qtype.toString()<<" will be replaced"<<endl;
+  }
 
   if(!di.backend->replaceRRSet(di.id, name, rr.qtype, newrrs)) {
     cerr<<"backend did not accept the new RRset, aborting"<<endl;

regression-tests/backends/bind-master

@@ -1,5 +1,6 @@
 case $context in
     bind)
+        backend=bind
         cat > pdns-bind.conf << __EOF__
 module-dir=$PDNS_BUILD_PATH/modules
 launch=bind
@@ -17,6 +18,7 @@ __EOF__
 
     bind-dnssec | bind-dnssec-nsec3 | bind-hybrid-nsec3 | bind-dnssec-nsec3-optout | bind-dnssec-nsec3-narrow)
         rm -f dnssec.sqlite3
+        backend=bind
         cat > pdns-bind.conf << __EOF__
 module-dir=$PDNS_BUILD_PATH/modules
 launch=bind

regression-tests/backends/geoip-master

@@ -116,6 +116,7 @@ Rcode: 0 (No Error), RD: 0, QR: 1, TC: 0, AA: 1, opcode: 0
 Reply to question for qname='continent.geo.example.com.', qtype=TXT
 EOF
 		# generate pdns.conf for pdnsutil
+		backend=geoip
 		cat > pdns-geoip.conf <<EOF
 module-dir=./modules
 launch=geoip

regression-tests/backends/gmysql-slave

@@ -11,6 +11,7 @@
 	mysql --user="$GMYSQL2USER" --password="$GMYSQL2PASSWD" --host="$GMYSQL2HOST" \
 		"$GMYSQL2DB" < ../modules/gmysqlbackend/schema.mysql.sql
 
+	backend=gmysql2
 	cat > pdns-gmysql2.conf << __EOF__
 module-dir=./modules
 launch=gmysql

regression-tests/backends/godbc_mssql-slave

@@ -5,6 +5,7 @@
 		echo "drop table $table" | $ISQL -b
 	done
 	$ISQL < ../modules/godbcbackend/schema.mssql.sql
+	backend=godbc2
 	cat > pdns-godbc2.conf << __EOF__
 module-dir=./modules
 launch=godbc

regression-tests/backends/gpgsql-slave

@@ -6,6 +6,7 @@
 	createdb --user="$GPGSQL2USER" "$GPGSQL2DB" || echo ignoring createdb failure
 	psql --user="$GPGSQL2USER" "$GPGSQL2DB" < ../modules/gpgsqlbackend/schema.pgsql.sql
 
+	backend=gpgsql2
 	cat > pdns-gpgsql2.conf << __EOF__
 module-dir=./modules
 launch=gpgsql

regression-tests/backends/gsqlite3-slave

@@ -2,6 +2,7 @@
 	rm -f pdns.sqlite32
 	sqlite3 pdns.sqlite32 < ../modules/gsqlite3backend/schema.sqlite3.sql
 
+	backend=gsqlite32
 	cat > pdns-gsqlite32.conf << __EOF__
 module-dir=./modules
 launch=gsqlite3

regression-tests/backends/ldap-master

@@ -16,6 +16,7 @@ __EOF__
 
 		$ZONE2LDAP --dnsttl=yes --basedn=$LDAPBASEDN --layout=$layout --named-conf=named.conf | ldapmodify -D $LDAPUSER -w $LDAPPASSWD -H $LDAPHOST -c > /dev/null || true
 
+		backend=ldap
 		cat > pdns-ldap.conf << __EOF__
 module-dir=$PDNS_BUILD_PATH/modules
 launch=ldap

regression-tests/backends/lmdb-master

@@ -1,5 +1,6 @@
 case $context in
     lmdb | lmdb-nodnssec | lmdb-nsec3 | lmdb-nsec3-optout | lmdb-nsec3-narrow)
+        backend=lmdb
         cat > pdns-lmdb.conf << __EOF__
 module-dir=$PDNS_BUILD_PATH/modules
 launch=lmdb

regression-tests/backends/lmdb-slave

@@ -1,4 +1,5 @@
         context=${context}-presigned-lmdb
+        backend=lmdb2
         cat > pdns-lmdb2.conf << __EOF__
 module-dir=./modules
 launch=lmdb

regression-tests/backends/lua2-master

@@ -22,6 +22,7 @@ case $context in
 		fi
 
 		# generate pdns.conf for pdnsutil
+		backend=lua2
 		cat > pdns-lua2.conf <<EOF
 module-dir=$PDNS_BUILD_PATH/modules
 launch=lua2

regression-tests/backends/remote-master

@@ -89,6 +89,7 @@ case $context in
 
 
 		# generate pdns.conf for pdnsutil
+		backend=remote
 		cat > pdns-remote.conf <<EOF
 module-dir=$PDNS_BUILD_PATH/modules
 launch=remote

regression-tests/tests/pdnsutil-zone-handling/command

@@ -0,0 +1,39 @@
+#!/bin/sh
+
+# The first invocation of pdnsutil is redirected to /dev/null to hide the
+# "local files have been created" message if using lmdb as backend.
+# All other pdnsutil invocation have the Ueberbackend destructor messages from
+# --enable-verbose-logging removed.
+# Invocations which will output zone contents are passed through sort(1), as
+# the order of entries for records having multiple entries is backend-specific
+# and not guaranteed to be in any particular order.
+$PDNSUTIL --config-dir=. --config-name=$backend \
+	create-zone bug.less \
+	> /dev/null 2>&1
+$PDNSUTIL --config-dir=. --config-name=$backend \
+	add-record bug.less cname CNAME host \
+	2>&1 | grep -v Ueber
+$PDNSUTIL --config-dir=. --config-name=$backend \
+	add-record bug.less host A 127.0.0.1 \
+	2>&1 | grep -v Ueber
+# Duplicate records should be omitted
+$PDNSUTIL --config-dir=. --config-name=$backend \
+	add-record bug.less host2 A 127.0.0.2 127.0.0.2 \
+	2>&1 | sort | grep -v Ueber
+# Can't add non-CNAME record to a CNAME record
+$PDNSUTIL --config-dir=. --config-name=$backend \
+	add-record bug.less cname A 127.0.0.1 \
+	2>&1 | grep -v Ueber
+# Can't add CNAME record if other records exist
+$PDNSUTIL --config-dir=. --config-name=$backend \
+	add-record bug.less host CNAME host2 \
+	2>&1 | grep -v Ueber
+# Adding existing record should ignore duplicates
+$PDNSUTIL --config-dir=. --config-name=$backend \
+	add-record bug.less host2 A 127.0.0.2 127.0.0.3 \
+	2>&1 | sort | grep -v Ueber
+
+# Display zone contents for final verification
+$PDNSUTIL --config-dir=. --config-name=$backend \
+	list-zone bug.less \
+	2>&1 | sort | grep -v Ueber

regression-tests/tests/pdnsutil-zone-handling/description

@@ -0,0 +1 @@
+This test checks various pdnsutil behaviour when editing zone contents.

regression-tests/tests/pdnsutil-zone-handling/expected_result

@@ -0,0 +1,18 @@
+New rrset:
+cname.bug.less. 3600 IN CNAME host
+New rrset:
+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
+Attempting to add record to cname.bug.less which already has a CNAME record
+Attempting to add CNAME to host.bug.less which already has existing records
+New rrset:
+host2.bug.less. 3600 IN A 127.0.0.2
+host2.bug.less. 3600 IN A 127.0.0.3
+$ORIGIN .
+bug.less	3600	IN	SOA	a.misconfigured.dns.server.invalid hostmaster.bug.less 0 10800 3600 604800 3600
+cname.bug.less	3600	IN	CNAME	host.
+host.bug.less	3600	IN	A	127.0.0.1
+host2.bug.less	3600	IN	A	127.0.0.2
+host2.bug.less	3600	IN	A	127.0.0.3