From 42a59253e3eb8e715515cbf5cd694bfa5a10f468 Mon Sep 17 00:00:00 2001 From: Sven Rosenzweig Date: Mon, 23 Jun 2025 13:54:53 +0200 Subject: [PATCH 1/4] Return correct Retry_After_Header value If we do not want to suspend requests (max_sleep_time = 0), the lua scripts, calculating the rate limit, always returns 0 for the Retry-After Header value. Tools like terraform/OpenTofu will not slow down their deployment process, but instead simply abort. With this commit the lua scripts returns the correct value rather than 2 * max_sleep_time. Additionally we ensure never to return zero to the client and log a warning if the lua script calculates zero as value. This commit adds three test cases for the rate limit calculus: 1. No limit reached 2. Limit reached and suspension 3. Limit reached and no suspension --- rate_limit/backend.py | 8 ++- rate_limit/common.py | 1 - rate_limit/lua/redis_sliding_window.lua | 4 +- rate_limit/tests/test_ratelimit_algorithm.py | 67 ++++++++++++++++++++ 4 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 rate_limit/tests/test_ratelimit_algorithm.py diff --git a/rate_limit/backend.py b/rate_limit/backend.py index 3e81383..e824732 100644 --- a/rate_limit/backend.py +++ b/rate_limit/backend.py @@ -220,13 +220,14 @@ def __rate_limit(self, key, window_seconds, max_calls, max_rate_string): # Parse result list safely. remaining = common.listitem_to_int(result, idx=0) retry_after_seconds = common.listitem_to_int(result, idx=1) + retry = remaining < 0 # Return here if we still have remaining requests. if remaining > 0: return None # Suspend the current request if its it has to wait no longer than max_sleep_time_seconds. - elif retry_after_seconds < self.__max_sleep_time_seconds: + elif retry and retry_after_seconds < self.__max_sleep_time_seconds: # Log the current request if it has to be suspended for at least log_sleep_time_seconds. if retry_after_seconds >= self.__log_sleep_time_seconds: self.logger.debug( @@ -236,6 +237,11 @@ def __rate_limit(self, key, window_seconds, max_calls, max_rate_string): eventlet.sleep(retry_after_seconds) return None + # Tools like opentofu/terraform do not retry but error out when response header returns retry_after 0 + if retry_after_seconds == 0: + self.logger.warning(f"Not rate limiting request as retry_after_seconds is 0. Remaining: {remaining}") + return None + # If rate limit exceeded and the request cannot be suspended return the rate limit response. # Set headers for rate limit response. self.__rate_limit_response.set_headers( diff --git a/rate_limit/common.py b/rate_limit/common.py index 79900c5..5f3ef9a 100644 --- a/rate_limit/common.py +++ b/rate_limit/common.py @@ -199,7 +199,6 @@ def listitem_to_int(listthing, idx, default=0): except (IndexError, TypeError): return default - def load_lua_script(filename, foldername="lua"): """ Load the specified LUA script. diff --git a/rate_limit/lua/redis_sliding_window.lua b/rate_limit/lua/redis_sliding_window.lua index e0c57f7..813f9b0 100644 --- a/rate_limit/lua/redis_sliding_window.lua +++ b/rate_limit/lua/redis_sliding_window.lua @@ -42,9 +42,9 @@ if (retry_after_seconds < max_sleep_time_seconds_int) and (remaining - 1 >= -max -- Reset expiry time for key. redis.call('expire', key, window_seconds_int) -- Return if the request can be suspended. - return {remaining - 1 , retry_after_seconds} + return {remaining - 1, retry_after_seconds} end -- Return if no more remaining requests and suspending request not possible. -- Ensure the 2nd argument (retry_after is greater than max_sleep_time_seconds_int) -return {0, 2 * max_sleep_time_seconds_int} +return {0, retry_after_seconds} diff --git a/rate_limit/tests/test_ratelimit_algorithm.py b/rate_limit/tests/test_ratelimit_algorithm.py new file mode 100644 index 0000000..24c0105 --- /dev/null +++ b/rate_limit/tests/test_ratelimit_algorithm.py @@ -0,0 +1,67 @@ +import eventlet +import json +import os +import random +import unittest + +from rate_limit.backend import RedisBackend +from rate_limit.response import RateLimitExceededResponse +from unittest.mock import MagicMock + +class TestRateLimitAlgorithm(unittest.TestCase): + def setUp(self): + self.configure_connection(max_sleep_time_seconds=600) + + def configure_connection(self, max_sleep_time_seconds): + ratelimit_response = RateLimitExceededResponse( + status="429", status_code=429, headerlist=[], body=None, json_body=None + ) + host = os.getenv("REDIS_HOST", "localhost") + port = os.getenv("REDIS_PORT", 6379) + + self.backend = RedisBackend(host=host, + port=port, + password=None, + rate_limit_response=ratelimit_response, + max_sleep_time_seconds=max_sleep_time_seconds, + log_sleep_time_seconds=0, ) + + if not self.backend.is_available()[0]: + raise RuntimeError(f"Redis backend {host}:{port} is not available") + + def test_rate_limit_hit(self): + rand = random.randint(1, 1000) + target_type = f"port-x{str(rand)}" + + response = self.backend.rate_limit('local', "create", target_type, '100r/m', ) + self.assertIsNone(response, "Expected response not to be limited") + + def test_rate_limit_hit_not_suspension(self): + eventlet.sleep = MagicMock() + rand = random.randint(1, 1000) + target_type = f"port-y{str(rand)}" + + # First request should not hit the rate limit + resp1 = self.backend.rate_limit('local', "suspend", target_type, '1r/m', ) + resp2 = self.backend.rate_limit('local', "suspend", target_type, '1r/m', ) + + self.assertEqual(True, eventlet.sleep.called) + self.assertIsNone(resp1, "Expected response not to be limited") + self.assertIsNone(resp2, "Expected response not to be limited") + + def test_rate_limit_hit_not_suspended(self): + #Override connection with new max sleep time + self.configure_connection(max_sleep_time_seconds=0) + rand = random.randint(1, 1000) + target_type = f"port-z{str(rand)}" + + # First request should not hit the rate limit + resp1 = self.backend.rate_limit('local', "suspend", target_type, '1r/m', ) + resp2 = self.backend.rate_limit('local', "suspend", target_type, '1r/m', ) + + if not resp2: + self.assertIsNotNone(resp2, "Expected response to be rate limited") + + body = json.loads(resp2.json_body) + self.assertIsNone(resp1, "Expected response not to be limited") + self.assertEqual(body["error"]["message"], "Too Many Requests") From 3365907de366ad1cb4febe2668e023df1ebc70ab Mon Sep 17 00:00:00 2001 From: Sven Rosenzweig Date: Mon, 23 Jun 2025 14:34:51 +0200 Subject: [PATCH 2/4] Add Github workflow --- .github/workflows/run-tox.yml | 47 +++++++++++++++++++++++++++++++++++ test-requirements.txt | 1 + tox.ini | 15 ++++++----- 3 files changed, 57 insertions(+), 6 deletions(-) create mode 100644 .github/workflows/run-tox.yml diff --git a/.github/workflows/run-tox.yml b/.github/workflows/run-tox.yml new file mode 100644 index 0000000..de27354 --- /dev/null +++ b/.github/workflows/run-tox.yml @@ -0,0 +1,47 @@ +name: run-tox +on: + push: + branches: + - master + pull_request: +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + python: ['3.10'] + services: + redis: + image: redis + ports: + - 6379:6379 + options: >- + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + steps: + - uses: actions/checkout@v2 + - name: Setup python + uses: actions/setup-python@v2 + with: + python-version: ${{ matrix.python }} + - name: Install Tox + run: pip install tox + - name: Run Tox + run: "tox -e py" + env: + REDIS_HOST: redis + REDIS_PORT: 6379 + pep8: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: Setup python + uses: actions/setup-python@v2 + with: + python-version: ${{ matrix.python }} + - name: Install Tox + run: pip install tox + - name: Run Tox pep8 + run: "tox -e pep8" diff --git a/test-requirements.txt b/test-requirements.txt index df4b868..8e8c95d 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -8,3 +8,4 @@ testrepository>=0.0.18 # Apache-2.0/BSD testtools>=1.4.0 # MIT os-testr>=0.8.0 # Apache-2.0 WebTest>=2.0 # MIT +stestr>=1.0.0 # Apache-2.0 diff --git a/tox.ini b/tox.ini index 7b63590..a5df9dd 100644 --- a/tox.ini +++ b/tox.ini @@ -2,22 +2,23 @@ minversion = 2.0 # avoid sdist skipsdist = True -envlist = py35,py36,py37,pep8 +envlist = py310,pep8 [testenv] -usedevelop = True -install_command = {toxinidir}/tools/tox_install.sh {env:UPPER_CONSTRAINTS_FILE:https://raw.githubusercontent.com/sapcc/requirements/stable/rocky-m3/upper-constraints.txt} {opts} {packages} +install_command = pip install -c {env:UPPER_CONSTRAINTS_FILE:https://raw.githubusercontent.com/sapcc/requirements/stable/2024.1-m3/upper-constraints.txt} -U {opts} {packages} setenv = VIRTUAL_ENV={envdir} + PYTHONWARNINGS=default::DeprecationWarning BRANCH_NAME=master CLIENT_NAME=rate-limit OS_STDOUT_CAPTURE=1 OS_STDERR_CAPTURE=1 OS_TEST_TIMEOUT=60 TESTS_DIR=./rate_limit/tests/ - deps = -r{toxinidir}/requirements.txt -r{toxinidir}/test-requirements.txt +whitelist_externals = sh commands = stestr run {posargs} +download = True [testenv:pep8] commands = flake8 @@ -26,10 +27,12 @@ commands = flake8 commands = {posargs} [flake8] -ignore = D100,D101,D102,D103,D104,D203,D205,W605 show-source = True -exclude = .venv,.tox,dist,doc,*egg,build,test* max-line-length = 140 +ignore = D100,D101,D102,D103,D104,D107,D203,D205,E123,E125,E402,E741,W503,W504,W605, H301 +builtins = _ +exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,build + [testenv:docs] deps = -r{toxinidir}/doc/requirements.txt From a927ac1d087757c052bf3313b8c8520e2555a18b Mon Sep 17 00:00:00 2001 From: Sven Rosenzweig Date: Tue, 24 Jun 2025 12:24:32 +0200 Subject: [PATCH 3/4] Remove flake8 errors --- rate_limit/common.py | 6 +++--- rate_limit/log.py | 4 +--- rate_limit/provider.py | 7 ++----- rate_limit/rate_limit.py | 7 +++++-- rate_limit/tests/fake.py | 2 +- rate_limit/tests/test_actiongroups.py | 4 ++-- rate_limit/tests/test_middleware.py | 8 ++++---- rate_limit/tests/test_parse_config.py | 6 +++--- rate_limit/tests/test_ratelimit_algorithm.py | 2 +- rate_limit/tests/test_response.py | 15 ++++++++++----- rate_limit/tests/test_units.py | 2 +- rate_limit/utils.py | 2 +- test-requirements.txt | 2 +- 13 files changed, 35 insertions(+), 32 deletions(-) diff --git a/rate_limit/common.py b/rate_limit/common.py index 5f3ef9a..1e98b0f 100644 --- a/rate_limit/common.py +++ b/rate_limit/common.py @@ -33,9 +33,8 @@ class Constants(object): - """ - Common constants used in various places. - """ + """Common constants used in various places.""" + ratelimit_response = 'ratelimit_response' blacklist_response = 'blacklist_response' max_sleep_time_seconds = 'max_sleep_time_seconds' @@ -199,6 +198,7 @@ def listitem_to_int(listthing, idx, default=0): except (IndexError, TypeError): return default + def load_lua_script(filename, foldername="lua"): """ Load the specified LUA script. diff --git a/rate_limit/log.py b/rate_limit/log.py index 7fa288b..c5ecb78 100644 --- a/rate_limit/log.py +++ b/rate_limit/log.py @@ -21,10 +21,8 @@ class Logger(object): - """ - Logger that attempts to log and ignores any error. + """Logger that attempts to log and ignores any error.""" - """ def __init__(self, name, product_name='rate_limit'): self.__logger = logging.getLogger(name) try: diff --git a/rate_limit/provider.py b/rate_limit/provider.py index c4223bd..9fd925f 100644 --- a/rate_limit/provider.py +++ b/rate_limit/provider.py @@ -116,13 +116,11 @@ def get_local_rate_limits(self, scope, action, target_type_uri, **kwargs): return -1 def _get_wildcard_ratelimits(self, ratelimits, target_type_uri): - """ - Get the target type URI rate limits from wildcard pattern. + """Get the target type URI rate limits from wildcard pattern. :param target_type_uri: the target type URI of the request :return: target type uri ratelimits if exists """ - ttu_ratelimits = [] pattern_list = [ lr_key for lr_key in ratelimits @@ -136,14 +134,13 @@ def _get_wildcard_ratelimits(self, ratelimits, target_type_uri): def _match(self, uri, pattern_list): """ - Check if a URI matches to one of the patterns + Check if a URI matches to one of the patterns. :param uri: URI to check if it matches to one of the patterns :param pattern_list : patterns to match against the URI :return: True if path matches a pattern of the list and pattern as key for self.local_ratelimits. """ - for pattern in pattern_list: if uri.startswith(pattern[:-1]): return True, pattern diff --git a/rate_limit/rate_limit.py b/rate_limit/rate_limit.py index 66b2c80..6dbd978 100644 --- a/rate_limit/rate_limit.py +++ b/rate_limit/rate_limit.py @@ -160,7 +160,7 @@ def __init__(self, app, **conf): self.logger.info("OpenStack Rate Limit Middleware ready for requests.") def _setup_response(self): - """Setup configurable RateLimitExceededResponse and BlacklistResponse.""" + """Set up configurable RateLimitExceededResponse and BlacklistResponse.""" # Default responses. ratelimit_response = response.RateLimitExceededResponse() blacklist_response = response.BlacklistResponse() @@ -199,7 +199,10 @@ def _setup_response(self): self.blacklist_response = blacklist_response def __setup_limes_ratelimit_provider(self): - """Setup Limes as provider for rate limits. If not successful fallback to configuration file.""" + """Set up Limes as provider for rate limits. + + If not successful fallback to configuration file. + """ try: limes_ratelimit_provider = provider.LimesRateLimitProvider( service_type=self.service_type, diff --git a/rate_limit/tests/fake.py b/rate_limit/tests/fake.py index 4ec5370..ac92dc5 100644 --- a/rate_limit/tests/fake.py +++ b/rate_limit/tests/fake.py @@ -36,7 +36,7 @@ def incr(self, key, delta=1, time=0): def decr(self, key, delta=1, time=0): return self.incr(key, delta=-delta, time=time) - def delete(self,key): + def delete(self, key): try: del self.store[key] except KeyError: diff --git a/rate_limit/tests/test_actiongroups.py b/rate_limit/tests/test_actiongroups.py index 0b1d788..4660196 100644 --- a/rate_limit/tests/test_actiongroups.py +++ b/rate_limit/tests/test_actiongroups.py @@ -54,7 +54,7 @@ def test_groups(self): - read - read/list - read/*/list -""", +""", # noqa rl_groups ) ) @@ -95,7 +95,7 @@ def test_mapping(self): }, { 'action': 'read/rules/list', - 'expected':'read/rules/list', + 'expected': 'read/rules/list', }, ] diff --git a/rate_limit/tests/test_middleware.py b/rate_limit/tests/test_middleware.py index 09c33f6..f4934a4 100644 --- a/rate_limit/tests/test_middleware.py +++ b/rate_limit/tests/test_middleware.py @@ -155,7 +155,7 @@ def test_is_ratelimited_swift_local_container_update(self): status='498 Rate Limited', body='Rate Limit Exceeded', headerlist=[('X-Retry-After', 58), ('X-RateLimit-Retry-After', 58), - ('X-RateLimit-Limit', '2r/m'), ('X-RateLimit-Remaining', 0)] + ('X-RateLimit-Limit', '2r/m'), ('X-RateLimit-Remaining', 0)] ) ] @@ -248,14 +248,14 @@ def response_equal(expected, got): return False, "expected status '{0}' but got '{1}'".format(expected.status, got.status) if expected.has_body and expected.body != got.body: - return False, "expected body '{0}' but got '{1}'".format(expected.body, got.body) + return False, "expected body '{0}' but got '{1}'".format(expected.body, got.body) if not expected.has_body and expected.json_body != got.json_body: - return False, "expected json body '{0}' but got '{1}'".format(expected.json_body, got.json_body) + return False, "expected json body '{0}' but got '{1}'".format(expected.json_body, got.json_body) return True, "items are equal" - if type(expected) != type(got): + if not isinstance(got, type(expected)): return False, "expected type {0} but got type {1}".format(type(expected), type(got)) # Compare arguments if neither RateLimitResponse nor BlacklistResponse. diff --git a/rate_limit/tests/test_parse_config.py b/rate_limit/tests/test_parse_config.py index 0fa5a41..e7d9e1a 100644 --- a/rate_limit/tests/test_parse_config.py +++ b/rate_limit/tests/test_parse_config.py @@ -72,16 +72,16 @@ def test_load_swift_config(self): def test_parse_and_convert_to_per_seconds(self): stimuli = [ { - 'in': '5r/s', + 'in': '5r/s', 'expected': 5 }, { 'in': '1r/m', - 'expected': round(1/60.0, 4) + 'expected': round(1 / 60.0, 4) }, { 'in': '10r/d', - 'expected': round(1/8640.0,4) + 'expected': round(1 / 8640.0, 4) }, { 'in': '0.5r/s', diff --git a/rate_limit/tests/test_ratelimit_algorithm.py b/rate_limit/tests/test_ratelimit_algorithm.py index 24c0105..8711222 100644 --- a/rate_limit/tests/test_ratelimit_algorithm.py +++ b/rate_limit/tests/test_ratelimit_algorithm.py @@ -8,6 +8,7 @@ from rate_limit.response import RateLimitExceededResponse from unittest.mock import MagicMock + class TestRateLimitAlgorithm(unittest.TestCase): def setUp(self): self.configure_connection(max_sleep_time_seconds=600) @@ -50,7 +51,6 @@ def test_rate_limit_hit_not_suspension(self): self.assertIsNone(resp2, "Expected response not to be limited") def test_rate_limit_hit_not_suspended(self): - #Override connection with new max sleep time self.configure_connection(max_sleep_time_seconds=0) rand = random.randint(1, 1000) target_type = f"port-z{str(rand)}" diff --git a/rate_limit/tests/test_response.py b/rate_limit/tests/test_response.py index f4ccc1b..fbfb387 100644 --- a/rate_limit/tests/test_response.py +++ b/rate_limit/tests/test_response.py @@ -1,6 +1,5 @@ import json import os -import six import unittest @@ -35,7 +34,8 @@ def test_default_ratelimitexceededresponse_json(self): self.assertEqual( ratelimit_response.content_type, common.Constants.content_type_json, - "expected response content type to be equal. want '{0}' but got '{1}'".format(common.Constants.content_type_json, ratelimit_response.content_type) + "expected response content type to be equal. want '{0}' " + "but got '{1}'".format(common.Constants.content_type_json, ratelimit_response.content_type) ) actual_body = sorted(json.loads(ratelimit_response.json_body)) @@ -51,7 +51,8 @@ def test_default_ratelimitexceededresponse_json(self): def test_custom_ratelimitexceededresponse_html(self): conf = common.load_config(SWIFTCONFIGPATH) - status, status_code, headers, body, json_body = response.response_parameters_from_config(conf.get(common.Constants.ratelimit_response)) + status, status_code, headers, body, json_body = ( + response.response_parameters_from_config(conf.get(common.Constants.ratelimit_response))) ratelimit_response = response.RateLimitExceededResponse( status=status, @@ -126,7 +127,8 @@ def test_default_blacklistresponse(self): def test_custom_blacklistresponse_json(self): conf = common.load_config(SWIFTCONFIGPATH) - status, status_code, headers, body, json_body = response.response_parameters_from_config(conf.get(common.Constants.blacklist_response)) + status, status_code, headers, body, json_body = ( + response.response_parameters_from_config(conf.get(common.Constants.blacklist_response))) blacklist_response = response.BlacklistResponse( status=status, @@ -156,7 +158,10 @@ def test_custom_blacklistresponse_json(self): .format(common.Constants.content_type_json, blacklist_response.content_type) ) - expected_json_body = json.dumps({"error": {"status": "497 Blacklisted", "message": "You have been blacklisted. Please contact and administrator."}}, sort_keys=True) + expected_json_body = json.dumps({"error": + {"status": "497 Blacklisted", + "message": "You have been blacklisted. " + "Please contact and administrator."}}, sort_keys=True) self.assertEqual( blacklist_response.json_body, expected_json_body, diff --git a/rate_limit/tests/test_units.py b/rate_limit/tests/test_units.py index 06a9b88..7067cfe 100644 --- a/rate_limit/tests/test_units.py +++ b/rate_limit/tests/test_units.py @@ -35,7 +35,7 @@ def test_parse_sliding_window_rate_limit(self): }, { 'input': '100r/d', - 'expected': (100.0, 24*3600.0) + 'expected': (100.0, 24 * 3600.0) }, { 'input': '5r/2m', diff --git a/rate_limit/utils.py b/rate_limit/utils.py index 101e960..4fb14c6 100644 --- a/rate_limit/utils.py +++ b/rate_limit/utils.py @@ -11,7 +11,7 @@ def str_if_bytes(value): # NOTE: This function was copied from # https://github.com/andymccurdy/redis-py/blob/master/redis/client.py#L114 def parse_info(response): - "Parse the result of Redis's INFO command into a Python dict" + """Parse the result of Redis's INFO command into a Python dict.""" info = {} response = str_if_bytes(response) diff --git a/test-requirements.txt b/test-requirements.txt index 8e8c95d..cc092d5 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,4 +1,4 @@ -flake8-docstrings==0.2.1.post1 # MIT +flake8-docstrings>=0.2.1.post1 # MIT fixtures>=3.0.0 # Apache-2.0/BSD mock>=2.0.0 # BSD From 440c0cb0119aa93efa036b565bf1409e2c003e53 Mon Sep 17 00:00:00 2001 From: Sven Rosenzweig Date: Fri, 25 Jul 2025 13:17:48 +0200 Subject: [PATCH 4/4] Update supported python versions We do not support python version 2, so we remove it from the setup.cfg. Run tox with multiple python versions. --- .github/workflows/run-tox.yml | 2 +- setup.cfg | 10 +++------- tox.ini | 2 +- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/.github/workflows/run-tox.yml b/.github/workflows/run-tox.yml index de27354..e90468f 100644 --- a/.github/workflows/run-tox.yml +++ b/.github/workflows/run-tox.yml @@ -9,7 +9,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - python: ['3.10'] + python: ['3.10', '3.11', '3.12'] services: redis: image: redis diff --git a/setup.cfg b/setup.cfg index 1736171..648566f 100644 --- a/setup.cfg +++ b/setup.cfg @@ -13,14 +13,10 @@ classifier = Intended Audience :: System Administrators License :: OSI Approved :: Apache Software License Operating System :: POSIX :: Linux - Programming Language :: Python - Programming Language :: Python :: 2 - Programming Language :: Python :: 2.7 Programming Language :: Python :: 3 - Programming Language :: Python :: 3.4 - Programming Language :: Python :: 3.5 - Programming Language :: Python :: 3.6 - Programming Language :: Python :: 3.7 + Programming Language :: Python :: 3.10 + Programming Language :: Python :: 3.11 + Programming Language :: Python :: 3.12 [files] packages = diff --git a/tox.ini b/tox.ini index a5df9dd..16656a5 100644 --- a/tox.ini +++ b/tox.ini @@ -2,7 +2,7 @@ minversion = 2.0 # avoid sdist skipsdist = True -envlist = py310,pep8 +envlist = py,pep8 [testenv] install_command = pip install -c {env:UPPER_CONSTRAINTS_FILE:https://raw.githubusercontent.com/sapcc/requirements/stable/2024.1-m3/upper-constraints.txt} -U {opts} {packages}