From aa6d3a22e29241cb6b8efb228821a99fd0b26240 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 27 Jun 2013 08:13:45 +0530 Subject: [PATCH 1/6] [2993] Untabify --- src/lib/datasrc/tests/client_list_unittest.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib/datasrc/tests/client_list_unittest.cc b/src/lib/datasrc/tests/client_list_unittest.cc index 36c50056f6..320833ccaa 100644 --- a/src/lib/datasrc/tests/client_list_unittest.cc +++ b/src/lib/datasrc/tests/client_list_unittest.cc @@ -1315,9 +1315,9 @@ ListTest::accessorIterate(const ConstZoneTableAccessorPtr& accessor, bool found = false; int i; for (i = 0; !it->isLast(); ++i, it->next()) { - if (Name(zoneName) == it->getCurrent().origin) { - found = true; - } + if (Name(zoneName) == it->getCurrent().origin) { + found = true; + } } EXPECT_EQ(i, numZones); if (numZones > 0) { From b178f2a38b94200e35758f6e8d7e570a0cfc41ec Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 27 Jun 2013 08:31:09 +0530 Subject: [PATCH 2/6] [2993] Extend ConfigurableClientList::getCachedZoneWriter() to have catch_load_error --- configure.ac | 1 + src/bin/auth/datasrc_clients_mgr.h | 2 +- src/lib/datasrc/client_list.cc | 4 +- src/lib/datasrc/client_list.h | 3 ++ src/lib/datasrc/tests/client_list_unittest.cc | 4 +- .../isc/datasrc/configurableclientlist_inc.cc | 3 +- .../datasrc/configurableclientlist_python.cc | 8 ++-- src/lib/python/isc/datasrc/tests/Makefile.am | 11 +---- .../isc/datasrc/tests/clientlist_test.py | 40 +++++++++++++++++-- .../isc/datasrc/tests/testdata/Makefile.am | 12 ++++++ .../tests/testdata/example.com-broken.zone | 6 +++ src/lib/python/isc/datasrc/zonewriter_inc.cc | 5 ++- 12 files changed, 76 insertions(+), 23 deletions(-) create mode 100644 src/lib/python/isc/datasrc/tests/testdata/Makefile.am create mode 100644 src/lib/python/isc/datasrc/tests/testdata/example.com-broken.zone diff --git a/configure.ac b/configure.ac index b01be12851..e5b9daca0f 100644 --- a/configure.ac +++ b/configure.ac @@ -1245,6 +1245,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 diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 7b40e1f160..11475e2fab 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -623,7 +623,7 @@ DataSrcClientsBuilderBase::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) { diff --git a/src/lib/datasrc/client_list.cc b/src/lib/datasrc/client_list.cc index 8a609af934..4fe1eb3741 100644 --- a/src/lib/datasrc/client_list.cc +++ b/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 diff --git a/src/lib/datasrc/client_list.h b/src/lib/datasrc/client_list.h index ad18f20e13..e4df415aec 100644 --- a/src/lib/datasrc/client_list.h +++ b/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. diff --git a/src/lib/datasrc/tests/client_list_unittest.cc b/src/lib/datasrc/tests/client_list_unittest.cc index 320833ccaa..04c3069f63 100644 --- a/src/lib/datasrc/tests/client_list_unittest.cc +++ b/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. diff --git a/src/lib/python/isc/datasrc/configurableclientlist_inc.cc b/src/lib/python/isc/datasrc/configurableclientlist_inc.cc index 9f26d13519..b16a6bb90a 100644 --- a/src/lib/python/isc/datasrc/configurableclientlist_inc.cc +++ b/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\ "; diff --git a/src/lib/python/isc/datasrc/configurableclientlist_python.cc b/src/lib/python/isc/datasrc/configurableclientlist_python.cc index ba020eb559..5e56b18c94 100644 --- a/src/lib/python/isc/datasrc/configurableclientlist_python.cc +++ b/src/lib/python/isc/datasrc/configurableclientlist_python.cc @@ -163,15 +163,17 @@ ConfigurableClientList_getCachedZoneWriter(PyObject* po_self, PyObject* args) { static_cast(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, - &name_obj, &datasrc_name_p)) { + if (PyArg_ParseTuple(args, "O!p|s", &isc::dns::python::name_type, + &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) { diff --git a/src/lib/python/isc/datasrc/tests/Makefile.am b/src/lib/python/isc/datasrc/tests/Makefile.am index 97fc2b6ff0..256ca6259d 100644 --- a/src/lib/python/isc/datasrc/tests/Makefile.am +++ b/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 diff --git a/src/lib/python/isc/datasrc/tests/clientlist_test.py b/src/lib/python/isc/datasrc/tests/clientlist_test.py index 774e5924aa..e8056a5180 100644 --- a/src/lib/python/isc/datasrc/tests/clientlist_test.py +++ b/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) diff --git a/src/lib/python/isc/datasrc/tests/testdata/Makefile.am b/src/lib/python/isc/datasrc/tests/testdata/Makefile.am new file mode 100644 index 0000000000..8365a24f17 --- /dev/null +++ b/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 diff --git a/src/lib/python/isc/datasrc/tests/testdata/example.com-broken.zone b/src/lib/python/isc/datasrc/tests/testdata/example.com-broken.zone new file mode 100644 index 0000000000..079b400dda --- /dev/null +++ b/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 diff --git a/src/lib/python/isc/datasrc/zonewriter_inc.cc b/src/lib/python/isc/datasrc/zonewriter_inc.cc index efe1144ccc..5d862b3601 100644 --- a/src/lib/python/isc/datasrc/zonewriter_inc.cc +++ b/src/lib/python/isc/datasrc/zonewriter_inc.cc @@ -46,8 +46,9 @@ 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\ -happens, a string with the error message may be returned. When\n\ +Depending on how the ZoneWriter was constructed (see catch_load_error\n\ +argument to ConfigurableClientList.get_cached_zone_writer()), in case a\n\ +load error happens, a string with the error message may be returned. When\n\ ZoneWriter is not constructed to do that, in case of a load error, a\n\ DataSourceError exception is raised. In all other cases, this method\n\ returns None.\n\ From d399683b0d058126c2889d090b6067dcac0de859 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 27 Jun 2013 08:59:00 +0530 Subject: [PATCH 3/6] [2993] Add unittest for the case where ZoneWriter encounters a bad zone and must not throw --- src/lib/datasrc/tests/client_list_unittest.cc | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/lib/datasrc/tests/client_list_unittest.cc b/src/lib/datasrc/tests/client_list_unittest.cc index 04c3069f63..90dbf3f250 100644 --- a/src/lib/datasrc/tests/client_list_unittest.cc +++ b/src/lib/datasrc/tests/client_list_unittest.cc @@ -1041,6 +1041,31 @@ 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; + // This 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(); +} + // Test we can reload a zone TEST_P(ListTest, reloadSuccess) { list_->configure(config_elem_zones_, true); From 204733844d2984ed49eb8e2da89666f6a1729732 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 27 Jun 2013 09:00:47 +0530 Subject: [PATCH 4/6] [2993] Update comment --- src/lib/datasrc/tests/client_list_unittest.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/lib/datasrc/tests/client_list_unittest.cc b/src/lib/datasrc/tests/client_list_unittest.cc index 90dbf3f250..602fa73afd 100644 --- a/src/lib/datasrc/tests/client_list_unittest.cc +++ b/src/lib/datasrc/tests/client_list_unittest.cc @@ -1059,8 +1059,9 @@ TEST_P(ListTest, checkZoneWriterCatchesExceptions) { ASSERT_TRUE(result.second); std::string error_msg; - // This should not throw and must return an error message in - // 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(); From b4f1cd645efbda886e123943b5e14222b40e7a9c Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Thu, 27 Jun 2013 09:03:40 +0530 Subject: [PATCH 5/6] [2993] Add unittest for the case where ZoneWriter encounters a bad zone and must throw --- src/lib/datasrc/tests/client_list_unittest.cc | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/lib/datasrc/tests/client_list_unittest.cc b/src/lib/datasrc/tests/client_list_unittest.cc index 602fa73afd..eb69556b8a 100644 --- a/src/lib/datasrc/tests/client_list_unittest.cc +++ b/src/lib/datasrc/tests/client_list_unittest.cc @@ -1067,6 +1067,33 @@ TEST_P(ListTest, checkZoneWriterCatchesExceptions) { 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); From 0d3ac3452be35e57346715a296e05deb48a03ccd Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Fri, 28 Jun 2013 06:09:25 +0530 Subject: [PATCH 6/6] [2993] Update Python documentation for ZoneWriter's load() method --- src/lib/python/isc/datasrc/zonewriter_inc.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/lib/python/isc/datasrc/zonewriter_inc.cc b/src/lib/python/isc/datasrc/zonewriter_inc.cc index 5d862b3601..1f10a9a475 100644 --- a/src/lib/python/isc/datasrc/zonewriter_inc.cc +++ b/src/lib/python/isc/datasrc/zonewriter_inc.cc @@ -46,12 +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 (see catch_load_error\n\ -argument to ConfigurableClientList.get_cached_zone_writer()), in case a\n\ -load error happens, a string with the error message may be returned. When\n\ -ZoneWriter is not constructed to do that, in case of a load error, a\n\ -DataSourceError exception is raised. In all other cases, this method\n\ -returns None.\n\ +If the zone loads successfully, this method returns None. In the case of\n\ +a load error, if the ZoneWriter was constructed with the\n\ +catch_load_error option (see\n\ +ConfigurableClientList.get_cached_zone_writer()), this method will\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\