From: Thomas Markwalder Date: Thu, 26 Jun 2014 14:03:08 +0000 (-0400) Subject: [3436] Added position info to top level parser creation in D2 X-Git-Tag: trac3473_base~5^2~9 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e10603ef674f60c36acbe1b9cc22bbb1a0022816;p=thirdparty%2Fkea.git [3436] Added position info to top level parser creation in D2 Added element position argument to DCfgMgrBase::createConfigParser(), so derivations have access to print position info unsupported top level element errors. Removed two log messages DCT_ORDER_ERROR and DCT_ORDER_NO_ELEMENT. These conditions are well explained in exceptions thrown and these logs just cluttered the log output. Removed extra text from DCTL_CONFIG_LOAD_FAIL and DCTL_PARSER_FAIL log messages. The log ID is self-explanatory and the underlying exceptions provide ample explanation of the error. Makes the log output much easier to understand. Revised items-not-in-parse-order detection in DCfgMgrBase::parseConfig(). Rather than complicated counting logic, objects are removed from the list as they are parsed. Any left over were not in the parsing-order. Removed try-catch-throw from DCfgMgrBase::buildAndCommit. This method already throws its own exception. Catching, logging, and re-throwing exceptions from underneath it really just server to clutter the log. --- diff --git a/src/bin/d2/d2_cfg_mgr.cc b/src/bin/d2/d2_cfg_mgr.cc index 536c9cfa24..5367d7ebc3 100644 --- a/src/bin/d2/d2_cfg_mgr.cc +++ b/src/bin/d2/d2_cfg_mgr.cc @@ -234,7 +234,8 @@ D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) { } isc::dhcp::ParserPtr -D2CfgMgr::createConfigParser(const std::string& config_id) { +D2CfgMgr::createConfigParser(const std::string& config_id, + const isc::data::Element::Position& pos) { // Get D2 specific context. D2CfgContextPtr context = getD2CfgContext(); @@ -263,7 +264,7 @@ D2CfgMgr::createConfigParser(const std::string& config_id) { } else { isc_throw(NotImplemented, "parser error: D2CfgMgr parameter not supported: " - << config_id); + << config_id << pos); } return (parser); diff --git a/src/bin/d2/d2_cfg_mgr.h b/src/bin/d2/d2_cfg_mgr.h index dcc03e1e8f..f62d680838 100644 --- a/src/bin/d2/d2_cfg_mgr.h +++ b/src/bin/d2/d2_cfg_mgr.h @@ -264,11 +264,15 @@ protected: /// /// @param element_id is the string name of the element as it will appear /// in the configuration set. + /// @param pos position within the configuration text (or file) of element + /// to be parsed. This is passed for error messaging. /// /// @return returns a ParserPtr to the parser instance. /// @throw throws DCfgMgrBaseError if an error occurs. virtual isc::dhcp::ParserPtr - createConfigParser(const std::string& element_id); + createConfigParser(const std::string& element_id, + const isc::data::Element::Position& pos = + isc::data::Element::Position()); /// @brief Creates an new, blank D2CfgContext context /// diff --git a/src/bin/d2/d2_messages.mes b/src/bin/d2/d2_messages.mes index 6ac82ad2c1..d66373e74d 100644 --- a/src/bin/d2/d2_messages.mes +++ b/src/bin/d2/d2_messages.mes @@ -32,7 +32,7 @@ new configuration. It is output during server startup, and when an updated configuration is committed by the administrator. Additional information may be provided. -% DCTL_CONFIG_FILE_LOAD_FAIL %1 configuration could not be loaded from file: %2 +% DCTL_CONFIG_FILE_LOAD_FAIL %1 : %2 This fatal error message indicates that the application attempted to load its initial configuration from file and has failed. The service will exit. @@ -72,22 +72,11 @@ application and will exit. A warning message is issued when an attempt is made to shut down the application when it is not running. -% DCTL_ORDER_ERROR configuration contains more elements than the parsing order -An error message which indicates that configuration being parsed includes -element ids not specified the configuration manager's parse order list. This -is a programmatic error. - -% DCTL_ORDER_NO_ELEMENT element: %1 is in the parsing order but is missing from the configuration -An error message output during a configuration update. The program is -expecting an item but has not found it in the new configuration. This may -mean that the BIND 10 configuration database is corrupt. - -% DCTL_PARSER_FAIL configuration parsing failed for configuration element: %1, reason: %2 -On receipt of message containing details to a change of its configuration, -the server failed to create a parser to decode the contents of the named -configuration element, or the creation succeeded but the parsing actions -and committal of changes failed. The reason for the failure is given in -the message. +% DCTL_PARSER_FAIL %1 +On receipt of a new configuration, the server failed to create a parser to +decode the contents of the named configuration element, or the creation +succeeded but the parsing actions and committal of changes failed. +The reason for the failure is given in the message. % DCTL_PROCESS_FAILED %1 application execution failed: %2 The controller has encountered a fatal error while running the diff --git a/src/bin/d2/d_cfg_mgr.cc b/src/bin/d2/d_cfg_mgr.cc index 151202f360..7743b4291e 100644 --- a/src/bin/d2/d_cfg_mgr.cc +++ b/src/bin/d2/d_cfg_mgr.cc @@ -189,17 +189,15 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { // thrown. Note, that elements tagged as "optional" from the user // perspective must still have default or empty entries in the // configuration set to be parsed. - int parsed_count = 0; std::map::const_iterator it; BOOST_FOREACH(element_id, parse_order_) { it = objects_map.find(element_id); if (it != objects_map.end()) { - ++parsed_count; buildAndCommit(element_id, it->second); + // We parsed it, take it out of the list. + objects_map.erase(it); } else { - LOG_ERROR(dctl_logger, DCTL_ORDER_NO_ELEMENT) - .arg(element_id); isc_throw(DCfgMgrBaseError, "Element:" << element_id << " is listed in the parse order but is not " " present in the configuration"); @@ -208,18 +206,25 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { // NOTE: When using ordered parsing, the parse order list MUST // include every possible element id that the value_map may contain. - // Entries in the map that are not in the parse order, would not be + // Entries in the map that are not in the parse order, will not be // parsed. For now we will flag this as a programmatic error. One // could attempt to adjust for this, by identifying such entries // and parsing them either first or last but which would be correct? - // Better to hold the engineer accountable. So, if we parsed none - // 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) < objects_map.size()))) { - LOG_ERROR(dctl_logger, DCTL_ORDER_ERROR); + // Better to hold the engineer accountable. So, if there are any + // left in the objects_map then they were not in the parse order. + if (objects_map.size() > 0) { + std::ostringstream stream; + bool add_comma = false; + ConfigPair config_pair; + BOOST_FOREACH(config_pair, objects_map) { + stream << ( add_comma ? ", " : "") << config_pair.first + << " at: " << config_pair.second->getPosition(); + add_comma = true; + } + isc_throw(DCfgMgrBaseError, - "Configuration contains elements not in parse order"); + "Configuration contains elements not in parse order: " + << stream.str()); } } else { // Order doesn't matter so iterate over the value map directly. @@ -235,10 +240,9 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) { LOG_INFO(dctl_logger, DCTL_CONFIG_COMPLETE).arg(""); answer = isc::config::createAnswer(0, "Configuration committed."); - } catch (const isc::Exception& ex) { - LOG_ERROR(dctl_logger, DCTL_PARSER_FAIL).arg(element_id).arg(ex.what()); - answer = isc::config::createAnswer(1, - string("Configuration parsing failed: ") + ex.what()); + } catch (const std::exception& ex) { + LOG_ERROR(dctl_logger, DCTL_PARSER_FAIL).arg(ex.what()); + answer = isc::config::createAnswer(1, + ex.what()); // An error occurred, so make sure that we restore original context. context_ = original_context; @@ -253,7 +257,8 @@ 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)); + dhcp::ParserPtr parser(createConfigParser(param.first, + param.second->getPosition())); parser->build(param.second); parser->commit(); } @@ -263,28 +268,21 @@ void DCfgMgrBase::buildAndCommit(std::string& element_id, isc::data::ConstElementPtr value) { // Call derivation's implementation to create the appropriate parser // based on the element id. - ParserPtr parser = createConfigParser(element_id); + ParserPtr parser = createConfigParser(element_id, value->getPosition()); if (!parser) { isc_throw(DCfgMgrBaseError, "Could not create parser"); } - try { - // Invoke the parser's build method passing in the value. This will - // "convert" the Element form of value into the actual data item(s) - // and store them in parser's local storage. - parser->build(value); - - // Invoke the parser's commit method. This "writes" the the data - // item(s) stored locally by the parser into the context. (Note that - // parsers are free to do more than update the context, but that is an - // nothing something we are concerned with here.) - parser->commit(); - } catch (const isc::Exception& ex) { - isc_throw(DCfgMgrBaseError, - "Could not build and commit: " << ex.what()); - } catch (...) { - isc_throw(DCfgMgrBaseError, "Non-ISC exception occurred"); - } + // Invoke the parser's build method passing in the value. This will + // "convert" the Element form of value into the actual data item(s) + // and store them in parser's local storage. + parser->build(value); + + // Invoke the parser's commit method. This "writes" the the data + // item(s) stored locally by the parser into the context. (Note that + // parsers are free to do more than update the context, but that is an + // nothing something we are concerned with here.) + parser->commit(); } }; // end of isc::dhcp namespace diff --git a/src/bin/d2/d_cfg_mgr.h b/src/bin/d2/d_cfg_mgr.h index d088feb8db..eee07e89a4 100644 --- a/src/bin/d2/d_cfg_mgr.h +++ b/src/bin/d2/d_cfg_mgr.h @@ -316,11 +316,15 @@ protected: /// /// @param element_id is the string name of the element as it will appear /// in the configuration set. + /// @param pos position within the configuration text (or file) of element + /// to be parsed. This is passed for error messaging. /// /// @return returns a ParserPtr to the parser instance. /// @throw throws DCfgMgrBaseError if an error occurs. virtual isc::dhcp::ParserPtr - createConfigParser(const std::string& element_id) = 0; + createConfigParser(const std::string& element_id, + const isc::data::Element::Position& pos + = isc::data::Element::Position()) = 0; /// @brief Abstract factory which creates a context instance. /// diff --git a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc index 19da56d5e9..1aaceef65e 100644 --- a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc @@ -83,6 +83,12 @@ public: return (config.str()); } + /// @brief Enumeration to select between expected configuration outcomes + enum RunConfigMode { + SHOULD_PASS, + SHOULD_FAIL + }; + /// @brief Parses a configuration string and tests against a given outcome /// /// Convenience method which accepts JSON text and an expected pass or fail @@ -93,15 +99,16 @@ public: /// 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) { + /// @param mode indicator if the parsing should fail or not. It defaults + /// defaults to SHOULD_PASS. + /// + void runConfig(std::string config_str, RunConfigMode mode=SHOULD_PASS) { // 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)); + ASSERT_TRUE(checkAnswer(mode == SHOULD_FAIL)); // Verify that the D2 context can be retrieved and is not null. D2CfgContextPtr context; @@ -397,6 +404,44 @@ TEST_F(D2CfgMgrTest, defaultValues) { d2_params_->getNcrFormat()); } +/// @brief Tests the unsupported scalar parameters and objects are detected. +TEST_F(D2CfgMgrTest, unsupportedTopLevelItems) { + // Check that an unsupported top level parameter fails. + std::string config = + "{" + " \"ip_address\": \"127.0.0.1\", " + " \"port\": 777 , " + " \"dns_server_timeout\": 333 , " + " \"ncr_protocol\": \"UDP\" , " + " \"ncr_format\": \"JSON\", " + "\"tsig_keys\": [], " + "\"forward_ddns\" : {}, " + "\"reverse_ddns\" : {}, " + "\"bogus_param\" : true " + "}"; + + runConfig(config, SHOULD_FAIL); + + // Check that unsupported top level objects fails. For + // D2 these fail as they are not in the parse order. + config = + "{" + " \"ip_address\": \"127.0.0.1\", " + " \"port\": 777 , " + " \"dns_server_timeout\": 333 , " + " \"ncr_protocol\": \"UDP\" , " + " \"ncr_format\": \"JSON\", " + "\"tsig_keys\": [], " + "\"bogus_object_one\" : {}, " + "\"forward_ddns\" : {}, " + "\"reverse_ddns\" : {}, " + "\"bogus_object_two\" : {} " + "}"; + + runConfig(config, SHOULD_FAIL); +} + + /// @brief Tests the enforcement of data validation when parsing D2Params. /// It verifies that: /// -# ip_address cannot be "0.0.0.0" @@ -409,27 +454,27 @@ TEST_F(D2CfgMgrTest, invalidEntry) { // Cannot use IPv4 ANY address std::string config = makeParamsConfigString ("0.0.0.0", 777, 333, "UDP", "JSON"); - runConfig(config, 1); + runConfig(config, SHOULD_FAIL); // Cannot use IPv6 ANY address config = makeParamsConfigString ("::", 777, 333, "UDP", "JSON"); - runConfig(config, 1); + runConfig(config, SHOULD_FAIL); // Cannot use port 0 config = makeParamsConfigString ("127.0.0.1", 0, 333, "UDP", "JSON"); - runConfig(config, 1); + runConfig(config, SHOULD_FAIL); // Cannot use dns server timeout of 0 config = makeParamsConfigString ("127.0.0.1", 777, 0, "UDP", "JSON"); - runConfig(config, 1); + runConfig(config, SHOULD_FAIL); // Invalid protocol config = makeParamsConfigString ("127.0.0.1", 777, 333, "BOGUS", "JSON"); - runConfig(config, 1); + runConfig(config, SHOULD_FAIL); // Invalid format config = makeParamsConfigString ("127.0.0.1", 777, 333, "UDP", "BOGUS"); - runConfig(config, 1); + runConfig(config, SHOULD_FAIL); } /// @brief Tests the enforcement of data validation when parsing TSIGKeyInfos. diff --git a/src/bin/d2/tests/d_cfg_mgr_unittests.cc b/src/bin/d2/tests/d_cfg_mgr_unittests.cc index 88438d215d..d67d15e8d4 100644 --- a/src/bin/d2/tests/d_cfg_mgr_unittests.cc +++ b/src/bin/d2/tests/d_cfg_mgr_unittests.cc @@ -53,7 +53,8 @@ public: /// @brief Dummy implementation as this method is abstract. virtual isc::dhcp::ParserPtr - createConfigParser(const std::string& /* element_id */) { + createConfigParser(const std::string& /* element_id */, + const isc::data::Element::Position& /* pos */) { return (isc::dhcp::ParserPtr()); } }; @@ -125,18 +126,23 @@ TEST_F(DStubCfgMgrTest, basicParseTest) { answer_ = cfg_mgr_->parseConfig(config_set_); EXPECT_TRUE(checkAnswer(0)); + +std::cout << __FILE__ << ":" << __LINE__ << std::endl; // Verify that an error building the element is caught and returns a // failed parse result. SimFailure::set(SimFailure::ftElementBuild); answer_ = cfg_mgr_->parseConfig(config_set_); EXPECT_TRUE(checkAnswer(1)); +std::cout << __FILE__ << ":" << __LINE__ << std::endl; // Verify that an error committing the element is caught and returns a // failed parse result. SimFailure::set(SimFailure::ftElementCommit); answer_ = cfg_mgr_->parseConfig(config_set_); EXPECT_TRUE(checkAnswer(1)); +std::cout << __FILE__ << ":" << __LINE__ << std::endl; + // Verify that an unknown element error is caught and returns a failed // parse result. SimFailure::set(SimFailure::ftElementUnknown); diff --git a/src/bin/d2/tests/d_test_stubs.cc b/src/bin/d2/tests/d_test_stubs.cc index 6cc15b3a3d..17df507c26 100644 --- a/src/bin/d2/tests/d_test_stubs.cc +++ b/src/bin/d2/tests/d_test_stubs.cc @@ -396,7 +396,8 @@ DStubCfgMgr::createNewContext() { } isc::dhcp::ParserPtr -DStubCfgMgr::createConfigParser(const std::string& element_id) { +DStubCfgMgr::createConfigParser(const std::string& element_id, + const isc::data::Element::Position& pos) { isc::dhcp::ParserPtr parser; DStubContextPtr context = boost::dynamic_pointer_cast(getContext()); @@ -416,7 +417,8 @@ DStubCfgMgr::createConfigParser(const std::string& element_id) { // to "succeed" without specifically supporting them. if (SimFailure::shouldFailOn(SimFailure::ftElementUnknown)) { isc_throw(DCfgMgrBaseError, - "Configuration parameter not supported: " << element_id); + "Configuration parameter not supported: " << element_id + << pos); } // Going to assume anything else is an object element. diff --git a/src/bin/d2/tests/d_test_stubs.h b/src/bin/d2/tests/d_test_stubs.h index 7fcf8b5f46..30229fb20d 100644 --- a/src/bin/d2/tests/d_test_stubs.h +++ b/src/bin/d2/tests/d_test_stubs.h @@ -691,11 +691,15 @@ public: /// /// @param element_id is the string name of the element as it will appear /// in the configuration set. + /// @param pos position within the configuration text (or file) of element + /// to be parsed. This is passed for error messaging. /// /// @return returns a ParserPtr to the parser instance. /// @throw throws DCfgMgrBaseError if SimFailure is ftElementUnknown. virtual isc::dhcp::ParserPtr - createConfigParser(const std::string& element_id); + createConfigParser(const std::string& element_id, + const isc::data::Element::Position& pos + = isc::data::Element::Position()); /// @brief A list for remembering the element ids in the order they were /// parsed.