Policy: rate_limit_headers fix cache issues. #1197
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eloycoto
force-pushed
the
FIX-RATELIMITS_HEADERS
branch
from
8f34ddf to
b82f3ff
Compare
When testing rate_limit_headers the number of APICast workers are always set to 1. When the number of workers increased, the lru_cache is not longer effective due to multiple threads. Because we only have information about usage statistics on Authrep, and this is using ngx.shared info, this policy also need to use shared information to be able to reply without doing the authrep in all sessions. Fix THREESCALE-3795 Signed-off-by: Eloy Coto <[email protected]>
eloycoto
force-pushed
the
FIX-RATELIMITS_HEADERS
branch
from
b82f3ff to
b489e35
Compare
davidor
reviewed
Jun 17, 2020
| @@ -41,4 +42,25 @@ function _M:reset(max, remaining, reset) | |||
| self.reset = countdown_counter.new(reset, now()) | |||
| end | |||
|
|
|||
| function _M:export() | |||
There was a problem hiding this comment.
Nitpick: should this be called tostring or similar for clarity?
There was a problem hiding this comment.
I do not think so, export/import is more declarative when you read the code. tostring was my first idea 😅
| return _M.Init_empty(usage) | ||
| end | ||
|
|
||
| local data = stringx.split(exported_data, "#") |
There was a problem hiding this comment.
Nitpick: the "#" could be extracted to a constant that could be used in this method and in export().
| local data = cache_entry.import(usage, entry:export()) | ||
| assert.same(data.limit:__tostring(), "1") | ||
| assert.same(data.remaining:__tostring(), "10") | ||
| end) |
There was a problem hiding this comment.
You forgot one assert for reset (3).
| usage:add("foo", 1) | ||
| end) | ||
|
|
||
| it('if Auth cache is disabled, report the usage', function() |
There was a problem hiding this comment.
Not too important, but in cases like this, I like to put the condition outside the it clause, so the it clause reads like natural English. Something like this:
describe('when the auth cache is disabled', function()
it('reports the usage', function()
...
)
)
davidor
approved these changes
Jun 18, 2020
eloycoto
force-pushed
the
FIX-RATELIMITS_HEADERS
branch
from
b1f73c6 to
1c0896d
Compare
Signed-off-by: Eloy Coto <[email protected]>
eloycoto
force-pushed
the
FIX-RATELIMITS_HEADERS
branch
from
1c0896d to
ea9133e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When testing rate_limit_headers the number of APICast workers are always set to
due to multiple threads.
Because we only have information about usage statistics on Authrep, and this is
using ngx.shared info, this policy also need to use shared information to be
able to reply without doing the authrep in all sessions.
Fix THREESCALE-3795
Signed-off-by: Eloy Coto [email protected]