From a49f3a04d02ef6fe43423eda313d9115588f0287 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Mon, 30 Aug 2021 08:45:39 -0700 Subject: [PATCH] Make calculate_usage() work if limits are missing The calculate_usage interface was added recently to allow consumers to probe limits and usage without requiring the enforce behavior workflow. If a limit was passed to it that was not registered in keystone, get_project_limits() would raise a ProjectOverLimit exception itself to abort the process immediately, providing the "unregistered means zero" behavior. This works fine for the enforce workflow, but not the calculate one. This changes get_project_limits() to just return a zero limit for a missing one, which will be considered by the enforce workflow in the same way, keeping the existing behavior. It will merely be reported by the calculate workflow, which is the desired change. Change-Id: Iaab1f0d5eb0da9a667267537d86f6c70bc8db51d --- doc/source/user/usage.rst | 5 ++- oslo_limit/limit.py | 16 +++------ oslo_limit/tests/test_limit.py | 65 ++++++++++++++++++++++------------ 3 files changed, 50 insertions(+), 36 deletions(-) diff --git a/doc/source/user/usage.rst b/doc/source/user/usage.rst index 8eca3e5..b62450b 100644 --- a/doc/source/user/usage.rst +++ b/doc/source/user/usage.rst @@ -146,7 +146,10 @@ Another usage pattern is to check a limit and usage for a given project, outside the scope of enforcement. This may be useful in a reporting API to be able to expose to a user the limit and usage information that the enforcer would use to judge a resource -consumption event. +consumption event. Any limit passed to this API which is not +registered in keystone will be considered to be zero, in keeping with +the behavior of the enforcer assuming that "unregistered means no +quota." .. note:: This should ideally not be used to provide your own enforcement of diff --git a/oslo_limit/limit.py b/oslo_limit/limit.py index 65c71eb..00260ee 100644 --- a/oslo_limit/limit.py +++ b/oslo_limit/limit.py @@ -280,29 +280,21 @@ class _EnforcerUtils(object): def get_project_limits(self, project_id, resource_names): """Get all the limits for given project a resource_name list - We will raise ClaimExceedsLimit if no limit is found to ensure that - all clients of this library react to this situation in the same way. + If a limit is not found, it will be considered to be zero + (i.e. no quota) :param project_id: :param resource_names: list of resource_name strings :return: list of (resource_name,limit) pairs - - :raises exception.ClaimExceedsLimit: if no limit is found """ # Using a list to preserver the resource_name order project_limits = [] - missing_limits = [] for resource_name in resource_names: try: limit = self._get_limit(project_id, resource_name) - project_limits.append((resource_name, limit)) except _LimitNotFound: - missing_limits.append(resource_name) - - if len(missing_limits) > 0: - over_limit_list = [exception.OverLimitInfo(name, 0, 0, 0) - for name in missing_limits] - raise exception.ProjectOverLimit(project_id, over_limit_list) + limit = 0 + project_limits.append((resource_name, limit)) return project_limits diff --git a/oslo_limit/tests/test_limit.py b/oslo_limit/tests/test_limit.py index 4c3d7ff..f38108d 100644 --- a/oslo_limit/tests/test_limit.py +++ b/oslo_limit/tests/test_limit.py @@ -143,6 +143,38 @@ class TestEnforcer(base.BaseTestCase): self.assertEqual(expected, enforcer.calculate_usage(project_id, ['a', 'b'])) + @mock.patch.object(limit._EnforcerUtils, "_get_project_limit") + @mock.patch.object(limit._EnforcerUtils, "_get_registered_limit") + def test_calculate_and_enforce_some_missing(self, mock_get_reglimit, + mock_get_limit): + # Registered and project limits for a and b, c is unregistered + reg_limits = {'a': mock.MagicMock(default_limit=10), + 'b': mock.MagicMock(default_limit=10)} + prj_limits = {('bar', 'b'): mock.MagicMock(resource_limit=6)} + mock_get_reglimit.side_effect = lambda r: reg_limits.get(r) + mock_get_limit.side_effect = lambda p, r: prj_limits.get((p, r)) + + # Regardless, we have usage for all three + mock_usage = mock.MagicMock() + mock_usage.return_value = {'a': 5, 'b': 5, 'c': 5} + + enforcer = limit.Enforcer(mock_usage) + + # When we calculate usage, we should expect the default limit + # of zero for the unregistered limit + expected = { + 'a': limit.ProjectUsage(10, 5), + 'b': limit.ProjectUsage(6, 5), + 'c': limit.ProjectUsage(0, 5), + } + self.assertEqual(expected, + enforcer.calculate_usage('bar', ['a', 'b', 'c'])) + + # Make sure that if we enforce, we get the expected behavior + # of c being considered to be zero + self.assertRaises(exception.ProjectOverLimit, + enforcer.enforce, 'bar', {'a': 1, 'b': 0, 'c': 1}) + def test_calculate_usage_bad_params(self): enforcer = limit.Enforcer(mock.MagicMock()) @@ -216,14 +248,17 @@ class TestFlatEnforcer(base.BaseTestCase): self.assertEqual(0, over_a.current_usage) self.assertEqual(2, over_a.delta) - @mock.patch.object(limit._EnforcerUtils, "get_project_limits") - def test_enforce_raises_on_missing_limit(self, mock_get_limits): - mock_usage = mock.MagicMock() + @mock.patch.object(limit._EnforcerUtils, "_get_project_limit") + @mock.patch.object(limit._EnforcerUtils, "_get_registered_limit") + def test_enforce_raises_on_missing_limit(self, mock_get_reglimit, + mock_get_limit): + def mock_usage(*a): + return {'a': 1, 'b': 1} project_id = uuid.uuid4().hex deltas = {"a": 0, "b": 0} - mock_get_limits.side_effect = exception.ProjectOverLimit( - project_id, [exception.OverLimitInfo("a", 0, 0, 0)]) + mock_get_reglimit.return_value = None + mock_get_limit.return_value = None enforcer = limit._FlatEnforcer(mock_usage) self.assertRaises(exception.ProjectOverLimit, enforcer.enforce, @@ -292,24 +327,8 @@ class TestEnforcerUtils(base.BaseTestCase): limits = utils.get_project_limits(project_id, ["a", "b"]) self.assertEqual([('a', 1), ('b', 2)], limits) - e = self.assertRaises(exception.ProjectOverLimit, - utils.get_project_limits, - project_id, ["c", "d"]) - expected = ("Project %s is over a limit for " - "[Resource c is over limit of 0 due to current usage 0 " - "and delta 0, " - "Resource d is over limit of 0 due to current usage 0 " - "and delta 0]") - self.assertEqual(expected % project_id, str(e)) - self.assertEqual(project_id, e.project_id) - self.assertEqual(2, len(e.over_limit_info_list)) - over_c = e.over_limit_info_list[0] - self.assertEqual("c", over_c.resource_name) - over_d = e.over_limit_info_list[1] - self.assertEqual("d", over_d.resource_name) - self.assertEqual(0, over_d.limit) - self.assertEqual(0, over_d.current_usage) - self.assertEqual(0, over_d.delta) + limits = utils.get_project_limits(project_id, ["c", "d"]) + self.assertEqual([('c', 0), ('d', 0)], limits) def test_get_limit_cache(self, cache=True): # No project limit and registered limit = 5 -- GitLab