Browse Source

[5039] Parameters inheritance implemented, doc updated

Tomek Mrugalski 8 years ago
parent
commit
0bbc1df7d0

+ 2 - 1
src/lib/dhcpsrv/Makefile.am

@@ -20,7 +20,8 @@ if HAVE_CQL
 AM_CPPFLAGS += $(CQL_CPPFLAGS)
 endif
 
-AM_CXXFLAGS = $(KEA_CXXFLAGS)
+# This is a temporary workaround until we have Kea-wide support for C++11.
+AM_CXXFLAGS = $(KEA_CXXFLAGS) -std=c++11
 
 # The files in the subfolder must be explicitly specified here so
 # as they are copied to the distribution. The other option would

+ 88 - 0
src/lib/dhcpsrv/libdhcpsrv.dox

@@ -476,4 +476,92 @@ is used by DHCPv4 and DHCPv6 components.
 
 DHCPv4-over-DHCPv6 which are relayed by a DHCPv6 relay are not yet supported.
 
+@section dhcp6SimpleParser Simple JSON Parser
+
+Since the early beginnings, our configuration parsing code was a mess. It
+started back in 2011 when Tomek joined ISC recently and was told to implement
+Kea configuration handling in similar way as DNS Auth module. The code grew
+over time (DHCP configuration is significantly more complex than DNS, with
+more interdependent values) and as of Kea 1.1 release it became very difficult
+to manage. The decision has been made to significantly refactor or even
+partially rewrite the parser code. The design for this effort is documented
+here: http://kea.isc.org/wiki/SimpleParser It discusses the original issues
+and the proposed architecture.
+
+There are several aspects of this new approach. The base class for all parsers
+is @ref isc::dhcp::SimpleParser. It simplifies the parser be rejecting the
+concept of build/commit phases. Instead, there should be a single method
+called parse that takes ConstElementPtr as a single parameter (that's the
+JSON structures to be parsed) and returns the config structure to be used
+in CfgMgr. An example of such a method can be the following:
+
+@code
+std::pair<OptionDescriptor, std::string>
+OptionDataParser::parse(isc::data::ConstElementPtr single_option)
+@endcode
+
+Since each derived class will have the same parameter, but a different return
+type, it's not possible to use virtual methods mechanism. That's perfectly
+ok, though, as there is only a single instance of the class needed to parse
+arbitrary number of parameters of the same type, so there is no need to
+keep pointers to the parser object. As such there's fewer incentives to have
+one generic way to handle all parsers.
+
+@subsection dhcp6SimpleParserDefaults Default values in Simple Parser
+
+Another simplification comes from the fact that almost all parameters
+are mandatory in SimpleParser. One source of complexities in the old
+parser was the necessity to deal with optional parameters. Simple
+parser deals with that by explicitly requiring the input structure to
+have all parameters filled. Obviously, it's not feasible to expect
+everyone to always specify all parameters, therefore there's an easy
+way to fill missing parameters with their default values. There are
+several methods to do this, but the most generic one is:
+
+@code
+static size_t
+isc::dhcp::SimpleParser::setDefaults(isc::data::ElementPtr scope,
+                                     const SimpleDefaults& default_values);
+@endcode
+
+It takes a pointer to element to be filled with default values and
+vector of default values. Having those values specified in a single
+place in a way that can easily be read even by non-programmers is a
+big advantage of this approach. Here's an example from simple_parser.cc file:
+
+@code
+/// This table defines default values for option definitions in DHCPv6
+const SimpleDefaults OPTION6_DEF_DEFAULTS = {
+    { "record-types", Element::string,  ""},
+    { "space",        Element::string,  "dhcp6"},
+    { "array",        Element::boolean, "false"},
+    { "encapsulate",  Element::string,  "" }
+};
+@endcode
+
+This array (which technically is implemented as a vector and
+initialized the C++11 way) can be passed to the aforementioned
+setDefaults. That code will iterate over all default values and see if
+there are explicit values provided. If not, the gaps will be filled
+with default values. There are also convenience methods specified for
+filling in option data defaults, option definition defaults and
+setAllDefaults that sets all defaults (starts with global, but then
+walks down the Element tree and fills defaults in subsequent scopes).
+
+@subsection dhcp6SimpleParserInherits Inheriting parameters between scopes
+
+SimpleParser provides a mechanism to inherit parameters between scopes,
+e.g. to inherit global parameters in the subnet scope if more specific
+values are not defined in the subnet scope. This is achieved by calling
+@code
+static size_t SimpleParser::deriveParams(isc::data::ConstElementPtr parent,
+                                         isc::data::ElementPtr child,
+                                         const ParamsList& params);
+
+@endcode
+
+ParamsList is a simple vector<string>. There will be more specific
+methods implemented in the future, but for the time being only
+@ref isc::dhcp::SimpleParser::inheritGlobalToSubnet is implemented.
+
 */

+ 4 - 6
src/lib/dhcpsrv/parsers/dhcp_parsers.h

@@ -710,13 +710,11 @@ class OptionDefListParser : public SimpleParser {
 public:
     /// @brief Constructor.
     ///
-    /// @param dummy first argument is ignored, all Parser constructors
-    /// accept string as first argument.
-    /// @param global_context is a pointer to the global context which
-    /// stores global scope parameters, options, option defintions.
+    /// Stores address family that will be used to make certain decisions
+    /// during parsing.
+    ///
+    /// @param address_family @c AF_INET or @c AF_INET6
     OptionDefListParser(const uint16_t address_family);
-                        //const std::string& dummy,
-                        //ParserContextPtr global_context);
 
     /// @brief Parses a list of option defintions, create them and store in cfg
     ///

+ 27 - 4
src/lib/dhcpsrv/parsers/simple_parser.cc

@@ -40,14 +40,30 @@ const SimpleDefaults OPTION6_DEFAULTS = {
     { "encapsulate",  Element::string,  "" }
 };
 
+/// This table defines default values for DHCPv4
+const SimpleDefaults GLOBAL4_DEFAULTS = {
+    { "renew-timer",        Element::integer, "900" },
+    { "rebind-timer",       Element::integer, "1800" },
+    { "valid-lifetime",     Element::integer, "7200" }
+};
+
 /// This table defines default values for both DHCPv4 and DHCPv6
-const SimpleDefaults GLOBAL_DEFAULTS = {
+const SimpleDefaults GLOBAL6_DEFAULTS = {
     { "renew-timer",        Element::integer, "900" },
     { "rebind-timer",       Element::integer, "1800" },
     { "preferred-lifetime", Element::integer, "3600" },
     { "valid-lifetime",     Element::integer, "7200" }
 };
 
+/// This list defines parameters that can be inherited from the global
+/// scope to subnet scope.
+const ParamsList INHERIT_GLOBAL_TO_SUBNET = {
+    "renew-timer",
+    "rebind-timer",
+    "preferred-lifetime",
+    "valid-lifetime"
+};
+
 std::string
 SimpleParser::getString(isc::data::ConstElementPtr scope, const std::string& name) {
     ConstElementPtr x = scope->get(name);
@@ -162,8 +178,8 @@ size_t SimpleParser::setDefaults(isc::data::ElementPtr scope,
     return (cnt);
 }
 
-size_t SimpleParser::setGlobalDefaults(isc::data::ElementPtr global) {
-    return (setDefaults(global, GLOBAL_DEFAULTS));
+size_t SimpleParser::setGlobalDefaults(isc::data::ElementPtr global, bool v6) {
+    return (setDefaults(global, v6 ? GLOBAL6_DEFAULTS : GLOBAL4_DEFAULTS));
 }
 
 size_t SimpleParser::setOptionDefaults(isc::data::ElementPtr option, bool v6) {
@@ -196,7 +212,8 @@ size_t SimpleParser::setAllDefaults(isc::data::ElementPtr global, bool v6) {
     size_t cnt = 0;
 
     // Set global defaults first.
-    //cnt = setGlobalDefaults(global);
+    /// @todo: Uncomment as part of the ticket 5019 work.
+    //cnt = setGlobalDefaults(global, v6);
 
     // Now set option defintion defaults for each specified option definition
     ConstElementPtr option_defs = global->get("option-def");
@@ -249,5 +266,11 @@ SimpleParser::deriveParams(isc::data::ConstElementPtr parent,
     return (cnt);
 }
 
+size_t
+SimpleParser::inheritGlobalToSubnet(isc::data::ConstElementPtr global,
+                                    isc::data::ElementPtr subnet) {
+    return deriveParams(global, subnet, INHERIT_GLOBAL_TO_SUBNET);
+}
+
 }; // end of isc::dhcp namespace
 }; // end of isc namespace

+ 15 - 2
src/lib/dhcpsrv/parsers/simple_parser.h

@@ -62,6 +62,17 @@ class SimpleParser {
     static size_t deriveParams(isc::data::ConstElementPtr parent,
                                isc::data::ElementPtr child,
                                const ParamsList& params);
+    /// @brief Derives (inherits) parameters from global to subnet scope
+    ///
+    /// This method derives global parameters into subnet scope if they're
+    /// not defined there yet.
+    ///
+    /// @param global parameters will be inherited from here
+    /// @param subnet parameters will be inserted here
+    /// @return number of parameters copied
+    static size_t
+    inheritGlobalToSubnet(isc::data::ConstElementPtr global,
+                          isc::data::ElementPtr subnet);
 
     /// @brief Sets the default values
     ///
@@ -84,8 +95,9 @@ class SimpleParser {
     /// for that.
     ///
     /// @param global scope to be filled in with defaults.
+    /// @param v6 is it v6 (true) or v4 (false) option?
     /// @return number of default values added
-    static size_t setGlobalDefaults(isc::data::ElementPtr global);
+    static size_t setGlobalDefaults(isc::data::ElementPtr global, bool v6);
 
     /// @brief Sets option defaults for a single option
     ///
@@ -131,6 +143,7 @@ class SimpleParser {
     /// set the global defaults only, see @ref setGlobalDefaults for that.
     ///
     /// @param global scope to be filled in with defaults.
+    /// @param v6 true = v6, false = v4
     /// @return number of default values added
     static size_t setAllDefaults(isc::data::ElementPtr global, bool v6);
 
@@ -145,7 +158,7 @@ class SimpleParser {
     getPosition(const std::string& name, const data::ConstElementPtr parent =
                 data::ConstElementPtr()) const;
 
-    /// @destructor
+    /// Destructor
     ///
     /// It's really simple, isn't it?
     virtual ~SimpleParser() {}

+ 93 - 0
src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -318,6 +318,16 @@ public:
                      (config_pair.first == "option-def")) {
                     continue;
                 }
+
+                // We also don't care about the default values that may be been
+                // inserted
+                if ( (config_pair.first == "preferred-lifetime") ||
+                     (config_pair.first == "valid-lifetime") ||
+                     (config_pair.first == "renew-timer") ||
+                     (config_pair.first == "rebind-timer")) {
+                    continue;
+                }
+
                 // Remaining ones are old style parsers. Need go do
                 // the build/commit dance with them.
 
@@ -477,6 +487,28 @@ public:
         CfgMgr::instance().setD2ClientConfig(tmp);
     }
 
+    /// @brief Checks if specified map has an integer parameter with expected value
+    ///
+    /// @param map map to be checked
+    /// @param param_name name of the parameter to be checked
+    /// @param exp_value expected value of the parameter.
+    void checkIntegerValue(const ConstElementPtr& map, const std::string& param_name,
+                           int64_t exp_value) {
+
+        // First check if the passed element is a map.
+        ASSERT_EQ(Element::map, map->getType());
+
+        // Now try to get the element being checked
+        ConstElementPtr elem = map->get(param_name);
+        ASSERT_TRUE(elem);
+
+        // Now check if it's indeed integer
+        ASSERT_EQ(Element::integer, elem->getType());
+
+        // Finally, check if its value meets expectation.
+        EXPECT_EQ(exp_value, elem->intValue());
+    }
+
     /// @brief Parsers used in the parsing of the configuration
     ///
     /// Allows the tests to interrogate the state of the parsers (if required).
@@ -2393,4 +2425,65 @@ TEST_F(ParseConfigTest, validRelayInfo6) {
 // (see CtrlDhcpv4SrvTest.commandSocketBasic in
 // src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc).
 
+// This test checks if global defaults are properly set for DHCPv6.
+TEST_F(ParseConfigTest, globalDefaults6) {
+
+    ElementPtr empty = Element::fromJSON("{ }");
+    size_t num;
+
+    EXPECT_NO_THROW(num = SimpleParser::setGlobalDefaults(empty, true));
+
+    // We expect at least 4 parameters to be inserted.
+    EXPECT_TRUE(num >= 4);
+
+    checkIntegerValue(empty, "valid-lifetime", 7200);
+    checkIntegerValue(empty, "preferred-lifetime", 3600);
+    checkIntegerValue(empty, "rebind-timer", 1800);
+    checkIntegerValue(empty, "renew-timer", 900);
+}
+
+// This test checks if global defaults are properly set for DHCPv4.
+TEST_F(ParseConfigTest, DISABLED_globalDefaults4) {
+
+    ElementPtr empty = Element::fromJSON("{ }");
+    size_t num;
+
+    EXPECT_NO_THROW(num = SimpleParser::setGlobalDefaults(empty, false));
+
+    // We expect at least 3 parameters to be inserted.
+    EXPECT_TRUE(num >= 3);
+
+    checkIntegerValue(empty, "valid-lifetime", 7200);
+    checkIntegerValue(empty, "rebind-timer", 1800);
+    checkIntegerValue(empty, "renew-timer", 900);
+
+    // Make sure that preferred-lifetime is not set for v4 (it's v6 only
+    // parameter)
+    EXPECT_FALSE(empty->get("preferred-lifetime"));
+}
+
+// This test checks if the parameters can be inherited from the global
+// scope to the subnet scope.
+TEST_F(ParseConfigTest, inheritGlobalToSubnet) {
+    ElementPtr global = Element::fromJSON("{ \"renew-timer\": 1,"
+                                          "  \"rebind-timer\": 2,"
+                                          "  \"preferred-lifetime\": 3,"
+                                          "  \"valid-lifetime\": 4"
+                                          "}");
+    ElementPtr subnet = Element::fromJSON("{ \"renew-timer\": 100 }");
+
+    // we should inherit 3 parameters. Renew-timer should remain intact,
+    // as it was already defined in the subnet scope.
+    size_t num;
+    EXPECT_NO_THROW(num = SimpleParser::inheritGlobalToSubnet(global, subnet));
+    EXPECT_EQ(3, num);
+
+    // Check the values. 3 of them are interited, while the fourth one
+    // was already defined in the subnet, so should not be inherited.
+    checkIntegerValue(subnet, "renew-timer", 100);
+    checkIntegerValue(subnet, "rebind-timer", 2);
+    checkIntegerValue(subnet, "preferred-lifetime", 3);
+    checkIntegerValue(subnet, "valid-lifetime", 4);
+}
+
 };  // Anonymous namespace