From 46f4c9fdd841b3cb26afb925b9386fbe3534e89d Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 15 Sep 2016 16:10:53 -0400 Subject: [PATCH 1/3] [5007] Suppress DDNS updates on DHCPv6 lease renewals unless the FQDN changes src/lib/dhcpsrv/alloc_engine.h src/lib/dhcpsrv/alloc_engine.cc - AllocEngine::extendLease6() - AllocEngine::updateLeaseData() - logic was added to clear the context DNS update flags when the renewal does not alter the lease's FQDN. src/bin/dhcp6/dhcp6_srv.h src/bin/dhcp6/dhcp6_srv.cc - Dhcpv6Srv::createNameChangeRequests() - added context as second parameter, and modified function to return without creating NCR(s) if both update flags in the context are false. src/bin/dhcp6/tests/fqdn_unittest.cc - TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) - modified to verify combinations of context update flags - TEST_F(FqdnDhcpv6SrvTest, DISABLED_processTwoRequests) - renamed to TEST_F(FqdnDhcpv6SrvTest, processTwoRequestsDiffFqdn) and enabled. It had been disabled pending 3677 which has been completed. - TEST_F(FqdnDhcpv6SrvTest, processTwoRequestsSameFqdn) - new test which verifies that client "renewing" a lease by sending a second request with the different FQDN generates the correct NCRs - TEST_F(FqdnDhcpv6SrvTest, DISABLED_processRequestRenew) - renamed to TEST_F(FqdnDhcpv6SrvTest, processRequestRenewDiffFqdn) and enabled. It had been disabled pending 3677 which has been completed. - TEST_F(FqdnDhcpv6SrvTest, processRequestRenewSameFqdn) - new test which verifies that client renewing a lease by sending a renew with the same FQDN does NOT generate any NCRs --- src/bin/dhcp6/dhcp6_srv.cc | 20 ++-- src/bin/dhcp6/dhcp6_srv.h | 4 +- src/bin/dhcp6/tests/fqdn_unittest.cc | 146 ++++++++++++++++++++++++--- src/lib/dhcpsrv/alloc_engine.cc | 51 +++++++--- src/lib/dhcpsrv/alloc_engine.h | 10 ++ 5 files changed, 195 insertions(+), 36 deletions(-) diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 58893bb15a..db76d15ee4 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1196,9 +1196,15 @@ Dhcpv6Srv::processClientFqdn(const Pkt6Ptr& question, const Pkt6Ptr& answer, } void -Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer) { - // Don't create NameChangeRequests if DNS updates are disabled. - if (!CfgMgr::instance().ddnsEnabled()) { +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_)) { return; } @@ -2304,7 +2310,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); @@ -2331,7 +2337,7 @@ Dhcpv6Srv::processRequest(const Pkt6Ptr& request) { processClientFqdn(request, reply, ctx); assignLeases(request, reply, ctx); generateFqdn(reply); - createNameChangeRequests(reply); + createNameChangeRequests(reply, ctx); return (reply); } @@ -2357,7 +2363,7 @@ Dhcpv6Srv::processRenew(const Pkt6Ptr& renew) { processClientFqdn(renew, reply, ctx); extendLeases(renew, reply, ctx); generateFqdn(reply); - createNameChangeRequests(reply); + createNameChangeRequests(reply, ctx); return (reply); } @@ -2383,7 +2389,7 @@ Dhcpv6Srv::processRebind(const Pkt6Ptr& rebind) { processClientFqdn(rebind, reply, ctx); extendLeases(rebind, reply, ctx); generateFqdn(reply); - createNameChangeRequests(rebind); + createNameChangeRequests(reply, ctx); return (reply); } diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 7513f909bf..9005913c8a 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/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. /// diff --git a/src/bin/dhcp6/tests/fqdn_unittest.cc b/src/bin/dhcp6/tests/fqdn_unittest.cc index ea5bb5d777..cbbd4ef308 100644 --- a/src/bin/dhcp6/tests/fqdn_unittest.cc +++ b/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. @@ -747,7 +758,9 @@ 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. +// 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. TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) { // Create Reply message with Client Id and Server id. Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY); @@ -766,7 +779,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. @@ -776,6 +791,32 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) { "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 @@ -799,10 +840,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 +960,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 +995,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 +1004,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 +1072,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 +1105,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 +1114,35 @@ 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()); +} + TEST_F(FqdnDhcpv6SrvTest, processRequestRelease) { // Create a Request message with FQDN option and generate server's // response using processRequest function. This will result in the diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 1f8d9d4e3d..b7c682f0d0 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/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,10 +1381,19 @@ 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); + } 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. @@ -1403,22 +1411,41 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) { 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); } + + // 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 69ac652f04..383807fee4 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -856,6 +856,11 @@ 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_, @@ -883,6 +888,11 @@ private: /// 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. /// /// @param ctx client context that passes all necessary information. See /// @ref ClientContext6 for details. From 1a986cf434453bdf6ccbcef5d707a23a23305093 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 16 Sep 2016 14:27:46 -0400 Subject: [PATCH 2/3] [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 --- src/bin/dhcp6/dhcp6_srv.cc | 28 ++++++++++++++++++------- src/bin/dhcp6/tests/fqdn_unittest.cc | 31 +--------------------------- src/lib/dhcpsrv/alloc_engine.cc | 21 ++++++------------- src/lib/dhcpsrv/alloc_engine.h | 14 +++---------- 4 files changed, 31 insertions(+), 63 deletions(-) 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. From 6d14c6bdd7011e05b1e211ca0752e20be4c5adfa Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 19 Sep 2016 09:08:54 -0400 Subject: [PATCH 3/3] [5007] Addressed more review comments src/bin/dhcp6/dhcp6_srv.cc Dhcpv6Srv::createNameChangeRequests() - modified logic DNS-updated needed logic to include changes to the FQDN flags. src/bin/dhcp6/tests/fqdn_unittest.cc TEST_F(FqdnDhcpv6SrvTest, processRequestRenewFqdnFlags) - new test to verify the permutations of DNS direction flags on NCR generation --- src/bin/dhcp6/dhcp6_srv.cc | 10 ++-- src/bin/dhcp6/tests/fqdn_unittest.cc | 77 ++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 1f12e4e80d..f8d7571a6a 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1270,13 +1270,16 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, continue; } - // see if the lease for iaadr is in changed_leases, and if so - // if the FQDN is different, if not 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()) { + if ((*l)->hostname_ == opt_fqdn->getDomainName() && + (*l)->fqdn_fwd_ == do_fwd && (*l)->fqdn_rev_ == do_rev) { extended_only = true; break; } @@ -1287,7 +1290,6 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, 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 9fb951b65a..d645995f9d 100644 --- a/src/bin/dhcp6/tests/fqdn_unittest.cc +++ b/src/bin/dhcp6/tests/fqdn_unittest.cc @@ -1114,6 +1114,83 @@ TEST_F(FqdnDhcpv6SrvTest, processRequestRenewSameFqdn) { 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