From: Thomas Markwalder <tmark@isc.org>
Date: Wed, 18 Mar 2015 19:16:09 +0000 (-0400)
Subject: [3689] Addressed primary review comments
X-Git-Tag: kea-0.9.1~8^2~5
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3e09b74567d827942a1e81777fce39b9dd7593bb;p=thirdparty%2Fkea.git

[3689] Addressed primary review comments

doc/guide/dhcp6-srv.xml
    Updated "Reserving a hostname" section
    (also removed trailing spaces)

src/bin/dhcp6/dhcp6_srv.cc
   corrected test and commentary typo in assignIA_NA
   removed second parameter to renewLeases6() calls

src/bin/dhcp6/dhcp6_srv.h
    updated commentary for createContext()

src/lib/dhcpsrv/alloc_engine.cc
src/lib/dhcpsrv/alloc_engine.h
    AllocEngine::createLease6()
        removed find_host parameter and logic which calls
        findReservation and/or alters context hostname

src/lib/dhcpsrv/tests/alloc_engine_utils.cc
    AllocEngine6Test::renewTest() - added call to
    AllocEngine6Test::findReservation()

src/lib/dhcpsrv/tests/alloc_engine_utils.h
    fixed typo
---

diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml
index b0b7fd7b6d..7cdd49eb7d 100644
--- a/doc/guide/dhcp6-srv.xml
+++ b/doc/guide/dhcp6-srv.xml
@@ -1178,7 +1178,7 @@ should include options from the isc option space:
     "data" is not set the parser will assume that this parameter is not
     specified and an error will be reported.
     </para>
-    
+
     <para>Note that it is possible to create an option which carries some data
     in addition to the sub-options defined in the encapsulated option space.
     For example, if the "container" option from the previous example was
@@ -1728,8 +1728,8 @@ should include options from the isc option space:
       The third row in the table above describes the case in which the client
       requests that no DNS updates be done. The parameter, "override-no-update",
       can be used to instruct the server to disregard the client's wishes. When
-      this parameter is true, kea-dhcp6 will generate DDNS update requests to 
-      kea-dhcp-ddns even if the client requests no updates be done.  The N-S-O 
+      this parameter is true, kea-dhcp6 will generate DDNS update requests to
+      kea-dhcp-ddns even if the client requests no updates be done.  The N-S-O
       flags in the server's response to the client will be 0-1-1.
       </para>
       <para>
@@ -2031,11 +2031,64 @@ should include options from the isc option space:
 
     <section id="reservation6-hostname">
       <title>Reserving a hostname</title>
-      <!-- @todo: replace this with the actual text once #3689 is implemented -->
-      <para>Reserving a hostname is currently not supported. It is possible
-      to specify that information in the configuration file, but that data
-      is not used by the server engine yet.</para>
-    </section>
+      <para>When the reservation for the client includes the <command>hostname
+      </command>, the server will assign this hostname to the client and send
+      it back in the Client FQDN, if the client sent the FQDN option to the
+      the server. The reserved hostname always takes precedence over the hostname
+       supplied by the client (via the FQDN option) or the autogenerated
+      (from the IPv6 address) hostname.</para>
+
+      <para>The server qualifies the reserved hostname with the value
+      of the <command>qualifying-suffix</command> parameter. For example, the
+      following subnet configuration:
+<screen>
+"subnet6": [
+    {
+        "subnet": "2001:db8:1::/48",
+        "pools": [ { "pool": "2001:db8:1::/80" } ],
+        "reservations": [
+            {
+                "duid": "01:02:03:04:05:0A:0B:0C:0D:0E",
+                "ip-addresses": [ "2001:db8:1::100" ]
+                "hostname": "alice-laptop"
+            }
+        ]
+    }
+],
+"dhcp-ddns": {
+    "enable-updates": true,
+    "qualifying-suffix": "example.isc.org."
+}
+</screen>
+      will result in assigning the "alice-laptop.example.isc.org." hostname to the
+      client using the DUID "01:02:03:04:05:0A:0B:0C:0D:0E". If the <command>qualifying-suffix
+      </command> is not specified, the default (empty) value will be used, and
+      in this case the value specified as a <command>hostname</command> will
+      be treated as fully qualified name.  Thus, by leaving the
+      <command>qualifying-suffix</command> empty it is possible to qualify
+      hostnames for the different clients with different domain names:
+<screen>
+"subnet6": [
+    {
+        "subnet": "2001:db8:1::/48",
+        "pools": [ { "pool": "2001:db8:1::/80" } ],
+        "reservations": [
+            {
+                "duid": "01:02:03:04:05:0A:0B:0C:0D:0E",
+                "ip-addresses": [ "2001:db8:1::100" ]
+                "hostname": "mark-desktop.example.org."
+            }
+        ]
+    }
+],
+"dhcp-ddns": {
+    "enable-updates": true,
+}
+</screen>
+      will result in assigning the "mark-desktip.example.org." hostname to the
+      client using the DUID "01:02:03:04:05:0A:0B:0C:0D:0E".
+    </para>
+</section>
 
     <section id="reservation6-options">
       <title>Reserving specific options</title>
@@ -2110,7 +2163,7 @@ should include options from the isc option space:
     ]
 }
 </screen>
-      </para>        
+      </para>
    </section>
 
     <!-- @todo: add support for per IA reservation (that specifies IAID in
@@ -2270,7 +2323,7 @@ should include options from the isc option space:
             "relay": {
                 "ip-address": "3000::1"
             }</userinput>
-        },	    
+        },
 
         {
             "subnet": "2001:db8:1::/64",
diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 3f2c0e47b9..2c192ebd91 100644
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -224,7 +224,8 @@ Dhcpv6Srv::testUnicast(const Pkt6Ptr& pkt) const {
     return (true);
 }
 
-DuidPtr Dhcpv6Srv::extractClientId(const Pkt6Ptr& pkt) {
+DuidPtr
+Dhcpv6Srv::extractClientId(const Pkt6Ptr& pkt) {
     // Let's find client's DUID. Client is supposed to include its client-id
     // option almost all the time (the only exception is an anonymous inf-request,
     // but that is mostly a theoretical case). Our allocation engine needs DUID
@@ -237,7 +238,8 @@ DuidPtr Dhcpv6Srv::extractClientId(const Pkt6Ptr& pkt) {
     }
 }
 
-AllocEngine::ClientContext6 Dhcpv6Srv::createContext(const Pkt6Ptr& pkt) {
+AllocEngine::ClientContext6
+Dhcpv6Srv::createContext(const Pkt6Ptr& pkt) {
     AllocEngine::ClientContext6 ctx;
     ctx.subnet_ = selectSubnet(pkt);
     ctx.duid_ = extractClientId(pkt);
@@ -1271,8 +1273,8 @@ Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer,
     // to say that we are sorry, but the user won't get an address. As a convenience, we
     // use a different status text to indicate that (compare to the same status code,
     // but different wording below)
-    if (!orig_ctx.subnet_) {
-        // Creatasse empty IA_NA option with IAID matching the request.
+    if (!subnet) {
+        // Create an empty IA_NA option with IAID matching the request.
         // Note that we don't use OptionDefinition class to create this option.
         // This is because we prefer using a constructor of Option6IA that
         // initializes IAID. Otherwise we would have to use setIAID() after
@@ -1613,7 +1615,7 @@ Dhcpv6Srv::extendIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer,
         ctx.allow_new_leases_in_renewals_ = true;
     }
 
-    Lease6Collection leases = alloc_engine_->renewLeases6(ctx, false);
+    Lease6Collection leases = alloc_engine_->renewLeases6(ctx);
 
     // Ok, now we have the leases extended. We have:
     // - what the client tried to renew in ctx.hints_
@@ -1796,7 +1798,7 @@ Dhcpv6Srv::extendIA_PD(const Pkt6Ptr& query,
     // - old_leases - leases that used to be, but are no longer valid
     // - changed_leases - leases that have FQDN changed (not really important
     //                    in PD context)
-    Lease6Collection leases = alloc_engine_->renewLeases6(ctx, false);
+    Lease6Collection leases = alloc_engine_->renewLeases6(ctx);
 
     // For all the leases we have now, add the IAPPREFIX with non-zero lifetimes
     for (Lease6Collection::const_iterator l = leases.begin(); l != leases.end(); ++l) {
diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h
index 52fd23a3de..86b63b85b8 100644
--- a/src/bin/dhcp6/dhcp6_srv.h
+++ b/src/bin/dhcp6/dhcp6_srv.h
@@ -649,8 +649,14 @@ protected:
 
     /// @brief Creates client context for specified packet
     ///
-    /// Creates context that includes subnet, client-id, hw address and
-    /// possibly other parameters.
+    /// Instantiates the ClientContext6 and then:
+    /// - Performs the subnet selection and stores the result in context
+    /// - Extracts the duid from the packet and saves it to the context
+    /// - Extracts the hardware address from the packet and saves it to
+    /// the context
+    /// - Performs host reservation lookup and stores the result in the
+    /// context
+    ///
     /// @return client context
     AllocEngine::ClientContext6 createContext(const Pkt6Ptr& pkt);
 
diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index dc5f7a70f1..a58eed28e2 100644
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -1006,7 +1006,7 @@ Lease6Ptr AllocEngine::createLease6(ClientContext6& ctx,
 }
 
 Lease6Collection
-AllocEngine::renewLeases6(ClientContext6& ctx, bool find_host) {
+AllocEngine::renewLeases6(ClientContext6& ctx) {
     try {
         if (!ctx.subnet_) {
             isc_throw(InvalidOperation, "Subnet is required for allocation");
@@ -1016,21 +1016,6 @@ AllocEngine::renewLeases6(ClientContext6& ctx, bool find_host) {
             isc_throw(InvalidOperation, "DUID is mandatory for allocation");
         }
 
-        // We can attempt to find appropriate reservation
-        if (find_host) {
-            findReservation(ctx);
-        }
-
-        // Let's check whether there's a hostname specified in the reservation
-        if (ctx.host_) {
-            std::string hostname = ctx.host_->getHostname();
-            // If there is, let's use it
-            if (!hostname.empty()) {
-                ctx.hostname_ = hostname;
-            }
-        }
-
-
         // Check if there are any leases for this client.
         Lease6Collection leases = LeaseMgrFactory::instance()
             .getLeases6(ctx.type_, *ctx.duid_, ctx.iaid_, ctx.subnet_->getID());
diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h
index bb6835b575..bdda4b8c5f 100644
--- a/src/lib/dhcpsrv/alloc_engine.h
+++ b/src/lib/dhcpsrv/alloc_engine.h
@@ -498,16 +498,8 @@ public:
     /// prefixes the client had sent. @ref ClientContext6::old_leases_ will
     /// contain removed leases in this case.
     ///
-    /// @param find_resrv specifies whether the code should search for host
-    ///   reservation. true means that the code will consult HostMgr, false means
-    ///   to skip this check. That is easier to use, but is redundant if the
-    ///   ctx.host_ field is already set. We can't use ctx.host_ == NULL as
-    ///   check, because for cases whithout reservations, the reservation
-    ///   search would be repeated.
-    ///
-    ///
     /// @return Returns renewed lease.
-    Lease6Collection renewLeases6(ClientContext6& ctx, bool find_resrv = true);
+    Lease6Collection renewLeases6(ClientContext6& ctx);
 
     /// @brief Attempts to find appropriate host reservation.
     ///
diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc
index 38b07611a2..f0ff369ab6 100644
--- a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc
@@ -230,6 +230,7 @@ AllocEngine6Test::renewTest(AllocEngine& engine, const Pool6Ptr& pool,
     ctx.query_.reset(new Pkt6(DHCPV6_RENEW, 123));
     ctx.allow_new_leases_in_renewals_ = allow_new_leases_in_renewal;
 
+    findReservation(engine, ctx);
     Lease6Collection leases = engine.renewLeases6(ctx);
 
     for (Lease6Collection::iterator it = leases.begin(); it != leases.end(); ++it) {
diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h
index 0c2a6f82aa..80f159dad5 100644
--- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h
+++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.h
@@ -113,7 +113,7 @@ public:
 
     /// @brief Wrapper around call to AllocEngine6::findRervation
     ///
-    /// If a reservation is found bg the engine, the function sets
+    /// If a reservation is found by the engine, the function sets
     /// ctx.hostname_ accordingly.
     ///
     /// @param engine allocation engine to use