From: Thomas Markwalder Date: Tue, 8 Dec 2020 13:34:23 +0000 (-0500) Subject: [#936] kea-dhcpX now calculates DDNS TTL X-Git-Tag: Kea-1.9.3~126 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fa63b59f194dab3445ab0f7d07609517b0bcfacb;p=thirdparty%2Fkea.git [#936] kea-dhcpX now calculates DDNS TTL DDNS TTL is now calculated based on RFC 4702 Sec 5 guidelines. src/lib/dhcpsrv/ncr_generator.* calculateDdnsTtl() - new function to calculate DNS RR TTL from a lease life time queueNCRCommon() - modified to use calculateDdnsTtl() src/lib/dhcpsrv/tests/ncr_generator_unittest.cc Updated unit tests src/bin/dhcp6/dhcp6_srv.cc Dhcpv6Srv::createNameChangeRequests() - modified to use calculateDdnsTtl() src/bin/dhcp4/tests/fqdn_unittest.cc src/bin/dhcp6/tests/fqdn_unittest.cc Updated unit tests --- diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index 7445bdd215..db17c04e03 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/src/bin/dhcp4/tests/fqdn_unittest.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -746,10 +747,11 @@ public: /// if the value supplied is not empty):w /// @param cltt - cltt value from the lease the NCR for which the NCR /// was generated expected value for - /// @param len - expected lease length in the NCR + /// @param lifetime - lease's valid lifetime from which NCR ttl was + /// generated /// @param not_strict_expire_check - when true the comparison of the NCR /// lease expiration time is conducted as greater than or equal to rather - /// equal to CLTT plus lease length. + /// equal to CLTT plus lease ttl . /// @param exp_use_cr expected value of conflict resolution flag void verifyNameChangeRequest(const isc::dhcp_ddns::NameChangeType type, const bool reverse, const bool forward, @@ -757,7 +759,7 @@ public: const std::string& fqdn, const std::string& dhcid, const time_t cltt, - const uint16_t len, + const uint16_t valid_lft, const bool not_strict_expire_check = false, const bool exp_use_cr = true) { NameChangeRequestPtr ncr; @@ -774,17 +776,20 @@ public: if (!dhcid.empty()) { EXPECT_EQ(dhcid, ncr->getDhcid().toStr()); } + // In some cases, the test doesn't have access to the last transmission // time for the particular client. In such cases, the test can use the // current time as cltt but the it may not check the lease expiration // time for equality but rather check that the lease expiration time // is not greater than the current time + lease lifetime. + uint32_t ttl = calculateDdnsTtl(valid_lft); if (not_strict_expire_check) { - EXPECT_GE(cltt + len, ncr->getLeaseExpiresOn()); + EXPECT_GE(cltt + ttl, ncr->getLeaseExpiresOn()); } else { - EXPECT_EQ(cltt + len, ncr->getLeaseExpiresOn()); + EXPECT_EQ(cltt + ttl, ncr->getLeaseExpiresOn()); } - EXPECT_EQ(len, ncr->getLeaseLength()); + + EXPECT_EQ(ttl, ncr->getLeaseLength()); EXPECT_EQ(isc::dhcp_ddns::ST_NEW, ncr->getStatus()); EXPECT_EQ(exp_use_cr, ncr->useConflictResolution()); diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index dd98c80521..38f4687e16 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1940,7 +1940,7 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, do_fwd, do_rev, opt_fqdn->getDomainName(), iaaddr->getAddress().toText(), - dhcid, 0, iaaddr->getValid(), + dhcid, 0, calculateDdnsTtl(iaaddr->getValid()), ctx.getDdnsParams()->getUseConflictResolution())); LOG_DEBUG(ddns6_logger, DBG_DHCP6_DETAIL, DHCP6_DDNS_CREATE_ADD_NAME_CHANGE_REQUEST).arg(ncr->toText()); diff --git a/src/bin/dhcp6/tests/fqdn_unittest.cc b/src/bin/dhcp6/tests/fqdn_unittest.cc index f5b7076c84..9c0e83e9a5 100644 --- a/src/bin/dhcp6/tests/fqdn_unittest.cc +++ b/src/bin/dhcp6/tests/fqdn_unittest.cc @@ -590,9 +590,9 @@ public: /// these values programmatically and place them here. Should the /// underlying implementation of createDigest() change these test values /// will likely need to be updated as well. - /// @param expires A timestamp when the lease associated with the - /// NameChangeRequest expires. - /// @param len A valid lifetime of the lease associated with the + /// @param expires The cltt of the lease associated with the + /// NameChangeRequest, and used to calculate NCR expires value. + /// @param valid_lft the valid lifetime of the lease associated with the /// NameChangeRequest. /// @param fqdn The expected string value of the FQDN, if blank the /// check is skipped @@ -602,7 +602,7 @@ public: const std::string& addr, const std::string& dhcid, const uint64_t expires, - const uint16_t len, + const uint16_t valid_lft, const std::string& fqdn = "", const bool exp_use_cr = true) { NameChangeRequestPtr ncr; @@ -617,11 +617,13 @@ public: EXPECT_EQ(dhcid, ncr->getDhcid().toStr()); } + uint32_t ttl = calculateDdnsTtl(valid_lft); if (expires != 0) { - EXPECT_EQ(expires, ncr->getLeaseExpiresOn()); + EXPECT_EQ(expires + ttl, ncr->getLeaseExpiresOn()); } - EXPECT_EQ(len, ncr->getLeaseLength()); + EXPECT_EQ(ttl, ncr->getLeaseLength()); + EXPECT_EQ(isc::dhcp_ddns::ST_NEW, ncr->getStatus()); if (! fqdn.empty()) { @@ -916,8 +918,7 @@ TEST_F(FqdnDhcpv6SrvTest, createRemovalNameChangeRequestFwdRev) { "2001:db8:1::1", "000201415AA33D1187D148275136FA30300478" "FAAAA3EBD29826B5C907B2C9268A6F52", - lease_->cltt_ + lease_->valid_lft_, 502); - + lease_->cltt_, lease_->valid_lft_); } // Checks that calling queueNCR would not result in error if DDNS updates are @@ -956,7 +957,7 @@ TEST_F(FqdnDhcpv6SrvTest, createRemovalNameChangeRequestRev) { "2001:db8:1::1", "000201415AA33D1187D148275136FA30300478" "FAAAA3EBD29826B5C907B2C9268A6F52", - lease_->cltt_ + lease_->valid_lft_, 502); + lease_->cltt_, lease_->valid_lft_); } @@ -1049,12 +1050,12 @@ TEST_F(FqdnDhcpv6SrvTest, processTwoRequestsDiffFqdn) { "2001:db8:1:1::dead:beef", "000201415AA33D1187D148275136FA30300478" "FAAAA3EBD29826B5C907B2C9268A6F52", - lease_->cltt_ + lease_->valid_lft_, 4000); + lease_->cltt_, lease_->valid_lft_); verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, "2001:db8:1:1::dead:beef", "000201D422AA463306223D269B6CB7AFE7AAD265FC" "EA97F93623019B2E0D14E5323D5A", - 0, 4000); + 0, lease_->valid_lft_); } @@ -1159,12 +1160,12 @@ TEST_F(FqdnDhcpv6SrvTest, processRequestRenewDiffFqdn) { "2001:db8:1:1::dead:beef", "000201415AA33D1187D148275136FA30300478" "FAAAA3EBD29826B5C907B2C9268A6F52", - lease_->cltt_ + lease_->valid_lft_, 4000); + lease_->cltt_, lease_->valid_lft_); verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, "2001:db8:1:1::dead:beef", "000201D422AA463306223D269B6CB7AFE7AAD265FC" "EA97F93623019B2E0D14E5323D5A", - 0, 4000); + 0, lease_->valid_lft_); } @@ -1251,13 +1252,13 @@ TEST_F(FqdnDhcpv6SrvTest, processRequestRenewFqdnFlags) { "2001:db8:1:1::dead:beef", "000201415AA33D1187D148275136FA30300478" "FAAAA3EBD29826B5C907B2C9268A6F52", - lease_->cltt_ + lease_->valid_lft_, 4000); + lease_->cltt_, lease_->valid_lft_); verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, "2001:db8:1:1::dead:beef", "000201415AA33D1187D148275136FA30300478" "FAAAA3EBD29826B5C907B2C9268A6F52", - 0, 4000); + 0, lease_->valid_lft_); // 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. @@ -1269,8 +1270,7 @@ TEST_F(FqdnDhcpv6SrvTest, processRequestRenewFqdnFlags) { "2001:db8:1:1::dead:beef", "000201415AA33D1187D148275136FA30300478" "FAAAA3EBD29826B5C907B2C9268A6F52", - lease_->cltt_ + lease_->valid_lft_, 4000); - + lease_->cltt_, lease_->valid_lft_); } @@ -1292,7 +1292,7 @@ TEST_F(FqdnDhcpv6SrvTest, processRequestRelease) { "2001:db8:1:1::dead:beef", "000201415AA33D1187D148275136FA30300478" "FAAAA3EBD29826B5C907B2C9268A6F52", - 0, 4000); + 0, lease_->valid_lft_); // Client may send Release message. In this case the lease should be // removed and all existing DNS entries for this lease should also @@ -1305,8 +1305,7 @@ TEST_F(FqdnDhcpv6SrvTest, processRequestRelease) { "2001:db8:1:1::dead:beef", "000201415AA33D1187D148275136FA30300478" "FAAAA3EBD29826B5C907B2C9268A6F52", - lease_->cltt_ + lease_->valid_lft_, 4000); - + lease_->cltt_, lease_->valid_lft_); } // Checks that the server include DHCPv6 Client FQDN option in its @@ -1379,7 +1378,8 @@ TEST_F(FqdnDhcpv6SrvTest, processRequestReuseExpiredLease) { verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, "2001:db8:1:1::dead:beef", "000201415AA33D1187D148275136FA30300478" - "FAAAA3EBD29826B5C907B2C9268A6F52", 0, 4); + "FAAAA3EBD29826B5C907B2C9268A6F52", + 0, lease_->valid_lft_); // One of the following: IAID, DUID or subnet identifier has to be changed // because otherwise the allocation engine will treat the lease as // being renewed by the same client. If we at least change subnet identifier @@ -1409,7 +1409,7 @@ TEST_F(FqdnDhcpv6SrvTest, processRequestReuseExpiredLease) { "2001:db8:1:1::dead:beef", "000201D422AA463306223D269B6CB7AFE7AAD2" "65FCEA97F93623019B2E0D14E5323D5A", - lease->cltt_ + lease->valid_lft_, 5); + lease->cltt_, lease->valid_lft_); // The second name change request should add a DNS mapping for // a new lease. verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, diff --git a/src/lib/dhcpsrv/ncr_generator.cc b/src/lib/dhcpsrv/ncr_generator.cc index 2fb2cdab16..bcb8011e68 100644 --- a/src/lib/dhcpsrv/ncr_generator.cc +++ b/src/lib/dhcpsrv/ncr_generator.cc @@ -56,12 +56,16 @@ void queueNCRCommon(const NameChangeType& chg_type, const LeasePtrType& lease, std::vector hostname_wire; OptionDataTypeUtil::writeFqdn(lease->hostname_, hostname_wire, true); D2Dhcid dhcid = D2Dhcid(identifier, hostname_wire); + + // Calculate the TTL based on lease life time. + uint32_t ttl = calculateDdnsTtl(lease->valid_lft_); + // Create name change request. NameChangeRequestPtr ncr (new NameChangeRequest(chg_type, lease->fqdn_fwd_, lease->fqdn_rev_, lease->hostname_, lease->addr_.toText(), - dhcid, lease->cltt_ + lease->valid_lft_, - lease->valid_lft_, use_conflict_resolution)); + dhcid, lease->cltt_ + ttl, + ttl, use_conflict_resolution)); LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL_DATA, DHCPSRV_QUEUE_NCR) .arg(label) @@ -129,5 +133,15 @@ void queueNCR(const NameChangeType& chg_type, const Lease6Ptr& lease) { } } +uint32_t calculateDdnsTtl(uint32_t lease_lft) { + // Per RFC 4702 DDNS RR TTL should be given by: + // ((lease life time / 3) < 10 minutes) ? 10 minutes : (lease life time / 3) + if (lease_lft < 1800) { + return (600); + } + + return (lease_lft / 3); +} + } } diff --git a/src/lib/dhcpsrv/ncr_generator.h b/src/lib/dhcpsrv/ncr_generator.h index 15c928fbc0..5f07939992 100644 --- a/src/lib/dhcpsrv/ncr_generator.h +++ b/src/lib/dhcpsrv/ncr_generator.h @@ -1,4 +1,4 @@ -// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-2020 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 @@ -41,6 +41,19 @@ void queueNCR(const dhcp_ddns::NameChangeType& chg_type, const Lease4Ptr& lease) /// @param lease Pointer to the lease. void queueNCR(const dhcp_ddns::NameChangeType& chg_type, const Lease6Ptr& lease); +/// @brief Calculates TTL for a DNS resource record based on lease life time. +/// +/// Per RFC 4702 Section 5, the RR TTL should be calculated as: +/// TTL = ((lease life time / 3) < 10 minutes) ? 10 minutes : (lease life time / 3) +/// +/// This function may be expanded in the future to support configurable +/// parameters. +/// +/// @param valid_lft valid life time of the lease +/// +/// @return the calculated TTL. +uint32_t calculateDdnsTtl(uint32_t lease_life_time); + } // end of isc::dhcp namespace } // end of isc namespace diff --git a/src/lib/dhcpsrv/tests/ncr_generator_unittest.cc b/src/lib/dhcpsrv/tests/ncr_generator_unittest.cc index 34ebee4838..905df55479 100644 --- a/src/lib/dhcpsrv/tests/ncr_generator_unittest.cc +++ b/src/lib/dhcpsrv/tests/ncr_generator_unittest.cc @@ -228,10 +228,12 @@ public: ASSERT_NO_FATAL_FAILURE(sendNCR(chg_type, fwd, rev, fqdn)); // Expecting one NCR be generated. ASSERT_EQ(1, d2_mgr_.getQueueSize()); + + uint32_t ttl = calculateDdnsTtl(lease_->valid_lft_); + // Check the details of the NCR. verifyNameChangeRequest(chg_type, rev, fwd, lease_->addr_.toText(), exp_dhcid, - lease_->cltt_ + lease_->valid_lft_, - lease_->valid_lft_, fqdn, exp_use_cr); + lease_->cltt_ + ttl, ttl, fqdn, exp_use_cr); } /// @brief Test that calling queueNCR for NULL lease doesn't cause @@ -621,11 +623,12 @@ TEST_F(NCRGenerator4Test, useClientId) { ASSERT_NO_FATAL_FAILURE(queueRemovalNCR(true, true, "myhost.example.com.")); ASSERT_EQ(1, d2_mgr_.getQueueSize()); + uint32_t ttl = calculateDdnsTtl(lease_->valid_lft_); verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true, "192.0.2.1", "000101C7AA5420483BDA99C437636EA7DA2FE18" "31C9679FEB031C360CA571298F3D1FA", - lease_->cltt_ + lease_->valid_lft_, 100); + lease_->cltt_ + ttl, ttl); { SCOPED_TRACE("case CHG_REMOVE"); testNCR(CHG_REMOVE, true, true, "myhost.example.com.", @@ -672,4 +675,14 @@ TEST_F(NCRGenerator4Test, useConflictResolution) { } } +// Verify that calculateDdnsTtl() produces the expected values. +TEST_F(NCRGenerator4Test, calculateDdnsTtl) { + + // A life time less than or equal to 1800 should yield a TTL of 600 seconds. + EXPECT_EQ(600, calculateDdnsTtl(100)); + + // A life time > 1800 should be 1/3 of the value. + EXPECT_EQ(601, calculateDdnsTtl(1803)); +} + } // end of anonymous namespace