diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 9ad93a1387..201511eef9 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/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); } diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 183d3a8ff6..4f1975a23c 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..d645995f9d 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. @@ -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 diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 1f8d9d4e3d..9463f8cf92 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,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); } diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 69ac652f04..c28775666c 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/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.