]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[3694] Addressed review comments.
authorMarcin Siodelski <marcin@isc.org>
Thu, 5 Mar 2015 19:43:02 +0000 (20:43 +0100)
committerMarcin Siodelski <marcin@isc.org>
Thu, 5 Mar 2015 19:43:02 +0000 (20:43 +0100)
doc/guide/dhcp4-srv.xml
src/bin/dhcp4/tests/direct_client_unittest.cc
src/bin/dhcp4/tests/dora_unittest.cc
src/lib/asiolink/tests/io_address_unittest.cc
src/lib/dhcpsrv/alloc_engine.cc
src/lib/dhcpsrv/alloc_engine.h
src/lib/dhcpsrv/dhcpsrv_messages.mes
src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc

index cd21aa668762897dfcdbbb1a32a9d44abd51d5ca..a5c4362cbdb0e1f8d3ae89029c94d975c8159d18 100644 (file)
@@ -2073,13 +2073,14 @@ temporarily override a list of interface names and listen on all interfaces.
       that it is no longer allowed to use it by sending DHCPNAK message. Host A
       will then revert to server discovery and will eventually get a different
       address.  The address 192.0.2.10 is now no longer used. When host B tries
-      to renew its temporary address, the server will detect that it has a valid
-      lease, but there is a reservation for a different address. The server will
-      send DHCPNAK to inform host B that its address is no longer usable. The
-      server will also remove its temporary lease. It will revert to the server
-      discovery phase and will eventually send a DHCPREQUEST message. This time the
-      server will find out that there is a reservation for that host and the
-      reserved address 192.0.2.10 is not used, so it will be granted.</para> -->
+      to renew its temporarily assigned  address, the server will detect that
+      it has a valid lease, but there is a reservation for a different address.
+      The server will send DHCPNAK to inform host B that its address is no
+      longer usable. The server will also remove its temporary lease. It will
+      revert to the server discovery phase and will eventually send a
+      DHCPREQUEST message. This time the server will find out that there is a
+      reservation for that host and the reserved address 192.0.2.10 is not used,
+      so it will be granted.</para> -->
 
       <para>When the Host A renews its address, the server will discover that
       the address being renewed is now reserved for another host - the Host
@@ -2092,16 +2093,17 @@ temporarily override a list of interface names and listen on all interfaces.
       discovery and will eventually get a different address. Besides
       allocating a new lease, the server will also remove the old one. As
       a result, the address 192.0.2.10 will be no longer used. When Host B
-      tries to renew its temporary address, the server will detect that it has
-      a valid lease, but there is a reservation for a different address. The
-      server will send DHCPNAK to inform Host B that its address is no longer
-      usable, but will keep its lease (again, the DHCPNAK may be lost, so the
-      server will keep it, until the client returns for a new address). The
-      Host B will revert to the server discovery phase and will eventually
-      send a DHCPREQUEST message. This time the server will find out that
-      there is a reservation for that host and the reserved address
+      tries to renew its temporarily assigned address, the server will detect
+      that it has a valid lease, but there is a reservation for a different
+      address. The server will send DHCPNAK to inform Host B that its address
+      is no longer usable, but will keep its lease (again, the DHCPNAK may be
+      lost, so the server will keep it, until the client returns for a new
+      address). The Host B will revert to the server discovery phase and will
+      eventually send a DHCPREQUEST message. This time the server will find
+      out that there is a reservation for that host and the reserved address
       192.0.2.10 is not used, so it will be granted. It will also remove the
-      lease for the temporary address that the Host B previously obtained.</para>
+      lease for the temporarily assigned address that the Host B previously
+      obtained.</para>
 
       <para>This recovery will succeed, even if other hosts will attempt to get
       the reserved address. Had the Host C requested address 192.0.2.10 after
index 812846367c72d6934270f17a0393bc4be44cfaf1..fa05bca7262a5170d7ec6457ead8732fe458a32a 100644 (file)
@@ -307,10 +307,6 @@ TEST_F(DirectClientTest, renew) {
     ASSERT_NO_THROW(IfaceMgr::instance().openSockets4());
     // Add a subnet.
     ASSERT_NO_FATAL_FAILURE(configureSubnet("10.0.0.0"));
-    // Make sure that the subnet has been really added. Also, the subnet
-    // will be needed to create a lease for a client.
-    Subnet4Ptr subnet = CfgMgr::instance().getCurrentCfg()->
-        getCfgSubnets4()->selectSubnet(IOAddress("10.0.0.10"));
 
     // Create the DHCPv4 client.
     Dhcp4Client client;
@@ -343,10 +339,6 @@ TEST_F(DirectClientTest, rebind) {
     ASSERT_NO_THROW(IfaceMgr::instance().openSockets4());
     // Add a subnet.
     ASSERT_NO_FATAL_FAILURE(configureSubnet("10.0.0.0"));
-    // Make sure that the subnet has been really added. Also, the subnet
-    // will be needed to create a lease for a client.
-    Subnet4Ptr subnet = CfgMgr::instance().getCurrentCfg()->
-        getCfgSubnets4()->selectSubnet(IOAddress("10.0.0.10"));
 
     // Create the DHCPv4 client.
     Dhcp4Client client;
index 591cab61e1f61a356c96cbb85a64de75701b994e..e70635d4af23d30af84145af45215b0ab4f7bfa7 100644 (file)
@@ -634,7 +634,8 @@ TEST_F(DORATest, reservationsWithConflicts) {
                                    IOAddress>(new IOAddress("0.0.0.0"))));
     ASSERT_TRUE(clientB.getContext().response_);
     ASSERT_EQ(DHCPACK, static_cast<int>(clientB.getContext().response_->getType()));
-    ASSERT_NE(clientB.config_.lease_.addr_, in_pool_addr);
+    IOAddress client_b_addr = clientB.config_.lease_.addr_;
+    ASSERT_NE(client_b_addr, in_pool_addr);
 
     // Client A renews the lease.
     client.setState(Dhcp4Client::RENEWING);
@@ -657,6 +658,7 @@ TEST_F(DORATest, reservationsWithConflicts) {
     ASSERT_TRUE(clientB.getContext().response_);
     ASSERT_EQ(DHCPACK, static_cast<int>(clientB.getContext().response_->getType()));
     ASSERT_NE(clientB.config_.lease_.addr_, in_pool_addr);
+    ASSERT_EQ(client_b_addr, clientB.config_.lease_.addr_);
 
     // Client B renews its lease.
     clientB.setState(Dhcp4Client::RENEWING);
@@ -667,6 +669,7 @@ TEST_F(DORATest, reservationsWithConflicts) {
     ASSERT_TRUE(clientB.getContext().response_);
     EXPECT_EQ(DHCPACK, static_cast<int>(clientB.getContext().response_->getType()));
     ASSERT_NE(clientB.config_.lease_.addr_, in_pool_addr);
+    ASSERT_EQ(client_b_addr, clientB.config_.lease_.addr_);
 
     // Client A performs 4-way exchange.
     client.setState(Dhcp4Client::SELECTING);
@@ -681,7 +684,7 @@ TEST_F(DORATest, reservationsWithConflicts) {
     ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
     // The server should have assigned a different address than the one
     // reserved for the Client B.
-    ASSERT_NE(client.config_.lease_.addr_.toText(), in_pool_addr.toText());
+    ASSERT_NE(client.config_.lease_.addr_, in_pool_addr);
     subnet = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->
         selectSubnet(client.config_.lease_.addr_);
     ASSERT_TRUE(subnet);
index ba9723561c61dcbf6be2d152a2da89037e4dfc9b..b6c831fba235c46668abada3f85db52051383a51 100644 (file)
@@ -120,19 +120,34 @@ TEST(IOAddressTest, isV4) {
 }
 
 TEST(IOAddressTest, isV4Zero) {
+    // 0.0.0.0
     const IOAddress address_zero("0.0.0.0");
-    const IOAddress address_non_zero("192.0.2.3");
-
     EXPECT_TRUE(address_zero.isV4Zero());
+    // 192.0.2.3
+    const IOAddress address_non_zero("192.0.2.3");
     EXPECT_FALSE(address_non_zero.isV4Zero());
+    // 0.0.0.100
+    const IOAddress address_non_zero1("0.0.0.100");
+    EXPECT_FALSE(address_non_zero1.isV4Zero());
+    // 64.0.0.0
+    const IOAddress address_non_zero2("64.0.0.0");
+    EXPECT_FALSE(address_non_zero2.isV4Zero());
 }
 
 TEST(IOAddressTest, isV4Bcast) {
+    // 255.255.255.255
     const IOAddress address_bcast("255.255.255.255");
-    const IOAddress address_non_bcast("10.2.3.4");
-
     EXPECT_TRUE(address_bcast.isV4Bcast());
+    // 10.2.3.4
+    const IOAddress address_non_bcast("10.2.3.4");
     EXPECT_FALSE(address_non_bcast.isV4Bcast());
+    // 255.255.255.23
+    const IOAddress address_non_bcast1("255.255.255.23");
+    EXPECT_FALSE(address_non_bcast1.isV4Bcast());
+    // 123.255.255.255
+    const IOAddress address_non_bcast2("123.255.255.255");
+    EXPECT_FALSE(address_non_bcast2.isV4Bcast());
+
 }
 
 TEST(IOAddressTest, isV6) {
@@ -144,11 +159,18 @@ TEST(IOAddressTest, isV6) {
 }
 
 TEST(IOAddressTest, isV6Zero) {
+    // ::
     const IOAddress address_zero("::");
-    const IOAddress address_non_zero("0.0.0.0");
-
     EXPECT_TRUE(address_zero.isV6Zero());
+    // 0.0.0.0
+    const IOAddress address_non_zero("0.0.0.0");
     EXPECT_FALSE(address_non_zero.isV6Zero());
+    // ::ff
+    const IOAddress address_non_zero1("::ff");
+    EXPECT_FALSE(address_non_zero1.isV6Zero());
+    // ff::
+    const IOAddress address_non_zero2("::ff");
+    EXPECT_FALSE(address_non_zero2.isV6Zero());
 }
 
 TEST(IOAddressTest, uint32) {
index 4b22cd75cd97678678fe8f5d89617fcb6789e0c2..193251e86c564ef8a60311dbf71013eabe3c9a99 100644 (file)
@@ -288,6 +288,37 @@ AllocEngine::AllocatorPtr AllocEngine::getAllocator(Lease::Type type) {
 // #    DHCPv6 lease allocation code starts here.
 // ##########################################################################
 
+AllocEngine::ClientContext6::ClientContext6()
+    : subnet_(), duid_(), iaid_(0), type_(Lease::TYPE_NA), hwaddr_(),
+      hints_(), fwd_dns_update_(false), rev_dns_update_(false), hostname_(""),
+      callout_handle_(), fake_allocation_(false), old_leases_(), host_(),
+      query_(), ia_rsp_(), allow_new_leases_in_renewals_(false) {
+}
+
+AllocEngine::ClientContext6::ClientContext6(const Subnet6Ptr& subnet, const DuidPtr& duid,
+                                            const uint32_t iaid,
+                                            const isc::asiolink::IOAddress& hint,
+                                            const Lease::Type type, const bool fwd_dns,
+                                            const bool rev_dns,
+                                            const std::string& hostname,
+                                            const bool fake_allocation):
+    subnet_(subnet), duid_(duid), iaid_(iaid), type_(type), hwaddr_(),
+    hints_(), fwd_dns_update_(fwd_dns), rev_dns_update_(rev_dns),
+    hostname_(hostname), fake_allocation_(fake_allocation),
+    old_leases_(), host_(), query_(), ia_rsp_(),
+    allow_new_leases_in_renewals_(false) {
+
+    static asiolink::IOAddress any("::");
+
+    if (hint != any) {
+        hints_.push_back(std::make_pair(hint, 128));
+    }
+    // callout_handle, host pointers initiated to NULL by their
+    // respective constructors.
+}
+
+
+
 Lease6Collection
 AllocEngine::allocateLeases6(ClientContext6& ctx) {
 
@@ -1177,7 +1208,7 @@ addressReserved(const IOAddress& address, const AllocEngine::ClientContext4& ctx
             return (host_hwaddr->hwaddr_ != ctx.hwaddr_->hwaddr_);
 
         } else {
-            return (true);
+            return (false);
 
         }
     }
@@ -1776,16 +1807,6 @@ AllocEngine::allocateUnreservedLease4(ClientContext4& ctx) {
 void
 AllocEngine::updateLease4Information(const Lease4Ptr& lease,
                                      AllocEngine::ClientContext4& ctx) const {
-    // This should not happen in theory.
-    if (!lease) {
-        isc_throw(BadValue, "null lease specified for updateLease4Information");
-    }
-
-    if (!ctx.subnet_) {
-        isc_throw(BadValue, "null subnet specified for"
-                  " updateLease4Information");
-    }
-
     lease->subnet_id_ = ctx.subnet_->getID();
     lease->hwaddr_ = ctx.hwaddr_;
     lease->client_id_ = ctx.clientid_;
index 3efd0c02b1a45e2d411d1c6a25c26ae3225692ef..28eb812206764c7690b1fecac458cd90466b79b3 100644 (file)
@@ -202,7 +202,7 @@ protected:
                     const isc::asiolink::IOAddress& hint);
     };
 
-    public:
+public:
 
     /// @brief specifies allocation type
     typedef enum {
@@ -391,12 +391,7 @@ public:
         bool allow_new_leases_in_renewals_;
 
         /// @brief Default constructor.
-        ClientContext6()
-           : subnet_(), duid_(), iaid_(0), type_(Lease::TYPE_NA), hwaddr_(),
-             hints_(), fwd_dns_update_(false), rev_dns_update_(false), hostname_(""),
-             callout_handle_(), fake_allocation_(false), old_leases_(), host_(),
-             query_(), ia_rsp_(), allow_new_leases_in_renewals_(false) {
-        }
+        ClientContext6();
 
         /// @brief Constructor with parameters.
         ///
@@ -422,21 +417,7 @@ public:
                        const uint32_t iaid, const isc::asiolink::IOAddress& hint,
                        const Lease::Type type, const bool fwd_dns, const bool
                        rev_dns, const std::string& hostname, const bool
-                       fake_allocation):
-            subnet_(subnet), duid_(duid), iaid_(iaid), type_(type), hwaddr_(),
-            hints_(), fwd_dns_update_(fwd_dns), rev_dns_update_(rev_dns),
-            hostname_(hostname), fake_allocation_(fake_allocation),
-            old_leases_(), host_(), query_(), ia_rsp_(),
-            allow_new_leases_in_renewals_(false){
-
-            static asiolink::IOAddress any("::");
-
-            if (hint != any) {
-                hints_.push_back(std::make_pair(hint, 128));
-            }
-            // callout_handle, host pointers initiated to NULL by their
-            // respective constructors.
-        }
+                       fake_allocation);
     };
 
     /// @brief Allocates IPv6 leases for a given IA container
@@ -853,15 +834,19 @@ public:
     /// dynamic pool or even an address currently allocated for this client.
     ///
     /// It is possible that the address reserved for the particular client
-    /// is in use by another client, e.g. as a result of pools reconfigruation.
+    /// is in use by another client, e.g. as a result of pools reconfiguration.
     /// In this case, when the client requests allocation of the reserved
     /// address and the server determines that it is leased to someone else,
-    /// the allocation engine doesn't allocate a lease for the client having
-    /// a reservation. When the client having a lease returns to renew, the
-    /// allocation engine doesn't extend the lease for it and returns a NULL
-    /// pointer. The client falls back to the 4-way exchange and a different
-    /// lease is allocated. At this point, the reserved address is freed and
-    /// can be allocated to the client which holds this reservation.
+    /// the allocation engine allocates a different address for this client.
+    ///
+    /// When the client having a lease returns to renew, the allocation engine
+    /// doesn't extend the lease for it and returns a NULL pointer. The client
+    /// falls back to the 4-way exchange and a different lease is allocated.
+    /// At this point, the reserved address is freed and can be allocated to
+    /// the client which holds this reservation. However, this client has a
+    /// lease for a different address at this time. When the client renews its
+    /// lease it receives the DHCPNAK and falls back to the DHCP server
+    /// discovery and obtains the lease for the reserved address.
     ///
     /// When a server should do DNS updates, it is required that allocation
     /// returns the information about how the lease was obtained by the allocation
@@ -1096,6 +1081,10 @@ private:
     ///
     /// Note that this doesn't update the lease address.
     ///
+    /// @warning This method doesn't check if the pointer to the lease is
+    /// valid nor if the subnet to the pointer in the @c ctx is valid.
+    /// The caller is responsible for making sure that they are valid.
+    ///
     /// @param [out] lease A pointer to the lease to be updated.
     /// @param ctx A context containing information from the server about the
     /// client and its message.
index 22124127953fd15d5ba333b95be9f7b68a6d919a..3a347ec47dda897bede79168737fa324fceb10d0 100644 (file)
@@ -183,8 +183,7 @@ the new parameters.
 This warning message is issued when the DHCP server finds that the
 address reserved for the client can't be offered because this address
 is currently allocated to another client. The server will try to allocate
-a different (temporary) address to the client to use until the conflict
-is resolved.
+a different address to the client to use until the conflict is resolved.
 
 % DHCPSRV_DHCP_DDNS_ERROR_EXCEPTION error handler for DHCP_DDNS IO generated an expected exception: %1
 This is an error message that occurs when an attempt to send a request to
index 46053aab78a1a4fae1efce0c9498d85cc734c2e2..c81e60a43976564f8bf72e671ccbc3cc9d0d1f48 100644 (file)
@@ -546,37 +546,44 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) {
     EXPECT_TRUE(*old_lease_ == original_lease);
 }
 
-/// @todo write renewLease6
-
+// This test checks that when the client requests the address which belongs
+// to another client, the allocation engine returns NULL (for the
+// DHCPREQUEST case) or a lease for the address which belongs to this
+// client (DHCPDISCOVER case).
 TEST_F(AllocEngine4Test, requestOtherClientLease) {
+    // Create the first lease.
     Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0,
                                100, 30, 60, time(NULL), subnet_->getID(),
                                false, false, ""));
-
+    // Create the second lease.
     Lease4Ptr lease2(new Lease4(IOAddress("192.0.2.102"), hwaddr2_, 0, 0,
                                100, 30, 60, time(NULL), subnet_->getID(),
                                false, false, ""));
-
+    // Add leases for both clients to the Lease Manager.
     LeaseMgrFactory::instance().addLease(lease);
     LeaseMgrFactory::instance().addLease(lease2);
 
     AllocEngine engine(AllocEngine::ALLOC_ITERATIVE, 100, false);
 
+    // First client requests the lease which belongs to the second client.
     Lease4Ptr new_lease = engine.allocateLease4(subnet_, clientid_, hwaddr_,
                                                 IOAddress("192.0.2.102"),
                                                 false, false, "",
                                                 false, CalloutHandlePtr(),
                                                 old_lease_);
-
+    // Allocation engine should return NULL.
     ASSERT_FALSE(new_lease);
 
+    // Now simulate the DHCPDISCOVER case when the provided address is
+    // treated as a hint. The engine should return a lease for a
+    // different address than requested.
     new_lease = engine.allocateLease4(subnet_, clientid_, hwaddr_,
                                       IOAddress("192.0.2.102"),
                                       false, false, "",
                                       true, CalloutHandlePtr(),
                                       old_lease_);
     ASSERT_TRUE(new_lease);
-
+    EXPECT_EQ("192.0.2.101", new_lease->addr_.toText());
 }
 
 // This test checks the behavior of the allocation engine in the following
@@ -880,7 +887,8 @@ TEST_F(AllocEngine4Test, reservedAddressHijackedFakeAllocation) {
                                                       false, false, "",
                                                       true, CalloutHandlePtr(),
                                                       old_lease_);
-    // The allocation engine should return no lease.
+    // The allocation engine should return a lease but for a different address
+    // than requested because this address is in use.
     ASSERT_TRUE(allocated_lease);
     EXPECT_NE(allocated_lease->addr_.toText(), "192.0.2.123");
     EXPECT_TRUE(subnet_->inPool(Lease::TYPE_V4, allocated_lease->addr_));
@@ -1074,8 +1082,8 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHint) {
 // - Client has a lease for a different address than reserved.
 // - Client sends a DHCPDISCOVER with no hint.
 // - Server determines that there is a reservation for the client and that
-// the current lease should be removed and the reserved address should be
-// allocated.
+//   the reserved address should be offered when the client sends a
+//   DHCPDISCOVER.
 TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHintFakeAllocation) {
     // Create a reservation.
     HostPtr host(new Host(&hwaddr_->hwaddr_[0], hwaddr_->hwaddr_.size(),
@@ -1113,6 +1121,8 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHintFakeAllocation) {
     Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
     ASSERT_TRUE(from_mgr);
     detailCompareLease(lease, from_mgr);
+
+
 }
 
 // This test checks that the behavior of the allocation engine in the following
@@ -1242,6 +1252,10 @@ TEST_F(AllocEngine4Test, reservedAddressVsDynamicPool) {
                                                       old_lease_);
     ASSERT_TRUE(allocated_lease);
     EXPECT_NE(allocated_lease->addr_.toText(), "192.0.2.100");
+
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(allocated_lease->addr_);
+    ASSERT_TRUE(from_mgr);
+    detailCompareLease(allocated_lease, from_mgr);
 }
 
 // This test checks that the client requesting an address which is