-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
base: master
Are you sure you want to change the base?
fix(response-ratelimiting): fix missing usage headers for upstream #13696
Conversation
@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. |
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 |
be001e0
to
d785d18
Compare
d785d18
to
7b344bb
Compare
@t-yuki Could you fix failure tests? |
OK, I'll try to continue within a week. |
7b344bb
to
9d4e821
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changelog/unreleased/kong/fix-response-ratelimiting-upstream-headers.yml
Outdated
Show resolved
Hide resolved
…eaders.yml Co-authored-by: Guilherme Salazar <[email protected]>
@@ -18,7 +18,16 @@ local fmt = string.format | |||
local proxy_client = helpers.proxy_client | |||
|
|||
|
|||
local function wait() | |||
local function wait(reset) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- 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-setss.000 ms
, the nginx process might be still atss.998 ms
.- Adding several jitters may relax this problem, but in my environment, it requires several hundred ms (WSL2+Docker)
- Some of test takes several hundred ms to run, especially, iteration based-tests such as
for i = n+1, ITERATIONS do
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good effort on making the tests more stable. However, we are doing refinement on the test cases, which is not just waiting for a longer time, we will rely on some status endpoint to make the waiting more reasonable and waiting time as short as possible.
So could you just make the functional changes, and revert the test spec changes? So that I could approve and merge your code. Much appreciates on your effort!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, It's reverted. However, I'm not sure it passes the tests on CI environments. It never passes the tests in my local environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think you can leave the test name unchanged(keep the "flaky" in description), we will remove it after we make it stable.
changelog/unreleased/kong/fix-response-ratelimiting-upstream-headers.yml
Outdated
Show resolved
Hide resolved
…eaders.yml Co-authored-by: BrianChen <[email protected]>
…limiting-usage-headers
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
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdIssue reference
Fix #13682