[master] kea-dhcp6 now only does DDNS on renewals if FQDN changes

    Merges in branch 'trac5007'

src/bin/dhcp6/dhcp6_srv.cc

@@ -1198,7 +1198,8 @@ Dhcpv6Srv::processClientFqdn(const Pkt6Ptr& question, const Pkt6Ptr& answer,
 }
 
 void
-Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer) {
+Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer,
+                                    AllocEngine::ClientContext6& ctx) {
     // Don't create NameChangeRequests if DNS updates are disabled.
     if (!CfgMgr::instance().ddnsEnabled()) {
         return;
@@ -1270,6 +1271,27 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer) {
         if (!iaaddr) {
             continue;
         }
+
+        // If the lease for iaaddr is in the list of changed leases, we need
+        // to determine if the changes included changes to the FQDN. If there
+        // were  changes to the FQDN then we need to update DNS, otherwise
+        // we do not.
+        bool extended_only = false;
+        for (Lease6Collection::const_iterator l = ctx.currentIA().changed_leases_.begin();
+             l != ctx.currentIA().changed_leases_.end(); ++l) {
+            if ((*l)->addr_ == iaaddr->getAddress()) {
+                if ((*l)->hostname_ == opt_fqdn->getDomainName() &&
+                    (*l)->fqdn_fwd_ == do_fwd && (*l)->fqdn_rev_ == do_rev) {
+                    extended_only = true;
+                    break;
+                }
+            }
+        }
+
+        if (extended_only) {
+            continue;
+        }
+
         // Create new NameChangeRequest. Use the domain name from the FQDN.
         // This is an FQDN included in the response to the client, so it
         // holds a fully qualified domain-name already (not partial).
@@ -2307,7 +2329,7 @@ Dhcpv6Srv::processSolicit(const Pkt6Ptr& solicit) {
     // Only generate name change requests if sending a Reply as a result
     // of receiving Rapid Commit option.
     if (response->getType() == DHCPV6_REPLY) {
-        createNameChangeRequests(response);
+        createNameChangeRequests(response, ctx);
     }
 
     return (response);
@@ -2335,7 +2357,7 @@ Dhcpv6Srv::processRequest(const Pkt6Ptr& request) {
     processClientFqdn(request, reply, ctx);
     assignLeases(request, reply, ctx);
     generateFqdn(reply);
-    createNameChangeRequests(reply);
+    createNameChangeRequests(reply, ctx);
 
     return (reply);
 }
@@ -2362,7 +2384,7 @@ Dhcpv6Srv::processRenew(const Pkt6Ptr& renew) {
     processClientFqdn(renew, reply, ctx);
     extendLeases(renew, reply, ctx);
     generateFqdn(reply);
-    createNameChangeRequests(reply);
+    createNameChangeRequests(reply, ctx);
 
     return (reply);
 }
@@ -2389,7 +2411,7 @@ Dhcpv6Srv::processRebind(const Pkt6Ptr& rebind) {
     processClientFqdn(rebind, reply, ctx);
     extendLeases(rebind, reply, ctx);
     generateFqdn(reply);
-    createNameChangeRequests(rebind);
+    createNameChangeRequests(reply, ctx);
 
     return (reply);
 }

src/bin/dhcp6/dhcp6_srv.h

@@ -567,8 +567,10 @@ protected:
     /// @todo Add support for multiple IAADDR options in the IA_NA.
     ///
     /// @param answer A message begins sent to the Client. If it holds the
+    /// @param ctx client context (contains subnet, duid and other parameters)
     /// Client FQDN option, this option is used to create NameChangeRequests.
-    void createNameChangeRequests(const Pkt6Ptr& answer);
+    void createNameChangeRequests(const Pkt6Ptr& answer,
+                                  AllocEngine::ClientContext6& ctx);
 
     /// @brief Attempts to extend the lifetime of IAs.
     ///

src/bin/dhcp6/tests/fqdn_unittest.cc

@@ -364,8 +364,12 @@ public:
         }
 
         if (create_ncr_check) {
+            // Context flags are normally set during lease allocation. Since that
+            // hasn't occurred we'll set them here to match the expected values.
             // Call createNameChangeRequests
-            ASSERT_NO_THROW(srv_->createNameChangeRequests(answer));
+            ctx.fwd_dns_update_ =  exp_fwd.value_;
+            ctx.rev_dns_update_ = exp_rev.value_;
+            ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
             if (exp_fwd.value_ || exp_rev.value_) {
                 // Should have created 1 NCR.
                 NameChangeRequestPtr ncr;
@@ -697,7 +701,9 @@ TEST_F(FqdnDhcpv6SrvTest, clientAAAAUpdateNotAllowed) {
 TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoAnswer) {
     Pkt6Ptr answer;
 
-    EXPECT_THROW(srv_->createNameChangeRequests(answer),
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    EXPECT_THROW(srv_->createNameChangeRequests(answer, ctx),
                  isc::Unexpected);
 
 }
@@ -711,7 +717,9 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoDUID) {
                                                  Option6ClientFqdn::FULL);
     answer->addOption(fqdn);
 
-    EXPECT_THROW(srv_->createNameChangeRequests(answer), isc::Unexpected);
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    EXPECT_THROW(srv_->createNameChangeRequests(answer, ctx), isc::Unexpected);
 
 }
 
@@ -721,7 +729,9 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoFQDN) {
     // Create Reply message with Client Id and Server id.
     Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY);
 
-    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer));
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
 
     // There should be no new NameChangeRequests.
     ASSERT_EQ(0, d2_mgr_.getQueueSize());
@@ -738,8 +748,9 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoAddr) {
                                                  "myhost.example.com",
                                                  Option6ClientFqdn::FULL);
     answer->addOption(fqdn);
-
-    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer));
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
 
     // We didn't add any IAs, so there should be no NameChangeRequests in the
     // queue.
@@ -766,7 +777,9 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) {
     answer->addOption(fqdn);
 
     // Create NameChangeRequest for the first allocated address.
-    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer));
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
     ASSERT_EQ(1, d2_mgr_.getQueueSize());
 
     // Verify that NameChangeRequest is correct.
@@ -775,7 +788,6 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) {
                             "000201415AA33D1187D148275136FA30300478"
                             "FAAAA3EBD29826B5C907B2C9268A6F52",
                             0, 500);
-
 }
 
 // Checks that NameChangeRequests to add entries are not
@@ -799,10 +811,11 @@ TEST_F(FqdnDhcpv6SrvTest, noAddRequestsWhenDisabled) {
     answer->addOption(fqdn);
 
     // An attempt to send a NCR would throw.
-    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer));
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
 }
 
-
 // Test creation of the NameChangeRequest to remove both forward and reverse
 // mapping for the given lease.
 TEST_F(FqdnDhcpv6SrvTest, createRemovalNameChangeRequestFwdRev) {
@@ -918,20 +931,26 @@ TEST_F(FqdnDhcpv6SrvTest, processSolicit) {
 // a different domain-name. Server should use existing lease for the second
 // request but modify the DNS entries for the lease according to the contents
 // of the FQDN sent in the second request.
-/// @todo: Fix will be available on trac3677
-TEST_F(FqdnDhcpv6SrvTest, DISABLED_processTwoRequests) {
+TEST_F(FqdnDhcpv6SrvTest, processTwoRequestsDiffFqdn) {
     // Create a Request message with FQDN option and generate server's
     // response using processRequest function. This will result in the
     // creation of a new lease and the appropriate NameChangeRequest
     // to add both reverse and forward mapping to DNS.
     testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
                        "myhost.example.com.");
+
+    // The lease should have been recorded in the database.
+    lease_ = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA,
+                                                   IOAddress("2001:db8:1:1::dead:beef"));
+    ASSERT_TRUE(lease_);
+
     ASSERT_EQ(1, d2_mgr_.getQueueSize());
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             "2001:db8:1:1::dead:beef",
                             "000201415AA33D1187D148275136FA30300478"
                             "FAAAA3EBD29826B5C907B2C9268A6F52",
-                            lease_->cltt_ + lease_->valid_lft_, 4000);
+                            0, 4000);
+
 
     // Client may send another request message with a new domain-name. In this
     // case the same lease will be returned. The existing DNS entry needs to
@@ -947,7 +966,7 @@ TEST_F(FqdnDhcpv6SrvTest, DISABLED_processTwoRequests) {
                             "2001:db8:1:1::dead:beef",
                             "000201415AA33D1187D148275136FA30300478"
                             "FAAAA3EBD29826B5C907B2C9268A6F52",
-                            0, 4000);
+                            lease_->cltt_ + lease_->valid_lft_, 4000);
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             "2001:db8:1:1::dead:beef",
                             "000201D422AA463306223D269B6CB7AFE7AAD265FC"
@@ -956,6 +975,38 @@ TEST_F(FqdnDhcpv6SrvTest, DISABLED_processTwoRequests) {
 
 }
 
+// Test that client may send two requests, each carrying FQDN option with
+// the same domain-name. Server should use existing lease for the second
+// request and not modify the DNS entries.
+TEST_F(FqdnDhcpv6SrvTest, processTwoRequestsSameFqdn) {
+    // Create a Request message with FQDN option and generate server's
+    // response using processRequest function. This will result in the
+    // creation of a new lease and the appropriate NameChangeRequest
+    // to add both reverse and forward mapping to DNS.
+    testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
+                       "myhost.example.com.");
+
+    // The lease should have been recorded in the database.
+    lease_ = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA,
+                                                   IOAddress("2001:db8:1:1::dead:beef"));
+    ASSERT_TRUE(lease_);
+
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            "2001:db8:1:1::dead:beef",
+                            "000201415AA33D1187D148275136FA30300478"
+                            "FAAAA3EBD29826B5C907B2C9268A6F52",
+                            0, 4000);
+
+
+    // Client may send another request message with a same domain-name. In this
+    // case the same lease will be returned. The existing DNS entry should be
+    // left alone, so we expect no NameChangeRequests queued..
+    testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
+                       "myhost.example.com.");
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+}
+
 // Test that NameChangeRequest is not generated when Solicit message is sent.
 // The Solicit is here sent after a lease has been allocated for a client.
 // The Solicit conveys a different hostname which would trigger updates to
@@ -992,13 +1043,18 @@ TEST_F(FqdnDhcpv6SrvTest, processRequestSolicit) {
 // DNS entry added previously when Request was processed, another one to
 // add a new entry for the FQDN held in the Renew.
 /// @todo: Fix will be available on trac3677
-TEST_F(FqdnDhcpv6SrvTest, DISABLED_processRequestRenew) {
+TEST_F(FqdnDhcpv6SrvTest, processRequestRenewDiffFqdn) {
     // Create a Request message with FQDN option and generate server's
     // response using processRequest function. This will result in the
     // creation of a new lease and the appropriate NameChangeRequest
     // to add both reverse and forward mapping to DNS.
     testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
                        "myhost.example.com.");
+    // The lease should have been recorded in the database.
+    lease_ = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA,
+                                                   IOAddress("2001:db8:1:1::dead:beef"));
+    ASSERT_TRUE(lease_);
+
     ASSERT_EQ(1, d2_mgr_.getQueueSize());
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             "2001:db8:1:1::dead:beef",
@@ -1020,7 +1076,7 @@ TEST_F(FqdnDhcpv6SrvTest, DISABLED_processRequestRenew) {
                             "2001:db8:1:1::dead:beef",
                             "000201415AA33D1187D148275136FA30300478"
                             "FAAAA3EBD29826B5C907B2C9268A6F52",
-                            0, 4000);
+                            lease_->cltt_ + lease_->valid_lft_, 4000);
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             "2001:db8:1:1::dead:beef",
                             "000201D422AA463306223D269B6CB7AFE7AAD265FC"
@@ -1029,6 +1085,112 @@ TEST_F(FqdnDhcpv6SrvTest, DISABLED_processRequestRenew) {
 
 }
 
+// Test that client may send Request followed by the Renew, both holding
+// FQDN options, but each option holding different domain-name. The Renew
+// should result in generation of the two NameChangeRequests, one to remove
+// DNS entry added previously when Request was processed, another one to
+// add a new entry for the FQDN held in the Renew.
+TEST_F(FqdnDhcpv6SrvTest, processRequestRenewSameFqdn) {
+    // Create a Request message with FQDN option and generate server's
+    // response using processRequest function. This will result in the
+    // creation of a new lease and the appropriate NameChangeRequest
+    // to add both reverse and forward mapping to DNS.
+    testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
+                       "myhost.example.com.");
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            "2001:db8:1:1::dead:beef",
+                            "000201415AA33D1187D148275136FA30300478"
+                            "FAAAA3EBD29826B5C907B2C9268A6F52",
+                            0, 4000);
+
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+
+    // Client may send Renew message with a same domain-name. In this
+    // case the same lease will be returned. No DNS updates should be
+    // required, so the NCR queue should be empty.
+    testProcessMessage(DHCPV6_RENEW, "myhost.example.com",
+                       "myhost.example.com.");
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+}
+
+// Tests that renewals using the same domain name but differing values for
+// the directional update flags result in NCRs or not, accordingly.
+// If the new leases's flags are the same as the previous lease's flags,
+// then no requests should be generated.  If at lease one of the new lease's
+// flags differ from the previous lease, then:
+// A: A removal NCR should be created based on the previous leases's flags
+// if at least one of those flags are true
+// B: An add NCR should be created based on the new lease's flags, if at
+// least one of those flags are true
+TEST_F(FqdnDhcpv6SrvTest, processRequestRenewFqdnFlags) {
+    // Create a Request message with FQDN option but with N flag = 1, which
+    // means no updates should be done. This should result in no NCRs.
+    testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
+                       "myhost.example.com.", Option6ClientFqdn::FLAG_N);
+    // Queue should be empty.
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+
+    // Now renew with Both N and S = 0.  This means the server should only
+    // do reverse updates and should result in a reverse-only NCR.
+    testProcessMessage(DHCPV6_RENEW, "myhost.example.com",
+                       "myhost.example.com.", 0);
+    // We should a only have reverse-only ADD, no remove.
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, false,
+                            "2001:db8:1:1::dead:beef",
+                            "000201415AA33D1187D148275136FA30300478"
+                            "FAAAA3EBD29826B5C907B2C9268A6F52",
+                            0, 4000);
+
+    // Renew again with the same flags, this should not generate any NCRs.
+    testProcessMessage(DHCPV6_RENEW, "myhost.example.com",
+                       "myhost.example.com.", 0);
+    // Queue should be empty.
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+
+
+    // Renew with both N and S flags = 0. This tells the server to update
+    // both directions, which should change forward flag to true. This should
+    // generate a reverse only remove and a dual add.
+    testProcessMessage(DHCPV6_RENEW, "myhost.example.com",
+                       "myhost.example.com.", Option6ClientFqdn::FLAG_S);
+
+    // We need the lease for the expiration value.
+    lease_ = LeaseMgrFactory::
+             instance().getLease6(Lease::TYPE_NA,
+                                  IOAddress("2001:db8:1:1::dead:beef"));
+    ASSERT_TRUE(lease_);
+
+    // We should have two NCRs, one remove and one add.
+    ASSERT_EQ(2, d2_mgr_.getQueueSize());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, false,
+                            "2001:db8:1:1::dead:beef",
+                            "000201415AA33D1187D148275136FA30300478"
+                            "FAAAA3EBD29826B5C907B2C9268A6F52",
+                            lease_->cltt_ + lease_->valid_lft_, 4000);
+
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            "2001:db8:1:1::dead:beef",
+                            "000201415AA33D1187D148275136FA30300478"
+                            "FAAAA3EBD29826B5C907B2C9268A6F52",
+                            0, 4000);
+
+    // Lastly, we renew with the N flag = 1 (which means no updates) so we
+    // should have a dual direction remove NCR but NO add NCR.
+    testProcessMessage(DHCPV6_RENEW, "myhost.example.com",
+                       "myhost.example.com.", Option6ClientFqdn::FLAG_N);
+    // We should only have the removal NCR.
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true,
+                            "2001:db8:1:1::dead:beef",
+                            "000201415AA33D1187D148275136FA30300478"
+                            "FAAAA3EBD29826B5C907B2C9268A6F52",
+                            lease_->cltt_ + lease_->valid_lft_, 4000);
+
+}
+
+
 TEST_F(FqdnDhcpv6SrvTest, processRequestRelease) {
     // Create a Request message with FQDN option and generate server's
     // response using processRequest function. This will result in the

src/lib/dhcpsrv/alloc_engine.cc

@@ -410,7 +410,6 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) {
                                                    *ctx.duid_,
                                                    ctx.currentIA().iaid_,
                                                    ctx.subnet_->getID());
-
         // Now do the checks:
         // Case 1. if there are no leases, and there are reservations...
         //   1.1. are the reserved addresses are used by someone else?
@@ -1382,11 +1381,14 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) {
         if (old_data->expired()) {
             reclaimExpiredLease(old_data, ctx.callout_handle_);
 
-        } else  if (!lease->hasIdenticalFqdn(*old_data)) {
-            // We're not reclaiming the lease but since the FQDN has changed
-            // we have to at least send NCR.
-            queueNCR(CHG_REMOVE, old_data);
+        } else {
+            if (!lease->hasIdenticalFqdn(*old_data)) {
+                // We're not reclaiming the lease but since the FQDN has changed
+                // we have to at least send NCR.
+                queueNCR(CHG_REMOVE, old_data);
+            }
         }
+
         // Now that the lease has been reclaimed, we can go ahead and update it
         // in the lease database.
         LeaseMgrFactory::instance().updateLease6(lease);
@@ -1398,27 +1400,43 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) {
         // fields of returned Lease6Ptr, the actual updateLease6() is no-op.
         *lease = *old_data;
     }
+
+    // Add the old lease to the changed lease list. This allows the server
+    // to make decisions regarding DNS updates.
+    ctx.currentIA().changed_leases_.push_back(old_data);
 }
 
+
 Lease6Collection
 AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases) {
     Lease6Collection updated_leases;
+    bool remove_queued = false;
     for (Lease6Collection::const_iterator lease_it = leases.begin();
          lease_it != leases.end(); ++lease_it) {
         Lease6Ptr lease(new Lease6(**lease_it));
         lease->fqdn_fwd_ = ctx.fwd_dns_update_;
         lease->fqdn_rev_ = ctx.rev_dns_update_;
         lease->hostname_ = ctx.hostname_;
-        if (!ctx.fake_allocation_ &&
-            (conditionalExtendLifetime(*lease) ||
-             (lease->fqdn_fwd_ != (*lease_it)->fqdn_fwd_) ||
-             (lease->fqdn_rev_ != (*lease_it)->fqdn_rev_) ||
-             (lease->hostname_ != (*lease_it)->hostname_))) {
-            ctx.currentIA().changed_leases_.push_back(*lease_it);
-            LeaseMgrFactory::instance().updateLease6(lease);
+        if (!ctx.fake_allocation_) {
+            bool fqdn_changed = ((lease->type_ != Lease::TYPE_PD) &&
+                                 !(lease->hasIdenticalFqdn(**lease_it)));
+
+            if (conditionalExtendLifetime(*lease) || fqdn_changed) {
+                ctx.currentIA().changed_leases_.push_back(*lease_it);
+                LeaseMgrFactory::instance().updateLease6(lease);
+
+                // If the FQDN differs, remove existing DNS entries.
+                // We only need one remove.
+                if (fqdn_changed && !remove_queued) {
+                    queueNCR(CHG_REMOVE, *lease_it);
+                    remove_queued = true;
+                }
+            }
         }
+
         updated_leases.push_back(lease);
     }
+
     return (updated_leases);
 }
 

src/lib/dhcpsrv/alloc_engine.h

@@ -882,7 +882,9 @@ private:
     /// This method attempts to extend the lease. It will call the lease6_renew
     /// or lease6_rebind hooks (depending on the client's message specified in
     /// ctx.query). The lease will be extended in LeaseMgr, unless the hooks
-    /// library will set the skip flag.
+    /// library will set the skip flag.  The old lease is added to the
+    /// the context's changed_leases_ list which allows the server to make
+    /// decisions regarding DNS updates.
     ///
     /// @param ctx client context that passes all necessary information. See
     ///        @ref ClientContext6 for details.