From: Thomas Markwalder Date: Thu, 23 Sep 2021 12:23:49 +0000 (-0400) Subject: [#1622] Fixed v6 incorrect DNS update flags X-Git-Tag: Kea-2.0.0~65 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=68a8b91b08366252317ec667c5b84f5e9e7b00bf;p=thirdparty%2Fkea.git [#1622] Fixed v6 incorrect DNS update flags kea-dhcp6 now correctly determines DNS update flags when the allocation engine dynamically re-selects the lease's network Added ChangeLog entry src/bin/dhcp6/dhcp6_srv.* Dhcpv6Srv::assignLeases() removed response parameter from calls that no longer need it Dhcpv6Srv::processClientFqdn() - now sets the context DNS direction flags Dhcpv6Srv::assignIA_NA() Dhcpv6Srv::extendIA_NA() - no longer set context DNS direction flags, removed now unused response parameter Dhcpv6Srv::assignIA_PD() - removed unused response parameter src/bin/dhcp6/tests/fqdn_unittest.cc TEST_F(FqdnDhcpv6SrvTest, ddnsSharedNetworkTest) - now verifies lease DNS flags TEST_F(FqdnDhcpv6SrvTest, ddnsSharedNetworkTest2) - new test to cover broken scenario --- diff --git a/ChangeLog b/ChangeLog index 6de3a1de42..95c526ab16 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +1949. [bug] tmark + kea-dhcp6 now correctly determines DNS update flags when + the allocation engine dynamically changes the selected + network subnet. + (Gitlab #1622) + 1948. [func] tmark HTTP library will now emit a warning log when the queue of pending client requests for a given URL exceeds a threshold. diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index c5ba575134..b0c16d346c 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1763,7 +1763,7 @@ Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer, opt != question->options_.end(); ++opt) { switch (opt->second->getType()) { case D6O_IA_NA: { - OptionPtr answer_opt = assignIA_NA(question, answer, ctx, + OptionPtr answer_opt = assignIA_NA(question, ctx, boost::dynamic_pointer_cast< Option6IA>(opt->second)); if (answer_opt) { @@ -1772,7 +1772,7 @@ Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer, break; } case D6O_IA_PD: { - OptionPtr answer_opt = assignIA_PD(question, answer, ctx, + OptionPtr answer_opt = assignIA_PD(question, ctx, boost::dynamic_pointer_cast< Option6IA>(opt->second)); if (answer_opt) { @@ -1835,6 +1835,10 @@ Dhcpv6Srv::processClientFqdn(const Pkt6Ptr& question, const Pkt6Ptr& answer, // current configuration. d2_mgr.adjustFqdnFlags(*fqdn, *fqdn_resp, *ddns_params); + // Get DDNS update direction flags + CfgMgr::instance().getD2ClientMgr().getUpdateDirections(*fqdn_resp, ctx.fwd_dns_update_, + ctx.rev_dns_update_); + // If there's a reservation and it has a hostname specified, use it! if (ctx.currentHost() && !ctx.currentHost()->getHostname().empty()) { // Add the qualifying suffix. @@ -2005,7 +2009,7 @@ Dhcpv6Srv::getMAC(const Pkt6Ptr& pkt) { } OptionPtr -Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer, +Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query, AllocEngine::ClientContext6& ctx, boost::shared_ptr ia) { @@ -2046,20 +2050,6 @@ Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer, return (ia_rsp); } - // Get DDNS update direction flags - bool do_fwd = false; - bool do_rev = false; - Option6ClientFqdnPtr fqdn = boost::dynamic_pointer_cast< - Option6ClientFqdn>(answer->getOption(D6O_CLIENT_FQDN)); - if (fqdn) { - CfgMgr::instance().getD2ClientMgr().getUpdateDirections(*fqdn, do_fwd, - do_rev); - } - - // Update per-packet context values. - ctx.fwd_dns_update_ = do_fwd; - ctx.rev_dns_update_ = do_rev; - // Set per-IA context values. ctx.createIAContext(); ctx.currentIA().iaid_ = ia->getIAID(); @@ -2146,7 +2136,7 @@ Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer, } OptionPtr -Dhcpv6Srv::assignIA_PD(const Pkt6Ptr& query, const Pkt6Ptr& /*answer*/, +Dhcpv6Srv::assignIA_PD(const Pkt6Ptr& query, AllocEngine::ClientContext6& ctx, boost::shared_ptr ia) { @@ -2288,7 +2278,7 @@ Dhcpv6Srv::assignIA_PD(const Pkt6Ptr& query, const Pkt6Ptr& /*answer*/, } OptionPtr -Dhcpv6Srv::extendIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer, +Dhcpv6Srv::extendIA_NA(const Pkt6Ptr& query, AllocEngine::ClientContext6& ctx, boost::shared_ptr ia) { @@ -2317,20 +2307,6 @@ Dhcpv6Srv::extendIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer, return (ia_rsp); } - // Get DDNS update directions - bool do_fwd = false; - bool do_rev = false; - Option6ClientFqdnPtr fqdn = boost::dynamic_pointer_cast< - Option6ClientFqdn>(answer->getOption(D6O_CLIENT_FQDN)); - if (fqdn) { - CfgMgr::instance().getD2ClientMgr().getUpdateDirections(*fqdn, - do_fwd, do_rev); - } - - // Set per-packet context values. - ctx.fwd_dns_update_ = do_fwd; - ctx.rev_dns_update_ = do_rev; - // Set per-IA context values. ctx.createIAContext(); ctx.currentIA().iaid_ = ia->getIAID(); @@ -2424,15 +2400,15 @@ Dhcpv6Srv::extendIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer, // If the new FQDN settings have changed for the lease, we need to // delete any existing FQDN records for this lease. - if (((*l)->hostname_ != ctx.hostname_) || ((*l)->fqdn_fwd_ != do_fwd) || - ((*l)->fqdn_rev_ != do_rev)) { + if (((*l)->hostname_ != ctx.hostname_) || ((*l)->fqdn_fwd_ != ctx.fwd_dns_update_) || + ((*l)->fqdn_rev_ != ctx.rev_dns_update_)) { LOG_DEBUG(ddns6_logger, DBG_DHCP6_DETAIL, DHCP6_DDNS_REMOVE_OLD_LEASE_FQDN) .arg(query->getLabel()) .arg((*l)->toText()) .arg(ctx.hostname_) - .arg(do_rev ? "true" : "false") - .arg(do_fwd ? "true" : "false"); + .arg(ctx.rev_dns_update_ ? "true" : "false") + .arg(ctx.fwd_dns_update_ ? "true" : "false"); queueNCR(CHG_REMOVE, *l); } @@ -2680,7 +2656,7 @@ Dhcpv6Srv::extendLeases(const Pkt6Ptr& query, Pkt6Ptr& reply, opt != query->options_.end(); ++opt) { switch (opt->second->getType()) { case D6O_IA_NA: { - OptionPtr answer_opt = extendIA_NA(query, reply, ctx, + OptionPtr answer_opt = extendIA_NA(query, ctx, boost::dynamic_pointer_cast< Option6IA>(opt->second)); if (answer_opt) { diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 349a2bb204..4a891a7081 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -443,15 +443,12 @@ protected: /// allocation failure. /// /// @param query client's message (typically SOLICIT or REQUEST) - /// @param answer server's response to the client's message. This - /// message should contain Client FQDN option being sent by the server /// to the client (if the client sent this option to the server). /// @param ctx client context (contains subnet, duid and other parameters) /// @param ia pointer to client's IA_NA option (client's request) /// /// @return IA_NA option (server's response) OptionPtr assignIA_NA(const isc::dhcp::Pkt6Ptr& query, - const isc::dhcp::Pkt6Ptr& answer, AllocEngine::ClientContext6& ctx, Option6IAPtr ia); @@ -464,12 +461,10 @@ protected: /// allocation failure. /// /// @param query client's message (typically SOLICIT or REQUEST) - /// @param answer server's response to the client's message (unused). /// @param ctx client context (contains subnet, duid and other parameters) /// @param ia pointer to client's IA_PD option (client's request) /// @return IA_PD option (server's response) OptionPtr assignIA_PD(const Pkt6Ptr& query, - const isc::dhcp::Pkt6Ptr& answer, AllocEngine::ClientContext6& ctx, boost::shared_ptr ia); @@ -481,14 +476,12 @@ protected: /// status code. /// /// @param query client's message (Renew or Rebind) - /// @param answer server's response to the client's message. This - /// message should contain Client FQDN option being sent by the server /// to the client (if the client sent this option to the server). /// @param ctx client context (contains subnet, duid and other parameters) /// @param ia IA_NA option which carries address for which lease lifetime /// will be extended. /// @return IA_NA option (server's response) - OptionPtr extendIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer, + OptionPtr extendIA_NA(const Pkt6Ptr& query, AllocEngine::ClientContext6& ctx, Option6IAPtr ia); diff --git a/src/bin/dhcp6/tests/fqdn_unittest.cc b/src/bin/dhcp6/tests/fqdn_unittest.cc index 2e2910ef9f..46c7b6d5f7 100644 --- a/src/bin/dhcp6/tests/fqdn_unittest.cc +++ b/src/bin/dhcp6/tests/fqdn_unittest.cc @@ -1799,11 +1799,13 @@ TEST_F(FqdnDhcpv6SrvTest, ddnsSharedNetworkTest) { 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. + // Make sure the lease hostname and fqdn flags are 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_); + EXPECT_TRUE(lease->fqdn_fwd_); + EXPECT_TRUE(lease->fqdn_rev_); // Now let's try with a different client. Subnet 1 is full so we should get an // address from subnet 2. @@ -1831,10 +1833,12 @@ TEST_F(FqdnDhcpv6SrvTest, ddnsSharedNetworkTest) { 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. + // Make sure the lease hostname and fqdn flags are correct. lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, IOAddress("2001:db8:2::1")); ASSERT_TRUE(lease); EXPECT_EQ("client2.two.example.com.", lease->hostname_); + EXPECT_TRUE(lease->fqdn_fwd_); + EXPECT_TRUE(lease->fqdn_rev_); // Now let's check Renewals // First we'll renew a client2. @@ -1872,10 +1876,122 @@ TEST_F(FqdnDhcpv6SrvTest, ddnsSharedNetworkTest) { // not update, unless the FQDN or flags change. ASSERT_EQ(0, CfgMgr::instance().getD2ClientMgr().getQueueSize()); - // Make sure the lease hostname is correct. + // Make sure the lease hostname and fqdn flags are correct. lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, IOAddress("2001:db8:1::1")); ASSERT_TRUE(lease); EXPECT_EQ("client1.one.example.com.", lease->hostname_); + EXPECT_TRUE(lease->fqdn_fwd_); + EXPECT_TRUE(lease->fqdn_rev_); +} + +// Verifies lease and NCR content (or lack of NCRs) are correct when +// subnets in a shared-network define different values for send-ddns-updates +// 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 client 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. +TEST_F(FqdnDhcpv6SrvTest, ddnsSharedNetworkTest2) { + 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" + "\"ddns-send-updates\": true \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" + "\"ddns-send-updates\": false \n" + " } ] \n" + "} ], \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 and fdqn flags are 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_); + EXPECT_TRUE(lease->fqdn_fwd_); + EXPECT_TRUE(lease->fqdn_rev_); + + // 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 disabled, verify there is no NCR. + ASSERT_EQ(0, CfgMgr::instance().getD2ClientMgr().getQueueSize()); + + // Make sure the lease hostname and fdqn flags are correct. + lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, IOAddress("2001:db8:2::1")); + ASSERT_TRUE(lease); + EXPECT_EQ("client2.two.example.com.", lease->hostname_); + EXPECT_FALSE(lease->fqdn_fwd_); + EXPECT_FALSE(lease->fqdn_rev_); } // Verifies that renews only generate NCRs if the situation dictates