From: Marcin Siodelski Date: Wed, 20 Mar 2019 10:36:34 +0000 (+0100) Subject: [#103,!277] Setup timer for CB config fetch in the DHCPv4 server. X-Git-Tag: Kea-1.6.0-beta~323 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d60636d4e4ada1584769f6a588e43cdbdf6c6778;p=thirdparty%2Fkea.git [#103,!277] Setup timer for CB config fetch in the DHCPv4 server. --- diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 0ab41233b4..6f3d40d44c 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2019 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 @@ -674,6 +674,32 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) { return (isc::config::createAnswer(1, err.str())); } + auto ctl_info = CfgMgr::instance().getStagingCfg()->getConfigControlInfo(); + if (ctl_info) { + long fetch_time = static_cast(ctl_info->getConfigFetchWaitTime()); + // Only schedule the CB fetch timer if the fetch wait time is greater + // than 0. + if (fetch_time > 0) { + // The port number is 0 only when we run unit tests. In such case, we + // want to use milliseconds unit for the specified interval. Otherwise, + // we use seconds. Note that using milliseconds as a unit in unit tests + // prevents us from waiting 1 second on more before the timer goes off. + // Instead, we wait one millisecond which significantly reduces the + // test time. + if (server_->getServerPort() != 0) { + fetch_time = 1000 * fetch_time; + } + + TimerMgr::instance()-> + registerTimer("Dhcp4CBFetchTimer", + boost::bind(&ControlledDhcpv4Srv::cbFetchUpdates, + server_, CfgMgr::instance().getStagingCfg()), + fetch_time, + asiolink::IntervalTimer::ONE_SHOT); + TimerMgr::instance()->setup("Dhcp4CBFetchTimer"); + } + } + // This hook point notifies hooks libraries that the configuration of the // DHCPv4 server has completed. It provides the hook library with the pointer // to the common IO service object, new server configuration in the JSON @@ -941,5 +967,25 @@ ControlledDhcpv4Srv::dbLostCallback(ReconnectCtlPtr db_reconnect_ctl) { return(true); } +void +ControlledDhcpv4Srv::cbFetchUpdates(const SrvConfigPtr& srv_cfg) { + try { + // The true value indicates that the server should not reconnect + // to the configuration backends and should take into account + // audit entries stored in the database since last fetch. + server_->getCBControl()->databaseConfigFetch(srv_cfg, true); + + } catch (const std::exception& ex) { + LOG_ERROR(dhcp4_logger, DHCP4_CB_FETCH_UPDATES_FAIL) + .arg(ex.what()); + } + + // Reschedule the timer to fetch new updates or re-try if + // the previous attempt resulted in an error. + if (TimerMgr::instance()->isTimerRegistered("Dhcp4CBFetchTimer")) { + TimerMgr::instance()->setup("Dhcp4CBFetchTimer"); + } +} + }; // end of isc::dhcp namespace }; // end of isc namespace diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.h b/src/bin/dhcp4/ctrl_dhcp4_srv.h index de7a02ef66..c078e898cd 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.h +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.h @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2019 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 @@ -360,6 +360,16 @@ private: /// @return false if reconnect is not configured, true otherwise bool dbLostCallback(db::ReconnectCtlPtr db_reconnect_ctl); + /// @brief Callback invoked periodically to fetch configuration updates + /// from the Config Backends. + /// + /// This method calls @c CBControlDHCPv4::databaseConfigFetch and then + /// reschedules the timer. + /// + /// @param srv_cfg Server configuration holding the database credentials + /// and server tag. + void cbFetchUpdates(const SrvConfigPtr& srv_cfg); + /// @brief Static pointer to the sole instance of the DHCP server. /// /// This is required for config and command handlers to gain access to diff --git a/src/bin/dhcp4/dhcp4_messages.cc b/src/bin/dhcp4/dhcp4_messages.cc index 45c82e4644..0f65549719 100644 --- a/src/bin/dhcp4/dhcp4_messages.cc +++ b/src/bin/dhcp4/dhcp4_messages.cc @@ -1,4 +1,4 @@ -// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Tue Mar 19 2019 10:21 +// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Wed Mar 20 2019 11:09 #include #include @@ -13,6 +13,7 @@ extern const isc::log::MessageID DHCP4_BUFFER_RECEIVED = "DHCP4_BUFFER_RECEIVED" extern const isc::log::MessageID DHCP4_BUFFER_RECEIVE_FAIL = "DHCP4_BUFFER_RECEIVE_FAIL"; extern const isc::log::MessageID DHCP4_BUFFER_UNPACK = "DHCP4_BUFFER_UNPACK"; extern const isc::log::MessageID DHCP4_BUFFER_WAIT_SIGNAL = "DHCP4_BUFFER_WAIT_SIGNAL"; +extern const isc::log::MessageID DHCP4_CB_FETCH_UPDATES_FAIL = "DHCP4_CB_FETCH_UPDATES_FAIL"; extern const isc::log::MessageID DHCP4_CLASS_ASSIGNED = "DHCP4_CLASS_ASSIGNED"; extern const isc::log::MessageID DHCP4_CLASS_UNCONFIGURED = "DHCP4_CLASS_UNCONFIGURED"; extern const isc::log::MessageID DHCP4_CLASS_UNDEFINED = "DHCP4_CLASS_UNDEFINED"; @@ -144,6 +145,7 @@ const char* values[] = { "DHCP4_BUFFER_RECEIVE_FAIL", "error on attempt to receive packet: %1", "DHCP4_BUFFER_UNPACK", "parsing buffer received from %1 to %2 over interface %3", "DHCP4_BUFFER_WAIT_SIGNAL", "signal received while waiting for next packet, next waiting signal is %1", + "DHCP4_CB_FETCH_UPDATES_FAIL", "error on attempt to fetch configuration updates from the configuration backend(s): %1", "DHCP4_CLASS_ASSIGNED", "%1: client packet has been assigned to the following class(es): %2", "DHCP4_CLASS_UNCONFIGURED", "%1: client packet belongs to an unconfigured class: %2", "DHCP4_CLASS_UNDEFINED", "required class %1 has no definition", diff --git a/src/bin/dhcp4/dhcp4_messages.h b/src/bin/dhcp4/dhcp4_messages.h index c240a8464e..1168c531f1 100644 --- a/src/bin/dhcp4/dhcp4_messages.h +++ b/src/bin/dhcp4/dhcp4_messages.h @@ -1,4 +1,4 @@ -// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Tue Mar 19 2019 10:21 +// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Wed Mar 20 2019 11:09 #ifndef DHCP4_MESSAGES_H #define DHCP4_MESSAGES_H @@ -14,6 +14,7 @@ extern const isc::log::MessageID DHCP4_BUFFER_RECEIVED; extern const isc::log::MessageID DHCP4_BUFFER_RECEIVE_FAIL; extern const isc::log::MessageID DHCP4_BUFFER_UNPACK; extern const isc::log::MessageID DHCP4_BUFFER_WAIT_SIGNAL; +extern const isc::log::MessageID DHCP4_CB_FETCH_UPDATES_FAIL; extern const isc::log::MessageID DHCP4_CLASS_ASSIGNED; extern const isc::log::MessageID DHCP4_CLASS_UNCONFIGURED; extern const isc::log::MessageID DHCP4_CLASS_UNDEFINED; diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 7612e2fa92..2cac51e41d 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -46,6 +46,13 @@ by the process. The signal will be handled before the server starts waiting for next packets. The argument specifies the next signal to be handled by the server. +% DHCP4_CB_FETCH_UPDATES_FAIL error on attempt to fetch configuration updates from the configuration backend(s): %1 +This error message is issued when the server attempted to fetch +configuration updates from the database and this attempt failed. +The server will re-try according to the configured value of the +config-fetch-wait-time parameter. The sole argument contains the +reason for failure. + % DHCP4_CLASS_ASSIGNED %1: client packet has been assigned to the following class(es): %2 This debug message informs that incoming packet has been assigned to specified class or classes. This is a normal behavior and indicates successful operation. diff --git a/src/bin/dhcp4/tests/kea_controller_unittest.cc b/src/bin/dhcp4/tests/kea_controller_unittest.cc index 71bce5eb40..4e8a5668ed 100644 --- a/src/bin/dhcp4/tests/kea_controller_unittest.cc +++ b/src/bin/dhcp4/tests/kea_controller_unittest.cc @@ -16,9 +16,11 @@ #include #include #include +#include #include #include #include +#include #ifdef HAVE_MYSQL #include @@ -27,9 +29,11 @@ #include #include +#include #include #include +#include #include #include #include @@ -54,10 +58,84 @@ using namespace isc::hooks; namespace { +/// @brief Test implementation of the @c CBControlDHCPv4. +/// +/// This implementation is installed on the test server instance. It +/// overrides the implementation of the @c databaseConfigFetch function +/// to verify arguments passed to this function and throw an exception +/// when desired in the negative test scenarios. It doesn't do the +/// actual configuration fetch as this is tested elswhere and would +/// require setting up a database configuration backend. +class TestCBControlDHCPv4 : public CBControlDHCPv4 { +public: + + /// @brief Constructor. + TestCBControlDHCPv4() + : CBControlDHCPv4(), db_config_fetch_calls_(0), enable_throw_(false) { + } + + /// @brief Stub implementation of the "fetch" function. + /// + /// It checks if the @c fetch_updates_only is set to true when it + /// is a later than first invocation of the function. It also + /// throws an exception when desired by a test, to verify that the + /// server gracefully handles such exception. + /// + /// @param fetch_updates_only Boolean flag indicating that the function + /// is executed to fetch configuration updates from the database. It + /// should be set to false when the server is starting up. + /// + /// @throw Unexpected when configured to do so. + virtual void databaseConfigFetch(const process::ConfigPtr&, + const bool fetch_updates_only) { + if ((db_config_fetch_calls_++ > 0) && !fetch_updates_only) { + ADD_FAILURE() << "databaseConfigFetch was called with the value " + "of fetch_updates_only=false"; + } + + if (enable_throw_) { + isc_throw(Unexpected, "testing if exceptions are corectly handled"); + } + } + + /// @brief Returns number of invocations of the @c databaseConfigFetch. + size_t getDatabaseConfigFetchCalls() const { + return (db_config_fetch_calls_); + } + + /// @brief Enables the object to throw from @c databaseConfigFetch. + void enableThrow() { + enable_throw_ = true; + } + +private: + + /// @brief Counter holding number of invocations of the @c databaseConfigFetch. + size_t db_config_fetch_calls_; + + /// @brief Boolean flag indicating if the @c databaseConfigFetch should + /// throw. + bool enable_throw_; +}; + +/// @brief Shared pointer to the @c TestCBControlDHCPv4. +typedef boost::shared_ptr TestCBControlDHCPv4Ptr; + +/// @brief "Naked" DHCPv4 server. +/// +/// Exposes internal fields and installs stub implementation of the +/// @c CBControlDHCPv4 object. class NakedControlledDhcpv4Srv: public ControlledDhcpv4Srv { - // "Naked" DHCPv4 server, exposes internal fields public: - NakedControlledDhcpv4Srv():ControlledDhcpv4Srv(0) { } + + /// @brief Constructor. + NakedControlledDhcpv4Srv() + : ControlledDhcpv4Srv(0) { + // We're replacing the @c CBControlDHCPv4 instance with our + // stub implementation used in tests. + cb_control_.reset(new TestCBControlDHCPv4()); + } + using ControlledDhcpv4Srv::signal_handler_; }; @@ -98,15 +176,90 @@ public: /// /// @param io_service Pointer to the IO service to be ran. /// @param timeout_ms Amount of time after which the method returns. - void runTimersWithTimeout(const IOServicePtr& io_service, const long timeout_ms) { + /// @param cond Pointer to the function which if returns true it + /// stops the IO service and causes the function to return. + void runTimersWithTimeout(const IOServicePtr& io_service, const long timeout_ms, + std::function cond = std::function()) { IntervalTimer timer(*io_service); - timer.setup([&io_service]() { + bool stopped = false; + timer.setup([&io_service, &stopped]() { io_service->stop(); + stopped = true; }, timeout_ms, IntervalTimer::ONE_SHOT); - io_service->run(); + + // Run as long as the timeout hasn't occurred and the interrupting + // condition is not specified or not met. + while (!stopped && (!cond || !cond())) { + io_service->run_one(); + } io_service->get_io_service().reset(); } + /// @brief This test verifies that the timer used to fetch the configuration + /// updates from the database works as expected. + void testConfigBackendTimer(const int config_wait_fetch_time, + const bool throw_during_fetch = false) { + std::ostringstream config; + config << + "{ \"Dhcp4\": {" + "\"interfaces-config\": {" + " \"interfaces\": [ ]" + "}," + "\"lease-database\": {" + " \"type\": \"memfile\"," + " \"persist\": false" + "}," + "\"config-control\": {" + " \"config-fetch-wait-time\": " << config_wait_fetch_time << + "}," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, \n" + "\"subnet4\": [ ]," + "\"valid-lifetime\": 4000 }" + "}"; + writeFile(TEST_FILE, config.str()); + + // Create an instance of the server and initialize it. + boost::scoped_ptr srv; + ASSERT_NO_THROW(srv.reset(new NakedControlledDhcpv4Srv())); + ASSERT_NO_THROW(srv->init(TEST_FILE)); + + // Get the CBControlDHCPv4 object belonging to this server. + auto cb_control = boost::dynamic_pointer_cast(srv->getCBControl()); + + // Instruct our stub implementation of the CBControlDHCPv4 to throw as a + // result of fetch if desired. + if (throw_during_fetch) { + cb_control->enableThrow(); + } + + // So far there should be exactly one attempt to fetch the configuration + // from the backend. That's the attempt made upon startup. + EXPECT_EQ(1, cb_control->getDatabaseConfigFetchCalls()); + + + if (config_wait_fetch_time > 0) { + // If we're configured to run the timer, we expect that it was + // invoked at least 3 times. This is sufficient to verify that + // the timer was scheduled and that the timer continued to run + // even when an exception occurred during fetch (that's why it + // is 3 not 2). + ASSERT_NO_THROW(runTimersWithTimeout(srv->getIOService(), 500, + [cb_control]() { + // Interrupt the timers poll if we have recorded at + // least 3 attempts to fetch the updates. + return (cb_control->getDatabaseConfigFetchCalls() >= 3); + })); + EXPECT_GE(cb_control->getDatabaseConfigFetchCalls(), 3); + + } else { + // If the server is not configured to schedule the timer, + // we should still have one fetch attempt recorded. + ASSERT_NO_THROW(runTimersWithTimeout(srv->getIOService(), 500)); + EXPECT_EQ(1, cb_control->getDatabaseConfigFetchCalls()); + } + } + /// Name of a config file used during tests static const char* TEST_FILE; static const char* TEST_INCLUDE; @@ -648,6 +801,28 @@ TEST_F(JSONFileBackendTest, defaultLeaseDbBackend) { EXPECT_NO_THROW(static_cast(LeaseMgrFactory::instance())); } +// This test verifies that the timer triggering configuration updates +// is invoked according to the configured value of the +// config-fetch-wait-time. +TEST_F(JSONFileBackendTest, configBackendTimer) { + testConfigBackendTimer(1); +} + +// This test verifies that the timer for triggering configuration updates +// is not invoked when the value of the config-fetch-wait-time is set +// to 0. +TEST_F(JSONFileBackendTest, configBackendTimerDisabled) { + testConfigBackendTimer(0); +} + +// This test verifies that the server will gracefully handle exceptions +// thrown from the CBControlDHCPv4::databaseConfigFetch, i.e. will +// reschedule the timer. +TEST_F(JSONFileBackendTest, configBackendTimerWithThrow) { + // The true value instructs the test to throw during the fetch. + testConfigBackendTimer(1, true); +} + // Starting tests which require MySQL backend availability. Those tests // will not be executed if Kea has been compiled without the // --with-mysql. diff --git a/src/lib/process/cb_ctl_base.h b/src/lib/process/cb_ctl_base.h index f5215cdc57..bfea292fa9 100644 --- a/src/lib/process/cb_ctl_base.h +++ b/src/lib/process/cb_ctl_base.h @@ -158,8 +158,8 @@ public: /// @param fetch_updates_only boolean value indicating if the method is /// called upon the server start up (false) or it is called to fetch /// configuration updates (true). - void databaseConfigFetch(const ConfigPtr& srv_cfg, - const bool fetch_updates_only = false) { + virtual void databaseConfigFetch(const ConfigPtr& srv_cfg, + const bool fetch_updates_only = false) { // If the server starts up we need to connect to the database(s). // If there are no databases available simply do nothing. if (!fetch_updates_only && !databaseConfigConnect(srv_cfg)) {