From 5916bcf3d3f5ccddf17f3b1da8b6e194d951d202 Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Mon, 19 May 2014 11:21:42 +0200 Subject: [PATCH] [3400] Changes after review: - files renamed to {kea,bundy}_controller.cc - Daemon::init() is now void - status variable removed from main - added unit-tests for broken configs --- src/bin/dhcp6/Makefile.am | 3 +- src/bin/dhcp6/ctrl_dhcp6_srv.h | 2 +- src/bin/dhcp6/kea_controller.cc | 54 ++++++------------- src/bin/dhcp6/main.cc | 8 +-- src/bin/dhcp6/tests/Makefile.am | 4 +- .../dhcp6/tests/kea_controller_unittest.cc | 49 ++++++++++++++++- src/lib/dhcpsrv/daemon.cc | 2 +- src/lib/dhcpsrv/daemon.h | 4 +- 8 files changed, 73 insertions(+), 53 deletions(-) diff --git a/src/bin/dhcp6/Makefile.am b/src/bin/dhcp6/Makefile.am index 60e9d0f33c..8f0dd76f12 100644 --- a/src/bin/dhcp6/Makefile.am +++ b/src/bin/dhcp6/Makefile.am @@ -3,7 +3,6 @@ SUBDIRS = . tests AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib AM_CPPFLAGS += -I$(top_srcdir)/src/bin -I$(top_builddir)/src/bin AM_CPPFLAGS += -I$(top_srcdir)/src/lib/cc -I$(top_builddir)/src/lib/cc -AM_CPPFLAGS += -DTOP_BUILDDIR="\"$(top_builddir)\"" AM_CPPFLAGS += $(BOOST_INCLUDES) AM_CXXFLAGS = $(B10_CXXFLAGS) @@ -63,7 +62,7 @@ b10_dhcp6_SOURCES += bundy_controller.cc endif if CONFIG_BACKEND_JSON -b10_dhcp6_SOURCES += jsonfile_controller.cc +b10_dhcp6_SOURCES += kea_controller.cc endif nodist_b10_dhcp6_SOURCES = dhcp6_messages.h dhcp6_messages.cc diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.h b/src/bin/dhcp6/ctrl_dhcp6_srv.h index 632007a041..c0a5a5709c 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.h +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.h @@ -54,7 +54,7 @@ public: /// *_backend.cc /// /// @return true if initialization was successful, false if it failed - bool init(const std::string& config_file); + void init(const std::string& config_file); /// @brief Performs cleanup, immediately before termination /// diff --git a/src/bin/dhcp6/kea_controller.cc b/src/bin/dhcp6/kea_controller.cc index 2196d23462..7e42df9ede 100644 --- a/src/bin/dhcp6/kea_controller.cc +++ b/src/bin/dhcp6/kea_controller.cc @@ -48,7 +48,7 @@ using namespace std; namespace isc { namespace dhcp { -bool +void ControlledDhcpv6Srv::init(const std::string& file_name) { // This is a configuration backend implementation that reads the // configuration from a JSON file. @@ -58,11 +58,13 @@ ControlledDhcpv6Srv::init(const std::string& file_name) { isc::data::ConstElementPtr result; // Basic sanity check: file name must not be empty. - if (file_name.empty()) { - isc_throw(BadValue, "JSON configuration file not specified"); - } - try { + if (file_name.empty()) { + // Basic sanity check: file name must not be empty. + isc_throw(BadValue, "JSON configuration file not specified. Please" + "use -c command line option."); + } + // Read contents of the file and parse it as JSON json = Element::fromJSONFile(file_name, true); @@ -99,7 +101,7 @@ ControlledDhcpv6Srv::init(const std::string& file_name) { LOG_ERROR(dhcp6_logger, DHCP6_CONFIG_LOAD_FAIL) .arg("Configuration failed: Undefined result of configureDhcp6Server" "() function after attempting to read " + file_name); - return (false); + return; } // Now check is the returned result is successful (rcode=0) or not @@ -112,14 +114,12 @@ ControlledDhcpv6Srv::init(const std::string& file_name) { reason = string(" (") + comment->stringValue() + string(")"); } LOG_ERROR(dhcp6_logger, DHCP6_CONFIG_LOAD_FAIL).arg(reason); - return (false); + isc_throw(BadValue, "Failed to apply configuration:" << reason); } // We don't need to call openActiveSockets() or startD2() as these // methods are called in processConfig() which is called by // processCommand("reload-config", ...) - - return (true); } void ControlledDhcpv6Srv::cleanup() { @@ -129,34 +129,14 @@ void ControlledDhcpv6Srv::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* log_name, bool verbose, bool ) { - // This method configures logger. For now it is very simple. - // We'll make it more robust once we add support for JSON-based logging - // configuration. - - using namespace isc::log; - - Severity severity = b10LoggerSeverity(verbose ? isc::log::DEBUG : isc::log::INFO); - - // Set a directory for creating lockfiles when running tests - // @todo: Find out why this is needed. Without this, the logger doesn't - // work. - setenv("B10_LOCKFILE_DIR_FROM_BUILD", TOP_BUILDDIR, 1); - - // Initialize logging - initLogger(log_name, severity, isc::log::MAX_DEBUG_LEVEL, NULL); - - // Now configure logger output to stdout. - /// @todo: Make this configurable as part of #3427. - LoggerSpecification spec(log_name, severity, - b10LoggerDbglevel(isc::log::MAX_DEBUG_LEVEL)); - OutputOption option; - option.destination = OutputOption::DEST_CONSOLE; - option.stream = OutputOption::STR_STDOUT; - - spec.addOutputOption(option); - LoggerManager manager; - manager.process(spec); +void Daemon::loggerInit(const char*, bool verbose, bool ) { + + setenv("B10_LOCKFILE_DIR_FROM_BUILD", "/tmp", 1); + setenv("B10_LOGGER_ROOT", "kea", 0); + setenv("B10_LOGGER_SEVERITY", (verbose ? "DEBUG":"INFO"), 0); + setenv("B10_LOGGER_DBGLEVEL", "99", 0); + setenv("B10_LOGGER_DESTINATION", "stdout", 0); + isc::log::initLogger(); } }; diff --git a/src/bin/dhcp6/main.cc b/src/bin/dhcp6/main.cc index 08740eb732..49c70ec319 100644 --- a/src/bin/dhcp6/main.cc +++ b/src/bin/dhcp6/main.cc @@ -105,7 +105,6 @@ main(int argc, char* argv[]) { int ret = EXIT_SUCCESS; - bool status = true; try { // Initialize logging. If verbose, we'll use maximum verbosity. // If standalone is enabled, do not buffer initial log messages @@ -123,8 +122,8 @@ main(int argc, char* argv[]) { try { // Initialize the server, i.e. establish control session // if BIND10 backend is used or read a configuration file - // - status = server.init(config_file); + server.init(config_file); + } catch (const std::exception& ex) { LOG_ERROR(dhcp6_logger, DHCP6_INIT_FAIL).arg(ex.what()); @@ -139,9 +138,6 @@ main(int argc, char* argv[]) { } else { LOG_DEBUG(dhcp6_logger, DBG_DHCP6_START, DHCP6_STANDALONE); } - if (!status) { - isc_throw(isc::Unexpected, "Failed to initialize configuration backend."); - } server.run(); LOG_INFO(dhcp6_logger, DHCP6_SHUTDOWN); diff --git a/src/bin/dhcp6/tests/Makefile.am b/src/bin/dhcp6/tests/Makefile.am index f09eb3c18b..1c8b6e5a69 100644 --- a/src/bin/dhcp6/tests/Makefile.am +++ b/src/bin/dhcp6/tests/Makefile.am @@ -96,8 +96,8 @@ dhcp6_unittests_SOURCES += bundy_controller_unittest.cc endif if CONFIG_BACKEND_JSON -dhcp6_unittests_SOURCES += ../jsonfile_controller.cc -dhcp6_unittests_SOURCES += jsonfile_controller_unittest.cc +dhcp6_unittests_SOURCES += ../kea_controller.cc +dhcp6_unittests_SOURCES += kea_controller_unittest.cc endif nodist_dhcp6_unittests_SOURCES = ../dhcp6_messages.h ../dhcp6_messages.cc diff --git a/src/bin/dhcp6/tests/kea_controller_unittest.cc b/src/bin/dhcp6/tests/kea_controller_unittest.cc index 1f70c042e1..3198561345 100644 --- a/src/bin/dhcp6/tests/kea_controller_unittest.cc +++ b/src/bin/dhcp6/tests/kea_controller_unittest.cc @@ -101,7 +101,7 @@ TEST_F(JSONFileBackendTest, jsonFile) { ); // And configure it using the config file. - EXPECT_TRUE(srv->init(TEST_FILE)); + EXPECT_NO_THROW(srv->init(TEST_FILE)); // Now check if the configuration has been applied correctly. const Subnet6Collection* subnets = CfgMgr::instance().getSubnets6(); @@ -171,7 +171,7 @@ TEST_F(JSONFileBackendTest, comments) { ); // And configure it using config without - EXPECT_TRUE(srv->init(TEST_FILE)); + EXPECT_NO_THROW(srv->init(TEST_FILE)); // Now check if the configuration has been applied correctly. const Subnet6Collection* subnets = CfgMgr::instance().getSubnets6(); @@ -190,4 +190,49 @@ TEST_F(JSONFileBackendTest, comments) { EXPECT_EQ(Lease::TYPE_NA, pools1.at(0)->getType()); } +// This test checks if configuration detects failure when trying: +// - empty file +// - empty filename +// - no Dhcp6 element +// - Config file that contains Dhcp6 but has a content error +TEST_F(JSONFileBackendTest, configBroken) { + + // Empty config is not allowed, because Dhcp6 element is missing + string config_empty = ""; + + // This config does not have mandatory Dhcp6 element + string config_v4 = "{ \"Dhcp4\": { \"interfaces\": [ \"*\" ]," + "\"preferred-lifetime\": 3000," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"subnet4\": [ { " + " \"pool\": [ \"192.0.2.0/24\" ]," + " \"subnet\": \"192.0.2.0/24\" " + " } ]}"; + + // This has Dhcp6 element, but it's utter nonsense + string config_nonsense = "{ \"Dhcp6\": { \"reviews\": \"are so much fun\" } }"; + + // Now initialize the server + boost::scoped_ptr srv; + ASSERT_NO_THROW( + srv.reset(new ControlledDhcpv6Srv(0)) + ); + + // Try to configure without filename. Should fail. + EXPECT_THROW(srv->init(""), BadValue); + + // Try to configure it using empty file. Should fail. + writeFile(TEST_FILE, config_empty); + EXPECT_THROW(srv->init(TEST_FILE), BadValue); + + // Now try to load a config that does not have Dhcp6 component. + writeFile(TEST_FILE, config_v4); + EXPECT_THROW(srv->init(TEST_FILE), BadValue); + + // Now try to load a config with Dhcp6 full of nonsense. + writeFile(TEST_FILE, config_nonsense); + EXPECT_THROW(srv->init(TEST_FILE), BadValue); +} + } // End of anonymous namespace diff --git a/src/lib/dhcpsrv/daemon.cc b/src/lib/dhcpsrv/daemon.cc index c4ccfb86e8..bb9566745e 100644 --- a/src/lib/dhcpsrv/daemon.cc +++ b/src/lib/dhcpsrv/daemon.cc @@ -27,7 +27,7 @@ namespace dhcp { Daemon::Daemon() { } -bool Daemon::init(const std::string&) { +void Daemon::init(const std::string&) { return false; } diff --git a/src/lib/dhcpsrv/daemon.h b/src/lib/dhcpsrv/daemon.h index fe17938ca5..7c14489044 100644 --- a/src/lib/dhcpsrv/daemon.h +++ b/src/lib/dhcpsrv/daemon.h @@ -61,8 +61,8 @@ public: /// likely that the D2 will be started, it will analyze its config file, /// decide that it is not needed and will shut down. /// - /// @return true if initialization was successful, false if it was not - virtual bool init(const std::string& config_file); + /// @note this method may throw + virtual void init(const std::string& config_file); /// @brief Performs final deconfiguration. /// -- GitLab