[3259] Updates to handle new syntax only

As some incompatible changes have already been made to the hooks
interface (removal of the setSkip method), it seems pointless to
support both old and new hoos-libraries syntax.  These modifications
remove support for the old syntax.

doc/guide/hooks.xml

@@ -70,12 +70,6 @@
         a library.  The change has been made in Kea 1.0 to facilitate the
         specification of library-specific parameters, a feature that will be
         added to a future version of Kea.
-      </para>
-      <para>
-        To ease the transtion, the old syntax is still accepted, although a
-        warning message is issued when it is encountered.  Users are urged
-        to switch to the new syntax as soon as pssible: the old syntax will
-        be withdrawn when parameters are finally added to Kea.
       </para></note>
       <para>
       Notes:

src/bin/dhcp4/tests/config_parser_unittest.cc

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

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -176,16 +176,6 @@ have been experienced.  Any such errors should have preceding entries in the
 log with details.  No further attempts to communicate with kea-dhcp-ddns will
 be made without intervention.
 
-% DHCPSRV_HOOK_DEPRECATED_SYNTAX hook libraries specified using deprecated syntax, please update your configuration
-This warning message is issued when a configuration file is loaded and Kea notices
-that the hooks libraries have been specified as a simple list of libraries (the
-syntax used in Kea 0.9.2 and earlier).  In Kea 1.0 the syntax was changed to allow the
-specification of library-specific parameters.
-
-To ease the transition, the old syntax is accepted for now, but a future version of
-Kea will no longer accept it.  Please update your configuration files to use the new
-syntax (described in the Kea Administrator Reference Manual).
-
 % DHCPSRV_HOOK_LEASE4_RENEW_SKIP DHCPv4 lease was not renewed because a callout set the skip flag.
 This debug message is printed when a callout installed on lease4_renew
 hook point set the skip flag. For this particular hook point, the setting

src/lib/dhcpsrv/parsers/dhcp_parsers.cc

@@ -18,7 +18,6 @@
 #include <dhcp/libdhcp++.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/cfg_option.h>
-#include <dhcpsrv/dhcpsrv_log.h>
 #include <dhcpsrv/parsers/dhcp_parsers.h>
 #include <dhcpsrv/cfg_mac_source.h>
 #include <hooks/hooks_manager.h>
@@ -274,77 +273,35 @@ HooksLibrariesParser::HooksLibrariesParser(const std::string& param_name)
 //
 // Kea 1.0 has not yet implemented parameters, so additional elements in the
 // map are ignored.
-//
-// The following code eases the transition between the two syntaxes as both
-// are allowed, although the old syntax gives a warning message.
 void
 HooksLibrariesParser::build(ConstElementPtr value) {
     // Initialize.
     libraries_.clear();
     changed_ = false;
 
-    // Iterate through the list of values to see what is there.  This list
-    // should be all strings (old syntax) or all maps (new syntax).
-    bool all_string = true;
-    bool all_map = true;
+    // This is the new syntax.  Iterate through it and get each map.
     BOOST_FOREACH(ConstElementPtr library_entry, value->listValue()) {
-        if (library_entry->getType() == Element::string) {
-            all_map = false;
-        } else if  (library_entry->getType() == Element::map) {
-            all_string = false;
-        } else {
-            all_map = false;
-            all_string = false;
-        }
-    }
-
-    if (all_string && all_map) {
-        // The list must be empty.  This is valid in both syntaxes: do nothing
-        // as this is taken care of below.
-
-    } else if (all_string) {
-        // This is the old (pre Kea-1.0 syntax).  Warn about it, but
-        // otherwise accept it.
-        LOG_WARN(dhcpsrv_logger, DHCPSRV_HOOK_DEPRECATED_SYNTAX);
-
-        // Iterate through each entry in the list - this is just the library
-        // name.  Just remove quotes and add to the library list.
-        BOOST_FOREACH(ConstElementPtr library_entry, value->listValue()) {
-            string libname = library_entry->stringValue();
-            boost::erase_all(libname, "\"");
-            libraries_.push_back(libname);
-        }
-
-    } else if (all_map) {
-        // This is the new syntax.  Iterate through it and get each map.
-        BOOST_FOREACH(ConstElementPtr library_entry, value->listValue()) {
-            // 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.
-                    string libname = (entry_item.second)->stringValue();
-                    boost::erase_all(libname, "\"");
-                    libraries_.push_back(libname);
-
-                    // ... and we have found the library name.
-                    lib_found = true;
-                }
-            }
-            if (! lib_found) {
-                isc_throw(DhcpConfigError, "hooks library configuration error:"
-                    " one or more hooks-libraries elements are missing the "
-                    " name of the library"  <<
-                    " (" << 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.
+                string libname = (entry_item.second)->stringValue();
+                boost::erase_all(libname, "\"");
+                libraries_.push_back(libname);
+
+                // ... and we have found the library name.
+                lib_found = true;
             }
         }
-    } else {
-        isc_throw(DhcpConfigError, "hooks library configuration error:"
-            " list of hooks libraries is not a list of maps, each map"
-            " containing a 'library' element" <<
-            " (" << value->getPosition() << ")");
+        if (! lib_found) {
+            isc_throw(DhcpConfigError, "hooks library configuration error:"
+                " one or more hooks-libraries elements are missing the"
+                " name of the library"  <<
+                " (" << library_entry->getPosition() << ")");
+        }
     }
 
     // Check if the list of libraries has changed.  If not, nothing is done

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -823,50 +823,17 @@ TEST_F(ParseConfigTest, optionDataMinimalWithOptionDef) {
 }
 
 /// The next set of tests check basic operation of the HooksLibrariesParser.
-
-// Utility function for setting up the "hooks-libraries" configuration.
 //
-// Returns a hooks-libraries configuration element that contains zero to
-// four libraries, depending on what arguments are supplied.  The first
-/// three libraries have valid syntax, the last does not.
-//
-// This function uses the old syntax (Kea 0.9.1 and 0.9.2)
-std::string
-setHooksLibrariesConfig(const char* lib1 = NULL, const char* lib2 = NULL,
-                        const char* lib3 = NULL, const char* lib4 = NULL) {
-    const string quote("\"");
-    const string comma_space(", ");
-
-    string config = string("{ \"hooks-libraries\": [");
-    if (lib1 != NULL) {
-        config += (quote + string(lib1) + quote);
-        if (lib2 != NULL) {
-            config += (comma_space + quote + string(lib2) + quote);
-            if (lib3 != NULL) {
-                config += (comma_space + quote + string(lib3) + quote);
-                if (lib4 != NULL) {
-                    config += comma_space + string("{ \"library\": ") + 
-                              quote + string(lib4) + quote + string(" }");
-                }
-            }
-        }
-    }
-    config += std::string("] }");
-
-    return (config);
-}
-
-// Set up four libraries using the new configuration syntax.  As the new syntax
-// syntax allows for parameters, each library will have a differing sets of
-// parameters:
+// Convenience function to set a configuration of zero or more hooks
+// libraries:
 //
 // lib1 - No parameters
 // lib2 - Empty parameters statement
 // lib3 - Valid parameters
-// lib4 - Invalid (old syntax)
+// lib4 - No "library" in the map
 std::string
-setHooksLibrariesNewConfig(const char* lib1 = NULL, const char* lib2 = NULL,
-                           const char* lib3 = NULL, const char* lib4 = NULL) {
+setHooksLibrariesConfig(const char* lib1 = NULL, const char* lib2 = NULL,
+                        const char* lib3 = NULL, const char* lib4 = NULL) {
     const string lbrace("{");
     const string rbrace("}");
     const string quote("\"");
@@ -900,8 +867,12 @@ setHooksLibrariesNewConfig(const char* lib1 = NULL, const char* lib2 = NULL,
                 config += rbrace;
 
                 if (lib4 != NULL) {
-                    // Library 4 is invalidly specified (as a plain string)
-                    config += comma_space + quote + string(lib4) + quote;
+                    // 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;
                 }
             }
         }
@@ -911,61 +882,16 @@ setHooksLibrariesNewConfig(const char* lib1 = NULL, const char* lib2 = NULL,
     return (config);
 }
 
-// The tests of both syntaxes are more or less identical (there is an extra
-// test for invalid parameters with the new syntax).  The following class allows
-// the configuration for the tests to be generated by either the old or new
-// syntax.
-//
-// The type of the generator function is a template parameter to a Gtest
-// parent class.  This class itself is instantiated multiple times with
-// the INSTANTIATE_TEST_CASE_P macro, where a value of the templated parameter
-// is made available via the Gtest parent class's GetParam() method.
-//
-// This class stores the address of the configuration generation function and
-// uses it to obtain the configuration string when needed.
-
-class HooksParseConfigTest : public ParseConfigTest,
-    public ::testing::WithParamInterface<
-        std::string (*)(const char*, const char*, const char*, const char*)
-    > {
-
-public:
-    // Virtual desstructor: needed as the parent has a virtual destructor.
-    virtual ~HooksParseConfigTest() {}
-
-    // SetUp - stiore the configuration generation function
-    //
-    // This follows the example in the Gtest documentation, where the generator
-    // function is set in SetUp rather than the class constructor.
-    virtual void SetUp() {
-        generator_ = GetParam();
-    };
-
-    // createConfig - create Configuration String
-    //
-    // Uses the stored generator function to generate a configuration string
-    // based on the number of libraries requested.
-    std::string
-    createConfig(const char* lib1 = NULL, const char* lib2 = NULL,
-		         const char* lib3 = NULL, const char* lib4 = NULL) {
-       return ((*generator_)(lib1, lib2, lib3, lib4));
-    }
-
-    // Pointer to the configuration string generator function.
-    std::string (*generator_)(const char* lib1, const char* lib2,
-                              const char* lib3, const char* lib4);
-};
-
 };  // Anonymous namespace
 
 // hooks-libraries element that does not contain anything.
-TEST_P(HooksParseConfigTest, noHooksLibraries) {
+TEST_F(ParseConfigTest, noHooksLibraries) {
     // Check that no libraries are currently loaded
     vector<string> hooks_libraries = HooksManager::getLibraryNames();
     EXPECT_TRUE(hooks_libraries.empty());
 
     // Create an empty hooks-libraries configuration element.
-    const string config = createConfig();
+    const string config = setHooksLibrariesConfig();
 
     // Verify that the configuration string parses.
     const int rcode = parseConfiguration(config);
@@ -984,13 +910,13 @@ TEST_P(HooksParseConfigTest, noHooksLibraries) {
 }
 
 // hooks-libraries element that contains a single library.
-TEST_P(HooksParseConfigTest, oneHooksLibrary) {
+TEST_F(ParseConfigTest, oneHooksLibrary) {
     // Check that no libraries are currently loaded
     vector<string> hooks_libraries = HooksManager::getLibraryNames();
     EXPECT_TRUE(hooks_libraries.empty());
 
     // Configuration with hooks-libraries set to a single library.
-    const string config = createConfig(CALLOUT_LIBRARY_1);
+    const string config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1);
 
     // Verify that the configuration string parses.
     const int rcode = parseConfiguration(config);
@@ -1011,14 +937,14 @@ TEST_P(HooksParseConfigTest, oneHooksLibrary) {
 }
 
 // hooks-libraries element that contains two libraries
-TEST_P(HooksParseConfigTest, twoHooksLibraries) {
+TEST_F(ParseConfigTest, twoHooksLibraries) {
     // Check that no libraries are currently loaded
     vector<string> hooks_libraries = HooksManager::getLibraryNames();
     EXPECT_TRUE(hooks_libraries.empty());
 
     // Configuration with hooks-libraries set to two libraries.
-    const string config = createConfig(CALLOUT_LIBRARY_1,
-                                       CALLOUT_LIBRARY_2);
+    const string config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1,
+                                                  CALLOUT_LIBRARY_2);
 
     // Verify that the configuration string parses.
     const int rcode = parseConfiguration(config);
@@ -1041,14 +967,14 @@ TEST_P(HooksParseConfigTest, twoHooksLibraries) {
 }
 
 // Configure with two libraries, then reconfigure with the same libraries.
-TEST_P(HooksParseConfigTest, reconfigureSameHooksLibraries) {
+TEST_F(ParseConfigTest, reconfigureSameHooksLibraries) {
     // Check that no libraries are currently loaded
     vector<string> hooks_libraries = HooksManager::getLibraryNames();
     EXPECT_TRUE(hooks_libraries.empty());
 
     // Configuration with hooks-libraries set to two libraries.
-    const std::string config = createConfig(CALLOUT_LIBRARY_1,
-                                            CALLOUT_LIBRARY_2);
+    const std::string config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1,
+                                                       CALLOUT_LIBRARY_2);
 
     // Verify that the configuration string parses. The twoHooksLibraries
     // test shows that the list will be as expected.
@@ -1082,14 +1008,14 @@ TEST_P(HooksParseConfigTest, reconfigureSameHooksLibraries) {
 
 // Configure the hooks with two libraries, then reconfigure with the same
 // libraries, but in reverse order.
-TEST_P(HooksParseConfigTest, reconfigureReverseHooksLibraries) {
+TEST_F(ParseConfigTest, reconfigureReverseHooksLibraries) {
     // Check that no libraries are currently loaded
     vector<string> hooks_libraries = HooksManager::getLibraryNames();
     EXPECT_TRUE(hooks_libraries.empty());
 
     // Configuration with hooks-libraries set to two libraries.
-    std::string config = createConfig(CALLOUT_LIBRARY_1,
-                                      CALLOUT_LIBRARY_2);
+    std::string config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1,
+                                                 CALLOUT_LIBRARY_2);
 
     // Verify that the configuration string parses. The twoHooksLibraries
     // test shows that the list will be as expected.
@@ -1100,7 +1026,7 @@ TEST_P(HooksParseConfigTest, reconfigureReverseHooksLibraries) {
     // libraries and that they loaded correctly.
 
     // Parse the reversed set of libraries.
-    config = createConfig(CALLOUT_LIBRARY_2, CALLOUT_LIBRARY_1);
+    config = setHooksLibrariesConfig(CALLOUT_LIBRARY_2, CALLOUT_LIBRARY_1);
     rcode = parseConfiguration(config);
     ASSERT_TRUE(rcode == 0) << error_text_;
 
@@ -1122,14 +1048,14 @@ TEST_P(HooksParseConfigTest, reconfigureReverseHooksLibraries) {
 
 // Configure the hooks with two libraries, then reconfigure with
 // no libraries.
-TEST_P(HooksParseConfigTest, reconfigureZeroHooksLibraries) {
+TEST_F(ParseConfigTest, reconfigureZeroHooksLibraries) {
     // Check that no libraries are currently loaded
     vector<string> hooks_libraries = HooksManager::getLibraryNames();
     EXPECT_TRUE(hooks_libraries.empty());
 
     // Configuration with hooks-libraries set to two libraries.
-    std::string config = createConfig(CALLOUT_LIBRARY_1,
-                                      CALLOUT_LIBRARY_2);
+    std::string config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1,
+                                                 CALLOUT_LIBRARY_2);
 
     // Verify that the configuration string parses.
     int rcode = parseConfiguration(config);
@@ -1139,7 +1065,7 @@ TEST_P(HooksParseConfigTest, reconfigureZeroHooksLibraries) {
     // libraries and that they loaded correctly.
 
     // Parse the string again, this time without any libraries.
-    config = createConfig();
+    config = setHooksLibrariesConfig();
     rcode = parseConfiguration(config);
     ASSERT_TRUE(rcode == 0) << error_text_;
 
@@ -1156,16 +1082,16 @@ TEST_P(HooksParseConfigTest, reconfigureZeroHooksLibraries) {
 }
 
 // Check with a set of libraries, some of which are invalid.
-TEST_P(HooksParseConfigTest, invalidHooksLibraries) {
+TEST_F(ParseConfigTest, invalidHooksLibraries) {
     // Check that no libraries are currently loaded
     vector<string> hooks_libraries = HooksManager::getLibraryNames();
     EXPECT_TRUE(hooks_libraries.empty());
 
     // Configuration string.  This contains an invalid library which should
     // trigger an error in the "build" stage.
-    const std::string config = createConfig(CALLOUT_LIBRARY_1,
-                                            NOT_PRESENT_LIBRARY,
-                                            CALLOUT_LIBRARY_2);
+    const std::string config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1,
+                                                       NOT_PRESENT_LIBRARY,
+                                                       CALLOUT_LIBRARY_2);
 
     // Verify that the configuration fails to parse. (Syntactically it's OK,
     // but the library is invalid).
@@ -1193,13 +1119,13 @@ TEST_P(HooksParseConfigTest, invalidHooksLibraries) {
 }
 
 // Check that trying to reconfigure with an invalid set of libraries fails.
-TEST_P(HooksParseConfigTest, reconfigureInvalidHooksLibraries) {
+TEST_F(ParseConfigTest, reconfigureInvalidHooksLibraries) {
     // Check that no libraries are currently loaded
     vector<string> hooks_libraries = HooksManager::getLibraryNames();
     EXPECT_TRUE(hooks_libraries.empty());
 
     // Configure with a single library.
-    std::string config = createConfig(CALLOUT_LIBRARY_1);
+    std::string config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1);
     int rcode = parseConfiguration(config);
     ASSERT_TRUE(rcode == 0) << error_text_;
 
@@ -1208,8 +1134,8 @@ TEST_P(HooksParseConfigTest, reconfigureInvalidHooksLibraries) {
 
     // Configuration string.  This contains an invalid library which should
     // trigger an error in the "build" stage.
-    config = createConfig(CALLOUT_LIBRARY_1, NOT_PRESENT_LIBRARY,
-                          CALLOUT_LIBRARY_2);
+    config = setHooksLibrariesConfig(CALLOUT_LIBRARY_1, NOT_PRESENT_LIBRARY,
+                                     CALLOUT_LIBRARY_2);
 
     // Verify that the configuration fails to parse. (Syntactically it's OK,
     // but the library is invalid).
@@ -1239,29 +1165,28 @@ TEST_P(HooksParseConfigTest, reconfigureInvalidHooksLibraries) {
     EXPECT_EQ(CALLOUT_LIBRARY_1, hooks_libraries[0]);
 }
 
-// Check that if the syntax is not valid (mixture of old and new syntaxes),
-// the parse will fail.
-TEST_P(HooksParseConfigTest, invalidHooksLibrarySyntax) {
-    // Create string with multiple valid libraries.  This should fail to
-    // parse.
-    string config = createConfig(CALLOUT_LIBRARY_1, CALLOUT_LIBRARY_2,
-                                 CALLOUT_LIBRARY_1, CALLOUT_LIBRARY_2);
+// 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);
+
+    // The parse should fail...
     int rcode = parseConfiguration(config);
     ASSERT_NE(0, rcode);
 
-    // Check that it found that the error was down to the string not
-    // beiong a list of maps.
-    EXPECT_FALSE(error_text_.find("not a list of maps") == string::npos) <<
+    // ... 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) <<
         "Error text returned from parse failure is " << error_text_;
-    
 }
 
-
-INSTANTIATE_TEST_CASE_P(OldSyntax, HooksParseConfigTest,
-                         ::testing::Values(setHooksLibrariesConfig));
-INSTANTIATE_TEST_CASE_P(NewSyntax, HooksParseConfigTest,
-                         ::testing::Values(setHooksLibrariesNewConfig));
-
 /// @brief Checks that a valid, enabled D2 client configuration works correctly.
 TEST_F(ParseConfigTest, validD2Config) {