[master] Merge branch 'trac2178'

src/lib/datasrc/client_list.cc

@@ -17,6 +17,7 @@
 #include "factory.h"
 #include "memory_datasrc.h"
 #include "logger.h"
+#include <dns/masterload.h>
 
 #include <memory>
 #include <boost/foreach.hpp>
@@ -144,23 +145,29 @@ ConfigurableClientList::configure(const ConstElementPtr& config,
                         finder(new
                             InMemoryZoneFinder(rrclass_, origin));
                     if (type == "MasterFiles") {
-                        finder->load(paramConf->get(*it)->stringValue());
+                        try {
+                            finder->load(paramConf->get(*it)->stringValue());
+                            cache->addZone(finder);
+                        } catch (const isc::dns::MasterLoadError& mle) {
+                            LOG_ERROR(logger, DATASRC_MASTERLOAD_ERROR)
+                                .arg(mle.what());
+                        }
                     } else {
                         ZoneIteratorPtr iterator;
                         try {
                             iterator = client->getIterator(origin);
-                        }
-                        catch (const DataSourceError&) {
-                            isc_throw(ConfigurationError, "Unable to cache "
-                                      "non-existent zone " << origin);
+                        } catch (const DataSourceError&) {
+                            isc_throw(ConfigurationError, "Unable to "
+                                      "cache non-existent zone "
+                                      << origin);
                         }
                         if (!iterator) {
-                            isc_throw(isc::Unexpected, "Got NULL iterator for "
-                                      "zone " << origin);
+                            isc_throw(isc::Unexpected, "Got NULL iterator "
+                                      "for zone " << origin);
                         }
                         finder->load(*iterator);
+                        cache->addZone(finder);
                     }
-                    cache->addZone(finder);
                 }
             }
         }

src/lib/datasrc/datasrc_messages.mes

@@ -305,6 +305,11 @@ Therefore, the zone will not be available for this process. If this is
 a problem, you should move the zone to some database backend (sqlite3, for
 example) and use it from there.
 
+% DATASRC_MASTERLOAD_ERROR %1
+An error was found in the zone data for a MasterFiles zone. The zone
+is not loaded. The specific error is shown in the message, and should
+be addressed.
+
 % DATASRC_MEM_ADD_RRSET adding RRset '%1/%2' into zone '%3'
 Debug information. An RRset is being added to the in-memory data source.
 

src/lib/datasrc/tests/client_list_unittest.cc

@@ -332,7 +332,7 @@ public:
                   shared_ptr<InMemoryClient>());
     }
     shared_ptr<TestedList> list_;
-    const ClientList::FindResult negativeResult_;
+    const ClientList::FindResult negative_result_;
     vector<shared_ptr<MockDataSourceClient> > ds_;
     vector<ConfigurableClientList::DataSourceInfo> ds_info_;
     const ConstElementPtr config_elem_, config_elem_zones_;
@@ -349,7 +349,7 @@ TEST_F(ListTest, selfTest) {
     EXPECT_EQ(result::NOTFOUND, ds_[0]->findZone(Name("zzz")).code);
     // Nothing to keep alive here.
     EXPECT_EQ(shared_ptr<ClientList::FindResult::LifeKeeper>(),
-                  negativeResult_.life_keeper_);
+                  negative_result_.life_keeper_);
 }
 
 // Test the list we create with empty configuration is, in fact, empty
@@ -365,14 +365,14 @@ TEST_F(ListTest, emptySearch) {
 
     // Note: we don't have operator<< for the result class, so we cannot use
     // EXPECT_EQ.  Same for other similar cases.
-    EXPECT_TRUE(negativeResult_ == list_->find(Name("example.org"), false,
-                                               false));
-    EXPECT_TRUE(negativeResult_ == list_->find(Name("example.org"), false,
-                                               true));
-    EXPECT_TRUE(negativeResult_ == list_->find(Name("example.org"), true,
-                                               false));
-    EXPECT_TRUE(negativeResult_ == list_->find(Name("example.org"), true,
-                                               true));
+    EXPECT_TRUE(negative_result_ == list_->find(Name("example.org"), false,
+                                                false));
+    EXPECT_TRUE(negative_result_ == list_->find(Name("example.org"), false,
+                                                true));
+    EXPECT_TRUE(negative_result_ == list_->find(Name("example.org"), true,
+                                                false));
+    EXPECT_TRUE(negative_result_ == list_->find(Name("example.org"), true,
+                                                true));
 }
 
 // Put a single data source inside the list and check it can find an
@@ -380,21 +380,21 @@ TEST_F(ListTest, emptySearch) {
 TEST_F(ListTest, singleDSExactMatch) {
     list_->getDataSources().push_back(ds_info_[0]);
     // This zone is not there
-    EXPECT_TRUE(negativeResult_ == list_->find(Name("org."), true));
+    EXPECT_TRUE(negative_result_ == list_->find(Name("org."), true));
     // But this one is, so check it.
     positiveResult(list_->find(Name("example.org"), true), ds_[0],
                    Name("example.org"), true, "Exact match");
     // When asking for a sub zone of a zone there, we get nothing
     // (we want exact match, this would be partial one)
-    EXPECT_TRUE(negativeResult_ == list_->find(Name("sub.example.org."),
-                                               true));
+    EXPECT_TRUE(negative_result_ == list_->find(Name("sub.example.org."),
+                                                true));
 }
 
 // When asking for a partial match, we get all that the exact one, but more.
 TEST_F(ListTest, singleDSBestMatch) {
     list_->getDataSources().push_back(ds_info_[0]);
     // This zone is not there
-    EXPECT_TRUE(negativeResult_ == list_->find(Name("org.")));
+    EXPECT_TRUE(negative_result_ == list_->find(Name("org.")));
     // But this one is, so check it.
     positiveResult(list_->find(Name("example.org")), ds_[0],
                    Name("example.org"), true, "Exact match");
@@ -417,7 +417,7 @@ TEST_F(ListTest, multiExactMatch) {
         SCOPED_TRACE(test_names[i]);
         multiConfiguration(i);
         // Something that is nowhere there
-        EXPECT_TRUE(negativeResult_ == list_->find(Name("org."), true));
+        EXPECT_TRUE(negative_result_ == list_->find(Name("org."), true));
         // This one is there exactly.
         positiveResult(list_->find(Name("example.org"), true), ds_[0],
                        Name("example.org"), true, "Exact match");
@@ -425,7 +425,7 @@ TEST_F(ListTest, multiExactMatch) {
         positiveResult(list_->find(Name("sub.example.org."), true), ds_[1],
                        Name("sub.example.org"), true, "Subdomain match");
         // But this one is in neither data source.
-        EXPECT_TRUE(negativeResult_ ==
+        EXPECT_TRUE(negative_result_ ==
                     list_->find(Name("sub.example.com."), true));
     }
 }
@@ -436,7 +436,7 @@ TEST_F(ListTest, multiBestMatch) {
         SCOPED_TRACE(test_names[i]);
         multiConfiguration(i);
         // Something that is nowhere there
-        EXPECT_TRUE(negativeResult_ == list_->find(Name("org.")));
+        EXPECT_TRUE(negative_result_ == list_->find(Name("org.")));
         // This one is there exactly.
         positiveResult(list_->find(Name("example.org")), ds_[0],
                        Name("example.org"), true, "Exact match");
@@ -712,7 +712,7 @@ TEST_F(ListTest, cacheZones) {
     positiveResult(list_->find(Name("sub.example.com.")), ds_[0],
                    Name("example.com."), false, "Subdomain of com", true);
     // For now, the ones not cached are ignored.
-    EXPECT_TRUE(negativeResult_ == list_->find(Name("example.cz.")));
+    EXPECT_TRUE(negative_result_ == list_->find(Name("example.cz.")));
 }
 
 // Check the caching handles misbehaviour from the data source and
@@ -786,6 +786,32 @@ TEST_F(ListTest, masterFiles) {
     EXPECT_EQ(0, list_->getDataSources().size());
 }
 
+TEST_F(ListTest, BadMasterFile) {
+    // Configure two zone correctly, and one with the wrong origin
+    // (resulting in an out-of-zone data error)
+    // Configuration should succeed, and the correct zones should
+    // be loaded. Neither the 'bad' origin or the zone it used
+    // should be loaded
+    const ConstElementPtr elem(Element::fromJSON("["
+        "{"
+        "   \"type\": \"MasterFiles\","
+        "   \"cache-enable\": true,"
+        "   \"params\": {"
+        "       \"example.com.\": \"" TEST_DATA_DIR "/example.com.flattened\","
+        "       \"foo.bar.\": \"" TEST_DATA_DIR "/example.org.nsec3-signed\","
+        "       \".\": \"" TEST_DATA_DIR "/root.zone\""
+        "   }"
+        "}]"));
+    list_->configure(elem, true);
+
+    positiveResult(list_->find(Name("example.com."), true), ds_[0],
+                   Name("example.com."), true, "example.com", true);
+    EXPECT_TRUE(negative_result_ == list_->find(Name("example.org."), true));
+    EXPECT_TRUE(negative_result_ == list_->find(Name("foo.bar"), true));
+    positiveResult(list_->find(Name(".")), ds_[0], Name("."), true, "root",
+                   true);
+}
+
 // Test we can reload a zone
 TEST_F(ListTest, reloadSuccess) {
     list_->configure(config_elem_zones_, true);

src/lib/datasrc/tests/testdata/example.com.flattened

@@ -0,0 +1,3 @@
+example.com.    3600    IN  SOA ns.example.com. admin.example.com. 1234 3600 1800 2419200 7200
+example.com.    3600    IN  NS ns.example.com.
+ns.example.com.	3600    IN  A 192.0.2.1

src/lib/dns/masterload.cc

@@ -75,23 +75,28 @@ masterLoad(const char* const filename, const Name& origin,
         isc_throw(MasterLoadError, "Failed to open master file: " <<
                   filename << ": " << strerror(errno));
     }
-    masterLoad(ifs, origin, zone_class, callback);
+    masterLoad(ifs, origin, zone_class, callback, filename);
     ifs.close();
 }
 
 void
 masterLoad(istream& input, const Name& origin, const RRClass& zone_class,
-           MasterLoadCallback callback)
+           MasterLoadCallback callback, const char* source)
 {
     RRsetPtr rrset;
     ConstRdataPtr prev_rdata;   // placeholder for special case of RRSIGs
     string line;
     unsigned int line_count = 1;
 
+    if (source == NULL) {
+        source = "<unknown>";
+    }
+
     do {
         getline(input, line);
         if (input.bad() || (input.fail() && !input.eof())) {
-            isc_throw(MasterLoadError, "Unexpectedly failed to read a line");
+            isc_throw(MasterLoadError, "Unexpectedly failed to read a line in "
+                                       << source);
         }
 
         // blank/comment lines should be simply skipped.
@@ -102,7 +107,8 @@ masterLoad(istream& input, const Name& origin, const RRClass& zone_class,
         // The line shouldn't have leading space (which means omitting the
         // owner name).
         if (isspace(line[0])) {
-            isc_throw(MasterLoadError, "Leading space at line " << line_count);
+            isc_throw(MasterLoadError, "Leading space in " << source
+                                       << " at line " << line_count);
         }
 
         // Parse a single RR
@@ -111,15 +117,15 @@ masterLoad(istream& input, const Name& origin, const RRClass& zone_class,
         stringbuf rdatabuf;
         iss >> owner_txt >> ttl_txt >> rrclass_txt >> rrtype_txt >> &rdatabuf;
         if (iss.bad() || iss.fail()) {
-            isc_throw(MasterLoadError, "Parse failure for a valid RR at line "
-                      << line_count);
+            isc_throw(MasterLoadError, "Parse failure for a valid RR in "
+                      << source << " at line " << line_count);
         }
 
         // This simple version doesn't support relative owner names with a
         // separate origin.
         if (owner_txt.empty() || *(owner_txt.end() - 1) != '.') {
-            isc_throw(MasterLoadError, "Owner name is not absolute at line "
-                      << line_count);
+            isc_throw(MasterLoadError, "Owner name is not absolute in "
+                      << source << " at line " << line_count);
         }
 
         // XXX: this part is a bit tricky (and less efficient).  We are going
@@ -151,8 +157,9 @@ masterLoad(istream& input, const Name& origin, const RRClass& zone_class,
                 rdata = createRdata(*rrtype, *rrclass, stripLine(rdtext, ex));
             }
         } catch (const Exception& ex) {
-            isc_throw(MasterLoadError, "Invalid RR text at line " << line_count
-                      << ": " << ex.what());
+            isc_throw(MasterLoadError, "Invalid RR text in " << source <<
+                                       " at line " << line_count << ": "
+                                       << ex.what());
         }
 
         // Origin related validation:
@@ -161,20 +168,22 @@ masterLoad(istream& input, const Name& origin, const RRClass& zone_class,
         const NameComparisonResult cmp_result = owner->compare(origin);
         if (cmp_result.getRelation() != NameComparisonResult::EQUAL &&
             cmp_result.getRelation() != NameComparisonResult::SUBDOMAIN) {
-            isc_throw(MasterLoadError, "Out-of-zone data at line "
-                      << line_count);
+            isc_throw(MasterLoadError, "Out-of-zone data for " << origin
+                                       << "/" << rrclass_txt << " in "
+                                       << source << " at line "
+                                       << line_count);
         }
         if (*rrtype == RRType::SOA() &&
             cmp_result.getRelation() != NameComparisonResult::EQUAL) {
-            isc_throw(MasterLoadError, "SOA not at top of zone at line "
-                      << line_count);
+            isc_throw(MasterLoadError, "SOA not at top of zone in "
+                      << source << " at line " << line_count);
         }
 
         // Reject RR class mismatching
         if (*rrclass != zone_class) {
             isc_throw(MasterLoadError, "RR class (" << rrclass_txt
                       << ") does not match the zone class (" << zone_class
-                      << ") at line " << line_count);
+                      << ") in " << source << " at line " << line_count);
         }
 
         // Everything is okay.  Now create/update RRset with the new RR.
@@ -209,5 +218,6 @@ masterLoad(istream& input, const Name& origin, const RRClass& zone_class,
         callback(rrset);
     }
 }
+
 } // namespace dns
 } // namespace isc

src/lib/dns/masterload.h

@@ -232,9 +232,15 @@ void masterLoad(const char* const filename, const Name& origin,
 /// \param zone_class The RR class of the zone.
 /// \param callback A callback functor or function that is to be called for
 /// each RRset.
+/// \param source A string to use in error messages if zone content is bad
+/// (e.g. the file name when reading from a file). If this value is NULL,
+/// or left out, the error will use the string '<unknown>'
 void masterLoad(std::istream& input, const Name& origin,
-                const RRClass& zone_class, MasterLoadCallback callback);
+                const RRClass& zone_class, MasterLoadCallback callback,
+                const char* source = NULL);
 }
+
+
 //@}
 }