diff --git a/lower-constraints.txt b/lower-constraints.txt index 7bf2e20..42e0585 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -1,6 +1,7 @@ bandit==1.4.0 hacking==0.12.0 keystoneauth1==3.9.0 +mock==3.0.0 oslo.config==5.2.0 oslo.i18n==3.15.3 oslo.log==3.44.0 @@ -8,5 +9,6 @@ openstackdocstheme==1.20.0 openstacksdk==0.31.1 oslotest==3.2.0 reno==2.5.0 +six==1.10.0 Sphinx==1.8.0 stestr==1.0.0 diff --git a/oslo_limit/exception.py b/oslo_limit/exception.py index bb84d21..3f1d0ec 100644 --- a/oslo_limit/exception.py +++ b/oslo_limit/exception.py @@ -15,21 +15,49 @@ from oslo_limit._i18n import _ +class ProjectOverLimit(Exception): + def __init__(self, project_id, over_limit_info_list): + """Exception raised when a project goes over one or more limits + + :param project_id: the project id + :param over_limit_info_list: list of OverLimitInfo objects + """ + if not isinstance(over_limit_info_list, list): + raise ValueError(over_limit_info_list) + if len(over_limit_info_list) == 0: + raise ValueError(over_limit_info_list) + for info in over_limit_info_list: + if not isinstance(info, OverLimitInfo): + raise ValueError(over_limit_info_list) + + self.project_id = project_id + self.over_limit_info_list = over_limit_info_list + msg = _("Project %(project_id)s is over a limit for " + "%(limits)s") % {'project_id': project_id, + 'limits': over_limit_info_list} + super(ProjectOverLimit, self).__init__(msg) + + +class OverLimitInfo(object): + def __init__(self, resource_name, limit, current_usage, delta): + self.resource_name = resource_name + self.limit = int(limit) + self.current_usage = int(current_usage) + self.delta = int(delta) + + def __str__(self): + template = ("Resource %s is over limit of %s due to " + "current usage %s and delta %s") + return template % (self.resource_name, self.limit, + self.current_usage, self.delta) + + def __repr__(self): + return self.__str__() + + class SessionInitError(Exception): def __init__(self, reason): msg = _( "Can't initialise OpenStackSDK session: %(reason)s." ) % {'reason': reason} super(SessionInitError, self).__init__(msg) - - -class LimitNotFound(Exception): - def __init__(self, resource, service, region): - msg = _("Can't find the limit for resource %(resource)s " - "for service %(service)s in region %(region)s." - ) % { - 'resource': resource, 'service': service, 'region': region} - self.resource = resource - self.service = service - self.region = region - super(LimitNotFound, self).__init__(msg) diff --git a/oslo_limit/limit.py b/oslo_limit/limit.py index fbc5210..cd3d666 100644 --- a/oslo_limit/limit.py +++ b/oslo_limit/limit.py @@ -57,27 +57,25 @@ class Enforcer(object): :param usage_callback: A callable function that accepts a project_id string as a parameter and calculates the current usage of a resource. - :type callable function: - + :type usage_callback: callable function """ if not callable(usage_callback): msg = 'usage_callback must be a callable function.' raise ValueError(msg) - self.usage_callback = usage_callback self.connection = _get_keystone_connection() - self.model = self._get_model_impl() + self.model = self._get_model_impl(usage_callback) def _get_enforcement_model(self): """Query keystone for the configured enforcement model.""" return self.connection.get('/limits/model').json()['model']['name'] - def _get_model_impl(self): + def _get_model_impl(self, usage_callback): """get the enforcement model based on configured model in keystone.""" model = self._get_enforcement_model() for impl in _MODELS: if model == impl.name: - return impl() + return impl(usage_callback) raise ValueError("enforcement model %s is not supported" % model) def enforce(self, project_id, deltas): @@ -110,16 +108,17 @@ class Enforcer(object): Specify a quantity of zero to check current usage. :type deltas: dictionary + :raises exception.ClaimExceedsLimit: when over limits """ - if not isinstance(project_id, six.text_type): - msg = 'project_id must be a string.' + if not project_id or not isinstance(project_id, six.string_types): + msg = 'project_id must be a non-empty string.' raise ValueError(msg) - if not isinstance(deltas, dict): - msg = 'deltas must be a dictionary.' + if not isinstance(deltas, dict) or len(deltas) == 0: + msg = 'deltas must be a non-empty dictionary.' raise ValueError(msg) - for k, v in iter(deltas.items()): - if not isinstance(k, six.text_type): + for k, v in deltas.items(): + if not isinstance(k, six.string_types): raise ValueError('resource name is not a string.') elif not isinstance(v, int): raise ValueError('resource limit is not an integer.') @@ -131,14 +130,30 @@ class _FlatEnforcer(object): name = 'flat' + def __init__(self, usage_callback): + self._usage_callback = usage_callback + self._utils = _EnforcerUtils() + def enforce(self, project_id, deltas): - raise NotImplementedError() + resources_to_check = list(deltas.keys()) + # Always check the limits in the same order, for predictable errors + resources_to_check.sort() + + project_limits = self._utils.get_project_limits(project_id, + resources_to_check) + current_usage = self._usage_callback(project_id, resources_to_check) + + self._utils.enforce_limits(project_id, project_limits, + current_usage, deltas) class _StrictTwoLevelEnforcer(object): name = 'strict-two-level' + def __init__(self, usage_callback): + self._usage_callback = usage_callback + def enforce(self, project_id, deltas): raise NotImplementedError() @@ -146,6 +161,14 @@ class _StrictTwoLevelEnforcer(object): _MODELS = [_FlatEnforcer, _StrictTwoLevelEnforcer] +class _LimitNotFound(Exception): + def __init__(self, resource): + msg = "Can't find the limit for resource %(resource)s" % { + 'resource': resource} + self.resource = resource + super(_LimitNotFound, self).__init__(msg) + + class _EnforcerUtils(object): """Logic common used by multiple enforcers""" @@ -160,21 +183,61 @@ class _EnforcerUtils(object): self._service_id = self._endpoint.service_id self._region_id = self._endpoint.region_id + @staticmethod + def enforce_limits(project_id, limits, current_usage, deltas): + """Check that proposed usage is not over given limits + + :param project_id: project being checked + :param limits: list of (resource_name,limit) pairs + :param current_usage: dict of resource name and current usage + :param deltas: dict of resource name and proposed additional usage + + :raises exception.ClaimExceedsLimit: raise if over limit + """ + over_limit_list = [] + for resource_name, limit in limits: + if resource_name not in current_usage: + msg = "unable to get current usage for %s" % resource_name + raise ValueError(msg) + + current = int(current_usage[resource_name]) + delta = int(deltas[resource_name]) + proposed_usage_total = current + delta + if proposed_usage_total > limit: + over_limit_list.append(exception.OverLimitInfo( + resource_name, limit, current, delta)) + + if len(over_limit_list) > 0: + LOG.debug("hit limit for project: %s", over_limit_list) + raise exception.ProjectOverLimit(project_id, over_limit_list) + def get_project_limits(self, project_id, resource_names): """Get all the limits for given project a resource_name list - We will raise + 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. + :param project_id: :param resource_names: list of resource_name strings :return: list of (resource_name,limit) pairs - :raises exception.LimitNotFound if no limit is found + :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: - limit = self._get_limit(project_id, resource_name) - project_limits.append((resource_name, limit)) + 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) + return project_limits def _get_limit(self, project_id, resource_name): @@ -187,8 +250,12 @@ class _EnforcerUtils(object): if registered_limit: return registered_limit.default_limit - raise exception.LimitNotFound( - resource_name, self._service_id, self._region_id) + LOG.error( + "Unable to find registered limit for resource " + "%(resource)s for %(service)s in region %(region)s.", + {"resource": resource_name, "service": self._service_id, + "region": self._region_id}, exec_info=False) + raise _LimitNotFound(resource_name) def _get_project_limit(self, project_id, resource_name): limit = self.connection.limits( diff --git a/oslo_limit/tests/test_limit.py b/oslo_limit/tests/test_limit.py index 44f50c2..0ff231e 100644 --- a/oslo_limit/tests/test_limit.py +++ b/oslo_limit/tests/test_limit.py @@ -15,7 +15,6 @@ """ test_limit ---------------------------------- - Tests for `limit` module. """ @@ -57,12 +56,8 @@ class TestEnforcer(base.BaseTestCase): json.json.return_value = {"model": {"name": "flat"}} limit._SDK_CONNECTION.get.return_value = json - def _get_usage_for_project(self, project_id): - return 8 - - def test_required_parameters(self): - enforcer = limit.Enforcer(self._get_usage_for_project) - self.assertEqual(self._get_usage_for_project, enforcer.usage_callback) + def _get_usage_for_project(self, project_id, resource_names): + return {"a": 1} def test_usage_callback_must_be_callable(self): invalid_callback_types = [uuid.uuid4().hex, 5, 5.1] @@ -76,7 +71,7 @@ class TestEnforcer(base.BaseTestCase): def test_deltas_must_be_a_dictionary(self): project_id = uuid.uuid4().hex - invalid_delta_types = [uuid.uuid4().hex, 5, 5.1, True, False, []] + invalid_delta_types = [uuid.uuid4().hex, 5, 5.1, True, [], None, {}] enforcer = limit.Enforcer(self._get_usage_for_project) for invalid_delta in invalid_delta_types: @@ -87,6 +82,17 @@ class TestEnforcer(base.BaseTestCase): invalid_delta ) + def test_project_id_must_be_a_string(self): + enforcer = limit.Enforcer(self._get_usage_for_project) + invalid_delta_types = [{}, 5, 5.1, True, False, [], None, ""] + for invalid_project_id in invalid_delta_types: + self.assertRaises( + ValueError, + enforcer.enforce, + invalid_project_id, + {} + ) + def test_set_model_impl(self): enforcer = limit.Enforcer(self._get_usage_for_project) self.assertIsInstance(enforcer.model, limit._FlatEnforcer) @@ -97,17 +103,88 @@ class TestEnforcer(base.BaseTestCase): json.json.return_value = {"model": {"name": "flat"}} enforcer = limit.Enforcer(self._get_usage_for_project) - flat_impl = enforcer._get_model_impl() + flat_impl = enforcer._get_model_impl(self._get_usage_for_project) self.assertIsInstance(flat_impl, limit._FlatEnforcer) json.json.return_value = {"model": {"name": "strict-two-level"}} - flat_impl = enforcer._get_model_impl() + flat_impl = enforcer._get_model_impl(self._get_usage_for_project) self.assertIsInstance(flat_impl, limit._StrictTwoLevelEnforcer) json.json.return_value = {"model": {"name": "foo"}} - e = self.assertRaises(ValueError, enforcer._get_model_impl) + e = self.assertRaises(ValueError, enforcer._get_model_impl, + self._get_usage_for_project) self.assertEqual("enforcement model foo is not supported", str(e)) + @mock.patch.object(limit._FlatEnforcer, "enforce") + def test_enforce(self, mock_enforce): + enforcer = limit.Enforcer(self._get_usage_for_project) + project_id = uuid.uuid4().hex + deltas = {"a": 1} + + enforcer.enforce(project_id, deltas) + + mock_enforce.assert_called_once_with(project_id, deltas) + + +class TestFlatEnforcer(base.BaseTestCase): + def setUp(self): + super(TestFlatEnforcer, self).setUp() + self.mock_conn = mock.MagicMock() + limit._SDK_CONNECTION = self.mock_conn + + @mock.patch.object(limit._EnforcerUtils, "get_project_limits") + def test_enforce(self, mock_get_limits): + mock_usage = mock.MagicMock() + + project_id = uuid.uuid4().hex + deltas = {"a": 1, "b": 1} + mock_get_limits.return_value = [("a", 1), ("b", 2)] + mock_usage.return_value = {"a": 0, "b": 1} + + enforcer = limit._FlatEnforcer(mock_usage) + enforcer.enforce(project_id, deltas) + + self.mock_conn.get_endpoint.assert_called_once_with(None) + mock_get_limits.assert_called_once_with(project_id, ["a", "b"]) + mock_usage.assert_called_once_with(project_id, ["a", "b"]) + + @mock.patch.object(limit._EnforcerUtils, "get_project_limits") + def test_enforce_raises_on_over(self, mock_get_limits): + mock_usage = mock.MagicMock() + + project_id = uuid.uuid4().hex + deltas = {"a": 2, "b": 1} + mock_get_limits.return_value = [("a", 1), ("b", 2)] + mock_usage.return_value = {"a": 0, "b": 1} + + enforcer = limit._FlatEnforcer(mock_usage) + e = self.assertRaises(exception.ProjectOverLimit, enforcer.enforce, + project_id, deltas) + expected = ("Project %s is over a limit for " + "[Resource a is over limit of 1 due to current usage 0 " + "and delta 2]") + self.assertEqual(expected % project_id, str(e)) + self.assertEqual(project_id, e.project_id) + self.assertEqual(1, len(e.over_limit_info_list)) + over_a = e.over_limit_info_list[0] + self.assertEqual("a", over_a.resource_name) + self.assertEqual(1, over_a.limit) + 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() + + 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)]) + + enforcer = limit._FlatEnforcer(mock_usage) + self.assertRaises(exception.ProjectOverLimit, enforcer.enforce, + project_id, deltas) + class TestEnforcerUtils(base.BaseTestCase): def setUp(self): @@ -149,30 +226,43 @@ class TestEnforcerUtils(base.BaseTestCase): self.mock_conn.get_endpoint.return_value = fake_endpoint project_id = uuid.uuid4().hex - # a is a project limit, b and c don't have one + # a is a project limit, b, c and d don't have one empty_iterator = iter([]) a = klimit.Limit() a.resource_name = "a" a.resource_limit = 1 a_iterator = iter([a]) self.mock_conn.limits.side_effect = [a_iterator, empty_iterator, - empty_iterator] + empty_iterator, empty_iterator] - # b has a limit, but c doesn't, a isn't ever checked + # b has a limit, but c and d doesn't, a isn't ever checked b = registered_limit.RegisteredLimit() b.resource_name = "b" b.default_limit = 2 b_iterator = iter([b]) self.mock_conn.registered_limits.side_effect = [b_iterator, + empty_iterator, empty_iterator] utils = limit._EnforcerUtils() limits = utils.get_project_limits(project_id, ["a", "b"]) self.assertEqual([('a', 1), ('b', 2)], limits) - e = self.assertRaises(exception.LimitNotFound, + e = self.assertRaises(exception.ProjectOverLimit, utils.get_project_limits, - project_id, ["c"]) - self.assertEqual("c", e.resource) - self.assertEqual("service_id", e.service) - self.assertEqual("region_id", e.region) + 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) diff --git a/requirements.txt b/requirements.txt index f523f83..18e6e7f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,6 +2,7 @@ # of appearance. Changing the order has an impact on the overall integration # process, which may cause wedges in the gate later. keystoneauth1>=3.9.0 # Apache-2.0 +six>=1.10.0 # MIT oslo.config>=5.2.0 # Apache-2.0 oslo.i18n>=3.15.3 # Apache-2.0 oslo.log>=3.44.0 # Apache-2.0 diff --git a/test-requirements.txt b/test-requirements.txt index 838e1e7..1745b4b 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -2,6 +2,7 @@ # of appearance. Changing the order has an impact on the overall integration # process, which may cause wedges in the gate later. +mock>=3.0.0 # BSD hacking>=1.1.0,<1.2.0 # Apache-2.0 oslotest>=3.2.0 # Apache-2.0 stestr>=1.0.0 # Apache-2.0