Re: [PATCH] Dynamic rate limiting for limit_req module

Sun, 01 Jan 2023 22:21:24 -0800 

Hello,

Happy new year to you too, and thank you for the detailed feedback.
"The fundamental problem with dynamic rates in limit_req, which is a leaky bucket limiter, is that the rate is a property which applies to multiple requests as accumulated in the bucket (and this is basically why it is configured in the limit_req_zone, and not in limit_req), while the dynamically calculated rate is a property of the last request. 
This can easily result in counter-intuitive behaviour.
For example, consider 5 requests with 1 second interval, assuming burst 2, and rate being 1r/m or 100r/s depending on some request properties: 

- 1st request, rate 1r/m, request is accepted
- 2st request, rate 1r/m, request is accepted
- 3rd request, rate 1r/m, request is rejected, since there is no room in the bucket 
- 4th request, rate 100r/s, request is accepted
- 5th request, rate 1r/m, request is accepted (unexpectedly)
Note that the 5th request is accepted, while it is mostly equivalent to the 3rd request, and one could expect it to be rejected. But it happens to to be accepted because the 4th request "cleared" the bucket."
This is true - however I do wonder if the behavior (clearing the bucket) is truly counter intuitive in that scenario.
Should it not be expected that this user/key/state that has been assigned 100r/s (even if just for one request) would have it's outstanding excess requests recalculated (and in this case absolved) by that rate increase? 
You have after all assigned it more capacity.
I'm not sure how to elegantly avoid this if it is an issue, since there is no 'standard' rate to reference (it could interpolate over time/requests, but that might be even more confusing).
"Note well that the same limits now can be configured separately, in distinct shared memory zones. This makes it possible to limit them requests consistently, without any unexpected deviations from the configured behaviour if requests with different rates interfere.
Just in case, the very same behaviour can be implemented with multiple limit_req_zones, with something like this: 

    map $traffic_tier $limit_free {
        free          $binary_remote_addr;
    }

    map $traffic_tier $limit_basic {
        basic         $binary_remote_addr;
    }

    map $traffic_tier $limit_premium {
        premium       $binary_remote_addr;
    }

    limit_req_zone $limit_free zone=free:10m rate=1r/m;
    limit_req_zone $limit_basic zone=basic:10m rate=2r/m;
    limit_req_zone $limit_premium zone=premium:10m rate=1r/s;

    limit_req zone=free;
    limit_req zone=basic;
    limit_req zone=premium;
From the example it is not very clear how the suggested change is beneficial."
The issue with this approach (which is a similar set up to the one that prompted this patch) is the scalability of the solution -  one zone is required per unique rate, and the inability to assign arbitrary rates at runtime.
Perhaps a better example than the first one I gave to illustrate the full utility would be with this scenario - involving a user sending a JWT to determine the rate limit that is applied:
The limit_req_zone is assigned with $jwt_claim_sub as the key and $jwt_claim_rate as the rate.
A user presents the following JWT with every request, his requests now have a rate limit of 100r/s. 
{
    ...
    "sub": "user01",
    "rate": "100r/s"
}
Mid-session, that user then acquires a new JWT to get increased speed -  the limit associated with this state/node is now 1000r/s with the same key(sub). Note that these rate values do not need to be preset in the nginx configuration, and it can all be done with a single limit_req_zone. 
{
    ...
    "sub": "user01",
    "rate": "1000r/s"
}
Similar could also be done with key_val zone (ie. adding and modifying rate strings at runtime within a key_val zone, and doing a lookup in the key_val zone for that value based upon a user identifier or group as the key).
"Note that this approach implies parsing rate on each request. This is something we usually try to avoid as long as a static string is configured in advance, see these commits for an example: 

changeset:   7503:b82162b8496a
user:        Ruslan Ermilov <r...@nginx.com>
date:        Wed Apr 24 16:38:51 2019 +0300
summary:     Added ngx_http_set_complex_value_size_slot().

changeset:   7504:c19ca381b2e6
user:        Ruslan Ermilov <r...@nginx.com>
date:        Wed Apr 24 16:38:54 2019 +0300
summary:     Variables support in limit_rate and limit_rate_after (ticket #293).

changeset:   7505:16a1adadf437
user:        Ruslan Ermilov <r...@nginx.com>
date:        Wed Apr 24 16:38:56 2019 +0300
summary:     Variables support in proxy_upload_rate and proxy_download_rate.
A more self-contained solution can be seen in the image filter (src/http/modules/ngx_http_image_filter.c), which uses values parsed during configuration parsing if there are no variables, and complex values if there are variables (e.g., "imcf->angle" and "imcf->acv")." 

Thanks, that makes perfect sense. I'll update accordingly.


Note that this is a style change, and it is wrong.

The same here.
Note that this is a style change, since the max_delay is set in all possible code paths, and there are no reasons to additionally initialize it.
Note that this is a style change, since the max_delay is set in all possible code paths, and there are no reasons to additionally initialize it. 

See above.
Understood, I'll work to fix the style of my changes and I will keep it more consistent with the guidelines in future.
This seems to be separate change, irrelevant to what the patch does. If at all, it should be submitted as a separate patch, see http://nginx.org/en/docs/contributing_changes.html.
Note though that this changes looks wrong to me, as it will break request accounting on large time intervals with large burst, such as with rate=1r/m burst=100.
Note well that it fails to use an empty line before "} else ", which is a style bug.
You are correct, it will break for that scenario. I will rework this and the related portions to more generic overflow detection & handling.
I did include it in this patch as there is a danger (which is already present) of rate * ms overflowing. rate is only limited by atoi's limits at present(and it's multiplied by 1000 after that even) . This is more of a worrisome when arbitrary values (including user request values) are in use place of static values from the nginx configuration. 

I will make those changes into a separate patch as advised.
The "lr->rate", which is saved in the shared memory, seems to be never used except during processing of the same request. Any reasons to keep it in the shared memory, reducing the number of states which can be stored there?
A persistent record of the rate that was last assigned to a given node/state is needed exclusively for expiring records (in the expire function). I don't believe this can be avoided, as the rate of the current request is unrelated to the 'to be removed' record's. 

Thanks again.

On 01/01/2023 17:35, Maxim Dounin wrote:

Hello!

On Fri, Dec 30, 2022 at 10:23:12PM +0000, J Carter wrote:


Please find below a patch to enable dynamic rate limiting for
limit_req module.

Thanks for the patch, and Happy New Year.

The fundamental problem with dynamic rates in limit_req, which is
a leaky bucket limiter, is that the rate is a property which
applies to multiple requests as accumulated in the bucket (and
this is basically why it is configured in the limit_req_zone, and
not in limit_req), while the dynamically calculated rate is a
property of the last request.  This can easily result in
counter-intuitive behaviour.

For example, consider 5 requests with 1 second interval, assuming
burst 2, and rate being 1r/m or 100r/s depending on some request
properties:

- 1st request, rate 1r/m, request is accepted
- 2st request, rate 1r/m, request is accepted
- 3rd request, rate 1r/m, request is rejected, since there is no
   room in the bucket
- 4th request, rate 100r/s, request is accepted
- 5th request, rate 1r/m, request is accepted (unexpectedly)

Note that the 5th request is accepted, while it is mostly
equivalent to the 3rd request, and one could expect it to be
rejected.  But it happens to to be accepted because the 4th
request "cleared" the bucket.

Could you please clarify how this problem is addressed in your
patch?

Note well that the same limits now can be configured separately,
in distinct shared memory zones.  This makes it possible to limit
them requests consistently, without any unexpected deviations from
the configured behaviour if requests with different rates
interfere.


/* ----------------------------EXAMPLE---------------------------------*/

  geo $traffic_tier {
         default        free;
         127.0.1.0/24   basic;
         127.0.2.0/24   premium;
     }

     map $traffic_tier $rate {
         free           1r/m;
         basic          2r/m;
         premium        1r/s;
     }

     limit_req_zone $binary_remote_addr zone=one:10m rate=$rate;

     server {
         limit_req zone=one;
         listen       80;
         server_name  localhost;
         return 200;
     }

curl --interface 127.0.X.X localhost

Just in case, the very same behaviour can be implemented with
multiple limit_req_zones, with something like this:

     map $traffic_tier $limit_free {
         free          $binary_remote_addr;
     }

     map $traffic_tier $limit_basic {
         basic         $binary_remote_addr;
     }

     map $traffic_tier $limit_premium {
         premium       $binary_remote_addr;
     }

     limit_req_zone $limit_free zone=free:10m rate=1r/m;
     limit_req_zone $limit_basic zone=basic:10m rate=2r/m;
     limit_req_zone $limit_premium zone=premium:10m rate=1r/s;

     limit_req zone=free;
     limit_req zone=basic;
     limit_req zone=premium;

 From the example it is not very clear how the suggested change is
beneficial.

See below for some comments about the code.

[...]


# HG changeset patch
# User jordanc.car...@outlook.com
# Date 1672437935 0
#      Fri Dec 30 22:05:35 2022 +0000
# Branch dynamic-rate-limiting
# Node ID b2bd50efa81e5aeeb9b8f84ee0af34463add07fa
# Parent  07b0bee87f32be91a33210bc06973e07c4c1dac9
Changed 'rate=' to complex value and added limits to the rate value to prevent 
integer overflow/underflow

diff -r 07b0bee87f32 -r b2bd50efa81e 
src/http/modules/ngx_http_limit_req_module.c
--- a/src/http/modules/ngx_http_limit_req_module.c      Wed Dec 21 14:53:27 
2022 +0300
+++ b/src/http/modules/ngx_http_limit_req_module.c      Fri Dec 30 22:05:35 
2022 +0000
@@ -26,6 +26,7 @@
      /* integer value, 1 corresponds to 0.001 r/s */
      ngx_uint_t                   excess;
      ngx_uint_t                   count;
+    ngx_uint_t                   rate;
      u_char                       data[1];
  } ngx_http_limit_req_node_t;

@@ -41,7 +42,7 @@
      ngx_http_limit_req_shctx_t  *sh;
      ngx_slab_pool_t             *shpool;
      /* integer value, 1 corresponds to 0.001 r/s */
-    ngx_uint_t                   rate;
+    ngx_http_complex_value_t     rate;
      ngx_http_complex_value_t     key;
      ngx_http_limit_req_node_t   *node;
  } ngx_http_limit_req_ctx_t;
@@ -66,9 +67,9 @@

  static void ngx_http_limit_req_delay(ngx_http_request_t *r);
  static ngx_int_t ngx_http_limit_req_lookup(ngx_http_limit_req_limit_t *limit,
-    ngx_uint_t hash, ngx_str_t *key, ngx_uint_t *ep, ngx_uint_t account);
+    ngx_uint_t hash, ngx_str_t *key, ngx_uint_t *ep, ngx_uint_t account, 
ngx_uint_t rate);
  static ngx_msec_t ngx_http_limit_req_account(ngx_http_limit_req_limit_t 
*limits,
-    ngx_uint_t n, ngx_uint_t *ep, ngx_http_limit_req_limit_t **limit);
+    ngx_uint_t n, ngx_uint_t *ep, ngx_http_limit_req_limit_t **limit, 
ngx_uint_t rate);
  static void ngx_http_limit_req_unlock(ngx_http_limit_req_limit_t *limits,
      ngx_uint_t n);
  static void ngx_http_limit_req_expire(ngx_http_limit_req_ctx_t *ctx,
@@ -195,10 +196,13 @@
  ngx_http_limit_req_handler(ngx_http_request_t *r)
  {
      uint32_t                     hash;
-    ngx_str_t                    key;
+    ngx_str_t                    key, rate_s;
      ngx_int_t                    rc;
      ngx_uint_t                   n, excess;
+    ngx_uint_t                   scale;
+    ngx_uint_t                   rate;
      ngx_msec_t                   delay;
+    u_char                      *p;
      ngx_http_limit_req_ctx_t    *ctx;
      ngx_http_limit_req_conf_t   *lrcf;
      ngx_http_limit_req_limit_t  *limit, *limits;
@@ -243,10 +247,34 @@

          hash = ngx_crc32_short(key.data, key.len);

+        if (ngx_http_complex_value(r, &ctx->rate, &rate_s) != NGX_OK) {
+            ngx_http_limit_req_unlock(limits, n);
+            return NGX_HTTP_INTERNAL_SERVER_ERROR;
+        }
+
+        scale = 1;
+        rate = NGX_ERROR;
+
+        if (rate_s.len > 8) {
+
+            rate = (ngx_uint_t) ngx_atoi(rate_s.data + 5, rate_s.len - 8);
+
+            p = rate_s.data + rate_s.len - 3;
+            if (ngx_strncmp(p, "r/m", 3) == 0) {
+                scale = 60;
+            } else if (ngx_strncmp(p, "r/s", 3) != 0){
+                rate = NGX_ERROR;
+            }
+        }

Note that this approach implies parsing rate on each request.
This is something we usually try to avoid as long as a static
string is configured in advance, see these commits for an example:

changeset:   7503:b82162b8496a
user:        Ruslan Ermilov <r...@nginx.com>
date:        Wed Apr 24 16:38:51 2019 +0300
summary:     Added ngx_http_set_complex_value_size_slot().

changeset:   7504:c19ca381b2e6
user:        Ruslan Ermilov <r...@nginx.com>
date:        Wed Apr 24 16:38:54 2019 +0300
summary:     Variables support in limit_rate and limit_rate_after (ticket #293).

changeset:   7505:16a1adadf437
user:        Ruslan Ermilov <r...@nginx.com>
date:        Wed Apr 24 16:38:56 2019 +0300
summary:     Variables support in proxy_upload_rate and proxy_download_rate.

A more self-contained solution can be seen in the image filter
(src/http/modules/ngx_http_image_filter.c), which uses values
parsed during configuration parsing if there are no variables, and
complex values if there are variables (e.g., "imcf->angle" and
"imcf->acv").


+
+        rate = (rate != 0 && rate < NGX_MAX_INT_T_VALUE / 60000000 - 1001) ?
+                rate * 1000 / scale :
+                NGX_MAX_INT_T_VALUE / 60000000 - 1001;
+
          ngx_shmtx_lock(&ctx->shpool->mutex);

          rc = ngx_http_limit_req_lookup(limit, hash, &key, &excess,
-                                       (n == lrcf->limits.nelts - 1));
+                                       (n == lrcf->limits.nelts - 1), rate);

          ngx_shmtx_unlock(&ctx->shpool->mutex);

@@ -291,7 +319,7 @@
          excess = 0;
      }

-    delay = ngx_http_limit_req_account(limits, n, &excess, &limit);
+    delay = ngx_http_limit_req_account(limits, n, &excess, &limit, rate);

      if (!delay) {
          r->main->limit_req_status = NGX_HTTP_LIMIT_REQ_PASSED;
@@ -403,7 +431,7 @@

  static ngx_int_t
  ngx_http_limit_req_lookup(ngx_http_limit_req_limit_t *limit, ngx_uint_t hash,
-    ngx_str_t *key, ngx_uint_t *ep, ngx_uint_t account)
+    ngx_str_t *key, ngx_uint_t *ep, ngx_uint_t account, ngx_uint_t rate)
  {
      size_t                      size;
      ngx_int_t                   rc, excess;
@@ -412,7 +440,6 @@
      ngx_rbtree_node_t          *node, *sentinel;
      ngx_http_limit_req_ctx_t   *ctx;
      ngx_http_limit_req_node_t  *lr;
-
      now = ngx_current_msec;

      ctx = limit->shm_zone->data;

Note that this is a style change, and it is wrong.


@@ -446,12 +473,14 @@

              if (ms < -60000) {
                  ms = 1;
-

The same here.


              } else if (ms < 0) {
                  ms = 0;
+            } else if (ms > 60000) {
+                ms = 60000;
              }

This seems to be separate change, irrelevant to what the patch
does.  If at all, it should be submitted as a separate patch, see
http://nginx.org/en/docs/contributing_changes.html.

Note though that this changes looks wrong to me, as it will break
request accounting on large time intervals with large burst, such
as with rate=1r/m burst=100.

Note well that it fails to use an empty line before "} else ",
which is a style bug.


-            excess = lr->excess - ctx->rate * ms / 1000 + 1000;
+            lr->rate = rate;
+            excess = lr->excess - lr->rate * ms / 1000 + 1000;

              if (excess < 0) {
                  excess = 0;
@@ -510,6 +539,7 @@

      lr->len = (u_short) key->len;
      lr->excess = 0;
+    lr->rate = rate;

The "lr->rate", which is saved in the shared memory, seems to be
never used except during processing of the same request.  Any
reasons to keep it in the shared memory, reducing the number of
states which can be stored there?


      ngx_memcpy(lr->data, key->data, key->len);

@@ -534,7 +564,7 @@

  static ngx_msec_t
  ngx_http_limit_req_account(ngx_http_limit_req_limit_t *limits, ngx_uint_t n,
-    ngx_uint_t *ep, ngx_http_limit_req_limit_t **limit)
+    ngx_uint_t *ep, ngx_http_limit_req_limit_t **limit, ngx_uint_t rate)
  {
      ngx_int_t                   excess;
      ngx_msec_t                  now, delay, max_delay;
@@ -543,13 +573,13 @@
      ngx_http_limit_req_node_t  *lr;

      excess = *ep;
+    max_delay = 0;


Note that this is a style change, since the max_delay is set in
all possible code paths, and there are no reasons to additionally
initialize it.


      if ((ngx_uint_t) excess <= (*limit)->delay) {
          max_delay = 0;

      } else {
-        ctx = (*limit)->shm_zone->data;
-        max_delay = (excess - (*limit)->delay) * 1000 / ctx->rate;
+        max_delay = (excess - (*limit)->delay) * 1000 / rate;
      }

      while (n--) {
@@ -570,9 +600,11 @@

          } else if (ms < 0) {
              ms = 0;
+        } else if (ms > 60000) {
+            ms = 60000;
          }

See above.

[...]


_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel

Reply via email to