From a9a3d0d5dd49a9e1f772d9f85486bbe81612130a Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 7 Dec 2016 10:32:22 -0500 Subject: [PATCH 01/13] [5046] kea-dhcp6 now implements set-config command src/bin/dhcp6/ctrl_dhcp6_srv.h src/bin/dhcp6/ctrl_dhcp6_srv.cc ControlledDhcpv6Srv::commandSetConfigHandler() - new method to process the set-config command. ControlledDhcpv6Srv::processCommand() - call new set-config handler ControlledDhcpv6Srv::processConfig() - added logic to apply logging and commit configuration after successful reconfig ControlledDhcpv6Srv::ControlledDhcpv6Srv(uint16_t port) - added registration of set-config command ControlledDhcpv6Srv::~ControlledDhcpv6Srv() - unregisters set-config command src/bin/dhcp6/json_config_parser.cc configureCommandChannel() - extracted logic to reconfigure command channel to its own fucntion src/bin/dhcp6/kea_controller.cc configure() - removed logic to apply logging and commit config, now done in ControlledDhcpv6Srv::processConfig() src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc TEST_F(CtrlChannelDhcpv6SrvTest, set_config) - new test to exercise the set-config command --- src/bin/dhcp6/ctrl_dhcp6_srv.cc | 54 ++++++- src/bin/dhcp6/ctrl_dhcp6_srv.h | 14 ++ src/bin/dhcp6/json_config_parser.cc | 62 +++++--- src/bin/dhcp6/kea_controller.cc | 9 -- .../dhcp6/tests/ctrl_dhcp6_srv_unittest.cc | 133 +++++++++++++++++- 5 files changed, 242 insertions(+), 30 deletions(-) diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 8a39820916..4649713015 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -67,10 +67,46 @@ ControlledDhcpv6Srv::commandLibReloadHandler(const string&, ConstElementPtr) { ConstElementPtr ControlledDhcpv6Srv::commandConfigReloadHandler(const string&, ConstElementPtr args) { - return (processConfig(args)); } +ConstElementPtr +ControlledDhcpv6Srv::commandSetConfigHandler(const string&, + ConstElementPtr args) { + const int status_code = 1; // 1 indicates an error + ConstElementPtr dhcp6; + string message; + + // Throw out the preivous staging config that may be present + CfgMgr::instance().rollback(); + + // Command arguments are expected to be: + // { "Dhcp6": { ... }, "Logging": { ... } } + // The Logging component is technically optional, but very recommended. + if (!args) { + message = "Missing mandatory 'arguments' parameter."; + } else { + dhcp6 = args->get("Dhcp6"); + if (!dhcp6) { + message = "Missing mandatory 'Dhcp6' parameter."; + } else if (dhcp6->getType() != Element::map) { + message = "'Dhcp6' parameter expected to be a map."; + } + } + + if (!message.empty()) { + // Something is amiss with arguments, return a failure response. + ConstElementPtr result = isc::config::createAnswer(status_code, + message); + return (result); + } + + // Now that we have extracted the configuration, reuse the reload command + // to configure the server. + return (ControlledDhcpv6Srv::processCommand("config-reload", dhcp6)); +} + + ConstElementPtr ControlledDhcpv6Srv::commandLeasesReclaimHandler(const string&, ConstElementPtr args) { @@ -122,6 +158,9 @@ ControlledDhcpv6Srv::processCommand(const std::string& command, } else if (command == "config-reload") { return (srv->commandConfigReloadHandler(command, args)); + } else if (command == "set-config") { + return (srv->commandSetConfigHandler(command, args)); + } else if (command == "leases-reclaim") { return (srv->commandLeasesReclaimHandler(command, args)); } @@ -260,6 +299,14 @@ ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) { } } + // Configuration was parsed successfully, apply the new logger + // configuration to log4cplus. It is done before commit in case + // something goes wrong. + CfgMgr::instance().getStagingCfg()->applyLoggingCfg(); + + // Use new configuration. + CfgMgr::instance().commit(); + // Finally, we can commit runtime option definitions in libdhcp++. This is // exception free. LibDHCP::commitRuntimeOptionDefs(); @@ -280,6 +327,10 @@ ControlledDhcpv6Srv::ControlledDhcpv6Srv(uint16_t port) boost::bind(&ControlledDhcpv6Srv::commandShutdownHandler, this, _1, _2)); /// @todo: register config-reload (see CtrlDhcpv4Srv::commandConfigReloadHandler) + + CommandMgr::instance().registerCommand("set-config", + boost::bind(&ControlledDhcpv6Srv::commandSetConfigHandler, this, _1, _2)); + /// @todo: register libreload (see CtrlDhcpv4Srv::commandLibReloadHandler) CommandMgr::instance().registerCommand("leases-reclaim", @@ -324,6 +375,7 @@ ControlledDhcpv6Srv::~ControlledDhcpv6Srv() { // Deregister any registered commands CommandMgr::instance().deregisterCommand("shutdown"); + CommandMgr::instance().deregisterCommand("set-config"); CommandMgr::instance().deregisterCommand("leases-reclaim"); CommandMgr::instance().deregisterCommand("statistic-get"); CommandMgr::instance().deregisterCommand("statistic-reset"); diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.h b/src/bin/dhcp6/ctrl_dhcp6_srv.h index b1ca9587c1..1c6bde1664 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.h +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.h @@ -143,6 +143,20 @@ private: commandConfigReloadHandler(const std::string& command, isc::data::ConstElementPtr args); + /// @brief handler for processing 'set-config' command + /// + /// This handler processes set-config command, which processes + /// configuration specified in args parameter. + /// @param command (parameter ignored) + /// @param args configuration to be processed. Expected format: + /// map containing Dhcp6 map that contains DHCPv6 server configuration. + /// May also contain Logging map that specifies logging configuration. + /// + /// @return status of the command + isc::data::ConstElementPtr + commandSetConfigHandler(const std::string& command, + isc::data::ConstElementPtr args); + /// @brief Handler for processing 'leases-reclaim' command /// /// This handler processes leases-reclaim command, which triggers diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index 995390f4cc..648ffa9e27 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -770,6 +770,47 @@ void setGlobalParameters6() { } } +/// @brief Initialize the command channel based on the staging configuration +/// +/// Only close the current channel, if the new channel configuration is +/// different. This avoids disconnecting a client and hence not sending them +/// a command result, unless they specifically alter the channel configuration. +/// In that case the user simply has to accept they'll be disconnected. +/// +void configureCommandChannel() { + // Get new socket configuration. + ConstElementPtr sock_cfg = + CfgMgr::instance().getStagingCfg()->getControlSocketInfo(); + + // Get current socket configuration. + ConstElementPtr current_sock_cfg = + CfgMgr::instance().getCurrentCfg()->getControlSocketInfo(); + + // Determine if the socket configuration has changed. It has if + // both old and new configuration is specified but respective + // data elements are't equal. + bool sock_changed = (sock_cfg && current_sock_cfg && + !sock_cfg->equals(*current_sock_cfg)); + + // If the previous or new socket configuration doesn't exist or + // the new configuration differs from the old configuration we + // close the exisitng socket and open a new socket as appropriate. + // Note that closing an existing socket means the clien will not + // receive the configuration result. + if (!sock_cfg || !current_sock_cfg || sock_changed) { + // Close the existing socket (if any). + isc::config::CommandMgr::instance().closeCommandSocket(); + + if (sock_cfg) { + // This will create a control socket and install the external + // socket in IfaceMgr. That socket will be monitored when + // Dhcp4Srv::receivePacket() calls IfaceMgr::receive4() and + // callback in CommandMgr will be called, if necessary. + isc::config::CommandMgr::instance().openCommandSocket(sock_cfg); + } + } +} + isc::data::ConstElementPtr configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { if (!config_set) { @@ -904,25 +945,8 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { subnet_parser->build(subnet_config->second); } - // Get command socket configuration from the config file. - // This code expects the following structure: - // { - // "socket-type": "unix", - // "socket-name": "/tmp/kea6.sock" - // } - ConstElementPtr sock_cfg = - CfgMgr::instance().getStagingCfg()->getControlSocketInfo(); - - // Close existing socket (if any). - isc::config::CommandMgr::instance().closeCommandSocket(); - if (sock_cfg) { - // This will create a control socket and will install external socket - // in IfaceMgr. That socket will be monitored when Dhcp4Srv::receivePacket() - // calls IfaceMgr::receive4() and callback in CommandMgr will be called, - // if necessary. If there were previously open command socket, it will - // be closed. - isc::config::CommandMgr::instance().openCommandSocket(sock_cfg); - } + // Setup the command channel. + configureCommandChannel(); // The lease database parser is the last to be run. std::map::const_iterator leases_config = diff --git a/src/bin/dhcp6/kea_controller.cc b/src/bin/dhcp6/kea_controller.cc index f77ee5af4c..e8d7b8251d 100644 --- a/src/bin/dhcp6/kea_controller.cc +++ b/src/bin/dhcp6/kea_controller.cc @@ -107,15 +107,6 @@ void configure(const std::string& file_name) { "no details available"; isc_throw(isc::BadValue, reason); } - - // If configuration was parsed successfully, apply the new logger - // configuration to log4cplus. It is done before commit in case - // something goes wrong. - CfgMgr::instance().getStagingCfg()->applyLoggingCfg(); - - // Use new configuration. - CfgMgr::instance().commit(); - } catch (const std::exception& ex) { // If configuration failed at any stage, we drop the staging // configuration and continue to use the previous one. diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc index 9b6ba605be..0d66798b63 100644 --- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc @@ -146,6 +146,7 @@ public: ASSERT_NO_THROW(server_.reset(new NakedControlledDhcpv6Srv())); ConstElementPtr config = Element::fromJSON(config_txt); + ConstElementPtr answer = server_->processConfig(config); ASSERT_TRUE(answer); @@ -335,13 +336,143 @@ TEST_F(CtrlDhcpv6SrvTest, configReload) { // Check that the config was indeed applied. const Subnet6Collection* subnets = - CfgMgr::instance().getStagingCfg()->getCfgSubnets6()->getAll(); + CfgMgr::instance().getCurrentCfg()->getCfgSubnets6()->getAll(); EXPECT_EQ(3, subnets->size()); // Clean up after the test. CfgMgr::instance().clear(); } +// Check that the "set-config" command will replace current configuration +TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { + createUnixChannelServer(); + + // Define strings to permutate the config arguments + // (Note the line feeds makes errors easy to find) + string set_config_txt = "{ \"command\": \"set-config\" \n"; + string args_txt = " \"arguments\": { \n"; + string dhcp6_cfg_txt = + " \"Dhcp6\": { \n" + " \"interfaces-config\": { \n" + " \"interfaces\": [\"*\"] \n" + " }, \n" + " \"preferred-lifetime\": 3000, \n" + " \"valid-lifetime\": 4000, \n" + " \"renew-timer\": 1000, \n" + " \"rebind-timer\": 2000, \n" + " \"subnet6\": [ \n"; + string subnet1 = + " {\"subnet\": \"3002::/64\", \n" + " \"pools\": [{ \"pool\": \"3002::100-3002::200\" }]}\n"; + string subnet2 = + " {\"subnet\": \"3003::/64\", \n" + " \"pools\": [{ \"pool\": \"3003::100-3003::200\" }]}\n"; + string bad_subnet = + " {\"BOGUS\": \"3005::/64\", \n" + " \"pools\": [{ \"pool\": \"3005::100-3005::200\" }]}\n"; + string subnet_footer = + " ] \n"; + string control_socket_header = + " ,\"control-socket\": { \n" + " \"socket-type\": \"unix\", \n" + " \"socket-name\": \""; + string control_socket_footer = + "\" \n} \n"; + string logger_txt = + " \"Logging\": { \n" + " \"loggers\": [ { \n" + " \"name\": \"*\", \n" + " \"severity\": \"INFO\", \n" + " \"debuglevel\": 0, \n" + " \"output_options\": [{ \n" + " \"output\": \"stdout\" \n" + " }] \n" + " }] \n" + " } \n"; + + std::ostringstream os; + + // Create a valid config with all the parts should parse + os << set_config_txt << "," + << args_txt + << dhcp6_cfg_txt + << subnet1 + << subnet_footer + << control_socket_header + << socket_path_ + << control_socket_footer + << "}\n" // close dhcp6 + << "," + << logger_txt << "}}"; + + // Send the set-config command + std::string response; + sendUnixCommand(os.str(), response); + + // Verify the configuration was successful. + EXPECT_EQ("{ \"result\": 0, \"text\": \"Configuration successful.\" }", + response); + + // Check that the config was indeed applied. + const Subnet6Collection* subnets = + CfgMgr::instance().getCurrentCfg()->getCfgSubnets6()->getAll(); + EXPECT_EQ(1, subnets->size()); + + // Create a config with malformed subnet that should fail to parse. + os.str(""); + os << set_config_txt << "," + << args_txt + << dhcp6_cfg_txt + << bad_subnet + << subnet_footer + << control_socket_header + << socket_path_ + << control_socket_footer + << "}\n" // close dhcp6 + << "," + << logger_txt << "}}"; + + // Send the set-config command + sendUnixCommand(os.str(), response); + + // Should fail with a syntax error + EXPECT_EQ("{ \"result\": 1, " + "\"text\": \"unsupported parameter: BOGUS (:12:33)\" }", + response); + + // Check that the config was not lost + subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets6()->getAll(); + EXPECT_EQ(1, subnets->size()); + + // Create a valid config with two subnets and no command channel. + // It should succeed but client will not receive a the response + os.str(""); + os << set_config_txt << "," + << args_txt + << dhcp6_cfg_txt + << subnet1 + << ",\n" + << subnet2 + << subnet_footer + << "}\n" // close dhcp6 + << "," + << logger_txt << "}}"; + + // Send the set-config command + sendUnixCommand(os.str(), response); + + // With no command channel, no response + EXPECT_EQ("", response); + + // Check that the config was not lost + subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets6()->getAll(); + EXPECT_EQ(2, subnets->size()); + + // Clean up after the test. + CfgMgr::instance().clear(); +} + + typedef std::map ElementMap; // This test checks which commands are registered by the DHCPv4 server. From cb8f8ae36e8236e8181dac7dee212295d47a5778 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 8 Dec 2016 14:56:17 -0500 Subject: [PATCH 02/13] [5046] Fixed logger setup on set-config src/bin/dhcp6/ctrl_dhcp6_srv.cc ControlledDhcpv6Srv::commandSetConfigHandler() - Add logger config - Use processConfig() directly instead of config-reload command src/bin/dhcp6/kea_controller.cc configure(const std::string& file_name) - Remove logger config - Use set-config command instead of config-reload src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc createUnixChannelServer() - added call to initLogger() to revert logging to unit test logger src/bin/dhcp6/tests/dhcp6_test_utils.cc BaseServerTest::~BaseServerTest() { - added call to initLogger() to revert logging to unit test logger --- src/bin/dhcp6/ctrl_dhcp6_srv.cc | 12 +++++++---- src/bin/dhcp6/kea_controller.cc | 20 ++----------------- .../dhcp6/tests/ctrl_dhcp6_srv_unittest.cc | 19 +++++++++++------- src/bin/dhcp6/tests/dhcp6_test_utils.cc | 4 ++++ 4 files changed, 26 insertions(+), 29 deletions(-) diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 4649713015..0e493c5ade 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -80,6 +80,12 @@ ControlledDhcpv6Srv::commandSetConfigHandler(const string&, // Throw out the preivous staging config that may be present CfgMgr::instance().rollback(); + // Logging is a sibling element and so, has to be explicitly + // configured. We should call configureLogger even if there's no + // Logging element as this will revert it to default logging. + Daemon::configureLogger(args->get("Logging"), + CfgMgr::instance().getStagingCfg()); + // Command arguments are expected to be: // { "Dhcp6": { ... }, "Logging": { ... } } // The Logging component is technically optional, but very recommended. @@ -100,10 +106,8 @@ ControlledDhcpv6Srv::commandSetConfigHandler(const string&, message); return (result); } - - // Now that we have extracted the configuration, reuse the reload command - // to configure the server. - return (ControlledDhcpv6Srv::processCommand("config-reload", dhcp6)); + // Now we configure the server proper. + return (processConfig(dhcp6)); } diff --git a/src/bin/dhcp6/kea_controller.cc b/src/bin/dhcp6/kea_controller.cc index e8d7b8251d..0e7352ca81 100644 --- a/src/bin/dhcp6/kea_controller.cc +++ b/src/bin/dhcp6/kea_controller.cc @@ -72,29 +72,14 @@ void configure(const std::string& file_name) { " Did you forget to add { } around your configuration?"); } - // Let's configure logging before applying the configuration, - // so we can log things during configuration process. - // If there's no logging element, we'll just pass NULL pointer, - // which will be handled by configureLogger(). - Daemon::configureLogger(json->get("Logging"), - CfgMgr::instance().getStagingCfg()); - - // Get Dhcp6 component from the config - dhcp6 = json->get("Dhcp6"); - - if (!dhcp6) { - isc_throw(isc::BadValue, "no mandatory 'Dhcp6' entry in" - " the configuration"); - } - // Use parsed JSON structures to configure the server - result = ControlledDhcpv6Srv::processCommand("config-reload", dhcp6); + result = ControlledDhcpv6Srv::processCommand("set-config", json); if (!result) { // Undetermined status of the configuration. This should never // happen, but as the configureDhcp6Server returns a pointer, it is // theoretically possible that it will return NULL. isc_throw(isc::BadValue, "undefined result of " - "processCommand(\"config-reload\", dhcp6)"); + "processCommand(\"set-config\", dhcp6)"); } // Now check is the returned result is successful (rcode=0) or not @@ -117,7 +102,6 @@ void configure(const std::string& file_name) { isc_throw(isc::BadValue, "configuration error using file '" << file_name << "': " << ex.what()); } - } /// @brief Signals handler for DHCPv6 server. diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc index 0d66798b63..0dea9e3280 100644 --- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -150,6 +151,11 @@ public: ConstElementPtr answer = server_->processConfig(config); ASSERT_TRUE(answer); + // If the configuration doesn't contain logging config, processConfig() + // will revert the logging to default (stdout). We call initLogger() + // to restore unit test logging. + isc::log::initLogger(); + int status = 0; ConstElementPtr txt = isc::config::parseAnswer(status, answer); // This should succeed. If not, print the error message. @@ -385,7 +391,7 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { " \"severity\": \"INFO\", \n" " \"debuglevel\": 0, \n" " \"output_options\": [{ \n" - " \"output\": \"stdout\" \n" + " \"output\": \"/dev/null\" \n" " }] \n" " }] \n" " } \n"; @@ -403,7 +409,8 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { << control_socket_footer << "}\n" // close dhcp6 << "," - << logger_txt << "}}"; + << logger_txt + << "}}"; // Send the set-config command std::string response; @@ -429,15 +436,14 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { << socket_path_ << control_socket_footer << "}\n" // close dhcp6 - << "," - << logger_txt << "}}"; + "}}"; // Send the set-config command sendUnixCommand(os.str(), response); // Should fail with a syntax error EXPECT_EQ("{ \"result\": 1, " - "\"text\": \"unsupported parameter: BOGUS (:12:33)\" }", + "\"text\": \"unsupported parameter: BOGUS (:12:26)\" }", response); // Check that the config was not lost @@ -455,8 +461,7 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { << subnet2 << subnet_footer << "}\n" // close dhcp6 - << "," - << logger_txt << "}}"; + << "}}"; // Send the set-config command sendUnixCommand(os.str(), response); diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.cc b/src/bin/dhcp6/tests/dhcp6_test_utils.cc index e7f06b7969..5c4b70d30d 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.cc +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -46,6 +47,9 @@ BaseServerTest::~BaseServerTest() { // Revert to original data directory. CfgMgr::instance().setDataDir(original_datadir_); + + // Revert to unit test logging + isc::log::initLogger(); } Dhcpv6SrvTest::Dhcpv6SrvTest() From 767bf1503f982d75f5c9fcca219f86a8345b5802 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 9 Dec 2016 16:06:38 -0500 Subject: [PATCH 03/13] [5046] Avoid wiping logging when config is empty src/lib/dhcpsrv/srv_config.cc SrvConfig::applyLoggingCfg() - now only calls LoggerManager::process() if the logger config isn't empty src/bin/dhcp6/ctrl_dhcp6_srv.cc src/bin/dhcp6/tests/dhcp6_test_utils.cc Minor clean up and commentary src/bin/dhcp6/kea_controller.cc configure(const std::string& file_name) - Removed initial rollback, now done in commandSetConfigHandler() src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc - Removed unnecessary call to initLogger --- src/bin/dhcp6/ctrl_dhcp6_srv.cc | 19 ++++++++++++------- src/bin/dhcp6/kea_controller.cc | 9 ++------- .../dhcp6/tests/ctrl_dhcp6_srv_unittest.cc | 5 ----- src/bin/dhcp6/tests/dhcp6_test_utils.cc | 2 +- src/lib/dhcpsrv/srv_config.cc | 10 ++++++++-- 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 0e493c5ade..7bbf673349 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -77,15 +77,11 @@ ControlledDhcpv6Srv::commandSetConfigHandler(const string&, ConstElementPtr dhcp6; string message; - // Throw out the preivous staging config that may be present + // We are starting the configuration process so we should remove any + // staging configuration that has been created during previous + // configuration attempts. CfgMgr::instance().rollback(); - // Logging is a sibling element and so, has to be explicitly - // configured. We should call configureLogger even if there's no - // Logging element as this will revert it to default logging. - Daemon::configureLogger(args->get("Logging"), - CfgMgr::instance().getStagingCfg()); - // Command arguments are expected to be: // { "Dhcp6": { ... }, "Logging": { ... } } // The Logging component is technically optional, but very recommended. @@ -106,6 +102,15 @@ ControlledDhcpv6Srv::commandSetConfigHandler(const string&, message); return (result); } + + // Logging is a sibling element and must be be parsed explicitly. + // The call to configureLogger parses the given Logging element if + // not null, into the staging config. Note this DOES alter the + // current loggers, they remain in effect until we apply the + // logging config. + Daemon::configureLogger(args->get("Logging"), + CfgMgr::instance().getStagingCfg()); + // Now we configure the server proper. return (processConfig(dhcp6)); } diff --git a/src/bin/dhcp6/kea_controller.cc b/src/bin/dhcp6/kea_controller.cc index 0e7352ca81..8ee2b64b47 100644 --- a/src/bin/dhcp6/kea_controller.cc +++ b/src/bin/dhcp6/kea_controller.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -37,11 +37,6 @@ void configure(const std::string& file_name) { // This is a configuration backend implementation that reads the // configuration from a JSON file. - // We are starting the configuration process so we should remove any - // staging configuration that has been created during previous - // configuration attempts. - CfgMgr::instance().rollback(); - isc::data::ConstElementPtr json; isc::data::ConstElementPtr dhcp6; isc::data::ConstElementPtr logger; @@ -79,7 +74,7 @@ void configure(const std::string& file_name) { // happen, but as the configureDhcp6Server returns a pointer, it is // theoretically possible that it will return NULL. isc_throw(isc::BadValue, "undefined result of " - "processCommand(\"set-config\", dhcp6)"); + "processCommand(\"set-config\", json)"); } // Now check is the returned result is successful (rcode=0) or not diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc index 0dea9e3280..98c88b44d8 100644 --- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc @@ -151,11 +151,6 @@ public: ConstElementPtr answer = server_->processConfig(config); ASSERT_TRUE(answer); - // If the configuration doesn't contain logging config, processConfig() - // will revert the logging to default (stdout). We call initLogger() - // to restore unit test logging. - isc::log::initLogger(); - int status = 0; ConstElementPtr txt = isc::config::parseAnswer(status, answer); // This should succeed. If not, print the error message. diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.cc b/src/bin/dhcp6/tests/dhcp6_test_utils.cc index 5c4b70d30d..a1c74dab10 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.cc +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.cc @@ -48,7 +48,7 @@ BaseServerTest::~BaseServerTest() { // Revert to original data directory. CfgMgr::instance().setDataDir(original_datadir_); - // Revert to unit test logging + // Revert to unit test logging in case the test reconfigured logging. isc::log::initLogger(); } diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 7c7830966f..017d83767e 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -117,8 +117,14 @@ SrvConfig::applyLoggingCfg() const { it != logging_info_.end(); ++it) { specs.push_back(it->toSpec()); } - LoggerManager manager; - manager.process(specs.begin(), specs.end()); + + // If there are no specs, then leave logging alone. This will leave + // the existing logging setup intact. Calling process() with no + // specs will reset the logging heirarchy and so forth. + if (specs.size()) { + LoggerManager manager; + manager.process(specs.begin(), specs.end()); + } } bool From 44d38ce10ac4f0ed22fcf329a9ddd6a1f2d0099c Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 9 Dec 2016 16:08:24 -0500 Subject: [PATCH 04/13] [5046] Implement set-config command in kea-dhcp4 Mirror the changes made in kea-dhcp6. --- src/bin/dhcp4/ctrl_dhcp4_srv.cc | 59 ++++++++++++++++++ src/bin/dhcp4/ctrl_dhcp4_srv.h | 13 ++++ src/bin/dhcp4/json_config_parser.cc | 62 +++++++++++++------ src/bin/dhcp4/kea_controller.cc | 34 +--------- .../dhcp4/tests/ctrl_dhcp4_srv_unittest.cc | 8 +++ src/bin/dhcp4/tests/dhcp4_test_utils.cc | 4 ++ src/bin/dhcp4/tests/dhcp4_test_utils.h | 5 ++ 7 files changed, 135 insertions(+), 50 deletions(-) diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 4cd019a315..3ac909d914 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -65,6 +65,51 @@ ControlledDhcpv4Srv::commandConfigReloadHandler(const string&, return (processConfig(args)); } +ConstElementPtr +ControlledDhcpv4Srv::commandSetConfigHandler(const string&, + ConstElementPtr args) { + const int status_code = 1; // 1 indicates an error + ConstElementPtr dhcp4; + string message; + + // We are starting the configuration process so we should remove any + // staging configuration that has been created during previous + // configuration attempts. + CfgMgr::instance().rollback(); + + // Command arguments are expected to be: + // { "Dhcp4": { ... }, "Logging": { ... } } + // The Logging component is technically optional, but very recommended. + if (!args) { + message = "Missing mandatory 'arguments' parameter."; + } else { + dhcp4 = args->get("Dhcp4"); + if (!dhcp4) { + message = "Missing mandatory 'Dhcp4' parameter."; + } else if (dhcp4->getType() != Element::map) { + message = "'Dhcp4' parameter expected to be a map."; + } + } + + if (!message.empty()) { + // Something is amiss with arguments, return a failure response. + ConstElementPtr result = isc::config::createAnswer(status_code, + message); + return (result); + } + + // Logging is a sibling element and must be be parsed explicitly. + // The call to configureLogger parses the given Logging element if + // not null, into the staging config. Note this DOES alter the + // current loggers, they remain in effect until we apply the + // logging config. + Daemon::configureLogger(args->get("Logging"), + CfgMgr::instance().getStagingCfg()); + + // Now we configure the server proper. + return (processConfig(dhcp4)); +} + ConstElementPtr ControlledDhcpv4Srv::commandLeasesReclaimHandler(const string&, ConstElementPtr args) { @@ -116,6 +161,9 @@ ControlledDhcpv4Srv::processCommand(const string& command, } else if (command == "config-reload") { return (srv->commandConfigReloadHandler(command, args)); + } else if (command == "set-config") { + return (srv->commandSetConfigHandler(command, args)); + } else if (command == "leases-reclaim") { return (srv->commandLeasesReclaimHandler(command, args)); } @@ -235,7 +283,13 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) { } } + // Configuration was parsed successfully, apply the new logger + // configuration to log4cplus. It is done before commit in case + // something goes wrong. + CfgMgr::instance().getStagingCfg()->applyLoggingCfg(); + // Use new configuration. + CfgMgr::instance().commit(); return (answer); } @@ -253,6 +307,10 @@ ControlledDhcpv4Srv::ControlledDhcpv4Srv(uint16_t port /*= DHCP4_SERVER_PORT*/) boost::bind(&ControlledDhcpv4Srv::commandShutdownHandler, this, _1, _2)); /// @todo: register config-reload (see CtrlDhcpv4Srv::commandConfigReloadHandler) + + CommandMgr::instance().registerCommand("set-config", + boost::bind(&ControlledDhcpv4Srv::commandSetConfigHandler, this, _1, _2)); + /// @todo: register libreload (see CtrlDhcpv4Srv::commandLibReloadHandler) CommandMgr::instance().registerCommand("leases-reclaim", @@ -297,6 +355,7 @@ ControlledDhcpv4Srv::~ControlledDhcpv4Srv() { // Deregister any registered commands CommandMgr::instance().deregisterCommand("shutdown"); + CommandMgr::instance().deregisterCommand("set-config"); CommandMgr::instance().deregisterCommand("leases-reclaim"); CommandMgr::instance().deregisterCommand("statistic-get"); CommandMgr::instance().deregisterCommand("statistic-reset"); diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.h b/src/bin/dhcp4/ctrl_dhcp4_srv.h index 57883e5fd8..6ef362cd41 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.h +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.h @@ -143,6 +143,19 @@ private: commandConfigReloadHandler(const std::string& command, isc::data::ConstElementPtr args); + /// @brief handler for processing 'set-config' command + /// + /// This handler processes set-config command, which processes + /// configuration specified in args parameter. + /// @param command (parameter ignored) + /// @param args configuration to be processed. Expected format: + /// map containing Dhcp4 map that contains DHCPv4 server configuration. + /// May also contain Logging map that specifies logging configuration. + /// + /// @return status of the command + isc::data::ConstElementPtr + commandSetConfigHandler(const std::string& command, + isc::data::ConstElementPtr args); /// @brief Handler for processing 'leases-reclaim' command /// diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index 52b554972b..4b50286056 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -501,6 +501,47 @@ void setGlobalParameters4() { } } +/// @brief Initialize the command channel based on the staging configuration +/// +/// Only close the current channel, if the new channel configuration is +/// different. This avoids disconnecting a client and hence not sending them +/// a command result, unless they specifically alter the channel configuration. +/// In that case the user simply has to accept they'll be disconnected. +/// +void configureCommandChannel() { + // Get new socket configuration. + ConstElementPtr sock_cfg = + CfgMgr::instance().getStagingCfg()->getControlSocketInfo(); + + // Get current socket configuration. + ConstElementPtr current_sock_cfg = + CfgMgr::instance().getCurrentCfg()->getControlSocketInfo(); + + // Determine if the socket configuration has changed. It has if + // both old and new configuration is specified but respective + // data elements are't equal. + bool sock_changed = (sock_cfg && current_sock_cfg && + !sock_cfg->equals(*current_sock_cfg)); + + // If the previous or new socket configuration doesn't exist or + // the new configuration differs from the old configuration we + // close the exisitng socket and open a new socket as appropriate. + // Note that closing an existing socket means the clien will not + // receive the configuration result. + if (!sock_cfg || !current_sock_cfg || sock_changed) { + // Close the existing socket (if any). + isc::config::CommandMgr::instance().closeCommandSocket(); + + if (sock_cfg) { + // This will create a control socket and install the external + // socket in IfaceMgr. That socket will be monitored when + // Dhcp4Srv::receivePacket() calls IfaceMgr::receive4() and + // callback in CommandMgr will be called, if necessary. + isc::config::CommandMgr::instance().openCommandSocket(sock_cfg); + } + } +} + isc::data::ConstElementPtr configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { if (!config_set) { @@ -632,25 +673,8 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { subnet_parser->build(subnet_config->second); } - // Get command socket configuration from the config file. - // This code expects the following structure: - // { - // "socket-type": "unix", - // "socket-name": "/tmp/kea4.sock" - // } - ConstElementPtr sock_cfg = - CfgMgr::instance().getStagingCfg()->getControlSocketInfo(); - - // Close existing socket (if any). - isc::config::CommandMgr::instance().closeCommandSocket(); - if (sock_cfg) { - // This will create a control socket and will install external socket - // in IfaceMgr. That socket will be monitored when Dhcp4Srv::receivePacket() - // calls IfaceMgr::receive4() and callback in CommandMgr will be called, - // if necessary. If there were previously open command socket, it will - // be closed. - isc::config::CommandMgr::instance().openCommandSocket(sock_cfg); - } + // Setup the command channel. + configureCommandChannel(); // the leases database parser is the last to be run. std::map::const_iterator leases_config = diff --git a/src/bin/dhcp4/kea_controller.cc b/src/bin/dhcp4/kea_controller.cc index 102691cc3b..d79d9d037f 100644 --- a/src/bin/dhcp4/kea_controller.cc +++ b/src/bin/dhcp4/kea_controller.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -33,11 +33,6 @@ void configure(const std::string& file_name) { // This is a configuration backend implementation that reads the // configuration from a JSON file. - // We are starting the configuration process so we should remove any - // staging configuration that has been created during previous - // configuration attempts. - CfgMgr::instance().rollback(); - isc::data::ConstElementPtr json; isc::data::ConstElementPtr dhcp4; isc::data::ConstElementPtr logger; @@ -68,26 +63,14 @@ void configure(const std::string& file_name) { " Did you forget to add { } around your configuration?"); } - // If there's no logging element, we'll just pass NULL pointer, - // which will be handled by configureLogger(). - Daemon::configureLogger(json->get("Logging"), - CfgMgr::instance().getStagingCfg()); - - // Get Dhcp4 component from the config - dhcp4 = json->get("Dhcp4"); - if (!dhcp4) { - isc_throw(isc::BadValue, "no mandatory 'Dhcp4' entry in" - " the configuration"); - } - // Use parsed JSON structures to configure the server - result = ControlledDhcpv4Srv::processCommand("config-reload", dhcp4); + result = ControlledDhcpv4Srv::processCommand("set-config", json); if (!result) { // Undetermined status of the configuration. This should never // happen, but as the configureDhcp4Server returns a pointer, it is // theoretically possible that it will return NULL. isc_throw(isc::BadValue, "undefined result of " - "processCommand(\"config-reload\", dhcp4)"); + "processCommand(\"set-config\", json)"); } // Now check is the returned result is successful (rcode=0) or not @@ -100,17 +83,6 @@ void configure(const std::string& file_name) { "no details available"; isc_throw(isc::BadValue, reason); } - - // If configuration was parsed successfully, apply the new logger - // configuration to log4cplus. It is done before commit in case - // something goes wrong. - CfgMgr::instance().getStagingCfg()->applyLoggingCfg(); - - // Use new configuration. - /// @todo: This commit should be moved to - /// CtrlDhcp4Srv::commandConfigReloadHandler. - CfgMgr::instance().commit(); - } catch (const std::exception& ex) { // If configuration failed at any stage, we drop the staging // configuration and continue to use the previous one. diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc index 77bcb09035..1338a91c11 100644 --- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -121,6 +122,13 @@ public: ConstElementPtr answer = server_->processConfig(config); ASSERT_TRUE(answer); +#if 0 + // If the configuration doesn't contain logging config, processConfig() + // will revert the logging to default (stdout). We call initLogger() + // to restore unit test logging. + isc::log::initLogger(); +#endif + int status = 0; ConstElementPtr txt = isc::config::parseAnswer(status, answer); // This should succeed. If not, print the error message. diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index 7800554ce1..3126447cfb 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -24,6 +24,7 @@ #include #include #include +#include #include using namespace std; @@ -47,6 +48,9 @@ BaseServerTest::~BaseServerTest() { // Revert to original data directory. CfgMgr::instance().setDataDir(original_datadir_); + + // Revert to unit test logging, in case the test reconfigured it. + isc::log::initLogger(); } Dhcpv4SrvTest::Dhcpv4SrvTest() diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index b190c55dd0..2eed5762bd 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -122,6 +123,8 @@ public: // Create fixed server id. server_id_.reset(new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER, asiolink::IOAddress("192.0.3.1"))); + // Revert to unit test logging + isc::log::initLogger(); } /// @brief Returns fixed server identifier assigned to the naked server @@ -167,6 +170,8 @@ public: } virtual ~NakedDhcpv4Srv() { + // Revert to unit test logging + isc::log::initLogger(); } /// @brief Dummy server identifier option used by various tests. From 6ea58aa99aa6756d259f58100d72f3221a8c6746 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 9 Dec 2016 16:15:25 -0500 Subject: [PATCH 05/13] [5046] Minor cleanups --- src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc | 7 ------- src/bin/dhcp4/tests/dhcp4_test_utils.h | 2 -- 2 files changed, 9 deletions(-) diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc index 1338a91c11..779b1d5d79 100644 --- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc @@ -122,13 +122,6 @@ public: ConstElementPtr answer = server_->processConfig(config); ASSERT_TRUE(answer); -#if 0 - // If the configuration doesn't contain logging config, processConfig() - // will revert the logging to default (stdout). We call initLogger() - // to restore unit test logging. - isc::log::initLogger(); -#endif - int status = 0; ConstElementPtr txt = isc::config::parseAnswer(status, answer); // This should succeed. If not, print the error message. diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index 2eed5762bd..98520cba16 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h @@ -123,8 +123,6 @@ public: // Create fixed server id. server_id_.reset(new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER, asiolink::IOAddress("192.0.3.1"))); - // Revert to unit test logging - isc::log::initLogger(); } /// @brief Returns fixed server identifier assigned to the naked server From 354e68adecaa81b1ab8811ce64f1afe18bd6fe41 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 15 Dec 2016 16:15:26 -0500 Subject: [PATCH 06/13] [5046] Move apply logging and config commit from processConfig to set-config handler src/bin/dhcp4/ctrl_dhcp4_srv.cc commandConfigReloadHandler() - use commandSetConfigHandler() instead of processConfig() to account for logging config commandSetConfigHandler() - apply logging config and commit config here instead of in processConfig() src/bin/dhcp4/tests/dhcp4_test_utils.h ~NakedDhcpv4Srv() - removed unecesary initLogger call src/bin/dhcp4/tests/kea_controller_unittest.cc ~JSONFileBackendTest() - removed unecessary call to setDefaultLogging src/bin/dhcp6/ctrl_dhcp6_srv.cc commandConfigReloadHandler() - use commandSetConfigHandler() instead of processConfig() to account for logging config commandSetConfigHandler() - apply logging config and commit config here instead of in processConfig() src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc createUnixChannelServer() - added config commit so command channel behavior is correct TEST_F(CtrlDhcpv6SrvTest, configReload) Wrap configuration in Dhcp6 element TEST_F(CtrlChannelDhcpv6SrvTest, set_config) Turn off timers in config src/lib/dhcpsrv/srv_config.cc SrvConfig::applyLoggingCfg() - remove logic added to not call manager.process when there are no specs. --- src/bin/dhcp4/ctrl_dhcp4_srv.cc | 45 +++++++++++-------- src/bin/dhcp4/kea_controller.cc | 2 +- src/bin/dhcp4/tests/dhcp4_test_utils.h | 2 - .../dhcp4/tests/kea_controller_unittest.cc | 1 - src/bin/dhcp6/ctrl_dhcp6_srv.cc | 45 +++++++++++-------- .../dhcp6/tests/ctrl_dhcp6_srv_unittest.cc | 22 ++++++--- src/lib/dhcpsrv/srv_config.cc | 9 +--- 7 files changed, 73 insertions(+), 53 deletions(-) diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 3ac909d914..05879f7235 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -62,7 +62,8 @@ ControlledDhcpv4Srv::commandLibReloadHandler(const string&, ConstElementPtr) { ConstElementPtr ControlledDhcpv4Srv::commandConfigReloadHandler(const string&, ConstElementPtr args) { - return (processConfig(args)); + // Use set-config as it handles logging and server config + return (commandSetConfigHandler("", args)); } ConstElementPtr @@ -72,14 +73,10 @@ ControlledDhcpv4Srv::commandSetConfigHandler(const string&, ConstElementPtr dhcp4; string message; - // We are starting the configuration process so we should remove any - // staging configuration that has been created during previous - // configuration attempts. - CfgMgr::instance().rollback(); - // Command arguments are expected to be: // { "Dhcp4": { ... }, "Logging": { ... } } - // The Logging component is technically optional, but very recommended. + // The Logging component is technically optional. If it's not supplied + // logging will revert to default logging. if (!args) { message = "Missing mandatory 'arguments' parameter."; } else { @@ -98,16 +95,36 @@ ControlledDhcpv4Srv::commandSetConfigHandler(const string&, return (result); } + // We are starting the configuration process so we should remove any + // staging configuration that has been created during previous + // configuration attempts. + CfgMgr::instance().rollback(); + // Logging is a sibling element and must be be parsed explicitly. // The call to configureLogger parses the given Logging element if - // not null, into the staging config. Note this DOES alter the + // not null, into the staging config. Note this does not alter the // current loggers, they remain in effect until we apply the - // logging config. + // logging config below. If no logging is supplied logging will + // revert to default logging. Daemon::configureLogger(args->get("Logging"), CfgMgr::instance().getStagingCfg()); // Now we configure the server proper. - return (processConfig(dhcp4)); + ConstElementPtr result = processConfig(dhcp4); + + // If the configuration parsed successfully, apply the new logger + // configuration and the commit the new configuration. We apply + // the logging first in case there's a configuration failure. + int rcode = 0; + isc::config::parseAnswer(rcode, result); + if (rcode == 0) { + CfgMgr::instance().getStagingCfg()->applyLoggingCfg(); + + // Use new configuration. + CfgMgr::instance().commit(); + } + + return (result); } ConstElementPtr @@ -283,14 +300,6 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) { } } - // Configuration was parsed successfully, apply the new logger - // configuration to log4cplus. It is done before commit in case - // something goes wrong. - CfgMgr::instance().getStagingCfg()->applyLoggingCfg(); - - // Use new configuration. - CfgMgr::instance().commit(); - return (answer); } diff --git a/src/bin/dhcp4/kea_controller.cc b/src/bin/dhcp4/kea_controller.cc index d79d9d037f..ed74214045 100644 --- a/src/bin/dhcp4/kea_controller.cc +++ b/src/bin/dhcp4/kea_controller.cc @@ -137,7 +137,7 @@ ControlledDhcpv4Srv::init(const std::string& file_name) { // We don't need to call openActiveSockets() or startD2() as these // methods are called in processConfig() which is called by - // processCommand("reload-config", ...) + // processCommand("set-config", ...) // Set signal handlers. When the SIGHUP is received by the process // the server reconfiguration will be triggered. When SIGTERM or diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index 98520cba16..e438a58172 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h @@ -168,8 +168,6 @@ public: } virtual ~NakedDhcpv4Srv() { - // Revert to unit test logging - isc::log::initLogger(); } /// @brief Dummy server identifier option used by various tests. diff --git a/src/bin/dhcp4/tests/kea_controller_unittest.cc b/src/bin/dhcp4/tests/kea_controller_unittest.cc index c42a022c88..78d2c5651c 100644 --- a/src/bin/dhcp4/tests/kea_controller_unittest.cc +++ b/src/bin/dhcp4/tests/kea_controller_unittest.cc @@ -62,7 +62,6 @@ public: ~JSONFileBackendTest() { LeaseMgrFactory::destroy(); - isc::log::setDefaultLoggingOutput(); static_cast(remove(TEST_FILE)); }; diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 7bbf673349..c9ff04d85a 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -67,7 +67,8 @@ ControlledDhcpv6Srv::commandLibReloadHandler(const string&, ConstElementPtr) { ConstElementPtr ControlledDhcpv6Srv::commandConfigReloadHandler(const string&, ConstElementPtr args) { - return (processConfig(args)); + // Use set-config as it handles logging and server config + return (commandSetConfigHandler("", args)); } ConstElementPtr @@ -77,14 +78,10 @@ ControlledDhcpv6Srv::commandSetConfigHandler(const string&, ConstElementPtr dhcp6; string message; - // We are starting the configuration process so we should remove any - // staging configuration that has been created during previous - // configuration attempts. - CfgMgr::instance().rollback(); - // Command arguments are expected to be: // { "Dhcp6": { ... }, "Logging": { ... } } - // The Logging component is technically optional, but very recommended. + // The Logging component is technically optional. If it's not supplied + // logging will revert to default logging. if (!args) { message = "Missing mandatory 'arguments' parameter."; } else { @@ -103,16 +100,36 @@ ControlledDhcpv6Srv::commandSetConfigHandler(const string&, return (result); } + // We are starting the configuration process so we should remove any + // staging configuration that has been created during previous + // configuration attempts. + CfgMgr::instance().rollback(); + // Logging is a sibling element and must be be parsed explicitly. // The call to configureLogger parses the given Logging element if - // not null, into the staging config. Note this DOES alter the + // not null, into the staging config. Note this does not alter the // current loggers, they remain in effect until we apply the - // logging config. + // logging config below. If no logging is supplied logging will + // revert to default logging. Daemon::configureLogger(args->get("Logging"), CfgMgr::instance().getStagingCfg()); // Now we configure the server proper. - return (processConfig(dhcp6)); + ConstElementPtr result = processConfig(dhcp6); + + // If the configuration parsed successfully, apply the new logger + // configuration and the commit the new configuration. We apply + // the logging first in case there's a configuration failure. + int rcode = 0; + isc::config::parseAnswer(rcode, result); + if (rcode == 0) { + CfgMgr::instance().getStagingCfg()->applyLoggingCfg(); + + // Use new configuration. + CfgMgr::instance().commit(); + } + + return (result); } @@ -308,14 +325,6 @@ ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) { } } - // Configuration was parsed successfully, apply the new logger - // configuration to log4cplus. It is done before commit in case - // something goes wrong. - CfgMgr::instance().getStagingCfg()->applyLoggingCfg(); - - // Use new configuration. - CfgMgr::instance().commit(); - // Finally, we can commit runtime option definitions in libdhcp++. This is // exception free. LibDHCP::commitRuntimeOptionDefs(); diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc index 98c88b44d8..7c9fbe97d4 100644 --- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc @@ -149,6 +149,12 @@ public: ConstElementPtr config = Element::fromJSON(config_txt); ConstElementPtr answer = server_->processConfig(config); + + // Commit the configuration so any subsequent reconfigurations + // will only close the command channel if its configuration has + // changed. + CfgMgr::instance().commit(); + ASSERT_TRUE(answer); int status = 0; @@ -302,7 +308,7 @@ TEST_F(CtrlDhcpv6SrvTest, configReload) { // Use empty parameters list // Prepare configuration file. - string config_txt = "{ \"interfaces-config\": {" + string config_txt = "{ \"Dhcp6\": { \"interfaces-config\": {" " \"interfaces\": [ \"*\" ]" "}," "\"preferred-lifetime\": 3000," @@ -321,7 +327,7 @@ TEST_F(CtrlDhcpv6SrvTest, configReload) { " \"pools\": [ { \"pool\": \"2001:db8:3::/80\" } ]," " \"subnet\": \"2001:db8:3::/64\" " " } ]," - "\"valid-lifetime\": 4000 }"; + "\"valid-lifetime\": 4000 }}"; ElementPtr config = Element::fromJSON(config_txt); @@ -361,6 +367,11 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { " \"valid-lifetime\": 4000, \n" " \"renew-timer\": 1000, \n" " \"rebind-timer\": 2000, \n" + " \"expired-leases-processing\": { \n" + " \"reclaim-timer-wait-time\": 0, \n" + " \"hold-reclaimed-time\": 0, \n" + " \"flush-reclaimed-timer-wait-time\": 0 \n" + " }," " \"subnet6\": [ \n"; string subnet1 = " {\"subnet\": \"3002::/64\", \n" @@ -382,9 +393,8 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { string logger_txt = " \"Logging\": { \n" " \"loggers\": [ { \n" - " \"name\": \"*\", \n" - " \"severity\": \"INFO\", \n" - " \"debuglevel\": 0, \n" + " \"name\": \"kea\", \n" + " \"severity\": \"FATAL\", \n" " \"output_options\": [{ \n" " \"output\": \"/dev/null\" \n" " }] \n" @@ -438,7 +448,7 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { // Should fail with a syntax error EXPECT_EQ("{ \"result\": 1, " - "\"text\": \"unsupported parameter: BOGUS (:12:26)\" }", + "\"text\": \"unsupported parameter: BOGUS (:16:26)\" }", response); // Check that the config was not lost diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 017d83767e..37cac2f89d 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -118,13 +118,8 @@ SrvConfig::applyLoggingCfg() const { specs.push_back(it->toSpec()); } - // If there are no specs, then leave logging alone. This will leave - // the existing logging setup intact. Calling process() with no - // specs will reset the logging heirarchy and so forth. - if (specs.size()) { - LoggerManager manager; - manager.process(specs.begin(), specs.end()); - } + LoggerManager manager; + manager.process(specs.begin(), specs.end()); } bool From d9a652987126449a2e5c08a8a01341e9e34b16b5 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 16 Dec 2016 11:02:04 -0500 Subject: [PATCH 07/13] [5046] Added set-config command handler test to dhcp4 unit tests --- .../dhcp4/tests/ctrl_dhcp4_srv_unittest.cc | 138 ++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc index 779b1d5d79..7e0b6acb54 100644 --- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc @@ -120,6 +120,13 @@ public: ConstElementPtr config = Element::fromJSON(config_txt); ConstElementPtr answer = server_->processConfig(config); + + // Commit the configuration so any subsequent reconfigurations + // will only close the command channel if its configuration has + // changed. + CfgMgr::instance().commit(); + + ASSERT_TRUE(answer); int status = 0; @@ -480,4 +487,135 @@ TEST_F(CtrlChannelDhcpv4SrvTest, controlChannelStats) { response); } +// Check that the "set-config" command will replace current configuration +TEST_F(CtrlChannelDhcpv4SrvTest, set_config) { + createUnixChannelServer(); + + // Define strings to permutate the config arguments + // (Note the line feeds makes errors easy to find) + string set_config_txt = "{ \"command\": \"set-config\" \n"; + string args_txt = " \"arguments\": { \n"; + string dhcp4_cfg_txt = + " \"Dhcp4\": { \n" + " \"interfaces-config\": { \n" + " \"interfaces\": [\"*\"] \n" + " }, \n" + " \"valid-lifetime\": 4000, \n" + " \"renew-timer\": 1000, \n" + " \"rebind-timer\": 2000, \n" + " \"expired-leases-processing\": { \n" + " \"reclaim-timer-wait-time\": 0, \n" + " \"hold-reclaimed-time\": 0, \n" + " \"flush-reclaimed-timer-wait-time\": 0 \n" + " }," + " \"subnet4\": [ \n"; + string subnet1 = + " {\"subnet\": \"192.2.0.0/24\", \n" + " \"pools\": [{ \"pool\": \"192.2.0.1-192.2.0.50\" }]}\n"; + string subnet2 = + " {\"subnet\": \"192.2.1.0/24\", \n" + " \"pools\": [{ \"pool\": \"192.2.1.1-192.2.1.50\" }]}\n"; + string bad_subnet = + " {\"BOGUS\": \"192.2.2.0/24\", \n" + " \"pools\": [{ \"pool\": \"192.2.2.1-192.2.2.50\" }]}\n"; + string subnet_footer = + " ] \n"; + string control_socket_header = + " ,\"control-socket\": { \n" + " \"socket-type\": \"unix\", \n" + " \"socket-name\": \""; + string control_socket_footer = + "\" \n} \n"; + string logger_txt = + " \"Logging\": { \n" + " \"loggers\": [ { \n" + " \"name\": \"kea\", \n" + " \"severity\": \"FATAL\", \n" + " \"output_options\": [{ \n" + " \"output\": \"/dev/null\" \n" + " }] \n" + " }] \n" + " } \n"; + + std::ostringstream os; + + // Create a valid config with all the parts should parse + os << set_config_txt << "," + << args_txt + << dhcp4_cfg_txt + << subnet1 + << subnet_footer + << control_socket_header + << socket_path_ + << control_socket_footer + << "}\n" // close dhcp4 + << "," + << logger_txt + << "}}"; + + // Send the set-config command + std::string response; + sendUnixCommand(os.str(), response); + + // Verify the configuration was successful. + EXPECT_EQ("{ \"result\": 0, \"text\": \"Configuration successful.\" }", + response); + + // Check that the config was indeed applied. + const Subnet4Collection* subnets = + CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getAll(); + EXPECT_EQ(1, subnets->size()); + + // Create a config with malformed subnet that should fail to parse. + os.str(""); + os << set_config_txt << "," + << args_txt + << dhcp4_cfg_txt + << bad_subnet + << subnet_footer + << control_socket_header + << socket_path_ + << control_socket_footer + << "}\n" // close dhcp4 + "}}"; + + // Send the set-config command + sendUnixCommand(os.str(), response); + + // Should fail with a syntax error + EXPECT_EQ("{ \"result\": 1, " + "\"text\": \"unsupported parameter: BOGUS (:15:26)\" }", + response); + + // Check that the config was not lost + subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getAll(); + EXPECT_EQ(1, subnets->size()); + + // Create a valid config with two subnets and no command channel. + // It should succeed but client will not receive a the response + os.str(""); + os << set_config_txt << "," + << args_txt + << dhcp4_cfg_txt + << subnet1 + << ",\n" + << subnet2 + << subnet_footer + << "}\n" // close dhcp4 + << "}}"; + + // Send the set-config command + sendUnixCommand(os.str(), response); + + // With no command channel, no response + EXPECT_EQ("", response); + + // Check that the config was not lost + subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getAll(); + EXPECT_EQ(2, subnets->size()); + + // Clean up after the test. + CfgMgr::instance().clear(); +} + } // End of anonymous namespace From 0bfc63411dc929b07084e216739d482030363500 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 21 Dec 2016 15:51:24 -0500 Subject: [PATCH 08/13] [5046] Undo a couple of minor nits --- src/bin/dhcp4/tests/dhcp4_test_utils.h | 1 - src/lib/dhcpsrv/srv_config.cc | 1 - 2 files changed, 2 deletions(-) diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index e438a58172..b190c55dd0 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 37cac2f89d..7c7830966f 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -117,7 +117,6 @@ SrvConfig::applyLoggingCfg() const { it != logging_info_.end(); ++it) { specs.push_back(it->toSpec()); } - LoggerManager manager; manager.process(specs.begin(), specs.end()); } From 7289e65468262982cf3c1056b1678bb164d366fb Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 22 Dec 2016 09:24:35 -0500 Subject: [PATCH 09/13] [5046] Added documentation to the admin guide for set-config --- doc/guide/ctrl-channel.xml | 61 ++++++++++++++++++++++++++++++++++++++ doc/guide/dhcp4-srv.xml | 31 +++++++++++++------ doc/guide/dhcp6-srv.xml | 29 ++++++++++++------ 3 files changed, 103 insertions(+), 18 deletions(-) diff --git a/doc/guide/ctrl-channel.xml b/doc/guide/ctrl-channel.xml index 8db374be5f..e92340584f 100644 --- a/doc/guide/ctrl-channel.xml +++ b/doc/guide/ctrl-channel.xml @@ -161,6 +161,65 @@ will be sent to Kea and the responses received from Kea printed to standard outp +
+ set-config + + + The set-config command instructs the server to replace + its current configuration with the new configuration supplied in the + command's arguments. The supplied configuration is expected to be the full + configuration for the target server along with an optional Logger + configuration. While optional, the Logger configuration is highly + recommended as without it the server will revert to its default logging + configuration. The structure of the command is as follows: + + +{ + "command": "set-config", + "arguments": { + "<server>": { + }, + "Logging": { + } + } +} + + + where <server> is the configuration element name for a given server + such as "Dhcp4" or "Dhcp6". For example: + + +{ + "command": "set-config", + "arguments": { + "Dhcp6": { + : + }, + "Logging": { + : + } + } +} + + + If the new configuration proves to be invalid the server will retain + its current configuration. Please note that the new configuration is + retained in memory only. If the server is restarted or a configuration + reload is triggered via a signal, the server will use the configuration + stored in its configuration file. + + The server's response will contain a numeric code, "result" (0 for success, + non-zero on failure), and a string, "text", describing the outcome: + + {"result": 0, "text": "Configuration successful." } + + or + + {"result": 1, "text": "unsupported parameter: BOGUS (<string>:16:26)" } + + +
+
shutdown @@ -182,6 +241,8 @@ will be sent to Kea and the responses received from Kea printed to standard outp
+ + diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index 4c7c3efa4c..9771f3452f 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -3659,17 +3659,30 @@ src/lib/dhcpsrv/cfg_host_operations.cc --> Communication over control channel is conducted using JSON structures. - See the Control Channel section in the Kea Developer's Guide for more details. + See the Control Channel section in the Kea Developer's Guide for more + details. + + + The DHCPv4 server supports the following operational commands: + + leases-reclaim + list-commands + set-config + shutdown + + as described in . In addition, + it supports the following statistics related commands: + + statistic-get + statistic-reset + statistic-remove + statistic-get-all + statistic-reset-all + statistic-remove-all + + as described here . - The DHCPv4 server supports statistic-get, - statistic-reset, statistic-remove, - statistic-get-all, statistic-reset-all - and statistic-remove-all, specified in - . It also supports - list-commands and shutdown, - specified in and - , respectively.
diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index 8fb04a40d1..3a509f68bb 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -4067,16 +4067,27 @@ If not specified, the default value is: See the Control Channel section in the Kea Developer's Guide for more details. - The DHCPv6 server supports statistic-get, - statistic-reset, statistic-remove, - statistic-get-all, statistic-reset-all - and statistic-remove-all, specified in - . It also supports - list-commands and shutdown, - specified in and - , respectively. -
+ The DHCPv6 server supports the following operational commands: + + leases-reclaim + list-commands + set-config + shutdown + + as described in . In addition, + it supports the following statistics related commands: + + statistic-get + statistic-reset + statistic-remove + statistic-get-all + statistic-reset-all + statistic-remove-all + + as described here . + +
User context in IPv6 pools From e5a6c2b4f824928168fd48f5e1960497425a1aca Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 22 Dec 2016 13:38:00 -0500 Subject: [PATCH 10/13] [5046] CommandMgr dups the connection socket prior to executing command src/lib/config/command_mgr.cc CommandMgr::commandReader(int sockfd) - duplicates the connection socket to use for repsonding in case the command closes the channel. src/lib/testutils/io_utils.cc fileExists() - now uses stat() function so one can use it on any type of file, like a unix socket updated unit tests accordingly --- .../dhcp4/tests/ctrl_dhcp4_srv_unittest.cc | 14 +++++++-- .../dhcp6/tests/ctrl_dhcp6_srv_unittest.cc | 13 ++++++-- src/lib/config/command_mgr.cc | 30 +++++++++++++++++-- src/lib/config/config_messages.mes | 9 +++++- src/lib/testutils/io_utils.cc | 6 ++-- src/lib/testutils/io_utils.h | 1 + 6 files changed, 61 insertions(+), 12 deletions(-) diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc index 7e0b6acb54..47de13d40e 100644 --- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include "marker_file.h" @@ -592,7 +593,7 @@ TEST_F(CtrlChannelDhcpv4SrvTest, set_config) { EXPECT_EQ(1, subnets->size()); // Create a valid config with two subnets and no command channel. - // It should succeed but client will not receive a the response + // It should succeed, client should still receive the response os.str(""); os << set_config_txt << "," << args_txt @@ -604,11 +605,18 @@ TEST_F(CtrlChannelDhcpv4SrvTest, set_config) { << "}\n" // close dhcp4 << "}}"; + /* Verify the control channel socket exists */ + ASSERT_TRUE(fileExists(socket_path_)); + // Send the set-config command sendUnixCommand(os.str(), response); - // With no command channel, no response - EXPECT_EQ("", response); + /* Verify the control channel socket no longer exists */ + EXPECT_FALSE(fileExists(socket_path_)); + + // With no command channel, should still receive the response. + EXPECT_EQ("{ \"result\": 0, \"text\": \"Configuration successful.\" }", + response); // Check that the config was not lost subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getAll(); diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc index 7c9fbe97d4..3f1331374c 100644 --- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include "marker_file.h" #include "test_libraries.h" @@ -26,6 +27,7 @@ #include #include +#include #include #include @@ -468,11 +470,18 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { << "}\n" // close dhcp6 << "}}"; + /* Verify the control channel socket exists */ + ASSERT_TRUE(fileExists(socket_path_)); + // Send the set-config command sendUnixCommand(os.str(), response); - // With no command channel, no response - EXPECT_EQ("", response); + /* Verify the control channel socket no longer exists */ + EXPECT_FALSE(fileExists(socket_path_)); + + // With no command channel, should still receive the response. + EXPECT_EQ("{ \"result\": 0, \"text\": \"Configuration successful.\" }", + response); // Check that the config was not lost subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets6()->getAll(); diff --git a/src/lib/config/command_mgr.cc b/src/lib/config/command_mgr.cc index ae33188014..c3e3b43fe6 100644 --- a/src/lib/config/command_mgr.cc +++ b/src/lib/config/command_mgr.cc @@ -11,6 +11,7 @@ #include #include #include +#include using namespace isc::data; @@ -145,6 +146,19 @@ CommandMgr::commandReader(int sockfd) { return; } + // Duplicate the connection's socket in the event, the command causes the + // channel to close (like a reconfig). This permits us to always have + // a socket on which to respond. If for some reason we can't fall back + // to the connection socket. + int rsp_fd = dup(sockfd); + if (rsp_fd < 0 ) { + // Highly unlikely + const char* errmsg = strerror(errno); + LOG_DEBUG(command_logger, DBG_COMMAND, COMMAND_SOCKET_DUP_WARN) + .arg(errmsg); + rsp_fd = sockfd; + } + LOG_DEBUG(command_logger, DBG_COMMAND, COMMAND_SOCKET_READ).arg(rval).arg(sockfd); // Ok, we received something. Let's see if we can make any sense of it. @@ -163,6 +177,11 @@ CommandMgr::commandReader(int sockfd) { if (!rsp) { LOG_WARN(command_logger, COMMAND_RESPONSE_ERROR); + // Only close the dupped socket if it's different (should be) + if (rsp_fd != sockfd) { + close(rsp_fd); + } + return; } @@ -179,7 +198,8 @@ CommandMgr::commandReader(int sockfd) { } // Send the data back over socket. - rval = write(sockfd, txt.c_str(), len); + rval = write(rsp_fd, txt.c_str(), len); + int saverr = errno; LOG_DEBUG(command_logger, DBG_COMMAND, COMMAND_SOCKET_WRITE).arg(len).arg(sockfd); @@ -187,7 +207,13 @@ CommandMgr::commandReader(int sockfd) { // Response transmission failed. Since the response failed, it doesn't // make sense to send any status codes. Let's log it and be done with // it. - LOG_ERROR(command_logger, COMMAND_SOCKET_WRITE_FAIL).arg(len).arg(sockfd); + LOG_ERROR(command_logger, COMMAND_SOCKET_WRITE_FAIL) + .arg(len).arg(sockfd).arg(strerror(saverr)); + } + + // Only close the dupped socket if it's different (should be) + if (rsp_fd != sockfd) { + close(rsp_fd); } } diff --git a/src/lib/config/config_messages.mes b/src/lib/config/config_messages.mes index 2a4558fd85..ef7c092881 100644 --- a/src/lib/config/config_messages.mes +++ b/src/lib/config/config_messages.mes @@ -57,6 +57,13 @@ This error message indicates that the server failed to set non-blocking mode on just created socket. That socket was created for accepting specific incoming connection. Additional information may be provided as third parameter. +% COMMAND_SOCKET_DUP_WARN Failed to duplicate socket for response: %1 +This debug message indicates that the commandReader was unable to duplicate +the connection socket prior to executing the command. This is most likely a +system resource issue. The command should still be processed and the response +sent, unless the command caused the command channel to be closed (e.g. a +reconfiguration command). + % COMMAND_SOCKET_READ Received %1 bytes over command socket %2 This debug message indicates that specified number of bytes was received over command socket identified by specified file descriptor. @@ -86,6 +93,6 @@ descriptor and path specified. This debug message indicates that the specified number of bytes was sent over command socket identifier by the specified file descriptor. -% COMMAND_SOCKET_WRITE_FAIL Error while writing %1 bytes to command socket %2 +% COMMAND_SOCKET_WRITE_FAIL Error while writing %1 bytes to command socket %2 : %3 This error message indicates that an error was encountered while attempting to send a response to the command socket. diff --git a/src/lib/testutils/io_utils.cc b/src/lib/testutils/io_utils.cc index 54aff4cb6f..caf9437071 100644 --- a/src/lib/testutils/io_utils.cc +++ b/src/lib/testutils/io_utils.cc @@ -15,10 +15,8 @@ namespace dhcp { namespace test { bool fileExists(const std::string& file_path) { - std::ifstream fs(file_path.c_str()); - const bool file_exists = fs.good(); - fs.close(); - return (file_exists); + struct stat statbuf; + return(stat(file_path.c_str(), &statbuf) == 0); } std::string readFile(const std::string& file_path) { diff --git a/src/lib/testutils/io_utils.h b/src/lib/testutils/io_utils.h index 8cf1ef6e14..61a18f8c79 100644 --- a/src/lib/testutils/io_utils.h +++ b/src/lib/testutils/io_utils.h @@ -8,6 +8,7 @@ #define TEST_IO_UTILS_H #include +#include namespace isc { namespace dhcp { From b21784dc799a2ae5f3180d9f935714193f36ccdf Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Thu, 22 Dec 2016 23:28:50 +0100 Subject: [PATCH 11/13] [5046] Removed spurious spaces --- doc/guide/dhcp4-srv.xml | 2 +- doc/guide/dhcp6-srv.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index 9771f3452f..dc7034e834 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -3674,7 +3674,7 @@ src/lib/dhcpsrv/cfg_host_operations.cc --> it supports the following statistics related commands: statistic-get - statistic-reset + statistic-reset statistic-remove statistic-get-all statistic-reset-all diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index 3a509f68bb..67d285bdfc 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -4078,7 +4078,7 @@ If not specified, the default value is: it supports the following statistics related commands: statistic-get - statistic-reset + statistic-reset statistic-remove statistic-get-all statistic-reset-all From f3f1c5230fd57bc0319196f8292362efcad7203b Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Fri, 23 Dec 2016 00:55:48 +0100 Subject: [PATCH 12/13] [5046] Set set-config command and fixed "be be" typo --- src/bin/dhcp4/ctrl_dhcp4_srv.cc | 4 ++-- src/bin/dhcp6/ctrl_dhcp6_srv.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 05879f7235..6f2cfc1391 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -63,7 +63,7 @@ ConstElementPtr ControlledDhcpv4Srv::commandConfigReloadHandler(const string&, ConstElementPtr args) { // Use set-config as it handles logging and server config - return (commandSetConfigHandler("", args)); + return (commandSetConfigHandler("set-config", args)); } ConstElementPtr @@ -100,7 +100,7 @@ ControlledDhcpv4Srv::commandSetConfigHandler(const string&, // configuration attempts. CfgMgr::instance().rollback(); - // Logging is a sibling element and must be be parsed explicitly. + // Logging is a sibling element and must be parsed explicitly. // The call to configureLogger parses the given Logging element if // not null, into the staging config. Note this does not alter the // current loggers, they remain in effect until we apply the diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index c9ff04d85a..5b03783e47 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -68,7 +68,7 @@ ControlledDhcpv6Srv::commandLibReloadHandler(const string&, ConstElementPtr) { ConstElementPtr ControlledDhcpv6Srv::commandConfigReloadHandler(const string&, ConstElementPtr args) { // Use set-config as it handles logging and server config - return (commandSetConfigHandler("", args)); + return (commandSetConfigHandler("set-config", args)); } ConstElementPtr @@ -105,7 +105,7 @@ ControlledDhcpv6Srv::commandSetConfigHandler(const string&, // configuration attempts. CfgMgr::instance().rollback(); - // Logging is a sibling element and must be be parsed explicitly. + // Logging is a sibling element and must be parsed explicitly. // The call to configureLogger parses the given Logging element if // not null, into the staging config. Note this does not alter the // current loggers, they remain in effect until we apply the From ebb863e37217bcea92f1818302bb6e5ea29c1673 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Tue, 3 Jan 2017 08:23:44 -0500 Subject: [PATCH 13/13] [5046] Addressed review comments Added memfile without persistence to set_config unit tests. This eliminates the tests attempting to create the csv file. Updated copyrights. Fixed misspelling. --- src/bin/dhcp4/ctrl_dhcp4_srv.cc | 2 +- src/bin/dhcp4/ctrl_dhcp4_srv.h | 2 +- src/bin/dhcp4/json_config_parser.cc | 2 +- src/bin/dhcp4/kea_controller.cc | 2 +- src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc | 9 +++++++-- src/bin/dhcp4/tests/dhcp4_test_utils.cc | 2 +- src/bin/dhcp4/tests/kea_controller_unittest.cc | 2 +- src/bin/dhcp6/ctrl_dhcp6_srv.cc | 2 +- src/bin/dhcp6/ctrl_dhcp6_srv.h | 2 +- src/bin/dhcp6/json_config_parser.cc | 2 +- src/bin/dhcp6/kea_controller.cc | 2 +- src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc | 11 ++++++++--- src/bin/dhcp6/tests/dhcp6_test_utils.cc | 2 +- src/lib/config/command_mgr.cc | 6 +++--- src/lib/config/config_messages.mes | 2 +- src/lib/testutils/io_utils.cc | 2 +- src/lib/testutils/io_utils.h | 2 +- 17 files changed, 32 insertions(+), 22 deletions(-) diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 05879f7235..c75c532efb 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.h b/src/bin/dhcp4/ctrl_dhcp4_srv.h index 6ef362cd41..2d96b34427 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.h +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.h @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index 4b50286056..7e29954dcd 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/dhcp4/kea_controller.cc b/src/bin/dhcp4/kea_controller.cc index ed74214045..1c3e783034 100644 --- a/src/bin/dhcp4/kea_controller.cc +++ b/src/bin/dhcp4/kea_controller.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc index 47de13d40e..615274e5f5 100644 --- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -504,6 +504,11 @@ TEST_F(CtrlChannelDhcpv4SrvTest, set_config) { " \"valid-lifetime\": 4000, \n" " \"renew-timer\": 1000, \n" " \"rebind-timer\": 2000, \n" + " \"lease-database\": { \n" + " \"type\": \"memfile\", \n" + " \"persist\":false, \n" + " \"lfc-interval\": 0 \n" + " }, \n" " \"expired-leases-processing\": { \n" " \"reclaim-timer-wait-time\": 0, \n" " \"hold-reclaimed-time\": 0, \n" @@ -585,7 +590,7 @@ TEST_F(CtrlChannelDhcpv4SrvTest, set_config) { // Should fail with a syntax error EXPECT_EQ("{ \"result\": 1, " - "\"text\": \"unsupported parameter: BOGUS (:15:26)\" }", + "\"text\": \"unsupported parameter: BOGUS (:20:26)\" }", response); // Check that the config was not lost diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index 3126447cfb..4da164d8c5 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/dhcp4/tests/kea_controller_unittest.cc b/src/bin/dhcp4/tests/kea_controller_unittest.cc index 78d2c5651c..ccdd355c1a 100644 --- a/src/bin/dhcp4/tests/kea_controller_unittest.cc +++ b/src/bin/dhcp4/tests/kea_controller_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index c9ff04d85a..642a831608 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.h b/src/bin/dhcp6/ctrl_dhcp6_srv.h index 1c6bde1664..23abb69632 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.h +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.h @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index 648ffa9e27..571e4662c9 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/dhcp6/kea_controller.cc b/src/bin/dhcp6/kea_controller.cc index 8ee2b64b47..31402c10fb 100644 --- a/src/bin/dhcp6/kea_controller.cc +++ b/src/bin/dhcp6/kea_controller.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc index 3f1331374c..12647f96ee 100644 --- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -369,7 +369,12 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { " \"valid-lifetime\": 4000, \n" " \"renew-timer\": 1000, \n" " \"rebind-timer\": 2000, \n" - " \"expired-leases-processing\": { \n" + " \"lease-database\": { \n" + " \"type\": \"memfile\", \n" + " \"persist\":false, \n" + " \"lfc-interval\": 0 \n" + " }, \n" + " \"expired-leases-processing\": { \n" " \"reclaim-timer-wait-time\": 0, \n" " \"hold-reclaimed-time\": 0, \n" " \"flush-reclaimed-timer-wait-time\": 0 \n" @@ -450,7 +455,7 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) { // Should fail with a syntax error EXPECT_EQ("{ \"result\": 1, " - "\"text\": \"unsupported parameter: BOGUS (:16:26)\" }", + "\"text\": \"unsupported parameter: BOGUS (:21:26)\" }", response); // Check that the config was not lost diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.cc b/src/bin/dhcp6/tests/dhcp6_test_utils.cc index a1c74dab10..3ae1478a31 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.cc +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/lib/config/command_mgr.cc b/src/lib/config/command_mgr.cc index c3e3b43fe6..cf9dfb7306 100644 --- a/src/lib/config/command_mgr.cc +++ b/src/lib/config/command_mgr.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -177,7 +177,7 @@ CommandMgr::commandReader(int sockfd) { if (!rsp) { LOG_WARN(command_logger, COMMAND_RESPONSE_ERROR); - // Only close the dupped socket if it's different (should be) + // Only close the duped socket if it's different (should be) if (rsp_fd != sockfd) { close(rsp_fd); } @@ -211,7 +211,7 @@ CommandMgr::commandReader(int sockfd) { .arg(len).arg(sockfd).arg(strerror(saverr)); } - // Only close the dupped socket if it's different (should be) + // Only close the duped socket if it's different (should be) if (rsp_fd != sockfd) { close(rsp_fd); } diff --git a/src/lib/config/config_messages.mes b/src/lib/config/config_messages.mes index ef7c092881..ed5df85c8b 100644 --- a/src/lib/config/config_messages.mes +++ b/src/lib/config/config_messages.mes @@ -1,4 +1,4 @@ -# Copyright (C) 2011-2015 Internet Systems Consortium, Inc. ("ISC") +# Copyright (C) 2011-2017 Internet Systems Consortium, Inc. ("ISC") # # This Source Code Form is subject to the terms of the Mozilla Public # License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/lib/testutils/io_utils.cc b/src/lib/testutils/io_utils.cc index caf9437071..384cb68481 100644 --- a/src/lib/testutils/io_utils.cc +++ b/src/lib/testutils/io_utils.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/lib/testutils/io_utils.h b/src/lib/testutils/io_utils.h index 61a18f8c79..9155a7412e 100644 --- a/src/lib/testutils/io_utils.h +++ b/src/lib/testutils/io_utils.h @@ -1,4 +1,4 @@ -// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this