From: Marcin Siodelski Date: Fri, 29 Aug 2014 07:28:30 +0000 (+0200) Subject: [3534] Basic implementation of staging and rolling back configurations. X-Git-Tag: trac3595_base~7^2~16 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=59cd21bb28754a53fd449f465df5c02624177c5a;p=thirdparty%2Fkea.git [3534] Basic implementation of staging and rolling back configurations. --- diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index e52e00b3aa..0f78b777fa 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -149,8 +149,9 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) { // safe and we really don't want to emit exceptions to whoever called this // method. Instead, catch an exception and create appropriate answer. try { - CfgMgr::instance().getConfiguration()->cfg_iface_ - .openSockets(srv->getPort(), getInstance()->useBroadcast()); + CfgMgr::instance().getCurrent()->getCfgIface() + .openSockets(CfgIface::V4, srv->getPort(), + getInstance()->useBroadcast()); } catch (std::exception& ex) { err << "failed to open sockets after server reconfiguration: " diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index 4cdde0fe6c..503373dd95 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -443,7 +443,7 @@ namespace dhcp { parser = new Uint32Parser(config_id, globalContext()->uint32_values_); } else if (config_id.compare("interfaces") == 0) { - parser = new InterfaceListConfigParser(config_id); + parser = new InterfaceListConfigParser(config_id, globalContext()); } else if (config_id.compare("subnet4") == 0) { parser = new Subnets4ListConfigParser(config_id); } else if (config_id.compare("option-data") == 0) { @@ -619,9 +619,8 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { subnet_parser->commit(); } - if (iface_parser) { - iface_parser->commit(); - } + // No need to commit interface names as this is handled by the + // CfgMgr::commit() function. // Apply global options commitGlobalOptions(); @@ -649,6 +648,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { // Rollback changes as the configuration parsing failed. if (rollback) { globalContext().reset(new ParserContext(original_context)); + CfgMgr::instance().rollback(); return (answer); } diff --git a/src/bin/dhcp4/kea_controller.cc b/src/bin/dhcp4/kea_controller.cc index 926832dbc0..4f6f851d67 100644 --- a/src/bin/dhcp4/kea_controller.cc +++ b/src/bin/dhcp4/kea_controller.cc @@ -41,6 +41,11 @@ void configure(const std::string& file_name) { // This is a configuration backend implementation that reads the // configuration from a JSON file. + // We are starting the configuration process so we should remove any + // staging configuration that has been created during previous + // configuration attempts. + CfgMgr::instance().rollback(); + isc::data::ConstElementPtr json; isc::data::ConstElementPtr dhcp4; isc::data::ConstElementPtr logger; @@ -66,7 +71,7 @@ void configure(const std::string& file_name) { // If there's no logging element, we'll just pass NULL pointer, // which will be handled by configureLogger(). Daemon::configureLogger(json->get("Logging"), - CfgMgr::instance().getConfiguration(), + CfgMgr::instance().getStaging(), ControlledDhcpv4Srv::getInstance()->getVerbose()); // Get Dhcp4 component from the config diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 7cd2015157..886017a98f 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -2917,7 +2917,8 @@ TEST_F(Dhcp4ParserTest, selectedInterfaces) { ASSERT_TRUE(status); checkResult(status, 0); - CfgMgr::instance().getConfiguration()->cfg_iface_.openSockets(10000); + CfgMgr::instance().getStaging()-> + getCfgIface().openSockets(CfgIface::V4, 10000); // eth0 and eth1 were explicitly selected. eth2 was not. EXPECT_TRUE(test_config.socketOpen("eth0", AF_INET)); @@ -2952,7 +2953,8 @@ TEST_F(Dhcp4ParserTest, allInterfaces) { ASSERT_TRUE(status); checkResult(status, 0); - CfgMgr::instance().getConfiguration()->cfg_iface_.openSockets(10000); + CfgMgr::instance().getStaging()-> + getCfgIface().openSockets(CfgIface::V4, 10000); // All interfaces should be now active. ASSERT_TRUE(test_config.socketOpen("eth0", AF_INET)); diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index 67de23ee47..d4c352e1e7 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -45,7 +45,7 @@ Dhcpv4SrvTest::Dhcpv4SrvTest() pool_ = Pool4Ptr(new Pool4(IOAddress("192.0.2.100"), IOAddress("192.0.2.110"))); subnet_->addPool(pool_); - CfgMgr::instance().getConfiguration()->cfg_iface_.reset(); + CfgMgr::instance().clear(); CfgMgr::instance().deleteSubnets4(); CfgMgr::instance().addSubnet4(subnet_); @@ -58,7 +58,7 @@ Dhcpv4SrvTest::Dhcpv4SrvTest() Dhcpv4SrvTest::~Dhcpv4SrvTest() { // Make sure that we revert to default value - CfgMgr::instance().getConfiguration()->cfg_iface_.reset(); + CfgMgr::instance().clear(); CfgMgr::instance().echoClientId(true); } diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index bc96d3b36e..99aba3ba87 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -144,8 +144,8 @@ ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) { // safe and we really don't want to emit exceptions to the callback caller. // Instead, catch an exception and create appropriate answer. try { - CfgMgr::instance().getConfiguration()->cfg_iface_ - .openSockets(srv->getPort()); + CfgMgr::instance().getConfiguration()->getCfgIface() + .openSockets(CfgIface::V6, srv->getPort()); } catch (const std::exception& ex) { std::ostringstream err; diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 9e960f322e..ac7401d98a 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -150,12 +150,6 @@ Dhcpv6Srv::Dhcpv6Srv(uint16_t port) // Instantiate allocation engine alloc_engine_.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)); - // We have to point out to the CfgMgr that the we are in the IPv6 - // domain, so as the IPv6 sockets are opened rather than IPv4 sockets - // which are the default. - CfgMgr::instance().getConfiguration() - ->cfg_iface_.setFamily(CfgIface::V6); - /// @todo call loadLibraries() when handling configuration changes } catch (const std::exception &e) { diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index 1208e21d39..a2c2c649aa 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -662,7 +662,7 @@ namespace dhcp { parser = new Uint32Parser(config_id, globalContext()->uint32_values_); } else if (config_id.compare("interfaces") == 0) { - parser = new InterfaceListConfigParser(config_id); + parser = new InterfaceListConfigParser(config_id, globalContext()); } else if (config_id.compare("subnet6") == 0) { parser = new Subnets6ListConfigParser(config_id); } else if (config_id.compare("option-data") == 0) { @@ -821,9 +821,8 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { subnet_parser->commit(); } - if (iface_parser) { - iface_parser->commit(); - } + // No need to commit interface names as this is handled by the + // CfgMgr::commit() function. // This occurs last as if it succeeds, there is no easy way to // revert it. As a result, the failure to commit a subsequent @@ -850,6 +849,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { // Rollback changes as the configuration parsing failed. if (rollback) { globalContext().reset(new ParserContext(original_context)); + CfgMgr::instance().rollback(); return (answer); } diff --git a/src/bin/dhcp6/kea_controller.cc b/src/bin/dhcp6/kea_controller.cc index ff2f2ad347..9b49ec26cf 100644 --- a/src/bin/dhcp6/kea_controller.cc +++ b/src/bin/dhcp6/kea_controller.cc @@ -45,6 +45,11 @@ void configure(const std::string& file_name) { // This is a configuration backend implementation that reads the // configuration from a JSON file. + // We are starting the configuration process so we should remove any + // staging configuration that has been created during previous + // configuration attempts. + CfgMgr::instance().rollback(); + isc::data::ConstElementPtr json; isc::data::ConstElementPtr dhcp6; isc::data::ConstElementPtr logger; diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 34b3b69f0b..eba6f9b3e1 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -359,9 +359,7 @@ public: // properly test interface configuration we disable listening on // all interfaces before each test and later check that this setting // has been overriden by the configuration used in the test. - CfgMgr::instance().getConfiguration()->cfg_iface_.reset(); - CfgMgr::instance().getConfiguration()-> - cfg_iface_.setFamily(CfgIface::V6); + CfgMgr::instance().clear(); // Create fresh context. globalContext()->copyContext(ParserContext(Option::V6)); } @@ -3057,7 +3055,8 @@ TEST_F(Dhcp6ParserTest, selectedInterfaces) { // as the pool does not belong to that subnet checkResult(status, 0); - CfgMgr::instance().getConfiguration()->cfg_iface_.openSockets(10000); + CfgMgr::instance().getStaging()-> + getCfgIface().openSockets(CfgIface::V6, 10000); // eth0 and eth1 were explicitly selected. eth2 was not. EXPECT_TRUE(test_config.socketOpen("eth0", AF_INET6)); @@ -3075,7 +3074,7 @@ TEST_F(Dhcp6ParserTest, allInterfaces) { ConstElementPtr status; // This configuration specifies two interfaces on which server should listen - // bu also includes keyword 'all'. This keyword switches server into the + // but also includes '*'. This keyword switches server into the // mode when it listens on all interfaces regardless of what interface names // were specified in the "interfaces" parameter. string config = "{ \"interfaces\": [ \"eth0\", \"eth1\", \"*\" ]," @@ -3090,7 +3089,8 @@ TEST_F(Dhcp6ParserTest, allInterfaces) { EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json)); checkResult(status, 0); - CfgMgr::instance().getConfiguration()->cfg_iface_.openSockets(10000); + CfgMgr::instance().getStaging()-> + getCfgIface().openSockets(CfgIface::V6, 10000); // All interfaces should be now active. EXPECT_TRUE(test_config.socketOpen("eth0", AF_INET6)); diff --git a/src/lib/dhcpsrv/cfg_iface.cc b/src/lib/dhcpsrv/cfg_iface.cc index 21651820db..e7a630d875 100644 --- a/src/lib/dhcpsrv/cfg_iface.cc +++ b/src/lib/dhcpsrv/cfg_iface.cc @@ -25,24 +25,24 @@ namespace dhcp { const char* CfgIface::ALL_IFACES_KEYWORD = "*"; -CfgIface::CfgIface(Family family) - : family_(family), - wildcard_used_(false) { +CfgIface::CfgIface() + : wildcard_used_(false) { } void -CfgIface::closeSockets() { +CfgIface::closeSockets() const { IfaceMgr::instance().closeSockets(); } void -CfgIface::openSockets(const uint16_t port, const bool use_bcast) { +CfgIface::openSockets(const Family& family, const uint16_t port, + const bool use_bcast) const { // If wildcard interface '*' was not specified, set all interfaces to // inactive state. We will later enable them selectively using the // interface names specified by the user. If wildcard interface was // specified, mark all interfaces active. In all cases, mark loopback // inactive. - setState(!wildcard_used_, true); + setState(family, !wildcard_used_, true); // Remove selection of unicast addresses from all interfaces. IfaceMgr::instance().clearUnicasts(); // If there is no wildcard interface specified, we will have to iterate @@ -61,7 +61,7 @@ CfgIface::openSockets(const uint16_t port, const bool use_bcast) { << *iface_name << "' as this interface doesn't" " exist"); - } else if (getFamily() == V4) { + } else if (family == V4) { iface->inactive4_ = false; } else { @@ -71,7 +71,7 @@ CfgIface::openSockets(const uint16_t port, const bool use_bcast) { } // Select unicast sockets. It works only for V6. Ignore for V4. - if (getFamily() == V6) { + if (family == V6) { for (UnicastMap::const_iterator unicast = unicast_map_.begin(); unicast != unicast_map_.end(); ++unicast) { Iface* iface = IfaceMgr::instance().getIface(unicast->first); @@ -95,7 +95,7 @@ CfgIface::openSockets(const uint16_t port, const bool use_bcast) { IfaceMgrErrorMsgCallback error_callback = boost::bind(&CfgIface::socketOpenErrorHandler, _1); bool sopen; - if (getFamily() == V4) { + if (family == V4) { sopen = IfaceMgr::instance().openSockets4(port, use_bcast, error_callback); } else { @@ -117,12 +117,13 @@ CfgIface::reset() { } void -CfgIface::setState(const bool inactive, const bool loopback_inactive) { +CfgIface::setState(const Family& family, const bool inactive, + const bool loopback_inactive) const { IfaceMgr::IfaceCollection ifaces = IfaceMgr::instance().getIfaces(); for (IfaceMgr::IfaceCollection::iterator iface = ifaces.begin(); iface != ifaces.end(); ++iface) { Iface* iface_ptr = IfaceMgr::instance().getIface(iface->getName()); - if (getFamily() == V4) { + if (family == V4) { iface_ptr->inactive4_ = iface_ptr->flag_loopback_ ? loopback_inactive : inactive; } else { @@ -138,7 +139,7 @@ CfgIface::socketOpenErrorHandler(const std::string& errmsg) { } void -CfgIface::use(const std::string& iface_name) { +CfgIface::use(const Family& family, const std::string& iface_name) { // The interface name specified may have two formats, e.g.: // - eth0 // - eth0/2001:db8:1::1. @@ -184,7 +185,7 @@ CfgIface::use(const std::string& iface_name) { } - } else if (getFamily() == V4) { + } else if (family == V4) { isc_throw(InvalidIfaceName, "unicast addresses in the format of: " "iface-name/unicast-addr_stress can only be specified for" " IPv6 addr_stress family"); diff --git a/src/lib/dhcpsrv/cfg_iface.h b/src/lib/dhcpsrv/cfg_iface.h index 01376da358..14c5236936 100644 --- a/src/lib/dhcpsrv/cfg_iface.h +++ b/src/lib/dhcpsrv/cfg_iface.h @@ -81,17 +81,10 @@ public: }; /// @brief Constructor. - /// - /// @param family Protocol family (default is V4). - CfgIface(Family family = V4); + CfgIface(); /// @brief Convenience function which closes all open sockets. - void closeSockets(); - - /// @brief Returns protocol family used by the @c CfgIface. - Family getFamily() const { - return (family_); - } + void closeSockets() const; /// @brief Tries to open sockets on selected interfaces. /// @@ -100,24 +93,19 @@ public: /// documentation for details how to specify interfaces and unicast /// addresses to bind the sockets to. /// + /// @param family Address family (v4 or v6). /// @param port Port number to be used to bind sockets to. /// @param use_bcast A boolean flag which indicates if the broadcast /// traffic should be received through the socket. This parameter is /// ignored for IPv6. - void openSockets(const uint16_t port, const bool use_bcast = true); + void openSockets(const Family& family, const uint16_t port, + const bool use_bcast = true) const; /// @brief Puts the interface configuration into default state. /// /// This function removes interface names from the set. void reset(); - /// @brief Sets protocol family. - /// - /// @param family New family value (V4 or V6). - void setFamily(Family family) { - family_ = family; - } - /// @brief Select interface to be used to receive DHCP traffic. /// /// This function controls the selection of the interface on which the @@ -137,6 +125,7 @@ public: /// not allowed when specifying a unicast address. For example: /// */2001:db8:1::1 is not allowed. /// + /// @param family Address family (v4 or v6). /// @param iface_name Explicit interface name, a wildcard name (*) of /// the interface(s) or the pair of iterface/unicast-address to be used /// to receive DHCP traffic. @@ -148,7 +137,7 @@ public: /// @throw DuplicateIfaceName If the interface is already selected, i.e. /// @throw IOError when specified unicast address is invalid. /// @c CfgIface::use has been already called for this interface. - void use(const std::string& iface_name); + void use(const Family& family, const std::string& iface_name); private: @@ -157,12 +146,14 @@ private: /// This function selects all interfaces to receive DHCP traffic or /// deselects all interfaces so as none of them receives a DHCP traffic. /// + /// @param family Address family (v4 or v6). /// @param inactive A boolean value which indicates if all interfaces /// (except loopback) should be selected or deselected. /// @param loopback_inactive A boolean value which indicates if loopback /// interface should be selected or deselected. /// should be deselected/inactive (true) or selected/active (false). - void setState(const bool inactive, const bool loopback_inactive); + void setState(const Family& family, const bool inactive, + const bool loopback_inactive) const; /// @brief Error handler for executed when opening a socket fail. /// diff --git a/src/lib/dhcpsrv/cfgmgr.cc b/src/lib/dhcpsrv/cfgmgr.cc index 606b9e6e12..5dccbce321 100644 --- a/src/lib/dhcpsrv/cfgmgr.cc +++ b/src/lib/dhcpsrv/cfgmgr.cc @@ -357,17 +357,63 @@ CfgMgr::getD2ClientMgr() { return (d2_client_mgr_); } +void +CfgMgr::ensureCurrentAllocated() { + if (!configuration_ || configs_.empty()) { + configuration_.reset(new Configuration()); + configs_.push_back(configuration_); + } +} + +void +CfgMgr::clear() { + configs_.clear(); + ensureCurrentAllocated(); +} + +void +CfgMgr::commit() { + if (!configs_.empty() && configs_.back() != configuration_) { + configuration_ = configs_.back(); + } +} + +void +CfgMgr::rollback() { + ensureCurrentAllocated(); + if (!configuration_->sequenceEquals(*configs_.back())) { + configs_.pop_back(); + } +} + ConfigurationPtr CfgMgr::getConfiguration() { return (configuration_); } +ConstConfigurationPtr +CfgMgr::getCurrent() { + ensureCurrentAllocated(); + return (configuration_); +} + +ConfigurationPtr +CfgMgr::getStaging() { + ensureCurrentAllocated(); + if (configuration_->sequenceEquals(*configs_.back())) { + uint32_t sequence = configuration_->getSequence(); + configs_.push_back(ConfigurationPtr(new Configuration(++sequence))); + } + return (configs_.back()); +} + CfgMgr::CfgMgr() : datadir_(DHCP_DATA_DIR), echo_v4_client_id_(true), - d2_client_mgr_(), configuration_(new Configuration()) { + d2_client_mgr_() { // DHCP_DATA_DIR must be set set with -DDHCP_DATA_DIR="..." in Makefile.am // Note: the definition of DHCP_DATA_DIR needs to include quotation marks // See AM_CPPFLAGS definition in Makefile.am + ensureCurrentAllocated(); } CfgMgr::~CfgMgr() { diff --git a/src/lib/dhcpsrv/cfgmgr.h b/src/lib/dhcpsrv/cfgmgr.h index 9be178aeef..43d4456000 100644 --- a/src/lib/dhcpsrv/cfgmgr.h +++ b/src/lib/dhcpsrv/cfgmgr.h @@ -373,12 +373,21 @@ public: /// @return a reference to the DHCP-DDNS manager. D2ClientMgr& getD2ClientMgr(); + void clear(); + + void commit(); + + void rollback(); /// @brief Returns the current configuration. /// /// @return a pointer to the current configuration. ConfigurationPtr getConfiguration(); + ConstConfigurationPtr getCurrent(); + + ConfigurationPtr getStaging(); + protected: /// @brief Protected constructor. @@ -410,6 +419,8 @@ protected: private: + void ensureCurrentAllocated(); + /// @brief Checks that the IPv4 subnet with the given id already exists. /// /// @param subnet Subnet for which this function will check if the other @@ -453,6 +464,16 @@ private: /// @todo: maybe this should be a vector, so we could keep /// previous configurations and do a rollback if needed? ConfigurationPtr configuration_; + + /// @name Configuration List. + /// + //@{ + /// @brief Configuration list type. + typedef std::list ConfigurationList; + + /// @brief Container holding all previous and current configurations. + ConfigurationList configs_; + //@} }; } // namespace isc::dhcp diff --git a/src/lib/dhcpsrv/configuration.cc b/src/lib/dhcpsrv/configuration.cc index 49d1c263d9..2207c5a044 100644 --- a/src/lib/dhcpsrv/configuration.cc +++ b/src/lib/dhcpsrv/configuration.cc @@ -19,6 +19,14 @@ namespace isc { namespace dhcp { +Configuration::Configuration() + : sequence_(0) { +} + +Configuration::Configuration(uint32_t sequence) + : sequence_(sequence) { +} + std::string Configuration::getConfigSummary(const uint32_t selection) const { std::ostringstream s; @@ -60,5 +68,10 @@ Configuration::getConfigSummary(const uint32_t selection) const { return (summary); } +bool +Configuration::sequenceEquals(const Configuration& other) { + return (getSequence() == other.getSequence()); +} + } } diff --git a/src/lib/dhcpsrv/configuration.h b/src/lib/dhcpsrv/configuration.h index f5a132bed4..4bc48cf954 100644 --- a/src/lib/dhcpsrv/configuration.h +++ b/src/lib/dhcpsrv/configuration.h @@ -83,8 +83,8 @@ typedef std::vector LoggingInfoStorage; /// @brief Specifies current DHCP configuration /// /// @todo Migrate all other configuration parameters from cfgmgr.h here -struct Configuration { - +class Configuration { +public: /// @name Constants for selection of parameters returned by @c getConfigSummary /// //@{ @@ -113,11 +113,15 @@ struct Configuration { /// @brief logging specific information LoggingInfoStorage logging_info_; - /// @brief Interface configuration. + /// @brief Default constructor. /// - /// Used to select interfaces on which the DHCP server will listen to - /// queries. - CfgIface cfg_iface_; + /// This constructor sets configuration sequence number to 0. + Configuration(); + + /// @brief Constructor. + /// + /// Sets arbitrary configuration sequence number. + Configuration(uint32_t sequence); /// @brief Returns summary of the configuration in the textual format. /// @@ -138,11 +142,64 @@ struct Configuration { /// /// @return Summary of the configuration in the textual format. std::string getConfigSummary(const uint32_t selection) const; + + /// @brief Returns configuration sequence number. + uint32_t getSequence() const { + return (sequence_); + } + + /// @brief Compares configuration sequence with other sequence. + /// + /// This method compares sequence numbers of two configurations for + /// equality. The sequence numbers are meant to be unique, so if + /// they are equal it means that they point to the same configuration. + /// + /// @param other Configuration which sequence number should be + /// compared with the sequence number of this configuration. + /// + /// @return true if sequence numbers are equal. + bool sequenceEquals(const Configuration& other); + + /// @brief Returns object which represents selection of interfaces. + /// + /// This function returns a reference to the object which represents the + /// set of interfaces being used to receive DHCP traffic. + /// + /// @return Object representing selection of interfaces. + const CfgIface& getCfgIface() const { + return (cfg_iface_); + } + + /// @brief Sets the object representing selection of interfaces. + /// + /// @param cfg_iface Object representing selection of interfaces. + void setCfgIface(const CfgIface& cfg_iface) { + cfg_iface_ = cfg_iface; + } + +private: + + /// @brief Sequence number identifying the configuration. + uint32_t sequence_; + + /// @brief Interface configuration. + /// + /// Used to select interfaces on which the DHCP server will listen to + /// queries. + CfgIface cfg_iface_; + }; -/// @brief pointer to the configuration +/// @name Pointers to the @c Configuration object. +/// +//@{ +/// @brief Non-const pointer to the @ Configuration. typedef boost::shared_ptr ConfigurationPtr; +/// @brief Const pointer to the @c Configuration. +typedef boost::shared_ptr ConstConfigurationPtr; +//@} + } // namespace isc::dhcp } // namespace isc diff --git a/src/lib/dhcpsrv/dhcp_parsers.cc b/src/lib/dhcpsrv/dhcp_parsers.cc index eef68663dc..d03e38644d 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/dhcp_parsers.cc @@ -179,8 +179,9 @@ template <> void ValueParser::build(ConstElementPtr value) { // ******************** InterfaceListConfigParser ************************* InterfaceListConfigParser:: -InterfaceListConfigParser(const std::string& param_name) - : param_name_(param_name) { +InterfaceListConfigParser(const std::string& param_name, + ParserContextPtr global_context) + : param_name_(param_name), global_context_(global_context) { if (param_name_ != "interfaces") { isc_throw(BadValue, "Internal error. Interface configuration " "parser called for the wrong parameter: " << param_name); @@ -189,38 +190,24 @@ InterfaceListConfigParser(const std::string& param_name) void InterfaceListConfigParser::build(ConstElementPtr value) { - // Copy the current interface configuration. - ConfigurationPtr config = CfgMgr::instance().getConfiguration(); - cfg_iface_ = config->cfg_iface_; - cfg_iface_.reset(); + CfgIface cfg_iface; BOOST_FOREACH(ConstElementPtr iface, value->listValue()) { std::string iface_name = iface->stringValue(); try { - cfg_iface_.use(iface_name); + cfg_iface.use(global_context_->universe_ == Option::V4 ? + CfgIface::V4 : CfgIface::V6, iface_name); } catch (const std::exception& ex) { isc_throw(DhcpConfigError, "Failed to select interface: " << ex.what() << " (" << value->getPosition() << ")"); } } + CfgMgr::instance().getStaging()->setCfgIface(cfg_iface); } void InterfaceListConfigParser::commit() { - // Use the new configuration created in a build time. - CfgMgr::instance().getConfiguration()->cfg_iface_ = cfg_iface_; -} - -bool -InterfaceListConfigParser::isIfaceAdded(const std::string& iface) const { - - for (IfaceListStorage::const_iterator it = interfaces_.begin(); - it != interfaces_.end(); ++it) { - if (iface == *it) { - return (true); - } - } - return (false); + // Nothing to do. } // ******************** HooksLibrariesParser ************************* diff --git a/src/lib/dhcpsrv/dhcp_parsers.h b/src/lib/dhcpsrv/dhcp_parsers.h index 1acb3df4ac..d1de4ac9fe 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.h +++ b/src/lib/dhcpsrv/dhcp_parsers.h @@ -396,8 +396,10 @@ public: /// "interface" parameter only. All other types will throw exception. /// /// @param param_name name of the configuration parameter being parsed + /// @param global_context Global parser context. /// @throw BadValue if supplied parameter name is not "interface" - InterfaceListConfigParser(const std::string& param_name); + InterfaceListConfigParser(const std::string& param_name, + ParserContextPtr global_context); /// @brief parses parameters value /// @@ -407,29 +409,16 @@ public: /// @param value pointer to the content of parsed values virtual void build(isc::data::ConstElementPtr value); - /// @brief Assignes a parsed list of interfaces to the configuration. - /// - /// This is exception safe operation. + /// @brief Does nothing. virtual void commit(); private: - /// @brief Check that specified interface exists in - /// @c InterfaceListConfigParser::interfaces_. - /// - /// @param iface A name of the interface. - /// - /// @return true if specified interface name was found. - bool isIfaceAdded(const std::string& iface) const; - - /// contains list of network interfaces - typedef std::list IfaceListStorage; - IfaceListStorage interfaces_; // Parsed parameter name std::string param_name_; - /// Holds the configuration created during - CfgIface cfg_iface_; + /// Global parser context. + ParserContextPtr global_context_; }; /// @brief Parser for hooks library list diff --git a/src/lib/dhcpsrv/tests/cfg_iface_unittest.cc b/src/lib/dhcpsrv/tests/cfg_iface_unittest.cc index 01aba833c5..1884d3b358 100644 --- a/src/lib/dhcpsrv/tests/cfg_iface_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_iface_unittest.cc @@ -65,13 +65,13 @@ CfgIfaceTest::unicastOpen(const std::string& iface_name) const { // This test checks that the interface names can be explicitly selected // by their names and IPv4 sockets are opened on these interfaces. TEST_F(CfgIfaceTest, explicitNamesV4) { - CfgIface cfg(CfgIface::V4); + CfgIface cfg; // Specify valid interface names. There should be no error. - ASSERT_NO_THROW(cfg.use("eth0")); - ASSERT_NO_THROW(cfg.use("eth1")); + ASSERT_NO_THROW(cfg.use(CfgIface::V4, "eth0")); + ASSERT_NO_THROW(cfg.use(CfgIface::V4, "eth1")); // Open sockets on specified interfaces. - cfg.openSockets(DHCP4_SERVER_PORT); + cfg.openSockets(CfgIface::V4, DHCP4_SERVER_PORT); // Sockets should be now open on eth0 and eth1, but not on loopback. EXPECT_TRUE(socketOpen("eth0", AF_INET)); @@ -91,9 +91,9 @@ TEST_F(CfgIfaceTest, explicitNamesV4) { // Reset configuration and select only one interface this time. cfg.reset(); - ASSERT_NO_THROW(cfg.use("eth1")); + ASSERT_NO_THROW(cfg.use(CfgIface::V4, "eth1")); - cfg.openSockets(DHCP4_SERVER_PORT); + cfg.openSockets(CfgIface::V4, DHCP4_SERVER_PORT); // Socket should be open on eth1 only. EXPECT_FALSE(socketOpen("eth0", AF_INET)); @@ -105,13 +105,13 @@ TEST_F(CfgIfaceTest, explicitNamesV4) { // This test checks that the interface names can be explicitly selected // by their names and IPv6 sockets are opened on these interfaces. TEST_F(CfgIfaceTest, explicitNamesV6) { - CfgIface cfg(CfgIface::V6); + CfgIface cfg; // Specify valid interface names. There should be no error. - ASSERT_NO_THROW(cfg.use("eth0")); - ASSERT_NO_THROW(cfg.use("eth1")); + ASSERT_NO_THROW(cfg.use(CfgIface::V6, "eth0")); + ASSERT_NO_THROW(cfg.use(CfgIface::V6, "eth1")); // Open sockets on specified interfaces. - cfg.openSockets(DHCP6_SERVER_PORT); + cfg.openSockets(CfgIface::V6, DHCP6_SERVER_PORT); // Sockets should be now open on eth0 and eth1, but not on loopback. EXPECT_TRUE(socketOpen("eth0", AF_INET6)); @@ -131,9 +131,9 @@ TEST_F(CfgIfaceTest, explicitNamesV6) { // Reset configuration and select only one interface this time. cfg.reset(); - ASSERT_NO_THROW(cfg.use("eth1")); + ASSERT_NO_THROW(cfg.use(CfgIface::V6, "eth1")); - cfg.openSockets(DHCP6_SERVER_PORT); + cfg.openSockets(CfgIface::V6, DHCP6_SERVER_PORT); // Socket should be open on eth1 only. EXPECT_FALSE(socketOpen("eth0", AF_INET6)); @@ -145,10 +145,10 @@ TEST_F(CfgIfaceTest, explicitNamesV6) { // This test checks that the wildcard interface name can be specified to // select all interfaces to open IPv4 sockets. TEST_F(CfgIfaceTest, wildcardV4) { - CfgIface cfg(CfgIface::V4); - ASSERT_NO_THROW(cfg.use("*")); + CfgIface cfg; + ASSERT_NO_THROW(cfg.use(CfgIface::V4, "*")); - cfg.openSockets(DHCP4_SERVER_PORT); + cfg.openSockets(CfgIface::V4, DHCP4_SERVER_PORT); // Sockets should be now open on eth0 and eth1, but not on loopback. EXPECT_TRUE(socketOpen("eth0", AF_INET)); @@ -164,10 +164,10 @@ TEST_F(CfgIfaceTest, wildcardV4) { // This test checks that the wildcard interface name can be specified to // select all interfaces to open IPv6 sockets. TEST_F(CfgIfaceTest, wildcardV6) { - CfgIface cfg(CfgIface::V6); - ASSERT_NO_THROW(cfg.use("*")); + CfgIface cfg; + ASSERT_NO_THROW(cfg.use(CfgIface::V6, "*")); - cfg.openSockets(DHCP4_SERVER_PORT); + cfg.openSockets(CfgIface::V6, DHCP6_SERVER_PORT); // Sockets should be now open on eth0 and eth1, but not on loopback. EXPECT_TRUE(socketOpen("eth0", AF_INET6)); @@ -184,14 +184,14 @@ TEST_F(CfgIfaceTest, wildcardV6) { // the interface on which the socket bound to link local address is also // opened. TEST_F(CfgIfaceTest, validUnicast) { - CfgIface cfg(CfgIface::V6); + CfgIface cfg; // One socket will be opened on link-local address, one on unicast but // on the same interface. - ASSERT_NO_THROW(cfg.use("eth0")); - ASSERT_NO_THROW(cfg.use("eth0/2001:db8:1::1")); + ASSERT_NO_THROW(cfg.use(CfgIface::V6, "eth0")); + ASSERT_NO_THROW(cfg.use(CfgIface::V6, "eth0/2001:db8:1::1")); - cfg.openSockets(DHCP6_SERVER_PORT); + cfg.openSockets(CfgIface::V6, DHCP6_SERVER_PORT); EXPECT_TRUE(socketOpen("eth0", AF_INET6)); EXPECT_TRUE(unicastOpen("eth0")); @@ -199,28 +199,28 @@ TEST_F(CfgIfaceTest, validUnicast) { // Test that when invalid interface names are specified an exception is thrown. TEST_F(CfgIfaceTest, invalidValues) { - CfgIface cfg(CfgIface::V4); - ASSERT_THROW(cfg.use(""), InvalidIfaceName); - ASSERT_THROW(cfg.use(" "), InvalidIfaceName); - ASSERT_THROW(cfg.use("bogus"), NoSuchIface); - - ASSERT_NO_THROW(cfg.use("eth0")); - ASSERT_THROW(cfg.use("eth0"), DuplicateIfaceName); - - ASSERT_THROW(cfg.use("eth0/2001:db8:1::1"), InvalidIfaceName); - - cfg.setFamily(CfgIface::V6); - - ASSERT_THROW(cfg.use("eth0/"), InvalidIfaceName); - ASSERT_THROW(cfg.use("/2001:db8:1::1"), InvalidIfaceName); - ASSERT_THROW(cfg.use("*/2001:db8:1::1"), InvalidIfaceName); - ASSERT_THROW(cfg.use("bogus/2001:db8:1::1"), NoSuchIface); - ASSERT_THROW(cfg.use("eth0/fe80::3a60:77ff:fed5:cdef"), InvalidIfaceName); - ASSERT_THROW(cfg.use("eth0/fe80::3a60:77ff:fed5:cdef"), InvalidIfaceName); - ASSERT_THROW(cfg.use("eth0/2001:db8:1::2"), NoSuchAddress); - - ASSERT_NO_THROW(cfg.use("*")); - ASSERT_THROW(cfg.use("*"), DuplicateIfaceName); + CfgIface cfg; + ASSERT_THROW(cfg.use(CfgIface::V4, ""), InvalidIfaceName); + ASSERT_THROW(cfg.use(CfgIface::V4, " "), InvalidIfaceName); + ASSERT_THROW(cfg.use(CfgIface::V4, "bogus"), NoSuchIface); + + ASSERT_NO_THROW(cfg.use(CfgIface::V4, "eth0")); + ASSERT_THROW(cfg.use(CfgIface::V4, "eth0"), DuplicateIfaceName); + + ASSERT_THROW(cfg.use(CfgIface::V4, "eth0/2001:db8:1::1"), InvalidIfaceName); + + ASSERT_THROW(cfg.use(CfgIface::V6, "eth0/"), InvalidIfaceName); + ASSERT_THROW(cfg.use(CfgIface::V6, "/2001:db8:1::1"), InvalidIfaceName); + ASSERT_THROW(cfg.use(CfgIface::V6, "*/2001:db8:1::1"), InvalidIfaceName); + ASSERT_THROW(cfg.use(CfgIface::V6, "bogus/2001:db8:1::1"), NoSuchIface); + ASSERT_THROW(cfg.use(CfgIface::V6, "eth0/fe80::3a60:77ff:fed5:cdef"), + InvalidIfaceName); + ASSERT_THROW(cfg.use(CfgIface::V6, "eth0/fe80::3a60:77ff:fed5:cdef"), + InvalidIfaceName); + ASSERT_THROW(cfg.use(CfgIface::V6, "eth0/2001:db8:1::2"), NoSuchAddress); + + ASSERT_NO_THROW(cfg.use(CfgIface::V6, "*")); + ASSERT_THROW(cfg.use(CfgIface::V6, "*"), DuplicateIfaceName); } } // end of anonymous namespace diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc index 2757306789..0316a562fa 100644 --- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc @@ -262,9 +262,7 @@ class CfgMgrTest : public ::testing::Test { public: CfgMgrTest() { // make sure we start with a clean configuration - CfgMgr::instance().deleteSubnets4(); - CfgMgr::instance().deleteSubnets6(); - CfgMgr::instance().deleteOptionDefs(); + clear(); } /// @brief generates interface-id option based on provided text @@ -279,9 +277,14 @@ public: ~CfgMgrTest() { // clean up after the test + clear(); + } + + void clear() { CfgMgr::instance().deleteSubnets4(); CfgMgr::instance().deleteSubnets6(); CfgMgr::instance().deleteOptionDefs(); + CfgMgr::instance().clear(); } /// used in client classification (or just empty container for other tests) @@ -1119,6 +1122,76 @@ TEST_F(CfgMgrTest, subnet6Duplication) { } +// This test verifies that the configuration staging and commit works +// as expected. +TEST_F(CfgMgrTest, staging) { + CfgMgr& cfg_mgr = CfgMgr::instance(); + // Initially, the current configuration is a default one. We are going + // to get the current configuration a couple of times and make sure + // that always the same instance is returned. + ConstConfigurationPtr const_config; + for (int i = 0; i < 5; ++i) { + const_config = cfg_mgr.getCurrent(); + ASSERT_TRUE(const_config) << "Returned NULL current configuration" + " for iteration " << i; + EXPECT_EQ(0, const_config->getSequence()) + << "Returned invalid sequence number " + << const_config->getSequence() << " for iteration " << i; + } + + // Try to get the new staging configuration. When getStaging() is called + // for the first time the new instance of the staging configuration is + // returned. This instance is returned for every call to getStaging() + // until commit is called. + ConfigurationPtr config; + for (int i = 0; i < 5; ++i) { + config = cfg_mgr.getStaging(); + ASSERT_TRUE(config) << "Returned NULL staging configuration for" + " iteration " << i; + // The sequence id is 1 for staging because it is ahead of current + // configuration having sequence number 0. + EXPECT_EQ(1, config->getSequence()) << "Returned invalid sequence" + " number " << config->getSequence() << " for iteration " << i; + } + + // This should change the staging configuration so as it becomes a current + // one. + CfgMgr::instance().commit(); + const_config = cfg_mgr.getCurrent(); + ASSERT_TRUE(const_config); + // Sequence id equal to 1 indicates that the current configuration points + // to the configuration that used to be a staging configuration previously. + EXPECT_EQ(1, const_config->getSequence()); + + // Create a new staging configuration. It should be assigned a new + // sequence id. + config = cfg_mgr.getStaging(); + ASSERT_TRUE(config); + EXPECT_EQ(2, config->getSequence()); + + // Let's execute commit a couple of times. The first invocation to commit + // changes the configuration having sequence 2 to current configuration. + // Other commits are no-op. + for (int i = 0; i < 5; ++i) { + CfgMgr::instance().commit(); + } + + // The current configuration now have sequence number 2. + const_config = cfg_mgr.getCurrent(); + ASSERT_TRUE(const_config); + EXPECT_EQ(2, const_config->getSequence()); + + // Clear configuration along with a history. + CfgMgr::instance().clear(); + + // After clearing configuration we should successfully get the + // new staging configuration. + const_config = cfg_mgr.getStaging(); + ASSERT_TRUE(const_config); + EXPECT_EQ(1, const_config->getSequence()); +} + + /// @todo Add unit-tests for testing: /// - addActiveIface() with invalid interface name /// - addActiveIface() with the same interface twice diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 420c882bdf..da8a244a35 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -59,8 +59,7 @@ public: /// @brief Resets selection of the interfaces from previous tests. void resetIfaceCfg() { - CfgMgr::instance().getConfiguration()->cfg_iface_.closeSockets(); - CfgMgr::instance().getConfiguration()->cfg_iface_.reset(); + CfgMgr::instance().clear(); } }; @@ -225,11 +224,14 @@ TEST_F(DhcpParserTest, interfaceListParserTest) { const std::string name = "interfaces"; + ParserContextPtr parser_context(new ParserContext(Option::V4)); + // Verify that parser constructor fails if parameter name isn't "interface" - EXPECT_THROW(InterfaceListConfigParser("bogus_name"), isc::BadValue); + EXPECT_THROW(InterfaceListConfigParser("bogus_name", parser_context), + isc::BadValue); boost::scoped_ptr - parser(new InterfaceListConfigParser(name)); + parser(new InterfaceListConfigParser(name, parser_context)); ElementPtr list_element = Element::createList(); list_element->add(Element::create("eth0")); @@ -240,9 +242,9 @@ TEST_F(DhcpParserTest, interfaceListParserTest) { // Use CfgMgr instance to check if eth0 and eth1 was added, and that // eth2 was not added. - ConfigurationPtr cfg = CfgMgr::instance().getConfiguration(); + ConfigurationPtr cfg = CfgMgr::instance().getStaging(); ASSERT_TRUE(cfg); - ASSERT_NO_THROW(cfg->cfg_iface_.openSockets(10000)); + ASSERT_NO_THROW(cfg->getCfgIface().openSockets(CfgIface::V4, 10000)); EXPECT_TRUE(test_config.socketOpen("eth0", AF_INET)); EXPECT_FALSE(test_config.socketOpen("eth1", AF_INET)); @@ -253,13 +255,15 @@ TEST_F(DhcpParserTest, interfaceListParserTest) { list_element->add(Element::create("*")); // Reset parser and configuration. - parser.reset(new InterfaceListConfigParser(name)); - cfg->cfg_iface_.closeSockets(); - cfg->cfg_iface_.reset(); + parser.reset(new InterfaceListConfigParser(name, parser_context)); + cfg->getCfgIface().closeSockets(); + CfgMgr::instance().clear(); parser->build(list_element); parser->commit(); - ASSERT_NO_THROW(cfg->cfg_iface_.openSockets(10000)); + + cfg = CfgMgr::instance().getStaging(); + ASSERT_NO_THROW(cfg->getCfgIface().openSockets(CfgIface::V4, 10000)); EXPECT_TRUE(test_config.socketOpen("eth0", AF_INET)); EXPECT_TRUE(test_config.socketOpen("eth1", AF_INET));