Commit dec8d15f authored by Thomas Markwalder's avatar Thomas Markwalder

[5596] renew-timer and rebind-timer now optional for DHCPv4

v4 Parsing now allows renew/rebind timers to be unspecified,
and kea-dhcp4 logic now matchs ISC DHCP:

1. Send T2 only if it is specified and is less than lease lifetime
2. Send T1 only if it is specified and is less than either T2
if specified, or lease lifetime in the absence of T2

doc/guide/dhcp4-srv.xml
    Updated discussion of rebind/renew-timers

src/bin/dhcp4/dhcp4_srv.cc
    Dhcpv4Srv::assignLease() - modified to sanity check
    T1 and T2

src/bin/dhcp4/tests/config_parser_unittest.cc
src/bin/dhcp4/tests/get_config_unittest.cc
src/bin/dhcp4/tests/simple_parser4_unittest.cc
src/bin/dhcp6/tests/get_config_unittest.cc
src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc
src/lib/dhcpsrv/tests/cfg_shared_networks6_unittest.cc
    updated numerous tests

src/lib/dhcpsrv/network.cc
    Network::toElement() - only outputs T1, T2, and
    valid lifetime if they are specified

src/lib/dhcpsrv/network.h
    Network() - inits t1_, t2_, and valid_ to unspecified
    Triplet value

src/lib/dhcpsrv/parsers/dhcp_parsers.cc
    Subnet4ConfigParser::initSubnet() - allow renew-timer and
    rebind-timer to be optional

src/lib/dhcpsrv/parsers/simple_parser4.cc
    SimpleDefaults SimpleParser4::GLOBAL4_DEFAULTS - removed
    entries for renew-timer and rebind-timer
parent 505ce96e
......@@ -219,16 +219,20 @@ file.</para>
<para>Moving onto the DHCPv4 configuration elements, the first few elements
define some global parameters. <command>valid-lifetime</command>
defines for how long the addresses (leases) given out by the
defines how long the addresses (leases) given out by the
server are valid. If nothing changes, a client that got an address is allowed to
use it for 4000 seconds. (Note that integer numbers are specified as is,
without any quotes around them.) <command>renew-timer</command> and
<command>rebind-timer</command> are values (also in seconds) that
define T1 and T2 timers that govern when the client will begin the renewal and
rebind procedures. Note that <command>renew-timer</command> and
<command>rebind-timer</command> are optional. If they are not specified the
client will select values for T1 and T2 timers according to the
<link xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="http://tools.ietf.org/html/rfc2131">RFC 2131</link>.</para>
rebind procedures. <note> Both <command>renew-timer</command> and
<command>rebind-timer</command> are optional. The server will only send
rebind-timer to the client, via DHPv4 option code 59, if it is less than
valid-lifetime; and it will only send renew-timer, via DHCPv4 option code 58,
if it is less than rebind-timer (or valid-lifetime if rebind-timer was not
specified). In their absence, the client should select values for T1 and T2
timers according to the <link xmlns:xlink="http://www.w3.org/1999/xlink"
xlink:href="http://tools.ietf.org/html/rfc2131">RFC 2131</link>.</note></para>
<para>The <command>interfaces-config</command> map specifies the server
configuration concerning the network interfaces, on which the server should
......@@ -4322,15 +4326,15 @@ autogenerated IDs are not stable across configuration changes.</para>
<para>If "relay" is specified, the "ip-addresses" parameter within
it is mandatory.</para>
<note>
<note>
<para>
As of Kea 1.4, the "ip-address" parameter has been deprecated in favor
of "ip-addresses" which supports specifying a list of addresses.
As of Kea 1.4, the "ip-address" parameter has been deprecated in favor
of "ip-addresses" which supports specifying a list of addresses.
Configuration parsing, will honor the singular form for now but users are
encouraged to migrate.
</para>
</note>
</note>
</section>
......
......@@ -2137,21 +2137,32 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) {
// Subnet mask (type 1)
resp->addOption(getNetmaskOption(subnet));
// renewal-timer (type 58)
if (!subnet->getT1().unspecified()) {
// rebind timer (type 59) - if specified then send it only it if
// it is lease than lease life time. Note we "sanity" check T1
// and T2 against lease lifetime here in event the lifetime has
// been altered somewhere along the line.
uint32_t timer_ceiling = lease->valid_lft_;
if ((!subnet->getT2().unspecified()) &&
(subnet->getT2() < timer_ceiling)) {
OptionUint32Ptr t2(new OptionUint32(Option::V4,
DHO_DHCP_REBINDING_TIME,
subnet->getT2()));
resp->addOption(t2);
// If T2 is specified, then it becomes the ceiling for T1
timer_ceiling = subnet->getT2();
}
// renewal-timer (type 58) - if specified then send it only if
// it is less than the ceiling (T2 if given, lease life time if not)
if ((!subnet->getT1().unspecified()) &&
(subnet->getT1() < timer_ceiling)) {
OptionUint32Ptr t1(new OptionUint32(Option::V4,
DHO_DHCP_RENEWAL_TIME,
subnet->getT1()));
resp->addOption(t1);
}
// rebind timer (type 59)
if (!subnet->getT2().unspecified()) {
OptionUint32Ptr t2(new OptionUint32(Option::V4,
DHO_DHCP_REBINDING_TIME,
subnet->getT2()));
resp->addOption(t2);
}
// Create NameChangeRequests if DDNS is enabled and this is a
// real allocation.
......
......@@ -826,15 +826,14 @@ TEST_F(Dhcp4ParserTest, unspecifiedRenewTimer) {
Subnet4Ptr subnet = CfgMgr::instance().getStagingCfg()->
getCfgSubnets4()->selectSubnet(IOAddress("192.0.2.200"));
ASSERT_TRUE(subnet);
EXPECT_FALSE(subnet->getT1().unspecified());
EXPECT_TRUE(subnet->getT1().unspecified());
EXPECT_FALSE(subnet->getT2().unspecified());
EXPECT_EQ(900, subnet->getT1()); // that's the default value
EXPECT_EQ(2000, subnet->getT2());
EXPECT_EQ(4000, subnet->getValid());
// Check that subnet-id is 1
EXPECT_EQ(1, subnet->getID());
}
/// Check that the rebind-timer doesn't have to be specified, in which case
......@@ -863,8 +862,7 @@ TEST_F(Dhcp4ParserTest, unspecifiedRebindTimer) {
ASSERT_TRUE(subnet);
EXPECT_FALSE(subnet->getT1().unspecified());
EXPECT_EQ(1000, subnet->getT1());
EXPECT_FALSE(subnet->getT2().unspecified());
EXPECT_EQ(1800, subnet->getT2()); // that's the default value
EXPECT_TRUE(subnet->getT2().unspecified());
EXPECT_EQ(4000, subnet->getValid());
// Check that subnet-id is 1
......
......@@ -760,25 +760,24 @@ TEST_F(Dhcpv4SrvTest, DiscoverBasic) {
checkClientId(offer, clientid);
}
// Check that option 58 and 59 are not included if they are not specified.
TEST_F(Dhcpv4SrvTest, DiscoverNoTimers) {
// Check that option 58 and 59 are only included if they were specified
// and T2 is less than valid lft; T1 is less than T2 (if given) or valid
// lft if T2 is not given.
TEST_F(Dhcpv4SrvTest, DiscoverTimers) {
IfaceMgrTestConfig test_config(true);
IfaceMgr::instance().openSockets4();
boost::scoped_ptr<NakedDhcpv4Srv> srv;
ASSERT_NO_THROW(srv.reset(new NakedDhcpv4Srv(0)));
Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234));
dis->setRemoteAddr(IOAddress("192.0.2.1"));
OptionPtr clientid = generateClientId();
dis->addOption(clientid);
dis->setIface("eth1");
// Recreate a subnet but set T1 and T2 to "unspecified".
// Recreate subnet
Triplet<uint32_t> unspecified;
Triplet<uint32_t> valid_lft(1000);
subnet_.reset(new Subnet4(IOAddress("192.0.2.0"), 24,
Triplet<uint32_t>(),
Triplet<uint32_t>(),
3000));
unspecified,
unspecified,
valid_lft));
pool_ = Pool4Ptr(new Pool4(IOAddress("192.0.2.100"),
IOAddress("192.0.2.110")));
subnet_->addPool(pool_);
......@@ -786,20 +785,131 @@ TEST_F(Dhcpv4SrvTest, DiscoverNoTimers) {
CfgMgr::instance().getStagingCfg()->getCfgSubnets4()->add(subnet_);
CfgMgr::instance().commit();
// Pass it to the server and get an offer
Pkt4Ptr offer = srv->processDiscover(dis);
// Struct for describing an individual timer test scenario
struct TimerTest {
// logged test description
std::string description_;
// configured value for subnet's T1
Triplet<uint32_t> cfg_t1_;
// configured value for subnet's T1
Triplet<uint32_t> cfg_t2_;
// True if Offer should contain Subnet's T1 value
bool exp_t1_;
// True if Offer should contain Subnet's T2 value
bool exp_t2_;
};
// Check if we get response at all
checkResponse(offer, DHCPOFFER, 1234);
// Convenience constants
bool T1 = true;
bool T2 = true;
// Test scenarios
std::vector<TimerTest> tests = {
{
"T1:unspecified, T2:unspecified",
unspecified, unspecified,
// Client should neither.
!T1, !T2
},
{
"T1 unspecified, T2 < VALID",
unspecified, valid_lft - 1,
// Client should only get T2.
!T1, T2
},
{
"T1:unspecified, T2 = VALID",
unspecified, valid_lft,
// Client should get neither.
!T1, !T2
},
{
"T1:unspecified, T2 > VALID",
unspecified, valid_lft + 1,
// Client should get neither.
!T1, !T2
},
{
"T1 < VALID, T2:unspecified",
valid_lft - 1, unspecified,
// Client should only get T1.
T1, !T2
},
{
"T1 = VALID, T2:unspecified",
valid_lft, unspecified,
// Client should get neither.
!T1, !T2
},
{
"T1 > VALID, T2:unspecified",
valid_lft + 1, unspecified,
// Client should get neither.
!T1, !T2
},
{
"T1 < T2 < VALID",
valid_lft - 2, valid_lft - 1,
// Client should get both.
T1, T2
},
{
"T1 = T2 < VALID",
valid_lft - 1, valid_lft - 1,
// Client should only get T2.
!T1, T2
},
{
"T1 > T2 < VALID",
valid_lft - 1, valid_lft - 2,
// Client should only get T2.
!T1, T2
},
{
"T1 = T2 = VALID",
valid_lft, valid_lft,
// Client should get neither.
!T1, !T2
},
{
"T1 > VALID < T2, T2 > VALID",
valid_lft + 1, valid_lft + 2,
// Client should get neither.
!T1, !T2
}
};
// T1 and T2 timers must not be present.
checkAddressParams(offer, subnet_, false, false);
// Create a discover packet to use
Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234));
dis->setRemoteAddr(IOAddress("192.0.2.1"));
OptionPtr clientid = generateClientId();
dis->addOption(clientid);
dis->setIface("eth1");
// Iterate over the test scenarios.
for (auto test = tests.begin(); test != tests.end(); ++test) {
{
SCOPED_TRACE((*test).description_);
// Configure sunbet's timer values
subnet_->setT1((*test).cfg_t1_);
subnet_->setT2((*test).cfg_t2_);
// Discover/Offer exchange with the server
Pkt4Ptr offer = srv->processDiscover(dis);
// Verify we have an offer
checkResponse(offer, DHCPOFFER, 1234);
// Verify the timers are as expected.
checkAddressParams(offer, subnet_,
(*test).exp_t1_, (*test).exp_t2_);
}
}
// Check identifiers
checkServerId(offer, srv->getServerID());
checkClientId(offer, clientid);
}
// This test verifies that incoming DISCOVER can be handled properly, that an
// OFFER is generated, that the response has an address and that address
// really belongs to the configured pool.
......@@ -4298,7 +4408,6 @@ TEST_F(Dhcpv4SrvTest, truncatedVIVSOOption) {
ASSERT_TRUE(offer);
}
/// @todo: Implement proper tests for MySQL lease/host database,
/// see ticket #4214.
......
......@@ -1960,7 +1960,6 @@ const char* UNPARSED_CONFIGS[] = {
" \"relay\": {\n"
" \"ip-addresses\": [ ]\n"
" },\n"
" \"renew-timer\": 900,\n"
" \"reservation-mode\": \"all\",\n"
" \"reservations\": [ ],\n"
" \"server-hostname\": \"\",\n"
......@@ -2026,7 +2025,6 @@ const char* UNPARSED_CONFIGS[] = {
" \"pool\": \"192.0.2.1-192.0.2.100\"\n"
" }\n"
" ],\n"
" \"rebind-timer\": 1800,\n"
" \"relay\": {\n"
" \"ip-addresses\": [ ]\n"
" },\n"
......@@ -6965,11 +6963,9 @@ const char* UNPARSED_CONFIGS[] = {
" \"match-client-id\": true,\n"
" \"name\": \"foo\",\n"
" \"option-data\": [ ],\n"
" \"rebind-timer\": 0,\n"
" \"relay\": {\n"
" \"ip-addresses\": [ ]\n"
" },\n"
" \"renew-timer\": 0,\n"
" \"reservation-mode\": \"all\",\n"
" \"subnet4\": [\n"
" {\n"
......@@ -6989,11 +6985,9 @@ const char* UNPARSED_CONFIGS[] = {
" \"pool\": \"192.0.1.1-192.0.1.10\"\n"
" }\n"
" ],\n"
" \"rebind-timer\": 1800,\n"
" \"relay\": {\n"
" \"ip-addresses\": [ ]\n"
" },\n"
" \"renew-timer\": 900,\n"
" \"reservation-mode\": \"all\",\n"
" \"reservations\": [\n"
" {\n"
......@@ -7021,8 +7015,7 @@ const char* UNPARSED_CONFIGS[] = {
" \"subnet\": \"192.0.1.0/24\",\n"
" \"valid-lifetime\": 7200\n"
" }\n"
" ],\n"
" \"valid-lifetime\": 0\n"
" ]\n"
" }\n"
" ],\n"
" \"subnet4\": [ ]\n"
......
// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2016-2018 Internet Systems Consortium, Inc. ("ISC")
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
......@@ -95,12 +95,14 @@ TEST_F(SimpleParser4Test, globalDefaults4) {
EXPECT_NO_THROW(num = SimpleParser4::setAllDefaults(empty));
// We expect at least 3 parameters to be inserted.
EXPECT_TRUE(num >= 3);
// We expect at least 1 parameter to be inserted.
EXPECT_TRUE(num >= 1);
checkIntegerValue(empty, "valid-lifetime", 7200);
checkIntegerValue(empty, "rebind-timer", 1800);
checkIntegerValue(empty, "renew-timer", 900);
// Timers are optional and by default are not present
EXPECT_FALSE(empty->contains("rebind-timer"));
EXPECT_FALSE(empty->contains("renew-timer"));
// Make sure that preferred-lifetime is not set for v4 (it's v6 only
// parameter)
......
......@@ -6534,11 +6534,9 @@ const char* UNPARSED_CONFIGS[] = {
" \"option-data\": [ ],\n"
" \"preferred-lifetime\": 0,\n"
" \"rapid-commit\": false,\n"
" \"rebind-timer\": 0,\n"
" \"relay\": {\n"
" \"ip-addresses\": [ ]\n"
" },\n"
" \"renew-timer\": 0,\n"
" \"reservation-mode\": \"all\",\n"
" \"subnet6\": [\n"
" {\n"
......@@ -6593,8 +6591,7 @@ const char* UNPARSED_CONFIGS[] = {
" \"subnet\": \"2001:db1::/48\",\n"
" \"valid-lifetime\": 7200\n"
" }\n"
" ],\n"
" \"valid-lifetime\": 0\n"
" ]\n"
" }\n"
" ],\n"
" \"subnet6\": [ ]\n"
......
......@@ -136,18 +136,26 @@ Network::toElement() const {
map->set("require-client-classes", class_list);
}
// Set renew-timer
map->set("renew-timer",
Element::create(static_cast<long long>
(getT1().get())));
// T1, T2, and Valid are optional for SharedNetworks, and
// T1 and T2 are optional for Subnet4 thus we will only
// output them if they are marked as specified.
if (!getT1().unspecified()) {
map->set("renew-timer",
Element::create(static_cast<long long>(getT1().get())));
}
// Set rebind-timer
map->set("rebind-timer",
Element::create(static_cast<long long>
(getT2().get())));
if (!getT2().unspecified()) {
map->set("rebind-timer",
Element::create(static_cast<long long>(getT2().get())));
}
// Set valid-lifetime
map->set("valid-lifetime",
Element::create(static_cast<long long>
if (!getValid().unspecified()) {
map->set("valid-lifetime",
Element::create(static_cast<long long>
(getValid().get())));
}
// Set reservation mode
Network::HRMode hrmode = getHostReservationMode();
......
......@@ -109,7 +109,7 @@ public:
/// @brief Constructor.
Network()
: iface_name_(), client_class_(""), t1_(0), t2_(0), valid_(0),
: iface_name_(), client_class_(""), t1_(), t2_(), valid_(),
host_reservation_mode_(HR_ALL), cfg_option_(new CfgOption()) {
}
......
......@@ -652,9 +652,16 @@ Subnet4ConfigParser::initSubnet(data::ConstElementPtr params,
asiolink::IOAddress addr, uint8_t len) {
// The renew-timer and rebind-timer are optional. If not set, the
// option 58 and 59 will not be sent to a client. In this case the
// client will use default values based on the valid-lifetime.
Triplet<uint32_t> t1 = getInteger(params, "renew-timer");
Triplet<uint32_t> t2 = getInteger(params, "rebind-timer");
// client should formulate default values based on the valid-lifetime.
Triplet<uint32_t> t1;
if (params->contains("renew-timer")) {
t1 = getInteger(params, "renew-timer");
}
Triplet<uint32_t> t2;
if (params->contains("rebind-timer")) {
t2 = getInteger(params, "rebind-timer");
}
// The valid-lifetime is mandatory. It may be specified for a
// particular subnet. If not, the global value should be present.
......
// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2016-2018 Internet Systems Consortium, Inc. ("ISC")
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
......@@ -56,8 +56,6 @@ const SimpleDefaults SimpleParser4::OPTION4_DEFAULTS = {
/// in Dhcp4) are optional. If not defined, the following values will be
/// used.
const SimpleDefaults SimpleParser4::GLOBAL4_DEFAULTS = {
{ "renew-timer", Element::integer, "900" },
{ "rebind-timer", Element::integer, "1800" },
{ "valid-lifetime", Element::integer, "7200" },
{ "decline-probation-period", Element::integer, "86400" }, // 24h
{ "dhcp4o6-port", Element::integer, "0" },
......
// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC")
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
......@@ -109,12 +109,14 @@ TEST(CfgSharedNetworks4Test, duplicateName) {
TEST(CfgSharedNetworks4Test, unparse) {
SharedNetwork4Ptr network1(new SharedNetwork4("frog"));
SharedNetwork4Ptr network2(new SharedNetwork4("dog"));
network1->setIface("eth0");
network1->addRelayAddress(IOAddress("198.16.1.1"));
network1->addRelayAddress(IOAddress("198.16.1.2"));
network2->setIface("eth1");
network2->setT1(Triplet<uint32_t>(100));
network2->setT2(Triplet<uint32_t>(200));
network2->setValid(Triplet<uint32_t>(300));
CfgSharedNetworks4 cfg;
ASSERT_NO_THROW(cfg.add(network1));
......@@ -126,25 +128,22 @@ TEST(CfgSharedNetworks4Test, unparse) {
" \"interface\": \"eth1\",\n"
" \"match-client-id\": true,\n"
" \"name\": \"dog\",\n"
" \"rebind-timer\": 200,\n"
" \"option-data\": [ ],\n"
" \"rebind-timer\": 0,\n"
" \"renew-timer\": 100,\n"
" \"relay\": { \"ip-addresses\": [ ] },\n"
" \"renew-timer\": 0,\n"
" \"reservation-mode\": \"all\","
" \"subnet4\": [ ],\n"
" \"valid-lifetime\": 0\n"
" \"valid-lifetime\": 300\n"
" },\n"
" {\n"
" \"interface\": \"eth0\",\n"
" \"match-client-id\": true,\n"
" \"name\": \"frog\",\n"
" \"option-data\": [ ],\n"
" \"rebind-timer\": 0,\n"
" \"relay\": { \"ip-addresses\": [ \"198.16.1.1\", \"198.16.1.2\" ] },\n"
" \"renew-timer\": 0,\n"
" \"reservation-mode\": \"all\","
" \"subnet4\": [ ],\n"
" \"valid-lifetime\": 0\n"
" \"subnet4\": [ ]\n"
" }\n"
"]\n";
......
// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC")
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
......@@ -113,7 +113,11 @@ TEST(CfgSharedNetworks6Test, unparse) {
network1->setIface("eth0");
network1->addRelayAddress(IOAddress("2001:db8:1::1"));
network1->addRelayAddress(IOAddress("2001:db8:1::2"));
network2->setIface("eth1");
network2->setT1(Triplet<uint32_t>(100));
network2->setT2(Triplet<uint32_t>(200));
network2->setValid(Triplet<uint32_t>(300));
CfgSharedNetworks6 cfg;
ASSERT_NO_THROW(cfg.add(network1));
......@@ -127,12 +131,12 @@ TEST(CfgSharedNetworks6Test, unparse) {
" \"option-data\": [ ],\n"
" \"preferred-lifetime\": 0,\n"
" \"rapid-commit\": false,\n"
" \"rebind-timer\": 0,\n"
" \"rebind-timer\": 200,\n"
" \"relay\": { \"ip-addresses\": [ ] },\n"
" \"renew-timer\": 0,\n"
" \"renew-timer\": 100,\n"
" \"reservation-mode\": \"all\","
" \"subnet6\": [ ],\n"
" \"valid-lifetime\": 0\n"
" \"valid-lifetime\": 300\n"
" },\n"
" {\n"
" \"interface\": \"eth0\",\n"
......@@ -140,12 +144,9 @@ TEST(CfgSharedNetworks6Test, unparse) {
" \"option-data\": [ ],\n"
" \"preferred-lifetime\": 0,\n"
" \"rapid-commit\": false,\n"
" \"rebind-timer\": 0,\n"
" \"relay\": { \"ip-addresses\": [ \"2001:db8:1::1\", \"2001:db8:1::2\" ] },\n"
" \"renew-timer\": 0,\n"
" \"reservation-mode\": \"all\","
" \"subnet6\": [ ],\n"
" \"valid-lifetime\": 0\n"
" \"subnet6\": [ ]\n"
" }\n"
"]\n";
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment