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

feat(proxy-cache): add ignore_uri_case to configuring cache-key uri to be handled as lowercase #10453

Merged
merged 12 commits into from
Mar 15, 2023
Merged

feat(proxy-cache): add ignore_uri_case to configuring cache-key uri to be handled as lowercase #10453

merged 12 commits into from
Mar 15, 2023

Conversation

tobiasehlert
Copy link
Contributor

@tobiasehlert tobiasehlert commented Mar 8, 2023

Summary

The proxy-cache plugin uri input is taken straight from ngx.var.request and when incoming requests are randomized with lowercase and uppercase letters, all requests are handled as new ones.

The issue with this is that the proxy-cache sees then as unique requests, even if the backend request response is exactly the same and the cache-key should therefore be possible to set to lowercase that upstream servers are not receiving unnecessary requests.

Example of requests being randomized and handled as unique cache entries:

  • /api/greeting/tobias
  • /api/greeting/Tobias
  • /api/greeting/tObIaS

Default of the ignore_uri_case is false and the cache-key will be based on the actual ngx.var.request, but when setting the parameter to true instead, it will be lowercased instead.

Checklist

Full changelog

  • Implementing new parameter called ignore_uri_case for the proxy-cache plugin to lowercase the request and use that for cache-key calculation.

Discussion reference

Rel #7821 & #10425

@CLAassistant
Copy link

CLAassistant commented Mar 8, 2023

CLA assistant check
All committers have signed the CLA.

@ghost
Copy link

ghost commented Mar 9, 2023

hi @tobiasehlert ,add test case in #10466

@tobiasehlert
Copy link
Contributor Author

hi @tobiasehlert ,add test case in #10466

@sabertobihwy should I close this PR? Your PR good good 👌

@hbagdi
Copy link
Member

hbagdi commented Mar 9, 2023

Hey @sabertobihwy, thank you for your PR. Could you please close it? We want contributors to work through the PR process and get to the finish line of merge. We wish to give everyone an opportunity to submit a clean and complete patch. If the PRs stall, then we can take someone's contribution and build on top of it (provided they have signed the CLA and we give them due credit).

@tobiasehlert Could you please write a test for this?

@ghost
Copy link

ghost commented Mar 10, 2023

Hey @sabertobihwy, thank you for your PR.

OK, I'll close my PR. i just want to help @tobiasehlert write a test case. But I don't have permission to update it on his PR. So let's work together on this PR.

@tobiasehlert tobiasehlert changed the title feat(proxy-cache): add cache_lowercase_uri to configuring cache-key uri to be handled as lowercase feat(proxy-cache): add ignore_uri_case to configuring cache-key uri to be handled as lowercase Mar 10, 2023
@pull-request-size pull-request-size bot added size/M and removed size/S labels Mar 10, 2023
@hbagdi hbagdi requested a review from samugi March 10, 2023 21:15
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.

Looks good, just a couple of small suggestions

spec/03-plugins/31-proxy-cache/02-access_spec.lua Outdated Show resolved Hide resolved
spec/03-plugins/31-proxy-cache/02-access_spec.lua Outdated Show resolved Hide resolved
spec/03-plugins/31-proxy-cache/02-access_spec.lua Outdated Show resolved Hide resolved
@samugi
Copy link
Member

samugi commented Mar 14, 2023

Note: the schema-change-labels check is expected to fail until this PR: #10479 is merged

@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 14, 2023
@tobiasehlert tobiasehlert requested review from samugi and removed request for hbagdi March 14, 2023 21:19
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.

One more thing @tobiasehlert, to keep compatibility with hybrid mode we need to add the new configuration parameter to the removed_fields for dataplanes older than 3.3.0, essentially adding this bit:

proxy_cache = {
  "ignore_uri_case",
}

in this section:

[3003000000] = {
acme = {
"account_key",
}
},

for compability in hybrid mode
@tobiasehlert
Copy link
Contributor Author

One more thing @tobiasehlert, to keep compatibility with hybrid mode we need to add the new configuration parameter to the removed_fields for dataplanes older than 3.3.0, essentially adding this bit:

Sound important to add! I did that in 402f3e8 now :)

@tobiasehlert
Copy link
Contributor Author

@samugi seems like something faulty in the second test.. https://github.com/Kong/kong/actions/runs/4425523590/jobs/7760716719#step:9:711

You have a clue why it complains on line 1435? :|

@samugi
Copy link
Member

samugi commented Mar 15, 2023

should be good now @tobiasehlert

@tobiasehlert
Copy link
Contributor Author

should be good now @tobiasehlert

Awesome, thanks for the support @samugi 🥇

@hanshuebner hanshuebner merged commit 78d75b4 into Kong:master Mar 15, 2023
@tobiasehlert tobiasehlert deleted the add-cache-key-uri-lowercase-parameter branch March 15, 2023 12:17
@samugi
Copy link
Member

samugi commented Mar 15, 2023

thank you for your contribution @tobiasehlert !!

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.

5 participants