[2981] Configuration changes now load and unload hooks libraries

configure.ac

@@ -1381,6 +1381,8 @@ AC_OUTPUT([doc/version.ent
            src/bin/dbutil/run_dbutil.sh
            src/bin/dbutil/tests/dbutil_test.sh
            src/bin/ddns/ddns.py
+           src/bin/dhcp4/tests/marker_file.h
+           src/bin/dhcp4/tests/test_libraries.h
            src/bin/dhcp6/tests/marker_file.h
            src/bin/dhcp6/tests/test_libraries.h
            src/bin/xfrin/tests/xfrin_test

src/bin/dhcp4/config_parser.cc

@@ -364,6 +364,8 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
                                     globalContext()->string_values_);
     } else if (config_id.compare("lease-database") == 0) {
         parser = new DbAccessParser(config_id);
+    } else if (config_id.compare("hooks-libraries") == 0) {
+        parser = new HooksLibrariesParser(config_id);
     } else {
         isc_throw(NotImplemented,
                 "Parser error: Global configuration parameter not supported: "
@@ -399,6 +401,11 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
     ParserPtr option_parser;
     ParserPtr iface_parser;
 
+    // Some of the parsers alter the state of the system in a way that can't
+    // easily be undone. (Or alter it in a way such that undoing the change has
+    // the same risk of failurre 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
@@ -434,6 +441,12 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
                 // parser and can be run here before any other parsers.
                 iface_parser = parser;
                 parser->build(config_pair.second);
+            } else if (config_pair.first == "hooks-libraries") {
+                // Executing commit will alter currently-loaded hooks
+                // libraries.  Check if the supplied libraries are valid,
+                // but defer the commit until everything else has committed.
+                hooks_parser_ = parser;
+                parser->build(config_pair.second);
             } else {
                 // Those parsers should be started before other
                 // parsers so we can call build straight away.
@@ -514,6 +527,12 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
         return (answer);
     }
 
+    // Now commit changes that have been validated but not yet committed,
+    // and which can't be rolled back.
+    if (hooks_parser_) {
+        hooks_parser_->commit();
+    }
+
     LOG_INFO(dhcp4_logger, DHCP4_CONFIG_COMPLETE).arg(config_details);
 
     // Everything was fine. Configuration is successful.

src/bin/dhcp4/dhcp4.spec

@@ -3,6 +3,19 @@
     "module_name": "Dhcp4",
     "module_description": "DHCPv4 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": "interfaces",
         "item_type": "list",
         "item_optional": false,
@@ -272,7 +285,13 @@
                     "item_optional": true
                 }
             ]
+        },
+
+        {
+            "command_name": "libreload",
+            "command_description": "Reloads the current hooks libraries.", 
         }
+
     ]
   }
 }

src/bin/dhcp4/tests/Makefile.am

@@ -48,6 +48,16 @@ TESTS_ENVIRONMENT = \
 
 TESTS =
 if HAVE_GTEST
+# Build shared libraries for testing.
+lib_LTLIBRARIES = libco1.la libco2.la
+
+libco1_la_SOURCES  = callout_library_1.cc callout_library_common.h
+libco1_la_CXXFLAGS = $(AM_CXXFLAGS)
+libco1_la_CPPFLAGS = $(AM_CPPFLAGS)
+
+libco2_la_SOURCES  = callout_library_2.cc callout_library_common.h
+libco2_la_CXXFLAGS = $(AM_CXXFLAGS)
+libco2_la_CPPFLAGS = $(AM_CPPFLAGS)
 
 TESTS += dhcp4_unittests
 
@@ -59,6 +69,7 @@ dhcp4_unittests_SOURCES += dhcp4_srv_unittest.cc
 dhcp4_unittests_SOURCES += ctrl_dhcp4_srv_unittest.cc
 dhcp4_unittests_SOURCES += config_parser_unittest.cc
 nodist_dhcp4_unittests_SOURCES = ../dhcp4_messages.h ../dhcp4_messages.cc
+nodist_dhcp4_unittests_SOURCES += marker_file.h test_libraries.h
 
 dhcp4_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
 dhcp4_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS)

src/bin/dhcp4/tests/callout_library_1.cc

+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+/// @file
+/// @brief Marker file callout library
+///
+/// This is the source of a test library for the DHCP parser and configuration
+/// tests.  See callout_common.cc for details.
+
+static const int LIBRARY_NUMBER = 1;
+#include "callout_library_common.h"

src/bin/dhcp4/tests/callout_library_2.cc

+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+/// @file
+/// @brief Marker file callout library
+///
+/// This is the source of a test library for the DHCP parser and configuration
+/// tests.  See callout_common.cc for details.
+
+static const int LIBRARY_NUMBER = 2;
+#include "callout_library_common.h"

src/bin/dhcp4/tests/callout_library_common.h

+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+/// @file
+/// @brief Marker file callout library
+///
+/// This is the source of a test library for the DHCP parser and configuration
+/// tests.
+///
+/// To check that they libraries are loaded and unloaded correctly, the load
+/// and unload functions in this library maintain two marker files - the load
+/// marker file and the unload marker file.  The functions append a single
+/// to the single line in the file, creating the file if need be.  In
+/// this way, the test code can determine whether the load/unload functions
+/// have been run and, if so, in what order.
+///
+/// This file is the common library file for the tests.  It will not compile
+/// by itself - it is included into each callout library which specifies the
+/// missing constant LIBRARY_NUMBER before the inclusion.
+
+#include <hooks/hooks.h>
+#include "marker_file.h"
+
+#include <fstream>
+
+using namespace isc::hooks;
+using namespace std;
+
+extern "C" {
+
+/// @brief Append digit to marker file
+///
+/// If the marker file does not exist, create it.  Then append the single
+/// digit (given by the constant LIBRARY_NUMBER) defined earlier to it and
+/// close the file.
+///
+/// @param name Name of the file to open
+///
+/// @return 0 on success, non-zero on error.
+int
+appendDigit(const char* name) {
+    // Open the file and check if successful.
+    fstream file(name, fstream::out | fstream::app);
+    if (!file.good()) {
+        return (1);
+    }
+
+    // Add the library number to it and close.
+    file << LIBRARY_NUMBER;
+    file.close();
+
+    return (0);
+}
+
+// Framework functions
+int
+version() {
+    return (BIND10_HOOKS_VERSION);
+}
+
+int load(LibraryHandle&) {
+    return (appendDigit(LOAD_MARKER_FILE));
+}
+
+int unload() {
+    return (appendDigit(UNLOAD_MARKER_FILE));
+}
+
+};

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -25,6 +25,10 @@
 #include <dhcp/option_int.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/cfgmgr.h>
+#include <hooks/hooks_manager.h>
+
+#include "marker_file.h"
+#include "test_libraries.h"
 
 #include <boost/foreach.hpp>
 #include <boost/scoped_ptr.hpp>
@@ -34,12 +38,13 @@
 #include <sstream>
 #include <limits.h>
 
-using namespace std;
 using namespace isc;
-using namespace isc::dhcp;
 using namespace isc::asiolink;
-using namespace isc::data;
 using namespace isc::config;
+using namespace isc::data;
+using namespace isc::dhcp;
+using namespace isc::hooks;
+using namespace std;
 
 namespace {
 
@@ -78,6 +83,10 @@ public:
 
     ~Dhcp4ParserTest() {
         resetConfiguration();
+
+        // ... and delete the hooks library marker files if present
+        unlink(LOAD_MARKER_FILE);
+        unlink(UNLOAD_MARKER_FILE);
     };
 
     /// @brief Create the simple configuration with single option.
@@ -236,6 +245,57 @@ public:
                             expected_data_len));
     }
 
+    /// @brief Parse and Execute configuration
+    ///
+    /// Parses a configuration and executes a configuration of the server.
+    /// If the operation fails, the current test will register a failure.
+    ///
+    /// @param config Configuration to parse
+    /// @param operation Operation being performed.  In the case of an error,
+    ///        the error text will include the string "unable to <operation>.".
+    ///
+    /// @return true if the configuration succeeded, false if not.  In the
+    ///         latter case, a failure will have been added to the current test.
+    bool
+    executeConfiguration(const std::string& config, const char* operation) {
+        ConstElementPtr status;
+        try {
+            ElementPtr json = Element::fromJSON(config);
+            status = configureDhcp4Server(*srv_, json);
+        } catch (const std::exception& ex) {
+            ADD_FAILURE() << "Unable to " << operation << ". "
+                   << "The following configuration was used: " << std::endl
+                   << config << std::endl
+                   << " and the following error message was returned:"
+                   << ex.what() << std::endl;
+            return (false);
+        }
+
+        // The status object must not be NULL
+        if (!status) {
+            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);
+        if (rcode_ != 0) {
+            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
@@ -244,42 +304,71 @@ public:
     /// contents of the database do not affect result of
     /// subsequent tests.
     void resetConfiguration() {
-        ConstElementPtr status;
-
         string config = "{ \"interfaces\": [ \"*\" ],"
+            "\"hooks-libraries\": [ ], "
             "\"rebind-timer\": 2000, "
             "\"renew-timer\": 1000, "
             "\"valid-lifetime\": 4000, "
             "\"subnet4\": [ ], "
             "\"option-def\": [ ], "
             "\"option-data\": [ ] }";
+        static_cast<void>(executeConfiguration(config,
+                                               "reset configuration database"));
+    }
 
-        try {
-            ElementPtr json = Element::fromJSON(config);
-            status = configureDhcp4Server(*srv_, json);
-        } catch (const std::exception& ex) {
-            FAIL() << "Fatal error: unable to reset configuration database"
-                   << " after the test. The following configuration was used"
-                   << " to reset database: " << std::endl
-                   << config << std::endl
-                   << " and the following error message was returned:"
-                   << ex.what() << std::endl;
+    /// @brief Check marker file
+    ///
+    /// Marker files are used by the load/unload functions in the hooks
+    /// libraries in these tests to signal whether they have been loaded or
+    /// unloaded.  The file (if present) contains a single line holding
+    /// a set of characters.
+    ///
+    /// This convenience function checks the file to see if the characters
+    /// are those expected.
+    ///
+    /// @param name Name of the marker file.
+    /// @param expected Characters expected.  If a marker file is present,
+    ///        it is expected to contain characters.  Therefore a value of NULL
+    ///        is used to signify that the marker file is not expected to be
+    ///        present.
+    ///
+    /// @return true if all tests pass, false if not (in which case a failure
+    ///         will have been logged).
+    bool
+    checkMarkerFile(const char* name, const char* expected) {
+        // Open the file for input
+        fstream file(name, fstream::in);
+
+        // Is it open?
+        if (!file.is_open()) {
+
+            // No.  This is OK if we don't expected is to be present but is
+            // a failure otherwise.
+            if (expected == NULL) {
+                return (true);
+            }
+            ADD_FAILURE() << "Unable to open " << name << ". It was expected "
+                          << "to be present and to contain the string '"
+                          << expected << "'";
+            return (false);
+        } else if (expected == NULL) {
+
+            // File is open but we don't expect it to be present.
+            ADD_FAILURE() << "Opened " << name << " but it is not expected to "
+                          << "be present.";
+            return (false);
         }
 
-        // 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;
-        }
+        // OK, is open, so read the data and see what we have.  Compare it
+        // against what is expected.
+        string content;
+        getline(file, content);
 
-        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 expected_str(expected);
+        EXPECT_EQ(expected_str, content) << "Data was read from " << name;
+        file.close();
+
+        return (expected_str == content);
     }
 
     boost::scoped_ptr<Dhcpv4Srv> srv_;
@@ -1750,6 +1839,156 @@ TEST_F(Dhcp4ParserTest, stdOptionDataEncapsulate) {
     EXPECT_FALSE(desc.option->getOption(3));
 }
 
+// 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<std::string>& libraries) {
+    const string quote("\"");
+
+    // Create the first part of the configuration string.
+    string config =
+        "{ \"interfaces\": [ \"*\" ],"
+            "\"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\": \"dhcp-message\","
+        "    \"space\": \"dhcp4\","
+        "    \"code\": 56,"
+        "    \"data\": \"AB CDEF0105\","
+        "    \"csv-format\": False"
+        " },"
+        " {"
+        "    \"name\": \"foo\","
+        "    \"space\": \"isc\","
+        "    \"code\": 56,"
+        "    \"data\": \"1234\","
+        "    \"csv-format\": True"
+        " } ],"
+        "\"option-def\": [ {"
+        "    \"name\": \"foo\","
+        "    \"code\": 56,"
+        "    \"type\": \"uint32\","
+        "    \"array\": False,"
+        "    \"record-types\": \"\","
+        "    \"space\": \"isc\","
+        "    \"encapsulate\": \"\""
+        " } ],"
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"subnet\": \"192.0.2.0/24\""
+        " } ]"
+        "}");
+
+    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<std::string> 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(Dhcp4ParserTest, NoHooksLibraries) {
+    // Ensure that no libraries are loaded at the start of the test.
+    std::vector<std::string> 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;
+    }
+
+    // No libraries should be loaded at the end of the test.
+    libraries = HooksManager::getLibraryNames();
+    ASSERT_TRUE(libraries.empty());
+}
+
+// Verify parsing fails with one library that will fail validation.
+TEST_F(Dhcp4ParserTest, InvalidLibrary) {
+    // Ensure that no libraries are loaded at the start of the test.
+    std::vector<std::string> 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 = configureDhcp4Server(*srv_, json));
+
+    // The status object must not be NULL
+    ASSERT_TRUE(status);
+
+    // Returned value should not be 0
+    comment_ = parseAnswer(rcode_, status);
+    EXPECT_NE(0, rcode_);
+}
+
+// Verify the configuration of hooks libraries with two being specified.
+TEST_F(Dhcp4ParserTest, LibrariesSpecified) {
+    // Ensure that no libraries are loaded at the start of the test.
+    std::vector<std::string> 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,
+                                     "load two valid libraries"));
+
+    // Expect two libraries to be loaded in the correct order (load marker file
+    // is present, no unload marker file).
+     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());
+
+}
+
 // This test verifies that it is possible to select subset of interfaces
 // on which server should listen.
 TEST_F(Dhcp4ParserTest, selectedInterfaces) {

src/bin/dhcp4/tests/marker_file.h.in

+// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef MARKER_FILE_H
+#define MARKER_FILE_H
+
+/// @file
+/// Define a marker file that is used in tests to prove that an "unload"
+/// function has been called.