diff --git a/src/lib/dhcpsrv/memfile_lease_limits.cc b/src/lib/dhcpsrv/memfile_lease_limits.cc index b1ba336921..f972eb6c9d 100644 --- a/src/lib/dhcpsrv/memfile_lease_limits.cc +++ b/src/lib/dhcpsrv/memfile_lease_limits.cc @@ -54,15 +54,23 @@ ClassLeaseCounter::adjustClassCount(const ClientClass& client_class, int offset, ConstElementPtr -ClassLeaseCounter::getLeaseClientClasses(LeasePtr lease) const { +ClassLeaseCounter::getLeaseClientClasses(LeasePtr lease) { + if (!lease) { + isc_throw(BadValue, "getLeaseClientCLasses - lease cannot be empty"); + } + ConstElementPtr classes; auto ctx = lease->getContext(); - if (ctx) { - classes = ctx->find("ISC/client-classes"); - if (classes && classes->getType() != Element::list) { - isc_throw(Unexpected, "getLeaseClientClasses: " - << lease->toText() << " is not a list!"); + try { + if (ctx) { + classes = ctx->find("ISC/client-classes"); + if (classes && classes->getType() != Element::list) { + isc_throw(BadValue, "client-classes is not a list"); + } } + } catch (const std::exception& ex) { + isc_throw(BadValue, "getLeaseClientClasses - invalid context: " + << data::prettyPrint(ctx) << ", " << ex.what()); } return (classes); @@ -84,7 +92,7 @@ ClassLeaseCounter::adjustClassCounts(ConstElementPtr classes, int offset, void ClassLeaseCounter::addLease(LeasePtr lease) { if (!lease) { - isc_throw(BadValue, "addLease(): lease cannot be empty"); + isc_throw(BadValue, "addLease - lease cannot be empty"); } ConstElementPtr classes = getLeaseClientClasses(lease); @@ -102,11 +110,11 @@ void ClassLeaseCounter::updateLease(LeasePtr new_lease, LeasePtr old_lease) { // Sanity checks. if (!new_lease) { - isc_throw(BadValue, "updateLease(): new_lease cannot be empty"); + isc_throw(BadValue, "updateLease - new_lease cannot be empty"); } if (!old_lease) { - isc_throw(BadValue, "updateLease(): old_lease cannot be empty"); + isc_throw(BadValue, "updateLease - old_lease cannot be empty"); } ConstElementPtr new_classes = getLeaseClientClasses(new_lease); @@ -135,7 +143,7 @@ ClassLeaseCounter::updateLease(LeasePtr new_lease, LeasePtr old_lease) { void ClassLeaseCounter::removeLease(LeasePtr lease) { if (!lease) { - isc_throw(BadValue, "removeLease(): lease cannot be empty"); + isc_throw(BadValue, "removeLease - lease cannot be empty"); } ConstElementPtr classes = getLeaseClientClasses(lease); diff --git a/src/lib/dhcpsrv/memfile_lease_limits.h b/src/lib/dhcpsrv/memfile_lease_limits.h index baa0583adf..79f62ad651 100644 --- a/src/lib/dhcpsrv/memfile_lease_limits.h +++ b/src/lib/dhcpsrv/memfile_lease_limits.h @@ -129,7 +129,7 @@ public: /// @param lease lease from which to fetch classes /// @return ElementPtr to an Element::List containing the client classes or an /// empty List. - data::ConstElementPtr getLeaseClientClasses(LeasePtr lease) const; + static data::ConstElementPtr getLeaseClientClasses(LeasePtr lease); private: /// @brief Fetches the map used to count the given lease type. diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index a69bcaca11..89f920360d 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -2191,7 +2191,7 @@ Memfile_LeaseMgr::checkLimits4(isc::data::ConstElementPtr const& user_context) c if (getLeaseLimit(subnet_elem, Lease::TYPE_V4, limit)) { // If the limit is > 0 look up the subnet lease count. Limit of 0 always // denies the lease. - auto lease_count = 0; + int64_t lease_count = 0; if (limit) { lease_count = getSubnetStat(subnet_id, "assigned-addresses"); } @@ -2211,7 +2211,7 @@ Memfile_LeaseMgr::checkLimits4(isc::data::ConstElementPtr const& user_context) c } std::string -Memfile_LeaseMgr:: checkLimits6(isc::data::ConstElementPtr const& user_context) const { +Memfile_LeaseMgr::checkLimits6(isc::data::ConstElementPtr const& user_context) const { if (!user_context) { return (""); } @@ -2231,7 +2231,7 @@ Memfile_LeaseMgr:: checkLimits6(isc::data::ConstElementPtr const& user_context) // Get class name. ConstElementPtr name_elem = class_elem->get("name"); if (!name_elem) { - isc_throw(BadValue, "client-class.name is missing: " + isc_throw(BadValue, "checkLimits6 - client-class.name is missing: " << prettyPrint(limits)); } @@ -2272,7 +2272,7 @@ Memfile_LeaseMgr:: checkLimits6(isc::data::ConstElementPtr const& user_context) // Get the subnet id. ConstElementPtr id_elem = subnet_elem->get("id"); if (!id_elem) { - isc_throw(BadValue, "subnet.id is missing: " + isc_throw(BadValue, "checkLimits6 - subnet.id is missing: " << prettyPrint(limits)); } @@ -2291,7 +2291,7 @@ Memfile_LeaseMgr:: checkLimits6(isc::data::ConstElementPtr const& user_context) // If the limit is > 0 look up the class lease count. Limit of 0 always // denies the lease. - size_t lease_count = 0; + int64_t lease_count = 0; if (limit) { lease_count = getSubnetStat(subnet_id, (ltype == Lease::TYPE_NA ? "assigned-nas" : "assigned-pds")); diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.h b/src/lib/dhcpsrv/memfile_lease_mgr.h index 5b77a92e4c..c9535d9991 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.h +++ b/src/lib/dhcpsrv/memfile_lease_mgr.h @@ -843,7 +843,7 @@ private: /// @brief Fetches the most recent value for a subnet statistic /// /// @param subnet_id subnet id of the subnet for which the stat is desired - /// @param stat_label name of the statisitic desired (e.g. "assigned-addresses") + /// @param stat_label name of the statistic desired (e.g. "assigned-addresses") /// /// @return Value of the statistic or zero if there are no entries found. int64_t getSubnetStat(const SubnetID& subnet_id, const std::string& stat_label) const; diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index 01a83dff27..e278eb735d 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -3906,15 +3906,6 @@ GenericLeaseMgrTest::testLeaseStatsQueryAttribution6() { void GenericLeaseMgrTest::testLeaseLimits4() { - // Create a subnet. We need the subnet to exist so statistics will exist. - CfgSubnets4Ptr cfg = CfgMgr::instance().getStagingCfg()->getCfgSubnets4(); - Subnet4Ptr subnet; - - subnet.reset(new Subnet4(IOAddress("192.0.1.0"), 24, 1, 2, 3, 1)); - cfg->add(subnet); - - ASSERT_NO_THROW(CfgMgr::instance().commit()); - std::string text; ElementPtr user_context; @@ -3963,15 +3954,6 @@ GenericLeaseMgrTest::testLeaseLimits4() { void GenericLeaseMgrTest::testLeaseLimits6() { - // Create a subnet. We need the subnet to exist so statistics will exist. - CfgSubnets4Ptr cfg = CfgMgr::instance().getStagingCfg()->getCfgSubnets4(); - Subnet4Ptr subnet; - - subnet.reset(new Subnet4(IOAddress("192.0.1.0"), 24, 1, 2, 3, 1)); - cfg->add(subnet); - - ASSERT_NO_THROW(CfgMgr::instance().commit()); - std::string text; ElementPtr user_context; diff --git a/src/lib/dhcpsrv/tests/memfile_lease_limits_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_limits_unittest.cc index 648294e6d8..abc26da151 100644 --- a/src/lib/dhcpsrv/tests/memfile_lease_limits_unittest.cc +++ b/src/lib/dhcpsrv/tests/memfile_lease_limits_unittest.cc @@ -195,7 +195,7 @@ public: /// #brief Test updating type of lease for given old and new states and class lists /// /// The test creates an old and new version of the same lease and passes them into - /// @c ClassLeaseCount::updateLease(). It then verifies the class lease counts + /// @c ClassLeaseCounter::updateLease(). It then verifies the class lease counts /// against expected count lists. /// /// @param old_state state of the old lease @@ -304,6 +304,26 @@ public: } + /// @brief Verifies ClassLeaseCounter::adjustClassCounts + /// + /// @param ltype type of lease to count. + void adjustClassCountsTest(const Lease::Type ltype) { + ConstElementPtr class_list1 = ctx1_->find("ISC/client-classes"); + ASSERT_TRUE(class_list1); + + // Increment the classes by 2 and verify. + ASSERT_NO_THROW_LOG(clc_.adjustClassCounts(class_list1, 2, ltype)); + checkClassCounts(classes1_, std::list({ 2, 2, 2 }), ltype); + + // Decrement the classes by 1 and verify. + ASSERT_NO_THROW_LOG(clc_.adjustClassCounts(class_list1, -1, ltype)); + checkClassCounts(classes1_, std::list({ 1, 1, 1 }), ltype); + + // Decrement the classes by 2 and verify that roll-over is avoided.. + ASSERT_NO_THROW_LOG(clc_.adjustClassCounts(class_list1, -2, ltype)); + checkClassCounts(classes1_, std::list({ 0, 0, 0 }), ltype); + } + /// @brief First test list of classes std::list classes1_; @@ -372,6 +392,19 @@ TEST_F(ClassLeaseCounterTest, basicCountingTests4) { EXPECT_EQ(39, count); } +// Tests getting and adjusting basic class counts for +// a Lease::TYPE_V4. +TEST_F(ClassLeaseCounterTest, adjustClassCountsTest4) { + adjustClassCountsTest(Lease::TYPE_V4); +} + +// Tests getting and adjusting basic class counts for +// a Lease::TYPE_NA and TYPE_PD. +TEST_F(ClassLeaseCounterTest, adjustClassCountsTest6) { + adjustClassCountsTest(Lease::TYPE_NA); + adjustClassCountsTest(Lease::TYPE_PD); +} + // Tests getting and adjusting basic class counts for // a Lease::TYPE_NA and TYPE_PD. TEST_F(ClassLeaseCounterTest, basicCountingTests6) { @@ -436,6 +469,110 @@ TEST_F(ClassLeaseCounterTest, basicCountingTests6) { EXPECT_EQ(0, count); } +// Exercises ClassLeaseCounter::getLeaseClientClasses() +// No need for v4 and v6 versions of this test, getLeaseClientClasses() +// is not protocol specific. +TEST_F(ClassLeaseCounterTest, getLeaseClientClassesTest) { + LeasePtr lease; + + // Describes an invalid context scenario, that + // is expected to cause an exception throw. + struct InvalidScenario { + std::string ctx_json_; // User context contents in JSON form. + std::string exp_message_; // Expected exception text. + }; + + // Invalid context scenarios. + std::list invalid_scenarios { + { + " \"bogus\" ", + "getLeaseClientClasses - invalid context: \"bogus\"," + " find(string) called on a non-map Element in (:1:2)" + }, + { + "{\"ISC\": \"bogus\" }", + "getLeaseClientClasses - invalid context: {\n \"ISC\": \"bogus\"\n}," + " find(string) called on a non-map Element in (:1:9)" + }, + { + "{\"ISC\": { \"client-classes\": \"bogus\" } }", + "getLeaseClientClasses - invalid context:" + " {\n \"ISC\": {\n \"client-classes\": \"bogus\"\n }\n}," + " client-classes is not a list" + } + }; + + // Iterate over the invalid scenarios. + for (auto scenario : invalid_scenarios) { + // Cosntruct the lease and context. + lease = leaseFactory(Lease::TYPE_V4); + ElementPtr ctx; + ASSERT_NO_THROW(ctx = Element::fromJSON(scenario.ctx_json_)) + << "test is broken" << scenario.ctx_json_; + lease->setContext(ctx); + + // Calling getLeaseClientClasses() should throw. + ASSERT_THROW_MSG(clc_.getLeaseClientClasses(lease), BadValue, scenario.exp_message_); + } + + // Describes an valid context scenario, that is expected + // to return normally. + struct ValidScenario { + std::string ctx_json_; // User context contents in JSON form. + std::string exp_classes_; // Expected class list in JSON form. + }; + + // Valid scenarios. + std::list valid_scenarios { + { + // No context + "", + "" + }, + { + // No client-classes element + "{\"ISC\": {} }", + "" + }, + { + // Empty client-classes list + "{\"ISC\": { \"client-classes\": []} }", + "[]" + }, + { + "{\"ISC\": { \"client-classes\": [ \"one\", \"two\", \"three\" ]} }", + "[ \"one\", \"two\", \"three\" ]" + } + }; + + // Iterate over the scenarios. + for (auto scenario : valid_scenarios) { + // Cosntruct the lease and context. + lease = leaseFactory(Lease::TYPE_V4); + if (!scenario.ctx_json_.empty()) { + ElementPtr ctx; + ASSERT_NO_THROW(ctx = Element::fromJSON(scenario.ctx_json_)) + << "test is broken" << scenario.ctx_json_; + lease->setContext(ctx); + } + + // Call getLeaseClientClasses(). + ConstElementPtr classes; + ASSERT_NO_THROW_LOG(classes = clc_.getLeaseClientClasses(lease)); + + // Verify we got the expected outcome for a class list. + if (scenario.exp_classes_.empty()) { + ASSERT_FALSE(classes); + } else { + ASSERT_TRUE(classes); + ElementPtr exp_classes; + ASSERT_NO_THROW(exp_classes = Element::fromJSON(scenario.exp_classes_)) + << "test is broken" << scenario.exp_classes_; + EXPECT_EQ(*classes, *exp_classes); + } + } +} + TEST_F(ClassLeaseCounterTest, addRemoveLeaseTest4) { addAndRemoveLeaseTest(leaseFactory(Lease::TYPE_V4)); }