Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ratelimit plugin not working as expected - kong 1.4 #5311

Closed
ghost opened this issue Dec 8, 2019 · 7 comments · Fixed by #10559
Closed

Ratelimit plugin not working as expected - kong 1.4 #5311

ghost opened this issue Dec 8, 2019 · 7 comments · Fixed by #10559
Labels
plugins/rate-limiting plugins/response-ratelimiting task/needs-investigation Requires investigation and reproduction before classifying it as a bug or not.

Comments

@ghost
Copy link

ghost commented Dec 8, 2019

Summary

I have configured rate-limit plugin with key-auth. 5 req/sec is working fine if requests are more then 50 per seconds its not detecting. i have tried out with cluster/local/redis policy.

SUMMARY_GOES_HERE

Steps To Reproduce

kong 1.4 . cassandra backend. deploy on kubernets

  1. enable key-auth plugin
  2. enable rate limit plugin with 5 req/sec
  3. try with apache benchmark ab -c 100 -n 1000
  4. check live kong logs

Additional Details & Logs
@hishamhm hishamhm added the task/needs-investigation Requires investigation and reproduction before classifying it as a bug or not. label Dec 9, 2019
@bungle
Copy link
Member

bungle commented Dec 9, 2019

Currently rate-limiting is counted async, the function is created here on access phase:
https://github.com/Kong/kong/blob/master/kong/plugins/rate-limiting/handler.lua#L130-L135

And it is executed here on log phase:
https://github.com/Kong/kong/blob/master/kong/plugins/rate-limiting/handler.lua#L148-L150

@bungle
Copy link
Member

bungle commented Dec 9, 2019 •
edited
Loading

I think on high load, the timer that is executed on log is not executed as fast as new requests are proxied. This has potential good and bad sides. Good sides is that if we increase the counters after request we can increase counters only when we actually proxied. Bad side of it being that we are possibly going to proxy some requests before the rate-limit is effective. Good side of this is that we also add little latency to request itself. Having fully up-to-date rate-limiting requires making db requests for both get usage and increase counters run on access and make request to wait on both. Perhaps it should be done like that. increasing counters has always been a timer, but on originally it was executed on access instead of log.

@bungle
Copy link
Member

bungle commented Dec 9, 2019 •
edited
Loading

On Kong Enterprise we have this (which does not run timers on each request):
https://docs.konghq.com/hub/kong-inc/rate-limiting-advanced/

@bungle bungle mentioned this issue Jan 16, 2020
Closed
@bungle
Copy link
Member

bungle commented Jan 16, 2020

@jogendrajangid1, would you like to try: #5460, and give feedback?

bungle added a commit that referenced this issue Jan 17, 2020
@bungle
refactor(rate-limiting) remove timer and phase handlers
197dba8 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

### Issues Resolved

This might fix #5311
bungle added a commit that referenced this issue Jan 22, 2020
@bungle
refactor(rate-limiting) remove timer and phase handlers
4847757 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

### Issues Resolved

This might fix #5311
bungle added a commit that referenced this issue Feb 28, 2020
@bungle
feat(rate-limiting) remove timer and phase handlers
e77f6e2 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
@vasuharish vasuharish mentioned this issue Apr 17, 2020
Closed
bungle added a commit that referenced this issue Apr 17, 2020
@bungle
feat(rate-limiting) remove timer and phase handlers
ca0a552 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
bungle added a commit that referenced this issue May 22, 2020
@bungle
feat(rate-limiting) remove timer and phase handlers
59f3cdd 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
snt8amr pushed a commit to snt8amr/kong that referenced this issue May 30, 2020
@bungle
feat(rate-limiting) remove timer and phase handlers
54c8ec1 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix Kong#5311
bungle added a commit that referenced this issue Sep 16, 2020
@bungle
feat(rate-limiting) remove timer and phase handlers
c5f1379 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
bungle added a commit that referenced this issue Sep 27, 2020
@bungle
feat(rate-limiting) remove timer and phase handlers
ee98e77 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
bungle added a commit that referenced this issue Oct 6, 2020
@bungle
feat(rate-limiting) remove timer and phase handlers
cac933b 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
bungle added a commit that referenced this issue Dec 8, 2020
@bungle
feat(rate-limiting) remove timer and phase handlers
14208d7 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
bungle added a commit that referenced this issue Dec 18, 2020
@bungle
feat(rate-limiting) remove timer and phase handlers
8ed5299 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
@perjerz
Copy link

perjerz commented Dec 19, 2020 •
edited
Loading

On Kong Enterprise we have this (which does not run timers on each request):
https://docs.konghq.com/hub/kong-inc/rate-limiting-advanced/

We have to buy Kong Enterprise to use it right?

bungle added a commit that referenced this issue Jan 8, 2021
@bungle
feat(rate-limiting) remove timer and phase handlers
83cf783 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
bungle added a commit that referenced this issue Jan 20, 2021
@bungle
feat(rate-limiting) remove timer and phase handlers
03aaf5e 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
bungle added a commit that referenced this issue Feb 1, 2021
@bungle
feat(rate-limiting) remove timer and phase handlers
4729a2e 
### Summary

Context:

- starting `timers` on busy servers on each request will just move
  the load elsewhere
- there is little change that upstream sets rate-limiting headers,
  so we can just set them on access

So this PR removes timers and all the phase handlers except `access`
from rate-limiting plugin. Yes, it might add some latency to request,
but on a bright side we have more accurate rate-limits.

This PR does change the rate-limiting consistency mode.

### Issues Resolved

This might fix #5311
@bungle bungle mentioned this issue Feb 3, 2021
Merged
bungle added a commit that referenced this issue Feb 3, 2021
@bungle
fix(rate-limiting) remove unneeded phase handlers for better accuracy
f4f22df 
### Summary

The PR #5460 tried to fix rate-limiting accuracy issues as reported with
#5311. That PR has stayed open for over a year now.

There is a smaller change in that PR that we can make without a lot of
work or debating, and this PR contains that only:

1. it removes extra phase handlers and context passing of data and
   now does everything in `access` phase (this could make it more
   accurate as well, compared to post-poning that to log phase)
2. it now also sets rate-limiting headers when the limits have
   exceeded
kikito pushed a commit that referenced this issue Feb 4, 2021
@bungle
fix(rate-limiting) remove unneeded phase handlers for better accuracy (…
01c76f7 
…#6802)

### Summary

The PR #5460 tried to fix rate-limiting accuracy issues as reported with
#5311. That PR has stayed open for over a year now.

There is a smaller change in that PR that we can make without a lot of
work or debating, and this PR contains that only:

1. it removes extra phase handlers and context passing of data and
   now does everything in `access` phase (this could make it more
   accurate as well, compared to post-poning that to log phase)
2. it now also sets rate-limiting headers when the limits have
   exceeded
@giovanibrioni giovanibrioni mentioned this issue Sep 10, 2021
Closed
@giovanibrioni
Copy link
Contributor

@jogendrajangid1 you can reproduce your scenario with the version of PR: #7831 that will work

@giovanibrioni giovanibrioni mentioned this issue Dec 22, 2021
Closed
giovanibrioni added a commit to giovanibrioni/kong that referenced this issue Apr 15, 2022
@giovanibrioni
fix rate-limiting accuracy reported on issue: Kong#5311
66fbaba
@giovanibrioni giovanibrioni mentioned this issue Mar 25, 2023
Merged
2 tasks
@hbagdi
Copy link
Member

hbagdi commented Apr 5, 2023

Kong 1.4 came out in October 2019 and is not longer supported.
Please try a 3.x version and open a new issue if needed.

@hbagdi hbagdi closed this as completed Apr 5, 2023
ADD-SP pushed a commit that referenced this issue May 31, 2023
@giovanibrioni @ADD-SP
fix rate-limiting accuracy reported on issue: #5311
5debe06 
it is same fix made in PR #8227, but I remove sliding-window feature.
Kong have no plans to add a sliding window to this plugin, Sliding window is an enterprise-only feature at this point
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
plugins/rate-limiting plugins/response-ratelimiting task/needs-investigation Requires investigation and reproduction before classifying it as a bug or not.
Projects
None yet
Milestone
No milestone
6 participants
@kikito @hishamhm @bungle @hbagdi @perjerz @giovanibrioni