From: Thomas Markwalder Date: Thu, 15 Sep 2016 20:10:53 +0000 (-0400) Subject: [5007] Suppress DDNS updates on DHCPv6 lease renewals unless the FQDN changes X-Git-Tag: trac5049_base~15^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=46f4c9fdd841b3cb26afb925b9386fbe3534e89d;p=thirdparty%2Fkea.git [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 --- 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.