From: Thomas Markwalder Date: Fri, 16 Sep 2016 18:27:46 +0000 (-0400) Subject: [5007] Addressed review comments X-Git-Tag: trac5049_base~15^2~1 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=1a986cf434453bdf6ccbcef5d707a23a23305093;p=thirdparty%2Fkea.git [5007] Addressed review comments This is solution #2: src/lib/dhcpsrv/alloc_engine.h src/lib/dhcpsrv/alloc_engine.cc AllocEngine::extendLease6() - replaced logic to set the context flags with simply adding the original lease to the changed_leases_ list. AllocEngine::updateLeaseData() - removed logic to set the context flags. src/bin/dhcp6/dhcp6_srv.cc Dhcpv6Srv::createNameChangeRequests() - replaced the context flag check with logic which looks for candidate IA addresses in the ctx.changed_leases_ list. If found and the FDQN doman name has not changed, we move on to the next candidate. src/bin/dhcp6/tests/fqdn_unittest.cc TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoAddr) - removed testing of context flag permutations --- diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index db76d15ee4..1f12e4e80d 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1198,13 +1198,8 @@ Dhcpv6Srv::processClientFqdn(const Pkt6Ptr& question, const Pkt6Ptr& answer, void Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, AllocEngine::ClientContext6& ctx) { - // Don't create NameChangeRequests if DNS updates are disabled - // or if no requests are actually required. The context flags may be - // different than the lease flags. Primarily when existing leases - // are being extended without FQDN changes in which case the context - // flags will both be false. - if ((!CfgMgr::instance().ddnsEnabled()) || - (!ctx.fwd_dns_update_ && !ctx.rev_dns_update_)) { + // Don't create NameChangeRequests if DNS updates are disabled. + if (!CfgMgr::instance().ddnsEnabled()) { return; } @@ -1274,6 +1269,25 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, if (!iaaddr) { continue; } + + // see if the lease for iaadr is in changed_leases, and if so + // if the FQDN is different, if not continue + 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()) { + 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). diff --git a/src/bin/dhcp6/tests/fqdn_unittest.cc b/src/bin/dhcp6/tests/fqdn_unittest.cc index cbbd4ef308..9fb951b65a 100644 --- a/src/bin/dhcp6/tests/fqdn_unittest.cc +++ b/src/bin/dhcp6/tests/fqdn_unittest.cc @@ -758,9 +758,7 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoAddr) { } // Test that exactly one NameChangeRequest is created as a result of processing -// the answer message which holds 3 IAs and when FQDN is specified. We also -// verify that the context flags, fwd_dns_update_ and rev_dns_update_, gate -// whether or not a NameChangeRequest is created. +// the answer message which holds 3 IAs and when FQDN is specified. TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) { // Create Reply message with Client Id and Server id. Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY); @@ -790,33 +788,6 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) { "000201415AA33D1187D148275136FA30300478" "FAAAA3EBD29826B5C907B2C9268A6F52", 0, 500); - - // If context flags are false we should not create the NCR. - ctx.fwd_dns_update_ = ctx.rev_dns_update_ = false; - ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx)); - ASSERT_EQ(0, d2_mgr_.getQueueSize()); - - // If only the forward flag is true, we create the NCR. - ctx.fwd_dns_update_ = true; - ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx)); - // Verify that NameChangeRequest is correct. - verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, - "2001:db8:1::1", - "000201415AA33D1187D148275136FA30300478" - "FAAAA3EBD29826B5C907B2C9268A6F52", - 0, 500); - ASSERT_EQ(0, d2_mgr_.getQueueSize()); - - // If only the reverse flag is true, we create the NCR. - ctx.fwd_dns_update_ = false; - ctx.rev_dns_update_ = true; - ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx)); - // Verify that NameChangeRequest is correct. - verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, - "2001:db8:1::1", - "000201415AA33D1187D148275136FA30300478" - "FAAAA3EBD29826B5C907B2C9268A6F52", - 0, 500); } // Checks that NameChangeRequests to add entries are not diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index b7c682f0d0..9463f8cf92 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1386,15 +1386,9 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) { // 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 { - // Callers should use the context flags to drive whether or - // not we do an update, not the lease flags. Changing those - // would alter the flags in the database, which we want to - // preserve them as they were were when the lease was created. - ctx.fwd_dns_update_ = false; - ctx.rev_dns_update_ = false; } } + // Now that the lease has been reclaimed, we can go ahead and update it // in the lease database. LeaseMgrFactory::instance().updateLease6(lease); @@ -1406,8 +1400,13 @@ 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; @@ -1438,14 +1437,6 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases updated_leases.push_back(lease); } - // If we didn't do a remove, then the FQDN is unchanged so this amounts - // to renew. Note this assumes this method is only called when client - // issues a request, rather than a renew or rebind. - if (!remove_queued) { - ctx.fwd_dns_update_ = false; - ctx.rev_dns_update_ = false; - } - return (updated_leases); } diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 383807fee4..c28775666c 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -856,11 +856,6 @@ private: /// - client's last transmission time (cltt), if the lease to be returned /// to the client should have its lifetime extended, /// - FQDN data, when the client has negotiated new FQDN with the server. - /// - If the FQDN data has not changed, the DNS update flags in the - /// context, fwd_dns_update_ and rev_dns_update_, are set false. This - /// should indicate to callers that DDNS updates should not be done. - /// The lease flags are left intact to indicate their state when the - /// lease was assigned. /// /// @param ctx IPv6 client context (old versions of the leases that had /// FQDN data changed will be stored in ctx.changed_leases_, @@ -887,12 +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. - /// If the FQDN data has not changed, the DNS update flags in the - /// context, fwd_dns_update_ and rev_dns_update_, are set false. This - /// should indicate to callers that DDNS updates should not be done. - /// The lease flags are left intact to indicate their state when the - /// lease was assigned. + /// 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.