[2833] consolidate some cache related config checks in ZoneTableConfig.

this eliminated the need for if-else cases in
ConfigurableClientList::configure().  to make it possible getDataSourceClient()
was slightly updated so that it recognizes "MasterFiles".

src/lib/datasrc/client_list.cc

@@ -120,57 +120,33 @@ ConfigurableClientList::configure(const ConstElementPtr& config,
                           << name);
             }
 
+            if (type == "MasterFiles" && !allow_cache) { // XXX type specific
+                // We're not going to load these zones. Issue warnings about
+                // it.
+                LOG_WARN(logger, DATASRC_LIST_NOT_CACHED).
+                            arg(name).arg(rrclass_);
+                continue;
+            }
+            // Create a client for the underling data source via factory.
+            // (If it's our internal type of data source, this is essentially
+            // no-op).
+            const DataSourcePair dsrc_pair = getDataSourceClient(type,
+                                                                 paramConf);
+
+            internal::ZoneTableConfig ztconfig(type, dsrc_pair.first, *dconf);
             shared_ptr<ZoneTableSegment> ztable_segment;
-            if (want_cache) {
-                // For now we use dummy config
-                internal::ZoneTableConfig ztconfig("MasterFiles", 0,
-                                                   *Element::fromJSON("{\"params\": {}}"));
+            if (ztconfig.isEnabled()) {
                 ztable_segment.reset(ZoneTableSegment::create(rrclass_,
                                                               ztconfig));
             }
-
-            if (type == "MasterFiles") {
-                // In case the cache is not allowed, we just skip the master
-                // files (at least for now)
-                if (!allow_cache) {
-                    // We're not going to load these zones. Issue warnings about it.
-                    const map<string, ConstElementPtr>
-                        zones_files(paramConf->mapValue());
-                    for (map<string, ConstElementPtr>::const_iterator
-                         it(zones_files.begin()); it != zones_files.end();
-                         ++it) {
-                        LOG_WARN(logger, DATASRC_LIST_NOT_CACHED).
-                            arg(it->first).arg(rrclass_);
-                    }
-                    continue;
-                }
-                if (!want_cache) {
-                    isc_throw(ConfigurationError, "The cache must be enabled "
-                              "for the MasterFiles type");
-                }
-                new_data_sources.push_back(DataSourceInfo(rrclass_,
-                                                          ztable_segment,
-                                                          true, name));
-            } else {
-                // Ask the factory to create the data source for us
-                const DataSourcePair ds(this->getDataSourceClient(type,
-                                                                  paramConf));
-                // And put it into the vector
-                new_data_sources.push_back(DataSourceInfo(ds.first, ds.second,
-                                                          want_cache, rrclass_,
-                                                          ztable_segment,
-                                                          name));
-            }
+            new_data_sources.push_back(DataSourceInfo(dsrc_pair.first,
+                                                      dsrc_pair.second,
+                                                      allow_cache &&
+                                                      ztconfig.isEnabled(),
+                                                      rrclass_, ztable_segment,
+                                                      name));
 
             if (want_cache) {
-                if (!dconf->contains("cache-zones") && type != "MasterFiles") {
-                    isc_throw(isc::NotImplemented, "Auto-detection of zones "
-                              "to cache is not yet implemented, supply "
-                              "cache-zones parameter");
-                    // TODO: Auto-detect list of all zones in the
-                    // data source.
-                }
-
                 // List the zones we are loading
                 vector<string> zones_origins;
                 if (type == "MasterFiles") {
@@ -193,9 +169,6 @@ ConfigurableClientList::configure(const ConstElementPtr& config,
                 const DataSourceClient* const
                     client(new_data_sources.back().data_src_client_);
 
-                // temporary, just validate it
-                internal::ZoneTableConfig ztconfig(type, client, *dconf);
-
                 for (vector<string>::const_iterator it(zones_origins.begin());
                      it != zones_origins.end(); ++it) {
                     const Name origin(*it);
@@ -239,6 +212,9 @@ ConfigurableClientList::configure(const ConstElementPtr& config,
     } catch (const TypeError& te) {
         isc_throw(ConfigurationError, "Malformed configuration at data source "
                   "no. " << i << ": " << te.what());
+    } catch (const internal::ZoneTableConfigError& ex) {
+        // convert to the "public" exception type.
+        isc_throw(ConfigurationError, ex.what());
     }
 }
 
@@ -482,6 +458,10 @@ ConfigurableClientList::getDataSourceClient(const string& type,
                                             const ConstElementPtr&
                                             configuration)
 {
+    if (type == "MasterFiles") {
+        return (DataSourcePair(0, DataSourceClientContainerPtr()));
+    }
+
     DataSourceClientContainerPtr
         container(new DataSourceClientContainer(type, configuration));
     return (DataSourcePair(&container->getInstance(), container));

src/lib/datasrc/client_list.h

@@ -441,6 +441,13 @@ public:
     /// Also, derived classes could want to create the data source clients
     /// in a different way, though inheriting this class is not recommended.
     ///
+    /// Some types of data sources can be internal to the \c ClientList
+    /// implementation and do not require a corresponding dynamic module
+    /// loaded via \c DataSourceClientContainer.  In such a case, this method
+    /// simply returns a pair of null pointers.  It will help the caller reduce
+    /// type dependent processing.  Currently, "MasterFiles" is considered to
+    /// be this type of data sources.
+    ///
     /// The parameters are the same as of the constructor.
     /// \return Pair containing both the data source client and the container.
     ///     The container might be NULL in the derived class, it is

src/lib/datasrc/static_datasrc_link.cc

@@ -46,7 +46,8 @@ public:
     ZoneTableSegmentStatic(const string& zone_file) :
         memory::ZoneTableSegment(RRClass::CH()),
         ztconfig_("MasterFiles", 0, *data::Element::fromJSON(
-                      "{\"params\": {\"BIND\": \"" + zone_file + "\"}}")),
+                      "{\"cache-enable\": true,"
+                      " \"params\": {\"BIND\": \"" + zone_file + "\"}}")),
         ztsegment_(memory::ZoneTableSegment::create(RRClass::CH(), ztconfig_))
     {}
 

src/lib/datasrc/tests/client_list_unittest.cc

@@ -67,6 +67,9 @@ public:
         if (type == "error") {
             isc_throw(DataSourceError, "The error data source type");
         }
+        if (type == "MasterFiles") {
+            return (DataSourcePair(0, DataSourceClientContainerPtr()));
+        }
         shared_ptr<MockDataSourceClient>
             ds(new MockDataSourceClient(type, configuration));
         // Make sure it is deleted when the test list is deleted.
@@ -117,7 +120,9 @@ public:
             "   \"params\": [\"example.org\", \"example.com\", "
             "                \"noiter.org\", \"null.org\"]"
             "}]")),
-        ztconfig_("MasterFiles", 0, *Element::fromJSON("{\"params\": {}}")),
+        ztconfig_("MasterFiles", 0,
+                  *Element::fromJSON("{\"cache-enable\": true,"
+                                     " \"params\": {}}")),
         ztable_segment_(ZoneTableSegment::create(rrclass_, ztconfig_))
     {
         for (size_t i(0); i < ds_count; ++ i) {

src/lib/datasrc/tests/memory/zone_table_segment_unittest.cc

@@ -32,7 +32,8 @@ namespace {
 class ZoneTableSegmentTest : public ::testing::Test {
 protected:
     ZoneTableSegmentTest() :
-        ztconf_("MasterFiles", 0, *Element::fromJSON("{\"params\": {}}")),
+        ztconf_("MasterFiles", 0, *Element::fromJSON("{\"cache-enable\": true,"
+                                                     " \"params\": {}}")),
         ztable_segment_(ZoneTableSegment::create(RRClass::IN(), ztconf_))
     {}
 
@@ -52,7 +53,8 @@ TEST_F(ZoneTableSegmentTest, create) {
 
     // Unknown types of segment are rejected.
     const isc::datasrc::internal::ZoneTableConfig bad_ztconf(
-        "MasterFiles", 0, *Element::fromJSON("{\"cache-type\": \"unknown\","
+        "MasterFiles", 0, *Element::fromJSON("{\"cache-enable\": true,"
+                                             " \"cache-type\": \"unknown\","
                                              " \"params\": {}}"));
     EXPECT_THROW(ZoneTableSegment::create(RRClass::IN(), bad_ztconf),
                  UnknownSegmentType);

src/lib/datasrc/tests/memory/zone_writer_unittest.cc

@@ -43,7 +43,8 @@ public:
         // FIXME: The NullElement probably isn't the best one, but we don't
         // know how the config will look, so it just fills the argument
         // (which is currently ignored)
-        ztconf_("MasterFiles", 0, *Element::fromJSON("{\"params\": {}}")),
+        ztconf_("MasterFiles", 0, *Element::fromJSON("{\"cache-enable\": true,"
+                                                     " \"params\": {}}")),
         segment_(ZoneTableSegment::create(RRClass::IN(), ztconf_)),
         writer_(new
             ZoneWriterLocal(dynamic_cast<ZoneTableSegmentLocal*>(segment_.

src/lib/datasrc/tests/zone_finder_context_unittest.cc

@@ -121,7 +121,8 @@ protected:
     ZoneFinderContextTest() :
         qclass_(RRClass::IN()), qzone_("example.org"),
         ztconfig_("MasterFiles", 0, *Element::fromJSON(
-                      "{\"params\": "
+                      "{\"cache-enable\": true,"
+                      " \"params\": "
                       " {\"" + qzone_.toText() + "\": "
                       "  \"" + TEST_ZONE_FILE + "\"}"
                       "}"))

src/lib/datasrc/tests/zone_loader_unittest.cc

@@ -305,7 +305,7 @@ protected:
 
         // (re)configure zone table, then (re)construct the in-memory client
         // with it.
-        string ztconf_txt = "{\"params\": {";
+        string ztconf_txt = "{\"cache-enable\": true, \"params\": {";
         if (filename) {
             ztconf_txt += "\"" + zone.toText() + "\": \"" + filename + "\"";
         }

src/lib/datasrc/tests/zone_table_config_unittest.cc

@@ -25,6 +25,7 @@ using namespace isc::data;
 using namespace isc::dns;
 using isc::datasrc::unittest::MockDataSourceClient;
 using isc::datasrc::internal::ZoneTableConfig;
+using isc::datasrc::internal::ZoneTableConfigError;
 
 namespace {
 
@@ -39,10 +40,12 @@ protected:
     ZoneTableConfigTest() :
         mock_client_(zones),
         master_config_(Element::fromJSON(
-                           "{\"params\": "
+                           "{\"cache-enable\": true,"
+                           " \"params\": "
                            "  {\".\": \"" TEST_DATA_DIR "/root.zone\"}"
                            "}")),
-        mock_config_(Element::fromJSON("{\"cache-zones\": [\".\"]}"))
+        mock_config_(Element::fromJSON("{\"cache-enable\": true,"
+                                       " \"cache-zones\": [\".\"]}"))
     {}
 
     MockDataSourceClient mock_client_;
@@ -64,7 +67,8 @@ TEST_F(ZoneTableConfigTest, constructMasterFiles) {
     // only check the size of getZoneConfig.  Note that the constructor
     // doesn't check if the file exists, so they can be anything.
     const ConstElementPtr config_elem_multi(
-        Element::fromJSON("{\"params\": "
+        Element::fromJSON("{\"cache-enable\": true,"
+                          " \"params\": "
                           "{\"example.com\": \"file1\","
                           " \"example.org\": \"file2\","
                           " \"example.info\": \"file3\"}"
@@ -74,36 +78,59 @@ TEST_F(ZoneTableConfigTest, constructMasterFiles) {
 
     // A bit unusual, but acceptable case: empty parameters, so no zones.
     EXPECT_TRUE(ZoneTableConfig("MasterFiles", 0,
-                                *Element::fromJSON("{\"params\": {}}")).
+                                *Element::fromJSON("{\"cache-enable\": true,"
+                                                   " \"params\": {}}")).
                 getZoneConfig().empty());
 }
 
 TEST_F(ZoneTableConfigTest, badConstructMasterFiles) {
     // no "params"
-    EXPECT_THROW(ZoneTableConfig("MasterFiles", 0, *Element::fromJSON("{}")),
+    EXPECT_THROW(ZoneTableConfig("MasterFiles", 0,
+                                 *Element::fromJSON("{\"cache-enable\": "
+                                                    "true}")),
+                 isc::data::TypeError);
+
+    // no "cache-enable"
+    EXPECT_THROW(ZoneTableConfig("MasterFiles", 0,
+                                 *Element::fromJSON("{\"params\": {}}")),
+                 ZoneTableConfigError);
+    // cache disabled for MasterFiles
+    EXPECT_THROW(ZoneTableConfig("MasterFiles", 0,
+                                 *Element::fromJSON("{\"cache-enable\": "
+                                                    " false,"
+                                                    " \"params\": {}}")),
+                 ZoneTableConfigError);
+    // type error for cache-enable
+    EXPECT_THROW(ZoneTableConfig("MasterFiles", 0,
+                                 *Element::fromJSON("{\"cache-enable\": 1,"
+                                                    " \"params\": {}}")),
                  isc::data::TypeError);
 
     // "params" is not a map
     EXPECT_THROW(ZoneTableConfig("MasterFiles", 0,
-                                 *Element::fromJSON("{\"params\": []}")),
+                                 *Element::fromJSON("{\"cache-enable\": true,"
+                                                    " \"params\": []}")),
                  isc::data::TypeError);
 
     // bogus zone name
     const ConstElementPtr bad_config(Element::fromJSON(
-                                         "{\"params\": "
+                                         "{\"cache-enable\": true,"
+                                         " \"params\": "
                                          "{\"bad..name\": \"file1\"}}"));
     EXPECT_THROW(ZoneTableConfig("MasterFiles", 0, *bad_config),
                  isc::dns::EmptyLabel);
 
     // file name is not a string
     const ConstElementPtr bad_config2(Element::fromJSON(
-                                          "{\"params\": {\".\": 1}}"));
+                                          "{\"cache-enable\": true,"
+                                          " \"params\": {\".\": 1}}"));
     EXPECT_THROW(ZoneTableConfig("MasterFiles", 0, *bad_config2),
                  isc::data::TypeError);
 
     // Specify data source client (must be null for MasterFiles)
     EXPECT_THROW(ZoneTableConfig("MasterFiles", &mock_client_,
-                                 *Element::fromJSON("{\"params\": {}}")),
+                                 *Element::fromJSON("{\"cache-enable\": true,"
+                                                    " \"params\": {}}")),
                  isc::InvalidParameter);
 }
 
@@ -115,10 +142,12 @@ TEST_F(ZoneTableConfigTest, constructWithMock) {
     EXPECT_EQ(1, ztconf.getZoneConfig().size());
     EXPECT_EQ(Name::ROOT_NAME(), ztconf.getZoneConfig().begin()->first);
     EXPECT_EQ("", ztconf.getZoneConfig().begin()->second);
+    EXPECT_TRUE(ztconf.isEnabled());
 
     // Configure with multiple zones.
     const ConstElementPtr config_elem_multi(
-        Element::fromJSON("{\"cache-zones\": "
+        Element::fromJSON("{\"cache-enable\": true,"
+                          " \"cache-zones\": "
                           "[\"example.com\", \"example.org\",\"example.info\"]"
                           "}"));
     EXPECT_EQ(3, ZoneTableConfig("mock", &mock_client_, *config_elem_multi).
@@ -126,35 +155,48 @@ TEST_F(ZoneTableConfigTest, constructWithMock) {
 
     // Empty
     EXPECT_TRUE(ZoneTableConfig("mock", &mock_client_,
-                                *Element::fromJSON("{\"cache-zones\": []}")).
+                                *Element::fromJSON("{\"cache-enable\": true,"
+                                                   " \"cache-zones\": []}")).
+                getZoneConfig().empty());
+
+    // disabled.  value of cache-zones are ignored.
+    const ConstElementPtr config_elem_disabled(
+        Element::fromJSON("{\"cache-enable\": false,"
+                          " \"cache-zones\": [\"example.com\"]}"));
+    EXPECT_TRUE(ZoneTableConfig("mock", &mock_client_, *config_elem_disabled).
                 getZoneConfig().empty());
 }
 
 TEST_F(ZoneTableConfigTest, badConstructWithMock) {
     // no "cache-zones" (may become valid in future, but for now "notimp")
     EXPECT_THROW(ZoneTableConfig("mock", &mock_client_,
-                                 *Element::fromJSON("{}")),
+                                 *Element::fromJSON(
+                                     "{\"cache-enable\": true}")),
                  isc::NotImplemented);
 
     // "cache-zones" is not a list
     EXPECT_THROW(ZoneTableConfig("mock", &mock_client_,
-                                 *Element::fromJSON("{\"cache-zones\": {}}")),
+                                 *Element::fromJSON("{\"cache-enable\": true,"
+                                                    " \"cache-zones\": {}}")),
                  isc::data::TypeError);
 
     // "cache-zone" entry is not a string
     EXPECT_THROW(ZoneTableConfig("mock", &mock_client_,
-                                 *Element::fromJSON("{\"cache-zones\": [1]}")),
+                                 *Element::fromJSON("{\"cache-enable\": true,"
+                                                    " \"cache-zones\": [1]}")),
                  isc::data::TypeError);
 
     // bogus zone name
     const ConstElementPtr bad_config(Element::fromJSON(
-                                         "{\"cache-zones\": [\"bad..\"]}"));
+                                         "{\"cache-enable\": true,"
+                                         " \"cache-zones\": [\"bad..\"]}"));
     EXPECT_THROW(ZoneTableConfig("mock", &mock_client_, *bad_config),
                  isc::dns::EmptyLabel);
 
     // duplicate zone name
     const ConstElementPtr dup_config(Element::fromJSON(
-                                         "{\"cache-zones\": "
+                                         "{\"cache-enable\": true,"
+                                         " \"cache-zones\": "
                                          " [\"example\", \"example\"]}"));
     EXPECT_THROW(ZoneTableConfig("mock", &mock_client_, *dup_config),
                  isc::InvalidParameter);
@@ -171,13 +213,15 @@ TEST_F(ZoneTableConfigTest, getSegmentType) {
                               *master_config_).getSegmentType());
 
     // If we explicitly configure it, that value should be used.
-    ConstElementPtr config(Element::fromJSON("{\"cache-type\": \"mapped\","
+    ConstElementPtr config(Element::fromJSON("{\"cache-enable\": true,"
+                                             " \"cache-type\": \"mapped\","
                                              " \"params\": {}}" ));
     EXPECT_EQ("mapped",
               ZoneTableConfig("MasterFiles", 0, *config).getSegmentType());
 
     // Wrong types: should be rejected at construction time
-    ConstElementPtr badconfig(Element::fromJSON("{\"cache-type\": 1,"
+    ConstElementPtr badconfig(Element::fromJSON("{\"cache-enable\": true,"
+                                                " \"cache-type\": 1,"
                                                 " \"params\": {}}"));
     EXPECT_THROW(ZoneTableConfig("MasterFiles", 0, *badconfig),
                  isc::data::TypeError);

src/lib/datasrc/zone_table_config.cc

@@ -29,6 +29,12 @@ namespace datasrc {
 namespace internal {
 
 namespace {
+bool
+getEnabledFromConf(const Element& conf) {
+    return (conf.contains("cache-enable") &&
+            conf.get("cache-enable")->boolValue());
+}
+
 std::string
 getSegmentTypeFromConf(const Element& conf) {
     // If cache-zones is not explicitly configured, use the default type.
@@ -43,6 +49,7 @@ getSegmentTypeFromConf(const Element& conf) {
 ZoneTableConfig::ZoneTableConfig(const std::string& datasrc_type,
                                  const DataSourceClient* datasrc_client,
                                  const Element& datasrc_conf) :
+    enabled_(getEnabledFromConf(datasrc_conf)),
     segment_type_(getSegmentTypeFromConf(datasrc_conf)),
     datasrc_client_(datasrc_client)
 {
@@ -50,12 +57,15 @@ ZoneTableConfig::ZoneTableConfig(const std::string& datasrc_type,
     if (!params) {
         params.reset(new NullElement());
     }
-
     if (datasrc_type == "MasterFiles") {
         if (datasrc_client) {
             isc_throw(isc::InvalidParameter,
                       "data source client is given for MasterFiles");
         }
+        if (!enabled_) {
+            isc_throw(ZoneTableConfigError,
+                      "The cache must be enabled for the MasterFiles type");
+        }
 
         typedef std::map<std::string, ConstElementPtr> ZoneToFile;
         const ZoneToFile& zone_to_file = params->mapValue();
@@ -72,6 +82,9 @@ ZoneTableConfig::ZoneTableConfig(const std::string& datasrc_type,
                       "data source client is missing for data source type: "
                       << datasrc_type);
         }
+        if (!enabled_) {
+            return;
+        }
 
         if (!datasrc_conf.contains("cache-zones")) {
             isc_throw(isc::NotImplemented, "Auto-detection of zones "

src/lib/datasrc/zone_table_config.h

@@ -15,6 +15,8 @@
 #ifndef DATASRC_ZONE_TABLE_CONFIG_H
 #define DATASRC_ZONE_TABLE_CONFIG_H
 
+#include <exceptions/exceptions.h>
+
 #include <dns/name.h>
 #include <cc/data.h>
 #include <datasrc/memory/load_action.h>
@@ -28,6 +30,14 @@ class DataSourceClient;
 
 namespace internal {
 
+/// \brief Exception thrown for configuration error related to zone table.
+class ZoneTableConfigError : public Exception {
+public:
+    ZoneTableConfigError(const char* file, size_t line, const char* what) :
+        Exception(file, line, what)
+    {}
+};
+
 /// This class is intended to be an interface between DataSourceClient and
 /// memory ZoneTableSegment implementations.  This class understands the
 /// configuration parameters for DataSourceClient related to in-memory cache,
@@ -50,7 +60,14 @@ public:
                     const DataSourceClient* datasrc_client,
                     const data::Element& datasrc_conf);
 
+    /// \brief Return if the cache is enabled.
+    ///
+    /// \throw None
+    bool isEnabled() const { return (enabled_); }
+
     /// \brief Return the memory segment type to be used for the zone table.
+    ///
+    /// \throw None
     const std::string& getSegmentType() const { return (segment_type_); }
 
     /// Return corresponding \c LoadAction for the given name of zone.
@@ -68,6 +85,7 @@ public:
     const Zones& getZoneConfig() const { return (zone_config_); }
 
 private:
+    const bool enabled_; // if the use of in-memory zone table is enabled
     const std::string segment_type_;
     // client of underlying data source, will be NULL for MasterFile datasrc
     const DataSourceClient* datasrc_client_;