From: Tomek Mrugalski Date: Fri, 28 Sep 2018 18:22:17 +0000 (+0200) Subject: [#6,!54] More code removed from lib/process X-Git-Tag: 128-netconf-config_base~9 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7a03509652fcfcd3058d51afd9a910131f04045c;p=thirdparty%2Fkea.git [#6,!54] More code removed from lib/process --- diff --git a/src/bin/d2/d2_cfg_mgr.cc b/src/bin/d2/d2_cfg_mgr.cc index c3e87f037f..fd16344185 100644 --- a/src/bin/d2/d2_cfg_mgr.cc +++ b/src/bin/d2/d2_cfg_mgr.cc @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -106,11 +107,6 @@ const char* D2CfgMgr::IPV4_REV_ZONE_SUFFIX = "in-addr.arpa."; 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("tsig-keys"); - addToParseOrder("forward-ddns"); - addToParseOrder("reverse-ddns"); } D2CfgMgr::~D2CfgMgr() { @@ -246,177 +242,58 @@ D2CfgMgr::getConfigSummary(const uint32_t) { return (getD2Params()->getConfigSummary()); } -namespace { - -template int_type -getInt(const std::string& name, ConstElementPtr value) { - int64_t val_int = value->intValue(); - if ((val_int < std::numeric_limits::min()) || - (val_int > std::numeric_limits::max())) { - isc_throw(D2CfgError, "out of range value (" << val_int - << ") specified for parameter '" << name - << "' (" << value->getPosition() << ")"); - } - return (static_cast(val_int)); +void +D2CfgMgr::setCfgDefaults(ElementPtr mutable_config) { + D2SimpleParser::setAllDefaults(mutable_config); } -isc::asiolink::IOAddress -getIOAddress(const std::string& name, ConstElementPtr value) { - std::string str = value->stringValue(); - try { - return (isc::asiolink::IOAddress(str)); - } catch (const std::exception& ex) { - isc_throw(D2CfgError, "invalid address (" << str - << ") specified for parameter '" << name - << "' (" << value->getPosition() << ")"); +isc::data::ConstElementPtr +D2CfgMgr::parse(isc::data::ConstElementPtr config_set, bool check_only) { + // Do a sanity check first. + if (!config_set) { + isc_throw(D2CfgError, "Mandatory config parameter not provided"); } -} -dhcp_ddns::NameChangeProtocol -getProtocol(const std::string& name, ConstElementPtr value) { - std::string str = value->stringValue(); - try { - return (dhcp_ddns::stringToNcrProtocol(str)); - } catch (const std::exception& ex) { - isc_throw(D2CfgError, - "invalid NameChangeRequest protocol (" << str - << ") specified for parameter '" << name - << "' (" << value->getPosition() << ")"); - } -} + D2CfgContextPtr ctx = getD2CfgContext(); -dhcp_ddns::NameChangeFormat -getFormat(const std::string& name, ConstElementPtr value) { - std::string str = value->stringValue(); + // Set the defaults + ElementPtr cfg = boost::const_pointer_cast(config_set); + D2SimpleParser::setAllDefaults(cfg); + + // And parse the configuration. + ConstElementPtr answer; + std::string excuse; try { - return (dhcp_ddns::stringToNcrFormat(str)); - } catch (const std::exception& ex) { - isc_throw(D2CfgError, - "invalid NameChangeRequest format (" << str - << ") specified for parameter '" << name - << "' (" << value->getPosition() << ")"); + // Do the actual parsing + D2SimpleParser parser; + parser.parse(ctx, cfg, check_only); + } catch (const isc::Exception& ex) { + excuse = ex.what(); + answer = isc::config::createAnswer(2, excuse); + } catch (...) { + excuse = "undefined configuration parsing error"; + answer = isc::config::createAnswer(2, excuse); } -} -} // anon - -void -D2CfgMgr::parseElement(const std::string& element_id, - ConstElementPtr element) { - try { - // Get D2 specific context. - D2CfgContextPtr context = getD2CfgContext(); - - if ((element_id == "ip-address") || - (element_id == "ncr-protocol") || - (element_id == "ncr-format") || - (element_id == "port") || - (element_id == "dns-server-timeout")) { - // global scalar params require nothing extra be done - } else if (element_id == "user-context") { - if (element->getType() == Element::map) { - context->setContext(element); - } - } else if (element_id == "tsig-keys") { - TSIGKeyInfoListParser parser; - context->setKeys(parser.parse(element)); - } else if (element_id == "forward-ddns") { - DdnsDomainListMgrParser parser; - DdnsDomainListMgrPtr mgr = parser.parse(element, element_id, - context->getKeys()); - context->setForwardMgr(mgr); - } else if (element_id == "reverse-ddns") { - DdnsDomainListMgrParser parser; - DdnsDomainListMgrPtr mgr = parser.parse(element, element_id, - context->getKeys()); - context->setReverseMgr(mgr); + // At this stage the answer was created only in case of exception. + if (answer) { + if (check_only) { + LOG_ERROR(d2_logger, DHCP_DDNS_CONFIG_CHECK_FAIL).arg(excuse); } else { - // Shouldn't occur if the JSON parser is doing its job. - isc_throw(D2CfgError, "Unsupported element: " - << element_id << element->getPosition()); + LOG_ERROR(d2_logger, DHCP_DDNS_CONFIG_FAIL).arg(excuse); } - } catch (const D2CfgError& ex) { - // Should already have a specific error and position info - throw ex; - } catch (const std::exception& ex) { - isc_throw(D2CfgError, "element: " << element_id << " : " << ex.what() - << element->getPosition()); + return (answer); } -}; - -void -D2CfgMgr::setCfgDefaults(ElementPtr mutable_config) { - D2SimpleParser::setAllDefaults(mutable_config); -} - -void -D2CfgMgr::buildParams(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 in the config, performing any logical - // validation required. - asiolink::IOAddress ip_address(0); - uint32_t port = 0; - uint32_t dns_server_timeout = 0; - dhcp_ddns::NameChangeProtocol ncr_protocol = dhcp_ddns::NCR_UDP; - dhcp_ddns::NameChangeFormat ncr_format = dhcp_ddns::FMT_JSON; - - // Assumes that params_config has had defaults added - BOOST_FOREACH(isc::dhcp::ConfigPair param, params_config->mapValue()) { - std::string entry(param.first); - ConstElementPtr value(param.second); - try { - if (entry == "ip-address") { - ip_address = getIOAddress(entry, value); - if ((ip_address.toText() == "0.0.0.0") || - (ip_address.toText() == "::")) { - isc_throw(D2CfgError, "IP address cannot be \"" - << ip_address << "\"" - << " (" << value->getPosition() << ")"); - } - } else if (entry == "port") { - port = getInt(entry, value); - } else if (entry == "dns-server-timeout") { - dns_server_timeout = getInt(entry, value); - } else if (entry == "ncr-protocol") { - ncr_protocol = getProtocol(entry, value); - if (ncr_protocol != dhcp_ddns::NCR_UDP) { - isc_throw(D2CfgError, "ncr-protocol : " - << dhcp_ddns::ncrProtocolToString(ncr_protocol) - << " is not yet supported " - << " (" << value->getPosition() << ")"); - } - } else if (entry == "ncr-format") { - ncr_format = getFormat(entry, value); - if (ncr_format != dhcp_ddns::FMT_JSON) { - isc_throw(D2CfgError, "NCR Format:" - << dhcp_ddns::ncrFormatToString(ncr_format) - << " is not yet supported" - << " (" << value->getPosition() << ")"); - } - } else { - isc_throw(D2CfgError, - "unsupported parameter '" << entry - << " (" << value->getPosition() << ")"); - } - } catch (const isc::data::TypeError&) { - isc_throw(D2CfgError, - "invalid value type specified for parameter '" << entry - << " (" << value->getPosition() << ")"); - } + if (check_only) { + answer = isc::config::createAnswer(0, "Configuration check successful"); + } else { + answer = isc::config::createAnswer(0, "Configuration applied successfully."); } - // Attempt to create the new client config. This ought to fly as - // we already validated everything. - D2ParamsPtr params(new D2Params(ip_address, port, dns_server_timeout, - ncr_protocol, ncr_format)); - - getD2CfgContext()->getD2Params() = params; + return (answer); } + }; // end of isc::dhcp namespace }; // end of isc namespace diff --git a/src/bin/d2/d2_cfg_mgr.h b/src/bin/d2/d2_cfg_mgr.h index 564a551ae8..1f67eec57b 100644 --- a/src/bin/d2/d2_cfg_mgr.h +++ b/src/bin/d2/d2_cfg_mgr.h @@ -264,19 +264,14 @@ public: protected: - /// @brief Parses an element using alternate parsers + /// @brief Parses configuration of the D2. /// - /// Each element to be parsed is passed first into this method to allow - /// it to be processed by SimpleParser derivations if they've been - /// implemented. The method should return true if it has processed the - /// element or false if the element should be passed onto the original - /// DhcpConfigParser mechanisms. This method is invoked in both - /// @c DCfgMgrBase::buildParams() and DCfgMgrBase::buildAndCommit(). - /// - /// @param element_id name of the element as it is expected in the cfg - /// @param element value of the element as ElementPtr - virtual void parseElement(const std::string& element_id, - isc::data::ConstElementPtr element); + /// @param config Pointer to a configuration specified for D2. + /// @param check_only Boolean flag indicating if this method should + /// only verify correctness of the provided configuration. + /// @return Pointer to a result of configuration parsing. + virtual isc::data::ConstElementPtr + parse(isc::data::ConstElementPtr config, bool check_only); /// @brief Adds default values to the given config /// @@ -286,26 +281,6 @@ protected: /// @param mutable_config - configuration to which defaults should be added virtual void setCfgDefaults(isc::data::ElementPtr mutable_config); - /// @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 - /// parameter's configuration value. - /// - /// It then fetches the parameters, validating their values and if - /// valid instantiates a D2Params instance. Invalid values result in - /// a throw. - /// - /// @param params_config set of scalar configuration elements to parse - /// - /// @throw D2CfgError if any of the following are true: - /// -# 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 - virtual void buildParams(isc::data::ConstElementPtr params_config); - /// @brief Creates an new, blank D2CfgContext context /// /// This method is used at the beginning of configuration process to diff --git a/src/bin/d2/d2_messages.mes b/src/bin/d2/d2_messages.mes index 55406ba873..7412ee8027 100644 --- a/src/bin/d2/d2_messages.mes +++ b/src/bin/d2/d2_messages.mes @@ -41,6 +41,16 @@ has been invoked. This is a debug message issued when the DHCP-DDNS application configure method has been invoked. +% DHCP_DDNS_CONFIG_CHECK_FAIL Control Agent configuration check failed: %1 +This error message indicates that the DHCP-DDNS had failed configuration +check. Details are provided. Additional details may be available +in earlier log entries, possibly on lower levels. + +% DHCP_DDNS_CONFIG_FAIL Control Agent configuration failed: %1 +This error message indicates that the DHCP-DDNS had failed configuration +attempt. Details are provided. Additional details may be available +in earlier log entries, possibly on lower levels. + % DHCP_DDNS_FAILED application experienced a fatal error: %1 This is a debug message issued when the DHCP-DDNS application encounters an unrecoverable error from within the event loop. diff --git a/src/bin/d2/d2_process.cc b/src/bin/d2/d2_process.cc index 6981d2d591..3ff9a669e4 100644 --- a/src/bin/d2/d2_process.cc +++ b/src/bin/d2/d2_process.cc @@ -198,7 +198,7 @@ D2Process::configure(isc::data::ConstElementPtr config_set, bool check_only) { .arg(config_set->str()); isc::data::ConstElementPtr answer; - answer = getCfgMgr()->parseConfig(config_set, check_only);; + answer = getCfgMgr()->simpleParseConfig(config_set, check_only); if (check_only) { return (answer); } diff --git a/src/bin/d2/d2_simple_parser.cc b/src/bin/d2/d2_simple_parser.cc index 708cb7a24f..89654d8593 100644 --- a/src/bin/d2/d2_simple_parser.cc +++ b/src/bin/d2/d2_simple_parser.cc @@ -10,6 +10,48 @@ #include using namespace isc::data; +using namespace isc::d2; +using namespace isc; + +namespace { + +dhcp_ddns::NameChangeProtocol +getProtocol(ConstElementPtr map, const std::string& name) { + ConstElementPtr value = map->get(name); + if (!value) { + isc_throw(D2CfgError, "Mandatory parameter " << name + << " not found (" << map->getPosition() << ")"); + } + std::string str = value->stringValue(); + try { + return (dhcp_ddns::stringToNcrProtocol(str)); + } catch (const std::exception& ex) { + isc_throw(D2CfgError, + "invalid NameChangeRequest protocol (" << str + << ") specified for parameter '" << name + << "' (" << value->getPosition() << ")"); + } +} + +dhcp_ddns::NameChangeFormat +getFormat(ConstElementPtr map, const std::string& name) { + ConstElementPtr value = map->get(name); + if (!value) { + isc_throw(D2CfgError, "Mandatory parameter " << name + << " not found (" << map->getPosition() << ")"); + } + std::string str = value->stringValue(); + try { + return (dhcp_ddns::stringToNcrFormat(str)); + } catch (const std::exception& ex) { + isc_throw(D2CfgError, + "invalid NameChangeRequest format (" << str + << ") specified for parameter '" << name + << "' (" << value->getPosition() << ")"); + } +} + +} // anon namespace isc { namespace d2 { @@ -150,5 +192,87 @@ D2SimpleParser::setManagerDefaults(ElementPtr global, return (cnt); } +void D2SimpleParser::parse(const D2CfgContextPtr& ctx, + const isc::data::ConstElementPtr& config, + bool /*check_only*/) { + // TSIG keys need to parse before the Domains, so we can catch Domains + // that specify undefined keys. Create the necessary parsing order now. + // addToParseOrder("tsig-keys"); + // addToParseOrder("forward-ddns"); + // addToParseOrder("reverse-ddns"); + + ConstElementPtr keys = config->get("tsig-keys"); + if (keys) { + TSIGKeyInfoListParser parser; + ctx->setKeys(parser.parse(keys)); + } + + ConstElementPtr fwd = config->get("forward-ddns"); + if (fwd) { + DdnsDomainListMgrParser parser; + DdnsDomainListMgrPtr mgr = parser.parse(fwd, "forward-ddns", + ctx->getKeys()); + ctx->setForwardMgr(mgr); + } + + ConstElementPtr rev = config->get("reverse-ddns"); + if (rev) { + DdnsDomainListMgrParser parser; + DdnsDomainListMgrPtr mgr = parser.parse(rev, "reverse-ddns", + ctx->getKeys()); + ctx->setReverseMgr(mgr); + } + + // Fetch the parameters in the config, performing any logical + // validation required. + asiolink::IOAddress ip_address(0); + uint32_t port = 0; + uint32_t dns_server_timeout = 0; + dhcp_ddns::NameChangeProtocol ncr_protocol = dhcp_ddns::NCR_UDP; + dhcp_ddns::NameChangeFormat ncr_format = dhcp_ddns::FMT_JSON; + + ip_address = SimpleParser::getAddress(config, "ip-address"); + + if ((ip_address.toText() == "0.0.0.0") || + (ip_address.toText() == "::")) { + isc_throw(D2CfgError, "IP address cannot be \"" + << ip_address << "\"" + << " (" << config->get("ip-address")->getPosition() << ")"); + } + + port = SimpleParser::getUint32(config, "port"); + + dns_server_timeout = SimpleParser::getUint32(config, "dns-server-timeout"); + + ncr_protocol = getProtocol(config, "ncr-protocol"); + if (ncr_protocol != dhcp_ddns::NCR_UDP) { + isc_throw(D2CfgError, "ncr-protocol : " + << dhcp_ddns::ncrProtocolToString(ncr_protocol) + << " is not yet supported " + << " (" << config->get("ncr-protocol")->getPosition() << ")"); + } + + ncr_format = getFormat(config, "ncr-format"); + if (ncr_format != dhcp_ddns::FMT_JSON) { + isc_throw(D2CfgError, "NCR Format:" + << dhcp_ddns::ncrFormatToString(ncr_format) + << " is not yet supported" + << " (" << config->get("ncr-format")->getPosition() << ")"); + } + + ConstElementPtr user = config->get("user-context"); + if (user) { + ctx->setContext(user); + } + + // Attempt to create the new client config. This ought to fly as + // we already validated everything. + D2ParamsPtr params(new D2Params(ip_address, port, dns_server_timeout, + ncr_protocol, ncr_format)); + + ctx->getD2Params() = params; +} + + }; }; diff --git a/src/bin/d2/d2_simple_parser.h b/src/bin/d2/d2_simple_parser.h index 036275a111..9191da201b 100644 --- a/src/bin/d2/d2_simple_parser.h +++ b/src/bin/d2/d2_simple_parser.h @@ -8,6 +8,7 @@ #define D2_SIMPLE_PARSER_H #include +#include namespace isc { namespace d2 { @@ -76,6 +77,17 @@ public: static size_t setManagerDefaults(data::ElementPtr global, const std::string& mgr_name, const data::SimpleDefaults& mgr_defaults); + + /// @brief Parses the whole D2 configuration + /// + /// @param ctx - parsed information will be stored here + /// @param config - Element tree structure that holds configuration + /// @param check_only - if true the configuration is verified only, not applied + /// + /// @throw ConfigError if any issues are encountered. + void parse(const D2CfgContextPtr& ctx, + const isc::data::ConstElementPtr& config, + bool check_only); }; }; diff --git a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc index eaf9ba37af..2579b2ef46 100644 --- a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc @@ -158,7 +158,7 @@ public: // The JSON parsed ok and we've added the defaults, pass the config // into the Element parser and check for the expected outcome. data::ConstElementPtr answer; - answer = cfg_mgr_->parseConfig(config_set_, false); + answer = cfg_mgr_->simpleParseConfig(config_set_, false); // Extract the result and error text from the answer. int rcode = 0; @@ -585,7 +585,7 @@ TEST_F(D2CfgMgrTest, fullConfig) { // Verify that parsing the exact same configuration a second time // does not cause a duplicate value errors. - answer_ = cfg_mgr_->parseConfig(config_set_, false); + answer_ = cfg_mgr_->simpleParseConfig(config_set_, false); ASSERT_TRUE(checkAnswer(0)); } diff --git a/src/bin/d2/tests/d2_controller_unittests.cc b/src/bin/d2/tests/d2_controller_unittests.cc index 1f09d330a0..fa47157fa3 100644 --- a/src/bin/d2/tests/d2_controller_unittests.cc +++ b/src/bin/d2/tests/d2_controller_unittests.cc @@ -183,16 +183,16 @@ TEST_F(D2ControllerTest, configUpdateTests) { EXPECT_EQ(0, rcode); // Use an invalid configuration to verify parsing error return. - std::string config = "{ \"bogus\": 1000 } "; + std::string config = "{ \"ip-address\": 1000 } "; config_set = isc::data::Element::fromJSON(config); answer = updateConfig(config_set); isc::config::parseAnswer(rcode, answer); - EXPECT_EQ(1, rcode); + EXPECT_EQ(2, rcode); // Use an invalid configuration to verify checking error return. answer = checkConfig(config_set); isc::config::parseAnswer(rcode, answer); - EXPECT_EQ(1, rcode); + EXPECT_EQ(2, rcode); } // Tests that the original configuration is retained after a SIGHUP triggered diff --git a/src/bin/d2/tests/d2_process_unittests.cc b/src/bin/d2/tests/d2_process_unittests.cc index 900a893c61..35be2b0d97 100644 --- a/src/bin/d2/tests/d2_process_unittests.cc +++ b/src/bin/d2/tests/d2_process_unittests.cc @@ -202,9 +202,18 @@ TEST_F(D2ProcessTest, configure) { // Invoke configure() with the invalid configuration. answer = configure(config_set_, false); - // Verify that configure result is failure, the reconfigure flag is - // false, and that the queue manager is still running. - ASSERT_TRUE(checkAnswer(answer, 1)); + // Verify that configure result is a success, as extra parameters are + // ignored. the reconfigure flag is false, and that the queue manager is + // still running. + ASSERT_TRUE(checkAnswer(answer, 0)); + EXPECT_TRUE(getReconfQueueFlag()); + EXPECT_EQ(D2QueueMgr::RUNNING, queue_mgr->getMgrState()); + + // Finally, try with an invalid configuration. + // Create an invalid configuration set from text config. + ASSERT_TRUE(fromJSON("{ \"ip-address\": \"950 Charter St.\" } ")); + answer = configure(config_set_, false); + ASSERT_TRUE(checkAnswer(answer, 2)); EXPECT_FALSE(getReconfQueueFlag()); EXPECT_EQ(D2QueueMgr::RUNNING, queue_mgr->getMgrState()); } diff --git a/src/bin/d2/tests/d2_update_mgr_unittests.cc b/src/bin/d2/tests/d2_update_mgr_unittests.cc index 021f234f51..6bc05bac31 100644 --- a/src/bin/d2/tests/d2_update_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_update_mgr_unittests.cc @@ -144,7 +144,7 @@ public: // If this configuration fails to parse most tests will fail. ASSERT_TRUE(fromJSON(canned_config_)); - answer_ = cfg_mgr_->parseConfig(config_set_); + answer_ = cfg_mgr_->simpleParseConfig(config_set_); ASSERT_TRUE(checkAnswer(0)); } diff --git a/src/bin/d2/tests/get_config_unittest.cc b/src/bin/d2/tests/get_config_unittest.cc index 06643f54f4..9133081579 100644 --- a/src/bin/d2/tests/get_config_unittest.cc +++ b/src/bin/d2/tests/get_config_unittest.cc @@ -154,7 +154,7 @@ public: // try DHCPDDNS configure ConstElementPtr status; try { - status = srv_->parseConfig(d2, false); + status = srv_->simpleParseConfig(d2, false); } catch (const std::exception& ex) { ADD_FAILURE() << "configure for " << operation << " failed with " << ex.what() diff --git a/src/bin/d2/tests/testdata/d2_cfg_tests.json b/src/bin/d2/tests/testdata/d2_cfg_tests.json index 162fd0ffd9..c90a788eb9 100644 --- a/src/bin/d2/tests/testdata/d2_cfg_tests.json +++ b/src/bin/d2/tests/testdata/d2_cfg_tests.json @@ -137,7 +137,7 @@ #----- ,{ "description" : "D2Params.ip-address invalid value", -"logic-error" : "invalid address (bogus) specified for parameter 'ip-address' (:1:39)", +"logic-error" : "Failed to convert 'bogus' to address: Failed to convert string to address 'bogus': Invalid argument(:1:39)", "data" : { "ip-address" : "bogus", @@ -315,7 +315,7 @@ #----- ,{ "description" : "D2.tsig-keys, missing key name", -"logic-error" : "element: tsig-keys : missing parameter 'name' (:1:62):1:47", +"logic-error" : "missing parameter 'name' (:1:62)", "data" : { "forward-ddns" : {}, @@ -420,7 +420,7 @@ #----- D2.tsig-keys, algorithm tests ,{ "description" : "D2.tsig-keys, missing algorithm", -"logic-error" : "element: tsig-keys : missing parameter 'algorithm' (:1:62):1:47", +"logic-error" : "missing parameter 'algorithm' (:1:62)", "data" : { "forward-ddns" : {}, @@ -665,7 +665,7 @@ #----- D2.tsig-keys, secret tests ,{ "description" : "D2.tsig-keys, missing secret", -"logic-error" : "element: tsig-keys : missing parameter 'secret' (:1:62):1:47", +"logic-error" : "missing parameter 'secret' (:1:62)", "data" : { "forward-ddns" : {}, diff --git a/src/lib/process/d_cfg_mgr.cc b/src/lib/process/d_cfg_mgr.cc index aded3cbb20..b8fdb543c5 100644 --- a/src/lib/process/d_cfg_mgr.cc +++ b/src/lib/process/d_cfg_mgr.cc @@ -41,8 +41,7 @@ DCfgContextBase::~DCfgContextBase() { // *********************** DCfgMgrBase ************************* -DCfgMgrBase::DCfgMgrBase(DCfgContextBasePtr context) - : parse_order_() { +DCfgMgrBase::DCfgMgrBase(DCfgContextBasePtr context) { setContext(context); } @@ -64,167 +63,6 @@ DCfgMgrBase::setContext(DCfgContextBasePtr& context) { context_ = context; } -isc::data::ConstElementPtr -DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set, - bool check_only) { - LOG_DEBUG(dctl_logger, isc::log::DBGLVL_COMMAND, - DCTL_CONFIG_START).arg(config_set->str()); - - if (!config_set) { - return (isc::config::createAnswer(1, - std::string("Can't parse NULL config"))); - } - - // The parsers implement data inheritance by directly accessing - // configuration context. For this reason the data parsers must store - // the parsed data into context immediately. This may cause data - // 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_; - resetContext(); - - // Answer will hold the result returned to the caller. - ConstElementPtr answer; - - // Holds the name of the element being parsed. - std::string element_id; - - try { - - // Make the configuration mutable so we can then insert default values. - ElementPtr mutable_cfg = boost::const_pointer_cast(config_set); - setCfgDefaults(mutable_cfg); - - // 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, mutable_cfg->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. - - // 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 - // associated data in for parsing. - // If there is no matching entry in the value map an error is - // 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. - std::map::iterator it; - BOOST_FOREACH(element_id, parse_order_) { - it = objects_map.find(element_id); - if (it != objects_map.end()) { - buildAndCommit(element_id, it->second); - // We parsed it, take it out of the list. - objects_map.erase(it); - } - else { - isc_throw(DCfgMgrBaseError, - "Element required by parsing order is missing: " - << element_id << " (" - << mutable_cfg->getPosition() << ")"); - } - } - - // Handle user context here as it is really optional. - std::string user_context_id("user-context"); - it = objects_map.find(user_context_id); - if (it != objects_map.end()) { - buildAndCommit(user_context_id, it->second); - // We parsed it, take it out of the list. - objects_map.erase(it); - } - - // 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, 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 there are any - // left in the objects_map then they were not in the parse order. - if (!objects_map.empty()) { - std::ostringstream stream; - bool add_comma = false; - ConfigPair config_pair; - BOOST_FOREACH(config_pair, objects_map) { - stream << ( add_comma ? ", " : "") << config_pair.first - << " (" << config_pair.second->getPosition() << ")"; - add_comma = true; - } - - isc_throw(DCfgMgrBaseError, - "Configuration contains elements not in parse order: " - << stream.str()); - } - } else { - // 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, objects_map) { - element_id = config_pair.first; - buildAndCommit(element_id, config_pair.second); - } - } - - // Everything was fine. Configuration set processed successfully. - if (!check_only) { - LOG_INFO(dctl_logger, DCTL_CONFIG_COMPLETE).arg(getConfigSummary(0)); - answer = isc::config::createAnswer(0, "Configuration committed."); - } else { - answer = isc::config::createAnswer(0, "Configuration seems sane."); - LOG_INFO(dctl_logger, DCTL_CONFIG_CHECK_COMPLETE) - .arg(getConfigSummary(0)) - .arg(config::answerToText(answer)); - } - } 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; - return (answer); - } - - if (check_only) { - // If this is a configuration check only, then don't actually apply - // the configuration and reverse to the previous one. - context_ = original_context; - } - - return (answer); -} - isc::data::ConstElementPtr DCfgMgrBase::simpleParseConfig(isc::data::ConstElementPtr config_set, bool check_only, @@ -252,14 +90,22 @@ DCfgMgrBase::simpleParseConfig(isc::data::ConstElementPtr config_set, // Let's call the actual implementation answer = parse(config_set, check_only); + // and check the response returned. + int code = 0; + isc::config::parseAnswer(code, answer); + // Everything was fine. Configuration set processed successfully. if (!check_only) { if (post_config_cb) { post_config_cb(); } - LOG_INFO(dctl_logger, DCTL_CONFIG_COMPLETE).arg(getConfigSummary(0)); - answer = isc::config::createAnswer(0, "Configuration committed."); + if (code == 0) { + LOG_INFO(dctl_logger, DCTL_CONFIG_COMPLETE).arg(getConfigSummary(0)); + } + + // Use the answer provided. + //answer = isc::config::createAnswer(0, "Configuration committed."); } else { LOG_INFO(dctl_logger, DCTL_CONFIG_CHECK_COMPLETE) .arg(getConfigSummary(0)) @@ -289,26 +135,6 @@ void DCfgMgrBase::setCfgDefaults(isc::data::ElementPtr) { } -void -DCfgMgrBase::parseElement(const std::string&, isc::data::ConstElementPtr) { -}; - - -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 element parser to parse the element. - parseElement(param.first, param.second); - } -} - -void DCfgMgrBase::buildAndCommit(std::string& element_id, - isc::data::ConstElementPtr value) { - // Call derivation's element parser to parse the element. - parseElement(element_id, value); -} - isc::data::ConstElementPtr DCfgMgrBase::parse(isc::data::ConstElementPtr, bool) { isc_throw(DCfgMgrBaseError, "This class does not implement simple parser paradigm yet"); diff --git a/src/lib/process/d_cfg_mgr.h b/src/lib/process/d_cfg_mgr.h index 6e365e3c06..89decf0c08 100644 --- a/src/lib/process/d_cfg_mgr.h +++ b/src/lib/process/d_cfg_mgr.h @@ -116,9 +116,6 @@ private: DCfgContextBase& operator=(const DCfgContextBase& rhs); }; -/// @brief Defines a sequence of Element IDs used to specify a parsing order. -typedef std::vector ElementIdList; - /// @brief Configuration Manager /// /// DCfgMgrBase is an abstract class that provides the mechanisms for managing @@ -205,20 +202,6 @@ public: /// @brief Destructor virtual ~DCfgMgrBase(); - /// @brief Acts as the receiver of new configurations and coordinates - /// the parsing as described in the class brief. - /// - /// @param config_set is a set of configuration elements to be parsed. - /// @param check_only true if the config is to be checked only, but not applied - /// - /// @return an Element that contains the results of configuration composed - /// of an integer status value (0 means successful, non-zero means failure), - /// and a string explanation of the outcome. - isc::data::ConstElementPtr - parseConfig(isc::data::ConstElementPtr config_set, - bool check_only = false); - - /// @brief Acts as the receiver of new configurations. /// /// This method is similar to what @ref parseConfig does, execept it employs @@ -246,26 +229,6 @@ public: bool check_only = false, const std::function& post_config_cb = nullptr); - /// @brief Adds a given element id to the end of the parse order list. - /// - /// 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. - void addToParseOrder(const std::string& element_id){ - parse_order_.push_back(element_id); - } - - /// @brief Fetches the parse order list. - /// - /// @return returns a const reference to the list. - const ElementIdList& getParseOrder() const { - return (parse_order_); - } - /// @brief Fetches the configuration context. /// /// @return returns a pointer reference to the configuration context. @@ -294,33 +257,6 @@ protected: /// @param mutable_config - configuration to which defaults should be added virtual void setCfgDefaults(isc::data::ElementPtr mutable_config); - /// @brief Parses an individual element - /// - /// Each element to be parsed is passed into this method to be converted - /// into the requisite application object(s). - /// - /// @param element_id name of the element as it is expected in the cfg - /// @param element value of the element as ElementPtr - /// - virtual void parseElement(const std::string& element_id, - isc::data::ConstElementPtr element); - - /// @brief Parses a set of scalar configuration elements into global - /// parameters - /// - /// For each scalar element in the set: - /// - Invoke parseElement - /// - If it returns true go to the next element otherwise: - /// - 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 Abstract factory which creates a context instance. /// /// This method is used at the beginning of configuration process to @@ -361,29 +297,6 @@ protected: bool check_only); private: - - /// @brief Parse a configuration element. - /// - /// Given an element_id and data value, invoke parseElement. If - /// it returns true the return, otherwise created the appropriate - /// parser, parse the data value, and commit the results. - /// - /// - /// @param element_id is the string name of the element as it will appear - /// in the configuration set. - /// @param value is the data value to be parsed and associated with - /// element_id. - /// - /// @throw throws DCfgMgrBaseError if an error occurs. - void buildAndCommit(std::string& element_id, - isc::data::ConstElementPtr value); - - /// @brief A list of element ids which specifies the element parsing order. - /// - /// If the list is empty, the natural order in the configuration set - /// it used. - ElementIdList parse_order_; - /// @brief Pointer to the configuration context instance. DCfgContextBasePtr context_; }; diff --git a/src/lib/process/tests/d_cfg_mgr_unittests.cc b/src/lib/process/tests/d_cfg_mgr_unittests.cc index 99620dcb20..14ed0feb01 100644 --- a/src/lib/process/tests/d_cfg_mgr_unittests.cc +++ b/src/lib/process/tests/d_cfg_mgr_unittests.cc @@ -114,140 +114,12 @@ TEST_F(DStubCfgMgrTest, basicParseTest) { ASSERT_TRUE(fromJSON(config)); // Verify that we can parse a simple configuration. - answer_ = cfg_mgr_->parseConfig(config_set_, false); + answer_ = cfg_mgr_->simpleParseConfig(config_set_, false); EXPECT_TRUE(checkAnswer(0)); // Verify that we can check a simple configuration. - answer_ = cfg_mgr_->parseConfig(config_set_, true); - EXPECT_TRUE(checkAnswer(0)); - - // Verify that an unknown element error is caught and returns a failed - // parse result. - SimFailure::set(SimFailure::ftElementUnknown); - answer_ = cfg_mgr_->parseConfig(config_set_, false); - EXPECT_TRUE(checkAnswer(1)); - - // Verify that an error is caught too when the config is checked for. - SimFailure::set(SimFailure::ftElementUnknown); - answer_ = cfg_mgr_->parseConfig(config_set_, true); - EXPECT_TRUE(checkAnswer(1)); -} - -///@brief Tests ordered and non-ordered element parsing -/// This test verifies that: -/// 1. Non-ordered parsing parses elements in the order they are presented -/// by the configuration set (as-they-come). -/// 2. A parse order list with too few elements is detected. -/// 3. Ordered parsing parses the elements in the order specified by the -/// configuration manager's parse order list. -/// 4. A parse order list with too many elements is detected. -TEST_F(DStubCfgMgrTest, parseOrderTest) { - // Element ids used for test. - 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. - - // NOTE that element sets produced by isc::data::Element::fromJSON(), - // 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 = "{" - " \"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. - // Create an expected parse order. - // (NOTE that iterating over Element sets produced by fromJSON() will - // 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); - - // Verify that the manager has an EMPTY parse order list. (Empty list - // instructs the manager to parse them as-they-come.) - EXPECT_EQ(0, cfg_mgr_->getParseOrder().size()); - - // Parse the configuration, verify it parses without error. - answer_ = cfg_mgr_->parseConfig(config_set_, false); - EXPECT_TRUE(checkAnswer(0)); - - // Verify that the parsed order matches what we expected. - EXPECT_TRUE(cfg_mgr_->parsed_order_ == order_expected); - - // Clear the manager's parse order "memory". - cfg_mgr_->parsed_order_.clear(); - - // Create a parse order list that has too few entries. Verify that - // when parsing the test config, it fails. - cfg_mgr_->addToParseOrder(charlie); - // Verify the parse order list is the size we expect. - EXPECT_EQ(1, cfg_mgr_->getParseOrder().size()); - - // Verify the configuration fails. - answer_ = cfg_mgr_->parseConfig(config_set_, false); - EXPECT_TRUE(checkAnswer(1)); - - // Verify that the configuration parses correctly, when the parse order - // is correct. Add the needed entries to the parse order - cfg_mgr_->addToParseOrder(bravo); - cfg_mgr_->addToParseOrder(alpha); - - // Verify the parse order list is the size we expect. - EXPECT_EQ(3, cfg_mgr_->getParseOrder().size()); - - // Clear the manager's parse order "memory". - cfg_mgr_->parsed_order_.clear(); - - // Verify the configuration parses without error. - answer_ = cfg_mgr_->parseConfig(config_set_, false); + answer_ = cfg_mgr_->simpleParseConfig(config_set_, true); 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_->parsed_order_ == order_expected); - - // Create a parse order list that has too many entries. Verify that - // when parsing the test config, it fails. - cfg_mgr_->addToParseOrder("delta"); - - // Verify the parse order list is the size we expect. - EXPECT_EQ(4, cfg_mgr_->getParseOrder().size()); - - // Verify the configuration fails. - answer_ = cfg_mgr_->parseConfig(config_set_, false); - EXPECT_TRUE(checkAnswer(1)); } /// @brief Tests that element ids supported by the base class as well as those @@ -270,7 +142,7 @@ TEST_F(DStubCfgMgrTest, simpleTypesTest) { ASSERT_TRUE(fromJSON(config)); // Verify that the configuration parses without error. - answer_ = cfg_mgr_->parseConfig(config_set_, false); + answer_ = cfg_mgr_->simpleParseConfig(config_set_, false); ASSERT_TRUE(checkAnswer(0)); DStubContextPtr context = getStubContext(); ASSERT_TRUE(context); @@ -284,7 +156,7 @@ TEST_F(DStubCfgMgrTest, simpleTypesTest) { ASSERT_TRUE(fromJSON(config2)); // Verify that the configuration parses without error. - answer_ = cfg_mgr_->parseConfig(config_set_, false); + answer_ = cfg_mgr_->simpleParseConfig(config_set_, false); EXPECT_TRUE(checkAnswer(0)); context = getStubContext(); ASSERT_TRUE(context); @@ -303,7 +175,7 @@ TEST_F(DStubCfgMgrTest, rollBackTest) { ASSERT_TRUE(fromJSON(config)); // Verify that the configuration parses without error. - answer_ = cfg_mgr_->parseConfig(config_set_, false); + answer_ = cfg_mgr_->simpleParseConfig(config_set_, false); EXPECT_TRUE(checkAnswer(0)); DStubContextPtr context = getStubContext(); ASSERT_TRUE(context); @@ -317,13 +189,6 @@ TEST_F(DStubCfgMgrTest, rollBackTest) { " \"list_test2\": [] , " " \"zeta_unknown\": 33 } "; ASSERT_TRUE(fromJSON(config2)); - - // Force a failure on the last element - SimFailure::set(SimFailure::ftElementUnknown); - answer_ = cfg_mgr_->parseConfig(config_set_, false); - EXPECT_TRUE(checkAnswer(1)); - context = getStubContext(); - ASSERT_TRUE(context); } /// @brief Tests that the configuration context is preserved during @@ -338,7 +203,7 @@ TEST_F(DStubCfgMgrTest, checkOnly) { ASSERT_TRUE(fromJSON(config)); // Verify that the configuration parses without error. - answer_ = cfg_mgr_->parseConfig(config_set_, false); + answer_ = cfg_mgr_->simpleParseConfig(config_set_, false); EXPECT_TRUE(checkAnswer(0)); DStubContextPtr context = getStubContext(); ASSERT_TRUE(context); @@ -352,7 +217,7 @@ TEST_F(DStubCfgMgrTest, checkOnly) { " \"list_test2\": [] }"; ASSERT_TRUE(fromJSON(config2)); - answer_ = cfg_mgr_->parseConfig(config_set_, true); + answer_ = cfg_mgr_->simpleParseConfig(config_set_, true); EXPECT_TRUE(checkAnswer(0)); context = getStubContext(); ASSERT_TRUE(context); @@ -369,7 +234,7 @@ TEST_F(DStubCfgMgrTest, paramPosition) { ASSERT_TRUE(fromJSON(config)); // Verify that the configuration parses without error. - answer_ = cfg_mgr_->parseConfig(config_set_, false); + answer_ = cfg_mgr_->simpleParseConfig(config_set_, false); ASSERT_TRUE(checkAnswer(0)); DStubContextPtr context = getStubContext(); ASSERT_TRUE(context); diff --git a/src/lib/process/testutils/d_test_stubs.cc b/src/lib/process/testutils/d_test_stubs.cc index 0b573c402d..ed15e0a854 100644 --- a/src/lib/process/testutils/d_test_stubs.cc +++ b/src/lib/process/testutils/d_test_stubs.cc @@ -71,7 +71,7 @@ DStubProcess::configure(isc::data::ConstElementPtr config_set, bool check_only) "Simulated process configuration error.")); } - return (getCfgMgr()->parseConfig(config_set, check_only)); + return (getCfgMgr()->simpleParseConfig(config_set, check_only)); } DStubProcess::~DStubProcess() { @@ -296,24 +296,6 @@ DStubCfgMgr::createNewContext() { return (DCfgContextBasePtr (new DStubContext())); } -void -DStubCfgMgr::parseElement(const std::string& element_id, - isc::data::ConstElementPtr element) { - DStubContextPtr context - = boost::dynamic_pointer_cast(getContext()); - - // 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 - << element->getPosition()); - } - - parsed_order_.push_back(element_id); -} - isc::data::ConstElementPtr DStubCfgMgr::parse(isc::data::ConstElementPtr /*config*/, bool /*check_only*/) { return (isc::config::createAnswer(0, "It all went fine. I promise")); diff --git a/src/lib/process/testutils/d_test_stubs.h b/src/lib/process/testutils/d_test_stubs.h index 92d56eaf06..505de4880a 100644 --- a/src/lib/process/testutils/d_test_stubs.h +++ b/src/lib/process/testutils/d_test_stubs.h @@ -622,30 +622,6 @@ public: /// @brief Destructor virtual ~DStubCfgMgr(); - /// @brief Parses the given element into the appropriate object - /// - /// The method supports three named elements: - /// - /// -# "bool_test" - /// -# "uint32_test" - /// -# "string_test" - /// - /// which are parsed and whose value is then stored in the - /// the appropriate context value store. - /// - /// Any other element_id is treated generically and stored - /// in the context's object store, unless the simulated - /// error has been set to SimFailure::ftElementUnknown. - /// - /// @param element_id name of the element to parse - /// @param element Element to parse - /// - /// @throw DCfgMgrBaseError if simulated error is set - /// to ftElementUnknown and element_id is not one of - /// the named elements. - virtual void parseElement(const std::string& element_id, - isc::data::ConstElementPtr element); - /// @brief Pretends to parse the config /// /// This method pretends to parse the configuration specified on input @@ -666,10 +642,6 @@ public: return (""); } - /// @brief A list for remembering the element ids in the order they were - /// parsed. - ElementIdList parsed_order_; - /// @todo virtual DCfgContextBasePtr createNewContext(); };