From: Marcin Siodelski Date: Tue, 27 May 2014 17:58:11 +0000 (+0200) Subject: [3336] DHCPv4 server includes renew and rebind timers when specified. X-Git-Tag: trac3434_base~14^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3eebafd76b4fecdf115ab4ead52d3ca21663d1b9;p=thirdparty%2Fkea.git [3336] DHCPv4 server includes renew and rebind timers when specified. --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 60b5a35c06..5670cff8e4 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -1044,15 +1044,28 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) { } // IP Address Lease time (type 51) - opt = OptionPtr(new Option(Option::V4, DHO_DHCP_LEASE_TIME)); - opt->setUint32(lease->valid_lft_); + opt.reset(new OptionUint32(Option::V4, DHO_DHCP_LEASE_TIME, + lease->valid_lft_)); answer->addOption(opt); // Subnet mask (type 1) answer->addOption(getNetmaskOption(subnet)); - /// @todo: send renew timer option (T1, option 58) - /// @todo: send rebind timer option (T2, option 59) + // renewal-timer (type 58) + if (!subnet->getT1().unspecified()) { + OptionUint32Ptr t1(new OptionUint32(Option::V4, + DHO_DHCP_RENEWAL_TIME, + subnet->getT1())); + answer->addOption(t1); + } + + // rebind timer (type 59) + if (!subnet->getT2().unspecified()) { + OptionUint32Ptr t2(new OptionUint32(Option::V4, + DHO_DHCP_REBINDING_TIME, + subnet->getT2())); + answer->addOption(t2); + } // Create NameChangeRequests if DDNS is enabled and this is a // real allocation. diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index a332cbb5eb..2a88558aed 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -558,13 +558,51 @@ TEST_F(Dhcpv4SrvTest, DiscoverBasic) { // Check that address was returned from proper range, that its lease // lifetime is correct, that T1 and T2 are returned properly - checkAddressParams(offer, subnet_); + checkAddressParams(offer, subnet_, true, true); // Check identifiers checkServerId(offer, srv->getServerID()); checkClientId(offer, clientid); } +// Check that option 58 and 59 are not included if they are not specified. +TEST_F(Dhcpv4SrvTest, DiscoverNoTimers) { + IfaceMgrTestConfig test_config(true); + IfaceMgr::instance().openSockets4(); + + boost::scoped_ptr 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". + subnet_.reset(new Subnet4(IOAddress("192.0.2.0"), 24, + Triplet(), + Triplet(), + 3000)); + pool_ = Pool4Ptr(new Pool4(IOAddress("192.0.2.100"), + IOAddress("192.0.2.110"))); + subnet_->addPool(pool_); + CfgMgr::instance().deleteSubnets4(); + CfgMgr::instance().addSubnet4(subnet_); + + // Pass it to the server and get an offer + Pkt4Ptr offer = srv->processDiscover(dis); + + // Check if we get response at all + checkResponse(offer, DHCPOFFER, 1234); + + // T1 and T2 timers must not be present. + checkAddressParams(offer, subnet_, false, false); + + // 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 @@ -601,7 +639,7 @@ TEST_F(Dhcpv4SrvTest, DiscoverHint) { // Check that address was returned from proper range, that its lease // lifetime is correct, that T1 and T2 are returned properly - checkAddressParams(offer, subnet_); + checkAddressParams(offer, subnet_, true, true); EXPECT_EQ(offer->getYiaddr(), hint); @@ -644,7 +682,7 @@ TEST_F(Dhcpv4SrvTest, DiscoverNoClientId) { // Check that address was returned from proper range, that its lease // lifetime is correct, that T1 and T2 are returned properly - checkAddressParams(offer, subnet_); + checkAddressParams(offer, subnet_, true, true); EXPECT_EQ(offer->getYiaddr(), hint); @@ -687,7 +725,7 @@ TEST_F(Dhcpv4SrvTest, DiscoverInvalidHint) { // Check that address was returned from proper range, that its lease // lifetime is correct, that T1 and T2 are returned properly - checkAddressParams(offer, subnet_); + checkAddressParams(offer, subnet_, true, true); EXPECT_NE(offer->getYiaddr(), hint); @@ -750,9 +788,9 @@ TEST_F(Dhcpv4SrvTest, ManyDiscovers) { IOAddress addr3 = offer3->getYiaddr(); // Check that the assigned address is indeed from the configured pool - checkAddressParams(offer1, subnet_); - checkAddressParams(offer2, subnet_); - checkAddressParams(offer3, subnet_); + checkAddressParams(offer1, subnet_, true, true); + checkAddressParams(offer2, subnet_, true, true); + checkAddressParams(offer3, subnet_, true, true); // Check server-ids checkServerId(offer1, srv->getServerID()); @@ -840,7 +878,7 @@ TEST_F(Dhcpv4SrvTest, RequestBasic) { // Check that address was returned from proper range, that its lease // lifetime is correct, that T1 and T2 are returned properly - checkAddressParams(ack, subnet_); + checkAddressParams(ack, subnet_, true, true); // Check identifiers checkServerId(ack, srv->getServerID()); @@ -853,6 +891,45 @@ TEST_F(Dhcpv4SrvTest, RequestBasic) { LeaseMgrFactory::instance().deleteLease(l->addr_); } +// Check that option 58 and 59 are not included if they are not specified. +TEST_F(Dhcpv4SrvTest, RequestNoTimers) { + IfaceMgrTestConfig test_config(true); + IfaceMgr::instance().openSockets4(); + + boost::scoped_ptr srv; + ASSERT_NO_THROW(srv.reset(new NakedDhcpv4Srv(0))); + + Pkt4Ptr req = Pkt4Ptr(new Pkt4(DHCPREQUEST, 1234)); + req->setRemoteAddr(IOAddress("192.0.2.1")); + OptionPtr clientid = generateClientId(); + req->addOption(clientid); + req->setIface("eth1"); + + // Recreate a subnet but set T1 and T2 to "unspecified". + subnet_.reset(new Subnet4(IOAddress("192.0.2.0"), 24, + Triplet(), + Triplet(), + 3000)); + pool_ = Pool4Ptr(new Pool4(IOAddress("192.0.2.100"), + IOAddress("192.0.2.110"))); + subnet_->addPool(pool_); + CfgMgr::instance().deleteSubnets4(); + CfgMgr::instance().addSubnet4(subnet_); + + // Pass it to the server and get an ACK. + Pkt4Ptr ack = srv->processRequest(req); + + // Check if we get response at all + checkResponse(ack, DHCPACK, 1234); + + // T1 and T2 timers must not be present. + checkAddressParams(ack, subnet_, false, false); + + // Check identifiers + checkServerId(ack, srv->getServerID()); + checkClientId(ack, clientid); +} + // This test verifies that incoming REQUEST can be handled properly, that an // ACK is generated, that the response has an address and that address // really belongs to the configured pool. @@ -927,9 +1004,9 @@ TEST_F(Dhcpv4SrvTest, ManyRequests) { EXPECT_EQ(req_addr3, addr3); // Check that the assigned address is indeed from the configured pool - checkAddressParams(ack1, subnet_); - checkAddressParams(ack2, subnet_); - checkAddressParams(ack3, subnet_); + checkAddressParams(ack1, subnet_, true, true); + checkAddressParams(ack2, subnet_, true, true); + checkAddressParams(ack3, subnet_, true, true); // Check DUIDs checkServerId(ack1, srv->getServerID()); @@ -1049,7 +1126,7 @@ TEST_F(Dhcpv4SrvTest, RenewBasic) { // Check that address was returned from proper range, that its lease // lifetime is correct, that T1 and T2 are returned properly - checkAddressParams(ack, subnet_); + checkAddressParams(ack, subnet_, true, true); // Check identifiers checkServerId(ack, srv->getServerID()); diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index ab1afc642f..9567b24bc6 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -238,9 +239,10 @@ HWAddrPtr Dhcpv4SrvTest::generateHWAddr(size_t size /*= 6*/) { return (HWAddrPtr(new HWAddr(mac, hw_type))); } -void Dhcpv4SrvTest::checkAddressParams(const Pkt4Ptr& rsp, const SubnetPtr subnet, - bool t1_mandatory /*= false*/, - bool t2_mandatory /*= false*/) { +void Dhcpv4SrvTest::checkAddressParams(const Pkt4Ptr& rsp, + const SubnetPtr subnet, + bool t1_present, + bool t2_present /*= false*/) { // Technically inPool implies inRange, but let's be on the safe // side and check both. @@ -248,31 +250,32 @@ void Dhcpv4SrvTest::checkAddressParams(const Pkt4Ptr& rsp, const SubnetPtr subne EXPECT_TRUE(subnet->inPool(Lease::TYPE_V4, rsp->getYiaddr())); // Check lease time - OptionPtr opt = rsp->getOption(DHO_DHCP_LEASE_TIME); + OptionUint32Ptr opt = boost::dynamic_pointer_cast< + OptionUint32>(rsp->getOption(DHO_DHCP_LEASE_TIME)); if (!opt) { ADD_FAILURE() << "Lease time option missing in response"; } else { - EXPECT_EQ(opt->getUint32(), subnet->getValid()); + EXPECT_EQ(opt->getValue(), subnet->getValid()); } // Check T1 timer - opt = rsp->getOption(DHO_DHCP_RENEWAL_TIME); - if (opt) { - EXPECT_EQ(opt->getUint32(), subnet->getT1()); + opt = boost::dynamic_pointer_cast< + OptionUint32>(rsp->getOption(DHO_DHCP_RENEWAL_TIME)); + if (t1_present) { + ASSERT_TRUE(opt) << "Required T1 option missing"; + EXPECT_EQ(opt->getValue(), subnet->getT1()); } else { - if (t1_mandatory) { - ADD_FAILURE() << "Required T1 option missing"; - } + EXPECT_FALSE(opt); } // Check T2 timer - opt = rsp->getOption(DHO_DHCP_REBINDING_TIME); - if (opt) { - EXPECT_EQ(opt->getUint32(), subnet->getT2()); + opt = boost::dynamic_pointer_cast< + OptionUint32>(rsp->getOption(DHO_DHCP_REBINDING_TIME)); + if (t2_present) { + ASSERT_TRUE(opt) << "Required T2 option missing"; + EXPECT_EQ(opt->getValue(), subnet->getT2()); } else { - if (t2_mandatory) { - ADD_FAILURE() << "Required T2 option missing"; - } + EXPECT_FALSE(opt); } } diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index b978979c02..c17bd84e70 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h @@ -290,11 +290,13 @@ public: /// @param rsp response to be checked /// @param subnet subnet that should be used to verify assigned address /// and options - /// @param t1_mandatory is T1 mandatory? - /// @param t2_mandatory is T2 mandatory? + /// @param t1_present check that t1 must be present (true) or must not be + /// present (false) + /// @param t2_present check that t2 must be present (true) or must not be + /// present (false) void checkAddressParams(const Pkt4Ptr& rsp, const SubnetPtr subnet, - bool t1_mandatory = false, - bool t2_mandatory = false); + bool t1_present = false, + bool t2_present = false); /// @brief Basic checks for generated response (message type and trans-id). /// diff --git a/src/lib/dhcp/option_int.h b/src/lib/dhcp/option_int.h index f11b4eba1f..b1c0f45e9a 100644 --- a/src/lib/dhcp/option_int.h +++ b/src/lib/dhcp/option_int.h @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2014 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 @@ -25,6 +25,22 @@ namespace isc { namespace dhcp { +template +class OptionInt; + +/// @defgroup option_int_array_defs Typedefs for OptionInt class. +/// +/// @brief Classes that represent options comprising an integer. +/// +/// @{ +typedef OptionInt OptionUint8; +typedef boost::shared_ptr OptionUint8Ptr; +typedef OptionInt OptionUint16; +typedef boost::shared_ptr OptionUint16Ptr; +typedef OptionInt OptionUint32; +typedef boost::shared_ptr OptionUint32Ptr; +/// @} + /// This template class represents DHCP option with single value. /// This value is of integer type and can be any of the following: /// - uint8_t,