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(runloop): query args not forwarded bug #11328

Merged
merged 1 commit into from
Aug 6, 2023

Conversation

chirag-manwani
Copy link
Contributor

@chirag-manwani chirag-manwani commented Jul 29, 2023

Summary

This PR fixes the issue described in #11325

Checklist

  • The Pull Request has tests

Full changelog

Fix query args handling code introduced in this commit.

Issue reference

Fix #11325

@CLAassistant
Copy link

CLAassistant commented Jul 29, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
11 out of 12 committers have signed the CLA.

✅ vm-001
✅ flrgh
✅ oowl
✅ chronolaw
✅ windmgc
✅ ADD-SP
✅ samugi
✅ curiositycasualty
✅ outsinre
✅ zhongweiy
✅ hanshuebner
❌ chirag-manwani
You have signed the CLA already but the status is still pending? Let us recheck it.

@chirag-manwani chirag-manwani changed the base branch from master to release/3.3.x July 29, 2023 17:47
@chirag-manwani chirag-manwani changed the title Fix/query args not forwarded fix(balancer): fix query args not forwarded bug Jul 29, 2023
@chirag-manwani chirag-manwani changed the title fix(balancer): fix query args not forwarded bug fix(balancer): query args not forwarded bug Jul 29, 2023
@chirag-manwani
Copy link
Contributor Author

@samugi Can this change be released in the next patch version of Kong 3.3.x?

@oowl
Copy link
Member

oowl commented Aug 4, 2023

@samugi Can this change be released in the next patch version of Kong 3.3.x?

In general, we do not do this backport to the old kong community old version.

Copy link
Member

@oowl oowl left a comment

Choose a reason for hiding this comment

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

Mostly is Okay for me, Please fix failure CI!

@chirag-manwani chirag-manwani force-pushed the fix/query-args-not-forwarded branch 2 times, most recently from 5085407 to 573361a Compare August 4, 2023 08:18
@chirag-manwani
Copy link
Contributor Author

@samugi @oowl Could you approve the workflow runs once more?

@chirag-manwani
Copy link
Contributor Author

chirag-manwani commented Aug 4, 2023

The DB less integration test is still failing, and I am unsure as to why. Should this test be skipped for db less?

@chirag-manwani
Copy link
Contributor Author

chirag-manwani commented Aug 4, 2023

I noticed that the tests I was running on local were only running for postgres. Once I made them run for dbless, the tests failed. On further search I found that the route I am creating is being created, but the plugin I am applying on that route is not being created.
Is there something wrong in the way I've setup the test?

Update:
NVM, I found why the plugin was not being set. The complete config for the test is being set in insert_routes. In case of db strategy, the bp variable is set to admin_api and setting config outside this function works fine. In case of dbless, the config set until that point by bp variable is used to create a declarative config, and since I was setting the config after the insert_routes function was called, the plugin config was ignored.

@chirag-manwani
Copy link
Contributor Author

@samugi @oowl Please allow the workflow once more. I've updated the tests to work for dbless mode also. All tests should pass now.

@chirag-manwani
Copy link
Contributor Author

@oowl @samugi
An unrelated test case failed. Is this a flaky test case? The tests should just be re-run in this case right?

FAILED  spec/02-integration/05-proxy/10-balancer/01-healthchecks_spec.lua:1893: Ring-balancer #postgres #healthchecks (#cluster #db) #hostname #healthchecks perform active health checks on targets that resolve to the same IP -- automatic recovery #http
...ntegration/05-proxy/10-balancer/01-healthchecks_spec.lua:1995: Expected values to be near.
Passed in:
(number) 5
Expected:
(number) 10 +/- 3

@chirag-manwani
Copy link
Contributor Author

Please re-run the failed workflow when you have the time.
@oowl @samugi

Copy link
Member

@samugi samugi left a comment

Choose a reason for hiding this comment

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

thank you @chirag-manwani for your efforts writing this test.
Would you mind moving it out of the 02-router_spec.lua file, since this is not exactly a router test? I believe it would be appropriate to create a new test file, e.g. spec/02-integration/05-proxy/32-query-params_spec.lua.

For this specific change, feel free to use the default output of helpers.each_strategy(), which won't require extra work to make the dbless test pass, so that an implementation similar to your first attempt should work just fine.

@chirag-manwani
Copy link
Contributor Author

@samugi Added the test case in a separate file.

@github-actions github-actions bot added the chore Not part of the core functionality of kong, but still needed label Aug 5, 2023
@github-actions github-actions bot removed the chore Not part of the core functionality of kong, but still needed label Aug 5, 2023
query args that are set in a plugin by kong are not forwarded to the
upstream in case of empty query args in request but with a ?
(e.g. /route?)
also add test to validate the same

Fixes Kong#11325
@chirag-manwani
Copy link
Contributor Author

@samugi
All tests have passed, please merge the PR. I have one additional request though. If possible please make this change available in any patch version of Kong 3.3.x

Copy link
Member

@oowl oowl left a comment

Choose a reason for hiding this comment

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

LGTM!

@samugi samugi merged commit aa3abd2 into Kong:master Aug 6, 2023
20 checks passed
@samugi
Copy link
Member

samugi commented Aug 6, 2023

thanks again for your contribution @chirag-manwani! As @oowl also mentioned before I believe there is no plan to backport this right now, but we will update you here if that changes.

@samugi samugi changed the title fix(balancer): query args not forwarded bug fix(runloop): query args not forwarded bug Aug 6, 2023
chronolaw added a commit that referenced this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query Args not forwarded in proxied request
4 participants