[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 60b5a35c061c10a97b5fdf4edce7aab709b2799d..5670cff8e48e7c3ad59765633b9ec8e60f17c603 100644 (file)
--- 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 a332cbb5eb38624ec9dc32b7e07715332b14db5b..2a88558aed451e89affbeeeec26efb76cb0d230f 100644 (file)
--- 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<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".
+    subnet_.reset(new Subnet4(IOAddress("192.0.2.0"), 24,
+                              Triplet<uint32_t>(),
+                              Triplet<uint32_t>(),
+                              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<NakedDhcpv4Srv> 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<uint32_t>(),
+                              Triplet<uint32_t>(),
+                              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 ab1afc642ffedf7e51690df6be8e647d4c30f79a..9567b24bc624f465b1b545bb48d404ea8c317d41 100644 (file)
--- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc
+++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc
@@ -20,6 +20,7 @@
 #include <dhcp4/config_parser.h>
 #include <dhcp4/tests/dhcp4_test_utils.h>
 #include <dhcp/option4_addrlst.h>
+#include <dhcp/option_int.h>
 #include <dhcp/option_int_array.h>
 #include <dhcp/option_custom.h>
 #include <dhcp/iface_mgr.h>
@@ -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 b978979c0256ddaceb296b7af54c8626421eddb1..c17bd84e70966eb358e247b2f09f06754192e617 100644 (file)
--- 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 f11b4eba1fe50a9217f689e889fc50ff32d0609f..b1c0f45e9acd0a62b632ecbce2481cef8e800be1 100644 (file)
--- 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<typename T>
+class OptionInt;
+
+/// @defgroup option_int_array_defs Typedefs for OptionInt class.
+///
+/// @brief Classes that represent options comprising an integer.
+///
+/// @{
+typedef OptionInt<uint8_t> OptionUint8;
+typedef boost::shared_ptr<OptionUint8> OptionUint8Ptr;
+typedef OptionInt<uint16_t> OptionUint16;
+typedef boost::shared_ptr<OptionUint16> OptionUint16Ptr;
+typedef OptionInt<uint32_t> OptionUint32;
+typedef boost::shared_ptr<OptionUint32> 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,