Policy: Rate-limit fix window exception #1113
Conversation
eloycoto
force-pushed
the
RateLimitIssue
branch
from
eb7dfea to
9bf7f73
Compare
| @@ -181,6 +181,7 @@ | |||
| "type": "array", | |||
| "items": { | |||
| "type": "object", | |||
| "required": ["key", "count", "window"], | |||
There was a problem hiding this comment.
I see that in the other kind of limiters, the number used to limit is not required. I think it's the same case as window here. Have you checked them?
There was a problem hiding this comment.
Yup, tested the others and no division happens.
| @@ -196,7 +197,8 @@ | |||
| "window": { | |||
| "type": "integer", | |||
| "description": "The time window in seconds before the request count is reset", | |||
| "exclusiveMinimum": 0 | |||
| "exclusiveMinimum": 0, | |||
| "default": 1 | |||
There was a problem hiding this comment.
Why is the default needed? It's required and there's a minimum value.
Also, this made me think about the limit in count (I know it's not part of the PR), but it's also exclusiveMinimum: 0. Not sure if that's correct. Should we allow 0 to reject always?
There was a problem hiding this comment.
t/apicast-policy-rate-limit.t
Outdated
| @@ -1313,3 +1313,62 @@ limit. The limit is set to 2. Only the third one should fail. | |||
| [200, 200, 429] | |||
| --- no_error_log | |||
| [error] | |||
|
|
|||
|
|
|||
| === TEST 22: Window is not set and default to 1 | |||
There was a problem hiding this comment.
The title does not seem to be correct. In the config, window is set to 0.
t/apicast-policy-rate-limit.t
Outdated
| window = 0 | ||
| } | ||
| }, | ||
| redis_url = "redis://$TEST_NGINX_REDIS_HOST:$TEST_NGINX_REDIS_PORT/1", |
There was a problem hiding this comment.
Optional: if you don't define this limit as global, you will not need redis, and you'll be able to simplify a bit the test. You'll be able to delete this line, and also the /flush_redis location and the first request in the test.
eloycoto
force-pushed
the
RateLimitIssue
branch
3 times, most recently
from
be91f44 to
9374110
Compare
On rate-limit window can be set as 0 so the division that happens on resty will fail due to division by 0 is not allowed. This commit fixes that and validates that window value is bigger than 0, if not set the default to 1. Also, add an integration test to prove this scenario and update the JSON schema to verify that the number is a valid integer. FIX: THREESCALE-3382 Signed-off-by: Eloy Coto <[email protected]>
eloycoto
force-pushed
the
RateLimitIssue
branch
from
9374110 to
dbe4917
Compare
On rate-limit window can be set as 0 so the division that happens on
resty will fail due to division by 0 is not allowed.
This commit fixes that and validates that window value is bigger than 0,
if not set the default to 1. Also, add an integration test to prove this
scenario and update the JSON schema to verify that the number is a valid
integer.
FIX: THREESCALE-3382
JSONSchema changes:
https://mozilla-services.github.io/react-jsonschema-form/#eyJmb3JtRGF0YSI6eyJmaXJzdE5hbWUiOiJDaHVjayIsImxhc3ROYW1lIjoiTm9ycmlzIiwiYWdlIjo3NSwiYmlvIjoiUm91bmRob3VzZSBraWNraW5nIGFzc2VzIHNpbmNlIDE5NDAiLCJwYXNzd29yZCI6Im5vbmVlZCJ9LCJzY2hlbWEiOnt9LCJ1aVNjaGVtYSI6e319
Signed-off-by: Eloy Coto [email protected]