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

fix(proxy-cache): follow request Cache-Control: no-cache header field #14139

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

fix(proxy-cache): follow request Cache-Control: no-cache header field #14139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d779eb4
fix(proxy-cache): let request no-cache arguement functionality follow…
oowl Jan 13, 2025
77d6931
fix(proxy-cache): let request no-cache arguement functionality follow…
oowl Jan 13, 2025
5a02f4b
fix: fix code
oowl Jan 17, 2025
d821d32
fix: fix code
oowl Jan 17, 2025
91afdc4
fix: fix code
oowl Jan 17, 2025
9b99ddc
fix: fix code
oowl Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/unreleased/kong/fix-proxy-cache-plugin-follow-no-cache-header.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
message: "**proxy-cache**: let cache action follow request `cache-control` header `no-cache` field base on rfc7234"
type: bugfix
scope: Plugin
12 changes: 9 additions & 3 deletions kong/plugins/proxy-cache/handler.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,7 @@ local function cacheable_request(conf, cc)
end

-- check for explicit disallow directives
-- TODO note that no-cache isnt quite accurate here
if conf.cache_control and (cc["no-store"] or cc["no-cache"] or
if conf.cache_control and (cc["no-store"] or
ngx.var.authorization) then
return false
end
Expand Down Expand Up @@ -260,15 +259,22 @@ function ProxyCacheHandler:access(conf)
return
end

local ctx = kong.ctx.plugin

set_header(conf, "X-Cache-Key", cache_key)
if conf.cache_control then
if cc["no-cache"] then
return signal_cache_req(ctx, conf, cache_key, "Bypass")
Copy link
Contributor

Choose a reason for hiding this comment

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

If we return here, we're not caching the request body.

Copy link

Choose a reason for hiding this comment

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

@locao ,
Just out of curiosity, why are we caching the request body? Used somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

@khaled4vokalz yes, it is not used directly by this plugin, but exposed in the request ctx variable so logging plugins can use it to log full request details.

Copy link

Choose a reason for hiding this comment

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

I understand sharing it via the ctx but as we're using ngx.shared memory space to cache things, caching some object which is not used for anything seems a bit ambitious to me. There can be large request bodies from clients. That's the reason I asked. I was wondering if it'll be a good idea to remove the request_body from the cached object to save some space.

One more thing here is that the ctx we're using in the plugin itself is the plugin context that's not shared between plugins for use, rather it's only available in this plugin's lifecycle.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if it'll be a good idea to remove the request_body from the cached object to save some space.

Removing it now is not expected by custom plugins which can use this field. It can go from just changing the behavior to break someone's logging process completely.

But if you're using Kong Enterprise Edition, you can select redis as a caching strategy :)

the ctx we're using in the plugin itself is the plugin context

Actually, we copy the cached data to the shared context when cache is hit.

end
end


-- try to fetch the cached object from the computed cache key
local strategy = require(STRATEGY_PATH)({
strategy_name = conf.strategy,
strategy_opts = conf[conf.strategy],
})

local ctx = kong.ctx.plugin
local res, err = strategy:fetch(cache_key)
if err == "request object not in cache" then -- TODO make this a utils enum err

Expand Down
12 changes: 12 additions & 0 deletions spec/03-plugins/31-proxy-cache/02-access_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,7 @@ do

assert.res_status(200, res)
assert.same("Miss", res.headers["X-Cache-Status"])

--local cache_key = res.headers["X-Cache-Key"]

-- wait until the underlying strategy converges
Expand All @@ -611,6 +612,17 @@ do
assert.res_status(200, res)
assert.same("Hit", res.headers["X-Cache-Status"])

local res = assert(client:get("/cache/2", {
headers = {
host = "route-7.test",
["Cache-Control"] = "no-cache",
}
}))

assert.res_status(200, res)
assert.same("Bypass", res.headers["X-Cache-Status"])
assert.is_not_nil(res.headers["X-Cache-Key"])

-- if strategy is local, it's enough to simply use a sleep
if strategies.LOCAL_DATA_STRATEGIES[policy] then
ngx.sleep(3)
Expand Down
Loading