Return correct Retry After Header value

.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', '3.11', '3.12']
+    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"

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(

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'

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:

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}

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

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,

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:

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',
             },
         ]
 

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.

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',

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):
+        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")

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,

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',

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)
 

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 =