[3564] Addressed review comments.

doc/Makefile.am

@@ -5,6 +5,7 @@ EXTRA_DIST = version.ent.in differences.txt Doxyfile Doxyfile-xml
 nobase_dist_doc_DATA  = examples/kea4/single-subnet.json
 nobase_dist_doc_DATA += examples/kea4/several-subnets.json
 nobase_dist_doc_DATA += examples/kea4/multiple-options.json
+nobase_dist_doc_DATA += examples/kea4/reservations.json
 nobase_dist_doc_DATA += examples/kea6/simple.json
 nobase_dist_doc_DATA += examples/kea6/several-subnets.json
 nobase_dist_doc_DATA += examples/kea6/multiple-options.json

doc/examples/kea4/reservations.json

@@ -0,0 +1,68 @@
+# This is an example configuration file for the DHCPv4 server in Kea.
+# It contains one subnet in which there are two static address reservations
+# for the clients identified by the MAC addresses.
+{ "Dhcp4":
+
+{
+# Kea is told to listen on eth0 interface only.
+  "interfaces": [ "eth0" ],
+
+# We need to specify lease type. As of May 2014, three backends are supported:
+# memfile, mysql and pgsql. We'll just use memfile, because it doesn't require
+# any prior set up.
+  "lease-database": {
+    "type": "memfile"
+  },
+
+# Addresses will be assigned with valid lifetimes being 4000. Client
+# is told to start renewing after 1000 seconds. If the server does not respond
+# after 2000 seconds since the lease was granted, client is supposed
+# to start REBIND procedure (emergency renewal that allows switching
+# to a different server).
+  "valid-lifetime": 4000,
+
+# Renew and rebind timers are commented out. This implies that options
+# 58 and 59 will not be sent to the client. In this case it is up to
+# the client to pick the timer values according to RFC2131. Uncomment the
+# timers to send these options to the client.
+#  "renew-timer": 1000,
+#  "rebind-timer": 2000,
+
+# Define a subnet with two reservations for the 192.0.2.202 and
+# 192.0.2.203 address.
+  "subnet4": [
+    {
+       "pools": [ { "pool":  "192.0.2.1 - 192.0.2.200" } ],
+       "subnet": "192.0.2.0/24",
+       "interface": "eth0",
+       "reservations": [
+         {
+             "hw-address": "1a:1b:1c:1d:1e:1f",
+             "ip-address": "192.0.2.202"
+         },
+         {
+             "hw-address": "0a:0b:0c:0d:0e:0f",
+             "ip-address": "192.0.2.203"
+         }
+       ]
+    } 
+  ]
+},
+
+# The following configures logging. It assumes that messages with at least
+# informational level (info, warn, error) will will be logged to stdout.
+"Logging": {
+    "loggers": [
+        {
+            "name": "kea-dhcp4",
+            "output_options": [
+                {
+                    "output": "stdout"
+                }
+            ],
+            "severity": "INFO"
+        }
+    ]
+}
+
+}

src/bin/dhcp4/tests/dhcp4_client.cc

@@ -349,6 +349,11 @@ Dhcp4Client::sendMsg(const Pkt4Ptr& msg) {
     srv_->run();
 }
 
+void
+Dhcp4Client::setHWAddress(const std::string& hwaddr_str) {
+    hwaddr_.reset(new HWAddr(HWAddr::fromText(hwaddr_str)));
+}
+
 } // end of namespace isc::dhcp::test
 } // end of namespace isc::dhcp
 } // end of namespace isc

src/bin/dhcp4/tests/dhcp4_client.h

@@ -268,6 +268,11 @@ public:
         dest_addr_ = dest_addr;
     }
 
+    /// @brief Sets the explicit hardware address for the client.
+    ///
+    /// @param hwaddr_str String representation of the HW address.
+    void setHWAddress(const std::string& hwaddr_str);
+
     /// @brief Sets client state.
     ///
     /// Depending on the current state the client's behavior is different

src/bin/dhcp4/tests/dora_unittest.cc

@@ -51,6 +51,12 @@ namespace {
 ///   - Domain Name Server option present: 192.0.2.202, 192.0.2.203.
 ///   - Log Servers option present: 192.0.2.200 and 192.0.2.201
 ///   - Quotes Servers option present: 192.0.2.202, 192.0.2.203.
+///
+/// - Configuration 2:
+///   - Use for testing simple scenarios with host reservations
+///   - 1 subnet: 10.0.0.0/24
+///   - One reservation for the client using MAC address:
+///     aa:bb:cc:dd:ee:ff, reserved address 10.0.0.7
 const char* DORA_CONFIGS[] = {
 // Configuration 0
     "{ \"interfaces\": [ \"*\" ],"
@@ -124,6 +130,21 @@ const char* DORA_CONFIGS[] = {
         "        \"space\": \"dhcp4\""
         "    } ]"
         " } ]"
+    "}",
+
+// Configuration 2
+    "{ \"interfaces\": [ \"*\" ],"
+        "\"valid-lifetime\": 600,"
+        "\"subnet4\": [ { "
+        "    \"subnet\": \"10.0.0.0/24\", "
+        "    \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ],"
+        "    \"reservations\": [ "
+        "       {"
+        "         \"hw-address\": \"aa:bb:cc:dd:ee:ff\","
+        "         \"ip-address\": \"10.0.0.7\""
+        "       }"
+        "    ]"
+        "} ]"
     "}"
 };
 
@@ -407,43 +428,87 @@ TEST_F(DORATest, ciaddr) {
     EXPECT_EQ("0.0.0.0", resp->getCiaddr().toText());
 }
 
+// This is a simple test for the host reservation. It creates a reservation
+// for an address for a single client, identified by the HW address. The
+// test verifies that the client using this HW address will obtain a
+// lease for the reserved address. It also checks that the client using
+// a different HW address will obtain an address from the dynamic pool.
+TEST_F(DORATest, reservation) {
+    // Client A is a one which will have a reservation.
+    Dhcp4Client clientA(Dhcp4Client::SELECTING);
+    // Set explicit HW address so as it matches the reservation in the
+    // configuration used below.
+    clientA.setHWAddress("aa:bb:cc:dd:ee:ff");
+    // Configure DHCP server.
+    configure(DORA_CONFIGS[2], *clientA.getServer());
+    // Client A performs 4-way exchange and should obtain a reserved
+    // address.
+    ASSERT_NO_THROW(clientA.doDORA(boost::shared_ptr<
+                                  IOAddress>(new IOAddress("0.0.0.0"))));
+    // Make sure that the server responded.
+    ASSERT_TRUE(clientA.getContext().response_);
+    Pkt4Ptr resp = clientA.getContext().response_;
+    // Make sure that the server has responded with DHCPACK.
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+    // Make sure that the client has got the lease for the reserved address.
+    ASSERT_EQ("10.0.0.7", clientA.config_.lease_.addr_.toText());
+
+    // Client B uses the same server as Client A.
+    Dhcp4Client clientB(clientA.getServer(), Dhcp4Client::SELECTING);
+    // Client B has no reservation so it should get the lease from
+    // the dynamic pool.
+    ASSERT_NO_THROW(clientB.doDORA(boost::shared_ptr<
+                                  IOAddress>(new IOAddress("0.0.0.0"))));
+    // Make sure that the server responded.
+    ASSERT_TRUE(clientB.getContext().response_);
+    resp = clientB.getContext().response_;
+    // Make sure that the server has responded with DHCPACK.
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+    // Obtain the subnet to which the returned address belongs.
+    Subnet4Ptr subnet = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->
+        selectSubnet(clientB.config_.lease_.addr_);
+    ASSERT_TRUE(subnet);
+    // Make sure that the address has been allocated from the dynamic pool.
+    ASSERT_TRUE(subnet->inPool(Lease::TYPE_V4, clientB.config_.lease_.addr_));
+}
+
 // This test checks the following scenario:
-// - Client A performs 4-way exchange and obrains a lease from the dynamic pool.
-// - Reservation is created for the client A using an address out of the dynamic
-//   pool.
-// - Client A renews the lease.
-// - Server responds with DHCPNAK to indicate that the client should stop using
-//   an address for which it has a lease. Server doesn't want to renew an
-//   address for which the client doesn't have a reservation, while it has
-//   a reservation for a different address.
-// - Client A receives a DHCPNAK and returns to the DHCP server discovery.
-// - Client A performs a 4-way exchange with a server and the server allocates
+// 1. Client A performs 4-way exchange and obrains a lease from the dynamic pool.
+// 2. Reservation is created for the client A using an address out of the dynamic
+//    pool.
+// 3. Client A renews the lease.
+// 4. Server responds with DHCPNAK to indicate that the client should stop using
+//    an address for which it has a lease. Server doesn't want to renew an
+//    address for which the client doesn't have a reservation, while it has
+//    a reservation for a different address.
+// 5. Client A receives a DHCPNAK and returns to the DHCP server discovery.
+// 6. Client A performs a 4-way exchange with a server and the server allocates
 //   a reserved address to the Client A.
-// - Client A renews the allocated address and the server returns a DHCPACK.
-// - Reservation for the Client A is removed.
-// - Client A renews the (previously reserved) lease and the server returns
-//   DHCPNAK because the address in use is neither reserved nor belongs to
-//   the dynamic pool.
-// - Client A returns to the DHCP server discovery.
-// - Client A uses 4-way exchange to obtain a lease from the dynamic pool.
-// - The new address that the Client A is using is reserved for Client B.
-//   Client A still holds this address.
-// - Client B uses 4-way exchange to obtain a new lease.
-// - The server determines that the Client B has a reservation for the
-//   address which is in use by Client A. The server drops the client's
-//   DHCPDISCOVER message.
-// - Client A renews the lease.
-// - The server determines that the address that Client A is using is reserved
-//   for Client B. The server returns DHCPNAK to the Client A.
-// - Client B uses 4-way echange to obtain the reserved lease but the lease
-//   for the Client A hasn't been removed yet. Client B's DHCPDISCOVER
-//   message is dropped again.
-// - Client A uses 4-way exchange to allocate a new lease.
-// - The server allocates a new lease from the dynamic pool but it avoids
-//   allocating the address reserved for the Client B.
-// - Client B uses 4-way exchange to obtain a new lease.
-// - The server finally allocates a reserved address to the Client B.
-TEST_F(DORATest, reservations) {
+// 7. Client A renews the allocated address and the server returns a DHCPACK.
+// 8. Reservation for the Client A is removed.
+// 9. Client A renews the (previously reserved) lease and the server returns
+//    DHCPNAK because the address in use is neither reserved nor belongs to
+//    the dynamic pool.
+// 10. Client A returns to the DHCP server discovery.
+// 11. Client A uses 4-way exchange to obtain a lease from the dynamic pool.
+// 12. The new address that the Client A is using is reserved for Client B.
+//     Client A still holds this address.
+// 13. Client B uses 4-way exchange to obtain a new lease.
+// 14. The server determines that the Client B has a reservation for the
+//     address which is in use by Client A. The server drops the client's
+//     DHCPDISCOVER message.
+// 15. Client A renews the lease.
+// 16. The server determines that the address that Client A is using is reserved
+//     for Client B. The server returns DHCPNAK to the Client A.
+// 17. Client B uses 4-way echange to obtain the reserved lease but the lease
+//     for the Client A hasn't been removed yet. Client B's DHCPDISCOVER
+//     message is dropped again.
+// 18. Client A uses 4-way exchange to allocate a new lease.
+// 19. The server allocates a new lease from the dynamic pool but it avoids
+//     allocating the address reserved for the Client B.
+// 20. Client B uses 4-way exchange to obtain a new lease.
+// 21. The server finally allocates a reserved address to the Client B.
+TEST_F(DORATest, reservationsWithConflicts) {
     Dhcp4Client client(Dhcp4Client::SELECTING);
     // Configure DHCP server.
     configure(DORA_CONFIGS[0], *client.getServer());
@@ -456,10 +521,6 @@ TEST_F(DORATest, reservations) {
     Pkt4Ptr resp = client.getContext().response_;
     // Make sure that the server has responded with DHCPACK.
     ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
-    // Response must not be relayed.
-    EXPECT_FALSE(resp->isRelayed());
-    // Make sure that the server id is present.
-    EXPECT_EQ("10.0.0.1", client.config_.serverid_.toText());
     // Make sure that the client has got the lease with the requested address.
     ASSERT_EQ("10.0.0.50", client.config_.lease_.addr_.toText());
 

src/lib/dhcpsrv/alloc_engine.cc

@@ -516,7 +516,7 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
         /// The ticket introducing a context and host reservation in the
         /// allocation engine is complex enough by itself to warrant that
         /// the API change is done with a separate ticket.
-        Context4 ctx;
+        ClientContext4 ctx;
         ctx.subnet_ = subnet;
         ctx.clientid_ = clientid;
         ctx.hwaddr_ = hwaddr;
@@ -656,7 +656,7 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
         }
 
         // No address was requested, requested address was not in pool or the
-        // allocation was not successful so far. Let's try to find a diffferent
+        // allocation was not successful so far. Let's try to find a different
         // address for the client.  Search the pool until first of the following
         // occurs:
         // - we find a free address
@@ -726,7 +726,7 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
 
 Lease4Ptr
 AllocEngine::renewLease4(const Lease4Ptr& lease,
-                         AllocEngine::Context4& ctx) {
+                         AllocEngine::ClientContext4& ctx) {
     if (!lease) {
         isc_throw(BadValue, "null lease specified for renewLease4");
     }
@@ -893,7 +893,7 @@ Lease6Ptr AllocEngine::reuseExpiredLease(Lease6Ptr& expired,
 
 Lease4Ptr
 AllocEngine::reuseExpiredLease(Lease4Ptr& expired,
-                               AllocEngine::Context4& ctx) {
+                               AllocEngine::ClientContext4& ctx) {
     if (!expired) {
         isc_throw(BadValue, "null lease specified for reuseExpiredLease");
     }
@@ -962,7 +962,7 @@ AllocEngine::reuseExpiredLease(Lease4Ptr& expired,
 }
 
 Lease4Ptr
-AllocEngine::replaceClientLease(Lease4Ptr& lease, Context4& ctx) {
+AllocEngine::replaceClientLease(Lease4Ptr& lease, ClientContext4& ctx) {
 
     if (!lease) {
         isc_throw(BadValue, "null lease specified for replaceClientLease");
@@ -1052,7 +1052,7 @@ AllocEngine::replaceClientLease(Lease4Ptr& lease, Context4& ctx) {
 
 Lease4Ptr
 AllocEngine::reallocateClientLease(Lease4Ptr& lease,
-                                   AllocEngine::Context4& ctx) {
+                                   AllocEngine::ClientContext4& ctx) {
     // Save the old lease, before renewal.
     ctx.old_lease_.reset(new Lease4(*lease));
 
@@ -1263,7 +1263,7 @@ Lease4Ptr AllocEngine::createLease4(const SubnetPtr& subnet,
 
 void
 AllocEngine::updateLease4Information(const Lease4Ptr& lease,
-                                     AllocEngine::Context4& ctx) const {
+                                     AllocEngine::ClientContext4& ctx) const {
     // This should not happen in theory.
     if (!lease) {
         isc_throw(BadValue, "null lease specified for updateLease4Information");

src/lib/dhcpsrv/alloc_engine.h

@@ -232,7 +232,7 @@ protected:
     /// that the big advantage of using the context structure to pass
     /// information to the allocation engine methods is that adding
     /// new information doesn't modify the API of the allocation engine.
-    struct Context4 {
+    struct ClientContext4 {
         /// @brief Subnet selected for the client by the server.
         SubnetPtr subnet_;
 
@@ -255,6 +255,9 @@ protected:
         bool rev_dns_update_;
 
         /// @brief Hostname.
+        ///
+        /// The server retrieves the hostname from the Client FQDN option,
+        /// Hostname option or the host reservation record for the client.
         std::string hostname_;
 
         /// @brief Callout handle associated with the client's message.
@@ -279,10 +282,18 @@ protected:
         /// @c AllocEngine::allocateLease4. This flag is set to true to
         /// indicate that an attempt to allocate a lease should be
         /// interrupted.
+        ///
+        /// One possible use case is when the allocation engine tries
+        /// to renew the client's lease and the leased address appears
+        /// to be reserved for someone else. In such case, the allocation
+        /// engine should signal to the server that the address that the
+        /// client should stop using this address. The
+        /// @c AllocEngine::renewLease4 sets this flag so as the
+        /// upstream methods return the NULL lease pointer to the server.
         bool interrupt_processing_;
 
         /// @brief Default constructor.
-        Context4()
+        ClientContext4()
             : subnet_(), clientid_(), hwaddr_(), requested_address_("0.0.0.0"),
               fwd_dns_update_(false), rev_dns_update_(false),
               hostname_(""), callout_handle_(), fake_allocation_(false),
@@ -311,9 +322,9 @@ protected:
     ///   has a reservation for an address for which the lease was created or
     ///   the client desires to renew the lease for this address (ciaddr or
     ///   requested IP address option), the server renews the lease for the
-    ///   client. If the client desires a different address or the client has
-    ///   a (potentially new) reservation for a different address, the existing
-    ///   lease is replaced with a new lease.
+    ///   client. If the client desires a different address or the server has
+    ///   a (potentially new) reservation for a different address for this
+    ///   client, the existing lease is replaced with a new lease.
     /// - If the client has no lease in the lease database the server will try
     ///   to allocate a new lease. If the client has a reservation for the
     ///   particular address or if it has specified a desired address the
@@ -379,7 +390,8 @@ protected:
     /// returned, it is an indication that allocation engine reused/renewed an
     /// existing lease.
     ///
-    /// @todo Replace parameters with a single parameter of a @c Context4 type.
+    /// @todo Replace parameters with a single parameter of a
+    /// @c ClientContext4 type.
     ///
     /// @param subnet subnet the allocation should come from
     /// @param clientid Client identifier
@@ -422,7 +434,7 @@ protected:
     /// @return Returns renewed lease. Note that the lease is only updated when
     /// it is an actual allocation (not processing DHCPDISCOVER message).
     Lease4Ptr
-    renewLease4(const Lease4Ptr& lease, Context4& ctx);
+    renewLease4(const Lease4Ptr& lease, ClientContext4& ctx);
 
     /// @brief Allocates an IPv6 lease
     ///
@@ -521,7 +533,7 @@ private:
     /// @param ctx A context containing information from the server about the
     /// client and its message.
     void updateLease4Information(const Lease4Ptr& lease,
-                                 Context4& ctx) const;
+                                 ClientContext4& ctx) const;
 
     /// @brief creates a lease and inserts it in LeaseMgr if necessary
     ///
@@ -574,7 +586,7 @@ private:
     /// @return Updated lease instance.
     /// @throw BadValue if trying to reuse a lease which is still valid or
     /// when the provided parameters are invalid.
-    Lease4Ptr reuseExpiredLease(Lease4Ptr& expired, Context4& ctx);
+    Lease4Ptr reuseExpiredLease(Lease4Ptr& expired, ClientContext4& ctx);
 
     /// @brief Updates the existing, non expired lease with a information from
     /// the context.
@@ -592,7 +604,7 @@ private:
     ///
     /// @return Pointer to the updated lease.
     /// @throw BadValue if the provided parameters are invalid.
-    Lease4Ptr replaceClientLease(Lease4Ptr& lease, Context4& ctx);
+    Lease4Ptr replaceClientLease(Lease4Ptr& lease, ClientContext4& ctx);
 
     /// @brief Replace or renew client's lease.
     ///
@@ -610,7 +622,7 @@ private:
     ///
     /// @return Updated lease, or NULL if allocation was unsucessful.
     /// @throw BadValue if specified parameters are invalid.
-    Lease4Ptr reallocateClientLease(Lease4Ptr& lease, Context4& ctx);
+    Lease4Ptr reallocateClientLease(Lease4Ptr& lease, ClientContext4& ctx);
 
     /// @brief Reuses expired IPv6 lease
     ///

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

@@ -487,8 +487,8 @@ public:
     Pool4Ptr pool_;             ///< Pool belonging to subnet_
     LeaseMgrFactory factory_;   ///< Pointer to LeaseMgr factory
     Lease4Ptr old_lease_;       ///< Holds previous instance of the lease.
-    AllocEngine::Context4 ctx_; ///< Context information passed to various
-                                ///< allocation engine functions.
+    AllocEngine::ClientContext4 ctx_; ///< Context information passed to various
+                                     ///< allocation engine functions.
 };
 
 // This test checks if the v6 Allocation Engine can be instantiated, parses
@@ -1682,7 +1682,8 @@ TEST_F(AllocEngine4Test,reservedAddressNoHintFakeAllocation) {
 // - Client has a reservation.
 // - Client sends DHCPREQUEST with a requested IP address
 // - Server returns DHCPNAK when requested IP address is different than
-// the reserved address.
+//   the reserved address. Note that the allocation engine returns NULL
+//   to indicate to the server that it should send DHCPNAK.
 // - Server allocates a reserved address to the client when the client requests
 // this address using requested IP address option.
 TEST_F(AllocEngine4Test, reservedAddressHint) {
@@ -1699,9 +1700,13 @@ TEST_F(AllocEngine4Test, reservedAddressHint) {
                                             false, false, "",
                                             false, CalloutHandlePtr(),
                                             old_lease_);
+    // The client requested a different address than reserved, so
+    // the allocation engine should return NULL lease. When the server
+    // receives a NULL lease for the client, it will send a DHCPNAK.
     ASSERT_FALSE(lease);
     ASSERT_FALSE(old_lease_);
 
+    // Now, request a correct address. The client should obtain it.
     lease = engine.allocateLease4(subnet_, clientid_, hwaddr_,
                                   IOAddress("192.0.2.123"),
                                   false, false, "",