Merge branch 'trac2993'

configure.ac

@@ -1257,6 +1257,7 @@ AC_CONFIG_FILES([Makefile
                  src/lib/python/isc/util/cio/tests/Makefile
                  src/lib/python/isc/datasrc/Makefile
                  src/lib/python/isc/datasrc/tests/Makefile
+                 src/lib/python/isc/datasrc/tests/testdata/Makefile
                  src/lib/python/isc/dns/Makefile
                  src/lib/python/isc/cc/Makefile
                  src/lib/python/isc/cc/cc_generated/Makefile

src/bin/auth/datasrc_clients_mgr.h

@@ -623,7 +623,7 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::getZoneWriter(
     datasrc::ConfigurableClientList::ZoneWriterPair writerpair;
     {
         typename MutexType::Locker locker(*map_mutex_);
-        writerpair = client_list.getCachedZoneWriter(origin);
+        writerpair = client_list.getCachedZoneWriter(origin, false);
     }
 
     switch (writerpair.first) {

src/lib/datasrc/client_list.cc

@@ -347,6 +347,7 @@ ConfigurableClientList::resetMemorySegment
 
 ConfigurableClientList::ZoneWriterPair
 ConfigurableClientList::getCachedZoneWriter(const Name& name,
+                                            bool catch_load_error,
                                             const std::string& datasrc_name)
 {
     if (!allow_cache_) {
@@ -385,7 +386,8 @@ ConfigurableClientList::getCachedZoneWriter(const Name& name,
                                ZoneWriterPtr(
                                    new memory::ZoneWriter(
                                        *info.ztable_segment_,
-                                       load_action, name, rrclass_, false))));
+                                       load_action, name, rrclass_,
+                                       catch_load_error))));
     }
 
     // We can't find the specified zone.  If a specific data source was

src/lib/datasrc/client_list.h

@@ -432,6 +432,8 @@ public:
     /// of the pair.
     ///
     /// \param zone The origin of the zone to load.
+    /// \param catch_load_errors Whether to make the zone writer catch
+    /// load errors (see \c ZoneWriter constructor documentation).
     /// \param datasrc_name If not empty, the name of the data source
     /// to be used for loading the zone (see above).
     /// \return The result has two parts. The first one is a status indicating
@@ -442,6 +444,7 @@ public:
     /// \throw DataSourceError or anything else that the data source
     ///      containing the zone might throw is propagated.
     ZoneWriterPair getCachedZoneWriter(const dns::Name& zone,
+                                       bool catch_load_error,
                                        const std::string& datasrc_name = "");
 
     /// \brief Implementation of the ClientList::find.

src/lib/datasrc/tests/client_list_unittest.cc

@@ -210,7 +210,7 @@ public:
                               config_ztable_segment);
 
             const ConfigurableClientList::ZoneWriterPair result =
-                list_->getCachedZoneWriter(zone, dsrc_info.name_);
+                list_->getCachedZoneWriter(zone, false, dsrc_info.name_);
 
             ASSERT_EQ(ConfigurableClientList::ZONE_SUCCESS, result.first);
             result.second->load();
@@ -1023,7 +1023,7 @@ TEST_P(ListTest, BadMasterFile) {
 ConfigurableClientList::CacheStatus
 ListTest::doReload(const Name& origin, const string& datasrc_name) {
     ConfigurableClientList::ZoneWriterPair
-        result(list_->getCachedZoneWriter(origin, datasrc_name));
+        result(list_->getCachedZoneWriter(origin, false, datasrc_name));
     if (result.first == ConfigurableClientList::ZONE_SUCCESS) {
         // Can't use ASSERT_NE here, it would want to return(), which
         // it can't in non-void function.
@@ -1041,6 +1041,59 @@ ListTest::doReload(const Name& origin, const string& datasrc_name) {
     return (result.first);
 }
 
+// Check that ZoneWriter doesn't throw when asked not to
+TEST_P(ListTest, checkZoneWriterCatchesExceptions) {
+    const ConstElementPtr config_elem_zones_(Element::fromJSON("["
+        "{"
+        "   \"type\": \"MasterFiles\","
+        "   \"params\": {"
+        "       \"example.edu\": \"" TEST_DATA_DIR "example.edu-broken\""
+        "    },"
+        "   \"cache-enable\": true"
+        "}]"));
+
+    list_->configure(config_elem_zones_, true);
+    ConfigurableClientList::ZoneWriterPair
+        result(list_->getCachedZoneWriter(Name("example.edu"), true));
+    ASSERT_EQ(ConfigurableClientList::ZONE_SUCCESS, result.first);
+    ASSERT_TRUE(result.second);
+
+    std::string error_msg;
+    // Because of the way we called getCachedZoneWriter() with
+    // catch_load_error=true, the following should not throw and must
+    // return an error message in error_msg.
+    EXPECT_NO_THROW(result.second->load(&error_msg));
+    EXPECT_FALSE(error_msg.empty());
+    result.second->cleanup();
+}
+
+// Check that ZoneWriter throws when asked to
+TEST_P(ListTest, checkZoneWriterThrows) {
+    const ConstElementPtr config_elem_zones_(Element::fromJSON("["
+        "{"
+        "   \"type\": \"MasterFiles\","
+        "   \"params\": {"
+        "       \"example.edu\": \"" TEST_DATA_DIR "example.edu-broken\""
+        "    },"
+        "   \"cache-enable\": true"
+        "}]"));
+
+    list_->configure(config_elem_zones_, true);
+    ConfigurableClientList::ZoneWriterPair
+        result(list_->getCachedZoneWriter(Name("example.edu"), false));
+    ASSERT_EQ(ConfigurableClientList::ZONE_SUCCESS, result.first);
+    ASSERT_TRUE(result.second);
+
+    std::string error_msg;
+    // Because of the way we called getCachedZoneWriter() with
+    // catch_load_error=false, the following should throw and must not
+    // modify error_msg.
+    EXPECT_THROW(result.second->load(&error_msg),
+                 isc::datasrc::ZoneLoaderException);
+    EXPECT_TRUE(error_msg.empty());
+    result.second->cleanup();
+}
+
 // Test we can reload a zone
 TEST_P(ListTest, reloadSuccess) {
     list_->configure(config_elem_zones_, true);

src/lib/python/isc/datasrc/configurableclientlist_inc.cc

@@ -65,7 +65,7 @@ Parameters:\n\
 ";
 
 const char* const ConfigurableClientList_get_cached_zone_writer_doc = "\
-get_cached_zone_writer(zone, datasrc_name) -> status, zone_writer\n\
+get_cached_zone_writer(zone, catch_load_error, datasrc_name) -> status, zone_writer\n\
 \n\
 This method returns a ZoneWriter that can be used to (re)load a zone.\n\
 \n\
@@ -89,6 +89,7 @@ the status is anything else, the second element is None.\n\
 \n\
 Parameters:\n\
   zone              The origin of the zone to (re)load.\n\
+  catch_load_error  Whether to catch load errors, or to raise them as exceptions.\n\
   datasrc_name      The name of the data source where the zone is to be loaded (optional).\n\
 ";
 

src/lib/python/isc/datasrc/configurableclientlist_python.cc

@@ -163,15 +163,17 @@ ConfigurableClientList_getCachedZoneWriter(PyObject* po_self, PyObject* args) {
         static_cast<s_ConfigurableClientList*>(po_self);
     try {
         PyObject* name_obj;
+        int catch_load_error;
         const char* datasrc_name_p = "";
-        if (PyArg_ParseTuple(args, "O!|s", &isc::dns::python::name_type,
+        if (PyArg_ParseTuple(args, "O!p|s", &isc::dns::python::name_type,
-                             &name_obj, &datasrc_name_p)) {
+                             &name_obj, &catch_load_error, &datasrc_name_p)) {
             const isc::dns::Name&
                 name(isc::dns::python::PyName_ToName(name_obj));
             const std::string datasrc_name(datasrc_name_p);
 
             const ConfigurableClientList::ZoneWriterPair result =
-                self->cppobj->getCachedZoneWriter(name, datasrc_name);
+                self->cppobj->getCachedZoneWriter(name, catch_load_error,
+                                                  datasrc_name);
 
             PyObjectContainer writer;
             if (!result.second) {

src/lib/python/isc/datasrc/tests/Makefile.am

@@ -1,17 +1,10 @@
+SUBDIRS = testdata
+
 PYCOVERAGE_RUN = @PYCOVERAGE_RUN@
 PYTESTS =  datasrc_test.py sqlite3_ds_test.py
 PYTESTS += clientlist_test.py zone_loader_test.py
 EXTRA_DIST = $(PYTESTS)
 
-EXTRA_DIST += testdata/brokendb.sqlite3
-EXTRA_DIST += testdata/example.com.sqlite3
-EXTRA_DIST += testdata/example.com.source.sqlite3
-EXTRA_DIST += testdata/newschema.sqlite3
-EXTRA_DIST += testdata/oldschema.sqlite3
-EXTRA_DIST += testdata/new_minor_schema.sqlite3
-EXTRA_DIST += testdata/example.com
-EXTRA_DIST += testdata/example.com.ch
-EXTRA_DIST += testdata/static.zone
 CLEANFILES = $(abs_builddir)/rwtest.sqlite3.copied
 CLEANFILES += $(abs_builddir)/zoneloadertest.sqlite3
 

src/lib/python/isc/datasrc/tests/clientlist_test.py

@@ -253,7 +253,9 @@ class ClientListTest(unittest.TestCase):
         self.clist.reset_memory_segment("MasterFiles",
                                         isc.datasrc.ConfigurableClientList.CREATE,
                                         map_params)
-        result, self.__zone_writer = self.clist.get_cached_zone_writer(isc.dns.Name("example.com"))
+        result, self.__zone_writer = \
+            self.clist.get_cached_zone_writer(isc.dns.Name("example.com"),
+                                              False)
         self.assertEqual(isc.datasrc.ConfigurableClientList.CACHE_STATUS_ZONE_SUCCESS,
                          result)
         err_msg = self.__zone_writer.load()
@@ -264,7 +266,9 @@ class ClientListTest(unittest.TestCase):
         self.clist.reset_memory_segment("MasterFiles",
                                         isc.datasrc.ConfigurableClientList.READ_ONLY,
                                         map_params)
-        result, self.__zone_writer = self.clist.get_cached_zone_writer(isc.dns.Name("example.com"))
+        result, self.__zone_writer = \
+            self.clist.get_cached_zone_writer(isc.dns.Name("example.com"),
+                                              False)
         self.assertEqual(isc.datasrc.ConfigurableClientList.CACHE_STATUS_CACHE_NOT_WRITABLE,
                          result)
 
@@ -280,7 +284,9 @@ class ClientListTest(unittest.TestCase):
         self.clist = isc.datasrc.ConfigurableClientList(isc.dns.RRClass.IN)
         self.configure_helper()
 
-        result, self.__zone_writer = self.clist.get_cached_zone_writer(isc.dns.Name("example.com"))
+        result, self.__zone_writer = \
+            self.clist.get_cached_zone_writer(isc.dns.Name("example.com"),
+                                              False)
         self.assertEqual(isc.datasrc.ConfigurableClientList.CACHE_STATUS_ZONE_SUCCESS,
                          result)
         err_msg = self.__zone_writer.load()
@@ -288,6 +294,30 @@ class ClientListTest(unittest.TestCase):
         self.assertRaises(isc.datasrc.Error, self.__zone_writer.load)
         self.__zone_writer.cleanup()
 
+    def test_zone_writer_load_without_raise(self):
+        """
+        Test that the zone writer does not throw when asked not to.
+        """
+
+        self.clist = isc.datasrc.ConfigurableClientList(isc.dns.RRClass.IN)
+        self.clist.configure('''[{
+            "type": "MasterFiles",
+            "params": {
+                "example.com": "''' + TESTDATA_PATH + '''example.com-broken.zone"
+            },
+            "cache-enable": true
+        }]''', True)
+
+        result, self.__zone_writer = \
+            self.clist.get_cached_zone_writer(isc.dns.Name("example.com"),
+                                              True)
+        self.assertEqual(isc.datasrc.ConfigurableClientList.CACHE_STATUS_ZONE_SUCCESS,
+                         result)
+        err_msg = self.__zone_writer.load()
+        self.assertIsNotNone(err_msg)
+        self.assertTrue('Errors found when validating zone' in err_msg)
+        self.__zone_writer.cleanup()
+
     def test_zone_writer_install_without_load(self):
         """
         Test that the zone writer throws when install() is called
@@ -297,7 +327,9 @@ class ClientListTest(unittest.TestCase):
         self.clist = isc.datasrc.ConfigurableClientList(isc.dns.RRClass.IN)
         self.configure_helper()
 
-        result, self.__zone_writer = self.clist.get_cached_zone_writer(isc.dns.Name("example.com"))
+        result, self.__zone_writer = \
+            self.clist.get_cached_zone_writer(isc.dns.Name("example.com"),
+                                              False)
         self.assertEqual(isc.datasrc.ConfigurableClientList.CACHE_STATUS_ZONE_SUCCESS,
                          result)
         self.assertRaises(isc.datasrc.Error, self.__zone_writer.install)

src/lib/python/isc/datasrc/tests/testdata/Makefile.am

@@ -0,0 +1,12 @@
+EXTRA_DIST = \
+	brokendb.sqlite3 \
+	example.com \
+	example.com-broken.zone \
+	example.com.ch \
+	example.com.source.sqlite3 \
+	example.com.sqlite3 \
+	Makefile.am \
+	new_minor_schema.sqlite3 \
+	newschema.sqlite3 \
+	oldschema.sqlite3 \
+	static.zone

src/lib/python/isc/datasrc/tests/testdata/example.com-broken.zone

@@ -0,0 +1,6 @@
+; This zone is broken (contains no NS records).
+example.com.         1000  IN  SOA a.dns.example.com. mail.example.com. 2 1 1 1 1
+a.dns.example.com.   1000  IN  A    1.1.1.1
+b.dns.example.com.   1000  IN  A    3.3.3.3
+b.dns.example.com.   1000  IN  AAAA 4:4::4:4
+b.dns.example.com.   1000  IN  AAAA 5:5::5:5

src/lib/python/isc/datasrc/zonewriter_inc.cc

@@ -46,11 +46,13 @@ the data actually served, it only prepares them for future use.\n\
 This is the first method you should call on the object. Never call it\n\
 multiple times.\n\
 \n\
-Depending on how the ZoneWriter was constructed, in case a load error\n\
+If the zone loads successfully, this method returns None. In the case of\n\
-happens, a string with the error message may be returned. When\n\
+a load error, if the ZoneWriter was constructed with the\n\
-ZoneWriter is not constructed to do that, in case of a load error, a\n\
+catch_load_error option (see\n\
-DataSourceError exception is raised. In all other cases, this method\n\
+ConfigurableClientList.get_cached_zone_writer()), this method will\n\
-returns None.\n\
+return an error message string; if it was created without the\n\
+catch_load_error option, this method will raise a DataSourceError\n\
+exception.\n\
 \n\
 Exceptions:\n\
   isc.InvalidOperation if called second time.\n\