From: Thomas Markwalder Date: Fri, 11 Sep 2020 20:11:38 +0000 (-0400) Subject: [#1389] V6 uses DDNS parameters from correct subnet X-Git-Tag: Kea-1.9.0~50 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=025c2e7f7bbb2850485d71a0b0c0899d1233646c;p=thirdparty%2Fkea.git [#1389] V6 uses DDNS parameters from correct subnet src/bin/dhcp6/dhcp6_srv.cc Dhcpv6Srv::checkDynamicSubnetChange() - new method to detect and account for dynamic subnet changes. Dhcpv6Srv::assignLeases() - replaced logic to log dynamic subnet change with call to checkDynamicSubnetChange() Dhcpv6Srv::extendLeases() - added call to checkDynamicSubnetChange() src/bin/dhcp6/tests/dhcp6_client.cc src/bin/dhcp6/tests/dhcp6_client.cc Dhcp6Client::doRenew() - relocated appendFQDN() so it actually has an effect. src/bin/dhcp6/tests/fqdn_unittest.cc TEST_F(FqdnDhcpv6SrvTest, ddnsSharedNetworkTest) new test --- diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 2b0caf36f5..e708183c9e 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1701,8 +1701,8 @@ Dhcpv6Srv::selectSubnet(const Pkt6Ptr& question, bool& drop) { void Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer, AllocEngine::ClientContext6& ctx) { - - Subnet6Ptr subnet = ctx.subnet_; + // Save the originally selected subnet. + Subnet6Ptr orig_subnet = ctx.subnet_; // We need to allocate addresses for all IA_NA options in the client's // question (i.e. SOLICIT or REQUEST) message. @@ -1743,22 +1743,11 @@ Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer, } } - // Subnet may be modified by the allocation engine, if the initial subnet - // belongs to a shared network. - if (ctx.subnet_ && subnet && (subnet->getID() != ctx.subnet_->getID())) { - SharedNetwork6Ptr network; - subnet->getSharedNetwork(network); - if (network) { - LOG_DEBUG(packet6_logger, DBG_DHCP6_BASIC_DATA, DHCP6_SUBNET_DYNAMICALLY_CHANGED) - .arg(question->getLabel()) - .arg(subnet->toText()) - .arg(ctx.subnet_->toText()) - .arg(network->getName()); - } - } + // Subnet may be modified by the allocation engine, there are things + // we need to do when that happens. + checkDynamicSubnetChange(question, answer, ctx, orig_subnet); } - void Dhcpv6Srv::processClientFqdn(const Pkt6Ptr& question, const Pkt6Ptr& answer, AllocEngine::ClientContext6& ctx) { @@ -2607,6 +2596,9 @@ Dhcpv6Srv::extendLeases(const Pkt6Ptr& query, Pkt6Ptr& reply, // DUID. There is no need to check for the presence of the DUID here // because we have already checked it in the sanityCheck(). + // Save the originally selected subnet. + Subnet6Ptr orig_subnet = ctx.subnet_; + for (OptionCollection::iterator opt = query->options_.begin(); opt != query->options_.end(); ++opt) { switch (opt->second->getType()) { @@ -2634,6 +2626,10 @@ Dhcpv6Srv::extendLeases(const Pkt6Ptr& query, Pkt6Ptr& reply, break; } } + + // Subnet may be modified by the allocation engine, there are things + // we need to do when that happens. + checkDynamicSubnetChange(query, reply, ctx, orig_subnet); } void @@ -4240,5 +4236,57 @@ Dhcpv6Srv::setTeeTimes(uint32_t preferred_lft, const Subnet6Ptr& subnet, Option6 } } +void +Dhcpv6Srv::checkDynamicSubnetChange(const Pkt6Ptr& question, Pkt6Ptr& answer, + AllocEngine::ClientContext6& ctx, + const Subnet6Ptr orig_subnet) { + // If the subnet's are the same there's nothing to do. + if ((!ctx.subnet_) || (!orig_subnet) || (orig_subnet->getID() == ctx.subnet_->getID())) { + return; + } + + // We get the network for logging only. It should always be set as this a dynamic + // change should only happen within shared-networks. Not having one might not be + // an error if a hook changed the subnet? + SharedNetwork6Ptr network; + orig_subnet->getSharedNetwork(network); + LOG_DEBUG(packet6_logger, DBG_DHCP6_BASIC_DATA, DHCP6_SUBNET_DYNAMICALLY_CHANGED) + .arg(question->getLabel()) + .arg(orig_subnet->toText()) + .arg(ctx.subnet_->toText()) + .arg(network ? network->getName() : ""); + + // The DDNS parameters may have changed with the subnet, so we need to + // recalculate the client name. + + // Save the current DNS values on the context. + std::string prev_hostname = ctx.hostname_; + bool prev_fwd_dns_update = ctx.fwd_dns_update_; + bool prev_rev_dns_update = ctx.rev_dns_update_; + + // Remove the current FQDN option from the answer. + answer->delOption(D6O_CLIENT_FQDN); + + // Recalculate the client's FQDN. This will replace the FQDN option and + // update the context values for hostname_ and DNS directions. + processClientFqdn(question, answer, ctx); + + // If this is a real allocation and the DNS values changed we need to + // update the leases. + if (!ctx.fake_allocation_ && + ((prev_hostname != ctx.hostname_) || + (prev_fwd_dns_update != ctx.fwd_dns_update_) || + (prev_rev_dns_update != ctx.rev_dns_update_))) { + for (Lease6Collection::const_iterator l = ctx.new_leases_.begin(); + l != ctx.new_leases_.end(); ++l) { + (*l)->hostname_ = ctx.hostname_; + (*l)->fqdn_fwd_ = ctx.fwd_dns_update_; + (*l)->fqdn_rev_ = ctx.rev_dns_update_; + LeaseMgrFactory::instance().updateLease6(*l); + } + } +} + } // namespace dhcp } // namespace isc + diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 8bf65ea3cb..d72ef8b948 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -970,6 +970,44 @@ protected: void setStatusCode(boost::shared_ptr& container, const OptionPtr& status); + /// @brief Iterates over new leases, update stale DNS entries + /// + /// Checks the context's current subnet (most recently selected) against + /// an original selected subnet. If they are the same the function + /// simply returns. + /// + /// If they differ, we treat this as a dynamic subnet change made by the + /// allocation engine. It is possible that DDNS subnet parameters for + /// the new subnet are different and this needs to handled. We first + /// save the current DNS-related values from the context and then + /// re-run processClientFqdn(). This will rebuild the FQDN option + /// to send back to the client based on the new subnet as well as + /// update the context. If the new values are different from the + /// previous values, we iterate over the leases and update the + /// DNS values. + /// + /// @param question Client's message. + /// @param answer Server's response to a client. If server generated + /// @param ctx client context (contains subnet, duid and other parameters) + /// @param orig_subnet the originally selected subnet + /// + /// @note + /// Subnet may be modified by the allocation engine, if the initial subnet + /// belongs to a shared network. Note that this will only handle cases + /// where all IA_xx's in a client request result in a subnet change. It is + /// possible, currently, for the last IA_xx in request to end up using the + /// same subnet as originally selected, and we will miss a change incurred + /// by preceding IA_xx's. In general users should be strongly encouraged to + /// avoid situations where all of the following are true: + /// + /// 1. clients send more than one IA_xx in a query + /// 2. subnets in the shared-network are equally eligible (i.e don't have + /// class guards etc) + /// 3. subnets have differing options or DDNS parameters + // + void checkDynamicSubnetChange(const Pkt6Ptr& question, Pkt6Ptr& answer, + AllocEngine::ClientContext6& ctx, + const Subnet6Ptr orig_subnet); public: /// Used for DHCPv4-over-DHCPv6 too. diff --git a/src/bin/dhcp6/tests/dhcp6_client.cc b/src/bin/dhcp6/tests/dhcp6_client.cc index 3a1522942b..45c0831feb 100644 --- a/src/bin/dhcp6/tests/dhcp6_client.cc +++ b/src/bin/dhcp6/tests/dhcp6_client.cc @@ -527,11 +527,11 @@ Dhcp6Client::doRenew() { // RFC 8415. appendRequestedIAs(query); + context_.query_ = query; + // Add Client FQDN if configured. appendFQDN(); - context_.query_ = query; - sendMsg(context_.query_); context_.response_ = receiveOneMsg(); diff --git a/src/bin/dhcp6/tests/fqdn_unittest.cc b/src/bin/dhcp6/tests/fqdn_unittest.cc index f7b675dfdf..bbefd4f804 100644 --- a/src/bin/dhcp6/tests/fqdn_unittest.cc +++ b/src/bin/dhcp6/tests/fqdn_unittest.cc @@ -1687,4 +1687,155 @@ TEST_F(FqdnDhcpv6SrvTest, ddnsScopeTest) { } +// Verifies that the DDNS parameters used for a lease in subnet in +// a shared-network belong to lease's subnet. This checks that we +// get the right results even when the allocation engine changes the +// subnet chosen. The configuration is two 1-address pool subnets in +// a shared-network. The first will do a SARR, which consumes the first +// pool. This should cause the allocation engine to dynamically select +// the second subnet for the second client. The subnets define different +// values for qualifying suffixes, thus making it simple to verify +// the appropriate subnet parameters are used. Both clients then +// renew their leases. +TEST_F(FqdnDhcpv6SrvTest, ddnsSharedNetworkTest) { + std::string config_str = + "{ \"interfaces-config\": { \n" + " \"interfaces\": [ \"*\" ] \n" + "}, \n" + "\"preferred-lifetime\": 3000, \n" + "\"rebind-timer\": 2000, \n" + "\"renew-timer\": 1000, \n" + "\"valid-lifetime\": 4000, \n" + "\"shared-networks\": [ \n" + "{ \n" + "\"name\": \"frog\", \n" + "\"interface\": \"eth0\", \n" + "\"subnet6\": [ { \n" + "\"subnet\": \"2001:db8:1::/64\", \n" + "\"pools\": [ { \"pool\": \"2001:db8:1::1 - 2001:db8:1::1\" } ], \n" + "\"interface\": \"eth0\", \n" + "\"ddns-qualifying-suffix\": \"one.example.com.\" \n" + " }, \n" + " { \n" + "\"subnet\": \"2001:db8:2::/64\", \n" + "\"pools\": [ { \"pool\": \"2001:db8:2::1 - 2001:db8:2::1\" } ], \n" + "\"interface\": \"eth0\", \n" + "\"ddns-qualifying-suffix\": \"two.example.com.\" \n" + " } ] \n" + "} ], \n" + "\"ddns-send-updates\": true, \n" + "\"dhcp-ddns\" : { \n" + " \"enable-updates\" : true \n" + " } \n" + "}"; + + Dhcp6Client client1; + client1.setInterface("eth0"); + client1.requestAddress(); + + // Load a configuration with D2 enabled + ASSERT_NO_FATAL_FAILURE(configure(config_str, *client1.getServer())); + ASSERT_TRUE(CfgMgr::instance().ddnsEnabled()); + ASSERT_NO_THROW(client1.getServer()->startD2()); + + // Include the Client FQDN option. + ASSERT_NO_THROW(client1.useFQDN(Option6ClientFqdn::FLAG_S, "client1", + Option6ClientFqdn::PARTIAL)); + + // Now do a SARR. + ASSERT_NO_THROW(client1.doSARR()); + Pkt6Ptr resp = client1.getContext().response_; + ASSERT_TRUE(resp); + ASSERT_EQ(DHCPV6_REPLY, static_cast(resp->getType())); + + // Check that the response FQDN is as expected. + Option6ClientFqdnPtr fqdn; + fqdn = boost::dynamic_pointer_cast(resp->getOption(D6O_CLIENT_FQDN)); + ASSERT_TRUE(fqdn); + EXPECT_EQ("client1.one.example.com.", fqdn->getDomainName()); + + // ddns-send-updates for subnet 1 are enabled, verify the NCR is correct. + ASSERT_EQ(1, CfgMgr::instance().getD2ClientMgr().getQueueSize()); + verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, "2001:db8:1::1", + "", 0, 4000, "client1.one.example.com."); + + // Make sure the lease hostname is correct. + Lease6Ptr lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, + IOAddress("2001:db8:1::1")); + ASSERT_TRUE(lease); + EXPECT_EQ("client1.one.example.com.", lease->hostname_); + + // Now let's try with a different client. Subnet 1 is full so we should get an + // address from subnet 2. + Dhcp6Client client2(client1.getServer()); + client2.setInterface("eth0"); + client2.requestAddress(); + + // Include the Client FQDN option. + ASSERT_NO_THROW(client2.useFQDN(Option6ClientFqdn::FLAG_S, "client2", + Option6ClientFqdn::PARTIAL)); + + // Do a SARR. + ASSERT_NO_THROW(client2.doSARR()); + resp = client2.getContext().response_; + ASSERT_TRUE(resp); + ASSERT_EQ(DHCPV6_REPLY, static_cast(resp->getType())); + + // Check that the response FQDN is as expected. + fqdn = boost::dynamic_pointer_cast(resp->getOption(D6O_CLIENT_FQDN)); + ASSERT_TRUE(fqdn); + EXPECT_EQ("client2.two.example.com.", fqdn->getDomainName()); + + // ddns-send-updates for subnet 2 are enabled, verify the NCR is correct. + ASSERT_EQ(1, CfgMgr::instance().getD2ClientMgr().getQueueSize()); + verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, "2001:db8:2::1", + "", 0, 4000, "client2.two.example.com."); + + // Make sure the lease hostname is correct. + lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, IOAddress("2001:db8:2::1")); + ASSERT_TRUE(lease); + EXPECT_EQ("client2.two.example.com.", lease->hostname_); + + // Now let's check Renewals + // First we'll renew a client2. + ASSERT_NO_THROW(client2.doRenew()); + resp = client2.getContext().response_; + ASSERT_TRUE(resp); + ASSERT_EQ(DHCPV6_REPLY, static_cast(resp->getType())); + + // Check that the response FQDN is as expected. + fqdn = boost::dynamic_pointer_cast(resp->getOption(D6O_CLIENT_FQDN)); + ASSERT_TRUE(fqdn); + EXPECT_EQ("client2.two.example.com.", fqdn->getDomainName()); + + // ddns-send-updates for subnet 2 are enabled, but currently a renew/rebind does + // not update, unless the FQDN or flags change. + ASSERT_EQ(0, CfgMgr::instance().getD2ClientMgr().getQueueSize()); + + // Make sure the lease hostname is still correct. + lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, IOAddress("2001:db8:2::1")); + ASSERT_TRUE(lease); + EXPECT_EQ("client2.two.example.com.", lease->hostname_); + + // Next we'll renew client1 + ASSERT_NO_THROW(client1.doRenew()); + resp = client1.getContext().response_; + ASSERT_TRUE(resp); + ASSERT_EQ(DHCPV6_REPLY, static_cast(resp->getType())); + + // Check that the response FQDN is as expected. + fqdn = boost::dynamic_pointer_cast(resp->getOption(D6O_CLIENT_FQDN)); + ASSERT_TRUE(fqdn); + EXPECT_EQ("client1.one.example.com.", fqdn->getDomainName()); + + // ddns-send-updates for subnet 1 are enabled, but currently a renew/rebind does + // not update, unless the FQDN or flags change. + ASSERT_EQ(0, CfgMgr::instance().getD2ClientMgr().getQueueSize()); + + // Make sure the lease hostname is correct. + lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, IOAddress("2001:db8:1::1")); + ASSERT_TRUE(lease); + EXPECT_EQ("client1.one.example.com.", lease->hostname_); +} + } // end of anonymous namespace