From: Marcin Siodelski Date: Tue, 18 Jul 2017 09:53:13 +0000 (+0200) Subject: [5330] CommandMgrs now use hooks to process commands in hooks libraries. X-Git-Tag: trac5124a_base~38^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cdc3d714e354dd015e7a613ca89167171f3896fe;p=thirdparty%2Fkea.git [5330] CommandMgrs now use hooks to process commands in hooks libraries. --- diff --git a/src/bin/agent/ca_command_mgr.cc b/src/bin/agent/ca_command_mgr.cc index f2081e286f..85e998db61 100644 --- a/src/bin/agent/ca_command_mgr.cc +++ b/src/bin/agent/ca_command_mgr.cc @@ -97,18 +97,12 @@ CtrlAgentCommandMgr::handleCommandInternal(std::string cmd_name, ElementPtr answer_list = Element::createList(); - // Before the command is forwarded it should be processed by the hooks libraries. + // Before the command is forwarded we check if there are any hooks libraries + // which would process the command. if (HookedCommandMgr::delegateCommandToHookLibrary(cmd_name, params, original_cmd, answer_list)) { - // If the hooks libraries set the 'skip' flag, they indicate that the - // commands have been processed. The answer_list should contain the list - // of answers with each answer pertaining to one service. - if (callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { - LOG_DEBUG(agent_logger, isc::log::DBGLVL_COMMAND, - CTRL_AGENT_COMMAND_PROCESS_SKIP) - .arg(cmd_name); - return (answer_list); - } + // The command has been processed by hooks library. Return the result. + return (answer_list); } // We don't know whether the hooks libraries modified the value of the diff --git a/src/bin/agent/ca_command_mgr.h b/src/bin/agent/ca_command_mgr.h index 819fa3399c..fef155c75d 100644 --- a/src/bin/agent/ca_command_mgr.h +++ b/src/bin/agent/ca_command_mgr.h @@ -30,8 +30,6 @@ public: /// it is also intended to forward commands to the respective Kea servers /// when the command is not supported directly by the Control Agent. /// -/// @todo This Command Manager doesn't yet support forwarding commands. -/// /// The @ref CtrlAgentCommandMgr is implemented as a singleton. The commands /// are registered using @c CtrlAgentCommandMgr::instance().registerCommand(). /// The @ref CtrlAgentResponseCreator uses the sole instance of the Command diff --git a/src/bin/agent/ca_messages.mes b/src/bin/agent/ca_messages.mes index 181b4b4b91..ac042d3df8 100644 --- a/src/bin/agent/ca_messages.mes +++ b/src/bin/agent/ca_messages.mes @@ -19,11 +19,6 @@ This debug message is issued when the Control Agent failed forwarding a received command to one of the Kea servers. The second argument provides the details of the error. -% CTRL_AGENT_COMMAND_PROCESS_SKIP command %1 already processed by hooks libraries, skipping -This debug message is issued when the Control Agent skips processing -received command because it has determined that the hooks libraries -already processed the command. - % CTRL_AGENT_CONFIG_CHECK_FAIL Control Agent configuration check failed: %1 This error message indicates that the CA had failed configuration check. Details are provided. Additional details may be available diff --git a/src/lib/config/config_messages.mes b/src/lib/config/config_messages.mes index 4262f64673..a40bd2b569 100644 --- a/src/lib/config/config_messages.mes +++ b/src/lib/config/config_messages.mes @@ -16,13 +16,6 @@ This debug message indicates that the daemon started supporting specified command. The handler for the registered command includes a parameter holding entire command to be processed. -% COMMAND_HOOK_RECEIVE_SKIP command %1 has been handled by the hook library which returned the skip state -This debug message is issued when a hook library has processed the control -command and returned the skip status. The callout should have set the -'response' argument which contains the result of processing the command. -The Command Manager skips processing of this command and simply returns -the response generated by the hook library. - % COMMAND_PROCESS_ERROR1 Error while processing command: %1 This warning message indicates that the server encountered an error while processing received command. Additional information will be provided, if diff --git a/src/lib/config/hooked_command_mgr.cc b/src/lib/config/hooked_command_mgr.cc index 690a3595ff..b5d6186115 100644 --- a/src/lib/config/hooked_command_mgr.cc +++ b/src/lib/config/hooked_command_mgr.cc @@ -8,33 +8,13 @@ #include #include #include +#include #include +#include using namespace isc::data; using namespace isc::hooks; -namespace { - -/// @brief Structure that holds registered hook indexes. -struct CommandMgrHooks { - /// @brief Index for "control_command_receive" hook point. - int hook_index_control_command_receive_; - - /// @brief Constructor that registers hook points for HookedCommandMgr - CommandMgrHooks() { - hook_index_control_command_receive_ = - HooksManager::registerHook("control_command_receive"); - } -}; - -// Declare a hooks object. As this is outside any function or method, it -// will be instantiated (and the constructor run) when the module is loaded. -// As a result, the hook indexes will be defined before any method in this -// module is called. -CommandMgrHooks Hooks; - -} // end of anonymous namespace - namespace isc { namespace config { @@ -43,13 +23,13 @@ HookedCommandMgr::HookedCommandMgr() } bool -HookedCommandMgr::delegateCommandToHookLibrary(std::string& cmd_name, - ConstElementPtr& params, - ConstElementPtr& original_cmd, +HookedCommandMgr::delegateCommandToHookLibrary(const std::string& cmd_name, + const ConstElementPtr& params, + const ConstElementPtr& original_cmd, ElementPtr& answer) { ConstElementPtr hook_response; - if (HooksManager::calloutsPresent(Hooks.hook_index_control_command_receive_)) { + if (HooksManager::commandHandlersPresent(cmd_name)) { callout_handle_ = HooksManager::createCalloutHandle(); @@ -66,21 +46,11 @@ HookedCommandMgr::delegateCommandToHookLibrary(std::string& cmd_name, callout_handle_->setArgument("command", command); callout_handle_->setArgument("response", hook_response); - HooksManager::callCallouts(Hooks.hook_index_control_command_receive_, - *callout_handle_); + HooksManager::callCommandHandlers(cmd_name, *callout_handle_); // The callouts should set the response. callout_handle_->getArgument("response", hook_response); - - // The hook library can modify the command or arguments. Thus, we - // retrieve the command returned by the callouts and use it as input - // to the local command handler. - ConstElementPtr hook_command; - callout_handle_->getArgument("command", hook_command); - cmd_name = parseCommand(params, hook_command); - original_cmd = hook_command; - answer = boost::const_pointer_cast(hook_response); return (true); @@ -98,33 +68,64 @@ HookedCommandMgr::handleCommand(const std::string& cmd_name, "Manager: this is a programming error"); } - std::string mutable_cmd_name = cmd_name; - ConstElementPtr mutable_params = params; - ConstElementPtr mutable_cmd = original_cmd; - - ElementPtr hook_response; - if (delegateCommandToHookLibrary(mutable_cmd_name, mutable_params, - mutable_cmd, hook_response)) { - if (callout_handle_->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { - LOG_DEBUG(command_logger, DBG_COMMAND, COMMAND_HOOK_RECEIVE_SKIP) - .arg(cmd_name); - + // The 'list-commands' is a special case. Hook libraries do not implement + // this command. We determine what commands are supported by the hook + // libraries by checking what hook points are present that have callouts + // registered. + if ((cmd_name != "list-commands")) { + ElementPtr hook_response; + // Check if there are any hooks libraries to process this command. + if (delegateCommandToHookLibrary(cmd_name, params, original_cmd, + hook_response)) { + // Hooks libraries processed this command so simply return a + // result. return (hook_response); } + } // If we're here it means that the callouts weren't called or the 'skip' // status wasn't returned. The latter is the case when the 'list-commands' // is being processed. Anyhow, we need to handle the command using local // Command Mananger. - ConstElementPtr response = BaseCommandMgr::handleCommand(mutable_cmd_name, - mutable_params, - mutable_cmd); - - // For the 'list-commands' case we will have to combine commands supported - // by the hook libraries with the commands that this Command Manager supports. - if ((mutable_cmd_name == "list-commands") && hook_response && response) { - response = combineCommandsLists(hook_response, response); + ConstElementPtr response = BaseCommandMgr::handleCommand(cmd_name, + params, + original_cmd); + + // If we're processing 'list-commands' command we may need to include + // commands supported by hooks libraries in the response. + if (cmd_name == "list-commands") { + // Hooks names can be used to decode what commands are supported. + const std::vector& hooks = + ServerHooks::getServerHooksPtr()->getHookNames(); + + // Only update the response if there are any hooks present. + if (!hooks.empty()) { + ElementPtr hooks_commands = Element::createList(); + for (auto h = hooks.cbegin(); h != hooks.end(); ++h) { + // Try to convert hook name to command name. If non-empty + // string is returned it means that the hook point may have + // command hanlers associated with it. Otherwise, it means that + // existing hook points are not for command handlers but for + // regular callouts. + std::string command_name = ServerHooks::hookToCommandName(*h); + if (!command_name.empty()) { + // Final check: are command handlers registered for this + // hook point? If there are no command handlers associated, + // it means that the hook library was already unloaded. + if (HooksManager::commandHandlersPresent(command_name)) { + hooks_commands->add(Element::create(command_name)); + } + } + } + + // If there is at least one hook point with command handlers + // registered + // for it, combine the lists of commands. + if (!hooks_commands->empty()) { + response = combineCommandsLists(response, createAnswer(0, hooks_commands)); + } + } } return (response); diff --git a/src/lib/config/hooked_command_mgr.h b/src/lib/config/hooked_command_mgr.h index 4113bdf461..7154140e1c 100644 --- a/src/lib/config/hooked_command_mgr.h +++ b/src/lib/config/hooked_command_mgr.h @@ -18,7 +18,25 @@ namespace config { /// /// This class extends @ref BaseCommandMgr with the logic to delegate the /// commands to a hook library if the hook library is installed and provides -/// callouts for the control API. +/// command handlers for the control API. +/// +/// The command handlers are registered by a hook library by calling +/// @ref isc::hooks::LibraryHandle::registerCommandHandler. This call +/// creates a hook point for this command (if one doesn't exist) and then +/// registeres the specified handler(s). When the @ref HookedCommandMgr +/// receives a command for processing it calls the +/// @ref isc::hooks::HooksManager::commandHandlersPresent to check if there +/// are handlers present for this command. If so, the @ref HookedCommandMgr +/// calls @ref isc::hooks::HooksManager::callCommandHandlers to process +/// the command in the hooks libraries. If command handlers are not installed +/// for this command, the @ref HookedCommandMgr will try to process the +/// command on its own. +/// +/// The @ref isc::hooks::CalloutHandle::CalloutNextStep flag setting by the +/// command handlers have influence on the operation of the +/// @ref HookedCommandMgr, i.e. it will always skip processing command on +/// its own if the command handlers are present for the given command, even +/// if the handlers return an error code. class HookedCommandMgr : public BaseCommandMgr { public: @@ -39,32 +57,28 @@ protected: /// @brief Handles the command within the hooks libraries. /// /// This method checks if the hooks libraries are installed which implement - /// callouts for the 'control_command_receive' hook point, and calls them - /// if they exist. If the hooks library supports the given command it creates - /// a response and returns it in the @c answer argument. + /// command handlers for the specified command to be processed. If the + /// command handlers are present, this method calls them to create a response + /// and then passes the response back within the @c answer argument. /// /// Values of all arguments can be modified by the hook library. /// - /// @param [out] cmd_name Command name. - /// @param [out] params Command arguments. - /// @param [out] original_cmd Original command received. + /// @param cmd_name Command name. + /// @param params Command arguments. + /// @param original_cmd Original command received. /// @param [out] answer Command processing result returned by the hook. /// /// @return Boolean value indicating if any callouts have been executed. bool - delegateCommandToHookLibrary(std::string& cmd_name, - isc::data::ConstElementPtr& params, - isc::data::ConstElementPtr& original_cmd, + delegateCommandToHookLibrary(const std::string& cmd_name, + const isc::data::ConstElementPtr& params, + const isc::data::ConstElementPtr& original_cmd, isc::data::ElementPtr& answer); /// @brief Handles the command having a given name and arguments. /// /// This method calls @ref HookedCommandMgr::delegateCommandToHookLibrary to /// try to process the command with the hook libraries, if they are installed. - /// If the returned @c skip value indicates that the callout set the 'skip' flag - /// the command is assumed to have been processed and the response is returned. - /// If the 'skip' flag is not set, the @ref BaseCommandMgr::handleCommand is - /// called. /// /// @param cmd_name Command name. /// @param params Command arguments. diff --git a/src/lib/config/tests/command_mgr_unittests.cc b/src/lib/config/tests/command_mgr_unittests.cc index 9bff3f9839..db251c0b23 100644 --- a/src/lib/config/tests/command_mgr_unittests.cc +++ b/src/lib/config/tests/command_mgr_unittests.cc @@ -48,8 +48,6 @@ public: CommandMgr::instance().deregisterAll(); CommandMgr::instance().closeCommandSocket(); resetCalloutIndicators(); - HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts( - "control_command_receive"); } /// @brief Returns socket path (using either hardcoded path or env variable) @@ -67,9 +65,18 @@ public: } /// @brief Resets indicators related to callout invocation. + /// + /// It also removes any registered callouts. static void resetCalloutIndicators() { callout_name = ""; callout_argument_names.clear(); + + // Iterate over existing hook points and for each of them remove + // callouts registered. + std::vector hooks = ServerHooks::getServerHooksPtr()->getHookNames(); + for (auto h = hooks.cbegin(); h != hooks.cend(); ++h) { + HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts(*h); + } } /// @brief A simple command handler that always returns an eror @@ -92,76 +99,14 @@ public: return (createAnswer(234, "text generated by hook handler")); } - /// @brief Test callback which stores callout name and passed arguments. - /// - /// This callout doesn't indicate that the command has been processed, - /// allowing the Command Manager to process it. - /// - /// @param callout_handle Handle passed by the hooks framework. - /// @return Always 0. - static int - control_command_receive_callout(CalloutHandle& callout_handle) { - callout_name = "control_command_receive"; - - ConstElementPtr response; - callout_handle.setArgument("response", response); - - callout_argument_names = callout_handle.getArgumentNames(); - // Sort arguments alphabetically, so as we can access them on - // expected positions and verify. - std::sort(callout_argument_names.begin(), callout_argument_names.end()); - return (0); - } - /// @brief Test callback which stores callout name and passed arguments and /// which handles the command. /// - /// This callout returns the skip status to indicate the the command has - /// been handled. - /// /// @param callout_handle Handle passed by the hooks framework. /// @return Always 0. static int control_command_receive_handle_callout(CalloutHandle& callout_handle) { - callout_name = "control_command_receive"; - - // Create a hooks specific command manager. - BaseCommandMgr callout_command_mgr; - callout_command_mgr.registerCommand("my-command", my_hook_handler); - - ConstElementPtr command; - callout_handle.getArgument("command", command); - - ConstElementPtr arg; - std::string command_name = parseCommand(arg, command); - - ConstElementPtr response = callout_command_mgr.processCommand(command); - callout_handle.setArgument("response", response); - - // Set 'skip' status to indicate that the command has been handled. - if (command_name != "list-commands") { - callout_handle.setStatus(CalloutHandle::NEXT_STEP_SKIP); - } - - callout_argument_names = callout_handle.getArgumentNames(); - // Sort arguments alphabetically, so as we can access them on - // expected positions and verify. - std::sort(callout_argument_names.begin(), callout_argument_names.end()); - return (0); - } - - /// @brief Test callback which modifies parameters of the command and - /// does not return skip status. - /// - /// This callout is used to test the case when the callout modifies the - /// received command and does not set next state SKIP to propagate the - /// command with modified parameters to the local command handler. - /// - /// @param callout_handle Handle passed by the hooks framework. - /// @return Always 0. - static int - control_command_receive_modify_callout(CalloutHandle& callout_handle) { - callout_name = "control_command_receive"; + callout_name = "control_command_receive_handle"; ConstElementPtr command; callout_handle.getArgument("command", command); @@ -169,11 +114,8 @@ public: ConstElementPtr arg; std::string command_name = parseCommand(arg, command); - ElementPtr new_arg = Element::createList(); - new_arg->add(Element::create("hook-param")); - command = createCommand(command_name, new_arg); - - callout_handle.setArgument("command", command); + callout_handle.setArgument("response", + createAnswer(234, "text generated by hook handler")); callout_argument_names = callout_handle.getArgumentNames(); // Sort arguments alphabetically, so as we can access them on @@ -338,12 +280,6 @@ TEST_F(CommandMgrTest, deregisterAll) { // Test checks whether a command handler can be installed and then // runs through processCommand to check that it's indeed called. TEST_F(CommandMgrTest, processCommand) { - - // Register callout so as we can check that it is called before - // processing the command by the manager. - HooksManager::preCalloutsLibraryHandle().registerCallout( - "control_command_receive", control_command_receive_callout); - // Install my handler EXPECT_NO_THROW(CommandMgr::instance().registerCommand("my-command", my_handler)); @@ -371,21 +307,17 @@ TEST_F(CommandMgrTest, processCommand) { ASSERT_TRUE(handler_params); EXPECT_EQ("[ \"just\", \"some\", \"data\" ]", handler_params->str()); - EXPECT_EQ("control_command_receive", callout_name); - - // Check that the appropriate arguments have been set. Include the - // 'response' which should have been set by the callout. - ASSERT_EQ(2, callout_argument_names.size()); - EXPECT_EQ("command", callout_argument_names[0]); - EXPECT_EQ("response", callout_argument_names[1]); + // Command handlers not installed so expecting that callouts weren't + // called. + EXPECT_TRUE(callout_name.empty()); } // Verify that processing a command can be delegated to a hook library. TEST_F(CommandMgrTest, delegateProcessCommand) { // Register callout so as we can check that it is called before // processing the command by the manager. - HooksManager::preCalloutsLibraryHandle().registerCallout( - "control_command_receive", control_command_receive_handle_callout); + HooksManager::preCalloutsLibraryHandle().registerCommandHandler( + "my-command", control_command_receive_handle_callout); // Install local handler EXPECT_NO_THROW(CommandMgr::instance().registerCommand("my-command", @@ -411,7 +343,7 @@ TEST_F(CommandMgrTest, delegateProcessCommand) { ASSERT_NO_THROW(answer_arg = parseAnswer(status_code, answer)); EXPECT_EQ(234, status_code); - EXPECT_EQ("control_command_receive", callout_name); + EXPECT_EQ("control_command_receive_handle", callout_name); // Check that the appropriate arguments have been set. Include the // 'response' which should have been set by the callout. @@ -425,8 +357,8 @@ TEST_F(CommandMgrTest, delegateProcessCommand) { TEST_F(CommandMgrTest, delegateListCommands) { // Register callout so as we can check that it is called before // processing the command by the manager. - HooksManager::preCalloutsLibraryHandle().registerCallout( - "control_command_receive", control_command_receive_handle_callout); + HooksManager::preCalloutsLibraryHandle().registerCommandHandler( + "my-command", control_command_receive_handle_callout); // Create my-command-bis which is unique for the local Command Manager, // i.e. not supported by the hook library. This command should also @@ -464,56 +396,6 @@ TEST_F(CommandMgrTest, delegateListCommands) { EXPECT_EQ("my-command-bis", command_names_list[2]); } -// This test verifies the scenario in which the hook library influences the -// command processing by the Kea server. In this test, the callout modifies -// the arguments of the command and passes the command on to the Command -// Manager for processing. -TEST_F(CommandMgrTest, modifyCommandArgsInHook) { - // Register callout so as we can check that it is called before - // processing the command by the manager. - HooksManager::preCalloutsLibraryHandle().registerCallout( - "control_command_receive", control_command_receive_modify_callout); - - // Install local handler - EXPECT_NO_THROW(CommandMgr::instance().registerCommand("my-command", - my_handler)); - - // Now tell CommandMgr to process a command 'my-command' with the - // specified parameter. - ElementPtr my_params = Element::fromJSON("[ \"just\", \"some\", \"data\" ]"); - ConstElementPtr command = createCommand("my-command", my_params); - ConstElementPtr answer; - ASSERT_NO_THROW(answer = CommandMgr::instance().processCommand(command)); - - // There should be an answer. - ASSERT_TRUE(answer); - - // Returned status should be unique for the my_handler. - ConstElementPtr answer_arg; - int status_code; - ASSERT_NO_THROW(answer_arg = parseAnswer(status_code, answer)); - EXPECT_EQ(123, status_code); - - // Local handler should have been called after the callout. - ASSERT_TRUE(handler_called); - EXPECT_EQ("my-command", handler_name); - ASSERT_TRUE(handler_params); - // Check that the local handler received the command with arguments - // set by the callout. - EXPECT_EQ("[ \"hook-param\" ]", handler_params->str()); - - - // Check that the callout has been called with appropriate parameters. - EXPECT_EQ("control_command_receive", callout_name); - - // Check that the appropriate arguments have been set. Include the - // 'response' which should have been set by the callout. - ASSERT_EQ(2, callout_argument_names.size()); - EXPECT_EQ("command", callout_argument_names[0]); - EXPECT_EQ("response", callout_argument_names[1]); - -} - // This test verifies that a Unix socket can be opened properly and that input // parameters (socket-type and socket-name) are verified. TEST_F(CommandMgrTest, unixCreate) { diff --git a/src/lib/hooks/server_hooks.cc b/src/lib/hooks/server_hooks.cc index e8f12ead24..d7cfcdf901 100644 --- a/src/lib/hooks/server_hooks.cc +++ b/src/lib/hooks/server_hooks.cc @@ -182,7 +182,17 @@ ServerHooks::commandToHookName(const std::string& command_name) { return (hook_name); } +std::string +ServerHooks::hookToCommandName(const std::string& hook_name) { + if (!hook_name.empty() && hook_name.front() == '$') { + std::string command_name = hook_name.substr(1); + std::replace(command_name.begin(), command_name.end(), '_', '-'); + return (command_name); + } + return (""); +} + -} // namespace util +} // namespace hooks } // namespace isc diff --git a/src/lib/hooks/server_hooks.h b/src/lib/hooks/server_hooks.h index 43f7d7df14..edd5ab672b 100644 --- a/src/lib/hooks/server_hooks.h +++ b/src/lib/hooks/server_hooks.h @@ -167,8 +167,20 @@ public: /// /// @param command_name Command name for which the hook point name is /// to be generated. + /// + /// @return Hook point name. static std::string commandToHookName(const std::string& command_name); + /// @brief Returns command name for a specified hook name. + /// + /// This function removes leading dollar sign and replaces underscores + /// with hyphens. + /// + /// @param hook_name Hook name for which command name should be returned. + /// + /// @return Command name. + static std::string hookToCommandName(const std::string& hook_name); + private: /// @brief Constructor /// diff --git a/src/lib/hooks/tests/server_hooks_unittest.cc b/src/lib/hooks/tests/server_hooks_unittest.cc index 78d8e13788..6c012fa41f 100644 --- a/src/lib/hooks/tests/server_hooks_unittest.cc +++ b/src/lib/hooks/tests/server_hooks_unittest.cc @@ -198,7 +198,8 @@ TEST(ServerHooksTest, HookCount) { EXPECT_EQ(6, hooks.getCount()); } -// Check that the hook name is correctly generated for a control command name. +// Check that the hook name is correctly generated for a control command name +// and vice versa. TEST(ServerHooksTest, CommandToHookName) { EXPECT_EQ("$x_y_z", ServerHooks::commandToHookName("x-y-z")); @@ -206,4 +207,14 @@ TEST(ServerHooksTest, CommandToHookName) { EXPECT_EQ("$", ServerHooks::commandToHookName("")); } +TEST(ServerHooksTest, HookToCommandName) { + // Underscores replaced by hyphens. + EXPECT_EQ("x-y-z", ServerHooks::hookToCommandName("$x_y_z")); + EXPECT_EQ("foo-bar-foo", ServerHooks::hookToCommandName("$foo_bar-foo")); + // Single dollar is converted to empty string. + EXPECT_TRUE(ServerHooks::hookToCommandName("$").empty()); + // If no dollar, it is not a hook name. Return empty string. + EXPECT_TRUE(ServerHooks::hookToCommandName("abc").empty()); +} + } // Anonymous namespace