diff --git a/ChangeLog b/ChangeLog index 51b4bd3b5e..cab210e31a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +1824. [func] tmark + Added a new parameter, ddns-use-conflict-resolution, to + kea-dhcp4 and kea-dhcp6. This parameter is passed per request + to kea-dhcp-ddns which uses it to determine whether or not + conflict resolution rules (see RFC 4703) are followed for that + request. The default value is true. Disabling conflict + resolution should only be used after careful consideration. + (Gitlab #1386) + 1823. [doc] tomek Updated options documentation for DHCPv4 and DHCPv6. (Gitlab #1436, #1460) diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index f9ee08d7dd..fde07dbf4d 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/src/bin/dhcp4/tests/fqdn_unittest.cc @@ -384,6 +384,7 @@ public: : D2ClientConfig::RCM_NEVER); subnet_->setDdnsGeneratedPrefix("myhost"); subnet_->setDdnsQualifyingSuffix("example.com"); + subnet_->setDdnsUseConflictResolution(true); ASSERT_NO_THROW(srv_->startD2()); } @@ -756,7 +757,8 @@ public: const std::string& dhcid, const time_t cltt, const uint16_t len, - const bool not_strict_expire_check = false) { + const bool not_strict_expire_check = false, + const bool exp_use_cr = true) { NameChangeRequestPtr ncr; ASSERT_NO_THROW(ncr = d2_mgr_.peekAt(0)); ASSERT_TRUE(ncr); @@ -783,6 +785,7 @@ public: } EXPECT_EQ(len, ncr->getLeaseLength()); EXPECT_EQ(isc::dhcp_ddns::ST_NEW, ncr->getStatus()); + EXPECT_EQ(exp_use_cr, ncr->useConflictResolution()); // Process the message off the queue ASSERT_NO_THROW(d2_mgr_.runReadyIO()); @@ -1072,6 +1075,27 @@ TEST_F(NameDhcpv4SrvTest, createNameChangeRequestsNewLease) { lease->cltt_, 100); } +// Verify that conflict resolution is disabled in the NCR when it is +// disabled for the lease's subnet. +TEST_F(NameDhcpv4SrvTest, noConflictResolution) { + Lease4Ptr lease = createLease(IOAddress("192.0.2.3"), "myhost.example.com.", + true, true); + Lease4Ptr old_lease; + + ASSERT_TRUE(getDdnsParams()->getEnableUpdates()); + subnet_->setDdnsUseConflictResolution(false); + + ASSERT_NO_THROW(srv_->createNameChangeRequests(lease, old_lease, *getDdnsParams())); + ASSERT_EQ(1, d2_mgr_.getQueueSize()); + + verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, + "192.0.2.3", "myhost.example.com.", + "00010132E91AA355CFBB753C0F0497A5A940436965" + "B68B6D438D98E680BF10B09F3BCF", + lease->cltt_, 100, false, false); +} + + // Verifies that createNameChange request only generates requests // if the situation dictates that it should. It checks: // diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 5e3b70afdd..cb64425b67 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1931,8 +1931,8 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, do_fwd, do_rev, opt_fqdn->getDomainName(), iaaddr->getAddress().toText(), - dhcid, 0, iaaddr->getValid())); - + dhcid, 0, 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 46f1a0dcc0..f5b7076c84 100644 --- a/src/bin/dhcp6/tests/fqdn_unittest.cc +++ b/src/bin/dhcp6/tests/fqdn_unittest.cc @@ -132,6 +132,7 @@ public: subnet_->setDdnsQualifyingSuffix("example.com"); subnet_->setHostnameCharSet("[^A-Za-z0-9-]"); subnet_->setHostnameCharReplacement("x"); + subnet_->setDdnsUseConflictResolution(true); ASSERT_NO_THROW(srv_->startD2()); } @@ -595,13 +596,15 @@ public: /// NameChangeRequest. /// @param fqdn The expected string value of the FQDN, if blank the /// check is skipped + /// @param exp_use_cr expected value of NCR::conflict_resolution_ void verifyNameChangeRequest(const isc::dhcp_ddns::NameChangeType type, const bool reverse, const bool forward, const std::string& addr, const std::string& dhcid, const uint64_t expires, const uint16_t len, - const std::string& fqdn="") { + const std::string& fqdn = "", + const bool exp_use_cr = true) { NameChangeRequestPtr ncr; ASSERT_NO_THROW(ncr = d2_mgr_.peekAt(0)); ASSERT_TRUE(ncr); @@ -625,6 +628,7 @@ public: EXPECT_EQ(fqdn, ncr->getFqdn()); } + EXPECT_EQ(exp_use_cr, ncr->useConflictResolution()); // Process the message off the queue ASSERT_NO_THROW(d2_mgr_.runReadyIO()); @@ -835,6 +839,39 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) { 0, 500); } +// Verify that conflict resolution is turned off when the +// subnet has it disabled. +TEST_F(FqdnDhcpv6SrvTest, noConflictResolution) { + // Create Reply message with Client Id and Server id. + Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY); + + // Create three IAs, each having different address. + addIA(1234, IOAddress("2001:db8:1::1"), answer); + + // Use domain name in upper case. It should be converted to lower-case + // before DHCID is calculated. So, we should get the same result as if + // we typed domain name in lower-case. + Option6ClientFqdnPtr fqdn = createClientFqdn(Option6ClientFqdn::FLAG_S, + "MYHOST.EXAMPLE.COM", + Option6ClientFqdn::FULL); + answer->addOption(fqdn); + + // Create NameChangeRequest for the first allocated address. + AllocEngine::ClientContext6 ctx; + subnet_->setDdnsUseConflictResolution(false); + ctx.subnet_ = subnet_; + 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. + verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true, + "2001:db8:1::1", + "000201415AA33D1187D148275136FA30300478" + "FAAAA3EBD29826B5C907B2C9268A6F52", + 0, 500, "", false); +} + // Checks that NameChangeRequests to add entries are not // created when ddns updates are disabled. TEST_F(FqdnDhcpv6SrvTest, noAddRequestsWhenDisabled) { diff --git a/src/lib/dhcpsrv/ncr_generator.cc b/src/lib/dhcpsrv/ncr_generator.cc index 7b0e9f29d2..74f4d80597 100644 --- a/src/lib/dhcpsrv/ncr_generator.cc +++ b/src/lib/dhcpsrv/ncr_generator.cc @@ -30,12 +30,15 @@ namespace { /// identifier. For DHCPv6 it will be a DUID. /// @param label Client identification information in the textual format. /// This is used for logging purposes. +/// @param use_conflict_resolution flag that tells D2 whether or not to +/// use conflict resolution. /// /// @tparam LeasePtrType Pointer to a lease. /// @tparam IdentifierType HW Address, Client Identifier or DUID. template void queueNCRCommon(const NameChangeType& chg_type, const LeasePtrType& lease, - const IdentifierType& identifier, const std::string& label) { + const IdentifierType& identifier, const std::string& label, + const bool use_conflict_resolution = true) { // Check if there is a need for update. if (lease->hostname_.empty() || (!lease->fqdn_fwd_ && !lease->fqdn_rev_) @@ -44,7 +47,7 @@ void queueNCRCommon(const NameChangeType& chg_type, const LeasePtrType& lease, DHCPSRV_QUEUE_NCR_SKIP) .arg(label) .arg(lease->addr_.toText()); - + return; } @@ -53,13 +56,12 @@ 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); - // 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_)); + lease->valid_lft_, use_conflict_resolution)); LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL_DATA, DHCPSRV_QUEUE_NCR) .arg(label) @@ -85,16 +87,26 @@ namespace dhcp { void queueNCR(const NameChangeType& chg_type, const Lease4Ptr& lease) { if (lease) { + // Figure out from the lease's subnet if we should use conflict resolution. + // If there's no subnet, something hinky is going on so we'll set it true. + bool use_cr = true; + Subnet4Ptr subnet = CfgMgr::instance().getCurrentCfg() + ->getCfgSubnets4()->getSubnet(lease->subnet_id_); + if (subnet) { + // We should always have subnet. + use_cr = subnet->getDdnsUseConflictResolution(); + } + // Client id takes precedence over HW address. if (lease->client_id_) { queueNCRCommon(chg_type, lease, lease->client_id_->getClientId(), - Pkt4::makeLabel(lease->hwaddr_, lease->client_id_)); + Pkt4::makeLabel(lease->hwaddr_, lease->client_id_), use_cr); } else { // Client id is not specified for the lease. Use HW address // instead. queueNCRCommon(chg_type, lease, lease->hwaddr_, - Pkt4::makeLabel(lease->hwaddr_, lease->client_id_)); + Pkt4::makeLabel(lease->hwaddr_, lease->client_id_), use_cr); } } } @@ -102,8 +114,18 @@ void queueNCR(const NameChangeType& chg_type, const Lease4Ptr& lease) { void queueNCR(const NameChangeType& chg_type, const Lease6Ptr& lease) { // DUID is required to generate NCR. if (lease && (lease->type_ != Lease::TYPE_PD) && lease->duid_) { + // Figure out from the lease's subnet if we should use conflict resolution. + // If there's no subnet, something hinky is going on so we'll set it true. + bool use_cr = true; + Subnet6Ptr subnet = CfgMgr::instance().getCurrentCfg() + ->getCfgSubnets6()->getSubnet(lease->subnet_id_); + if (subnet) { + // We should always have subnet. + use_cr = subnet->getDdnsUseConflictResolution(); + } + queueNCRCommon(chg_type, lease, *(lease->duid_), - Pkt6::makeLabel(lease->duid_, lease->hwaddr_)); + Pkt6::makeLabel(lease->duid_, lease->hwaddr_), use_cr); } } diff --git a/src/lib/dhcpsrv/tests/ncr_generator_unittest.cc b/src/lib/dhcpsrv/tests/ncr_generator_unittest.cc index 1bc98b0a64..e447f973f2 100644 --- a/src/lib/dhcpsrv/tests/ncr_generator_unittest.cc +++ b/src/lib/dhcpsrv/tests/ncr_generator_unittest.cc @@ -41,6 +41,9 @@ public: /// @brief Pointer to the lease object used by the tests. LeasePtrType lease_; + /// @brief Pointer to the lease's subnet + SubnetPtr subnet_; + /// @brief Constructor. NCRGeneratorTest() : d2_mgr_(CfgMgr::instance().getD2ClientMgr()), lease_() { @@ -70,6 +73,8 @@ public: disableD2(); // Base class TearDown. ::testing::Test::TearDown(); + + CfgMgr::instance().clear(); } /// @brief Enables DHCP-DDNS updates. @@ -133,7 +138,8 @@ public: const std::string& dhcid, const uint64_t expires, const uint16_t len, - const std::string& fqdn="") { + const std::string& fqdn="", + const bool use_conflict_resolution = true) { NameChangeRequestPtr ncr; ASSERT_NO_THROW(ncr = CfgMgr::instance().getD2ClientMgr().peekAt(0)); ASSERT_TRUE(ncr); @@ -146,6 +152,7 @@ public: EXPECT_EQ(expires, ncr->getLeaseExpiresOn()); EXPECT_EQ(len, ncr->getLeaseLength()); EXPECT_EQ(isc::dhcp_ddns::ST_NEW, ncr->getStatus()); + EXPECT_EQ(use_conflict_resolution, ncr->useConflictResolution()); if (!fqdn.empty()) { EXPECT_EQ(fqdn, ncr->getFqdn()); @@ -214,7 +221,8 @@ public: /// @param fqdn Hostname. /// @param exp_dhcid Expected DHCID. void testNCR(const NameChangeType chg_type, const bool fwd, const bool rev, - const std::string& fqdn, const std::string exp_dhcid) { + const std::string& fqdn, const std::string exp_dhcid, + const bool conflict_resolution = true) { // Queue NCR. ASSERT_NO_FATAL_FAILURE(sendNCR(chg_type, fwd, rev, fqdn)); // Expecting one NCR be generated. @@ -222,7 +230,7 @@ public: // Check the details of the NCR. verifyNameChangeRequest(chg_type, rev, fwd, lease_->addr_.toText(), exp_dhcid, lease_->cltt_ + lease_->valid_lft_, - lease_->valid_lft_); + lease_->valid_lft_, fqdn, conflict_resolution); } /// @brief Test that calling queueNCR for NULL lease doesn't cause @@ -234,6 +242,7 @@ public: ASSERT_NO_FATAL_FAILURE(queueNCR(chg_type, lease_)); EXPECT_EQ(0, d2_mgr_.getQueueSize()); } + }; /// @brief Test fixture class implementation for DHCPv6. @@ -253,8 +262,21 @@ public: /// @brief Implementation of the method creating DHCPv6 lease instance. virtual void initLease() { + Subnet6Ptr subnet(new Subnet6(IOAddress("2001:db8:1::"), 64, 100, 200, 300, 400)); + // Normally, this would be set via defaults + subnet->setDdnsUseConflictResolution(true); + + Pool6Ptr pool(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1::1"), + IOAddress("2001:db8:1::200"))); + subnet->addPool(pool); + subnet_ = subnet; + + CfgMgr& cfg_mgr = CfgMgr::instance(); + cfg_mgr.getStagingCfg()->getCfgSubnets6()->add(subnet); + cfg_mgr.commit(); + lease_.reset(new Lease6(Lease::TYPE_NA, IOAddress("2001:db8:1::1"), - duid_, 1234, 501, 502, 1, HWAddrPtr(), 0)); + duid_, 1234, 501, 502, subnet_->getID(), HWAddrPtr())); } }; @@ -415,6 +437,24 @@ TEST_F(NCRGenerator6Test, nullLease) { } } +// Verify that conflict resolution is set correctly by v6 queueNCR() +TEST_F(NCRGenerator6Test, useConflictResolution) { + { + SCOPED_TRACE("Subnet flag is false"); + subnet_->setDdnsUseConflictResolution(false); + testNCR(CHG_REMOVE, true, true, "MYHOST.example.com.", + "000201BE0D7A66F8AB6C4082E7F8B81E2656667A102E3D0ECCEA5E0DD71730F392119A", + false); + } + { + SCOPED_TRACE("Subnet flag is true"); + subnet_->setDdnsUseConflictResolution(true); + testNCR(CHG_REMOVE, true, true, "MYHOST.example.com.", + "000201BE0D7A66F8AB6C4082E7F8B81E2656667A102E3D0ECCEA5E0DD71730F392119A", + true); + } +} + /// @brief Test fixture class implementation for DHCPv4. class NCRGenerator4Test : public NCRGeneratorTest { public: @@ -431,9 +471,24 @@ public: /// @brief Implementation of the method creating DHCPv4 lease instance. virtual void initLease() { + + Subnet4Ptr subnet(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3)); + // Normally, this would be set via defaults + subnet->setDdnsUseConflictResolution(true); + + Pool4Ptr pool(new Pool4(IOAddress("192.0.2.100"), + IOAddress("192.0.2.200"))); + subnet->addPool(pool); + + CfgMgr& cfg_mgr = CfgMgr::instance(); + cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet); + cfg_mgr.commit(); + + subnet_ = subnet; lease_.reset(new Lease4(IOAddress("192.0.2.1"), hwaddr_, ClientIdPtr(), - 100, time(NULL), 1)); + 100, time(NULL), subnet_->getID())); } + }; // Test creation of the NameChangeRequest for both forward and reverse @@ -599,4 +654,22 @@ TEST_F(NCRGenerator4Test, nullLease) { } } +// Verify that conflict resolution is set correctly by v4 queueNCR() +TEST_F(NCRGenerator4Test, useConflictResolution) { + { + SCOPED_TRACE("Subnet flag is false"); + subnet_->setDdnsUseConflictResolution(false); + testNCR(CHG_REMOVE, true, true, "MYHOST.example.com.", + "000001E356D43E5F0A496D65BCA24D982D646140813E3" + "B03AB370BFF46BFA309AE7BFD", false); + } + { + SCOPED_TRACE("Subnet flag is true"); + subnet_->setDdnsUseConflictResolution(true); + testNCR(CHG_REMOVE, true, true, "MYHOST.example.com.", + "000001E356D43E5F0A496D65BCA24D982D646140813E3" + "B03AB370BFF46BFA309AE7BFD", true); + } +} + } // end of anonymous namespace