[3259] Changes after review

doc/examples/kea4/hooks.json

@@ -0,0 +1,43 @@
+# This is an example configuration file for the DHCPv4 server in Kea
+# illustrating the configuration of hooks libraries.  It uses a basic scenario
+# of one IPv6 subnet configured with the default values for all parameters.
+
+{"Dhcp4":
+
+{
+# Kea is told to listen on the ethX interface only.
+  "interfaces-config": {
+    "interfaces": [ "ethX" ]
+  },
+
+# Set up the storage for leases.
+  "lease-database": {
+    "type": "memfile"
+  },
+
+# Define a single subnet.
+  "subnet4": [
+    {
+      "pools": [ { "pool": "192.0.2.1 - 19.2.0.2.200" } ],
+      "subnet": "192.0.2.0/24",
+      "interface": "ethX"
+    }
+  ],
+
+# Set up the hooks libraries.  For this example, we assume that two libraries
+# are loaded, called "security" and "charging".  Note that order is important:
+# "security" is specified first so if both libraries supply a hook function
+# for a given hook, the function in "security" will be called before that in
+# "charging".
+
+  "hooks-libraries": [
+     {
+        "library": "/opt/lib/security.so"
+     },
+     {
+        "library": "/opt/lib/charging.so"
+     }
+  ]
+}
+
+}

doc/examples/kea6/hooks.json

@@ -0,0 +1,43 @@
+# This is an example configuration file for the DHCPv6 server in Kea
+# illustrating the configuration of hooks libraries.  It uses a basic scenario
+# of one IPv6 subnet configured with the default values for all parameters.
+
+{"Dhcp6":
+
+{
+# Kea is told to listen on the ethX interface only.
+  "interfaces-config": {
+    "interfaces": [ "ethX" ]
+  },
+
+# Set up the storage for leases.
+  "lease-database": {
+    "type": "memfile"
+  },
+
+# Define a single subnet.
+  "subnet6": [
+    {
+      "pools": [ { "pool": "2001:db8:1::/80" } ],
+      "subnet": "2001:db8:1::/64",
+      "interface": "ethX"
+    }
+  ],
+
+# Set up the hooks libraries.  For this example, we assume that two libraries
+# are loaded, called "security" and "charging".  Note that order is important:
+# "security" is specified first so if both libraries supply a hook function
+# for a given hook, the function in "security" will be called before that in
+# "charging".
+
+  "hooks-libraries": [
+     {
+        "library": "/opt/lib/security.so"
+     },
+     {
+        "library": "/opt/lib/charging.so"
+     }
+  ]
+}
+
+}

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -2886,7 +2886,6 @@ TEST_F(Dhcp4ParserTest, vendorOptionsCsv) {
 // 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 =
@@ -2900,9 +2899,7 @@ buildHooksLibrariesConfig(const std::vector<std::string>& libraries) {
         if (i > 0) {
             config += string(", ");
         }
-        config += (string("{ \"library\": ") +
-                      quote + libraries[i] + quote) +
-                   string("}");
+        config += (string("{ \"library\": \"") + libraries[i] + string("\" }"));
     }
 
     // Append the remainder of the configuration.

src/lib/dhcpsrv/parsers/dhcp_parsers.cc

@@ -248,16 +248,10 @@ HooksLibrariesParser::HooksLibrariesParser(const std::string& param_name)
     }
 }
 
-// The pre-Kea-1.0 syntax for the hooks libraries was a simple list, e.g.
+// The syntax for specifying hooks libraries allow for library-specific
+// parameters to be specified along with the library, e.g.
 //
-//      ["hook-lib-1.so", "hook-lib-2.so", ... ]
-//
-// With Kea 1.0,. the sytntax was altered to allow for parameters to be
-// specified: instead of a list of strings, it is a list of maps, with
-// each map holding an element "library" giving the name of the library and
-// an element "parameters" listing the library's parameters, e.g.
-//
-//      [
+//      "hooks-libraries": [
 //          {
 //              "library": "hook-lib-1.so",
 //              "parameters": {
@@ -271,8 +265,12 @@ HooksLibrariesParser::HooksLibrariesParser(const std::string& param_name)
 //          }
 //      ]
 //
-// Kea 1.0 has not yet implemented parameters, so additional elements in the
-// map are ignored.
+// Kea has not yet implemented parameters, so the parsing code only checks
+// that:
+//
+// 1. Each element in the hooks-libraries list is a map
+// 2. The map contains an element "library" whose value is a string: all
+//    other elements in the map are ignored.
 void
 HooksLibrariesParser::build(ConstElementPtr value) {
     // Initialize.
@@ -281,18 +279,32 @@ HooksLibrariesParser::build(ConstElementPtr value) {
 
     // This is the new syntax.  Iterate through it and get each map.
     BOOST_FOREACH(ConstElementPtr library_entry, value->listValue()) {
+        // Is it a map?
+        if (library_entry->getType() != Element::map) {
+            isc_throw(DhcpConfigError, "hooks library configuration error:"
+                " one or more entries in the hooks-libraries list is not"
+                " a map (" << library_entry->getPosition() << ")");
+        }
+
         // Iterate iterate through each element in the map.  We check
         // whether we have found a library element.
         bool lib_found = false;
         BOOST_FOREACH(ConfigPair entry_item, library_entry->mapValue()) {
             if (entry_item.first == "library") {
-                // Name of the library. Add it to the list after trimming
-                // quotes.
+                if (entry_item.second->getType() != Element::string) {
+                    isc_throw(DhcpConfigError, "hooks library configuration"
+                        " error: value of 'library' element is not a valid"
+                        " path to a hooks library (" <<
+                        entry_item.second->getPosition() << ")");
+                }
+
+                // Get the name of the library and add it to the list after
+                // removing quotes.
                 string libname = (entry_item.second)->stringValue();
                 boost::erase_all(libname, "\"");
                 libraries_.push_back(libname);
 
-                // ... and we have found the library name.
+                // Note we have found the library name.
                 lib_found = true;
             }
         }

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -830,10 +830,9 @@ TEST_F(ParseConfigTest, optionDataMinimalWithOptionDef) {
 // lib1 - No parameters
 // lib2 - Empty parameters statement
 // lib3 - Valid parameters
-// lib4 - No "library" in the map
 std::string
 setHooksLibrariesConfig(const char* lib1 = NULL, const char* lib2 = NULL,
-                        const char* lib3 = NULL, const char* lib4 = NULL) {
+                        const char* lib3 = NULL) {
     const string lbrace("{");
     const string rbrace("}");
     const string quote("\"");
@@ -865,15 +864,6 @@ setHooksLibrariesConfig(const char* lib1 = NULL, const char* lib2 = NULL,
                 config += string("    \"bvalue\": true");     // Boolean value
                 config += string("}");
                 config += rbrace;
-
-                if (lib4 != NULL) {
-                    // Library 4 omits the "library" in the map statement
-                    config += comma_space + lbrace;
-                    config += string("\"parameters\": {");
-                    config += string("    \"dummy\": \"string value\"");
-                    config += string("}");
-                    config += rbrace;
-                }
             }
         }
     }
@@ -1165,25 +1155,49 @@ TEST_F(ParseConfigTest, reconfigureInvalidHooksLibraries) {
     EXPECT_EQ(CALLOUT_LIBRARY_1, hooks_libraries[0]);
 }
 
-// Check that trying to reconfigure with a map element missing the "library"
-// element fails.
-TEST_F(ParseConfigTest, missingLibraryHooksLibraries) {
-    // Set up the invalid configuration (the error occcurs in the configuration
-    // of the fourth library).  Multiple specifications of the same library
-    // are used for conveniencr: this is OK, as the failure should occur in the
-    // parsing, not the loading, of the libraries.
-    std::string config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1,
-                                                 CALLOUT_LIBRARY_2,
-                                                 CALLOUT_LIBRARY_1,
-                                                 CALLOUT_LIBRARY_2);
+// Check that if hooks-libraries contains invalid syntax, it is detected.
+TEST_F(ParseConfigTest, invalidSyntaxHooksLibraries) {
 
-    // The parse should fail...
-    int rcode = parseConfiguration(config);
+    // Element holds a mixture of (valid) maps and non-maps.
+    string config1 = "{ \"hooks-libraries\": [ "
+        "{ \"library\": \"/opt/lib/lib1\" }, "
+        "\"/opt/lib/lib2\" "
+        "] }";
+    string error1 = "one or more entries in the hooks-libraries list is not"
+                    " a map";
+
+    int rcode = parseConfiguration(config1);
     ASSERT_NE(0, rcode);
+    EXPECT_TRUE(error_text_.find(error1) != string::npos) <<
+        "Error text returned from parse failure is " << error_text_;
+
+    // Element holds valid maps, except one where the library element is not
+    // a string.
+    string config2 = "{ \"hooks-libraries\": [ "
+        "{ \"library\": \"/opt/lib/lib1\" }, "
+        "{ \"library\": 123 } "
+        "] }";
+    string error2 = "value of 'library' element is not a valid"
+                    " path to a hooks library";
+
+    rcode = parseConfiguration(config2);
+    ASSERT_NE(0, rcode);
+    EXPECT_TRUE(error_text_.find(error2) != string::npos) <<
+        "Error text returned from parse failure is " << error_text_;
 
-    // ... and the error indicate that it an element was found that was
-    // missing the name of the library.
-    EXPECT_FALSE(error_text_.find("missing the name") == string::npos) <<
+    // Element holds valid maps, except one that does not contain a
+    // 'library' element.
+    string config3 = "{ \"hooks-libraries\": [ "
+        "{ \"library\": \"/opt/lib/lib1\" }, "
+        "{ \"parameters\": { \"alpha\": 123 } }, "
+        "{ \"library\": \"/opt/lib/lib2\" } "
+        "] }";
+    string error3 = "one or more hooks-libraries elements are missing the"
+                    " name of the library";
+
+    rcode = parseConfiguration(config3);
+    ASSERT_NE(0, rcode);
+    EXPECT_TRUE(error_text_.find(error3) != string::npos) <<
         "Error text returned from parse failure is " << error_text_;
 }