[2178] better error log on masterfile parse problems

- add zone and file names to masterload errors
- catch masterload error and log it
- don't quit auth completely on a single zone load failure

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>
@@ -139,28 +140,34 @@ ConfigurableClientList::configure(const ConstElementPtr& config,
                     client(new_data_sources.back().data_src_client_);
                 for (vector<string>::const_iterator it(zones_origins.begin());
                      it != zones_origins.end(); ++it) {
-                    const Name origin(*it);
-                    shared_ptr<InMemoryZoneFinder>
-                        finder(new
-                            InMemoryZoneFinder(rrclass_, origin));
-                    if (type == "MasterFiles") {
-                        finder->load(paramConf->get(*it)->stringValue());
-                    } else {
-                        ZoneIteratorPtr iterator;
-                        try {
-                            iterator = client->getIterator(origin);
+                    try {
+                        const Name origin(*it);
+                        shared_ptr<InMemoryZoneFinder>
+                            finder(new
+                                InMemoryZoneFinder(rrclass_, origin));
+                        if (type == "MasterFiles") {
+                            finder->load(paramConf->get(*it)->stringValue());
+                        } else {
+                            ZoneIteratorPtr iterator;
+                            try {
+                                iterator = client->getIterator(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);
+                            }
+                            finder->load(*iterator);
                         }
-                        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);
-                        }
-                        finder->load(*iterator);
+                        cache->addZone(finder);
+                    } catch (const isc::dns::MasterLoadError& mle) {
+                        LOG_ERROR(logger, DATASRC_MASTERLOAD_ERROR)
+                            .arg(mle.what());
                     }
-                    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

@@ -786,6 +786,28 @@ TEST_F(ListTest, masterFiles) {
     EXPECT_EQ(0, list_->getDataSources().size());
 }
 
+TEST_F(ListTest, BadMasterFile) {
+    // Configure one zone correctly, and one with the wrong origin
+    // (resulting in out-of-zone data)
+    // Configuration should succeed, and the correct zone should
+    // be loaded.
+    const ConstElementPtr elem(Element::fromJSON("["
+        "{"
+        "   \"type\": \"MasterFiles\","
+        "   \"cache-enable\": true,"
+        "   \"params\": {"
+        "       \"foo.bar.\": \"" TEST_DATA_DIR "/example.org\","
+        "       \".\": \"" TEST_DATA_DIR "/root.zone\""
+        "   }"
+        "}]"));
+    list_->configure(elem, true);
+
+    EXPECT_TRUE(negativeResult_ == list_->find(Name("example.org."), true));
+    EXPECT_TRUE(negativeResult_ == list_->find(Name("foo.bar"), true));
+    positiveResult(list_->find(Name(".")), ds_[0], Name("."), true, "com",
+                   true);
+}
+
 // Test we can reload a zone
 TEST_F(ListTest, reloadSuccess) {
     list_->configure(config_elem_zones_, true);

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);
 }
+
+
 //@}
 }