From baa0674048557fa39d99026088439f071c28d548 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 7 May 2014 15:08:01 -0400 Subject: [PATCH 1/9] [3268] Treat top-level scalars as a group of globals parameters Restructured DCfgMgrBase to group the top level elements in a configuration into scalars (strings, bools, ints, etc...) and objects (maps, lists, etc), and parse the scalars first, then objects. This permits the top level scalars to be treated as a group of global parameters that are parsed first. Ordered parsing is now relegated to only object elements. Scalars are parsed first before any objects. Also added the ability to reset config manager's context and rather than than starting configuration parsing with a copy of the current context, it starts with an empty context. Modified unit tests accordingly. --- src/bin/d2/d_cfg_mgr.cc | 74 ++++++++++++-- src/bin/d2/d_cfg_mgr.h | 65 ++++++++++-- src/bin/d2/tests/d_cfg_mgr_unittests.cc | 127 +++++++++++++++++------- src/bin/d2/tests/d_test_stubs.cc | 70 +++++++------ src/bin/d2/tests/d_test_stubs.h | 58 ++++++----- 5 files changed, 281 insertions(+), 113 deletions(-) diff --git a/src/bin/d2/d_cfg_mgr.cc b/src/bin/d2/d_cfg_mgr.cc index 1e6bb57bfb..6be057eaca 100644 --- a/src/bin/d2/d_cfg_mgr.cc +++ b/src/bin/d2/d_cfg_mgr.cc @@ -97,15 +97,28 @@ DCfgContextBase::~DCfgContextBase() { // *********************** DCfgMgrBase ************************* DCfgMgrBase::DCfgMgrBase(DCfgContextBasePtr context) - : parse_order_(), context_(context) { - if (!context_) { - isc_throw(DCfgMgrBaseError, "DCfgMgrBase ctor: context cannot be NULL"); - } + : parse_order_() { + setContext(context); } DCfgMgrBase::~DCfgMgrBase() { } +void +DCfgMgrBase::resetContext() { + DCfgContextBasePtr context = createNewContext(); + setContext(context); +} + +void +DCfgMgrBase::setContext(DCfgContextBasePtr& context) { + if (!context) { + isc_throw(DCfgMgrBaseError, "DCfgMgrBase: context cannot be NULL"); + } + + context_ = context; +} + isc::data::ConstElementPtr DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { LOG_DEBUG(dctl_logger, DBGLVL_COMMAND, @@ -122,7 +135,9 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { // inconsistency if the parsing operation fails after the context has been // modified. We need to preserve the original context here // so as we can rollback changes when an error occurs. - DCfgContextBasePtr original_context = context_->clone(); +// DCfgContextBasePtr original_context = context_->clone(); + DCfgContextBasePtr original_context = context_; + resetContext(); // Answer will hold the result returned to the caller. ConstElementPtr answer; @@ -131,15 +146,43 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { std::string element_id; try { - // Grab a map of element_ids and their data values from the new - // configuration set. - const std::map& values_map = - config_set->mapValue(); + // Split the configuration into two maps. The first containing only + // top-level scalar parameters (i.e. globals), the second containing + // non-scalar or object elements (maps, lists, etc...). This allows + // us to parse and validate all of the global values before we do + // objects which may depend on them. + ElementMap params_map; + ElementMap objects_map; + + isc::dhcp::ConfigPair config_pair; + BOOST_FOREACH(config_pair, config_set->mapValue()) { + std::string element_id = config_pair.first; + isc::data::ConstElementPtr element = config_pair.second; + switch (element->getType()) { + case isc::data::Element::integer: + case isc::data::Element::real: + case isc::data::Element::boolean: + case isc::data::Element::string: + params_map[element_id] = element; + break; + default: + objects_map[element_id] = element; + break; + } + } + + // Parse the global, scalar parameters. These are "committed" to + // the context to make them available during object parsing. + boost::shared_ptr params_config(new MapElement()); + params_config->setValue(params_map); + buildParams(params_config); + + // Now parse the configuration objects. + const ElementMap& values_map = objects_map; // Use a pre-ordered list of element ids to parse the elements in a // specific order if the list (parser_order_) is not empty; otherwise // elements are parsed in the order the value_map presents them. - if (!parse_order_.empty()) { // For each element_id in the parse order list, look for it in the // value map. If the element exists in the map, pass it and it's @@ -207,6 +250,17 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { return (answer); } +void +DCfgMgrBase::buildParams(isc::data::ConstElementPtr params_config) { + // Loop through scalars parsing them and committing them to storage. + BOOST_FOREACH(dhcp::ConfigPair param, params_config->mapValue()) { + // Call derivation's method to create the proper parser. + dhcp::ParserPtr parser(createConfigParser(param.first)); + parser->build(param.second); + parser->commit(); + } +} + void DCfgMgrBase::buildAndCommit(std::string& element_id, isc::data::ConstElementPtr value) { // Call derivation's implementation to create the appropriate parser diff --git a/src/bin/d2/d_cfg_mgr.h b/src/bin/d2/d_cfg_mgr.h index a0bd9bb267..92468835a4 100644 --- a/src/bin/d2/d_cfg_mgr.h +++ b/src/bin/d2/d_cfg_mgr.h @@ -25,6 +25,9 @@ namespace isc { namespace d2 { +/// @brief Defines a map of ConstElementPtrs keyed by name +typedef std::map ElementMap; + /// @brief Exception thrown if the configuration manager encounters an error. class DCfgMgrBaseError : public isc::Exception { public: @@ -113,7 +116,7 @@ public: /// into parsers. /// /// @return returns a pointer to the Boolean Storage. - isc::dhcp::BooleanStoragePtr getBooleanStorage() { + isc::dhcp::BooleanStoragePtr& getBooleanStorage() { return (boolean_values_); } @@ -121,7 +124,7 @@ public: /// into parsers. /// /// @return returns a pointer to the uint32 Storage. - isc::dhcp::Uint32StoragePtr getUint32Storage() { + isc::dhcp::Uint32StoragePtr& getUint32Storage() { return (uint32_values_); } @@ -129,7 +132,7 @@ public: /// into parsers. /// /// @return returns a pointer to the string Storage. - isc::dhcp::StringStoragePtr getStringStorage() { + isc::dhcp::StringStoragePtr& getStringStorage() { return (string_values_); } @@ -184,6 +187,9 @@ private: /// @brief Defines an unsorted, list of string Element IDs. typedef std::vector ElementIdList; +/// @brief Defines an unsorted, list of string Element IDs. +typedef std::vector ElementIdList; + /// @brief Configuration Manager /// /// DCfgMgrBase is an abstract class that provides the mechanisms for managing @@ -198,7 +204,16 @@ typedef std::vector ElementIdList; /// /// @code /// make backup copy of configuration context -/// for each top level element in new configuration +/// Split top-level configuration elements into to sets: +/// 1. Set of scalar elements (strings, booleans, ints, etc..) +/// 2. Set of object elements (maps, lists, etc...) +/// For each entry in the scalar set: +/// get derivation-specific parser for element +/// run parser +/// update context with parsed results +/// break on error +/// +/// For each entry in the object set; /// get derivation-specific parser for element /// run parser /// update context with parsed results @@ -208,9 +223,9 @@ typedef std::vector ElementIdList; /// restore configuration context from backup /// @endcode /// -/// After making a backup of the current context, it iterates over the top-level -/// elements in the new configuration. The order in which the elements are -/// processed is either: +/// The above structuring ensures that global parameters are parsed first +/// making them available during subsequent object element parsing. The order +/// in which the object elements are processed is either: /// /// 1. Natural order presented by the configuration set /// 2. Specific order determined by a list of element ids @@ -256,9 +271,10 @@ public: /// @brief Adds a given element id to the end of the parse order list. /// - /// The order in which elements are retrieved from this is the order in - /// which they are added to the list. Derivations should use this method - /// to populate the parse order as part of their constructor. + /// The order in which object elements are retrieved from this is the + /// order in which they are added to the list. Derivations should use this + /// method to populate the parse order as part of their constructor. + /// Scalar parameters should NOT be included in this list. /// /// @param element_id is the string name of the element as it will appear /// in the configuration set. @@ -281,6 +297,20 @@ public: } protected: + /// @brief Parses a set of scalar configuration elements into global + /// parameters + /// + /// For each scalar element in the set: + /// - create a parser for the element + /// - invoke the parser's build method + /// - invoke the parser's commit method + /// + /// This will commit the values to context storage making them accessible + /// during object parsing. + /// + /// @param params_config set of scalar configuration elements to parse + virtual void buildParams(isc::data::ConstElementPtr params_config); + /// @brief Create a parser instance based on an element id. /// /// Given an element_id returns an instance of the appropriate parser. @@ -295,6 +325,21 @@ protected: virtual isc::dhcp::ParserPtr createConfigParser(const std::string& element_id) = 0; + /// @brief Abstract factory which creates a context instance. + /// + /// @return Returns a DCfgContextBasePtr to the new context instance. + virtual DCfgContextBasePtr createNewContext() = 0; + + /// @brief Replaces existing context with a new, emtpy context. + void resetContext(); + + /// @brief Update the current context. + /// + /// Replaces the existing context with the given context. + /// @param context Pointer to the new context. + /// @throw DCfgMgrBaseError if context is NULL. + void setContext(DCfgContextBasePtr& context); + private: /// @brief Parse a configuration element. diff --git a/src/bin/d2/tests/d_cfg_mgr_unittests.cc b/src/bin/d2/tests/d_cfg_mgr_unittests.cc index 512b896bed..235e954e78 100644 --- a/src/bin/d2/tests/d_cfg_mgr_unittests.cc +++ b/src/bin/d2/tests/d_cfg_mgr_unittests.cc @@ -18,6 +18,7 @@ #include #include +#include #include #include @@ -45,6 +46,11 @@ public: virtual ~DCtorTestCfgMgr() { } + /// @brief Dummy implementation as this method is abstract. + virtual DCfgContextBasePtr createNewContext() { + return (DCfgContextBasePtr()); + } + /// @brief Dummy implementation as this method is abstract. virtual isc::dhcp::ParserPtr createConfigParser(const std::string& /* element_id */) { @@ -112,7 +118,7 @@ TEST(DCfgMgrBase, construction) { /// 4. An unknown element error is handled. TEST_F(DStubCfgMgrTest, basicParseTest) { // Create a simple configuration. - string config = "{ \"test-value\": 1000 } "; + string config = "{ \"test-value\": [] } "; ASSERT_TRUE(fromJSON(config)); // Verify that we can parse a simple configuration. @@ -151,6 +157,9 @@ TEST_F(DStubCfgMgrTest, parseOrderTest) { std::string charlie("charlie"); std::string bravo("bravo"); std::string alpha("alpha"); + std::string string_test("string_test"); + std::string uint32_test("uint32_test"); + std::string bool_test("bool_test"); // Create the test configuration with the elements in "random" order. @@ -158,9 +167,15 @@ TEST_F(DStubCfgMgrTest, parseOrderTest) { // are in lexical order by element_id. This means that iterating over // such an element set, will present the elements in lexical order. Should // this change, this test will need to be modified accordingly. - string config = "{ \"bravo\": 2, " - " \"alpha\": 1, " - " \"charlie\": 3 } "; + string config = "{" + " \"string_test\": \"hoopla\", " + " \"bravo\": [], " + " \"uint32_test\": 55, " + " \"alpha\": {}, " + " \"charlie\": [], " + " \"bool_test\": true " + "} "; + ASSERT_TRUE(fromJSON(config)); // Verify that non-ordered parsing, results in an as-they-come parse order. @@ -169,6 +184,13 @@ TEST_F(DStubCfgMgrTest, parseOrderTest) { // present the elements in lexical order. Should this change, the expected // order list below would need to be changed accordingly). ElementIdList order_expected; + + // scalar params should be first and lexically + order_expected.push_back(bool_test); + order_expected.push_back(string_test); + order_expected.push_back(uint32_test); + + // objects second and lexically order_expected.push_back(alpha); order_expected.push_back(bravo); order_expected.push_back(charlie); @@ -183,6 +205,9 @@ TEST_F(DStubCfgMgrTest, parseOrderTest) { // Verify that the parsed order matches what we expected. EXPECT_TRUE(cfg_mgr_->parsed_order_ == order_expected); + for (int i = 0; i < cfg_mgr_->parsed_order_.size(); ++i) { + std::cout << i << ":" << cfg_mgr_->parsed_order_[i] << std::endl; + } // Clear the manager's parse order "memory". cfg_mgr_->parsed_order_.clear(); @@ -212,8 +237,20 @@ TEST_F(DStubCfgMgrTest, parseOrderTest) { answer_ = cfg_mgr_->parseConfig(config_set_); EXPECT_TRUE(checkAnswer(0)); + // Build expected order + // primitives should be first and lexically + order_expected.clear(); + order_expected.push_back(bool_test); + order_expected.push_back(string_test); + order_expected.push_back(uint32_test); + + // objects second and by the parse order + order_expected.push_back(charlie); + order_expected.push_back(bravo); + order_expected.push_back(alpha); + // Verify that the parsed order is the order we configured. - EXPECT_TRUE(cfg_mgr_->getParseOrder() == cfg_mgr_->parsed_order_); + EXPECT_TRUE(cfg_mgr_->parsed_order_ == order_expected); // Create a parse order list that has too many entries. Verify that // when parsing the test config, it fails. @@ -233,24 +270,24 @@ TEST_F(DStubCfgMgrTest, parseOrderTest) { /// 1. Boolean parameters can be parsed and retrieved. /// 2. Uint32 parameters can be parsed and retrieved. /// 3. String parameters can be parsed and retrieved. -/// 4. Derivation-specific parameters can be parsed and retrieved. -/// 5. Parsing a second configuration, updates the existing context values +/// 4. Map elements can be parsed and retrieved. +/// 5. List elements can be parsed and retrieved. +/// 6. Parsing a second configuration, updates the existing context values /// correctly. TEST_F(DStubCfgMgrTest, simpleTypesTest) { - // Fetch a derivation specific pointer to the context. - DStubContextPtr context = getStubContext(); - ASSERT_TRUE(context); - // Create a configuration with all of the parameters. string config = "{ \"bool_test\": true , " " \"uint32_test\": 77 , " " \"string_test\": \"hmmm chewy\" , " - " \"extra_test\": 430 } "; + " \"map_test\" : {} , " + " \"list_test\": [] }"; ASSERT_TRUE(fromJSON(config)); // Verify that the configuration parses without error. answer_ = cfg_mgr_->parseConfig(config_set_); ASSERT_TRUE(checkAnswer(0)); + DStubContextPtr context = getStubContext(); + ASSERT_TRUE(context); // Verify that the boolean parameter was parsed correctly by retrieving // its value from the context. @@ -270,22 +307,26 @@ TEST_F(DStubCfgMgrTest, simpleTypesTest) { EXPECT_NO_THROW(context->getParam("string_test", actual_string)); EXPECT_EQ("hmmm chewy", actual_string); - // Verify that the "extra" parameter was parsed correctly by retrieving - // its value from the context. - uint32_t actual_extra = 0; - EXPECT_NO_THROW(context->getExtraParam("extra_test", actual_extra)); - EXPECT_EQ(430, actual_extra); + isc::data::ConstElementPtr object; + EXPECT_NO_THROW(context->getObjectParam("map_test", object)); + EXPECT_TRUE(object); + + EXPECT_NO_THROW(context->getObjectParam("list_test", object)); + EXPECT_TRUE(object); // Create a configuration which "updates" all of the parameter values. string config2 = "{ \"bool_test\": false , " " \"uint32_test\": 88 , " " \"string_test\": \"ewww yuk!\" , " - " \"extra_test\": 11 } "; + " \"map_test2\" : {} , " + " \"list_test2\": [] }"; ASSERT_TRUE(fromJSON(config2)); // Verify that the configuration parses without error. answer_ = cfg_mgr_->parseConfig(config_set_); EXPECT_TRUE(checkAnswer(0)); + context = getStubContext(); + ASSERT_TRUE(context); // Verify that the boolean parameter was updated correctly by retrieving // its value from the context. @@ -305,31 +346,38 @@ TEST_F(DStubCfgMgrTest, simpleTypesTest) { EXPECT_NO_THROW(context->getParam("string_test", actual_string)); EXPECT_EQ("ewww yuk!", actual_string); - // Verify that the "extra" parameter was updated correctly by retrieving - // its value from the context. - actual_extra = 0; - EXPECT_NO_THROW(context->getExtraParam("extra_test", actual_extra)); - EXPECT_EQ(11, actual_extra); + // Verify previous objects are not there. + EXPECT_THROW(context->getObjectParam("map_test", object), + isc::dhcp::DhcpConfigError); + EXPECT_THROW(context->getObjectParam("list_test", object), + isc::dhcp::DhcpConfigError); + + // Verify new map object is there. + EXPECT_NO_THROW(context->getObjectParam("map_test2", object)); + EXPECT_TRUE(object); + + // Verify new list object is there. + EXPECT_NO_THROW(context->getObjectParam("list_test2", object)); + EXPECT_TRUE(object); } /// @brief Tests that the configuration context is preserved after failure /// during parsing causes a rollback. /// 1. Verifies configuration context rollback. TEST_F(DStubCfgMgrTest, rollBackTest) { - // Fetch a derivation specific pointer to the context. - DStubContextPtr context = getStubContext(); - ASSERT_TRUE(context); - // Create a configuration with all of the parameters. string config = "{ \"bool_test\": true , " " \"uint32_test\": 77 , " " \"string_test\": \"hmmm chewy\" , " - " \"extra_test\": 430 } "; + " \"map_test\" : {} , " + " \"list_test\": [] }"; ASSERT_TRUE(fromJSON(config)); // Verify that the configuration parses without error. answer_ = cfg_mgr_->parseConfig(config_set_); EXPECT_TRUE(checkAnswer(0)); + DStubContextPtr context = getStubContext(); + ASSERT_TRUE(context); // Verify that all of parameters have the expected values. bool actual_bool = false; @@ -344,16 +392,20 @@ TEST_F(DStubCfgMgrTest, rollBackTest) { EXPECT_NO_THROW(context->getParam("string_test", actual_string)); EXPECT_EQ("hmmm chewy", actual_string); - uint32_t actual_extra = 0; - EXPECT_NO_THROW(context->getExtraParam("extra_test", actual_extra)); - EXPECT_EQ(430, actual_extra); + isc::data::ConstElementPtr object; + EXPECT_NO_THROW(context->getObjectParam("map_test", object)); + EXPECT_TRUE(object); + + EXPECT_NO_THROW(context->getObjectParam("list_test", object)); + EXPECT_TRUE(object); // Create a configuration which "updates" all of the parameter values // plus one unknown at the end. string config2 = "{ \"bool_test\": false , " " \"uint32_test\": 88 , " " \"string_test\": \"ewww yuk!\" , " - " \"extra_test\": 11 , " + " \"map_test2\" : {} , " + " \"list_test2\": [] , " " \"zeta_unknown\": 33 } "; ASSERT_TRUE(fromJSON(config2)); @@ -361,9 +413,8 @@ TEST_F(DStubCfgMgrTest, rollBackTest) { SimFailure::set(SimFailure::ftElementUnknown); answer_ = cfg_mgr_->parseConfig(config_set_); EXPECT_TRUE(checkAnswer(1)); - - // Refresh our local pointer. context = getStubContext(); + ASSERT_TRUE(context); // Verify that all of parameters have the original values. actual_bool = false; @@ -378,9 +429,11 @@ TEST_F(DStubCfgMgrTest, rollBackTest) { EXPECT_NO_THROW(context->getParam("string_test", actual_string)); EXPECT_EQ("hmmm chewy", actual_string); - actual_extra = 0; - EXPECT_NO_THROW(context->getExtraParam("extra_test", actual_extra)); - EXPECT_EQ(430, actual_extra); + EXPECT_NO_THROW(context->getObjectParam("map_test", object)); + EXPECT_TRUE(object); + + EXPECT_NO_THROW(context->getObjectParam("list_test", object)); + EXPECT_TRUE(object); } } // end of anonymous namespace diff --git a/src/bin/d2/tests/d_test_stubs.cc b/src/bin/d2/tests/d_test_stubs.cc index 319dcd74e4..07dc1d4d37 100644 --- a/src/bin/d2/tests/d_test_stubs.cc +++ b/src/bin/d2/tests/d_test_stubs.cc @@ -22,7 +22,6 @@ namespace isc { namespace d2 { const char* valid_d2_config = "{ " - "\"interface\" : \"eth1\" , " "\"ip_address\" : \"127.0.0.1\" , " "\"port\" : 5031, " "\"tsig_keys\": [" @@ -218,16 +217,18 @@ DStubController::~DStubController() { // Initialize controller wrapper's static instance getter member. DControllerTest::InstanceGetter DControllerTest::instanceGetter_ = NULL; -//************************** TestParser ************************* +//************************** ObjectParser ************************* -TestParser::TestParser(const std::string& param_name):param_name_(param_name) { +ObjectParser::ObjectParser(const std::string& param_name, + ObjectStoragePtr& object_values) + : param_name_(param_name), object_values_(object_values) { } -TestParser::~TestParser(){ +ObjectParser::~ObjectParser(){ } void -TestParser::build(isc::data::ConstElementPtr new_config) { +ObjectParser::build(isc::data::ConstElementPtr new_config) { if (SimFailure::shouldFailOn(SimFailure::ftElementBuild)) { // Simulates an error during element data parsing. isc_throw (DCfgMgrBaseError, "Simulated build exception"); @@ -237,29 +238,33 @@ TestParser::build(isc::data::ConstElementPtr new_config) { } void -TestParser::commit() { +ObjectParser::commit() { if (SimFailure::shouldFailOn(SimFailure::ftElementCommit)) { // Simulates an error while committing the parsed element data. throw std::runtime_error("Simulated commit exception"); } + + object_values_->setParam(param_name_, value_, + isc::data::Element::Position()); } //************************** DStubContext ************************* -DStubContext::DStubContext(): extra_values_(new isc::dhcp::Uint32Storage()) { +DStubContext::DStubContext(): object_values_(new ObjectStorage()) { } DStubContext::~DStubContext() { } void -DStubContext::getExtraParam(const std::string& name, uint32_t& value) { - value = extra_values_->getParam(name); +DStubContext::getObjectParam(const std::string& name, + isc::data::ConstElementPtr& value) { + value = object_values_->getParam(name); } -isc::dhcp::Uint32StoragePtr -DStubContext::getExtraStorage() { - return (extra_values_); +ObjectStoragePtr& +DStubContext::getObjectStorage() { + return (object_values_); } DCfgContextBasePtr @@ -268,7 +273,7 @@ DStubContext::clone() { } DStubContext::DStubContext(const DStubContext& rhs): DCfgContextBase(rhs), - extra_values_(new isc::dhcp::Uint32Storage(*(rhs.extra_values_))) { + object_values_(new ObjectStorage(*(rhs.object_values_))) { } //************************** DStubCfgMgr ************************* @@ -280,40 +285,43 @@ DStubCfgMgr::DStubCfgMgr() DStubCfgMgr::~DStubCfgMgr() { } +DCfgContextBasePtr +DStubCfgMgr::createNewContext() { + return (DCfgContextBasePtr (new DStubContext())); +} + isc::dhcp::ParserPtr DStubCfgMgr::createConfigParser(const std::string& element_id) { - isc::dhcp::DhcpConfigParser* parser = NULL; - DStubContextPtr context = - boost::dynamic_pointer_cast(getContext()); + isc::dhcp::ParserPtr parser; + DStubContextPtr context + = boost::dynamic_pointer_cast(getContext()); if (element_id == "bool_test") { - parser = new isc::dhcp::BooleanParser(element_id, - context->getBooleanStorage()); + parser.reset(new isc::dhcp:: + BooleanParser(element_id, + context->getBooleanStorage())); } else if (element_id == "uint32_test") { - parser = new isc::dhcp::Uint32Parser(element_id, - context->getUint32Storage()); + parser.reset(new isc::dhcp::Uint32Parser(element_id, + context->getUint32Storage())); } else if (element_id == "string_test") { - parser = new isc::dhcp::StringParser(element_id, - context->getStringStorage()); - } else if (element_id == "extra_test") { - parser = new isc::dhcp::Uint32Parser(element_id, - context->getExtraStorage()); + parser.reset(new isc::dhcp::StringParser(element_id, + context->getStringStorage())); } else { // Fail only if SimFailure dictates we should. This makes it easier // to test parse ordering, by permitting a wide range of element ids // to "succeed" without specifically supporting them. if (SimFailure::shouldFailOn(SimFailure::ftElementUnknown)) { - isc_throw(DCfgMgrBaseError, "Configuration parameter not supported: " - << element_id); + isc_throw(DCfgMgrBaseError, + "Configuration parameter not supported: " << element_id); } - parsed_order_.push_back(element_id); - parser = new TestParser(element_id); + // Going to assume anything else is an object element. + parser.reset(new ObjectParser(element_id, context->getObjectStorage())); } - return (isc::dhcp::ParserPtr(parser)); + parsed_order_.push_back(element_id); + return (parser); } - }; // namespace isc::d2 }; // namespace isc diff --git a/src/bin/d2/tests/d_test_stubs.h b/src/bin/d2/tests/d_test_stubs.h index 7680a985ca..1e456e3d9b 100644 --- a/src/bin/d2/tests/d_test_stubs.h +++ b/src/bin/d2/tests/d_test_stubs.h @@ -135,9 +135,9 @@ public: /// indicate an abnormal termination. virtual void run(); - /// @brief Implements the process shutdown procedure. + /// @brief Implements the process shutdown procedure. /// - /// This sets the instance shutdown flag monitored by run() and stops + /// This sets the instance shutdown flag monitored by run() and stops /// the IO service. virtual isc::data::ConstElementPtr shutdown(isc::data::ConstElementPtr); @@ -446,9 +446,12 @@ public: } }; -/// @brief Simple parser derivation for testing the basics of configuration -/// parsing. -class TestParser : public isc::dhcp::DhcpConfigParser { +/// @brief a collection of elements that store uint32 values +typedef isc::dhcp::ValueStorage ObjectStorage; +typedef boost::shared_ptr ObjectStoragePtr; + +/// @brief Simple parser derivation for parsing object elements. +class ObjectParser : public isc::dhcp::DhcpConfigParser { public: /// @brief Constructor @@ -456,10 +459,10 @@ public: /// See @ref DhcpConfigParser class for details. /// /// @param param_name name of the parsed parameter - TestParser(const std::string& param_name); + ObjectParser(const std::string& param_name, ObjectStoragePtr& object_values); /// @brief Destructor - virtual ~TestParser(); + virtual ~ObjectParser(); /// @brief Builds parameter value. /// @@ -486,8 +489,12 @@ private: /// pointer to the parsed value of the parameter isc::data::ConstElementPtr value_; + + /// Pointer to the storage where committed value is stored. + ObjectStoragePtr object_values_; }; + /// @brief Test Derivation of the DCfgContextBase class. /// /// This class is used to test basic functionality of configuration context. @@ -505,6 +512,11 @@ public: /// @brief Destructor virtual ~DStubContext(); + /// @brief Creates a clone of a DStubContext. + /// + /// @return returns a pointer to the new clone. + virtual DCfgContextBasePtr clone(); + /// @brief Fetches the value for a given "extra" configuration parameter /// from the context. /// @@ -513,17 +525,10 @@ public: /// value. /// @throw throws DhcpConfigError if the context does not contain the /// parameter. - void getExtraParam(const std::string& name, uint32_t& value); + void getObjectParam(const std::string& name, + isc::data::ConstElementPtr& value); - /// @brief Fetches the extra storage. - /// - /// @return returns a pointer to the extra storage. - isc::dhcp::Uint32StoragePtr getExtraStorage(); - - /// @brief Creates a clone of a DStubContext. - /// - /// @return returns a pointer to the new clone. - virtual DCfgContextBasePtr clone(); + ObjectStoragePtr& getObjectStorage(); protected: /// @brief Copy constructor @@ -533,8 +538,8 @@ private: /// @brief Private assignment operator, not implemented. DStubContext& operator=(const DStubContext& rhs); - /// @brief Extra storage for uint32 parameters. - isc::dhcp::Uint32StoragePtr extra_values_; + /// @brief Stores non-scalar configuration elements + ObjectStoragePtr object_values_; }; /// @brief Defines a pointer to DStubContext. @@ -579,6 +584,9 @@ public: /// @brief A list for remembering the element ids in the order they were /// parsed. ElementIdList parsed_order_; + + /// @todo + virtual DCfgContextBasePtr createNewContext(); }; /// @brief Defines a pointer to DStubCfgMgr. @@ -609,9 +617,9 @@ public: try { config_set_ = isc::data::Element::fromJSON(json_text); } catch (const isc::Exception &ex) { - return (::testing::AssertionFailure(::testing::Message() << - "JSON text failed to parse:" - << ex.what())); + return (::testing::AssertionFailure(::testing::Message() << + "JSON text failed to parse:" + << ex.what())); } return (::testing::AssertionSuccess()); @@ -631,7 +639,7 @@ public: /// @brief Compares the status in the given parse result to a given value. /// /// @param answer Element set containing an integer response and string - /// comment. + /// comment. /// @param should_be is an integer against which to compare the status. /// /// @return returns AssertionSuccess if there were no parsing errors, @@ -645,8 +653,8 @@ public: return (testing::AssertionSuccess()); } - return (::testing::AssertionFailure(::testing::Message() << - "checkAnswer rcode:" << rcode + return (::testing::AssertionFailure(::testing::Message() << + "checkAnswer rcode:" << rcode << " comment: " << *comment)); } From 0f2ef12bf864a134f9d3c834cad51aa88356b486 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 7 May 2014 15:33:27 -0400 Subject: [PATCH 2/9] [3268] Added container class for D2 global parameters Added d2::D2Params to act as container class for D2 global parameters. This permits them to be converted from their context parameter form during configuration parsing. --- src/bin/d2/d2_config.cc | 99 +++++++++++++++++++++++++++++++++++++++ src/bin/d2/d2_config.h | 101 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+) diff --git a/src/bin/d2/d2_config.cc b/src/bin/d2/d2_config.cc index fbfbf763d3..d933047651 100644 --- a/src/bin/d2/d2_config.cc +++ b/src/bin/d2/d2_config.cc @@ -27,6 +27,105 @@ namespace isc { namespace d2 { +// *********************** D2Params ************************* + +const char *D2Params::DFT_IP_ADDRESS = "127.0.0.1"; +const size_t D2Params::DFT_PORT = 53001; +const size_t D2Params::DFT_DNS_SERVER_TIMEOUT = 100; +const char *D2Params::DFT_NCR_PROTOCOL = "UDP"; +const char *D2Params::DFT_NCR_FORMAT = "JSON"; + +D2Params::D2Params(const isc::asiolink::IOAddress& ip_address, + const size_t port, + const size_t dns_server_timeout, + const dhcp_ddns:: NameChangeProtocol& ncr_protocol, + const dhcp_ddns:: NameChangeFormat& ncr_format) + : ip_address_(ip_address), + port_(port), + dns_server_timeout_(dns_server_timeout), + ncr_protocol_(ncr_protocol), + ncr_format_(ncr_format) { + validateContents(); +} + +D2Params::D2Params() + : ip_address_(isc::asiolink::IOAddress(DFT_IP_ADDRESS)), + port_(DFT_PORT), + dns_server_timeout_(DFT_DNS_SERVER_TIMEOUT), + ncr_protocol_(dhcp_ddns::NCR_UDP), + ncr_format_(dhcp_ddns::FMT_JSON) { + validateContents(); +} + +D2Params::~D2Params(){}; + +void +D2Params::validateContents() { + if (ip_address_.toText() == "0.0.0.0") { + isc_throw(D2CfgError, "D2Params: IP address cannot be \"0.0.0.0\""); + } + + if (ip_address_.toText() == "::") { + isc_throw(D2CfgError, "D2Params: IP address cannot be \"::\""); + } + + if (port_ == 0) { + isc_throw(D2CfgError, "D2Params: port cannot be 0"); + } + + if (dns_server_timeout_ < 1) { + isc_throw(D2CfgError, + "D2Params: DNS server timeout must be larger than 0"); + } + + if (ncr_format_ != dhcp_ddns::FMT_JSON) { + isc_throw(D2CfgError, "D2Params: NCR Format:" + << dhcp_ddns::ncrFormatToString(ncr_format_) + << " is not yet supported"); + } + + if (ncr_protocol_ != dhcp_ddns::NCR_UDP) { + isc_throw(D2CfgError, "D2Params: NCR Protocol:" + << dhcp_ddns::ncrProtocolToString(ncr_protocol_) + << " is not yet supported"); + } +} + +bool +D2Params::operator == (const D2Params& other) const { + return ((ip_address_ == other.ip_address_) && + (port_ == other.port_) && + (dns_server_timeout_ == other.dns_server_timeout_) && + (ncr_protocol_ == other.ncr_protocol_) && + (ncr_format_ == other.ncr_format_)); +} + +bool +D2Params::operator != (const D2Params& other) const { + return (!(*this == other)); +} + +std::string +D2Params::toText() const { + std::ostringstream stream; + + stream << ", ip_address: " << ip_address_.toText() + << ", port: " << port_ + << ", dns_server_timeout_: " << dns_server_timeout_ + << ", ncr_protocol: " + << dhcp_ddns::ncrProtocolToString(ncr_protocol_) + << ", ncr_format: " << ncr_format_ + << dhcp_ddns::ncrFormatToString(ncr_format_); + + return (stream.str()); +} + +std::ostream& +operator<<(std::ostream& os, const D2Params& config) { + os << config.toText(); + return (os); +} + // *********************** TSIGKeyInfo ************************* TSIGKeyInfo::TSIGKeyInfo(const std::string& name, const std::string& algorithm, diff --git a/src/bin/d2/d2_config.h b/src/bin/d2/d2_config.h index 5af60eee80..8c5dd29cd4 100644 --- a/src/bin/d2/d2_config.h +++ b/src/bin/d2/d2_config.h @@ -143,6 +143,107 @@ public: isc::Exception(file, line, what) { }; }; +/// @brief Acts as a storage vault for D2 global scalar parameters +class D2Params { +public: + /// @brief Default configuration constants. + /// @todo For now these are hard-coded as configuration layer cannot + /// readily provide them (see Trac #3358). + static const char *DFT_IP_ADDRESS; + static const size_t DFT_PORT; + static const size_t DFT_DNS_SERVER_TIMEOUT; + static const char *DFT_NCR_PROTOCOL; + static const char *DFT_NCR_FORMAT; + + /// @brief Constructor + /// + /// @throw D2CfgError if given an invalid protocol or format. + D2Params(const isc::asiolink::IOAddress& ip_address, + const size_t port, + const size_t DFT_DNS_SERVER_TIMEOUT, + const dhcp_ddns::NameChangeProtocol& ncr_protocol, + const dhcp_ddns::NameChangeFormat& ncr_format); + + /// @brief Default constructor + /// The default constructor creates an instance that has updates disabled. + D2Params(); + + /// @brief Destructor + virtual ~D2Params(); + + /// @brief Return the IP D2 listens on. + const isc::asiolink::IOAddress& getIpAddress() const { + return(ip_address_); + } + + /// @brief Return the IP port D2 listens on. + size_t getPort() const { + return(port_); + } + + /// @brief Return the DNS server timeout value. + size_t getDnsServerTimeout() const { + return(dns_server_timeout_); + } + + /// @brief Return the socket protocol in use. + const dhcp_ddns::NameChangeProtocol& getNcrProtocol() const { + return(ncr_protocol_); + } + + /// @brief Return the expected format of inbound requests (NCRs). + const dhcp_ddns::NameChangeFormat& getNcrFormat() const { + return(ncr_format_); + } + + /// @brief Compares two D2Paramss for equality + bool operator == (const D2Params& other) const; + + /// @brief Compares two D2Paramss for inequality + bool operator != (const D2Params& other) const; + + /// @brief Generates a string representation of the class contents. + std::string toText() const; + +protected: + /// @brief Validates member values. + /// + /// Method is used by the constructor to validate member contents. + /// Currently checks: + /// -# ip_address is not 0.0.0.0 or :: + /// -# port is not 0 + /// -# dns_server_timeout is 0 + /// -# ncr_protocol is UDP + /// -# ncr_format is JSON + /// + /// @throw D2CfgError if contents are invalid + virtual void validateContents(); + +private: + /// @brief IP address D2 listens on. + isc::asiolink::IOAddress ip_address_; + + /// @brief IP port D2 listens on. + size_t port_; + + /// @brief Timeout for a single DNS packet exchange in milliseconds. + size_t dns_server_timeout_; + + /// @brief The socket protocol to use. + /// Currently only UDP is supported. + dhcp_ddns::NameChangeProtocol ncr_protocol_; + + /// @brief Format of the inbound requests (NCRs). + /// Currently only JSON format is supported. + dhcp_ddns::NameChangeFormat ncr_format_; +}; + +std::ostream& +operator<<(std::ostream& os, const D2Params& config); + +/// @brief Defines a pointer for D2Params instances. +typedef boost::shared_ptr D2ParamsPtr; + /// @brief Represents a TSIG Key. /// /// Currently, this is simple storage class containing the basic attributes of From 67f5da73b63a0bdb5adfd9b25090ab459f64bee5 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 7 May 2014 16:18:54 -0400 Subject: [PATCH 3/9] [3268] Modified D2CfgMgr and spec file Added D2CfgMgr::buildParams() method to support the new parameters first processing. Removed support D2's global "interface" parameter, and added three more: - dns_server_timeout - ncr_protocol - ncr_format --- src/bin/d2/d2_cfg_mgr.cc | 88 +++++++-- src/bin/d2/d2_cfg_mgr.h | 36 +++- src/bin/d2/dhcp-ddns.spec | 26 ++- src/bin/d2/tests/d2_cfg_mgr_unittests.cc | 230 +++++++++++++++++++++-- 4 files changed, 330 insertions(+), 50 deletions(-) diff --git a/src/bin/d2/d2_cfg_mgr.cc b/src/bin/d2/d2_cfg_mgr.cc index 4bddb99a6f..28796f54dc 100644 --- a/src/bin/d2/d2_cfg_mgr.cc +++ b/src/bin/d2/d2_cfg_mgr.cc @@ -30,12 +30,14 @@ typedef std::vector ByteAddress; // *********************** D2CfgContext ************************* D2CfgContext::D2CfgContext() - : forward_mgr_(new DdnsDomainListMgr("forward_mgr")), + : d2_params_(new D2Params()), + forward_mgr_(new DdnsDomainListMgr("forward_mgr")), reverse_mgr_(new DdnsDomainListMgr("reverse_mgr")), keys_(new TSIGKeyInfoMap()) { } D2CfgContext::D2CfgContext(const D2CfgContext& rhs) : DCfgContextBase(rhs) { + d2_params_ = rhs.d2_params_; if (rhs.forward_mgr_) { forward_mgr_.reset(new DdnsDomainListMgr(rhs.forward_mgr_->getName())); forward_mgr_->setDomains(rhs.forward_mgr_->getDomains()); @@ -61,9 +63,6 @@ const char* D2CfgMgr::IPV6_REV_ZONE_SUFFIX = "ip6.arpa."; D2CfgMgr::D2CfgMgr() : DCfgMgrBase(DCfgContextBasePtr(new D2CfgContext())) { // TSIG keys need to parse before the Domains, so we can catch Domains // that specify undefined keys. Create the necessary parsing order now. - addToParseOrder("interface"); - addToParseOrder("ip_address"); - addToParseOrder("port"); addToParseOrder("tsig_keys"); addToParseOrder("forward_ddns"); addToParseOrder("reverse_ddns"); @@ -72,6 +71,11 @@ D2CfgMgr::D2CfgMgr() : DCfgMgrBase(DCfgContextBasePtr(new D2CfgContext())) { D2CfgMgr::~D2CfgMgr() { } +DCfgContextBasePtr +D2CfgMgr::createNewContext() { + return (DCfgContextBasePtr(new D2CfgContext())); +} + bool D2CfgMgr::forwardUpdatesEnabled() { // Forward updates are not enabled if no forward servers are defined. @@ -187,6 +191,47 @@ D2CfgMgr::reverseV6Address(const isc::asiolink::IOAddress& ioaddr) { return(stream.str()); } +const D2ParamsPtr& +D2CfgMgr::getD2Params() { + return (getD2CfgContext()->getD2Params()); +} + +void +D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) { + // Base class build creates parses and invokes build on each parser. + // This populate the context scalar stores with all of the parameters. + DCfgMgrBase::buildParams(params_config); + + // Fetch the parameters from the context to create the D2Params. + D2CfgContextPtr context = getD2CfgContext(); + isc::dhcp::StringStoragePtr& strings = context->getStringStorage(); + asiolink::IOAddress ip_address(strings-> + getOptionalParam("ip_address", + D2Params::DFT_IP_ADDRESS)); + + isc::dhcp::Uint32StoragePtr& ints = context->getUint32Storage(); + uint32_t port = ints->getOptionalParam("port", D2Params::DFT_PORT); + + uint32_t dns_server_timeout + = ints->getOptionalParam("dns_server_timeout", + D2Params::DFT_DNS_SERVER_TIMEOUT); + + dhcp_ddns::NameChangeProtocol ncr_protocol + = dhcp_ddns::stringToNcrProtocol(strings-> + getOptionalParam("ncr_protocol", + D2Params:: + DFT_NCR_PROTOCOL)); + dhcp_ddns::NameChangeFormat ncr_format + = dhcp_ddns::stringToNcrFormat(strings-> + getOptionalParam("ncr_format", + D2Params:: + DFT_NCR_FORMAT)); + // Attempt to create the new client config. + D2ParamsPtr params(new D2Params(ip_address, port, dns_server_timeout, + ncr_protocol, ncr_format)); + + context->getD2Params() = params; +} isc::dhcp::ParserPtr D2CfgMgr::createConfigParser(const std::string& config_id) { @@ -194,31 +239,34 @@ D2CfgMgr::createConfigParser(const std::string& config_id) { D2CfgContextPtr context = getD2CfgContext(); // Create parser instance based on element_id. - isc::dhcp::DhcpConfigParser* parser = NULL; - if ((config_id == "interface") || - (config_id == "ip_address")) { - parser = new isc::dhcp::StringParser(config_id, - context->getStringStorage()); - } else if (config_id == "port") { - parser = new isc::dhcp::Uint32Parser(config_id, - context->getUint32Storage()); + isc::dhcp::ParserPtr parser; + if ((config_id.compare("port") == 0) || + (config_id.compare("dns_server_timeout") == 0)) { + parser.reset(new isc::dhcp::Uint32Parser(config_id, + context->getUint32Storage())); + } else if ((config_id.compare("ip_address") == 0) || + (config_id.compare("ncr_protocol") == 0) || + (config_id.compare("ncr_format") == 0)) { + parser.reset(new isc::dhcp::StringParser(config_id, + context->getStringStorage())); } else if (config_id == "forward_ddns") { - parser = new DdnsDomainListMgrParser("forward_mgr", - context->getForwardMgr(), - context->getKeys()); + parser.reset(new DdnsDomainListMgrParser("forward_mgr", + context->getForwardMgr(), + context->getKeys())); } else if (config_id == "reverse_ddns") { - parser = new DdnsDomainListMgrParser("reverse_mgr", - context->getReverseMgr(), - context->getKeys()); + parser.reset(new DdnsDomainListMgrParser("reverse_mgr", + context->getReverseMgr(), + context->getKeys())); } else if (config_id == "tsig_keys") { - parser = new TSIGKeyInfoListParser("tsig_key_list", context->getKeys()); + parser.reset(new TSIGKeyInfoListParser("tsig_key_list", + context->getKeys())); } else { isc_throw(NotImplemented, "parser error: D2CfgMgr parameter not supported: " << config_id); } - return (isc::dhcp::ParserPtr(parser)); + return (parser); } }; // end of isc::dhcp namespace diff --git a/src/bin/d2/d2_cfg_mgr.h b/src/bin/d2/d2_cfg_mgr.h index 0e903043e2..e12557be64 100644 --- a/src/bin/d2/d2_cfg_mgr.h +++ b/src/bin/d2/d2_cfg_mgr.h @@ -53,6 +53,11 @@ public: return (DCfgContextBasePtr(new D2CfgContext(*this))); } + /// @brief Fetches a reference to the D2params + D2ParamsPtr& getD2Params() { + return (d2_params_); + } + /// @brief Fetches the forward DNS domain list manager. /// /// @return returns a pointer to the forward manager. @@ -82,6 +87,9 @@ private: /// @brief Private assignment operator to avoid potential for slicing. D2CfgContext& operator=(const D2CfgContext& rhs); + /// @brief Global level parameter storage + D2ParamsPtr d2_params_; + /// @brief Forward domain list manager. DdnsDomainListMgrPtr forward_mgr_; @@ -226,17 +234,33 @@ public: /// @throw D2CfgError if not given an IPv6 address. static std::string reverseV6Address(const isc::asiolink::IOAddress& ioaddr); + /// @brief Convenience method fetches the D2Params from context + /// @return reference to const D2ParamsPtr + const D2ParamsPtr& getD2Params(); + protected: + /// @brief Performs the parsing of the given "params" element. + /// + /// Iterates over the set of parameters, creating a parser based on the + /// parameter's id and then invoking its build method passing in the + /// paramter's configuration value. + /// + /// @param params_config set of scalar configuration elements to parse + virtual void buildParams(isc::data::ConstElementPtr params_config); + /// @brief Given an element_id returns an instance of the appropriate /// parser. /// /// It is responsible for top-level or outermost DHCP-DDNS configuration /// elements (see dhcp-ddns.spec): - /// 1. interface - /// 2. ip_address - /// 3. port - /// 4. forward_ddns - /// 5. reverse_ddns + /// -# ip_address + /// -# port + /// -# dns_server_timeout + /// -# ncr_protocol + /// -# ncr_format + /// -# tsig_keys + /// -# forward_ddns + /// -# reverse_ddns /// /// @param element_id is the string name of the element as it will appear /// in the configuration set. @@ -245,6 +269,8 @@ protected: /// @throw throws DCfgMgrBaseError if an error occurs. virtual isc::dhcp::ParserPtr createConfigParser(const std::string& element_id); + + virtual DCfgContextBasePtr createNewContext(); }; /// @brief Defines a shared pointer to D2CfgMgr. diff --git a/src/bin/d2/dhcp-ddns.spec b/src/bin/d2/dhcp-ddns.spec index b4240b1420..ac336d7b6d 100644 --- a/src/bin/d2/dhcp-ddns.spec +++ b/src/bin/d2/dhcp-ddns.spec @@ -4,26 +4,36 @@ "module_name": "DhcpDdns", "module_description": "DHPC-DDNS Service", "config_data": [ - { - "item_name": "interface", - "item_type": "string", - "item_optional": true, - "item_default": "eth0" - }, - { "item_name": "ip_address", "item_type": "string", "item_optional": false, "item_default": "127.0.0.1" }, - { "item_name": "port", "item_type": "integer", "item_optional": true, "item_default": 53001 }, + { + "item_name": "dns_server_timeout", + "item_type": "integer", + "item_optional": true, + "item_default": 100 + }, + { + "item_name": "ncr_protocol", + "item_type": "string", + "item_optional": true, + "item_default": "UDP" + }, + { + "item_name": "ncr_format", + "item_type": "string", + "item_optional": true, + "item_default": "JSON" + }, { "item_name": "tsig_keys", "item_type": "list", diff --git a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc index e1b3313f1d..a9c476b9e5 100644 --- a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc @@ -39,7 +39,7 @@ class D2CfgMgrTest : public ConfigParseTest { public: /// @brief Constructor - D2CfgMgrTest():cfg_mgr_(new D2CfgMgr) { + D2CfgMgrTest():cfg_mgr_(new D2CfgMgr), d2_params_() { } /// @brief Destructor @@ -48,6 +48,58 @@ public: /// @brief Configuration manager instance. D2CfgMgrPtr cfg_mgr_; + + /// @brief Build JSON configuration string for a D2Params element + /// + /// Constructs a JSON string for "params" element using replacable + /// parameters. + /// + /// @param ip_address string to insert as ip_address value + /// @param port integer to insert as port value + /// @param dns_server_timeout integer to insert as dns_server_timeout value + /// @param ncr_protocol string to insert as ncr_protocol value + /// @param ncr_format string to insert as ncr_format value + /// + /// @return std::string containing the JSON configuration text + std::string makeParamsConfigString(const std::string& ip_address, + const int port, + const int dns_server_timeout, + const std::string& ncr_protocol, + const std::string& ncr_format) { + std::ostringstream config; + config << + "{" + " \"ip_address\": \"" << ip_address << "\" , " + " \"port\": " << port << " , " + " \"dns_server_timeout\": " << dns_server_timeout << " , " + " \"ncr_protocol\": \"" << ncr_protocol << "\" , " + " \"ncr_format\": \"" << ncr_format << "\", " + "\"tsig_keys\": [], " + "\"forward_ddns\" : {}, " + "\"reverse_ddns\" : {} " + "}"; + + return (config.str()); + } + + void runConfig(std::string config_str, bool should_fail=false) { + // We assume the config string is valid JSON. + ASSERT_TRUE(fromJSON(config_str)); + + // Parse the configuration and verify we got the expected outcome. + answer_ = cfg_mgr_->parseConfig(config_set_); + ASSERT_TRUE(checkAnswer(should_fail)); + + // Verify that the D2 context can be retrieved and is not null. + D2CfgContextPtr context; + ASSERT_NO_THROW(context = cfg_mgr_->getD2CfgContext()); + + // Verify that the global scalars have the proper values. + d2_params_ = context->getD2Params(); + ASSERT_TRUE(d2_params_); + } + + D2ParamsPtr d2_params_; }; /// @brief Tests that the spec file is valid. @@ -245,6 +297,150 @@ public: isc::dhcp::ParserPtr parser_; }; +/// @brief Tests a basic valid configuration for D2Param. +TEST_F(D2CfgMgrTest, validParamsEntry) { + // Verify that ip_address can be valid v4 address. + std::string config = makeParamsConfigString ("192.0.0.1", 777, 333, + "UDP", "JSON"); + runConfig(config); + + EXPECT_EQ(isc::asiolink::IOAddress("192.0.0.1"), + d2_params_->getIpAddress()); + EXPECT_EQ(777, d2_params_->getPort()); + EXPECT_EQ(333, d2_params_->getDnsServerTimeout()); + EXPECT_EQ(dhcp_ddns::NCR_UDP, d2_params_->getNcrProtocol()); + EXPECT_EQ(dhcp_ddns::FMT_JSON, d2_params_->getNcrFormat()); + + // Verify that ip_address can be valid v6 address. + config = makeParamsConfigString ("3001::5", 777, 333, "UDP", "JSON"); + runConfig(config); + + // Verify that the global scalars have the proper values. + EXPECT_EQ(isc::asiolink::IOAddress("3001::5"), + d2_params_->getIpAddress()); +} + +/// @brief Tests default values for D2Params. +/// It verifies that D2Params is populated with default value for optional +/// parameter if not supplied in the configuration. +/// Currently they are all optional. +TEST_F(D2CfgMgrTest, defaultValues) { + + // Check that omitting ip_address gets you its default + std::string config = + "{" + " \"port\": 777 , " + " \"dns_server_timeout\": 333 , " + " \"ncr_protocol\": \"UDP\" , " + " \"ncr_format\": \"JSON\", " + "\"tsig_keys\": [], " + "\"forward_ddns\" : {}, " + "\"reverse_ddns\" : {} " + "}"; + + runConfig(config); + EXPECT_EQ(isc::asiolink::IOAddress(D2Params::DFT_IP_ADDRESS), + d2_params_->getIpAddress()); + + // Check that omitting port gets you its default + config = + "{" + " \"ip_address\": \"192.0.0.1\" , " + " \"dns_server_timeout\": 333 , " + " \"ncr_protocol\": \"UDP\" , " + " \"ncr_format\": \"JSON\", " + "\"tsig_keys\": [], " + "\"forward_ddns\" : {}, " + "\"reverse_ddns\" : {} " + "}"; + + runConfig(config); + EXPECT_EQ(D2Params::DFT_PORT, d2_params_->getPort()); + + // Check that omitting timeout gets you its default + config = + "{" + " \"ip_address\": \"192.0.0.1\" , " + " \"port\": 777 , " + " \"ncr_protocol\": \"UDP\" , " + " \"ncr_format\": \"JSON\", " + "\"tsig_keys\": [], " + "\"forward_ddns\" : {}, " + "\"reverse_ddns\" : {} " + "}"; + + runConfig(config); + EXPECT_EQ(D2Params::DFT_DNS_SERVER_TIMEOUT, + d2_params_->getDnsServerTimeout()); + + // Check that protocol timeout gets you its default + config = + "{" + " \"ip_address\": \"192.0.0.1\" , " + " \"port\": 777 , " + " \"dns_server_timeout\": 333 , " + " \"ncr_format\": \"JSON\", " + "\"tsig_keys\": [], " + "\"forward_ddns\" : {}, " + "\"reverse_ddns\" : {} " + "}"; + + runConfig(config); + EXPECT_EQ(dhcp_ddns::stringToNcrProtocol(D2Params::DFT_NCR_PROTOCOL), + d2_params_->getNcrProtocol()); + + // Check that format timeout gets you its default + config = + "{" + " \"ip_address\": \"192.0.0.1\" , " + " \"port\": 777 , " + " \"dns_server_timeout\": 333 , " + " \"ncr_protocol\": \"UDP\", " + "\"tsig_keys\": [], " + "\"forward_ddns\" : {}, " + "\"reverse_ddns\" : {} " + "}"; + + runConfig(config); + EXPECT_EQ(dhcp_ddns::stringToNcrFormat(D2Params::DFT_NCR_FORMAT), + d2_params_->getNcrFormat()); +} + +/// @brief Tests the enforcement of data validation when parsing D2Params. +/// It verifies that: +/// -# ip_address cannot be "0.0.0.0" +/// -# ip_address cannot be "::" +/// -# port cannot be 0 +/// -# dns_server_timeout cannat be 0 +/// -# ncr_protocol must be valid +/// -# ncr_format must be valid +TEST_F(D2CfgMgrTest, invalidEntry) { + // Cannot use IPv4 ANY address + std::string config = makeParamsConfigString ("0.0.0.0", 777, 333, + "UDP", "JSON"); + runConfig(config, 1); + + // Cannot use IPv6 ANY address + config = makeParamsConfigString ("::", 777, 333, "UDP", "JSON"); + runConfig(config, 1); + + // Cannot use port 0 + config = makeParamsConfigString ("127.0.0.1", 0, 333, "UDP", "JSON"); + runConfig(config, 1); + + // Cannot use dns server timeout of 0 + config = makeParamsConfigString ("127.0.0.1", 777, 0, "UDP", "JSON"); + runConfig(config, 1); + + // Invalid protocol + config = makeParamsConfigString ("127.0.0.1", 777, 333, "BOGUS", "JSON"); + runConfig(config, 1); + + // Invalid format + config = makeParamsConfigString ("127.0.0.1", 777, 333, "UDP", "BOGUS"); + runConfig(config, 1); +} + /// @brief Tests the enforcement of data validation when parsing TSIGKeyInfos. /// It verifies that: /// 1. Name cannot be blank. @@ -864,6 +1060,9 @@ TEST(D2CfgMgr, construction) { EXPECT_NO_THROW(delete cfg_mgr); } +TEST_F(D2CfgMgrTest, paramsConfig) { +} + /// @brief Tests the parsing of a complete, valid DHCP-DDNS configuration. /// This tests passes the configuration into an instance of D2CfgMgr just /// as it would be done by d2_process in response to a configuration update @@ -873,9 +1072,11 @@ TEST_F(D2CfgMgrTest, fullConfig) { // both the forward and reverse ddns managers. Both managers have two // domains with three servers per domain. std::string config = "{ " - "\"interface\" : \"eth1\" , " "\"ip_address\" : \"192.168.1.33\" , " "\"port\" : 88 , " + " \"dns_server_timeout\": 333 , " + " \"ncr_protocol\": \"UDP\" , " + " \"ncr_format\": \"JSON\", " "\"tsig_keys\": [" "{" " \"name\": \"d2_key.tmark.org\" , " @@ -924,6 +1125,7 @@ TEST_F(D2CfgMgrTest, fullConfig) { " { \"hostname\": \"six.rev\" } " " ] } " "] } }"; + ASSERT_TRUE(fromJSON(config)); // Verify that we can parse the configuration. @@ -934,18 +1136,16 @@ TEST_F(D2CfgMgrTest, fullConfig) { D2CfgContextPtr context; ASSERT_NO_THROW(context = cfg_mgr_->getD2CfgContext()); - // Verify that the application level scalars have the proper values. - std::string interface; - EXPECT_NO_THROW (context->getParam("interface", interface)); - EXPECT_EQ("eth1", interface); + // Verify that the global scalars have the proper values. + D2ParamsPtr& d2_params = context->getD2Params(); + ASSERT_TRUE(d2_params); - std::string ip_address; - EXPECT_NO_THROW (context->getParam("ip_address", ip_address)); - EXPECT_EQ("192.168.1.33", ip_address); - - uint32_t port = 0; - EXPECT_NO_THROW (context->getParam("port", port)); - EXPECT_EQ(88, port); + EXPECT_EQ(isc::asiolink::IOAddress("192.168.1.33"), + d2_params->getIpAddress()); + EXPECT_EQ(88, d2_params->getPort()); + EXPECT_EQ(333, d2_params->getDnsServerTimeout()); + EXPECT_EQ(dhcp_ddns::NCR_UDP, d2_params->getNcrProtocol()); + EXPECT_EQ(dhcp_ddns::FMT_JSON, d2_params->getNcrFormat()); // Verify that the forward manager can be retrieved. DdnsDomainListMgrPtr mgr = context->getForwardMgr(); @@ -1014,7 +1214,6 @@ TEST_F(D2CfgMgrTest, forwardMatch) { // Create configuration with one domain, one sub domain, and the wild // card. std::string config = "{ " - "\"interface\" : \"eth1\" , " "\"ip_address\" : \"192.168.1.33\" , " "\"port\" : 88 , " "\"tsig_keys\": [] ," @@ -1088,7 +1287,6 @@ TEST_F(D2CfgMgrTest, forwardMatch) { TEST_F(D2CfgMgrTest, matchNoWildcard) { // Create a configuration with one domain, one sub-domain, and NO wild card. std::string config = "{ " - "\"interface\" : \"eth1\" , " "\"ip_address\" : \"192.168.1.33\" , " "\"port\" : 88 , " "\"tsig_keys\": [] ," @@ -1136,7 +1334,6 @@ TEST_F(D2CfgMgrTest, matchNoWildcard) { /// This test verifies that any FQDN matches the wild card. TEST_F(D2CfgMgrTest, matchAll) { std::string config = "{ " - "\"interface\" : \"eth1\" , " "\"ip_address\" : \"192.168.1.33\" , " "\"port\" : 88 , " "\"tsig_keys\": [] ," @@ -1183,7 +1380,6 @@ TEST_F(D2CfgMgrTest, matchAll) { /// as a match. TEST_F(D2CfgMgrTest, matchReverse) { std::string config = "{ " - "\"interface\" : \"eth1\" , " "\"ip_address\" : \"192.168.1.33\" , " "\"port\" : 88 , " "\"tsig_keys\": [] ," From eecc258336e4c196475fd81eb874e81e41bd3f13 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 8 May 2014 06:42:09 -0400 Subject: [PATCH 4/9] [3268] Modify D2Process to use D2Params Changed D2Process to get queue manager configuration values from D2Params. This makes NCR protocol and format configurable. --- src/bin/d2/d2_process.cc | 31 ++++++++++++--------- src/bin/d2/tests/d2_process_unittests.cc | 4 --- src/bin/d2/tests/d2_update_mgr_unittests.cc | 1 - 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/bin/d2/d2_process.cc b/src/bin/d2/d2_process.cc index cca71e21d3..e9ce245616 100644 --- a/src/bin/d2/d2_process.cc +++ b/src/bin/d2/d2_process.cc @@ -207,7 +207,7 @@ D2Process::configure(isc::data::ConstElementPtr config_set) { if (rcode) { // Non-zero means we got an invalid configuration, take no further - // action. In integrated mode, this will send a failed response back + // action. In integrated mode, this will send a failed response back // to BIND10. reconf_queue_flag_ = false; return (answer); @@ -248,7 +248,7 @@ D2Process::checkQueueStatus() { queue_mgr_->stopListening(); } catch (const isc::Exception& ex) { // It is very unlikey that we would experience an error - // here, but theoretically possible. + // here, but theoretically possible. LOG_ERROR(dctl_logger, DHCP_DDNS_QUEUE_MGR_STOP_ERROR) .arg(ex.what()); } @@ -325,37 +325,42 @@ D2Process::reconfigureQueueMgr() { queue_mgr_->removeListener(); // Get the configuration parameters that affect Queue Manager. - // @todo Need to add parameters for listener TYPE, FORMAT, address reuse - std::string ip_address; - uint32_t port; - getCfgMgr()->getContext()->getParam("ip_address", ip_address); + const D2ParamsPtr& d2_params = getD2CfgMgr()->getD2Params(); // Warn the user if the server address is not the loopback. /// @todo Remove this once we provide a secure mechanism. + std::string ip_address = d2_params->getIpAddress().toText(); if (ip_address != "127.0.0.1" && ip_address != "::1") { LOG_WARN(dctl_logger, DHCP_DDNS_NOT_ON_LOOPBACK).arg(ip_address); } - getCfgMgr()->getContext()->getParam("port", port); - isc::asiolink::IOAddress addr(ip_address); - // Instantiate the listener. - queue_mgr_->initUDPListener(addr, port, dhcp_ddns::FMT_JSON, true); + if (d2_params->getNcrProtocol() == dhcp_ddns::NCR_UDP) { + queue_mgr_->initUDPListener(d2_params->getIpAddress(), + d2_params->getPort(), + d2_params->getNcrFormat(), true); + } else { + /// @todo Add TCP/IP once it's supported + // We should never get this far but if we do deal with it. + isc_throw(DProcessBaseError, "Unsupported NCR listener protocol:" + << dhcp_ddns::ncrProtocolToString(d2_params-> + getNcrProtocol())); + } // Now start it. This assumes that starting is a synchronous, // blocking call that executes quickly. @todo Should that change then // we will have to expand the state model to accommodate this. queue_mgr_->startListening(); } catch (const isc::Exception& ex) { - // Queue manager failed to initialize and therefore not listening. - // This is most likely due to an unavailable IP address or port, + // Queue manager failed to initialize and therefore not listening. + // This is most likely due to an unavailable IP address or port, // which is a configuration issue. LOG_ERROR(dctl_logger, DHCP_DDNS_QUEUE_MGR_START_ERROR).arg(ex.what()); } } isc::data::ConstElementPtr -D2Process::command(const std::string& command, +D2Process::command(const std::string& command, isc::data::ConstElementPtr args) { // @todo This is the initial implementation. If and when D2 is extended // to support its own commands, this implementation must change. Otherwise diff --git a/src/bin/d2/tests/d2_process_unittests.cc b/src/bin/d2/tests/d2_process_unittests.cc index bd6f77042a..025d501c7e 100644 --- a/src/bin/d2/tests/d2_process_unittests.cc +++ b/src/bin/d2/tests/d2_process_unittests.cc @@ -35,7 +35,6 @@ namespace { /// @brief Valid configuration containing an unavailable IP address. const char* bad_ip_d2_config = "{ " - "\"interface\" : \"eth1\" , " "\"ip_address\" : \"1.1.1.1\" , " "\"port\" : 5031, " "\"tsig_keys\": [" @@ -607,7 +606,6 @@ TEST_F(D2ProcessTest, fatalErrorShutdown) { /// loopback. TEST_F(D2ProcessTest, notLoopbackTest) { const char* config = "{ " - "\"interface\" : \"\" , " "\"ip_address\" : \"0.0.0.0\" , " "\"port\" : 53001, " "\"tsig_keys\": []," @@ -626,7 +624,6 @@ TEST_F(D2ProcessTest, notLoopbackTest) { /// DHCP_DDNS_NOT_ON_LOOPBACK is not issued. TEST_F(D2ProcessTest, v4LoopbackTest) { const char* config = "{ " - "\"interface\" : \"\" , " "\"ip_address\" : \"127.0.0.1\" , " "\"port\" : 53001, " "\"tsig_keys\": []," @@ -640,7 +637,6 @@ TEST_F(D2ProcessTest, v4LoopbackTest) { /// DHCP_DDNS_NOT_ON_LOOPBACK is not issued. TEST_F(D2ProcessTest, v6LoopbackTest) { const char* config = "{ " - "\"interface\" : \"\" , " "\"ip_address\" : \"::1\" , " "\"port\" : 53001, " "\"tsig_keys\": []," diff --git a/src/bin/d2/tests/d2_update_mgr_unittests.cc b/src/bin/d2/tests/d2_update_mgr_unittests.cc index f669aeb970..4b17575b70 100644 --- a/src/bin/d2/tests/d2_update_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_update_mgr_unittests.cc @@ -120,7 +120,6 @@ public: void makeCannedConfig() { std::string canned_config_ = "{ " - "\"interface\" : \"eth1\" , " "\"ip_address\" : \"192.168.1.33\" , " "\"port\" : 88 , " "\"tsig_keys\": [] ," From e703c60481806cb393529c264a74a06574ab3247 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 8 May 2014 06:57:26 -0400 Subject: [PATCH 5/9] [3268] Modified NameChangeTransaction to use configurable DNS server timeout Update manager now passes the D2 config manager reference into transactions when it creates them. This makes D2 configuration services available at the transaction level. Changed NameChangeTransaction::sendUpdate() to get the timeout value from configuration rather than use hard-coded constant. --- src/bin/d2/d2_update_mgr.cc | 6 ++- src/bin/d2/nc_add.cc | 6 ++- src/bin/d2/nc_add.h | 4 +- src/bin/d2/nc_remove.cc | 6 ++- src/bin/d2/nc_remove.h | 4 +- src/bin/d2/nc_trans.cc | 20 +++++---- src/bin/d2/nc_trans.h | 8 +++- src/bin/d2/tests/d2_update_mgr_unittests.cc | 19 +++------ src/bin/d2/tests/nc_add_unittests.cc | 17 +++++--- src/bin/d2/tests/nc_remove_unittests.cc | 17 +++++--- src/bin/d2/tests/nc_test_utils.cc | 2 +- src/bin/d2/tests/nc_test_utils.h | 1 + src/bin/d2/tests/nc_trans_unittests.cc | 45 ++++++++++++++------- 13 files changed, 97 insertions(+), 58 deletions(-) diff --git a/src/bin/d2/d2_update_mgr.cc b/src/bin/d2/d2_update_mgr.cc index 269610b7f7..7b0438dc96 100644 --- a/src/bin/d2/d2_update_mgr.cc +++ b/src/bin/d2/d2_update_mgr.cc @@ -195,10 +195,12 @@ D2UpdateMgr::makeTransaction(dhcp_ddns::NameChangeRequestPtr& next_ncr) { NameChangeTransactionPtr trans; if (next_ncr->getChangeType() == dhcp_ddns::CHG_ADD) { trans.reset(new NameAddTransaction(io_service_, next_ncr, - forward_domain, reverse_domain)); + forward_domain, reverse_domain, + cfg_mgr_)); } else { trans.reset(new NameRemoveTransaction(io_service_, next_ncr, - forward_domain, reverse_domain)); + forward_domain, reverse_domain, + cfg_mgr_)); } // Add the new transaction to the list. diff --git a/src/bin/d2/nc_add.cc b/src/bin/d2/nc_add.cc index e350534298..70fff23c99 100644 --- a/src/bin/d2/nc_add.cc +++ b/src/bin/d2/nc_add.cc @@ -38,8 +38,10 @@ NameAddTransaction:: NameAddTransaction(IOServicePtr& io_service, dhcp_ddns::NameChangeRequestPtr& ncr, DdnsDomainPtr& forward_domain, - DdnsDomainPtr& reverse_domain) - : NameChangeTransaction(io_service, ncr, forward_domain, reverse_domain) { + DdnsDomainPtr& reverse_domain, + D2CfgMgrPtr& cfg_mgr) + : NameChangeTransaction(io_service, ncr, forward_domain, reverse_domain, + cfg_mgr) { if (ncr->getChangeType() != isc::dhcp_ddns::CHG_ADD) { isc_throw (NameAddTransactionError, "NameAddTransaction, request type must be CHG_ADD"); diff --git a/src/bin/d2/nc_add.h b/src/bin/d2/nc_add.h index 1fe167b99e..7fcdf1f98d 100644 --- a/src/bin/d2/nc_add.h +++ b/src/bin/d2/nc_add.h @@ -87,13 +87,15 @@ public: /// @param ncr is the NameChangeRequest to fulfill /// @param forward_domain is the domain to use for forward DNS updates /// @param reverse_domain is the domain to use for reverse DNS updates + /// @param cfg_mgr pointer to the configuration manager /// /// @throw NameAddTransaction error if given request is not a CHG_ADD, /// NameChangeTransaction error for base class construction errors. NameAddTransaction(IOServicePtr& io_service, dhcp_ddns::NameChangeRequestPtr& ncr, DdnsDomainPtr& forward_domain, - DdnsDomainPtr& reverse_domain); + DdnsDomainPtr& reverse_domain, + D2CfgMgrPtr& cfg_mgr); /// @brief Destructor virtual ~NameAddTransaction(); diff --git a/src/bin/d2/nc_remove.cc b/src/bin/d2/nc_remove.cc index 5eb0521133..224974ad2d 100644 --- a/src/bin/d2/nc_remove.cc +++ b/src/bin/d2/nc_remove.cc @@ -35,8 +35,10 @@ NameRemoveTransaction:: NameRemoveTransaction(IOServicePtr& io_service, dhcp_ddns::NameChangeRequestPtr& ncr, DdnsDomainPtr& forward_domain, - DdnsDomainPtr& reverse_domain) - : NameChangeTransaction(io_service, ncr, forward_domain, reverse_domain) { + DdnsDomainPtr& reverse_domain, + D2CfgMgrPtr& cfg_mgr) + : NameChangeTransaction(io_service, ncr, forward_domain, reverse_domain, + cfg_mgr) { if (ncr->getChangeType() != isc::dhcp_ddns::CHG_REMOVE) { isc_throw (NameRemoveTransactionError, "NameRemoveTransaction, request type must be CHG_REMOVE"); diff --git a/src/bin/d2/nc_remove.h b/src/bin/d2/nc_remove.h index f7b0dcc081..fb79eb495d 100644 --- a/src/bin/d2/nc_remove.h +++ b/src/bin/d2/nc_remove.h @@ -83,13 +83,15 @@ public: /// @param ncr is the NameChangeRequest to fulfill /// @param forward_domain is the domain to use for forward DNS updates /// @param reverse_domain is the domain to use for reverse DNS updates + /// @param cfg_mgr pointer to the configuration manager /// /// @throw NameRemoveTransaction error if given request is not a CHG_REMOVE, /// NameChangeTransaction error for base class construction errors. NameRemoveTransaction(IOServicePtr& io_service, dhcp_ddns::NameChangeRequestPtr& ncr, DdnsDomainPtr& forward_domain, - DdnsDomainPtr& reverse_domain); + DdnsDomainPtr& reverse_domain, + D2CfgMgrPtr& cfg_mgr); /// @brief Destructor virtual ~NameRemoveTransaction(); diff --git a/src/bin/d2/nc_trans.cc b/src/bin/d2/nc_trans.cc index f9319f0596..f615a03572 100644 --- a/src/bin/d2/nc_trans.cc +++ b/src/bin/d2/nc_trans.cc @@ -41,22 +41,22 @@ const int NameChangeTransaction::UPDATE_FAILED_EVT; const int NameChangeTransaction::NCT_DERIVED_EVENT_MIN; -const unsigned int NameChangeTransaction::DNS_UPDATE_DEFAULT_TIMEOUT; const unsigned int NameChangeTransaction::MAX_UPDATE_TRIES_PER_SERVER; NameChangeTransaction:: NameChangeTransaction(IOServicePtr& io_service, dhcp_ddns::NameChangeRequestPtr& ncr, DdnsDomainPtr& forward_domain, - DdnsDomainPtr& reverse_domain) + DdnsDomainPtr& reverse_domain, + D2CfgMgrPtr& cfg_mgr) : io_service_(io_service), ncr_(ncr), forward_domain_(forward_domain), reverse_domain_(reverse_domain), dns_client_(), dns_update_request_(), dns_update_status_(DNSClient::OTHER), dns_update_response_(), forward_change_completed_(false), reverse_change_completed_(false), current_server_list_(), current_server_(), next_server_pos_(0), - update_attempts_(0) { - // @todo if io_service is NULL we are multi-threading and should - // instantiate our own + update_attempts_(0), cfg_mgr_(cfg_mgr) { + /// @todo if io_service is NULL we are multi-threading and should + /// instantiate our own if (!io_service_) { isc_throw(NameChangeTransactionError, "IOServicePtr cannot be null"); } @@ -75,6 +75,11 @@ NameChangeTransaction(IOServicePtr& io_service, isc_throw(NameChangeTransactionError, "Reverse change must have a reverse domain"); } + + if (!cfg_mgr_) { + isc_throw(NameChangeTransactionError, + "Configuration manager cannot be null"); + } } NameChangeTransaction::~NameChangeTransaction(){ @@ -171,11 +176,10 @@ NameChangeTransaction::sendUpdate(const std::string& comment, // use_tsig_ is true. We should be able to navigate to the TSIG key // for the current server. If not we would need to add that. - // @todo time out should ultimately be configurable, down to - // server level? + D2ParamsPtr d2_params = cfg_mgr_->getD2Params(); dns_client_->doUpdate(*io_service_, current_server_->getIpAddress(), current_server_->getPort(), *dns_update_request_, - DNS_UPDATE_DEFAULT_TIMEOUT); + d2_params->getDnsServerTimeout()); // Message is on its way, so the next event should be NOP_EVT. postNextEvent(NOP_EVT); diff --git a/src/bin/d2/nc_trans.h b/src/bin/d2/nc_trans.h index b747839e6e..71bcce3454 100644 --- a/src/bin/d2/nc_trans.h +++ b/src/bin/d2/nc_trans.h @@ -19,7 +19,7 @@ #include #include -#include +#include #include #include #include @@ -175,7 +175,8 @@ public: NameChangeTransaction(IOServicePtr& io_service, dhcp_ddns::NameChangeRequestPtr& ncr, DdnsDomainPtr& forward_domain, - DdnsDomainPtr& reverse_domain); + DdnsDomainPtr& reverse_domain, + D2CfgMgrPtr& cfg_mgr); /// @brief Destructor virtual ~NameChangeTransaction(); @@ -570,6 +571,9 @@ private: /// @brief Number of transmit attempts for the current request. size_t update_attempts_; + + /// @brief Pointer to the configuration manager. + D2CfgMgrPtr cfg_mgr_; }; /// @brief Defines a pointer to a NameChangeTransaction. diff --git a/src/bin/d2/tests/d2_update_mgr_unittests.cc b/src/bin/d2/tests/d2_update_mgr_unittests.cc index 4b17575b70..9718a4ae88 100644 --- a/src/bin/d2/tests/d2_update_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_update_mgr_unittests.cc @@ -201,29 +201,22 @@ public: /// timer, the number of passes through the loop is also limited to /// a given number. This is a failsafe to guard against an infinite loop /// in the test. - /// - /// @param timeout_millisec Maximum amount of time to allow IOService to - /// run before failing the test. Note, If the intent of a test is to - /// verify proper handling of DNSClient timeouts, the value must be - /// slightly larger than that being used for DNSClient timeout value. - /// @param max_passes Maximum number of times through the loop before - /// failing the test. The default value of twenty is likely large enough - /// for most tests. The number of passes required for a given test can - /// vary. - void processAll(unsigned int timeout_millisec = - NameChangeTransaction::DNS_UPDATE_DEFAULT_TIMEOUT + 100, - size_t max_passes = 100) { + void processAll(size_t max_passes = 100) { // Loop until all the transactions have been dequeued and run through to // completion. size_t passes = 0; size_t handlers = 0; + + // Set the timeout to slightly more than DNSClient timeout to allow + // timeout processing to occur naturally. + size_t timeout = cfg_mgr_->getD2Params()->getDnsServerTimeout() + 100; while (update_mgr_->getQueueCount() || update_mgr_->getTransactionCount()) { ++passes; update_mgr_->sweep(); // If any transactions are waiting on IO, run the service. if (anyoneWaiting()) { - int cnt = runTimedIO(timeout_millisec); + int cnt = runTimedIO(timeout); // If cnt is zero then the service stopped unexpectedly. if (cnt == 0) { diff --git a/src/bin/d2/tests/nc_add_unittests.cc b/src/bin/d2/tests/nc_add_unittests.cc index 8b91f5a595..bb8d841251 100644 --- a/src/bin/d2/tests/nc_add_unittests.cc +++ b/src/bin/d2/tests/nc_add_unittests.cc @@ -35,8 +35,10 @@ public: NameAddStub(IOServicePtr& io_service, dhcp_ddns::NameChangeRequestPtr& ncr, DdnsDomainPtr& forward_domain, - DdnsDomainPtr& reverse_domain) - : NameAddTransaction(io_service, ncr, forward_domain, reverse_domain), + DdnsDomainPtr& reverse_domain, + D2CfgMgrPtr& cfg_mgr) + : NameAddTransaction(io_service, ncr, forward_domain, reverse_domain, + cfg_mgr), simulate_send_exception_(false), simulate_build_request_exception_(false) { } @@ -212,7 +214,7 @@ public: // Now create the test transaction as would occur in update manager. return (NameAddStubPtr(new NameAddStub(io_service_, ncr_, forward_domain_, - reverse_domain_))); + reverse_domain_, cfg_mgr_))); } /// @brief Creates a transaction which requests an IPv6 DNS update. @@ -230,7 +232,8 @@ public: // Now create the test transaction as would occur in update manager. return (NameAddStubPtr(new NameAddStub(io_service_, ncr_, forward_domain_, - reverse_domain_))); + reverse_domain_, + cfg_mgr_))); } /// @brief Create a test transaction at a known point in the state model. @@ -266,6 +269,7 @@ public: /// 2. Valid construction functions properly TEST(NameAddTransaction, construction) { IOServicePtr io_service(new isc::asiolink::IOService()); + D2CfgMgrPtr cfg_mgr(new D2CfgMgr()); const char* msg_str = "{" @@ -291,13 +295,14 @@ TEST(NameAddTransaction, construction) { // Verify that construction with wrong change type fails. EXPECT_THROW(NameAddTransaction(io_service, ncr, - forward_domain, reverse_domain), + forward_domain, reverse_domain, cfg_mgr), NameAddTransactionError); // Verify that a valid construction attempt works. ncr->setChangeType(isc::dhcp_ddns::CHG_ADD); EXPECT_NO_THROW(NameAddTransaction(io_service, ncr, - forward_domain, reverse_domain)); + forward_domain, reverse_domain, + cfg_mgr)); } /// @brief Tests event and state dictionary construction and verification. diff --git a/src/bin/d2/tests/nc_remove_unittests.cc b/src/bin/d2/tests/nc_remove_unittests.cc index 6ca4ecf87e..8e19e6eeed 100644 --- a/src/bin/d2/tests/nc_remove_unittests.cc +++ b/src/bin/d2/tests/nc_remove_unittests.cc @@ -35,9 +35,10 @@ public: NameRemoveStub(IOServicePtr& io_service, dhcp_ddns::NameChangeRequestPtr& ncr, DdnsDomainPtr& forward_domain, - DdnsDomainPtr& reverse_domain) + DdnsDomainPtr& reverse_domain, + D2CfgMgrPtr& cfg_mgr) : NameRemoveTransaction(io_service, ncr, forward_domain, - reverse_domain), + reverse_domain, cfg_mgr), simulate_send_exception_(false), simulate_build_request_exception_(false) { } @@ -212,7 +213,8 @@ public: // Now create the test transaction as would occur in update manager. return (NameRemoveStubPtr(new NameRemoveStub(io_service_, ncr_, forward_domain_, - reverse_domain_))); + reverse_domain_, + cfg_mgr_))); } /// @brief Creates a transaction which requests an IPv6 DNS update. @@ -230,7 +232,8 @@ public: // Now create the test transaction as would occur in update manager. return (NameRemoveStubPtr(new NameRemoveStub(io_service_, ncr_, forward_domain_, - reverse_domain_))); + reverse_domain_, + cfg_mgr_))); } /// @brief Create a test transaction at a known point in the state model. @@ -268,6 +271,7 @@ public: /// 2. Valid construction functions properly TEST(NameRemoveTransaction, construction) { IOServicePtr io_service(new isc::asiolink::IOService()); + D2CfgMgrPtr cfg_mgr(new D2CfgMgr()); const char* msg_str = "{" @@ -293,13 +297,14 @@ TEST(NameRemoveTransaction, construction) { // Verify that construction with wrong change type fails. EXPECT_THROW(NameRemoveTransaction(io_service, ncr, - forward_domain, reverse_domain), + forward_domain, reverse_domain, cfg_mgr), NameRemoveTransactionError); // Verify that a valid construction attempt works. ncr->setChangeType(isc::dhcp_ddns::CHG_REMOVE); EXPECT_NO_THROW(NameRemoveTransaction(io_service, ncr, - forward_domain, reverse_domain)); + forward_domain, reverse_domain, + cfg_mgr)); } /// @brief Tests event and state dictionary construction and verification. diff --git a/src/bin/d2/tests/nc_test_utils.cc b/src/bin/d2/tests/nc_test_utils.cc index 957688aad6..2f633abc52 100644 --- a/src/bin/d2/tests/nc_test_utils.cc +++ b/src/bin/d2/tests/nc_test_utils.cc @@ -197,7 +197,7 @@ const unsigned int TransactionTest::FORWARD_CHG = 0x01; const unsigned int TransactionTest::REVERSE_CHG = 0x02; const unsigned int TransactionTest::FWD_AND_REV_CHG = REVERSE_CHG | FORWARD_CHG; -TransactionTest::TransactionTest() : ncr_() { +TransactionTest::TransactionTest() : ncr_(), cfg_mgr_(new D2CfgMgr()) { } TransactionTest::~TransactionTest() { diff --git a/src/bin/d2/tests/nc_test_utils.h b/src/bin/d2/tests/nc_test_utils.h index a99752528c..e681edcbfa 100644 --- a/src/bin/d2/tests/nc_test_utils.h +++ b/src/bin/d2/tests/nc_test_utils.h @@ -166,6 +166,7 @@ public: dhcp_ddns::NameChangeRequestPtr ncr_; DdnsDomainPtr forward_domain_; DdnsDomainPtr reverse_domain_; + D2CfgMgrPtr cfg_mgr_; /// #brief constants used to specify change directions for a transaction. static const unsigned int FORWARD_CHG; // Only forward change. diff --git a/src/bin/d2/tests/nc_trans_unittests.cc b/src/bin/d2/tests/nc_trans_unittests.cc index 5784647fd2..60bfb56c46 100644 --- a/src/bin/d2/tests/nc_trans_unittests.cc +++ b/src/bin/d2/tests/nc_trans_unittests.cc @@ -28,11 +28,13 @@ #include #include +#include #include using namespace std; using namespace isc; using namespace isc::d2; +using namespace boost::posix_time; namespace { @@ -59,10 +61,11 @@ public: /// Parameters match those needed by NameChangeTransaction. NameChangeStub(IOServicePtr& io_service, dhcp_ddns::NameChangeRequestPtr& ncr, - DdnsDomainPtr forward_domain, - DdnsDomainPtr reverse_domain) + DdnsDomainPtr& forward_domain, + DdnsDomainPtr& reverse_domain, + D2CfgMgrPtr& cfg_mgr) : NameChangeTransaction(io_service, ncr, forward_domain, - reverse_domain), + reverse_domain, cfg_mgr), use_stub_callback_(false) { } @@ -296,7 +299,7 @@ public: // Now create the test transaction as would occur in update manager. // Instantiate the transaction as would be done by update manager. return (NameChangeStubPtr(new NameChangeStub(io_service_, ncr_, - forward_domain_, reverse_domain_))); + forward_domain_, reverse_domain_, cfg_mgr_))); } /// @brief Builds and then sends an update request @@ -348,6 +351,7 @@ public: /// 4. Valid construction functions properly TEST(NameChangeTransaction, construction) { IOServicePtr io_service(new isc::asiolink::IOService()); + D2CfgMgrPtr cfg_mgr(new D2CfgMgr()); const char* msg_str = "{" @@ -377,43 +381,54 @@ TEST(NameChangeTransaction, construction) { // @todo Subject to change if multi-threading is implemented. IOServicePtr empty; EXPECT_THROW(NameChangeTransaction(empty, ncr, - forward_domain, reverse_domain), + forward_domain, reverse_domain, cfg_mgr), NameChangeTransactionError); // Verify that construction with an empty NameChangeRequest throws. EXPECT_THROW(NameChangeTransaction(io_service, empty_ncr, - forward_domain, reverse_domain), + forward_domain, reverse_domain, cfg_mgr), NameChangeTransactionError); + // Verify that construction with an empty D2CfgMgr throws. + D2CfgMgrPtr empty_cfg; + EXPECT_THROW(NameChangeTransaction(io_service, empty_ncr, + forward_domain, reverse_domain, + empty_cfg), + NameChangeTransactionError); + + // Verify that construction with an empty forward domain when the // NameChangeRequest calls for a forward change throws. EXPECT_THROW(NameChangeTransaction(io_service, ncr, - empty_domain, reverse_domain), + empty_domain, reverse_domain, cfg_mgr), NameChangeTransactionError); // Verify that construction with an empty reverse domain when the // NameChangeRequest calls for a reverse change throws. EXPECT_THROW(NameChangeTransaction(io_service, ncr, - forward_domain, empty_domain), + forward_domain, empty_domain, cfg_mgr), NameChangeTransactionError); // Verify that a valid construction attempt works. EXPECT_NO_THROW(NameChangeTransaction(io_service, ncr, - forward_domain, reverse_domain)); + forward_domain, reverse_domain, + cfg_mgr)); // Verify that an empty forward domain is allowed when the requests does // not include a forward change. ncr->setForwardChange(false); ncr->setReverseChange(true); EXPECT_NO_THROW(NameChangeTransaction(io_service, ncr, - empty_domain, reverse_domain)); + empty_domain, reverse_domain, + cfg_mgr)); // Verify that an empty reverse domain is allowed when the requests does // not include a reverse change. ncr->setForwardChange(true); ncr->setReverseChange(false); EXPECT_NO_THROW(NameChangeTransaction(io_service, ncr, - forward_domain, empty_domain)); + forward_domain, empty_domain, + cfg_mgr)); } /// @brief General testing of member accessors. @@ -944,9 +959,11 @@ TEST_F(NameChangeTransactionTest, sendUpdateTimeout) { // not only it but the invoking test as well. In other words, if the // doOneExchange blows up the rest of test is pointless. I use // ASSERT_NO_FATAL_FAILURE to abort the test immediately. - ASSERT_NO_FATAL_FAILURE(doOneExchange(name_change, - NameChangeTransaction:: - DNS_UPDATE_DEFAULT_TIMEOUT + 100)); + + D2ParamsPtr d2_params = cfg_mgr_->getD2Params(); + size_t timeout = d2_params->getDnsServerTimeout() + 100; + + ASSERT_NO_FATAL_FAILURE(doOneExchange(name_change, timeout)); // Verify that next event is IO_COMPLETED_EVT and DNS status is TIMEOUT. ASSERT_EQ(NameChangeTransaction::IO_COMPLETED_EVT, From 83cbea18e3ca4eb2e12fecfe7af78dc854302285 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 8 May 2014 09:32:19 -0400 Subject: [PATCH 6/9] [3268] Updated bind10 guide with new D2 global parameters --- doc/guide/bind10-guide.xml | 43 ++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/doc/guide/bind10-guide.xml b/doc/guide/bind10-guide.xml index 44d804c7c1..7eaef3e44f 100644 --- a/doc/guide/bind10-guide.xml +++ b/doc/guide/bind10-guide.xml @@ -5221,9 +5221,11 @@ Dhcp6/renew-timer 1000 integer (default) configuration will be available. It will look similar to this: > config show DhcpDdns -DhcpDdns/interface "eth0" string (default) DhcpDdns/ip_address "127.0.0.1" string (default) DhcpDdns/port 53001 integer (default) +DhcpDdns/dns_server_timeout 100 integer (default) +DhcpDdns/ncr-protocol "UDP" string (default) +DhcpDdns/ncr-format "JSON" string (default) DhcpDdns/tsig_keys [] list (default) DhcpDdns/forward_ddns/ddns_domains [] list (default) DhcpDdns/reverse_ddns/ddns_domains [] list (default) @@ -5240,7 +5242,7 @@ DhcpDdns/reverse_ddns/ddns_domains [] list (default) - General Server Parameters — + Global Server Parameters — values which control connectivity and global server behavior @@ -5264,13 +5266,37 @@ DhcpDdns/reverse_ddns/ddns_domains [] list (default)
- General Server Parameters + Global Server Parameters + + + ip_address - IP address on which D2 listens for requests. The default is + the local loopback interface at address 127.0.0.1. You may specify + either an IPv4 or IPv6 address. + + + port - Port on which D2 listens for requests. The default value + is 53001. + + + ncr-format - Socket protocol use when sending requests to D2. Currently + only UDP is supported. TCP may be available in an upcoming release. + + + ncr-protocol - Packet format to use when sending requests to D2. + Currently only JSON format is supported. Other formats may be available + in future releases. + + + dns_server_timeout - The maximum amount of time in milliseconds, that + D2 will wait for a response from a DNS server to a single DDNS update + message. + + - The DHCP-DDNS server must listen for requests on a known address and - port. By default, it will listen at 127.0.0.1 on port 53001. This is - governed by the parameters, "ip-address" and "port". Either value - may be changed using config set/commit. For example to change the - server to listen at 192.168.1.10 port 900: + D2 must listen for change requests on a known address and port. By + default it listens at 127.0.0.1 on port 53001. The following example + illustrates how to change D2's global parameters so it will listen + at 192.168.1.10 port 900: > config set DhcpDdns/ip_address "192.168.1.10" > config set DhcpDdns/port 900 @@ -5290,7 +5316,6 @@ DhcpDdns/reverse_ddns/ddns_domains [] list (default) authentication to guard against such attacks. - If the ip_address and port are changed, it will be necessary to change the From e76ef58ca396e713a6e15c1733e955df8893473e Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 19 May 2014 16:03:31 +0200 Subject: [PATCH 7/9] [3268] Fixed a couple of typos during a review. --- src/bin/d2/d2_cfg_mgr.h | 4 ++-- src/bin/d2/d2_config.cc | 4 ++-- src/bin/d2/d2_process.cc | 4 ++-- src/bin/d2/tests/d2_cfg_mgr_unittests.cc | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/bin/d2/d2_cfg_mgr.h b/src/bin/d2/d2_cfg_mgr.h index e12557be64..08032213a8 100644 --- a/src/bin/d2/d2_cfg_mgr.h +++ b/src/bin/d2/d2_cfg_mgr.h @@ -53,7 +53,7 @@ public: return (DCfgContextBasePtr(new D2CfgContext(*this))); } - /// @brief Fetches a reference to the D2params + /// @brief Fetches a reference to the D2Params D2ParamsPtr& getD2Params() { return (d2_params_); } @@ -243,7 +243,7 @@ protected: /// /// Iterates over the set of parameters, creating a parser based on the /// parameter's id and then invoking its build method passing in the - /// paramter's configuration value. + /// parameter's configuration value. /// /// @param params_config set of scalar configuration elements to parse virtual void buildParams(isc::data::ConstElementPtr params_config); diff --git a/src/bin/d2/d2_config.cc b/src/bin/d2/d2_config.cc index d933047651..3865063dec 100644 --- a/src/bin/d2/d2_config.cc +++ b/src/bin/d2/d2_config.cc @@ -38,8 +38,8 @@ const char *D2Params::DFT_NCR_FORMAT = "JSON"; D2Params::D2Params(const isc::asiolink::IOAddress& ip_address, const size_t port, const size_t dns_server_timeout, - const dhcp_ddns:: NameChangeProtocol& ncr_protocol, - const dhcp_ddns:: NameChangeFormat& ncr_format) + const dhcp_ddns::NameChangeProtocol& ncr_protocol, + const dhcp_ddns::NameChangeFormat& ncr_format) : ip_address_(ip_address), port_(port), dns_server_timeout_(dns_server_timeout), diff --git a/src/bin/d2/d2_process.cc b/src/bin/d2/d2_process.cc index e9ce245616..acd58bc814 100644 --- a/src/bin/d2/d2_process.cc +++ b/src/bin/d2/d2_process.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2014 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 @@ -207,7 +207,7 @@ D2Process::configure(isc::data::ConstElementPtr config_set) { if (rcode) { // Non-zero means we got an invalid configuration, take no further - // action. In integrated mode, this will send a failed response back + // action. In integrated mode, this will send a failed response back // to BIND10. reconf_queue_flag_ = false; return (answer); diff --git a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc index a9c476b9e5..be9caa7fdd 100644 --- a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc @@ -39,7 +39,7 @@ class D2CfgMgrTest : public ConfigParseTest { public: /// @brief Constructor - D2CfgMgrTest():cfg_mgr_(new D2CfgMgr), d2_params_() { + D2CfgMgrTest():cfg_mgr_(new D2CfgMgr()), d2_params_() { } /// @brief Destructor From ff3fc5e41f5e45bbc0709e0ad28dd64579d81850 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Tue, 20 May 2014 07:16:05 -0400 Subject: [PATCH 8/9] [3432] Addressed review comments Added missing commentary, corrected typos. --- doc/guide/bind10-guide.xml | 15 +++++++------- src/bin/d2/d2_cfg_mgr.h | 9 ++++++++ src/bin/d2/d2_config.cc | 9 +++----- src/bin/d2/d2_config.h | 26 ++++++++++++++++++++---- src/bin/d2/d_cfg_mgr.cc | 10 ++++----- src/bin/d2/d_cfg_mgr.h | 17 +++++++++------- src/bin/d2/nc_trans.h | 1 + src/bin/d2/tests/d2_cfg_mgr_unittests.cc | 16 ++++++++++++--- src/bin/d2/tests/d_cfg_mgr_unittests.cc | 3 --- 9 files changed, 69 insertions(+), 37 deletions(-) diff --git a/doc/guide/bind10-guide.xml b/doc/guide/bind10-guide.xml index 7eaef3e44f..0bc4e7ecad 100644 --- a/doc/guide/bind10-guide.xml +++ b/doc/guide/bind10-guide.xml @@ -5224,8 +5224,8 @@ Dhcp6/renew-timer 1000 integer (default) DhcpDdns/ip_address "127.0.0.1" string (default) DhcpDdns/port 53001 integer (default) DhcpDdns/dns_server_timeout 100 integer (default) -DhcpDdns/ncr-protocol "UDP" string (default) -DhcpDdns/ncr-format "JSON" string (default) +DhcpDdns/ncr_protocol "UDP" string (default) +DhcpDdns/ncr_format "JSON" string (default) DhcpDdns/tsig_keys [] list (default) DhcpDdns/forward_ddns/ddns_domains [] list (default) DhcpDdns/reverse_ddns/ddns_domains [] list (default) @@ -5278,17 +5278,18 @@ DhcpDdns/reverse_ddns/ddns_domains [] list (default) is 53001. - ncr-format - Socket protocol use when sending requests to D2. Currently - only UDP is supported. TCP may be available in an upcoming release. + ncr_format - Socket protocol to use when sending requests to D2. + Currently only UDP is supported. TCP may be available in an upcoming + release. - ncr-protocol - Packet format to use when sending requests to D2. + ncr_protocol - Packet format to use when sending requests to D2. Currently only JSON format is supported. Other formats may be available in future releases. dns_server_timeout - The maximum amount of time in milliseconds, that - D2 will wait for a response from a DNS server to a single DDNS update + D2 will wait for a response from a DNS server to a single DNS update message. @@ -5302,8 +5303,6 @@ DhcpDdns/reverse_ddns/ddns_domains [] list (default) > config set DhcpDdns/port 900 > config commit - The server may be configured to listen over IPv4 or IPv6, therefore - ip-address may an IPv4 or IPv6 address. diff --git a/src/bin/d2/d2_cfg_mgr.h b/src/bin/d2/d2_cfg_mgr.h index 08032213a8..dcc03e1e8f 100644 --- a/src/bin/d2/d2_cfg_mgr.h +++ b/src/bin/d2/d2_cfg_mgr.h @@ -270,6 +270,15 @@ protected: virtual isc::dhcp::ParserPtr createConfigParser(const std::string& element_id); + /// @brief Creates an new, blank D2CfgContext context + /// + /// This method is used at the beginning of configuration process to + /// create a fresh, empty copy of a D2CfgContext. This new context will + /// be populated during the configuration process and will replace the + /// existing context provided the configuration process completes without + /// error. + /// + /// @return Returns a DCfgContextBasePtr to the new context instance. virtual DCfgContextBasePtr createNewContext(); }; diff --git a/src/bin/d2/d2_config.cc b/src/bin/d2/d2_config.cc index 3865063dec..c8e331e30e 100644 --- a/src/bin/d2/d2_config.cc +++ b/src/bin/d2/d2_config.cc @@ -61,12 +61,9 @@ D2Params::~D2Params(){}; void D2Params::validateContents() { - if (ip_address_.toText() == "0.0.0.0") { - isc_throw(D2CfgError, "D2Params: IP address cannot be \"0.0.0.0\""); - } - - if (ip_address_.toText() == "::") { - isc_throw(D2CfgError, "D2Params: IP address cannot be \"::\""); + if ((ip_address_.toText() == "0.0.0.0") || (ip_address_.toText() == "::")) { + isc_throw(D2CfgError, + "D2Params: IP address cannot be \"" << ip_address_ << "\""); } if (port_ == 0) { diff --git a/src/bin/d2/d2_config.h b/src/bin/d2/d2_config.h index 8c5dd29cd4..168559b192 100644 --- a/src/bin/d2/d2_config.h +++ b/src/bin/d2/d2_config.h @@ -147,6 +147,7 @@ public: class D2Params { public: /// @brief Default configuration constants. + //@{ /// @todo For now these are hard-coded as configuration layer cannot /// readily provide them (see Trac #3358). static const char *DFT_IP_ADDRESS; @@ -154,13 +155,26 @@ public: static const size_t DFT_DNS_SERVER_TIMEOUT; static const char *DFT_NCR_PROTOCOL; static const char *DFT_NCR_FORMAT; + //@} /// @brief Constructor /// - /// @throw D2CfgError if given an invalid protocol or format. + /// @param ip_address IP address at which D2 should listen for NCRs + /// @param port port on which D2 should listen NCRs + /// @param dns_server_timeout maximum amount of time in milliseconds to + /// wait for a response to a single DNS update request. + /// @param ncr_protocol socket protocol D2 should use to receive NCRS + /// @param ncr_format packet format of the inbound NCRs + /// + /// @throw D2CfgError if: + /// -# ip_address is 0.0.0.0 or :: + /// -# port is 0 + /// -# dns_server_timeout is < 1 + /// -# ncr_protocol is invalid, currently only NCR_UDP is supported + /// -# ncr_format is invalid, currently only FMT_JSON is supported D2Params(const isc::asiolink::IOAddress& ip_address, const size_t port, - const size_t DFT_DNS_SERVER_TIMEOUT, + const size_t dns_server_timeout, const dhcp_ddns::NameChangeProtocol& ncr_protocol, const dhcp_ddns::NameChangeFormat& ncr_format); @@ -171,12 +185,12 @@ public: /// @brief Destructor virtual ~D2Params(); - /// @brief Return the IP D2 listens on. + /// @brief Return the IP address D2 listens on. const isc::asiolink::IOAddress& getIpAddress() const { return(ip_address_); } - /// @brief Return the IP port D2 listens on. + /// @brief Return the TCP/UPD port D2 listens on. size_t getPort() const { return(port_); } @@ -238,6 +252,10 @@ private: dhcp_ddns::NameChangeFormat ncr_format_; }; +/// @brief Dumps the contents of a D2Params as text to an output stream +/// +/// @param os output stream to which text should be sent +/// @param config D2Param instnace to dump std::ostream& operator<<(std::ostream& os, const D2Params& config); diff --git a/src/bin/d2/d_cfg_mgr.cc b/src/bin/d2/d_cfg_mgr.cc index 6be057eaca..151202f360 100644 --- a/src/bin/d2/d_cfg_mgr.cc +++ b/src/bin/d2/d_cfg_mgr.cc @@ -135,7 +135,6 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { // inconsistency if the parsing operation fails after the context has been // modified. We need to preserve the original context here // so as we can rollback changes when an error occurs. -// DCfgContextBasePtr original_context = context_->clone(); DCfgContextBasePtr original_context = context_; resetContext(); @@ -178,7 +177,6 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { buildParams(params_config); // Now parse the configuration objects. - const ElementMap& values_map = objects_map; // Use a pre-ordered list of element ids to parse the elements in a // specific order if the list (parser_order_) is not empty; otherwise @@ -194,8 +192,8 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { int parsed_count = 0; std::map::const_iterator it; BOOST_FOREACH(element_id, parse_order_) { - it = values_map.find(element_id); - if (it != values_map.end()) { + it = objects_map.find(element_id); + if (it != objects_map.end()) { ++parsed_count; buildAndCommit(element_id, it->second); } @@ -218,7 +216,7 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { // or we parsed fewer than are in the map; then either the parse i // order is incomplete OR the map has unsupported values. if (!parsed_count || - (parsed_count && ((parsed_count + 1) < values_map.size()))) { + (parsed_count && ((parsed_count + 1) < objects_map.size()))) { LOG_ERROR(dctl_logger, DCTL_ORDER_ERROR); isc_throw(DCfgMgrBaseError, "Configuration contains elements not in parse order"); @@ -227,7 +225,7 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { // Order doesn't matter so iterate over the value map directly. // Pass each element and it's associated data in to be parsed. ConfigPair config_pair; - BOOST_FOREACH(config_pair, values_map) { + BOOST_FOREACH(config_pair, objects_map) { element_id = config_pair.first; buildAndCommit(element_id, config_pair.second); } diff --git a/src/bin/d2/d_cfg_mgr.h b/src/bin/d2/d_cfg_mgr.h index 92468835a4..d088feb8db 100644 --- a/src/bin/d2/d_cfg_mgr.h +++ b/src/bin/d2/d_cfg_mgr.h @@ -116,7 +116,7 @@ public: /// into parsers. /// /// @return returns a pointer to the Boolean Storage. - isc::dhcp::BooleanStoragePtr& getBooleanStorage() { + isc::dhcp::BooleanStoragePtr getBooleanStorage() { return (boolean_values_); } @@ -124,7 +124,7 @@ public: /// into parsers. /// /// @return returns a pointer to the uint32 Storage. - isc::dhcp::Uint32StoragePtr& getUint32Storage() { + isc::dhcp::Uint32StoragePtr getUint32Storage() { return (uint32_values_); } @@ -132,7 +132,7 @@ public: /// into parsers. /// /// @return returns a pointer to the string Storage. - isc::dhcp::StringStoragePtr& getStringStorage() { + isc::dhcp::StringStoragePtr getStringStorage() { return (string_values_); } @@ -184,10 +184,7 @@ private: isc::dhcp::StringStoragePtr string_values_; }; -/// @brief Defines an unsorted, list of string Element IDs. -typedef std::vector ElementIdList; - -/// @brief Defines an unsorted, list of string Element IDs. +/// @brief Defines a sequence of Element IDs used to specify a parsing order. typedef std::vector ElementIdList; /// @brief Configuration Manager @@ -327,6 +324,12 @@ protected: /// @brief Abstract factory which creates a context instance. /// + /// This method is used at the beginning of configuration process to + /// create a fresh, empty copy of the derivation-specific context. This + /// new context will be populated during the configuration process + /// and will replace the existing context provided the configuration + /// process completes without error. + /// /// @return Returns a DCfgContextBasePtr to the new context instance. virtual DCfgContextBasePtr createNewContext() = 0; diff --git a/src/bin/d2/nc_trans.h b/src/bin/d2/nc_trans.h index 71bcce3454..17f7f46ce9 100644 --- a/src/bin/d2/nc_trans.h +++ b/src/bin/d2/nc_trans.h @@ -168,6 +168,7 @@ public: /// @param ncr is the NameChangeRequest to fulfill /// @param forward_domain is the domain to use for forward DNS updates /// @param reverse_domain is the domain to use for reverse DNS updates + /// @param cfg_mgr reference to the current configuration manager /// /// @throw NameChangeTransactionError if given an null request, /// if forward change is enabled but forward domain is null, if diff --git a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc index be9caa7fdd..79d162ba7b 100644 --- a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc @@ -82,6 +82,18 @@ public: return (config.str()); } + /// @brief Parses a configuration string and tests against a given outcome + /// + /// Convenience method which accepts JSON text and an expected pass or fail + /// outcome. It converts the text into an ElementPtr and passes that to + /// configuration manager's parseConfig method. It then tests the + /// parse result against the expected outcome If they do not match it + /// the method asserts a failure. If they do match, it refreshes the + /// the D2Params pointer with the newly parsed instance. + /// + /// @param config_str the JSON configuration text to parse + /// @param should_fail boolean indicator if the parsing should fail or not. + /// It defaults to false. void runConfig(std::string config_str, bool should_fail=false) { // We assume the config string is valid JSON. ASSERT_TRUE(fromJSON(config_str)); @@ -99,6 +111,7 @@ public: ASSERT_TRUE(d2_params_); } + /// @brief Pointer the D2Params most recently parsed. D2ParamsPtr d2_params_; }; @@ -1060,9 +1073,6 @@ TEST(D2CfgMgr, construction) { EXPECT_NO_THROW(delete cfg_mgr); } -TEST_F(D2CfgMgrTest, paramsConfig) { -} - /// @brief Tests the parsing of a complete, valid DHCP-DDNS configuration. /// This tests passes the configuration into an instance of D2CfgMgr just /// as it would be done by d2_process in response to a configuration update diff --git a/src/bin/d2/tests/d_cfg_mgr_unittests.cc b/src/bin/d2/tests/d_cfg_mgr_unittests.cc index 235e954e78..88438d215d 100644 --- a/src/bin/d2/tests/d_cfg_mgr_unittests.cc +++ b/src/bin/d2/tests/d_cfg_mgr_unittests.cc @@ -205,9 +205,6 @@ TEST_F(DStubCfgMgrTest, parseOrderTest) { // Verify that the parsed order matches what we expected. EXPECT_TRUE(cfg_mgr_->parsed_order_ == order_expected); - for (int i = 0; i < cfg_mgr_->parsed_order_.size(); ++i) { - std::cout << i << ":" << cfg_mgr_->parsed_order_[i] << std::endl; - } // Clear the manager's parse order "memory". cfg_mgr_->parsed_order_.clear(); From 0387d65f6d395b5f69f514731f4ff628e5df6828 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Tue, 20 May 2014 07:27:22 -0400 Subject: [PATCH 9/9] [3432] Fixed compilation error in d2_cfg_mgr.cc Minor compilation error incurred after review changes. --- examples/m4/ax_isc_rpath.m4 | 8 ++++++-- src/bin/d2/d2_cfg_mgr.cc | 4 ++-- src/bin/d2/tests/nc_test_utils.cc | 2 +- src/lib/dns/tests/labelsequence_unittest.cc | 2 ++ 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/examples/m4/ax_isc_rpath.m4 b/examples/m4/ax_isc_rpath.m4 index ee1e472e14..ce8d3226f1 100644 --- a/examples/m4/ax_isc_rpath.m4 +++ b/examples/m4/ax_isc_rpath.m4 @@ -41,8 +41,12 @@ if test x$rpath != xno; then ISC_RPATH_FLAG=-Wl,-R ],[ AC_MSG_RESULT(no) AC_MSG_CHECKING([whether -R flag is available in linker]) - CXXFLAGS="$CXXFLAGS_SAVED -R" - CCFLAGS="$CCFLAGS_SAVED -R" + + # Apple clang 5.1 is now considers unknown parameters passed to linker (ld) as errors. + # However, the same unknown parameters passed to compiler (g++ ) are merely threated + # as warnings. To make sure that we pick those up, is to use -Werror. + CXXFLAGS="$CXXFLAGS_SAVED -R/usr/lib" + CCFLAGS="$CCFLAGS_SAVED -R/usr/lib" AC_TRY_LINK([], [], [ AC_MSG_RESULT([yes; note that -R is more sensitive about the position in option arguments]) ISC_RPATH_FLAG=-R diff --git a/src/bin/d2/d2_cfg_mgr.cc b/src/bin/d2/d2_cfg_mgr.cc index 28796f54dc..536c9cfa24 100644 --- a/src/bin/d2/d2_cfg_mgr.cc +++ b/src/bin/d2/d2_cfg_mgr.cc @@ -204,12 +204,12 @@ D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) { // Fetch the parameters from the context to create the D2Params. D2CfgContextPtr context = getD2CfgContext(); - isc::dhcp::StringStoragePtr& strings = context->getStringStorage(); + isc::dhcp::StringStoragePtr strings = context->getStringStorage(); asiolink::IOAddress ip_address(strings-> getOptionalParam("ip_address", D2Params::DFT_IP_ADDRESS)); - isc::dhcp::Uint32StoragePtr& ints = context->getUint32Storage(); + isc::dhcp::Uint32StoragePtr ints = context->getUint32Storage(); uint32_t port = ints->getOptionalParam("port", D2Params::DFT_PORT); uint32_t dns_server_timeout diff --git a/src/bin/d2/tests/nc_test_utils.cc b/src/bin/d2/tests/nc_test_utils.cc index 2f633abc52..69e766fa0a 100644 --- a/src/bin/d2/tests/nc_test_utils.cc +++ b/src/bin/d2/tests/nc_test_utils.cc @@ -31,7 +31,7 @@ namespace d2 { const char* TEST_DNS_SERVER_IP = "127.0.0.1"; size_t TEST_DNS_SERVER_PORT = 5301; -const bool HAS_RDATA = true; +//const bool HAS_RDATA = true; const bool NO_RDATA = false; //*************************** FauxServer class *********************** diff --git a/src/lib/dns/tests/labelsequence_unittest.cc b/src/lib/dns/tests/labelsequence_unittest.cc index c211a4b897..63ae464de3 100644 --- a/src/lib/dns/tests/labelsequence_unittest.cc +++ b/src/lib/dns/tests/labelsequence_unittest.cc @@ -649,6 +649,7 @@ const char* const root_servers[] = { "j.root-servers.net", "k.root-servers.net", "l.root-servers.net", "m.root-servers.net", NULL }; +#if 0 const char* const gtld_servers[] = { "a.gtld-servers.net", "b.gtld-servers.net", "c.gtld-servers.net", "d.gtld-servers.net", "e.gtld-servers.net", "f.gtld-servers.net", @@ -656,6 +657,7 @@ const char* const gtld_servers[] = { "j.gtld-servers.net", "k.gtld-servers.net", "l.gtld-servers.net", "m.gtld-servers.net", NULL }; +#endif const char* const jp_servers[] = { "a.dns.jp", "b.dns.jp", "c.dns.jp", "d.dns.jp", "e.dns.jp", "f.dns.jp", "g.dns.jp", NULL