From: Tomek Mrugalski Date: Mon, 16 Feb 2015 16:30:15 +0000 (+0100) Subject: [3565] Changes after review: X-Git-Tag: trac3723_base~26^2~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b505c120fb21f2a03de603d5b0c111b79aaaaa04;p=thirdparty%2Fkea.git [3565] Changes after review: - Documentation for reservation-mode added - spec files updated - unit-tests for HR no longer use eth0 or eth1 - @todos added when allow_new_leases_in_renewals_ is expected to be used - HR unit-test no longer uses at() - Copyright years updated - getOptionalParam() used when handling reservation-mode - hrModeFromText() moved to SubnetConfigParser - hrModeFromText() starts with lowercase and has its documentation expanded --- diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index d93f945789..859b9742bc 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -1886,6 +1886,72 @@ temporarily override a list of interface names and listen on all interfaces. reservation. Such a feature will be added in the upcoming Kea releases. + +
+ Fine Tuning IPv4 Host Reservation + + + reservation-mode in DHCPv6 server in 0.9.1 beta is + accepted, but not used. Full implementation will be available in the upcoming + releases. + + + Host reservation capability introduces additional restrictions for the + allocation engine during lease selection and renewal. In particular, three + major checks are necessary. First, when selecting a new lease, it is not + sufficient for a candidate lease to be not used by anyone else. It also must + not be reserved for anyone else. Second, when renewing a lease, additional + check must be performed whether the address being renewed is not reserved + for anyone else. Finally, when a host renews an address, the server has to + check whether there's a reservation for this host, so the exisiting (dynamically + allocated) address should be revoked and the reserved one be used instead. + + Some of those checks in certain deployments may be not necessary. Skipping + them may improve performance. The Kea server allows configuration of the + reservation types allowed for each subnet using reservation-mode. + Allowed values are: + + + all - enables all host reservation + types. This is the default value. This setting is the safest and the most + flexible. It allows in-pool and out-of-pool reservations. As all checks + are conducted, it is also the slowest. + + + out-of-pool - allows only out of + pool host reservations. With this setting in place, the server may assume + that all host reservations are for addresses that do not belong to the + dynamic pool. Therefore it can skip the reservation checks when dealing + with in-pool addresses, thus improving performance. Do not use this mode + if any of your reservations use in-pool address. Caution is advised when + using this setting. Kea 0.9.1 does not sanity check the reservations against + reservation-mode. Misconfiguration may cause problems. + + + + disabled - host reservation support is disabled. As there + are no reservations, the server will skip all checks. Any reservations defined + will be completely ignored. As the checks are skipped, the server may + operate faster in this mode. + + + + + + + An example configuration that disables reservation looks like follows: + +"Dhcp4": { + "subnet4": [ + "subnet": "192.0.2.0/24", + "reservation-mode": "disabled", + ... + ] +} + + +
+ diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index 074f05f7cb..6f609ece96 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -1894,6 +1894,70 @@ should include options from the isc option space: releases. +
+ Fine Tuning IPv6 Host Reservation + + + reservation-mode in the DHCPv6 server in 0.9.1 beta is + implemented, but was not tested and is considered experimental. + + + Host reservation capability introduces additional restrictions for the + allocation engine during lease selection and renewal. In particular, three + major checks are necessary. First, when selecting a new lease, it is not + sufficient for a candidate lease to be not used by anyone else. It also must + not be reserved for anyone else. Second, when renewing a lease, additional + check must be performed whether the address being renewed is not reserved + for anyone else. Finally, when a host renews an address, the server has to + check whether there's a reservation for this host, so the exisiting (dynamically + allocated) address should be revoked and the reserved one be used instead. + + Some of those checks in certain deployments may be not necessary. Skipping + them may improve performance. The Kea server allows configuration of the + reservation types allowed for each subnet using reservation-mode. + Allowed values are: + + + all - enables all host reservation + types. This is the default value. This setting is the safest and the most + flexible. It allows in-pool and out-of-pool reservations. As all checks + are conducted, it is also the slowest. + + + out-of-pool - allows only out of + pool host reservations. With this setting in place, the server may assume + that all host reservations are for addresses that do not belong to the + dynamic pool. Therefore it can skip the reservation checks when dealing + with in-pool addresses, thus improving performance. Do not use this mode + if any of your reservations use in-pool address. Caution is advised when + using this setting. Kea 0.9.1 does not sanity check the reservations against + reservation-mode. Misconfiguration may cause problems. + + + + disabled - host reservation support is disabled. As there + are no reservations, the server will skip all checks. Any reservations defined + will be completely ignored. As the checks are skipped, the server may + operate faster in this mode. + + + + + + + An example configuration that disables reservation looks like follows: + +"Dhcp6": { + "subnet6": [ + "subnet": "2001:db8:1::/64", + "reservation-mode": "disabled", + ... + ] +} + + +
+ diff --git a/src/bin/dhcp4/dhcp4.spec b/src/bin/dhcp4/dhcp4.spec index 8c99576a87..0d3922b71b 100644 --- a/src/bin/dhcp4/dhcp4.spec +++ b/src/bin/dhcp4/dhcp4.spec @@ -367,7 +367,15 @@ "item_default": "0.0.0.0" } ] } - } ] + }, + { + "item_name": "reservation-mode", + "item_type": "string", + "item_optional": true, + "item_default": "all", + "item_description": "Specifies allowed host reservation types. Disabling unused modes may improve performance. Allowed values: disabled, off, out-of-pool, all" + } + ] } }, diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index f8abcf1221..96ad96551c 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2015 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 diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 14fc0cf869..85ecb917b0 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2015 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 @@ -3498,30 +3498,25 @@ TEST_F(Dhcp4ParserTest, hostReservationPerSubnet) { "\"subnet4\": [ { " " \"pools\": [ { \"pool\": \"192.0.2.0/24\" } ]," " \"subnet\": \"192.0.2.0/24\", " - " \"reservation-mode\": \"all\"," - " \"interface\": \"eth0\"" + " \"reservation-mode\": \"all\"" " }," " {" " \"pools\": [ { \"pool\": \"192.0.3.0/24\" } ]," " \"subnet\": \"192.0.3.0/24\", " - " \"reservation-mode\": \"out-of-pool\"," - " \"interface\": \"eth1\"" + " \"reservation-mode\": \"out-of-pool\"" " }," " {" " \"pools\": [ { \"pool\": \"192.0.4.0/24\" } ]," " \"subnet\": \"192.0.4.0/24\", " - " \"reservation-mode\": \"disabled\"," - " \"interface\": \"eth1\"" + " \"reservation-mode\": \"disabled\"" " }," " {" " \"pools\": [ { \"pool\": \"192.0.5.0/24\" } ]," - " \"subnet\": \"192.0.5.0/24\", " - " \"interface\": \"eth1\"" + " \"subnet\": \"192.0.5.0/24\"" " } ]," "\"valid-lifetime\": 4000 }"; ElementPtr json = Element::fromJSON(hr_config); - CfgMgr::instance().clear(); ConstElementPtr result; EXPECT_NO_THROW(result = configureDhcp4Server(*srv_, json)); diff --git a/src/bin/dhcp6/dhcp6.spec b/src/bin/dhcp6/dhcp6.spec index 573129f0a8..618d1720a9 100644 --- a/src/bin/dhcp6/dhcp6.spec +++ b/src/bin/dhcp6/dhcp6.spec @@ -425,6 +425,13 @@ } } ] } + }, + { + "item_name": "reservation-mode", + "item_type": "string", + "item_optional": true, + "item_default": "all", + "item_description": "Specifies allowed host reservation types. Disabling unused modes may improve performance. Allowed values: disabled, off, out-of-pool, all" } ] } }, diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 13ac19dd3e..c1460df327 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1587,10 +1587,15 @@ Dhcpv6Srv::extendIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, hints_count++; } - // If this is a Renew, it's ok to allocate new leases + /// @todo: This was clarfied in draft-ietf-dhc-dhcpv6-stateful-issues that + /// the server is allowed to assign new leases in both Renew and Rebind. For + /// now, we only support it in Renew, because it breaks a lot of Rebind + /// unit-tests. Ultimately, whether we allow it or not, should be exposed + /// as configurable policy. See ticket #3717. if (query->getType() == DHCPV6_RENEW) { ctx.allow_new_leases_in_renewals_ = true; } + Lease6Collection leases = alloc_engine_->renewLeases6(ctx); // Ok, now we have the leases extended. We have: @@ -1755,6 +1760,12 @@ Dhcpv6Srv::extendIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, hints_count++; } + /// @todo: The draft-ietf-dhc-dhcpv6-stateful-issues added a new capability + /// of the server to to assign new PD leases in both Renew and Rebind. + /// There's allow_new_leases_in_renewals_ in the ClientContext6, but we + /// currently not use it in PD yet. This should be implemented as part + /// of the stateful-issues implementation effort. See ticket #3718. + // Call Allocation Engine and attempt to renew leases. Number of things // may happen. Leases may be extended, revoked (if the lease is no longer // valid or reserved for someone else), or new leases may be added. diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index f7a57e6f8a..2f294de5bd 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2015 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 diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index 690de2f77b..18e78d4076 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2015 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 diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 6dea50fe75..c0016d9fcb 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -3710,25 +3710,21 @@ TEST_F(Dhcp6ParserTest, hostReservationPerSubnet) { "\"subnet6\": [ { " " \"pools\": [ { \"pool\": \"2001:db8:1::/64\" } ]," " \"subnet\": \"2001:db8:1::/48\", " - " \"reservation-mode\": \"all\"," - " \"interface\": \"eth0\"" + " \"reservation-mode\": \"all\"" " }," " {" " \"pools\": [ { \"pool\": \"2001:db8:2::/64\" } ]," " \"subnet\": \"2001:db8:2::/48\", " - " \"reservation-mode\": \"out-of-pool\"," - " \"interface\": \"eth1\"" + " \"reservation-mode\": \"out-of-pool\"" " }," " {" " \"pools\": [ { \"pool\": \"2001:db8:3::/64\" } ]," " \"subnet\": \"2001:db8:3::/48\", " - " \"reservation-mode\": \"disabled\"," - " \"interface\": \"eth1\"" + " \"reservation-mode\": \"disabled\"" " }," " {" " \"pools\": [ { \"pool\": \"2001:db8:4::/64\" } ]," - " \"subnet\": \"2001:db8:4::/48\", " - " \"interface\": \"eth1\"" + " \"subnet\": \"2001:db8:4::/48\" " " } ]," "\"valid-lifetime\": 4000 }"; @@ -3741,16 +3737,30 @@ TEST_F(Dhcp6ParserTest, hostReservationPerSubnet) { checkResult(status, 0); CfgMgr::instance().commit(); - const Subnet6Collection* subnets = - CfgMgr::instance().getCurrentCfg()->getCfgSubnets6()->getAll(); + ConstCfgSubnets6Ptr subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets6(); + + const Subnet6Collection* subnet_col = subnets->getAll(); ASSERT_TRUE(subnets); - ASSERT_EQ(4, subnets->size()); // We expect 4 subnets + ASSERT_EQ(4, subnet_col->size()); // We expect 4 subnets + + // Let's check if the parsed subnets have correct HR modes + Subnet6Ptr subnet; + subnet = subnets->selectSubnet(IOAddress("2001:db8:1::1")); + ASSERT_TRUE(subnet); + EXPECT_EQ(Subnet::HR_ALL, subnet->getHostReservationMode()); - // Check subnet-ids of each subnet (it should be monotonously increasing) - EXPECT_EQ(Subnet::HR_ALL, subnets->at(0)->getHostReservationMode()); - EXPECT_EQ(Subnet::HR_OUT_OF_POOL, subnets->at(1)->getHostReservationMode()); - EXPECT_EQ(Subnet::HR_DISABLED, subnets->at(2)->getHostReservationMode()); - EXPECT_EQ(Subnet::HR_ALL, subnets->at(3)->getHostReservationMode()); + + subnet = subnets->selectSubnet(IOAddress("2001:db8:2::1")); + ASSERT_TRUE(subnet); + EXPECT_EQ(Subnet::HR_OUT_OF_POOL, subnet->getHostReservationMode()); + + subnet = subnets->selectSubnet(IOAddress("2001:db8:3::1")); + ASSERT_TRUE(subnet); + EXPECT_EQ(Subnet::HR_DISABLED, subnet->getHostReservationMode()); + + subnet = subnets->selectSubnet(IOAddress("2001:db8:4::1")); + ASSERT_TRUE(subnet); + EXPECT_EQ(Subnet::HR_ALL, subnet->getHostReservationMode()); } }; diff --git a/src/bin/dhcp6/tests/fqdn_unittest.cc b/src/bin/dhcp6/tests/fqdn_unittest.cc index 60d69ede32..c87f7a7577 100644 --- a/src/bin/dhcp6/tests/fqdn_unittest.cc +++ b/src/bin/dhcp6/tests/fqdn_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2015 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 diff --git a/src/lib/dhcpsrv/lease.h b/src/lib/dhcpsrv/lease.h index 0c536fb5a2..4ad892b895 100644 --- a/src/lib/dhcpsrv/lease.h +++ b/src/lib/dhcpsrv/lease.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2015 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 diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc index 794e9cc0e8..46c8cfab1c 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2015 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 @@ -1125,7 +1125,7 @@ SubnetConfigParser::build(ConstElementPtr subnet) { } Subnet::HRMode -HRModeFromText(const std::string& txt) { +SubnetConfigParser::hrModeFromText(const std::string& txt) { if ( (txt.compare("disabled") == 0) || (txt.compare("off") == 0) ) { return (Subnet::HR_DISABLED); @@ -1192,18 +1192,13 @@ SubnetConfigParser::createSubnet() { // iface not mandatory so swallow the exception } - std::string hr_mode; - try { - hr_mode = string_values_->getParam("reservation-mode"); - } catch (const DhcpConfigError &) { - // Host reservation mode is not mandatory, so don't complain - } + // Let's set host reservation mode. If not specified, the default value of + // all will be used. + std::string hr_mode; try { - // This may throw if the user didn't specify one of allowed values - if (!hr_mode.empty()) { - subnet_->setHostReservationMode(HRModeFromText(hr_mode)); - } + hr_mode = string_values_->getOptionalParam("reservation-mode", "all"); + subnet_->setHostReservationMode(hrModeFromText(hr_mode)); } catch (const BadValue& ex) { isc_throw(DhcpConfigError, "Failed to process specified value " " of reservation-mode parameter: " << ex.what() diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.h b/src/lib/dhcpsrv/parsers/dhcp_parsers.h index 46e1f7f865..0fb3c351f0 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.h +++ b/src/lib/dhcpsrv/parsers/dhcp_parsers.h @@ -1093,6 +1093,17 @@ protected: /// unspecified value. isc::dhcp::Triplet getOptionalParam(const std::string& name); + /// @brief Attempts to convert text representation to HRMode enum. + /// + /// Allowed values are "disabled", "off" (alias for disabled), + /// "out-of-pool" and "all". See Subnet::HRMode for their exact meaning. + /// + /// @throw BadValue if the text cannot be converted. + /// + /// @param text representation for conversion + /// @return one of allowed HRMode values + static Subnet::HRMode hrModeFromText(const std::string& txt); + private: /// @brief Create a new subnet using a data from child parsers. diff --git a/src/lib/dhcpsrv/subnet.h b/src/lib/dhcpsrv/subnet.h index ab63d903dd..e0f943128e 100644 --- a/src/lib/dhcpsrv/subnet.h +++ b/src/lib/dhcpsrv/subnet.h @@ -326,7 +326,7 @@ public: /// /// @return whether in-pool host reservations are allowed. HRMode - getHostReservationMode() { + getHostReservationMode() const { return (host_reservation_mode_); } @@ -339,14 +339,6 @@ public: host_reservation_mode_ = mode; } - /// @brief Attempts to convert text representation to HRMode enum - /// - /// @throw BadValue if the text cannot be converted - /// - /// @param text representation for conversion - /// @return one of allowed HRMode values - static HRMode HRModeFromText(const std::string& txt); - protected: /// @brief Returns all pools (non-const variant) /// diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h index d892477920..951990a642 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h +++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.h @@ -311,7 +311,7 @@ public: } DuidPtr duid_; ///< client-identifier (value used in tests) - HWAddrPtr hwaddr_; ///< client's hardware address + HWAddrPtr hwaddr_; ///< client's hardware address uint32_t iaid_; ///< IA identifier (value used in tests) Subnet6Ptr subnet_; ///< subnet6 (used in tests) Pool6Ptr pool_; ///< NA pool belonging to subnet_ diff --git a/src/lib/dhcpsrv/tests/host_unittest.cc b/src/lib/dhcpsrv/tests/host_unittest.cc index 0102785032..f89d044515 100644 --- a/src/lib/dhcpsrv/tests/host_unittest.cc +++ b/src/lib/dhcpsrv/tests/host_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2015 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 diff --git a/src/lib/dhcpsrv/tests/test_utils.h b/src/lib/dhcpsrv/tests/test_utils.h index 49f97f9ccf..dea983058c 100644 --- a/src/lib/dhcpsrv/tests/test_utils.h +++ b/src/lib/dhcpsrv/tests/test_utils.h @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2015 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