From: Stephen Morris Date: Wed, 24 Jul 2013 16:27:34 +0000 (+0100) Subject: [2981] Merge branch 'master' into trac2981 X-Git-Tag: bind10-1.2.0beta1-release~272^2~9 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3c285e7ef1b8890caa12ea83353b2e03f2bec23e;p=thirdparty%2Fkea.git [2981] Merge branch 'master' into trac2981 Conflicts: src/bin/dhcp6/config_parser.cc src/bin/dhcp6/dhcp6.spec src/bin/dhcp6/tests/Makefile.am src/bin/dhcp6/tests/config_parser_unittest.cc src/lib/dhcpsrv/dhcp_parsers.h src/lib/dhcpsrv/tests/Makefile.am src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc --- 3c285e7ef1b8890caa12ea83353b2e03f2bec23e diff --cc src/bin/dhcp6/config_parser.cc index d566bd4147,cc6bd028c9..fb92f88a11 --- a/src/bin/dhcp6/config_parser.cc +++ b/src/bin/dhcp6/config_parser.cc @@@ -465,12 -463,8 +465,13 @@@ configureDhcp6Server(Dhcpv6Srv&, isc::d ParserCollection independent_parsers; ParserPtr subnet_parser; ParserPtr option_parser; + ParserPtr iface_parser; + // Some of the parsers alter state of the system that can't easily + // be undone. (Or alter it in a way such that undoing the change + // has the same risk of failure as doing the change.) + ParserPtr hooks_parser; + // The subnet parsers implement data inheritance by directly // accessing global storage. For this reason the global data // parsers must store the parsed data into global storages @@@ -502,14 -496,11 +503,18 @@@ subnet_parser = parser; } else if (config_pair.first == "option-data") { option_parser = parser; - + } else if (config_pair.first == "hooks-libraries") { + // Executing the commit will alter currently loaded hooks + // libraries. Check if the supplied libraries are valid, + // but defer the commit until after everything else has + // committed. + hooks_parser = parser; + hooks_parser->build(config_pair.second); + } else if (config_pair.first == "interfaces") { + // The interface parser is independent from any other parser and + // can be run here before other parsers. + parser->build(config_pair.second); + iface_parser = parser; } else { // Those parsers should be started before other // parsers so we can call build straight away. diff --cc src/bin/dhcp6/dhcp6.spec index 44348c00e5,b61e811ddf..1235cfdbb2 --- a/src/bin/dhcp6/dhcp6.spec +++ b/src/bin/dhcp6/dhcp6.spec @@@ -3,23 -3,10 +3,23 @@@ "module_name": "Dhcp6", "module_description": "DHCPv6 server daemon", "config_data": [ + { + "item_name": "hooks-libraries", + "item_type": "list", + "item_optional": true, + "item_default": [], + "list_item_spec": + { + "item_name": "hooks-library", + "item_type": "string", + "item_optional": true, + } + }, + - { "item_name": "interface", + { "item_name": "interfaces", "item_type": "list", "item_optional": false, - "item_default": [ "all" ], + "item_default": [ "*" ], "list_item_spec": { "item_name": "interface_name", diff --cc src/bin/dhcp6/tests/Makefile.am index c1d25e5d2f,e8f0fcae71..be40ea555c --- a/src/bin/dhcp6/tests/Makefile.am +++ b/src/bin/dhcp6/tests/Makefile.am @@@ -77,10 -65,10 +77,10 @@@ dhcp6_unittests_LDADD += $(top_builddir dhcp6_unittests_LDADD += $(top_builddir)/src/lib/config/libb10-cfgclient.la dhcp6_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libb10-dhcp++.la dhcp6_unittests_LDADD += $(top_builddir)/src/lib/dhcpsrv/libb10-dhcpsrv.la ++dhcp6_unittests_LDADD += $(top_builddir)/src/lib/hooks/libb10-hooks.la dhcp6_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libb10-exceptions.la dhcp6_unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la dhcp6_unittests_LDADD += $(top_builddir)/src/lib/util/libb10-util.la - -dhcp6_unittests_LDADD += $(top_builddir)/src/lib/hooks/libb10-hooks.la endif noinst_PROGRAMS = $(TESTS) diff --cc src/bin/dhcp6/tests/config_parser_unittest.cc index b2da421611,cda9066d1a..16f460a688 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@@ -205,52 -199,27 +208,57 @@@ public << config << std::endl << " and the following error message was returned:" << ex.what() << std::endl; + return (false); } - // status object must not be NULL + // The status object must not be NULL if (!status) { - FAIL() << "Fatal error: unable to reset configuration database" - << " after the test. Configuration function returned" - << " NULL pointer" << std::endl; + ADD_FAILURE() << "Unable to " << operation << ". " + << "The configuration function returned a null pointer."; + return (false); } + + // Store the answer if we need it. + + // Returned value should be 0 (configuration success) comment_ = parseAnswer(rcode_, status); - // returned value should be 0 (configuration success) if (rcode_ != 0) { - FAIL() << "Fatal error: unable to reset configuration database" - << " after the test. Configuration function returned" - << " error code " << rcode_ << std::endl; + string reason = ""; + if (comment_) { + reason = string(" (") + comment_->stringValue() + string(")"); + } + ADD_FAILURE() << "Unable to " << operation << ". " + << "The configuration function returned error code " + << rcode_ << reason; + return (false); } + return (true); + } + + /// @brief Reset configuration database. + /// + /// This function resets configuration data base by removing all subnets + /// option-data, and hooks libraries. The reset must be performed after each + /// test to make sure that contents of the database do not affect the + /// results of subsequent tests. + void resetConfiguration() { - string config = "{ \"interface\": [ \"all\" ]," ++ string config = "{ \"interfaces\": [ \"all\" ]," + "\"hooks-libraries\": [ ]," + "\"preferred-lifetime\": 3000," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"valid-lifetime\": 4000, " + "\"subnet6\": [ ], " + "\"option-def\": [ ], " + "\"option-data\": [ ] }"; + static_cast(executeConfiguration(config, + "reset configuration database")); + // The default setting is to listen on all interfaces. In order to + // 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().deleteActiveIfaces(); } /// @brief Test invalid option parameter value. @@@ -1941,160 -1857,77 +1949,231 @@@ TEST_F(Dhcp6ParserTest, stdOptionDataEn EXPECT_FALSE(desc.option->getOption(112)); } +// Tests of the hooks libraries configuration. + +// Helper function to return a configuration containing an arbitrary number +// of hooks libraries. +std::string +buildHooksLibrariesConfig(const std::vector& libraries) { + const string quote("\""); + + // Create the first part of the configuration string. + string config = - "{ \"interface\": [ \"all\" ]," ++ "{ \"interfaces\": [ \"all\" ]," + "\"hooks-libraries\": ["; + + // Append the libraries (separated by commas if needed) + for (int i = 0; i < libraries.size(); ++i) { + if (i > 0) { + config += string(", "); + } + config += (quote + libraries[i] + quote); + } + + // Append the remainder of the configuration. + config += string( + "]," + "\"rebind-timer\": 2000," + "\"renew-timer\": 1000," + "\"option-data\": [ {" + " \"name\": \"foo\"," + " \"space\": \"vendor-opts-space\"," + " \"code\": 110," + " \"data\": \"1234\"," + " \"csv-format\": True" + " }," + " {" + " \"name\": \"foo2\"," + " \"space\": \"vendor-opts-space\"," + " \"code\": 111," + " \"data\": \"192.168.2.1\"," + " \"csv-format\": True" + " } ]," + "\"option-def\": [ {" + " \"name\": \"foo\"," + " \"code\": 110," + " \"type\": \"uint32\"," + " \"array\": False," + " \"record-types\": \"\"," + " \"space\": \"vendor-opts-space\"," + " \"encapsulate\": \"\"" + " }," + " {" + " \"name\": \"foo2\"," + " \"code\": 111," + " \"type\": \"ipv4-address\"," + " \"array\": False," + " \"record-types\": \"\"," + " \"space\": \"vendor-opts-space\"," + " \"encapsulate\": \"\"" + " } ]" + "}"); + + return (config); +} + +// Convenience function for creating hooks library configuration with one or +// two character string constants. +std::string +buildHooksLibrariesConfig(const char* library1 = NULL, + const char* library2 = NULL) { + std::vector libraries; + if (library1 != NULL) { + libraries.push_back(string(library1)); + if (library2 != NULL) { + libraries.push_back(string(library2)); + } + } + return (buildHooksLibrariesConfig(libraries)); +} + + +// The goal of this test is to verify the configuration of hooks libraries if +// none are specified. +TEST_F(Dhcp6ParserTest, NoHooksLibraries) { +// std::vector libraries = HooksManager::getLibraryNames(); +// ASSERT_TRUE(libraries.empty()); + + // Parse a configuration containing no names. + string config = buildHooksLibrariesConfig(); + if (!executeConfiguration(config, + "set configuration with no hooks libraries")) { + return; + } +// libraries = HooksManager::getLibraryNames(); +// ASSERT_TRUE(libraries.empty()); +} + +// Verify parsing fails with one library that will fail validation. +TEST_F(Dhcp6ParserTest, InvalidLibrary) { +// std::vector libraries = HooksManager::getLibraryNames(); +// ASSERT_TRUE(libraries.empty()); + + // Parse a configuration containing a failing library. + string config = buildHooksLibrariesConfig(NOT_PRESENT_LIBRARY); + + ConstElementPtr status; + ElementPtr json = Element::fromJSON(config); + ASSERT_NO_THROW(status = configureDhcp6Server(srv_, json)); + + // The status object must not be NULL + ASSERT_FALSE(status); + + // Returned value should not be 0 + comment_ = parseAnswer(rcode_, status); + EXPECT_NE(0, rcode_); + std::cerr << "Reason for success: " << comment_; +} + +// Verify the configuration of hooks libraries with two being specified. +TEST_F(Dhcp6ParserTest, LibrariesSpecified) { +// std::vector libraries = HooksManager::getLibraryNames(); +// ASSERT_TRUE(libraries.empty()); + + // Marker files should not be present. + EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, NULL)); + EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, NULL)); + + // Set up the configuration with two libraries and load them. + string config = buildHooksLibrariesConfig(CALLOUT_LIBRARY_1, + CALLOUT_LIBRARY_2); + ASSERT_TRUE(executeConfiguration(config, - "loading two valid libraries")); ++ "load two valid libraries")); + + // Expect two libraries to be loaded in the correct order (load marker file + // is present, no unload marker file). +// std::vector libraries = HooksManager::getLibraryNames(); +// ASSERT_EQ(2, libraries.size()); + EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12")); + EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, NULL)); + + // Unload the libraries. The load file should not have changed, but + // the unload one should indicate the unload() functions have been run. + config = buildHooksLibrariesConfig(); + ASSERT_TRUE(executeConfiguration(config, "unloading libraries")); + EXPECT_TRUE(checkMarkerFile(LOAD_MARKER_FILE, "12")); + EXPECT_TRUE(checkMarkerFile(UNLOAD_MARKER_FILE, "21")); + + // Expect the hooks system to say that none are loaded. + // libraries = HooksManager::getLibraryNames(); + // EXPECT_TRUE(libraries.empty()); + + } +// libraries = HooksManager::getLibraryNames(); +// ASSERT_TRUE(libraries.empty()); + + + // This test verifies that it is possible to select subset of interfaces on + // which server should listen. + TEST_F(Dhcp6ParserTest, selectedInterfaces) { + + // Make sure there is no garbage interface configuration in the CfgMgr. + ASSERT_FALSE(CfgMgr::instance().isActiveIface("eth0")); + ASSERT_FALSE(CfgMgr::instance().isActiveIface("eth1")); + ASSERT_FALSE(CfgMgr::instance().isActiveIface("eth2")); + + ConstElementPtr status; + + string config = "{ \"interfaces\": [ \"eth0\", \"eth1\" ]," + "\"preferred-lifetime\": 3000," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"valid-lifetime\": 4000 }"; + + + ElementPtr json = Element::fromJSON(config); + + EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json)); + + // returned value must be 1 (values error) + // as the pool does not belong to that subnet + ASSERT_TRUE(status); + comment_ = parseAnswer(rcode_, status); + EXPECT_EQ(0, rcode_); + + // eth0 and eth1 were explicitly selected. eth2 was not. + EXPECT_TRUE(CfgMgr::instance().isActiveIface("eth0")); + EXPECT_TRUE(CfgMgr::instance().isActiveIface("eth1")); + EXPECT_FALSE(CfgMgr::instance().isActiveIface("eth2")); + } + + // This test verifies that it is possible to configure the server to listen on + // all interfaces. + TEST_F(Dhcp6ParserTest, allInterfaces) { + + // Make sure there is no garbage interface configuration in the CfgMgr. + ASSERT_FALSE(CfgMgr::instance().isActiveIface("eth0")); + ASSERT_FALSE(CfgMgr::instance().isActiveIface("eth1")); + ASSERT_FALSE(CfgMgr::instance().isActiveIface("eth2")); + + ConstElementPtr status; + + // This configuration specifies two interfaces on which server should listen + // bu also includes keyword 'all'. 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\", \"*\" ]," + "\"preferred-lifetime\": 3000," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"valid-lifetime\": 4000 }"; + + + ElementPtr json = Element::fromJSON(config); + + EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json)); + + // returned value must be 1 (values error) + // as the pool does not belong to that subnet + ASSERT_TRUE(status); + comment_ = parseAnswer(rcode_, status); + EXPECT_EQ(0, rcode_); + + // All interfaces should be now active. + EXPECT_TRUE(CfgMgr::instance().isActiveIface("eth0")); + EXPECT_TRUE(CfgMgr::instance().isActiveIface("eth1")); + EXPECT_TRUE(CfgMgr::instance().isActiveIface("eth2")); + } }; diff --cc src/lib/dhcpsrv/Makefile.am index e1680ac2a3,0925182a47..8641f4f00c --- a/src/lib/dhcpsrv/Makefile.am +++ b/src/lib/dhcpsrv/Makefile.am @@@ -59,10 -59,10 +59,12 @@@ libb10_dhcpsrv_la_CXXFLAGS = $(AM_CXXFL libb10_dhcpsrv_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES) libb10_dhcpsrv_la_LIBADD = $(top_builddir)/src/lib/asiolink/libb10-asiolink.la libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/dhcp/libb10-dhcp++.la +libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/hooks/libb10-hooks.la +libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/log/libb10-log.la libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/util/libb10-util.la libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/cc/libb10-cc.la + libb10_dhcpsrv_la_LIBADD += $(top_builddir)/src/lib/hooks/libb10-hooks.la + libb10_dhcpsrv_la_LDFLAGS = -no-undefined -version-info 3:0:0 if HAVE_MYSQL libb10_dhcpsrv_la_LDFLAGS += $(MYSQL_LIBS) diff --cc src/lib/dhcpsrv/dhcp_parsers.cc index bd0311103f,52c226b6d2..826b957cc8 --- a/src/lib/dhcpsrv/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/dhcp_parsers.cc @@@ -158,76 -184,37 +187,101 @@@ InterfaceListConfigParser::build(ConstE } } - void + void InterfaceListConfigParser::commit() { - /// @todo: Implement per interface listening. Currently always listening - /// on all interfaces. + CfgMgr& cfg_mgr = CfgMgr::instance(); + // Remove active interfaces and clear a flag which marks all interfaces + // active + cfg_mgr.deleteActiveIfaces(); + + if (activate_all_) { + // Activate all interfaces. There is not need to add their names + // explicitly. + cfg_mgr.activateAllIfaces(); + + } else { + // Explicitly add names of the interfaces which server should listen on. + BOOST_FOREACH(std::string iface, interfaces_) { + cfg_mgr.addActiveIface(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); } +// ******************** HooksLibrariesParser ************************* + +HooksLibrariesParser::HooksLibrariesParser(const std::string& param_name) + : libraries_(), changed_(false) +{ + // Sanity check on the name. + if (param_name != "hooks-libraries") { + isc_throw(BadValue, "Internal error. Hooks libraries " + "parser called for the wrong parameter: " << param_name); + } +} + +void +HooksLibrariesParser::build(ConstElementPtr value) { + // Initialize. + libraries_.clear(); + changed_ = false; + + // Extract the list of libraries. + BOOST_FOREACH(ConstElementPtr iface, value->listValue()) { + string libname = iface->str(); + boost::erase_all(libname, "\""); + libraries_.push_back(libname); + } + + // Check if the list of libraries has changed. If not, nothing is done + // - the command "DhcpN libreload" is required to reload the same + // libraries (this prevents needless reloads when anything else in the + // configuration is changed). +/* + vector current_libraries = HooksManager::getLibraryNames(); + if (current_libraries == libraries_) { + return; + } + + // Library list has changed, validate each of the libraries specified. + string error_libs = HooksManager::validateLibraries(libraries_); + if (!error_libs.empty()) { + isc_throw(DhcpConfigError, "hooks libraries failed to validate - " + "library or libraries in error are: " + error_libs); + } +*/ + // The library list has changed and the libraries are valid, so flag for + // update when commit() is called. + changed_ = true; +} + +void +HooksLibrariesParser::commit() { + /// Commits the list of libraries to the configuration manager storage if + /// the list of libraries has changed. + if (changed_) { + HooksManager::loadLibraries(libraries_); + } +} + +// Method for testing +void +HooksLibrariesParser::getLibraries(std::vector& libraries, + bool& changed) { + libraries = libraries_; + changed = changed_; +} + // **************************** OptionDataParser ************************* OptionDataParser::OptionDataParser(const std::string&, OptionStoragePtr options, ParserContextPtr global_context) diff --cc src/lib/dhcpsrv/dhcp_parsers.h index 4e339a50ac,37dc95073e..49c917c78e --- a/src/lib/dhcpsrv/dhcp_parsers.h +++ b/src/lib/dhcpsrv/dhcp_parsers.h @@@ -45,13 -44,11 +45,13 @@@ typedef OptionSpaceContainer OptionStoragePtr; + + /// @brief A template class that stores named elements of a given data type. /// - /// This template class is provides data value storage for configuration - /// parameters of a given data type. The values are stored by parameter name - /// and as instances of type "ValueType". + /// This template class is provides data value storage for configuration parameters + /// of a given data type. The values are stored by parameter name and as instances + /// of type "ValueType". /// /// @param ValueType is the data type of the elements to store. template @@@ -319,80 -302,25 +319,95 @@@ public 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 - std::vector interfaces_; + typedef std::list IfaceListStorage; + IfaceListStorage interfaces_; + + // Should server listen on all interfaces. + bool activate_all_; + + // Parsed parameter name + std::string param_name_; }; +/// @brief parser for hooks library list +/// +/// This parser handles the list of hooks libraries. This is an optional list, +/// which may be empty. +/// +/// However, the parser does more than just check the list of library names. +/// It does two other things: +/// +/// -# The problem faced with the hooks libraries is that we wish to avoid +/// reloading the libraries if they have not changed. (This would cause the +/// "unload" and "load" methods to run. Although libraries should be written +/// to cope with this, it is feasible that such an action may be constly in +/// terms of time and resources, or may cause side effects such as clearning +/// an internal cache.) To this end, the parser also checks the list against +/// the list of libraries current loaded and notes if there are changes. +/// -# If there are, the parser validates the libraries; it opens them and +/// checks that the "version" function exists and returns the correct value. +/// +/// Only if the library list has changed and the libraries are valid will the +/// change be applied. +class HooksLibrariesParser : public DhcpConfigParser { +public: + + /// @brief Constructor + /// + /// As this is a dedicated parser, it must be used to parse + /// "hooks_libraries" parameter only. All other types will throw exception. + /// + /// @param param_name name of the configuration parameter being parsed. + /// + /// @throw BadValue if supplied parameter name is not "hooks_libraries" + HooksLibrariesParser(const std::string& param_name); + + /// @brief Parses parameters value + /// + /// Parses configuration entry (list of parameters) and adds each element + /// to the hooks libraries list. The method also checks whether the + /// list of libraries is the same as that already loaded. If not, it + /// checks each of the libraries in the list for validity (they exist and + /// have a "version" function that returns the correct value). + /// + /// @param value pointer to the content of parsed values + virtual void build(isc::data::ConstElementPtr value); + + /// @brief Commits hooks libraries data + /// + /// Providing that the specified libraries are valid and are different + /// to those already loaded, this method loads the new set of libraries + /// (and unloads the existing set). + virtual void commit(); + + /// @brief Returns list of parsed libraries + /// + /// Principally for testing, this returns the list of libraries as well as + /// an indication as to whether the list is different from the list of + /// libraries already loaded. + /// + /// @param libraries (out) List of libraries that were specified in the + /// new configuration. + /// @param changed (out) true if the list is different from that currently + /// loaded. + void getLibraries(std::vector& libraries, bool& changed); + +private: + /// List of hooks libraries. + std::vector libraries_; + + /// Indicator flagging that the list of libraries has changed. + bool changed_; +}; /// @brief Parser for option data value. /// diff --cc src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 5e935a289b,6704c57f91..747e38da1c --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@@ -357,15 -373,15 +389,15 @@@ public return (parser); } - /// @brief Convenicee method for parsing a configuration - /// + /// @brief Convenience method for parsing a configuration + /// /// Given a configuration string, convert it into Elements - /// and parse them. + /// and parse them. /// @param config is the configuration string to parse /// - /// @return retuns 0 if the configuration parsed successfully, + /// @return retuns 0 if the configuration parsed successfully, /// non-zero otherwise failure. - int parseConfiguration (std::string &config) { + int parseConfiguration(const std::string& config) { int rcode_ = 1; // Turn config into elements. // Test json just to make sure its valid. @@@ -488,9 -492,8 +520,9 @@@ TEST_F(ParseConfigTest, basicOptionDefT int rcode = parseConfiguration(config); ASSERT_TRUE(rcode == 0); + // Verify that the option definition can be retrieved. - OptionDefinitionPtr def = getOptionDef("isc", 100); + OptionDefinitionPtr def = getOptionDef("isc", 100); ASSERT_TRUE(def); // Verify that the option definition is correct.