[1771-2] remove seen_ns&other check in getRRsets.

on a closer look, it just doesn't seem to be necessary if we are okay
with just "hiding" records under a delegation NS for normal (ie. non
GLUE_OK) lookups, instead of throwing an exception.

Essentially only one existing test case (which expects an exception
in that setup) should be updated (and the "brokenns2" case just seems
to be redundant and so was removed), and the disabled tests
in zone_finder_context_unittest now pass.

diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc
index 358dce8aadc6b99f2d8f661e670225def475b149..f824acea9569bd192003365b1a09495d73a6888c 100644 (file)
--- a/src/lib/datasrc/database.cc
+++ b/src/lib/datasrc/database.cc
@@ -206,7 +206,6 @@ DatabaseClient::Finder::getRRsets(const string& name, const WantedTypes& types,
     bool seen_cname(false);
     bool seen_ds(false);
     bool seen_other(false);
-    bool seen_ns(false);
 
     while (context->getNext(columns)) {
         // The domain is not empty
@@ -249,8 +248,6 @@ DatabaseClient::Finder::getRRsets(const string& name, const WantedTypes& types,
 
             if (cur_type == RRType::CNAME()) {
                 seen_cname = true;
-            } else if (cur_type == RRType::NS()) {
-                seen_ns = true;
             } else if (cur_type == RRType::DS()) {
                 seen_ds = true;
             } else if (cur_type != RRType::RRSIG() &&
@@ -278,11 +275,11 @@ DatabaseClient::Finder::getRRsets(const string& name, const WantedTypes& types,
                       RDATA_COLUMN]);
         }
     }
-    if (seen_cname && (seen_other || seen_ns || seen_ds)) {
+    if (seen_cname && (seen_other || seen_ds)) {
         isc_throw(DataSourceError, "CNAME shares domain " << name <<
                   " with something else");
     }
-    if (check_ns && seen_ns && seen_other) {
+    if (check_ns && /*seen_ns*/ false && seen_other) {
         isc_throw(DataSourceError, "NS shares domain " << name <<
                   " with something else");
     }

diff --git a/src/lib/datasrc/tests/database_unittest.cc b/src/lib/datasrc/tests/database_unittest.cc
index 55d8052653a92966cc973ae8b628c4274d6fe03e..0302f0c2530a0268deaa5b4a563578b9cedfb318 100644 (file)
--- a/src/lib/datasrc/tests/database_unittest.cc
+++ b/src/lib/datasrc/tests/database_unittest.cc
@@ -168,13 +168,16 @@ const char* const TEST_RECORDS[][5] = {
     {"child.insecdelegation.example.org.", "DS", "3600", "", "DS 5 3 3600 "
      "20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE"},
 
-    // Broken NS
+    // Delegation NS and other ordinary type of RR coexist at the same
+    // name.  This is deviant (except for some special cases like the other
+    // RR could be used for addressing the NS name), but as long as the
+    // other records are hidden behind the delegation for normal queries
+    // it's not necessarily harmful. (so "broken" may be too strong, but we
+    // keep the name since it could be in a chain of sorted names for DNSSEC
+    // processing and renaming them may have other bad effects for tests).
     {"brokenns1.example.org.", "A", "3600", "", "192.0.2.1"},
     {"brokenns1.example.org.", "NS", "3600", "", "ns.example.com."},
 
-    {"brokenns2.example.org.", "NS", "3600", "", "ns.example.com."},
-    {"brokenns2.example.org.", "A", "3600", "", "192.0.2.1"},
-
     // Now double DNAME, to test failure mode
     {"baddname.example.org.", "DNAME", "3600", "", "dname1.example.com."},
     {"baddname.example.org.", "DNAME", "3600", "", "dname2.example.com."},
@@ -2202,15 +2205,23 @@ TYPED_TEST(DatabaseClientTest, findDelegation) {
                               ZoneFinder::FIND_DEFAULT),
                  DataSourceError);
 
-    // Broken NS - it lives together with something else
-    EXPECT_THROW(finder->find(isc::dns::Name("brokenns1.example.org."),
-                              this->qtype_,
-                              ZoneFinder::FIND_DEFAULT),
-                 DataSourceError);
-    EXPECT_THROW(finder->find(isc::dns::Name("brokenns2.example.org."),
-                              this->qtype_,
-                              ZoneFinder::FIND_DEFAULT),
-                 DataSourceError);
+    // NS and other type coexist: deviant and not necessarily harmful.
+    // It should normally just result in DELEGATION; if GLUE_OK is specified,
+    // the other RR should be visible.
+    this->expected_rdatas_.clear();
+    this->expected_rdatas_.push_back("ns.example.com");
+    doFindTest(*finder, Name("brokenns1.example.org"), this->qtype_,
+               RRType::NS(), this->rrttl_, ZoneFinder::DELEGATION,
+               this->expected_rdatas_, this->empty_rdatas_,
+               ZoneFinder::RESULT_DEFAULT);
+
+    this->expected_rdatas_.clear();
+    this->expected_rdatas_.push_back("192.0.2.1");
+    doFindTest(*finder, Name("brokenns1.example.org"), this->qtype_,
+               this->qtype_, this->rrttl_, ZoneFinder::SUCCESS,
+               this->expected_rdatas_, this->empty_rdatas_,
+               ZoneFinder::RESULT_DEFAULT, Name("brokenns1.example.org"),
+               ZoneFinder::FIND_GLUE_OK);
 }
 
 TYPED_TEST(DatabaseClientTest, findDS) {

diff --git a/src/lib/datasrc/tests/zone_finder_context_unittest.cc b/src/lib/datasrc/tests/zone_finder_context_unittest.cc
index 50d409eb4c817cd8b333ed881de559d95d8ac35e..b3c9b2d2fe789c81b42b24e58dcf143efc67a855 100644 (file)
--- a/src/lib/datasrc/tests/zone_finder_context_unittest.cc
+++ b/src/lib/datasrc/tests/zone_finder_context_unittest.cc
@@ -208,13 +208,6 @@ TEST_P(ZoneFinderContextTest, getAdditionalDelegation) {
 TEST_P(ZoneFinderContextTest, getAdditionalDelegationAtZoneCut) {
     // Similar to the previous case, but one of the NS addresses is at the
     // zone cut.
-
-    // XXX: the current database-based data source incorrectly rejects this
-    // setup (see #1771)
-    if (GetParam() == createSQLite3Client) {
-        return;
-    }
-
     ZoneFinderContextPtr ctx = finder_->find(Name("www.b.example.org"),
                                              RRType::SOA());
     EXPECT_EQ(ZoneFinder::DELEGATION, ctx->code);
@@ -316,12 +309,6 @@ TEST_P(ZoneFinderContextTest, getAdditionalMX) {
 }
 
 TEST_P(ZoneFinderContextTest, getAdditionalMXAtZoneCut) {
-    // XXX: the current database-based data source incorrectly rejects this
-    // setup (see #1771)
-    if (GetParam() == createSQLite3Client) {
-        return;
-    }
-
     ZoneFinderContextPtr ctx = finder_->find(Name("mxatcut.example.org."),
                                              RRType::MX());
     EXPECT_EQ(ZoneFinder::SUCCESS, ctx->code);