Rate Limit policy to take into account changes in the config #703
Conversation
|
@mikz Sure, I will divide this PR. |
y-tabata
force-pushed
the
improve-rate-limit-policy-retry
branch
from
304d623
to
b04fbf2
Compare
y-tabata
changed the title
y-tabata
force-pushed
the
improve-rate-limit-policy-retry
branch
from
1b59fe3
to
cc63df3
Compare
y-tabata
force-pushed
the
improve-rate-limit-policy-retry
branch
3 times, most recently
from
13301cc
to
396f259
Compare
| @@ -160,6 +161,7 @@ function _M.new(config) | |||
| self.redis_url = config.redis_url | |||
| self.error_settings = init_error_settings( | |||
| config.limits_exceeded_error, config.configuration_error) | |||
| self.timestamp = ngx.time() | |||
There was a problem hiding this comment.
There is no need to store time in self.timestamp. Local variable will be ok too.
:new() is evaluated only when the policy is initialized, if you have multiple gateways that initialize at different times then they'd not share the same key.
| @@ -133,9 +134,9 @@ local function build_limiters_and_keys(type, limiters, redis, error_settings, co | |||
| local key = limiter.template_string:render( | |||
| ngx_variable.available_context(context)) | |||
| if limiter.key.scope == "global" then | |||
| key = format("%s_%s", type, key) | |||
| key = format("%d_%s_%s", timestamp + window, type, key) | |||
There was a problem hiding this comment.
You can't use just timestamp + window. In case of multiple gateways they all need to generate the same key. That can be achieved by generating a time bucket.
The formula is: time - (time % window). That will for the whole time of window return the same value and then flip to the next one when the window is over.
y-tabata
force-pushed
the
improve-rate-limit-policy-retry
branch
from
ddde20b
to
e39eb9d
Compare
| @@ -168,23 +168,26 @@ describe('Rate limit policy', function() | |||
| rate_limit_policy:access(context) | |||
| rate_limit_policy:access(context) | |||
|
|
|||
| local redis_key = redis:keys('*_fixed_window_test3')[1] | |||
There was a problem hiding this comment.
It is possible to stub ngx.time() to return some fixed time: stub(ngx,time, function() return ... end).
To not duplicate the logic of calculating the fixed window, it could be extracted into a function that exported from own file, so it can be reused by the policy and the test.
Actually the more I think about it then how the limiters and keys are created could easily live in own module and could be unit tested standalone without the policy.
But it is no big deal, we can tackle that in the future. Definitely not a blocker for merging this.
There was a problem hiding this comment.
Changed to use stub.
This PR addresses to Issue #667.