From: Thomas Markwalder Date: Mon, 27 Aug 2018 17:23:17 +0000 (-0400) Subject: [#13,!6] Addressed review comments X-Git-Tag: gitlab20_base~17^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=91f8ca94f3f5b2232d31512e95b56560f26b8960;p=thirdparty%2Fkea.git [#13,!6] Addressed review comments Mostly corrections additions to commentary. --- diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index 3d44dec23a..ef57889451 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -3275,7 +3275,7 @@ should include options from the isc option space: specific subnet. Kea will still match inbound client packets to a subnet as before, but when the subnet's reservation mode is set to "global", Kea will look for host reservations only - among the global reservations defined. Typcially, such resrvations would + among the global reservations defined. Typcially, such reservations would be used to reserve hostnames for clients which may move from one subnet to another. @@ -3709,17 +3709,17 @@ If not specified, the default value is: This feature can be used to assign certain parameters, such as hostname or other dedicated, host-specific options. It can also be used to - assign addresses or prefixes. However, global reservations that assign - either of these bypass the whole topology determination provided by DHCP - logic implemented in Kea. It is very easy to misuse this feature and get - configuration that is inconsistent. To give a specific example, imagine a - global reservation for an address 2001:db8:1111::1 and two subnets - 2001:db8:1111::/64 and 2001:db8:ffff::/48. If global reservations are used - in both subnets and a device matching global host reservations visits part - of the network that is covered by 2001:db8:ffff::/48, it will get an IP - address 2001:db8:ffff::/48, which will be outside of the prefix announced + assign addresses or prefixes. However, global reservations that assign + either of these bypass the whole topology determination provided by DHCP + logic implemented in Kea. It is very easy to misuse this feature and get + configuration that is inconsistent. To give a specific example, imagine a + global reservation for an address 2001:db8:1111::1 and two subnets + 2001:db8:1111::/48 and 2001:db8:ffff::/48. If global reservations are used + in both subnets and a device matching global host reservations visits part + of the network that is covered by 2001:db8:ffff::/48, it will get an IP + address 2001:db8:ffff::1, which will be outside of the prefix announced by its local router using Router Advertisements. Such a configuration - would be unsuable or at the very least ridden with issues, such as the + would be unsuable or at the very least ridden with issues, such as the downlink traffic not reaching the device. @@ -3741,7 +3741,7 @@ If not specified, the default value is: "hostname": "hw-host-fixed", // Use of IP address is global reservation is risky. If used outside of - // matching subnet, such as 2001:db8:1::/64, it will result in a broken + // matching subnet, such as 3001::/64, it will result in a broken // configuration being handled to the client. "ip-address": "2001:db8:ff::77" }, diff --git a/src/bin/dhcp6/tests/host_unittest.cc b/src/bin/dhcp6/tests/host_unittest.cc index 0940dde2fc..b3d9f33c3f 100644 --- a/src/bin/dhcp6/tests/host_unittest.cc +++ b/src/bin/dhcp6/tests/host_unittest.cc @@ -1180,8 +1180,8 @@ HostTest::sarrTest(Dhcp6Client& client, const std::string& exp_address, Lease6 lease_client = client.getLease(0); EXPECT_EQ(exp_address, lease_client.addr_.toText()); - // Check that the server recorded the lease. - // and that the server lease has NO hostname + // Check that the server recorded the lease + // and that the server lease has expected hostname. Lease6Ptr lease_server = checkLease(lease_client); ASSERT_TRUE(lease_server); EXPECT_EQ(exp_hostname, lease_server->hostname_); diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 39aaabf604..95f2e0d025 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1252,7 +1252,7 @@ AllocEngine::allocateGlobalReservedLeases6(ClientContext6& ctx, return (false); } - // We want to avoid allocating new lease for an IA if there is already + // We want to avoid allocating a new lease for an IA if there is already // a valid lease for which client has reservation. So, we first check if // we already have a lease for a reserved address or prefix. BOOST_FOREACH(const Lease6Ptr& lease, existing_leases) { @@ -1266,6 +1266,7 @@ AllocEngine::allocateGlobalReservedLeases6(ClientContext6& ctx, .arg(lease->typeToText(lease->type_)) .arg(lease->addr_.toText()); + // Besides IP reservations we're also going to return other reserved // parameters, such as hostname. We want to hand out the hostname value // from the same reservation entry as IP addresses. Thus, let's see if // there is any hostname reservation. diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index b806af2285..e32e208f93 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -488,7 +488,7 @@ public: /// /// If the current subnet's reservation mode is global and /// there is a global host (i.e. reservation belonging to - /// the global subnet), return it. Otherwise return an + /// the global subnet), return it. Otherwise return an /// empty pointer. /// /// @return Pointer to the host object. @@ -783,7 +783,7 @@ public: if (lease.type_ == Lease::TYPE_NA) { return (IPv6Resrv(IPv6Resrv::TYPE_NA, lease.addr_, (lease.prefixlen_ ? lease.prefixlen_ : 128))); - } + } return (IPv6Resrv(IPv6Resrv::TYPE_PD, lease.addr_, lease.prefixlen_)); } @@ -842,16 +842,33 @@ private: /// @brief Creates new leases based on reservations. /// - /// This method allocates new leases, based on host reservation. Existing - /// leases are specified in existing_leases parameter. A new lease is not created, - /// if there is a lease for specified address on existing_leases list or there is - /// a lease used by someone else. + /// This method allcoates new leases, based on host reservations. + /// Existing leases are specified in the existing_leases parameter. + /// It first calls @c allocateGlobalReservedLeases6 to accomodate + /// subnets using global reservations. If that method allocates + /// addresses, we return, otherwise we continue and check for non-global + /// reservations. A new lease is not created, if there is a lease for + /// specified address on existing_leases list or there is a lease used by + /// someone else. /// /// @param ctx client context that contains all details (subnet, client-id, etc.) /// @param existing_leases leases that are already associated with the client void allocateReservedLeases6(ClientContext6& ctx, Lease6Collection& existing_leases); + /// @brief Creates new leases based on global reservations. + /// + /// This method is used by @allocateReservedLeases6, to allocate new leases based + /// on global reservation if one exists and global reservations are enabled for + /// the selected subnet. It differs from it's caller by looking only at the global + /// reservation and therefore has no need to iterate over the selected subnet or it's + /// siblings looking for host reservations. Like it's caller, existing leases are + /// specified in existing_leases parameter. A new lease is not created, if there is + /// a lease for specified address on existing_leases list or there is a lease used by + /// someone else. + /// + /// @param ctx client context that contains all details (subnet, client-id, etc.) + /// @param existing_leases leases that are already associated with the client bool allocateGlobalReservedLeases6(ClientContext6& ctx, Lease6Collection& existing_leases);