[5330] CommandMgrs now use hooks to process commands in hooks libraries.

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

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

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

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

src/lib/config/hooked_command_mgr.cc

@@ -8,33 +8,13 @@
 #include <config/hooked_command_mgr.h>
 #include <config/config_log.h>
 #include <hooks/hooks_manager.h>
+#include <hooks/server_hooks.h>
 #include <boost/pointer_cast.hpp>
+#include <vector>
 
 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<Element>(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<std::string>& 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);

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.

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<std::string> 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) {

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

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
     ///

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