Commit 6eb72206 authored by Tomek Mrugalski's avatar Tomek Mrugalski 🛰
Browse files

[3399] Changes after review

 - ChangLog updated (b10-dhcp4 => DHCPv4 server)
 - stand-alone mode removed in Kea4,Kea6
 - getInstance() used instead of server_
 - ctrl_dhcp4_srv.cc: one stringstream instance now shared by all error messages
 - ctrl_dhcp4_srv.h: comments cleaned up
 - Added log message for received configuration
 - kea_controller.cc: Unnecessary header/using namespace removed
 - *.json files are now cleaned up in src/bin/dhcp4/tests
 - added comment that explains lack of tests for Bundy backend
 - kea_controller_unittest.cc now uses the same filename everywhere

Changes also applied to Kea6 (where applicable).
parent cd2c29f8
7XX. [func] tomek
DHCPv4 server: New parameter added to configure.ac: --with-kea-config.
It allows selecting configuration backend and accepts one of two
values: BUNDY, which uses Bundy (former BIND10) framework as Kea
0.8 did, or JSON, which reads configuration from a JSON file.
(Trac #3399, git TBD)
784. [func] tmark
DHCP_DDNS's configuration was changed. The unused parameter, "interface"
was deleted. Three new parameters, "ncr_protocol", "ncr_format", and
......@@ -6,7 +13,7 @@
(Trac #3268, git bd60252e679f19b062f61926647f661ab169f21c)
783. [func]* tomek
b10-dhcp6: New parameter added to configure: --with-kea-config.
DHCPv6 server: New parameter added to configure: --with-kea-config.
It allows selecting configuration backend and accepts one of two
values: BUNDY, which uses Bundy (former BIND10 framework as Kea
0.8 did, or JSON, which reads configuration from a JSON file.
......
# This is an example configuration file for DHCPv4 server in Kea.
# This is an example configuration file for the DHCPv4 server in Kea.
# It is a basic scenario with one IPv4 subnet configured. The subnet
# contains a single pool of dynamically allocated addresses.
......
......@@ -209,10 +209,10 @@ void ControlledDhcpv4Srv::cleanup() {
}
void
Daemon::loggerInit(const char* log_name, bool verbose, bool stand_alone) {
Daemon::loggerInit(const char* log_name, bool verbose) {
isc::log::initLogger(log_name,
(verbose ? isc::log::DEBUG : isc::log::INFO),
isc::log::MAX_DEBUG_LEVEL, NULL, !stand_alone);
isc::log::MAX_DEBUG_LEVEL, NULL, true);
}
};
......
......@@ -30,8 +30,8 @@ ControlledDhcpv4Srv* ControlledDhcpv4Srv::server_ = NULL;
ConstElementPtr
ControlledDhcpv4Srv::commandShutdownHandler(const string&, ConstElementPtr) {
if (ControlledDhcpv4Srv::server_) {
ControlledDhcpv4Srv::server_->shutdown();
if (ControlledDhcpv4Srv::getInstance()) {
ControlledDhcpv4Srv::getInstance()->shutdown();
} else {
LOG_WARN(dhcp4_logger, DHCP4_NOT_RUNNING);
ConstElementPtr answer = isc::config::createAnswer(1,
......@@ -76,8 +76,8 @@ ControlledDhcpv4Srv::processCommand(const string& command,
if (!srv) {
ConstElementPtr no_srv = isc::config::createAnswer(1,
"Server object not initialized, can't process command '" +
command + "'.");
"Server object not initialized, so can't process command '" +
command + "', arguments: '" + args->str() + "'.");
return (no_srv);
}
......@@ -103,16 +103,23 @@ ControlledDhcpv4Srv::processCommand(const string& command,
isc::data::ConstElementPtr
ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) {
LOG_DEBUG(dhcp4_logger, DBG_DHCP4_COMMAND, DHCP4_CONFIG_RECEIVED)
.arg(config->str());
ControlledDhcpv4Srv* srv = ControlledDhcpv4Srv::getInstance();
// Single stream instance used in all error clauses
std::ostringstream err;
if (!srv) {
ConstElementPtr no_srv = isc::config::createAnswer(1,
"Server object not initialized, can't process config.");
return (no_srv);
err << "Server object not initialized, can't process config.";
return (isc::config::createAnswer(1, err.str()));
}
ConstElementPtr answer = configureDhcp4Server(*srv, config);
// Check that configuration was successful. If not, do not reopen sockets
// and don't bother with DDNS stuff.
try {
......@@ -122,29 +129,28 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) {
return (answer);
}
} catch (const std::exception& ex) {
return (isc::config::createAnswer(1, "Failed to process configuration:"
+ string(ex.what())));
err << "Failed to process configuration:" << ex.what();
return (isc::config::createAnswer(1, err.str()));
}
// Server will start DDNS communications if its enabled.
try {
srv->startD2();
} catch (const std::exception& ex) {
std::ostringstream err;
err << "error starting DHCP_DDNS client "
" after server reconfiguration: " << ex.what();
err << "Error starting DHCP_DDNS client after server reconfiguration: "
<< ex.what();
return (isc::config::createAnswer(1, err.str()));
}
// Configuration may change active interfaces. Therefore, we have to reopen
// sockets according to new configuration. This operation is not exception
// safe and we really don't want to emit exceptions to the callback caller.
// Instead, catch an exception and create appropriate answer.
// safe and we really don't want to emit exceptions to whoever called this
// method. Instead, catch an exception and create appropriate answer.
try {
srv->openActiveSockets(srv->getPort(), server_->useBroadcast());
srv->openActiveSockets(srv->getPort(), getInstance()->useBroadcast());
} catch (std::exception& ex) {
std::ostringstream err;
err << "failed to open sockets after server reconfiguration: " << ex.what();
err << "failed to open sockets after server reconfiguration: "
<< ex.what();
answer = isc::config::createAnswer(1, err.str());
}
return (answer);
......@@ -152,11 +158,11 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) {
ControlledDhcpv4Srv::ControlledDhcpv4Srv(uint16_t port /*= DHCP4_SERVER_PORT*/)
:Dhcpv4Srv(port) {
if (server_) {
if (getInstance()) {
isc_throw(InvalidOperation,
"There is another Dhcpv4Srv instance already.");
}
server_ = this; // remember this instance for use in callback
server_ = this; // remember this instance for later use in handlers
}
void ControlledDhcpv4Srv::shutdown() {
......@@ -167,15 +173,15 @@ void ControlledDhcpv4Srv::shutdown() {
ControlledDhcpv4Srv::~ControlledDhcpv4Srv() {
cleanup();
server_ = NULL; // forget this instance. There should be no callback anymore
// at this stage anyway.
server_ = NULL; // forget this instance. Noone should call any handlers at
// this stage.
}
void ControlledDhcpv4Srv::sessionReader(void) {
// Process one asio event. If there are more events, iface_mgr will call
// this callback more than once.
if (server_) {
server_->io_service_.run_one();
if (getInstance()) {
getInstance()->io_service_.run_one();
}
}
......
......@@ -26,12 +26,10 @@ namespace dhcp {
/// @brief Controlled version of the DHCPv4 server
///
/// This is a class that is responsible for establishing connection
/// with msqg (receving commands and configuration). This is an extended
/// version of Dhcpv4Srv class that is purely a DHCPv4 server, without
/// external control. ControlledDhcpv4Srv should be used in typical BIND10
/// (i.e. featuring msgq) environment, while Dhcpv4Srv should be used in
/// embedded environments.
/// This is a class that is responsible for DHCPv4 server being controllable.
/// It does various things, depending on the configuration backend.
/// For Bundy backend it establishes a connection with msqg and later receives
/// commands over it. For Kea backend, it reads configuration file from disk.
///
/// For detailed explanation or relations between main(), ControlledDhcpv4Srv,
/// Dhcpv4Srv and other classes, see \ref dhcpv4Session.
......@@ -53,7 +51,8 @@ public:
/// operation. For specific details, see actual implementation in
/// *_backend.cc
///
/// @return true if initialization was successful, false if it failed
/// This method may throw if initialization fails. Exception types may be
/// specific to used configuration backend.
void init(const std::string& config_file);
/// @brief Performs cleanup, immediately before termination
......@@ -77,6 +76,11 @@ public:
/// wrapper that calls process*Command() methods and catches exceptions
/// in them.
///
/// Currently supported commands are:
/// - shutdown
/// - libreload
/// - config-reload
///
/// @note It never throws.
///
/// @param command Text represenation of the command (e.g. "shutdown")
......@@ -88,7 +92,7 @@ public:
/// @brief Configuration processor
///
/// This is a callback for handling incoming configuration updates.
/// This is a method for handling incoming configuration updates.
/// This method should be called by all configuration backends when the
/// server is starting up or when configuration has changed.
///
......@@ -103,7 +107,7 @@ public:
/// @brief Returns pointer to the sole instance of Dhcpv4Srv
///
/// @note may return NULL, if called before server is spawned
/// @return server instance (may return NULL, if called before server is spawned)
static ControlledDhcpv4Srv* getInstance() {
return (server_);
}
......
......@@ -44,6 +44,10 @@ name was malformed or due to internal server error.
A debug message listing the command (and possible arguments) received
from the BIND 10 control system by the DHCPv4 server.
% DHCP4_CONFIG_RECEIVED received configuration %1
A debug message listing the configuration received by the DHCPv4 server.
The source of that configuration depends on used configuration backend.
% DHCP4_CONFIG_COMPLETE DHCPv4 server has completed configuration: %1
This is an informational message announcing the successful processing of a
new configuration. It is output during server startup, and when an updated
......@@ -339,17 +343,11 @@ core component within the DHCPv4 server (the Dhcpv4 server object)
has failed. As a result, the server will exit. The reason for the
failure is given within the message.
% DHCP4_STANDALONE skipping message queue, running standalone
This is a debug message indicating that the DHCPv4 server is running in
standalone mode, not connected to the message queue. Standalone mode
is only useful during program development, and should not be used in a
production environment.
% DHCP4_STARTING server starting
This informational message indicates that the DHCPv4 server has
processed any command-line switches and is starting.
% DHCP4_START_INFO pid: %1, port: %2, verbose: %3, standalone: %4
% DHCP4_START_INFO pid: %1, port: %2, verbose: %3
This is a debug message issued during the DHCPv4 server startup.
It lists some information about the parameters with which the server
is running.
......
......@@ -14,35 +14,15 @@
#include <config.h>
#include <asiolink/asiolink.h>
#include <dhcp/iface_mgr.h>
#include <dhcpsrv/dhcp_config_parser.h>
#include <dhcpsrv/cfgmgr.h>
#include <dhcp4/json_config_parser.h>
#include <dhcp4/ctrl_dhcp4_srv.h>
#include <dhcp4/dhcp4_log.h>
#include <dhcp4/spec_config.h>
#include <log/logger_level.h>
#include <log/logger_name.h>
#include <log/logger_manager.h>
#include <log/logger_specification.h>
#include <log/logger_support.h>
#include <log/output_option.h>
#include <exceptions/exceptions.h>
#include <util/buffer.h>
#include <cassert>
#include <iostream>
#include <string>
#include <vector>
using namespace isc::asiolink;
using namespace isc::cc;
using namespace isc::config;
using namespace isc::data;
using namespace isc::dhcp;
using namespace isc::log;
using namespace isc::util;
using namespace std;
namespace isc {
......@@ -66,7 +46,7 @@ ControlledDhcpv4Srv::init(const std::string& file_name) {
}
// Read contents of the file and parse it as JSON
json = Element::fromJSONFile(file_name, true);
json = isc::data::Element::fromJSONFile(file_name, true);
if (!json) {
LOG_ERROR(dhcp4_logger, DHCP4_CONFIG_LOAD_FAIL)
......@@ -105,9 +85,9 @@ ControlledDhcpv4Srv::init(const std::string& file_name) {
}
// Now check is the returned result is successful (rcode=0) or not
ConstElementPtr comment; /// see @ref isc::config::parseAnswer
isc::data::ConstElementPtr comment; /// see @ref isc::config::parseAnswer
int rcode;
comment = parseAnswer(rcode, result);
comment = isc::config::parseAnswer(rcode, result);
if (rcode != 0) {
string reason = "";
if (comment) {
......@@ -129,7 +109,7 @@ void ControlledDhcpv4Srv::cleanup() {
/// This is a logger initialization for JSON file backend.
/// For now, it's just setting log messages to be printed on stdout.
/// @todo: Implement this properly (see #3427)
void Daemon::loggerInit(const char*, bool verbose, bool ) {
void Daemon::loggerInit(const char*, bool verbose) {
setenv("B10_LOCKFILE_DIR_FROM_BUILD", "/tmp", 1);
setenv("B10_LOGGER_ROOT", "kea", 0);
......
......@@ -43,9 +43,8 @@ const char* const DHCP4_LOGGER_NAME = "kea";
void
usage() {
cerr << "Usage: " << DHCP4_NAME << " [-v] [-s] [-p number]" << endl;
cerr << "Usage: " << DHCP4_NAME << " [-v] [-p number] [-c file]" << endl;
cerr << " -v: verbose output" << endl;
cerr << " -s: stand-alone mode (don't connect to BIND10)" << endl;
cerr << " -p number: specify non-standard port number 1-65535 "
<< "(useful for testing only)" << endl;
cerr << " -c file: specify configuration file" << endl;
......@@ -58,22 +57,17 @@ main(int argc, char* argv[]) {
int ch;
int port_number = DHCP4_SERVER_PORT; // The default. any other values are
// useful for testing only.
bool stand_alone = false; // Should be connect to BIND10 msgq?
bool verbose_mode = false; // Should server be verbose?
// The standard config file
std::string config_file("");
while ((ch = getopt(argc, argv, "vsp:c:")) != -1) {
while ((ch = getopt(argc, argv, "vp:c:")) != -1) {
switch (ch) {
case 'v':
verbose_mode = true;
break;
case 's':
stand_alone = true;
break;
case 'p':
try {
port_number = boost::lexical_cast<int>(optarg);
......@@ -108,35 +102,34 @@ main(int argc, char* argv[]) {
try {
// Initialize logging. If verbose, we'll use maximum verbosity.
// If standalone is enabled, do not buffer initial log messages
Daemon::loggerInit(DHCP4_LOGGER_NAME, verbose_mode, stand_alone);
Daemon::loggerInit(DHCP4_LOGGER_NAME, verbose_mode);
LOG_DEBUG(dhcp4_logger, DBG_DHCP4_START, DHCP4_START_INFO)
.arg(getpid()).arg(port_number).arg(verbose_mode ? "yes" : "no")
.arg(stand_alone ? "yes" : "no" );
.arg(getpid()).arg(port_number).arg(verbose_mode ? "yes" : "no");
LOG_INFO(dhcp4_logger, DHCP4_STARTING);
// Create the server instance.
ControlledDhcpv4Srv server(port_number);
if (!stand_alone) {
try {
server.init(config_file);
} catch (const std::exception& ex) {
LOG_ERROR(dhcp4_logger, DHCP4_SESSION_FAIL).arg(ex.what());
try {
// Initialize the server.
server.init(config_file);
} catch (const std::exception& ex) {
LOG_ERROR(dhcp4_logger, DHCP4_SESSION_FAIL).arg(ex.what());
// We should not continue if were told to configure (either read
// config file or establish Bundy control session).
// We should not continue if were told to configure (either read
// config file or establish Bundy control session).
isc::log::LoggerManager log_manager;
log_manager.process();
isc::log::LoggerManager log_manager;
log_manager.process();
cerr << "Failed to initialize server: " << ex.what() << endl;
return (EXIT_FAILURE);
}
} else {
LOG_DEBUG(dhcp4_logger, DBG_DHCP4_START, DHCP4_STANDALONE);
cerr << "Failed to initialize server: " << ex.what() << endl;
return (EXIT_FAILURE);
}
// And run the main loop of the server.
server.run();
LOG_INFO(dhcp4_logger, DHCP4_SHUTDOWN);
} catch (const std::exception& ex) {
......
......@@ -33,6 +33,7 @@ AM_CPPFLAGS += -DINSTALL_PROG=\"$(abs_top_srcdir)/install-sh\"
CLEANFILES = $(builddir)/interfaces.txt $(builddir)/logger_lockfile
CLEANFILES += $(builddir)/load_marker.txt $(builddir)/unload_marker.txt
CLEANFILES += *.json
AM_CXXFLAGS = $(B10_CXXFLAGS)
if USE_CLANGPP
......
......@@ -18,6 +18,10 @@
namespace {
/// As of May 2014, maintaining or extending Bundy support is very low
/// prority for Kea team. We are looking for contributors, who would
/// like to maintain this backend.
// Bundy framework specific tests should be added here.
TEST(BundyBackendTest, dummy) {
......
......@@ -13,7 +13,7 @@
# NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
# WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
from init import ProcessInfo, parse_args, dump_pid, unlink_pid_file, _BASETIME
from init import ProcessInfo
import unittest
import sys
......@@ -207,22 +207,5 @@ class TestDhcpv4Daemon(unittest.TestCase):
# Check that there is an error message about invalid port number printed on stderr
self.assertEqual( str(error).count("Failed to parse port number"), 1)
def test_portnumber_nonroot(self):
print("Check that specifying unprivileged port number will work.")
# Check that there is a message about running with an unprivileged port
(returncode, output, error) = self.runCommand(['../b10-dhcp4', '-v', '-s', '-p', '10057'])
output_text = str(output) + str(error)
self.assertEqual(output_text.count("DHCP4_OPEN_SOCKET opening sockets on port 10057"), 1)
def test_skip_msgq(self):
print("Check that connection to BIND10 msgq can be disabled.")
# Check that the system outputs a message on one of its streams about running
# standalone.
(returncode, output, error) = self.runCommand(['../b10-dhcp4', '-v', '-s', '-p', '10057'])
output_text = str(output) + str(error)
self.assertEqual(output_text.count("DHCP4_STANDALONE"), 1)
if __name__ == '__main__':
unittest.main()
......@@ -45,7 +45,9 @@ public:
NakedControlledDhcpv4Srv():ControlledDhcpv4Srv(0) { }
};
/// @brief test class for Kea configuration backend
///
/// This class is used for testing
class JSONFileBackendTest : public ::testing::Test {
public:
JSONFileBackendTest() {
......@@ -55,10 +57,10 @@ public:
static_cast<void>(unlink(TEST_FILE));
};
void writeFile(const std::string& file_name, const std::string& content) {
static_cast<void>(unlink(file_name.c_str()));
void writeFile(const std::string& content) {
static_cast<void>(unlink(TEST_FILE));
ofstream out(file_name.c_str(), ios::trunc);
ofstream out(TEST_FILE, ios::trunc);
EXPECT_TRUE(out.is_open());
out << content;
out.close();
......@@ -92,7 +94,7 @@ TEST_F(JSONFileBackendTest, jsonFile) {
"\"valid-lifetime\": 4000 }"
"}";
writeFile(TEST_FILE, config);
writeFile(config);
// Now initialize the server
boost::scoped_ptr<ControlledDhcpv4Srv> srv;
......@@ -161,7 +163,7 @@ TEST_F(JSONFileBackendTest, comments) {
/// @todo: Implement C++-style (// ...) comments
/// @todo: Implement C-style (/* ... */) comments
writeFile(TEST_FILE, config_hash_comments);
writeFile(config_hash_comments);
// Now initialize the server
boost::scoped_ptr<ControlledDhcpv4Srv> srv;
......@@ -169,7 +171,7 @@ TEST_F(JSONFileBackendTest, comments) {
srv.reset(new ControlledDhcpv4Srv(0))
);
// And configure it using config without
// And configure it using config with comments.
EXPECT_NO_THROW(srv->init(TEST_FILE));
// Now check if the configuration has been applied correctly.
......@@ -222,34 +224,34 @@ TEST_F(JSONFileBackendTest, configBroken) {
EXPECT_THROW(srv->init(""), BadValue);
// Try to configure it using empty file. Should fail.
writeFile(TEST_FILE, config_empty);
writeFile(config_empty);
EXPECT_THROW(srv->init(TEST_FILE), BadValue);
// Now try to load a config that does not have Dhcp4 component.
writeFile(TEST_FILE, config_v4);
writeFile(config_v4);
EXPECT_THROW(srv->init(TEST_FILE), BadValue);
// Now try to load a config with Dhcp4 full of nonsense.
writeFile(TEST_FILE, config_nonsense);
writeFile(config_nonsense);
EXPECT_THROW(srv->init(TEST_FILE), BadValue);
}
// This unit-test reads all files enumerated in configs-test.txt file, loads
// each of them and verify that they can be loaded.
//
// @todo: Unfortunately, we have this test disabled, because all loaded
// configs use memfile, which attempts to create lease file in
// /usr/local/var/bind10/kea-leases4.csv. We have couple options here:
// a) disable persistence in example configs - a very bad thing to do
// as users will forget to reenable it and then will be surprised when their
// leases disappear
// b) change configs to store lease file in /tmp. It's almost as bad as the
// previous one. Users will then be displeased when all their leases are
// wiped. (most systems wipe /tmp during boot)
// c) read each config and rewrite it on the fly, so persistence is disabled.
// This is probably the way to go, but this is a work for a dedicated ticket.
//
// Hence I'm leaving the test in, but it is disabled.
/// This unit-test reads all files enumerated in configs-test.txt file, loads
/// each of them and verify that they can be loaded.
///
/// @todo: Unfortunately, we have this test disabled, because all loaded
/// configs use memfile, which attempts to create lease file in
/// /usr/local/var/bind10/kea-leases4.csv. We have couple options here:
/// a) disable persistence in example configs - a very bad thing to do
/// as users will forget to reenable it and then will be surprised when their
/// leases disappear
/// b) change configs to store lease file in /tmp. It's almost as bad as the
/// previous one. Users will then be displeased when all their leases are
/// wiped. (most systems wipe /tmp during boot)
/// c) read each config and rewrite it on the fly, so persistence is disabled.
/// This is probably the way to go, but this is a work for a dedicated ticket.
///
/// Hence I'm leaving the test in, but it is disabled.
TEST_F(JSONFileBackendTest, DISABLED_loadAllConfigs) {
// Create server first
......
......@@ -217,10 +217,10 @@ void ControlledDhcpv6Srv::cleanup() {
}
void
Daemon::loggerInit(const char* log_name, bool verbose, bool stand_alone) {
Daemon::loggerInit(const char* log_name, bool verbose) {
isc::log::initLogger(log_name,
(verbose ? isc::log::DEBUG : isc::log::INFO),
isc::log::MAX_DEBUG_LEVEL, NULL, !stand_alone);
isc::log::MAX_DEBUG_LEVEL, NULL, true);
}
}; // end of isc::dhcp namespace
......
......@@ -99,9 +99,12 @@ ControlledDhcpv6Srv::processCommand(const std::string& command,
}
}
isc::data::ConstElementPtr
ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) {
LOG_DEBUG(dhcp6_logger, DBG_DHCP6_COMMAND, DHCP6_CONFIG_RECEIVED)
.arg(config->str());
ControlledDhcpv6Srv* srv = ControlledDhcpv6Srv::getInstance();
if (!srv) {
......
......@@ -26,12 +26,10 @@ namespace dhcp {
/// @brief Controlled version of the DHCPv6 server
///
/// This is a class that is responsible for establishing connection
/// with msqg (receving commands and configuration). This is an extended
/// version of Dhcpv6Srv class that is purely a DHCPv6 server, without
/// external control. ControlledDhcpv6Srv should be used in typical BIND10
/// (i.e. featuring msgq) environment, while Dhcpv6Srv should be used in
/// embedded environments.
/// This is a class that is responsible for DHCPv6 server being controllable.
/// It does various things, depending on the configuration backend.
/// For Bundy backend it establishes a connection with msqg and later receives
/// commands over it. For Kea backend, it reads configuration file from disk.
///
/// For detailed explanation or relations between main(), ControlledDhcpv6Srv,
/// Dhcpv6Srv and other classes, see \ref dhcpv6Session.
......@@ -53,7 +51,8 @@ public:
/// operation. For specific details, see actual implementation in
/// *_backend.cc
///
/// @return true if initialization was successful, false if it failed
/// This method may throw if initialization fails. Exception types may be
/// specific to used configuration backend.
void init(const std::string& config_file);
/// @brief Performs cleanup, immediately before termination
......@@ -77,6 +76,11 @@ public:
/// wrapper that calls process*Command() methods and catches exceptions