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.

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

fix(response-ratelimiting): fix missing usage headers for upstream #13696

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

t-yuki
Copy link

@t-yuki t-yuki commented Sep 20, 2024

Summary

response-ratelimiting plugin should send usage headers to upstream server.
But in Kong 3.8, there are no usage headers such as X-RateLimit-Remaining-Videos: 10 for upstream requests.

In this change, it coming back usage headers for upstream.

Checklist

Issue reference

Fix #13682

@ADD-SP
Copy link
Contributor

ADD-SP commented Sep 20, 2024

@t-yuki Thanks for your contribution! Would you mind adding a regression test on this issue? I think we have missed this test for a long time.

And also a changelog entry.

@ADD-SP ADD-SP added the author/community PRs from the open-source community (not Kong Inc) label Sep 20, 2024
@ADD-SP ADD-SP added this to the 3.9.0 milestone Sep 20, 2024
@ADD-SP
Copy link
Contributor

ADD-SP commented Sep 20, 2024

Since this is a new issue introduced in 3.8 and should be a small fix, I added this to the 3.9 milestone.

Internal ticket: KAG-5447

@t-yuki t-yuki force-pushed the fix-response-ratelimiting-usage-headers branch from be001e0 to d785d18 Compare September 21, 2024 12:40
@pull-request-size pull-request-size bot added size/L and removed size/S labels Sep 21, 2024
@t-yuki t-yuki force-pushed the fix-response-ratelimiting-usage-headers branch from d785d18 to 7b344bb Compare September 21, 2024 12:46
@ADD-SP ADD-SP self-requested a review September 23, 2024 02:59
@ADD-SP
Copy link
Contributor

ADD-SP commented Sep 23, 2024

@t-yuki Could you fix failure tests?

@t-yuki
Copy link
Author

t-yuki commented Sep 25, 2024

@t-yuki Could you fix failure tests?

OK, I'll try to continue within a week.

@t-yuki t-yuki force-pushed the fix-response-ratelimiting-usage-headers branch from 7b344bb to 9d4e821 Compare October 15, 2024 06:47
Copy link
Member

@gszr gszr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is touching a recent nontrivial refactor, we should carefully verify it doesn't revert important bits of said refactor, fixing one but introducing other issues. cc @ADD-SP

@kikito kikito removed this from the 3.9.0 milestone Dec 3, 2024
@@ -18,7 +18,16 @@ local fmt = string.format
local proxy_client = helpers.proxy_client


local function wait()
local function wait(reset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that wait time granularity change from milli seconds to dozens of seconds, is there a reason? I think this will make the CI test cases spend much more time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, changing millis to seconds was due to avoid unstability of tests.

  1. As far as I tried, the nginx process and the test process are not synced well in the ms order. Thus, when the test process sleeps with ngx.sleep(1-ms) to on-set ss.000 ms, the nginx process might be still at ss.998 ms.
    • Adding several jitters may relax this problem, but in my environment, it requires several hundred ms (WSL2+Docker)
  2. Some of test takes several hundred ms to run, especially, iteration based-tests such as must be completed 10 requests in 1secs, that is not stable.

In this change, wait(reset) function will only sleep if current time is at 50-59s or counter-reset is required. Since most of tests are separated into different limit counters, the CI will sleep only 1 to 3 times during tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author/community PRs from the open-source community (not Kong Inc) backport release/3.9.x cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee plugins/response-ratelimiting size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[response-ratelimiting] Missing upstream usage headers in Kong 3.8
5 participants