From: Marcin Siodelski Date: Wed, 9 Dec 2020 17:58:04 +0000 (+0100) Subject: [#432] Always wrap responses from CA in a list X-Git-Tag: Kea-1.9.3~64 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d3ac59e10fd508754e13315bd3b40971be514497;p=thirdparty%2Fkea.git [#432] Always wrap responses from CA in a list --- diff --git a/src/bin/agent/ca_command_mgr.cc b/src/bin/agent/ca_command_mgr.cc index 8889e54ce4..db2e80a134 100644 --- a/src/bin/agent/ca_command_mgr.cc +++ b/src/bin/agent/ca_command_mgr.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2017-2019 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2020 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 @@ -44,31 +44,27 @@ CtrlAgentCommandMgr::CtrlAgentCommandMgr() : HookedCommandMgr() { } -ConstElementPtr -CtrlAgentCommandMgr::handleCommand(const std::string& cmd_name, - const isc::data::ConstElementPtr& params, - const isc::data::ConstElementPtr& original_cmd) { - ConstElementPtr answer = handleCommandInternal(cmd_name, params, original_cmd); +isc::data::ConstElementPtr +CtrlAgentCommandMgr::processCommand(const isc::data::ConstElementPtr& cmd) { + ConstElementPtr answer = HookedCommandMgr::processCommand(cmd); if (answer->getType() == Element::list) { return (answer); } - // In general, the handlers should return a list of answers rather than a - // single answer, but in some cases we rely on the generic handlers, - // e.g. 'list-commands', which may return a single answer not wrapped in - // the list. Such answers need to be wrapped in the list here. + // Responses from the Kea Control Agent must be always wrapped + // in a list because in general they contain responses from + // multiple daemons. ElementPtr answer_list = Element::createList(); answer_list->add(boost::const_pointer_cast(answer)); return (answer_list); } - ConstElementPtr -CtrlAgentCommandMgr::handleCommandInternal(std::string cmd_name, - isc::data::ConstElementPtr params, - isc::data::ConstElementPtr original_cmd) { +CtrlAgentCommandMgr::handleCommand(const std::string& cmd_name, + const isc::data::ConstElementPtr& params, + const isc::data::ConstElementPtr& original_cmd) { ConstElementPtr services = Element::createList(); diff --git a/src/bin/agent/ca_command_mgr.h b/src/bin/agent/ca_command_mgr.h index fef155c75d..1995cb5a72 100644 --- a/src/bin/agent/ca_command_mgr.h +++ b/src/bin/agent/ca_command_mgr.h @@ -1,4 +1,4 @@ -// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2020 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 @@ -41,6 +41,22 @@ public: /// @brief Returns sole instance of the Command Manager. static CtrlAgentCommandMgr& instance(); + /// @brief Triggers command processing. + /// + /// This method overrides the @c BaseCommandMgr::processCommand to ensure + /// that the response is always wrapped in a list. The base implementation + /// returns a response map. Kea Control Agent forwards commands to multiple + /// daemons behind it and thus it must return a list of responses from + /// respective daemons. If an error occurs during command processing the + /// error response must also be wrapped in a list because caller expects + /// that CA always returns a list. + /// + /// @param cmd Pointer to the data element representing command in JSON + /// format. + /// @return Pointer to the response. + virtual isc::data::ConstElementPtr + processCommand(const isc::data::ConstElementPtr& cmd); + /// @brief Handles the command having a given name and arguments. /// /// This method extends the base implementation with the ability to forward @@ -69,24 +85,6 @@ public: private: - /// @brief Implements the logic for @ref CtrlAgentCommandMgr::handleCommand. - /// - /// All parameters are passed by value because they may be modified within - /// the method. - /// - /// @param cmd_name Command name. - /// @param params Command arguments. - /// @param original_cmd Original command being processed. - /// - /// @return Pointer to the const data element representing a list of responses - /// to the command or a single response (not wrapped in a list). The - /// @ref CtrlAgentCommandMgr::handleCommand will wrap non-list value returned - /// in a single element list. - isc::data::ConstElementPtr - handleCommandInternal(std::string cmd_name, - isc::data::ConstElementPtr params, - isc::data::ConstElementPtr original_cmd); - /// @brief Tries to forward received control command to a specified server. /// /// @param service Contains name of the service where the command should be diff --git a/src/bin/agent/tests/ca_command_mgr_unittests.cc b/src/bin/agent/tests/ca_command_mgr_unittests.cc index 842e1f0462..f8429fc9c7 100644 --- a/src/bin/agent/tests/ca_command_mgr_unittests.cc +++ b/src/bin/agent/tests/ca_command_mgr_unittests.cc @@ -235,8 +235,7 @@ public: server_socket_->waitForRunning(); ConstElementPtr command = createCommand("foo", service); - ConstElementPtr answer = mgr_.handleCommand("foo", ConstElementPtr(), - command); + ConstElementPtr answer = mgr_.processCommand(command); // Stop IO service immediatelly and let the thread die. getIOService()->stop(); @@ -267,18 +266,28 @@ public: /// properly. TEST_F(CtrlAgentCommandMgrTest, bogus) { ConstElementPtr answer; - EXPECT_NO_THROW(answer = mgr_.handleCommand("fish-and-chips-please", - ConstElementPtr(), - ConstElementPtr())); + EXPECT_NO_THROW(answer = mgr_.processCommand(createCommand("fish-and-chips-please", ""))); checkAnswer(answer, isc::config::CONTROL_RESULT_COMMAND_UNSUPPORTED); }; +// Test verifying that parameter other than command, arguments and service is +// rejected and that the correct error is returned. +TEST_F(CtrlAgentCommandMgrTest, extraParameter) { + ElementPtr command = Element::createMap(); + command->set("command", Element::create("list-commands")); + command->set("arguments", Element::createMap()); + command->set("extra-arg", Element::createMap()); + + ConstElementPtr answer; + EXPECT_NO_THROW(answer = mgr_.processCommand(command)); + checkAnswer(answer, isc::config::CONTROL_RESULT_ERROR); +} + /// Just a basic test checking that 'list-commands' is supported. TEST_F(CtrlAgentCommandMgrTest, listCommands) { ConstElementPtr answer; - EXPECT_NO_THROW(answer = mgr_.handleCommand("list-commands", - ConstElementPtr(), - ConstElementPtr())); + EXPECT_NO_THROW(answer = mgr_.processCommand(createCommand("list-commands", ""))); + checkAnswer(answer, isc::config::CONTROL_RESULT_SUCCESS); }; diff --git a/src/bin/agent/tests/ca_controller_unittests.cc b/src/bin/agent/tests/ca_controller_unittests.cc index 4d657c796c..2be67dfe46 100644 --- a/src/bin/agent/tests/ca_controller_unittests.cc +++ b/src/bin/agent/tests/ca_controller_unittests.cc @@ -97,7 +97,7 @@ public: sock_info->get("socket-name")->stringValue()); } - /// @brief Compares the status in the given parse result to a given value. + /// @brief Compares the status in the given parse result to a given value. /// /// @param answer Element set containing an integer response and string /// comment. @@ -107,13 +107,6 @@ public: void checkAnswer(isc::data::ConstElementPtr answer, int exp_status, string exp_txt = "") { - - // Get rid of the outer list. - ASSERT_TRUE(answer); - ASSERT_EQ(Element::list, answer->getType()); - ASSERT_LE(1, answer->size()); - answer = answer->get(0); - int rcode = 0; isc::data::ConstElementPtr comment; comment = isc::config::parseAnswer(rcode, answer); @@ -547,9 +540,9 @@ TEST_F(CtrlAgentControllerTest, configReloadMissingFile) { params, cmd); // Verify the reload was rejected. - string expected = "[ { \"result\": 1, \"text\": " + string expected = "{ \"result\": 1, \"text\": " "\"Configuration parsing failed: " - "Unable to open file does-not-exist.json\" } ]"; + "Unable to open file does-not-exist.json\" }"; EXPECT_EQ(expected, answer->str()); // Now clean up after ourselves. @@ -594,9 +587,9 @@ TEST_F(CtrlAgentControllerTest, configReloadBrokenFile) { params, cmd); // Verify the reload was rejected. - string expected = "[ { \"result\": 1, \"text\": " + string expected = "{ \"result\": 1, \"text\": " "\"Configuration parsing failed: " - "testbad.json:1.1: Invalid character: b\" } ]"; + "testbad.json:1.1: Invalid character: b\" }"; EXPECT_EQ(expected, answer->str()); // Remove the file. @@ -646,8 +639,8 @@ TEST_F(CtrlAgentControllerTest, configReloadFileValid) { // Verify the reload was successful. - string expected = "[ { \"result\": 0, \"text\": " - "\"Configuration applied successfully.\" } ]"; + string expected = "{ \"result\": 0, \"text\": " + "\"Configuration applied successfully.\" }"; EXPECT_EQ(expected, answer->str()); // Check that the config was indeed applied? @@ -734,8 +727,8 @@ TEST_F(CtrlAgentControllerTest, shutdown) { params, cmd); // Verify the reload was successful. - string expected = "[ { \"result\": 0, \"text\": " - "\"Control Agent is shutting down\" } ]"; + string expected = "{ \"result\": 0, \"text\": " + "\"Control Agent is shutting down\" }"; EXPECT_EQ(expected, answer->str()); @@ -787,8 +780,8 @@ TEST_F(CtrlAgentControllerTest, shutdownExitValue) { params, cmd); // Verify the reload was successful. - string expected = "[ { \"result\": 0, \"text\": " - "\"Control Agent is shutting down\" } ]"; + string expected = "{ \"result\": 0, \"text\": " + "\"Control Agent is shutting down\" }"; EXPECT_EQ(expected, answer->str()); diff --git a/src/lib/config/base_command_mgr.h b/src/lib/config/base_command_mgr.h index 88a3099630..827ca6980f 100644 --- a/src/lib/config/base_command_mgr.h +++ b/src/lib/config/base_command_mgr.h @@ -112,9 +112,12 @@ public: /// After the command has been handled, callouts for the hook point, /// "command-processed" will be invoked. /// + /// This method is virtual so it can be overridden in derived classes to + /// pre-process command and post-process response if necessary. + /// /// @param cmd Pointer to the data element representing command in JSON /// format. - isc::data::ConstElementPtr + virtual isc::data::ConstElementPtr processCommand(const isc::data::ConstElementPtr& cmd); /// @brief Registers specified command handler for a given command