From 9e0dc74611b0554593c66dac297b340b73c2aaa3 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Sat, 29 Dec 2018 00:17:35 +0100 Subject: [PATCH] [30-implement-control-socket-for-ddns-2] Added config-set support --- src/bin/agent/agent_parser.yy | 2 +- src/bin/agent/ca_controller.cc | 4 + src/bin/agent/simple_parser.cc | 2 +- .../agent/tests/ca_controller_unittests.cc | 1 + src/bin/agent/tests/ca_process_unittests.cc | 2 +- src/bin/d2/d2_controller.cc | 4 + src/bin/d2/tests/d2_command_unittest.cc | 137 +++++++++++++++++- src/lib/process/d_cfg_mgr.cc | 21 ++- src/lib/process/d_controller.cc | 55 +++++++ src/lib/process/d_controller.h | 12 ++ src/lib/process/d_process.h | 3 + 11 files changed, 231 insertions(+), 12 deletions(-) diff --git a/src/bin/agent/agent_parser.yy b/src/bin/agent/agent_parser.yy index f02ee1bf90..fe590f7fea 100644 --- a/src/bin/agent/agent_parser.yy +++ b/src/bin/agent/agent_parser.yy @@ -113,7 +113,7 @@ using namespace std; // is parsed. start: START_JSON { ctx.ctx_ = ctx.NO_KEYWORDS; } json | START_AGENT { ctx.ctx_ = ctx.CONFIG; } agent_syntax_map - | START_SUB_AGENT { ctx.ctx_ = ctx.AGENT; } sub_agent + | START_SUB_AGENT { ctx.ctx_ = ctx.AGENT; } sub_agent ; // This rule defines a "shortcut". Instead of specifying the whole structure diff --git a/src/bin/agent/ca_controller.cc b/src/bin/agent/ca_controller.cc index 0c101b7ad6..4f38136ca3 100644 --- a/src/bin/agent/ca_controller.cc +++ b/src/bin/agent/ca_controller.cc @@ -57,6 +57,9 @@ CtrlAgentController::registerCommands() { CtrlAgentCommandMgr::instance().registerCommand(CONFIG_GET_COMMAND, boost::bind(&DControllerBase::configGetHandler, this, _1, _2)); + CtrlAgentCommandMgr::instance().registerCommand(CONFIG_SET_COMMAND, + boost::bind(&DControllerBase::configSetHandler, this, _1, _2)); + CtrlAgentCommandMgr::instance().registerCommand(CONFIG_TEST_COMMAND, boost::bind(&DControllerBase::configTestHandler, this, _1, _2)); @@ -74,6 +77,7 @@ void CtrlAgentController::deregisterCommands() { CtrlAgentCommandMgr::instance().deregisterCommand(BUILD_REPORT_COMMAND); CtrlAgentCommandMgr::instance().deregisterCommand(CONFIG_GET_COMMAND); + CtrlAgentCommandMgr::instance().deregisterCommand(CONFIG_SET_COMMAND); CtrlAgentCommandMgr::instance().deregisterCommand(CONFIG_TEST_COMMAND); CtrlAgentCommandMgr::instance().deregisterCommand(CONFIG_WRITE_COMMAND); CtrlAgentCommandMgr::instance().deregisterCommand(SHUT_DOWN_COMMAND); diff --git a/src/bin/agent/simple_parser.cc b/src/bin/agent/simple_parser.cc index 807b7c7d52..9f8718c7ba 100644 --- a/src/bin/agent/simple_parser.cc +++ b/src/bin/agent/simple_parser.cc @@ -104,7 +104,7 @@ AgentSimpleParser::parse(const CtrlAgentCfgContextPtr& ctx, } // Finally, let's get the hook libs! - + using namespace isc::hooks; HooksConfig& libraries = ctx->getHooksConfig(); ConstElementPtr hooks = config->get("hooks-libraries"); diff --git a/src/bin/agent/tests/ca_controller_unittests.cc b/src/bin/agent/tests/ca_controller_unittests.cc index 636c064300..b2b5d1581c 100644 --- a/src/bin/agent/tests/ca_controller_unittests.cc +++ b/src/bin/agent/tests/ca_controller_unittests.cc @@ -448,6 +448,7 @@ TEST_F(CtrlAgentControllerTest, registeredCommands) { // Check that the following command are really available. checkCommandRegistered("build-report"); checkCommandRegistered("config-get"); + checkCommandRegistered("config-set"); checkCommandRegistered("config-test"); checkCommandRegistered("config-write"); checkCommandRegistered("list-commands"); diff --git a/src/bin/agent/tests/ca_process_unittests.cc b/src/bin/agent/tests/ca_process_unittests.cc index a6abb47197..3f49dfcbdc 100644 --- a/src/bin/agent/tests/ca_process_unittests.cc +++ b/src/bin/agent/tests/ca_process_unittests.cc @@ -63,7 +63,7 @@ TEST(CtrlAgentProcess, construction) { } // Verifies that en external call to shutdown causes the run method to -// exit gracefully. +// exit gracefully. TEST_F(CtrlAgentProcessTest, shutdown) { // Use an asiolink IntervalTimer and callback to generate the // shutdown invocation. (Note IntervalTimer setup is in milliseconds). diff --git a/src/bin/d2/d2_controller.cc b/src/bin/d2/d2_controller.cc index 7e3bb084ae..a2e2d89156 100644 --- a/src/bin/d2/d2_controller.cc +++ b/src/bin/d2/d2_controller.cc @@ -58,6 +58,9 @@ D2Controller::registerCommands() { CommandMgr::instance().registerCommand(CONFIG_GET_COMMAND, boost::bind(&D2Controller::configGetHandler, this, _1, _2)); + CommandMgr::instance().registerCommand(CONFIG_SET_COMMAND, + boost::bind(&D2Controller::configSetHandler, this, _1, _2)); + CommandMgr::instance().registerCommand(CONFIG_TEST_COMMAND, boost::bind(&D2Controller::configTestHandler, this, _1, _2)); @@ -80,6 +83,7 @@ D2Controller::deregisterCommands() { // Deregister any registered commands (please keep in alphabetic order) CommandMgr::instance().deregisterCommand(BUILD_REPORT_COMMAND); CommandMgr::instance().deregisterCommand(CONFIG_GET_COMMAND); + CommandMgr::instance().deregisterCommand(CONFIG_SET_COMMAND); CommandMgr::instance().deregisterCommand(CONFIG_TEST_COMMAND); CommandMgr::instance().deregisterCommand(CONFIG_WRITE_COMMAND); CommandMgr::instance().deregisterCommand(SHUT_DOWN_COMMAND); diff --git a/src/bin/d2/tests/d2_command_unittest.cc b/src/bin/d2/tests/d2_command_unittest.cc index 4c7675516b..cb998b47ba 100644 --- a/src/bin/d2/tests/d2_command_unittest.cc +++ b/src/bin/d2/tests/d2_command_unittest.cc @@ -722,7 +722,8 @@ TEST_F(CtrlChannelD2Test, configTest) { EXPECT_EQ("{ \"result\": 1, \"text\": \"missing parameter 'name' (:9:14)\" }", response); - // Check that the config was not lost. + // Check that the config was not lost (fix: reacquire the context). + d2_context = cfg_mgr->getD2CfgContext(); keys = d2_context->getKeys(); ASSERT_TRUE(keys); EXPECT_EQ(1, keys->size()); @@ -754,11 +755,145 @@ TEST_F(CtrlChannelD2Test, configTest) { response); // Check that the config was not applied. + d2_context = cfg_mgr->getD2CfgContext(); keys = d2_context->getKeys(); ASSERT_TRUE(keys); EXPECT_EQ(1, keys->size()); } +// Verify that the "config-set" command will do what we expect. +TEST_F(CtrlChannelD2Test, configSet) { + + // Define strings to permutate the config arguments. + // (Note the line feeds makes errors easy to find) + string config_set_txt = "{ \"command\": \"config-set\" \n"; + string args_txt = " \"arguments\": { \n"; + string d2_header = + " \"DhcpDdns\": \n"; + string d2_cfg_txt = + " { \n" + " \"ip-address\": \"192.168.77.1\", \n" + " \"port\": 777, \n" + " \"forward-ddns\" : {}, \n" + " \"reverse-ddns\" : {}, \n" + " \"tsig-keys\": [ \n"; + string key1 = + " {\"name\": \"d2_key.example.com\", \n" + " \"algorithm\": \"hmac-md5\", \n" + " \"secret\": \"LSWXnfkKZjdPJI5QxlpnfQ==\"} \n"; + string key2 = + " {\"name\": \"d2_key.billcat.net\", \n" + " \"algorithm\": \"hmac-md5\", \n" + " \"digest-bits\": 120, \n" + " \"secret\": \"LSWXnfkKZjdPJI5QxlpnfQ==\"} \n"; + string bad_key = + " {\"BOGUS\": \"d2_key.example.com\", \n" + " \"algorithm\": \"hmac-md5\", \n" + " \"secret\": \"LSWXnfkKZjdPJI5QxlpnfQ==\"} \n"; + string key_footer = + " ] \n"; + string control_socket_header = + " ,\"control-socket\": { \n" + " \"socket-type\": \"unix\", \n" + " \"socket-name\": \""; + string control_socket_footer = + "\" \n} \n"; + + ostringstream os; + // Create a valid config with all the parts should parse. + os << d2_cfg_txt + << key1 + << key_footer + << control_socket_header + << socket_path_ + << control_socket_footer + << "}\n"; + + ASSERT_TRUE(server_); + + ConstElementPtr config; + ASSERT_NO_THROW(config = parseDHCPDDNS(os.str(), true)); + ASSERT_NO_THROW(d2Controller()->initProcess()); + D2ProcessPtr proc = d2Controller()->getProcess(); + ASSERT_TRUE(proc); + ConstElementPtr answer = proc->configure(config, false); + ASSERT_TRUE(answer); + EXPECT_EQ("{ \"result\": 0, \"text\": \"Configuration applied successfully.\" }", + answer->str()); + ASSERT_NO_THROW(d2Controller()->registerCommands()); + + // Check that the config was indeed applied. + D2CfgMgrPtr cfg_mgr = proc->getD2CfgMgr(); + ASSERT_TRUE(cfg_mgr); + D2CfgContextPtr d2_context = cfg_mgr->getD2CfgContext(); + ASSERT_TRUE(d2_context); + TSIGKeyInfoMapPtr keys = d2_context->getKeys(); + ASSERT_TRUE(keys); + EXPECT_EQ(1, keys->size()); + + ASSERT_GT(CommandMgr::instance().getControlSocketFD(), -1); + + // Create a config with malformed subnet that should fail to parse. + os.str(""); + os << config_set_txt << "," + << args_txt + << d2_header + << d2_cfg_txt + << bad_key + << key_footer + << control_socket_header + << socket_path_ + << control_socket_footer + << "}\n" // close DhcpDdns. + << "}}"; + + // Send the config-set command. + string response; + sendUnixCommand(os.str(), response); + + // Should fail with a syntax error. + EXPECT_EQ("{ \"result\": 1, \"text\": \"missing parameter 'name' (:9:14)\" }", + response); + + // Check that the config was not lost (fix: reacquire the context). + d2_context = cfg_mgr->getD2CfgContext(); + keys = d2_context->getKeys(); + ASSERT_TRUE(keys); + EXPECT_EQ(1, keys->size()); + + // Create a valid config with two keys and no command channel. + os.str(""); + os << config_set_txt << "," + << args_txt + << d2_header + << d2_cfg_txt + << key1 + << ",\n" + << key2 + << key_footer + << "}\n" // close DhcpDdns. + << "}}"; + + // Verify the control channel socket exists. + ASSERT_TRUE(test::fileExists(socket_path_)); + + // Send the config-set command. + sendUnixCommand(os.str(), response); + + // Verify the control channel socket no longer exists. + EXPECT_FALSE(test::fileExists(socket_path_)); + + // Verify the configuration was successful. + EXPECT_EQ("{ \"result\": 0, \"text\": \"Configuration applied successfully.\" }", + response); + + // Check that the config was applied. + d2_context = cfg_mgr->getD2CfgContext(); + keys = d2_context->getKeys(); + ASSERT_TRUE(keys); + EXPECT_EQ(2, keys->size()); +} + // Tests if config-write can be called without any parameters. TEST_F(CtrlChannelD2Test, writeConfigNoFilename) { EXPECT_NO_THROW(createUnixChannelServer()); diff --git a/src/lib/process/d_cfg_mgr.cc b/src/lib/process/d_cfg_mgr.cc index 800b7751bd..21704f0f25 100644 --- a/src/lib/process/d_cfg_mgr.cc +++ b/src/lib/process/d_cfg_mgr.cc @@ -74,6 +74,7 @@ DCfgMgrBase::simpleParseConfig(isc::data::ConstElementPtr config_set, // so as we can rollback changes when an error occurs. ConfigPtr original_context = context_; resetContext(); + bool rollback = false; // Answer will hold the result returned to the caller. ConstElementPtr answer; @@ -88,12 +89,14 @@ DCfgMgrBase::simpleParseConfig(isc::data::ConstElementPtr config_set, // Everything was fine. Configuration set processed successfully. if (!check_only) { - if (post_config_cb) { - post_config_cb(); - } - if (code == 0) { + // Call the callback only when parsing was successful. + if (post_config_cb) { + post_config_cb(); + } LOG_INFO(dctl_logger, DCTL_CONFIG_COMPLETE).arg(getConfigSummary(0)); + } else { + rollback = true; } // Use the answer provided. @@ -107,10 +110,7 @@ DCfgMgrBase::simpleParseConfig(isc::data::ConstElementPtr config_set, } 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); + rollback = true; } if (check_only) { @@ -119,6 +119,11 @@ DCfgMgrBase::simpleParseConfig(isc::data::ConstElementPtr config_set, context_ = original_context; } + if (rollback) { + // An error occurred, so make sure that we restore original context. + context_ = original_context; + } + return (answer); } diff --git a/src/lib/process/d_controller.cc b/src/lib/process/d_controller.cc index 02c4d90963..49fb6db336 100644 --- a/src/lib/process/d_controller.cc +++ b/src/lib/process/d_controller.cc @@ -540,6 +540,61 @@ DControllerBase::configTestHandler(const std::string&, ConstElementPtr args) { return (checkConfig(module_config)); } +ConstElementPtr +DControllerBase::configSetHandler(const std::string&, ConstElementPtr args) { + const int status_code = COMMAND_ERROR; // 1 indicates an error + ConstElementPtr module_config; + std::string app_name = getAppName(); + std::string message; + + // Command arguments are expected to be: + // { "Module": { ... }, "Logging": { ... } } + // 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 { + module_config = args->get(app_name); + if (!module_config) { + message = "Missing mandatory '" + app_name + "' parameter."; + } else if (module_config->getType() != Element::map) { + message = "'" + app_name + "' 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); + } + + // We are starting the configuration process so we should remove any + // staging configuration that has been created during previous + // configuration attempts. + // We're not using cfgmgr to store logging information anymore. + // isc::dhcp::CfgMgr::instance().rollback(); + + // Temporary storage for logging configuration + ConfigPtr storage = process_->getCfgMgr()->getContext(); + + // Get 'Logging' element from the config and use it to set up + // logging. If there's no such element, we'll just pass NULL. + Daemon::configureLogger(args->get("Logging"), storage); + + // Now we check the server proper. + ConstElementPtr answer = updateConfig(module_config); + int rcode = 0; + parseAnswer(rcode, answer); + if (!rcode) { + // Configuration successful, so apply the logging configuration + // to log4cplus. + storage->applyLoggingCfg(); + } + + return (answer); +} + ConstElementPtr DControllerBase::versionGetHandler(const std::string&, ConstElementPtr) { ConstElementPtr answer; diff --git a/src/lib/process/d_controller.h b/src/lib/process/d_controller.h index 34727508c8..ed4281b865 100644 --- a/src/lib/process/d_controller.h +++ b/src/lib/process/d_controller.h @@ -297,6 +297,18 @@ public: configTestHandler(const std::string& command, isc::data::ConstElementPtr args); + /// @brief handler for config-set command + /// + /// This method handles the config-set command, which checks + /// configuration specified in args parameter. + /// + /// @param command (ignored) + /// @param args configuration to be checked. + /// @return status of the command + isc::data::ConstElementPtr + configSetHandler(const std::string& command, + isc::data::ConstElementPtr args); + /// @brief handler for 'shutdown' command /// /// This method handles shutdown command. It initiates the shutdown procedure diff --git a/src/lib/process/d_process.h b/src/lib/process/d_process.h index 62b54703a0..f3af1b349d 100644 --- a/src/lib/process/d_process.h +++ b/src/lib/process/d_process.h @@ -40,6 +40,9 @@ static const std::string CONFIG_WRITE_COMMAND("config-write"); /// @brief String value for the config-test command. static const std::string CONFIG_TEST_COMMAND("config-test"); +/// @brief String value for the config-set command. +static const std::string CONFIG_SET_COMMAND("config-set"); + /// @brief String value for the shutdown command. static const std::string SHUT_DOWN_COMMAND("shutdown"); -- 2.21.0