From: Thomas Markwalder Date: Fri, 28 Jun 2019 15:16:44 +0000 (-0400) Subject: [#686,!403] LFC now sees sanity checking as disabled X-Git-Tag: Kea-1.6.0-beta2~149 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e292f3a234407a9a46fdf54d7a017e455b664327;p=thirdparty%2Fkea.git [#686,!403] LFC now sees sanity checking as disabled Add CfgConsistency defaults to application level parsing src/lib/dhcpsrv/cfg_consistency.h Changed constructor default mode to LEASE_CHECK_NONE src/lib/dhcpsrv/lease_file_loader.h LeaseFileLoader::load() - modified to create and use a sanity_checker only if checking is enabled. This avoids senselessly making the same decisions for every lease loaded. src/lib/dhcpsrv/parsers/simple_parser4.* src/lib/dhcpsrv/parsers/simple_parser6.* Added sanity checks defaults so they can be properly set at the application level. src/lib/dhcpsrv/sanity_checker.h SanityChecker::leaseCheckingEnabled() - new static function to test if sanity checking for leases is enabled src/lib/dhcpsrv/tests/sanity_checks_unittest.cc src/lib/dhcpsrv/tests/srv_config_unittest.cc updated tests for new constructor default value --- diff --git a/ChangeLog b/ChangeLog index 7fda87db0e..3a9103fd0d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +1607. [bug] tmark + Corrected an initialization issue which caused lease sanity + checking to be enabled inside the Lease File Cleanup (LFC) + process. The LFC cannot meaningfully perform sanity checking + as it does not have access to the full server configuration. + (Gitlab #686,!403 git TBD) + 1606. [bug] tmark Corrected an error with retrieving DHCPv6 leases, whose IAID values are larger than int32_t max, from Postgresql lease diff --git a/src/lib/dhcpsrv/cfg_consistency.h b/src/lib/dhcpsrv/cfg_consistency.h index c6f31f34f8..8446ab8306 100644 --- a/src/lib/dhcpsrv/cfg_consistency.h +++ b/src/lib/dhcpsrv/cfg_consistency.h @@ -1,4 +1,4 @@ -// Copyright (C) 2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2018-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 @@ -34,7 +34,7 @@ class CfgConsistency : public isc::data::UserContext, public isc::data::CfgToEle /// @brief Constructor CfgConsistency() - : lease_sanity_check_(LEASE_CHECK_WARN) { + : lease_sanity_check_(LEASE_CHECK_NONE) { } diff --git a/src/lib/dhcpsrv/lease_file_loader.h b/src/lib/dhcpsrv/lease_file_loader.h index 38bf101d1d..1a13f22af1 100644 --- a/src/lib/dhcpsrv/lease_file_loader.h +++ b/src/lib/dhcpsrv/lease_file_loader.h @@ -1,4 +1,4 @@ -// Copyright (C) 2015-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-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 @@ -12,6 +12,7 @@ #include #include +#include #include namespace isc { @@ -86,7 +87,14 @@ public: lease_file.close(); lease_file.open(); - SanityChecker lease_checker; + // Create lease sanity checker if checking is enabled. + boost::scoped_ptr lease_checker; + if (SanityChecker::leaseCheckingEnabled(false)) { + // Since lease file is loaded during the configuration, + // we have to use staging config, rather than current + // config for this (false = staging). + lease_checker.reset(new SanityChecker()); + } boost::shared_ptr lease; // Track the number of corrupted leases. @@ -125,12 +133,15 @@ public: DHCPSRV_MEMFILE_LEASE_LOAD) .arg(lease->toText()); - // Now see if we need to sanitize this lease. As lease file is - // loaded during the configuration, we have to use staging config, - // rather than current config for this (false = staging). - lease_checker.checkLease(lease, false); - if (!lease) { - continue; + if (lease_checker) { + // If the lease is insane the checker will rese the lease pointer. + // As lease file is loaded during the configuration, we have + // to use staging config, rather than current config for this + // (false = staging). + lease_checker->checkLease(lease, false); + if (!lease) { + continue; + } } // Check if this lease exists. diff --git a/src/lib/dhcpsrv/parsers/simple_parser4.cc b/src/lib/dhcpsrv/parsers/simple_parser4.cc index f769a87a4a..861356160f 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser4.cc +++ b/src/lib/dhcpsrv/parsers/simple_parser4.cc @@ -163,6 +163,18 @@ const SimpleDefaults SimpleParser4::IFACE4_DEFAULTS = { { "re-detect", Element::boolean, "true" } }; +/// @brief This table defines default values for dhcp-queue-control in DHCPv4. +const SimpleDefaults SimpleParser4::DHCP_QUEUE_CONTROL4_DEFAULTS = { + { "enable-queue", Element::boolean, "false"}, + { "queue-type", Element::string, "kea-ring4"}, + { "capacity", Element::integer, "500"} +}; + +/// @brief This defines default values for sanity checking for DHCPv4. +const SimpleDefaults SimpleParser4::SANITY_CHECKS4_DEFAULTS = { + { "lease-checks", Element::string, "warn" } +}; + /// @brief List of parameters that can be inherited to subnet4 scope. /// /// Some parameters may be defined on both global (directly in Dhcp4) and @@ -192,14 +204,6 @@ const ParamsList SimpleParser4::INHERIT_TO_SUBNET4 = { "t2-percent" }; -/// @brief This table defines default values for dhcp-queue-control in DHCPv4. -const SimpleDefaults SimpleParser4::DHCP_QUEUE_CONTROL4_DEFAULTS = { - { "enable-queue", Element::boolean, "false"}, - { "queue-type", Element::string, "kea-ring4"}, - { "capacity", Element::integer, "500"} -}; - - /// @} /// --------------------------------------------------------------------------- @@ -266,6 +270,18 @@ size_t SimpleParser4::setAllDefaults(isc::data::ElementPtr global) { cnt += setDefaults(mutable_cfg, DHCP_QUEUE_CONTROL4_DEFAULTS); + // Set the defaults for sanity-checks. If the element isn't + // there we'll add it. + ConstElementPtr sanity_checks = global->get("sanity-checks"); + if (sanity_checks) { + mutable_cfg = boost::const_pointer_cast(sanity_checks); + } else { + mutable_cfg = Element::createMap(); + global->set("sanity-checks", mutable_cfg); + } + + cnt += setDefaults(mutable_cfg, SANITY_CHECKS4_DEFAULTS); + return (cnt); } diff --git a/src/lib/dhcpsrv/parsers/simple_parser4.h b/src/lib/dhcpsrv/parsers/simple_parser4.h index 069deb27ef..d2f0b0acf4 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser4.h +++ b/src/lib/dhcpsrv/parsers/simple_parser4.h @@ -46,6 +46,7 @@ public: static const isc::data::SimpleDefaults SHARED_NETWORK4_DEFAULTS; static const isc::data::SimpleDefaults IFACE4_DEFAULTS; static const isc::data::SimpleDefaults DHCP_QUEUE_CONTROL4_DEFAULTS; + static const isc::data::SimpleDefaults SANITY_CHECKS4_DEFAULTS; static const isc::data::ParamsList INHERIT_TO_SUBNET4; }; diff --git a/src/lib/dhcpsrv/parsers/simple_parser6.cc b/src/lib/dhcpsrv/parsers/simple_parser6.cc index e76b60f21c..53088d5476 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser6.cc +++ b/src/lib/dhcpsrv/parsers/simple_parser6.cc @@ -146,6 +146,18 @@ const SimpleDefaults SimpleParser6::IFACE6_DEFAULTS = { { "re-detect", Element::boolean, "true" } }; +/// @brief This table defines default values for dhcp-queue-control in DHCPv4. +const SimpleDefaults SimpleParser6::DHCP_QUEUE_CONTROL6_DEFAULTS = { + { "enable-queue", Element::boolean, "false"}, + { "queue-type", Element::string, "kea-ring6"}, + { "capacity", Element::integer, "500"} +}; + +/// @brief This defines default values for sanity checking for DHCPv6. +const SimpleDefaults SimpleParser6::SANITY_CHECKS6_DEFAULTS = { + { "lease-checks", Element::string, "warn" } +}; + /// @brief List of parameters that can be inherited from the global to subnet6 scope. /// /// Some parameters may be defined on both global (directly in Dhcp6) and @@ -175,14 +187,6 @@ const ParamsList SimpleParser6::INHERIT_TO_SUBNET6 = { "t2-percent" }; -/// @brief This table defines default values for dhcp-queue-control in DHCPv4. -const SimpleDefaults SimpleParser6::DHCP_QUEUE_CONTROL6_DEFAULTS = { - { "enable-queue", Element::boolean, "false"}, - { "queue-type", Element::string, "kea-ring6"}, - { "capacity", Element::integer, "500"} -}; - - /// @} /// --------------------------------------------------------------------------- @@ -251,6 +255,18 @@ size_t SimpleParser6::setAllDefaults(isc::data::ElementPtr global) { cnt += setDefaults(mutable_cfg, DHCP_QUEUE_CONTROL6_DEFAULTS); + // Set the defaults for sanity-checks. If the element isn't + // there we'll add it. + ConstElementPtr sanity_checks = global->get("sanity-checks"); + if (sanity_checks) { + mutable_cfg = boost::const_pointer_cast(sanity_checks); + } else { + mutable_cfg = Element::createMap(); + global->set("sanity-checks", mutable_cfg); + } + + cnt += setDefaults(mutable_cfg, SANITY_CHECKS6_DEFAULTS); + return (cnt); } diff --git a/src/lib/dhcpsrv/parsers/simple_parser6.h b/src/lib/dhcpsrv/parsers/simple_parser6.h index 0004811561..65067f7803 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser6.h +++ b/src/lib/dhcpsrv/parsers/simple_parser6.h @@ -47,6 +47,7 @@ public: static const isc::data::SimpleDefaults SHARED_NETWORK6_DEFAULTS; static const isc::data::SimpleDefaults IFACE6_DEFAULTS; static const isc::data::SimpleDefaults DHCP_QUEUE_CONTROL6_DEFAULTS; + static const isc::data::SimpleDefaults SANITY_CHECKS6_DEFAULTS; static const isc::data::ParamsList INHERIT_TO_SUBNET6; }; diff --git a/src/lib/dhcpsrv/sanity_checker.cc b/src/lib/dhcpsrv/sanity_checker.cc index 1ce6959fa6..dd4b6264a8 100644 --- a/src/lib/dhcpsrv/sanity_checker.cc +++ b/src/lib/dhcpsrv/sanity_checker.cc @@ -5,7 +5,6 @@ // file, You can obtain one at http://mozilla.org/MPL/2.0/. #include #include -#include #include #include #include @@ -14,6 +13,22 @@ namespace isc { namespace dhcp { +bool SanityChecker::leaseCheckingEnabled(bool current) { + SrvConfigPtr cfg; + if (current) { + cfg = CfgMgr::instance().getCurrentCfg(); + } else { + cfg = CfgMgr::instance().getStagingCfg(); + } + + if (cfg) { + CfgConsistencyPtr sanity = cfg->getConsistency(); + return (sanity && (sanity->getLeaseSanityCheck() != CfgConsistency::LEASE_CHECK_NONE)); + } + + return (false); +} + void SanityChecker::checkLease(Lease4Ptr& lease, bool current) { SrvConfigPtr cfg; if (current) { @@ -21,7 +36,13 @@ void SanityChecker::checkLease(Lease4Ptr& lease, bool current) { } else { cfg = CfgMgr::instance().getStagingCfg(); } + CfgConsistencyPtr sanity = cfg->getConsistency(); + if (sanity->getLeaseSanityCheck() == CfgConsistency::LEASE_CHECK_NONE) { + // No sense going farther. + return; + } + CfgSubnets4Ptr subnets = cfg->getCfgSubnets4(); checkLeaseInternal(lease, sanity, subnets); } @@ -39,6 +60,11 @@ void SanityChecker::checkLease(Lease6Ptr& lease, bool current) { cfg = CfgMgr::instance().getStagingCfg(); } CfgConsistencyPtr sanity = cfg->getConsistency(); + if (sanity->getLeaseSanityCheck() == CfgConsistency::LEASE_CHECK_NONE) { + // No sense going farther. + return; + } + CfgSubnets6Ptr subnets = cfg->getCfgSubnets6(); checkLeaseInternal(lease, sanity, subnets); } @@ -47,12 +73,7 @@ template void SanityChecker::checkLeaseInternal(LeasePtrType& lease, const CfgConsistencyPtr& checks, const SubnetsType& subnets) { - if (checks->getLeaseSanityCheck() == CfgConsistency::LEASE_CHECK_NONE) { - return; - } - auto subnet = subnets->getBySubnetId(lease->subnet_id_); - if (subnet && subnet->inRange(lease->addr_)) { // If the subnet is defined and the address is in range, we're good. @@ -147,8 +168,6 @@ void SanityChecker::checkLeaseInternal(LeasePtrType& lease, const CfgConsistency template SubnetID SanityChecker::findSubnetId(const LeaseType& lease, const SubnetsType& subnets) { - //CfgSubnets4Ptr subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4(); - auto subnet = subnets->selectSubnet(lease->addr_); if (!subnet) { return (0); diff --git a/src/lib/dhcpsrv/sanity_checker.h b/src/lib/dhcpsrv/sanity_checker.h index ad0ee24473..57fe7f8e19 100644 --- a/src/lib/dhcpsrv/sanity_checker.h +++ b/src/lib/dhcpsrv/sanity_checker.h @@ -44,6 +44,14 @@ class SanityChecker { /// config void checkLease(Lease6Ptr& lease, bool current = true); + /// @brief Indicates the specified configuration enables lease sanity checking. + /// + /// @param current specifies whether to use current (true) or staging(false) + /// server configuration + /// @return true if the sanity-checks/lease-checks parameter value (see + /// @ref CfgConsistency for details) is not CfgConsistency::LEASE_CHECK_NONE. + static bool leaseCheckingEnabled(bool current = true); + private: /// @brief Internal implementation for checkLease command diff --git a/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc b/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc index 2f4be690e0..662b6c8f09 100644 --- a/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc +++ b/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc @@ -159,9 +159,9 @@ TEST_F(SanityChecksTest, leaseCheck) { SrvConfig cfg; - // The default should be to warn about inconsistency. + // The default should be to none. EXPECT_EQ(cfg.getConsistency()->getLeaseSanityCheck(), - CfgConsistency::LEASE_CHECK_WARN); + CfgConsistency::LEASE_CHECK_NONE); parserCheck(cfg, valid1, false, CfgConsistency::LEASE_CHECK_NONE); parserCheck(cfg, valid2, false, CfgConsistency::LEASE_CHECK_WARN); diff --git a/src/lib/dhcpsrv/tests/srv_config_unittest.cc b/src/lib/dhcpsrv/tests/srv_config_unittest.cc index 69eede8412..c368a9e2d9 100644 --- a/src/lib/dhcpsrv/tests/srv_config_unittest.cc +++ b/src/lib/dhcpsrv/tests/srv_config_unittest.cc @@ -513,7 +513,7 @@ TEST_F(SrvConfigTest, unparse) { defaults += "\"lease-database\": { \"type\": \"memfile\" },\n"; defaults += "\"hooks-libraries\": [ ],\n"; defaults += "\"sanity-checks\": {\n"; - defaults += " \"lease-checks\": \"warn\"\n"; + defaults += " \"lease-checks\": \"none\"\n"; defaults += " },\n"; defaults += "\"dhcp-ddns\": \n";