From: Tomek Mrugalski Date: Mon, 12 Dec 2016 19:28:27 +0000 (+0100) Subject: [5039] Parameters inheritance implemented, doc updated X-Git-Tag: trac5021_base~3^2~11 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0bbc1df7d05e0434fa3918ac514793966f322a23;p=thirdparty%2Fkea.git [5039] Parameters inheritance implemented, doc updated --- diff --git a/src/lib/dhcpsrv/Makefile.am b/src/lib/dhcpsrv/Makefile.am index 11bbdf6294..289b9e5f71 100644 --- a/src/lib/dhcpsrv/Makefile.am +++ b/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 diff --git a/src/lib/dhcpsrv/libdhcpsrv.dox b/src/lib/dhcpsrv/libdhcpsrv.dox index 71b5c6cc39..1c6a394618 100644 --- a/src/lib/dhcpsrv/libdhcpsrv.dox +++ b/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 +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. There will be more specific +methods implemented in the future, but for the time being only +@ref isc::dhcp::SimpleParser::inheritGlobalToSubnet is implemented. + */ diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.h b/src/lib/dhcpsrv/parsers/dhcp_parsers.h index f5cbea23b0..df829dc1db 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.h +++ b/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 /// diff --git a/src/lib/dhcpsrv/parsers/simple_parser.cc b/src/lib/dhcpsrv/parsers/simple_parser.cc index 5a88fca9cb..e40ba28f45 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser.cc +++ b/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 diff --git a/src/lib/dhcpsrv/parsers/simple_parser.h b/src/lib/dhcpsrv/parsers/simple_parser.h index 14fc804d52..16fe5c6e3e 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser.h +++ b/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() {} diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 77e03f4f94..b9d5a76322 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/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