[2833] tightened param validation in zt config ctor with dedicated tests.

src/lib/datasrc/client_list.cc

@@ -122,7 +122,6 @@ ConfigurableClientList::configure(const ConstElementPtr& config,
 
             shared_ptr<ZoneTableSegment> ztable_segment;
             if (want_cache) {
-                internal::ZoneTableConfig ztconfig(type, 0, *dconf);
                 ztable_segment.reset(ZoneTableSegment::create(*dconf, // XXX
                                                               rrclass_));
             }
@@ -190,6 +189,10 @@ ConfigurableClientList::configure(const ConstElementPtr& config,
                     cache(new_data_sources.back().cache_);
                 const DataSourceClient* const
                     client(new_data_sources.back().data_src_client_);
+
+                // temporary, just validate it
+                internal::ZoneTableConfig ztconfig(type, new_data_sources.back().data_src_client_, *dconf);
+
                 for (vector<string>::const_iterator it(zones_origins.begin());
                      it != zones_origins.end(); ++it) {
                     const Name origin(*it);

src/lib/datasrc/tests/Makefile.am

@@ -59,6 +59,7 @@ run_unittests_SOURCES += faked_nsec3.h faked_nsec3.cc
 run_unittests_SOURCES += client_list_unittest.cc
 run_unittests_SOURCES += master_loader_callbacks_test.cc
 run_unittests_SOURCES += zone_loader_unittest.cc
+run_unittests_SOURCES += zone_table_config_unittest.cc
 
 # We need the actual module implementation in the tests (they are not part
 # of libdatasrc)

src/lib/datasrc/tests/zone_table_config_unittest.cc

@@ -0,0 +1,167 @@
+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <datasrc/zone_table_config.h>
+#include <datasrc/tests/mock_client.h>
+
+#include <cc/data.h>
+#include <dns/name.h>
+
+#include <gtest/gtest.h>
+
+using namespace isc::datasrc;
+using namespace isc::data;
+using namespace isc::dns;
+using isc::datasrc::unittest::MockDataSourceClient;
+using isc::datasrc::internal::ZoneTableConfig;
+
+namespace {
+
+const char* zones[] = {
+    "example.org.",
+    "example.com.",
+    NULL
+};
+
+class ZoneTableConfigTest : public ::testing::Test {
+protected:
+    ZoneTableConfigTest() :
+        mock_client_(zones),
+        master_config_(Element::fromJSON(
+                           "{\"params\": "
+                           "  {\".\": \"" TEST_DATA_DIR "/root.zone\"}"
+                           "}")),
+        mock_config_(Element::fromJSON("{\"cache-zones\": [\".\"]}"))
+    {}
+
+    MockDataSourceClient mock_client_;
+    const ConstElementPtr master_config_; // valid config for MasterFiles
+    const ConstElementPtr mock_config_; // valid config for MasterFiles
+};
+
+TEST_F(ZoneTableConfigTest, constructMasterFiles) {
+    // A simple case: configuring a MasterFiles table with a single zone
+    const ZoneTableConfig ztconf("MasterFiles", 0, *master_config_);
+    // getZoneConfig() returns a map containing exactly one entry
+    // corresponding to the root zone information in the configuration.
+    EXPECT_EQ(1, ztconf.getZoneConfig().size());
+    EXPECT_EQ(Name::ROOT_NAME(), ztconf.getZoneConfig().begin()->first);
+    EXPECT_EQ(TEST_DATA_DIR "/root.zone",
+              ztconf.getZoneConfig().begin()->second);
+
+    // With multiple zones.  There shouldn't be anything special, so we
+    // 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\": "
+                          "{\"example.com\": \"file1\","
+                          " \"example.org\": \"file2\","
+                          " \"example.info\": \"file3\"}"
+                          "}"));
+    EXPECT_EQ(3, ZoneTableConfig("MasterFiles", 0, *config_elem_multi).
+              getZoneConfig().size());
+
+    // A bit unusual, but acceptable case: empty parameters, so no zones.
+    EXPECT_TRUE(ZoneTableConfig("MasterFiles", 0,
+                                *Element::fromJSON("{\"params\": {}}")).
+                getZoneConfig().empty());
+}
+
+TEST_F(ZoneTableConfigTest, badConstructMasterFiles) {
+    // no "params"
+    EXPECT_THROW(ZoneTableConfig("MasterFiles", 0, *Element::fromJSON("{}")),
+                 isc::data::TypeError);
+
+    // "params" is not a map
+    EXPECT_THROW(ZoneTableConfig("MasterFiles", 0,
+                                 *Element::fromJSON("{\"params\": []}")),
+                 isc::data::TypeError);
+
+    // bogus zone name
+    const ConstElementPtr bad_config(Element::fromJSON(
+                                         "{\"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}}"));
+    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\": {}}")),
+                 isc::InvalidParameter);
+}
+
+TEST_F(ZoneTableConfigTest, constructWithMock) {
+    // Performing equivalent set of tests as constructMasterFiles
+
+    // Configure with a single zone.
+    const ZoneTableConfig ztconf("mock", &mock_client_, *mock_config_);
+    EXPECT_EQ(1, ztconf.getZoneConfig().size());
+    EXPECT_EQ(Name::ROOT_NAME(), ztconf.getZoneConfig().begin()->first);
+    EXPECT_EQ("", ztconf.getZoneConfig().begin()->second);
+
+    // Configure with multiple zones.
+    const ConstElementPtr config_elem_multi(
+        Element::fromJSON("{\"cache-zones\": "
+                          "[\"example.com\", \"example.org\",\"example.info\"]"
+                          "}"));
+    EXPECT_EQ(3, ZoneTableConfig("mock", &mock_client_, *config_elem_multi).
+              getZoneConfig().size());
+
+    // Empty
+    EXPECT_TRUE(ZoneTableConfig("mock", &mock_client_,
+                                *Element::fromJSON("{\"cache-zones\": []}")).
+                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("{}")),
+                 isc::NotImplemented);
+
+    // "cache-zones" is not a list
+    EXPECT_THROW(ZoneTableConfig("mock", &mock_client_,
+                                 *Element::fromJSON("{\"cache-zones\": {}}")),
+                 isc::data::TypeError);
+
+    // "cache-zone" entry is not a string
+    EXPECT_THROW(ZoneTableConfig("mock", &mock_client_,
+                                 *Element::fromJSON("{\"cache-zones\": [1]}")),
+                 isc::data::TypeError);
+
+    // bogus zone name
+    const ConstElementPtr bad_config(Element::fromJSON(
+                                         "{\"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\": "
+                                         " [\"example\", \"example\"]}"));
+    EXPECT_THROW(ZoneTableConfig("mock", &mock_client_, *dup_config),
+                 isc::InvalidParameter);
+
+    // datasrc is null
+    EXPECT_THROW(ZoneTableConfig("mock", 0, *mock_config_),
+                 isc::InvalidParameter);
+}
+
+}

src/lib/datasrc/zone_table_config.cc

@@ -17,6 +17,7 @@
 #include <datasrc/memory/load_action.h>
 #include <dns/name.h>
 #include <cc/data.h>
+#include <exceptions/exceptions.h>
 
 #include <map>
 #include <string>
@@ -38,6 +39,11 @@ ZoneTableConfig::ZoneTableConfig(const std::string& datasrc_type,
     }
 
     if (datasrc_type == "MasterFiles") {
+        if (datasrc_client) {
+            isc_throw(isc::InvalidParameter,
+                      "data source client is given for MasterFiles");
+        }
+
         typedef std::map<std::string, ConstElementPtr> ZoneToFile;
         const ZoneToFile& zone_to_file = params->mapValue();
         ZoneToFile::const_iterator const it_end = zone_to_file.end();
@@ -48,6 +54,12 @@ ZoneTableConfig::ZoneTableConfig(const std::string& datasrc_type,
             zone_config_[dns::Name(it->first)] = it->second->stringValue();
         }
     } else {
+        if (!datasrc_client) {
+            isc_throw(isc::InvalidParameter,
+                      "data source client is missing for data source type: "
+                      << datasrc_type);
+        }
+
         if (!datasrc_conf.contains("cache-zones")) {
             isc_throw(isc::NotImplemented, "Auto-detection of zones "
                       "to cache is not yet implemented, supply "
@@ -56,10 +68,14 @@ ZoneTableConfig::ZoneTableConfig(const std::string& datasrc_type,
             // data source.
         }
 
-        // TBD: confirm cache-zones exist, must be a list, no duplicates
         const ConstElementPtr zones = datasrc_conf.get("cache-zones");
         for (size_t i = 0; i < zones->size(); ++i) {
-            zone_config_[dns::Name(zones->get(i)->stringValue())] = "";
+            const dns::Name zone_name(zones->get(i)->stringValue());
+            if (!zone_config_.insert(Zones::value_type(zone_name,
+                                                       "")).second) {
+                isc_throw(InvalidParameter, "Duplicate cache zone: " <<
+                          zone_name);
+            }
         }
     }
 }

src/lib/datasrc/zone_table_config.h

@@ -15,9 +15,9 @@
 #ifndef DATASRC_ZONE_TABLE_CONFIG_H
 #define DATASRC_ZONE_TABLE_CONFIG_H
 
-#include <datasrc/memory/load_action.h>
-#include <dns/dns_fwd.h>
+#include <dns/name.h>
 #include <cc/data.h>
+#include <datasrc/memory/load_action.h>
 
 #include <map>
 #include <string>
@@ -61,12 +61,13 @@ public:
     /// it's not clean in terms of encapsulation and performance (eventually
     /// we may have to look up in the underlying data source to get the list
     /// of zones, in which case constructing a map can be very expensive).
-    const std::map<dns::Name, std::string>& getZoneConfig() const;
+    typedef std::map<dns::Name, std::string> Zones;
+    const Zones& getZoneConfig() const { return (zone_config_); }
 
 private:
     // client of underlying data source, will be NULL for MasterFile datasrc
     DataSourceClient* datasrc_client_;
-    std::map<dns::Name, std::string> zone_config_;
+    Zones zone_config_;
 };
 }
 }