Commit 41b560cc authored by Marcin Siodelski's avatar Marcin Siodelski
Browse files

[5078] Addressed review comments.

parent bcb0eac2
......@@ -15,12 +15,14 @@
#include <cc/command_interpreter.h>
#include <cc/data.h>
#include <boost/pointer_cast.hpp>
#include <iterator>
#include <string>
#include <vector>
using namespace isc::asiolink;
using namespace isc::config;
using namespace isc::data;
using namespace isc::hooks;
using namespace isc::process;
namespace isc {
......@@ -40,66 +42,99 @@ ConstElementPtr
CtrlAgentCommandMgr::handleCommand(const std::string& cmd_name,
const isc::data::ConstElementPtr& params,
const isc::data::ConstElementPtr& original_cmd) {
ConstElementPtr answer;
ConstElementPtr answer = handleCommandInternal(cmd_name, params, original_cmd);
try {
// list-commands is a special case. The Control Agent always supports this
// command but most of the time users don't want to list commands supported
// by the CA but by one of the Kea servers. The user would indicate that
// by specifying 'service' value.
if (cmd_name == "list-commands") {
if (original_cmd && original_cmd->contains("service")) {
ConstElementPtr services = original_cmd->get("service");
if (services && !services->empty()) {
// The non-empty control command 'service' parameter exists which
// means we will forward this command to the Kea server. Let's
// cheat that Control Agent doesn't support this command to
// avoid it being handled by CA.
"forwarding list-commands command");
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.
ElementPtr answer_list = Element::createList();
return (answer_list);
CtrlAgentCommandMgr::handleCommandInternal(std::string cmd_name,
isc::data::ConstElementPtr params,
isc::data::ConstElementPtr original_cmd) {
ConstElementPtr services = Element::createList();
// Retrieve 'service' parameter to determine if we should forward the
// command or handle it on our own.
if (original_cmd && original_cmd->contains("service")) {
services = original_cmd->get("service");
// If 'service' value is not a list, this is a fatal error. We don't want
// to try processing commands that don't adhere to the required format.
if (services->getType() != Element::list) {
return (createAnswer(CONTROL_RESULT_ERROR, "service value must be a list"));
} catch (const std::exception& ex) {
answer = createAnswer(CONTROL_RESULT_ERROR, "invalid service parameter value: "
+ std::string(ex.what()));
if (!answer) {
// Try handling this command on our own.
answer = HookedCommandMgr::handleCommand(cmd_name, params, original_cmd);
// 'service' parameter hasn't been specified which indicates that the command
// is intended to be processed by the CA. The following command will try to
// process the command with hooks libraries (if available) or by one of the
// CA's native handlers.
if (services->empty()) {
return (HookedCommandMgr::handleCommand(cmd_name, params, original_cmd));
int rcode = 0;
static_cast<void>(parseAnswer(rcode, answer));
ElementPtr answer_list = Element::createList();
// We have tried handling the command on our own but it seems that neither
// the Control Agent nor a hook library can handle this command. We need
// to try forwarding the command to one of the Kea servers.
if (original_cmd && (rcode == CONTROL_RESULT_COMMAND_UNSUPPORTED)) {
try {
answer = tryForwardCommand(cmd_name, original_cmd);
// Before the command is forwarded it should be processed by the hooks libraries.
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,
return (answer_list);
} catch (const CommandForwardingError& ex) {
// This is apparently some configuration error or client's error.
// We have notify the client.
answer = createAnswer(CONTROL_RESULT_ERROR, ex.what());
// We don't know whether the hooks libraries modified the value of the
// answer list, so let's be safe and re-create the answer_list.
answer_list = Element::createList();
// For each value within 'service' we have to try forwarding the command.
for (unsigned i = 0; i < services->size(); ++i) {
if (original_cmd) {
ConstElementPtr answer;
try {
LOG_DEBUG(agent_logger, isc::log::DBGLVL_COMMAND,
answer = forwardCommand(services->get(i)->stringValue(),
cmd_name, original_cmd);
} catch (const CommandForwardingError& ex) {
LOG_DEBUG(agent_logger, isc::log::DBGLVL_COMMAND,
answer = createAnswer(CONTROL_RESULT_ERROR, ex.what());
} catch (const CommandForwardingSkip& ex) {
// Command is not intended to be forwarded so do nothing.
// We have a response, so let's wrap it in the list.
ElementPtr answer_list = Element::createList();
return (answer_list);
CtrlAgentCommandMgr::tryForwardCommand(const std::string& cmd_name,
const isc::data::ConstElementPtr& command) {
CtrlAgentCommandMgr::forwardCommand(const std::string& service,
const std::string& cmd_name,
const isc::data::ConstElementPtr& command) {
// Context will hold the server configuration.
CtrlAgentCfgContextPtr ctx;
......@@ -126,42 +161,11 @@ CtrlAgentCommandMgr::tryForwardCommand(const std::string& cmd_name,
" Control Agent configuration information");
// If the service is not specified it means that the Control Agent is the
// intended receiver of this message. This is not a fatal error, we simply
// skip forwarding the command and rely on the internal logic of the
// Control Agent to generate response.
ConstElementPtr service_elements = command->get("service");
if (!service_elements) {
isc_throw(CommandForwardingSkip, "service parameter not specified");
// If the service exists it must be a list, even though we currently allow
// only one service.
std::vector<ElementPtr> service_vec;
try {
service_vec = service_elements->listValue();
} catch (const std::exception& ex) {
isc_throw(CommandForwardingError, "service parameter is not a list");
// service list may be empty in which case we treat it as it is not specified.
if (service_vec.empty()) {
isc_throw(CommandForwardingSkip, "service parameter is empty");
// Do not allow more than one service value. This will be allowed in the
// future.
if (service_vec.size() > 1) {
isc_throw(CommandForwardingError, "service parameter must contain 0 or 1"
" service value");
// Convert the service to the server type values. Make sure the client
// provided right value.
CtrlAgentCfgContext::ServerType server_type;
try {
server_type = CtrlAgentCfgContext::toServerType(>stringValue());
server_type = CtrlAgentCfgContext::toServerType(service);
} catch (const std::exception& ex) {
// Invalid value in service list. Can't proceed.
......@@ -174,7 +178,7 @@ CtrlAgentCommandMgr::tryForwardCommand(const std::string& cmd_name,
ConstElementPtr socket_info = ctx->getControlSocketInfo(server_type);
if (!socket_info) {
isc_throw(CommandForwardingError, "forwarding socket is not configured"
" for the server type " <<>stringValue());
" for the server type " << service);
// If the configuration does its job properly the socket-name must be
......@@ -191,9 +195,9 @@ CtrlAgentCommandMgr::tryForwardCommand(const std::string& cmd_name,
unix_socket.write(&wire_command[0], wire_command.size());
receive_len = unix_socket.receive(&receive_buf_[0], receive_buf_.size());
} catch (...) {
} catch (const std::exception& ex) {
isc_throw(CommandForwardingError, "unable to forward command to the "
+>stringValue() + " service. The server "
<< service << " service: " << ex.what() << ". The server "
"is likely to be offline");
......@@ -212,8 +216,7 @@ CtrlAgentCommandMgr::tryForwardCommand(const std::string& cmd_name,
answer = Element::fromJSON(reply);
} catch (const std::exception& ex) {
isc_throw(CommandForwardingError, "internal server error: unable to parse"
......@@ -24,13 +24,6 @@ public:
isc::Exception(file, line, what) { };
/// @brief Exception thrown when command forwarding has been skipped.
class CommandForwardingSkip : public Exception {
CommandForwardingSkip(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) { };
/// @brief Command Manager for Control Agent.
/// This is an implementation of the Command Manager within Control Agent.
......@@ -54,18 +47,24 @@ public:
/// @brief Handles the command having a given name and arguments.
/// This method extends the base implementation with the ability to forward
/// commands to Kea servers if the Control Agent failed to handle it itself.
/// commands to Kea servers.
/// @todo Currently this method only wraps an answer within a list Element.
/// This will be later used to include multiple answers within this list.
/// For now it is just a single answer from the Control Agent.
/// If the received command doesn't include 'service' parameter or this
/// parameter is blank, the command is handled by the Control Agent or the
/// attached hooks libraries.
/// If the non-blank 'service' parameter has been specified the callouts
/// are executed. If the callouts process the command the result is returned
/// to the controlling client. Otherwise, the command is forwarded to each
/// Kea server listed in the 'service' parameter.
/// @param cmd_name Command name.
/// @param params Command arguments.
/// @param original_cmd Original command being processed.
/// @return Pointer to the const data element representing response
/// to a command.
/// @return Pointer to the const data element representing a list of
/// responses to the command. If the command has been handled by the CA,
/// this list includes one response.
virtual isc::data::ConstElementPtr
handleCommand(const std::string& cmd_name,
const isc::data::ConstElementPtr& params,
......@@ -73,34 +72,42 @@ public:
/// @brief Tries to forward received control command to Kea servers.
/// @brief Implements the logic for @ref CtrlAgentCommandMgr::handleCommand.
/// When the Control Agent was unable to process the control command
/// because it doesn't recognize it, the command should be forwarded to
/// the specific Kea services listed within a 'service' parameter.
/// All parameters are passed by value because they may be modified within
/// the method.
/// @todo Currently only one service per control command is supported.
/// Forwarding to multiple services should be allowed in the future.
/// @param cmd_name Command name.
/// @param params Command arguments.
/// @param original_cmd Original command being processed.
/// This method makes an attempt to forward the control command. If
/// the 'service' parameter is not specified or it is empty, the
/// command is not forwarded and the @ref CommandForwardingSkip exception
/// is thrown. The caller catching this exception should not treat
/// this situation as an error but this is normal situation when the
/// message is not intended to be forwarded.
/// @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.
handleCommandInternal(std::string cmd_name,
isc::data::ConstElementPtr params,
isc::data::ConstElementPtr original_cmd);
/// @brief Tries to forward received control command to Kea servers.
/// All other exceptions should be treated as an error.
/// When the Control Agent was unable to process the control command
/// because it doesn't recognize it, the command should be forwarded to
/// the specific Kea services listed within a 'service' parameter. This
/// method forwards the command to the specified Kea service.
/// @param service Contains name of the service where the command should be
/// forwarded.
/// @param cmd_name Command name.
/// @param command Pointer to the object representing the forwarded command.
/// @return Response to forwarded command.
/// @throw CommandForwardingError when an error occurred during forwarding.
/// @throw CommandForwardingSkip when 'service' parameter hasn't been
/// specified which means that the command should not be forwarded.
tryForwardCommand(const std::string& cmd_name,
const isc::data::ConstElementPtr& command);
forwardCommand(const std::string& destination,
const std::string& cmd_name,
const isc::data::ConstElementPtr& command);
/// @brief Private constructor.
......@@ -21,8 +21,6 @@ namespace agent {
class CtrlAgentController : public process::DControllerBase {
using DControllerBase::getIOService;
/// @brief Static singleton instance method.
/// This method returns the base class singleton instance member.
......@@ -24,10 +24,24 @@ This is a debug message issued when the Control Agent exits its
event loop.
% CTRL_AGENT_STARTED Kea Control Agent version %1 started
This informational message indicates that the DHCP-DDNS server has
This informational message indicates that the Control Agent has
processed all configuration information and is ready to begin processing.
The version is also printed.
% CTRL_AGENT_COMMAND_FORWARD_BEGIN begin forwarding command %1 to service %2
This debug message is issued when the Control Agent starts forwarding a
received command to one of the Kea servers.
% CTRL_AGENT_COMMAND_FORWARD_FAILED failed forwarding command %1: %2
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_FAIL Control Agent configuration failed: %1
This error message indicates that the CA had failed configuration
attempt. Details are provided. Additional details may be available
......@@ -55,11 +55,12 @@ CtrlAgentProcess::run() {
CtrlAgentCfgContextPtr ctx =
// Let's process incoming data or expiring timers in a loop until
// shutdown condition is detected.
while (!shouldShutdown()) {
// Remove unused listeners within the main loop because new listeners
// are created in within a callback method. This avoids removal the
// listeners within a callback.
......@@ -21,6 +21,7 @@
#include <boost/pointer_cast.hpp>
#include <gtest/gtest.h>
#include <cstdlib>
#include <vector>
using namespace isc::agent;
using namespace isc::asiolink;
......@@ -68,18 +69,40 @@ public:
/// verification of the response parameters.
/// @param answer answer to be verified
/// @param expected_code code expected to be returned in the answer
void checkAnswer(ConstElementPtr answer, int expected_code) {
/// @param expected_code0 code expected to be returned in first result within
/// the answer.
/// @param expected_code1 code expected to be returned in second result within
/// the answer.
/// @param expected_code2 code expected to be returned in third result within
/// the answer.
void checkAnswer(const ConstElementPtr& answer, const int expected_code0 = 0,
const int expected_code1 = -1, const int expected_code2 = -1) {
std::vector<int> expected_codes;
if (expected_code0 >= 0) {
if (expected_code1 >= 0) {
if (expected_code2 >= 0) {
int status_code;
// There may be multiple answers returned within a list.
std::vector<ElementPtr> answer_list = answer->listValue();
// There must be at least one answer.
ASSERT_GE(answer_list.size(), 1);
// Check that all answers indicate success.
ASSERT_EQ(expected_codes.size(), answer_list.size());
// Check all answers.
for (auto ans = answer_list.cbegin(); ans != answer_list.cend();
++ans) {
ASSERT_NO_THROW(isc::config::parseAnswer(status_code, *ans));
EXPECT_EQ(expected_code, status_code);
ConstElementPtr text;
ASSERT_NO_THROW(text = isc::config::parseAnswer(status_code, *ans));
EXPECT_EQ(expected_codes[std::distance(answer_list.cbegin(), ans)],
<< "answer contains text: " << text->stringValue();
......@@ -148,32 +171,34 @@ public:
/// @param response Stub response to be sent from the server socket to the
/// client.
void bindServerSocket(const std::string& response) {
/// @param stop_after_count Number of received messages received over the
/// server socket after which the IO service should be stopped.
void bindServerSocket(const std::string& response,
const unsigned int stop_after_count = 1) {
server_socket_.reset(new test::TestServerUnixSocket(*getIOService(),
/// @brief Creates command with no arguments.
/// @param command_name Command name.
/// @param service Service value to be added to the command. If this value
/// holds an empty string, the service parameter is not added.
/// @param service Service value to be added to the command. This value is
/// specified as a list of comma separated values, e.g. "dhcp4, dhcp6".
/// @return Pointer to the instance of the created command.
ConstElementPtr createCommand(const std::string& command_name,
const std::string& service) {
ElementPtr command = Element::createMap();
command->set("command", Element::create(command_name));
// Only add the 'service' parameter if non-empty.
if (!service.empty()) {
ElementPtr services = Element::createList();
command->set("service", services);
std::string s = boost::replace_all_copy(service, ",", "\",\"");
s = std::string("[ \"") + s + std::string("\" ]");
command->set("service", Element::fromJSON(s));
command->set("arguments", Element::createMap());
......@@ -186,16 +211,23 @@ public:
/// @param server_type Server for which the client socket should be
/// configured.
/// @param service Service to be included in the command.
/// @param expected_result Expected result in response from the server.
/// @param expected_result0 Expected first result in response from the server.
/// @param expected_result1 Expected second result in response from the server.
/// @param expected_result2 Expected third result in response from the server.
/// @param stop_after_count Number of received messages received over the
/// server socket after which the IO service should be stopped.
/// @param server_response Stub response to be sent by the server.
void testForward(const CtrlAgentCfgContext::ServerType& server_type,
const std::string& service,
const int expected_result,
const int expected_result0,
const int expected_result1 = -1,
const int expected_result2 = -1,
const unsigned stop_after_count = 1,
const std::string& server_response = "{ \"result\": 0 }") {
// Configure client side socket.
// Create server side socket.
bindServerSocket(server_response, stop_after_count);
// The client side communication is synchronous. To be able to respond
// to this we need to run the server side socket at the same time.
......@@ -208,7 +240,7 @@ public:
ConstElementPtr answer = mgr_.handleCommand("foo", ConstElementPtr(),
checkAnswer(answer, expected_result);
checkAnswer(answer, expected_result0, expected_result1, expected_result2);
/// @brief a convenience reference to control agent command manager
......@@ -249,6 +281,24 @@ TEST_F(CtrlAgentCommandMgrTest, forwardToDHCPv6Server) {
/// Check that the same command is forwarded to multiple servers.
TEST_F(CtrlAgentCommandMgrTest, forwardToBothDHCPServers) {
testForward(CtrlAgentCfgContext::TYPE_DHCP4, "dhcp4,dhcp6",
-1, 2);
/// Check that the command may forwarded to the second server even if
/// forwarding to a first server fails.
TEST_F(CtrlAgentCommandMgrTest, failForwardToServer) {
testForward(CtrlAgentCfgContext::TYPE_DHCP6, "dhcp4,dhcp6",
/// Check that control command is not forwarded if the service is not specified.
TEST_F(CtrlAgentCommandMgrTest, noService) {
testForward(CtrlAgentCfgContext::TYPE_DHCP6, "",
......@@ -259,7 +309,7 @@ TEST_F(CtrlAgentCommandMgrTest, noService) {
/// command was forwarded sent an invalid message.
TEST_F(CtrlAgentCommandMgrTest, invalidAnswer) {
testForward(CtrlAgentCfgContext::TYPE_DHCP6, "dhcp6",
isc::config::CONTROL_RESULT_ERROR, -1, -1, 1,
"{ \"result\": 0");
......@@ -21,19 +21,21 @@ TestServerUnixSocket::TestServerUnixSocket(IOService& io_service,
custom_response_(custom_response) {
read_count_(0) {
test_timer_.setup(boost::bind(&TestServerUnixSocket::timeoutHandler, this),
test_timeout, IntervalTimer::ONE_SHOT);
TestServerUnixSocket::bindServerSocket() {
TestServerUnixSocket::bindServerSocket(const unsigned int stop_after_count) {;
acceptHandler, this, _1));
stop_after_count_ = stop_after_count;
......@@ -44,19 +46,40 @@ TestServerUnixSocket::acceptHandler(const boost::system::error_code&) {
readHandler, this, _1, _2));
TestServerUnixSocket::accept() {
acceptHandler, this, _1));
TestServerUnixSocket::readHandler(const boost::system::error_code&,
size_t bytes_transferred) {
if (!custom_response_.empty()) {
boost::asio::write(server_socket_, boost::asio::buffer(custom_response_.c_str(),
} else {
std::string received(&raw_buf_[0], bytes_transferred);
std::string response("received " + received);
boost::asio::write(server_socket_, boost::asio::buffer(response.c_str(),