[master] Merge branch 'trac1771-2'

src/lib/datasrc/database.cc

@@ -179,8 +179,7 @@ private:
 
 DatabaseClient::Finder::FoundRRsets
 DatabaseClient::Finder::getRRsets(const string& name, const WantedTypes& types,
-                                  bool check_ns, const string* construct_name,
-                                  bool any,
+                                  const string* construct_name, bool any,
                                   DatabaseAccessor::IteratorContextPtr context)
 {
     RRsigStore sig_store;
@@ -204,9 +203,7 @@ DatabaseClient::Finder::getRRsets(const string& name, const WantedTypes& types,
     const Name construct_name_object(*construct_name);
 
     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,16 +246,12 @@ 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() &&
                        cur_type != RRType::NSEC3() &&
                        cur_type != RRType::NSEC()) {
                 // NSEC and RRSIG can coexist with anything, otherwise
                 // we've seen something that can't live together with potential
-                // CNAME or NS
+                // CNAME.
                 //
                 // NSEC3 lives in separate namespace from everything, therefore
                 // we just ignore it here for these checks as well.
@@ -278,14 +271,10 @@ 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) {
         isc_throw(DataSourceError, "CNAME shares domain " << name <<
                   " with something else");
     }
-    if (check_ns && seen_ns && seen_other) {
-        isc_throw(DataSourceError, "NS shares domain " << name <<
-                  " with something else");
-    }
     // Add signatures to all found RRsets
     for (std::map<RRType, RRsetPtr>::iterator i(result.begin());
          i != result.end(); ++ i) {
@@ -455,20 +444,20 @@ DatabaseClient::Finder::findDelegationPoint(const isc::dns::Name& name,
     for (int i = remove_labels; i > 0; --i) {
         const Name superdomain(name.split(i));
 
-        // Note if this is the origin. (We don't count NS records at the origin
-        // as a delegation so this controls whether NS RRs are included in
-        // the results of some searches.)
-        const bool not_origin = (i != remove_labels);
-
         // Look if there's NS or DNAME at this point of the tree, but ignore
         // the NS RRs at the apex of the zone.
         const FoundRRsets found = getRRsets(superdomain.toText(),
-                                            DELEGATION_TYPES(), not_origin);
+                                            DELEGATION_TYPES());
         if (found.first) {
             // This node contains either NS or DNAME RRs so it does exist.
             const FoundIterator nsi(found.second.find(RRType::NS()));
             const FoundIterator dni(found.second.find(RRType::DNAME()));
 
+            // Note if this is the origin. (We don't count NS records at the
+            // origin as a delegation so this controls whether NS RRs are
+            // included in the results of some searches.)
+            const bool not_origin = (i != remove_labels);
+
             // An optimisation.  We know that there is an exact match for
             // something at this point in the tree so remember it.  If we have
             // to do a wildcard search, as we search upwards through the tree
@@ -477,7 +466,7 @@ DatabaseClient::Finder::findDelegationPoint(const isc::dns::Name& name,
             last_known = superdomain.getLabelCount();
 
             if (glue_ok && !first_ns && not_origin &&
-                    nsi != found.second.end()) {
+                nsi != found.second.end()) {
                 // If we are searching for glue ("glue OK" mode), store the
                 // highest NS record that we find that is not the apex.  This
                 // is another optimisation for later, where we need the
@@ -590,8 +579,9 @@ DatabaseClient::Finder::findWildcardMatch(
         // TODO Add a check for DNAME, as DNAME wildcards are discouraged (see
         // RFC 4592 section 4.4).
         // Search for a match.  The types are the same as with original query.
-        FoundRRsets found = getRRsets(wildcard, final_types, true,
-                                      &construct_name, type == RRType::ANY());
+        const FoundRRsets found = getRRsets(wildcard, final_types,
+                                            &construct_name,
+                                            type == RRType::ANY());
         if (found.first) {
             // Found something - but what?
 
@@ -694,7 +684,7 @@ DatabaseClient::Finder::FindDNSSECContext::probe() {
             // such cases).
             const string origin = finder_.getOrigin().toText();
             const FoundRRsets nsec3_found =
-                finder_.getRRsets(origin, NSEC3PARAM_TYPES(), false);
+                finder_.getRRsets(origin, NSEC3PARAM_TYPES());
             const FoundIterator nfi=
                 nsec3_found.second.find(RRType::NSEC3PARAM());
             is_nsec3_ = (nfi != nsec3_found.second.end());
@@ -705,7 +695,7 @@ DatabaseClient::Finder::FindDNSSECContext::probe() {
             // described in Section 10.4 of RFC 5155.
             if (!is_nsec3_) {
                 const FoundRRsets nsec_found =
-                    finder_.getRRsets(origin, NSEC_TYPES(), false);
+                    finder_.getRRsets(origin, NSEC_TYPES());
                 const FoundIterator nfi =
                     nsec_found.second.find(RRType::NSEC());
                 is_nsec_ = (nfi != nsec_found.second.end());
@@ -757,10 +747,8 @@ DatabaseClient::Finder::FindDNSSECContext::getDNSSECRRset(const Name &name,
     try {
         const Name& nsec_name =
             covering ? finder_.findPreviousName(name) : name;
-        const bool need_nscheck = (nsec_name != finder_.getOrigin());
         const FoundRRsets found = finder_.getRRsets(nsec_name.toText(),
-                                                    NSEC_TYPES(),
-                                                    need_nscheck);
+                                                    NSEC_TYPES());
         const FoundIterator nci = found.second.find(RRType::NSEC());
         if (nci != found.second.end()) {
             return (nci->second);
@@ -984,16 +972,15 @@ DatabaseClient::Finder::findInternal(const Name& name, const RRType& type,
     // - Requested name is a delegation point (NS only but not at the zone
     //   apex - DNAME is ignored here as it redirects DNS names subordinate to
     //   the owner name - the owner name itself is not redirected.)
-    const bool is_origin = (name == getOrigin());
     WantedTypes final_types(FINAL_TYPES());
     final_types.insert(type);
     const FoundRRsets found = getRRsets(name.toText(), final_types,
-                                        !is_origin, NULL,
-                                        type == RRType::ANY());
+                                        NULL, type == RRType::ANY());
     FindDNSSECContext dnssec_ctx(*this, options);
     if (found.first) {
         // Something found at the domain name.  Look into it further to get
         // the final result.
+        const bool is_origin = (name == getOrigin());
         return (findOnNameResult(name, type, options, is_origin, found, NULL,
                                  target, dnssec_ctx));
     } else {
@@ -1021,7 +1008,7 @@ DatabaseClient::Finder::findNSEC3(const Name& name, bool recursive) {
     // Now, we need to get the NSEC3 params from the apex and create the hash
     // creator for it.
     const FoundRRsets nsec3param(getRRsets(getOrigin().toText(),
-                                 NSEC3PARAM_TYPES(), false));
+                                           NSEC3PARAM_TYPES()));
     const FoundIterator param(nsec3param.second.find(RRType::NSEC3PARAM()));
     if (!nsec3param.first || param == nsec3param.second.end()) {
         // No NSEC3 params? :-(
@@ -1061,7 +1048,7 @@ DatabaseClient::Finder::findNSEC3(const Name& name, bool recursive) {
         }
 
         const FoundRRsets nsec3(getRRsets(hash + "." + otext, NSEC3_TYPES(),
-                                          false, NULL, false, context));
+                                          NULL, false, context));
 
         if (nsec3.first) {
             // We found an exact match against the current label.
@@ -1086,8 +1073,8 @@ DatabaseClient::Finder::findNSEC3(const Name& name, bool recursive) {
                 arg(labels).arg(prevHash);
             context = accessor_->getNSEC3Records(prevHash, zone_id_);
             const FoundRRsets prev_nsec3(getRRsets(prevHash + "." + otext,
-                                                   NSEC3_TYPES(), false, NULL,
-                                                   false, context));
+                                                   NSEC3_TYPES(), NULL, false,
+                                                   context));
 
             if (!prev_nsec3.first) {
                 isc_throw(DataSourceError, "Hash " + prevHash + " returned "

src/lib/datasrc/database.h

@@ -963,17 +963,14 @@ public:
         ///
         /// \param name Which domain name should be scanned.
         /// \param types List of types the caller is interested in.
-        /// \param check_ns If this is set to true, it checks nothing lives
-        ///     together with NS record (with few little exceptions, like RRSIG
-        ///     or NSEC). This check is meant for non-apex NS records.
         /// \param construct_name If this is NULL, the resulting RRsets have
-        ///     their name set to name. If it is not NULL, it overrides the name
-        ///     and uses this one (this can be used for wildcard synthesized
-        ///     records).
+        ///     their name set to name. If it is not NULL, it overrides the
+        ///     name and uses this one (this can be used for wildcard
+        ///     synthesized records).
         /// \param any If this is true, it records all the types, not only the
         ///     ones requested by types. It also puts a NULL pointer under the
-        ///     ANY type into the result, if it finds any RRs at all, to easy the
-        ///     identification of success.
+        ///     ANY type into the result, if it finds any RRs at all, to easy
+        ///     the identification of success.
         /// \param srcContext This can be set to non-NULL value to override the
         ///     iterator context used for obtaining the data. This can be used,
         ///     for example, to get data from the NSEC3 namespace.
@@ -986,7 +983,7 @@ public:
         /// \throw DataSourceError If there's a low-level error with the
         ///     database or the database contains bad data.
         FoundRRsets getRRsets(const std::string& name,
-                              const WantedTypes& types, bool check_ns,
+                              const WantedTypes& types,
                               const std::string* construct_name = NULL,
                               bool any = false,
                               DatabaseAccessor::IteratorContextPtr srcContext =

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) {

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);

src/lib/datasrc/zone.h

@@ -376,6 +376,16 @@ public:
     ///   RRsets for that name are searched just like the normal case;
     ///   otherwise, if the search has encountered a zone cut, \c DELEGATION
     ///   with the information of the highest zone cut will be returned.
+    ///   Note: the term "glue" in the DNS protocol standard may sometimes
+    ///   cause confusion: some people use this term strictly for an address
+    ///   record (type AAAA or A) for the name used in the RDATA of an NS RR;
+    ///   some others seem to give it broader flexibility.  Nevertheless,
+    ///   in this API the "GLUE OK" simply means the search by find() can
+    ///   continue beyond a zone cut; the derived class implementation does
+    ///   not have to, and should not, check whether the type is an address
+    ///   record or whether the query name is pointed by some NS RR.
+    ///   It's up to the caller with which definition of "glue" the search
+    ///   result with this option should be used.
     /// - \c FIND_DNSSEC Request that DNSSEC data (like NSEC, RRSIGs) are
     ///   returned with the answer. It is allowed for the data source to
     ///   include them even when not requested.