From: Thomas Markwalder Date: Tue, 30 Apr 2019 14:48:39 +0000 (-0400) Subject: [#557,!310] Fixed kea-dhcp6 DNS updates on renewals with generated FQDNs X-Git-Tag: Kea-1.6.0-beta~156 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=66fc02e105ac8200c39914a18dc6ef1ecf7a0bbd;p=thirdparty%2Fkea.git [#557,!310] Fixed kea-dhcp6 DNS updates on renewals with generated FQDNs Moved logic for detecting FQDN changes from the alloc engine to the kea-dhcp6 server proper. This allows the change detection to be done after FQDN generation. src/bin/dhcp6/dhcp6_srv.* Dhcpv6Srv::createNameChangeRequests() - modified to detect when changes to a valid lease require a DNS remove Dhcpv6Srv::generateFqdn() - updated to store the generated name in the current context src/lib/dhcpsrv/alloc_engine.cc AllocEngine::extendLease6() AllocEngine::updateLeaseData() - removed logic that checks for FQDN changes src/bin/dhcp6/dhcp6_messages.* Revamped a message --- diff --git a/src/bin/dhcp6/dhcp6_messages.cc b/src/bin/dhcp6/dhcp6_messages.cc index 1bd1c73272..65004a8db2 100644 --- a/src/bin/dhcp6/dhcp6_messages.cc +++ b/src/bin/dhcp6/dhcp6_messages.cc @@ -1,4 +1,4 @@ -// File created from ../../../src/bin/dhcp6/dhcp6_messages.mes on Tue Mar 26 2019 13:41 +// File created from ../../../src/bin/dhcp6/dhcp6_messages.mes on Tue Apr 30 2019 10:41 #include #include @@ -37,8 +37,8 @@ extern const isc::log::MessageID DHCP6_DDNS_CREATE_ADD_NAME_CHANGE_REQUEST = "DH extern const isc::log::MessageID DHCP6_DDNS_FQDN_GENERATED = "DHCP6_DDNS_FQDN_GENERATED"; extern const isc::log::MessageID DHCP6_DDNS_GENERATED_FQDN_UPDATE_FAIL = "DHCP6_DDNS_GENERATED_FQDN_UPDATE_FAIL"; extern const isc::log::MessageID DHCP6_DDNS_GENERATE_FQDN = "DHCP6_DDNS_GENERATE_FQDN"; -extern const isc::log::MessageID DHCP6_DDNS_LEASE_RENEW_FQDN_CHANGE = "DHCP6_DDNS_LEASE_RENEW_FQDN_CHANGE"; extern const isc::log::MessageID DHCP6_DDNS_RECEIVE_FQDN = "DHCP6_DDNS_RECEIVE_FQDN"; +extern const isc::log::MessageID DHCP6_DDNS_REMOVE_OLD_LEASE_FQDN = "DHCP6_DDNS_REMOVE_OLD_LEASE_FQDN"; extern const isc::log::MessageID DHCP6_DDNS_REQUEST_SEND_FAILED = "DHCP6_DDNS_REQUEST_SEND_FAILED"; extern const isc::log::MessageID DHCP6_DDNS_RESPONSE_FQDN_DATA = "DHCP6_DDNS_RESPONSE_FQDN_DATA"; extern const isc::log::MessageID DHCP6_DDNS_SEND_FQDN = "DHCP6_DDNS_SEND_FQDN"; @@ -177,8 +177,8 @@ const char* values[] = { "DHCP6_DDNS_FQDN_GENERATED", "%1: generated FQDN for the client: %2", "DHCP6_DDNS_GENERATED_FQDN_UPDATE_FAIL", "%1: failed to update the lease using address %2, after generating FQDN for a client, reason: %3", "DHCP6_DDNS_GENERATE_FQDN", "%1: client did not send a FQDN option; FQDN will be", - "DHCP6_DDNS_LEASE_RENEW_FQDN_CHANGE", "FQDN %1: FQDN for the renewed lease: %2 has changed. New values: hostname = %3, reverse mapping = %4, forward mapping = %5", "DHCP6_DDNS_RECEIVE_FQDN", "%1: received DHCPv6 Client FQDN option: %2", + "DHCP6_DDNS_REMOVE_OLD_LEASE_FQDN", "%1: FQDN for a lease: %2 has changed. New values: hostname = %3, reverse mapping = %4, forward mapping = %5", "DHCP6_DDNS_REQUEST_SEND_FAILED", "failed sending a request to kea-dhcp-ddns, error: %1, ncr: %2", "DHCP6_DDNS_RESPONSE_FQDN_DATA", "%1: including FQDN option in the server's response: %2", "DHCP6_DDNS_SEND_FQDN", "sending DHCPv6 Client FQDN Option to the client: %1", diff --git a/src/bin/dhcp6/dhcp6_messages.h b/src/bin/dhcp6/dhcp6_messages.h index 19f22199fc..298d7d0bfe 100644 --- a/src/bin/dhcp6/dhcp6_messages.h +++ b/src/bin/dhcp6/dhcp6_messages.h @@ -1,4 +1,4 @@ -// File created from ../../../src/bin/dhcp6/dhcp6_messages.mes on Tue Mar 26 2019 13:41 +// File created from ../../../src/bin/dhcp6/dhcp6_messages.mes on Tue Apr 30 2019 10:41 #ifndef DHCP6_MESSAGES_H #define DHCP6_MESSAGES_H @@ -38,8 +38,8 @@ extern const isc::log::MessageID DHCP6_DDNS_CREATE_ADD_NAME_CHANGE_REQUEST; extern const isc::log::MessageID DHCP6_DDNS_FQDN_GENERATED; extern const isc::log::MessageID DHCP6_DDNS_GENERATED_FQDN_UPDATE_FAIL; extern const isc::log::MessageID DHCP6_DDNS_GENERATE_FQDN; -extern const isc::log::MessageID DHCP6_DDNS_LEASE_RENEW_FQDN_CHANGE; extern const isc::log::MessageID DHCP6_DDNS_RECEIVE_FQDN; +extern const isc::log::MessageID DHCP6_DDNS_REMOVE_OLD_LEASE_FQDN; extern const isc::log::MessageID DHCP6_DDNS_REQUEST_SEND_FAILED; extern const isc::log::MessageID DHCP6_DDNS_RESPONSE_FQDN_DATA; extern const isc::log::MessageID DHCP6_DDNS_SEND_FQDN; diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index 964f4bfff4..53bb696a77 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -1,4 +1,4 @@ -# Copyright (C) 2012-2018 Internet Systems Consortium, Inc. ("ISC") +# Copyright (C) 2012-2019 Internet Systems Consortium, Inc. ("ISC") # # This Source Code Form is subject to the terms of the Mozilla Public # License, v. 2.0. If a copy of the MPL was not distributed with this @@ -180,13 +180,14 @@ This debug message is issued when the server did not receive a FQDN option from the client and client name replacement is enabled. This provides a means to create DNS entries for unsophisticated clients. -% DHCP6_DDNS_LEASE_RENEW_FQDN_CHANGE FQDN %1: FQDN for the renewed lease: %2 has changed. New values: hostname = %3, reverse mapping = %4, forward mapping = %5 -This debug message is logged when FQDN mapping for a particular lease has been -changed by the recent Renew message. This mapping will be changed in DNS. -The first argument includes the client and the transaction identification -information. The second argument holds the details about the lease for which -the FQDN information and/or mappings have changed. The remaining arguments -hold the new FQDN information and flags for mappings. +% DHCP6_DDNS_REMOVE_OLD_LEASE_FQDN %1: FQDN for a lease: %2 has changed. New values: hostname = %3, reverse mapping = %4, forward mapping = %5 +This debug message is logged during lease renewal when an old lease that is +no longer being offered has a different FQDN than the renewing lesae. Thus +the old DNS entries need to be removed. The first argument includes the client +and the transaction identification information. The second argument holds the +details about the lease for which the FQDN information and/or mappings have +changed. The remaining arguments hold the new FQDN information and flags for +mappings. % DHCP6_DDNS_RECEIVE_FQDN %1: received DHCPv6 Client FQDN option: %2 This debug message is logged when server has found the DHCPv6 Client FQDN Option diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 34c7e5a4d1..607ca76406 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1637,12 +1637,6 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, bool do_rev = false; CfgMgr::instance().getD2ClientMgr().getUpdateDirections(*opt_fqdn, do_fwd, do_rev); - if (!do_fwd && !do_rev) { - // Flags indicate there is Nothing to do, get out now. - // The Most likely scenario is that we are honoring the client's - // request that no updates be done. - return; - } // Get the Client Id. It is mandatory and a function creating a response // would have thrown an exception if it was missing. Thus throwing @@ -1668,13 +1662,12 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, // Get all IAs from the answer. For each IA, holding an address we will // create a corresponding NameChangeRequest. - OptionCollection answer_ias = answer->getOptions(D6O_IA_NA); - for (OptionCollection::const_iterator answer_ia = - answer_ias.begin(); answer_ia != answer_ias.end(); ++answer_ia) { + for (auto answer_ia : answer->getOptions(D6O_IA_NA)) { /// @todo IA_NA may contain multiple addresses. We should process /// each address individually. Currently we get only one. Option6IAAddrPtr iaaddr = boost::static_pointer_cast< - Option6IAAddr>(answer_ia->second->getOption(D6O_IAADDR)); + Option6IAAddr>(answer_ia.second->getOption(D6O_IAADDR)); + // We need an address to create a name-to-address mapping. // If address is missing for any reason, go to the next IA. if (!iaaddr) { @@ -1682,22 +1675,36 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, } // 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. + // to determine if the changes included changes to the FQDN. If so + // then we may need to do a CHG_REMOVE. 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()) { + // The address is the same so this must be renewal? if ((*l)->hostname_ == opt_fqdn->getDomainName() && (*l)->fqdn_fwd_ == do_fwd && (*l)->fqdn_rev_ == do_rev) { + // The FQDN is the same, it must be an extension only. + // @todo - If we decide to allow updates on renews, we + // will need to bypass this. extended_only = true; - break; + } else { + // The FQDN has changed, queue a CHG_REMOVE of the old data. + // NCR will only be created if the lease hostname is not + // empty and at least one of the direction flags is true + queueNCR(CHG_REMOVE, *l); } + + break; } } - if (extended_only) { + if (!(do_fwd || do_rev) || (extended_only)) { + // Flags indicate no updates needed or it was an extension of + // an existing lease with no FQDN changes. In the case of the + // former, the most likely scenario is that we are honoring the + // client's request that no updates be done. continue; } @@ -2115,7 +2122,7 @@ Dhcpv6Srv::extendIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer, if (((*l)->hostname_ != ctx.hostname_) || ((*l)->fqdn_fwd_ != do_fwd) || ((*l)->fqdn_rev_ != do_rev)) { LOG_DEBUG(ddns6_logger, DBG_DHCP6_DETAIL, - DHCP6_DDNS_LEASE_RENEW_FQDN_CHANGE) + DHCP6_DDNS_REMOVE_OLD_LEASE_FQDN) .arg(query->getLabel()) .arg((*l)->toText()) .arg(ctx.hostname_) @@ -2850,7 +2857,7 @@ Dhcpv6Srv::processRequest(AllocEngine::ClientContext6& ctx) { appendRequestedVendorOptions(request, reply, ctx, co_list); updateReservedFqdn(ctx, reply); - generateFqdn(reply); + generateFqdn(reply, ctx); createNameChangeRequests(reply, ctx); return (reply); @@ -2876,7 +2883,7 @@ Dhcpv6Srv::processRenew(AllocEngine::ClientContext6& ctx) { appendRequestedVendorOptions(renew, reply, ctx, co_list); updateReservedFqdn(ctx, reply); - generateFqdn(reply); + generateFqdn(reply, ctx); createNameChangeRequests(reply, ctx); return (reply); @@ -2902,7 +2909,7 @@ Dhcpv6Srv::processRebind(AllocEngine::ClientContext6& ctx) { appendRequestedVendorOptions(rebind, reply, ctx, co_list); updateReservedFqdn(ctx, reply); - generateFqdn(reply); + generateFqdn(reply, ctx); createNameChangeRequests(reply, ctx); return (reply); @@ -3599,7 +3606,8 @@ Dhcpv6Srv::updateReservedFqdn(const AllocEngine::ClientContext6& ctx, } void -Dhcpv6Srv::generateFqdn(const Pkt6Ptr& answer) { +Dhcpv6Srv::generateFqdn(const Pkt6Ptr& answer, + AllocEngine::ClientContext6& ctx) { if (!answer) { isc_throw(isc::Unexpected, "an instance of the object encapsulating" " a message must not be NULL when generating FQDN"); @@ -3664,7 +3672,7 @@ Dhcpv6Srv::generateFqdn(const Pkt6Ptr& answer) { answer->delOption(D6O_CLIENT_FQDN); answer->addOption(fqdn); - + ctx.hostname_ = generated_name; } catch (const Exception& ex) { LOG_ERROR(ddns6_logger, DHCP6_DDNS_GENERATED_FQDN_UPDATE_FAIL) .arg(answer->getLabel()) diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 63e9b4369d..e421832d03 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -653,9 +653,11 @@ protected: /// The FQDN option carries response to the client about DNS updates that /// server intends to perform for the DNS client. Based on this, the /// function will create zero or more @c isc::dhcp_ddns::NameChangeRequest - /// objects and store them in the internal queue. Requests created by this - /// function are only for adding or updating DNS records. If DNS updates are - /// disabled, this method returns immediately. + /// objects and store them in the internal queue. To catch lease renewals + /// that alter the FQDN, the function first looks at the context's changed + /// list of leases (if any) to determine if DNS entries need to be removed. + /// It then looks at the valid leases to determine if any DNS entries need + /// to be added. If DNS updates are disabled, this method returns immediately. /// /// @todo Add support for multiple IAADDR options in the IA_NA. /// @@ -969,10 +971,12 @@ private: /// /// @param answer Message being sent to a client, which may hold IA_NA /// and Client FQDN options to be used to generate name for a client. + /// @param ctx Client context. /// /// @throw isc::Unexpected if specified message is NULL. This is treated /// as a programmatic error. - void generateFqdn(const Pkt6Ptr& answer); + void generateFqdn(const Pkt6Ptr& answer, + AllocEngine::ClientContext6& ctx); /// @brief Updates statistics for received packets /// @param query packet received diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 0929f39a01..4895aee795 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -2017,12 +2017,6 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) { "assigned-nas" : "assigned-pds"), static_cast(1)); } - } 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 @@ -2046,7 +2040,6 @@ 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)); @@ -2077,13 +2070,6 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases 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; - } } }