fix(proxy-cache): set Header `Accept-Encoding` as default values of vary_headers #12867

git-hulk · 2024-04-16T13:45:42Z

Summary

Currently, proxy-cache might mix the compression and uncompression values while caching, and return unexpected compression response to the request if users didn't specify the vary_headers.

Checklist

The Pull Request has tests
A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #12796

CLAassistant · 2024-04-16T13:45:49Z

All committers have signed the CLA.

CLAassistant · 2024-04-16T13:45:49Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

…ary_headers Currently, proxy-cache might mix the compression and uncompression values while cacheing, and return runexpected compression response to the request if users didn't specify the vary_headers.

StarlightIbuki

LGTM. I will add a changelog for this and approve the PR after it passes the tests

spec/03-plugins/31-proxy-cache/03-api_spec.lua

git-hulk · 2024-04-18T02:22:08Z

@chronolaw @oowl could you help to take a look at this PR?

oowl · 2024-04-18T05:28:07Z

kong/plugins/proxy-cache/schema.lua

@@ -72,6 +72,7 @@ return {
            elements = { type = "string" },
          }},
          { vary_headers = { description = "Relevant headers considered for the cache key. If undefined, none of the headers are taken into consideration.", type = "array",
+            default = { "Accept-Encoding" },


It seems that have introduced breaking change. I am not sure if it is safe for our old version user.

cc @dndx Can you help us to check it?

Thanks for your review. Guess it shouldn't be regarded as a breaking change since it only affects how the cache key is calculated.

I don't consider fixing a broken behavior a breaking change. ping @dndx

StarlightIbuki · 2024-04-19T07:06:01Z

I did not think it through when I wrote the original comment.

The RFC says:

An origin server might send Vary with a list of fields for two
purposes:

To inform cache recipients that they MUST NOT use this response
to satisfy a later request unless the later request has the same
values for the listed fields as the original request (Section 4.1
of [RFC7234]). In other words, Vary expands the cache key
required to match a new request to the stored cache entry.

So it's a must to support Vary header to comply with RFC.

I found a todo in the plugin's implementation:

kong/kong/plugins/proxy-cache/handler.lua

Line 435 in 7dd29d4

-- TODO handle Vary header

It's better to fix the issue here. Plus this PR could still be useful. If an upstream does not make use of the Vary header correctly, we are supporting it by default (though doubtful, as the upstream may want it to not vary).

Also, it's noteworthy that graphql-proxy-cache-advanced uses a similar way to handle the cache_key.

git-hulk · 2024-04-19T08:21:34Z

@StarlightIbuki So do you suggest adding the Vary response header while hitting the cache in this PR?

StarlightIbuki · 2024-04-19T08:35:17Z

@StarlightIbuki So do you suggest adding the Vary response header while hitting the cache in this PR?

We need to add content from the Vary header to the list of vary_headers before generating the cache key. And to get a constant result we may need to sort the header names.

git-hulk · 2024-04-19T09:12:16Z

@StarlightIbuki So do you suggest adding the Vary response header while hitting the cache in this PR?

We need to add content from the Vary header to the list of vary_headers before generating the cache key. And to get a constant result we may need to sort the header names.

@StarlightIbuki Thanks for the clarification. I misunderstood your point. I'll make sure to recapture your point and help to correct me if I'm wrong.

Check if the upstream response has the Vary header, and add it into vary_headers if exists and it has not been added before.
sort the vary_headers and build the cache key, then cache the response.

That said if we have the config.vary_headers = {"A"} and the upstream server sends two responses with Vary: A,B and Vary: B,C, then the config.vary_headers will finally become {A, B, C}. Is it right?

By the way, I'm not sure if it's good to add this in another PR to make the context more clear? I'm also happy to do that.

StarlightIbuki · 2024-04-22T02:45:20Z

@StarlightIbuki So do you suggest adding the Vary response header while hitting the cache in this PR?

We need to add content from the Vary header to the list of vary_headers before generating the cache key. And to get a constant result we may need to sort the header names.

@StarlightIbuki Thanks for the clarification. I misunderstood your point. I'll make sure to recapture your point and help to correct me if I'm wrong.

Check if the upstream response has the Vary header, and add it into vary_headers if exists and it has not been added before.

sort the vary_headers and build the cache key, then cache the response.

That said if we have the config.vary_headers = {"A"} and the upstream server sends two responses with Vary: A,B and Vary: B,C, then the config.vary_headers will finally become {A, B, C}. Is it right?

By the way, I'm not sure if it's good to add this in another PR to make the context more clear? I'm also happy to do that.

Thanks for offering help. Perhaps a dedicated PR for Vary header support is good.

Note we also need to make sure there are no duplicated headers.

git-hulk · 2024-04-22T04:02:56Z

Note we also need to make sure there are no duplicated headers.

Sure, thanks for your reply. I will take care of this condition. I won't submit an issue to track this enhancement since Kong's issue is only for bug reports.

BTW, I'm not sure how to push this PR forward to reviewing. To see if any maintainers can help.

StarlightIbuki · 2024-04-22T06:38:35Z

BTW, I'm not sure how to push this PR forward to reviewing. To see if any maintainers can help.

Please understand that we want the changes to Kong to align with the roadmap and the overall design, and it takes time to decide the solution. This PR particularly needs no more effort from your side and I will try to get some attention for this topic.

StarlightIbuki · 2024-04-22T06:48:36Z

Internally tracked: KAG-4292

git-hulk · 2024-04-22T06:56:24Z

@StarlightIbuki That's great, thanks a lot for your help.

git-hulk · 2024-05-31T09:57:42Z

Hi @StarlightIbuki, want to know if this fix discussion has any progress?

chronolaw · 2024-05-31T10:14:42Z

I checked the ticket (KAG-4292), it seems that there are still some discussions on it, no decision is out.

pull-request-size bot added the size/S label Apr 16, 2024

github-actions bot added plugins/proxy-cache schema-change-noteworthy cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Apr 16, 2024

fix(proxy-cache): set Header Accept-Encoding as default values of v…

07dcb59

…ary_headers Currently, proxy-cache might mix the compression and uncompression values while cacheing, and return runexpected compression response to the request if users didn't specify the vary_headers.

git-hulk force-pushed the fix/mix-response-in-proxy-cache branch from 1e60671 to 07dcb59 Compare April 16, 2024 13:48

team-eng-enablement added the author/community PRs from the open-source community (not Kong Inc) label Apr 16, 2024

StarlightIbuki reviewed Apr 17, 2024

View reviewed changes

spec/03-plugins/31-proxy-cache/03-api_spec.lua Show resolved Hide resolved

Add cache hit test case

38d5222

git-hulk requested a review from StarlightIbuki April 17, 2024 04:25

Create fix-proxy-cache-encoding.yml

c6a7a9a

pull-request-size bot added size/M and removed size/S labels Apr 17, 2024

StarlightIbuki approved these changes Apr 17, 2024

View reviewed changes

Update fix-proxy-cache-encoding.yml

1492089

oowl reviewed Apr 18, 2024

View reviewed changes

tobiasehlert mentioned this pull request May 2, 2024

Bug in proxy-cache plugin returning compressed cached content to clients not supporting compression #12796

Open

1 task

git-hulk requested a review from oowl May 31, 2024 05:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(proxy-cache): set Header `Accept-Encoding` as default values of vary_headers #12867

fix(proxy-cache): set Header `Accept-Encoding` as default values of vary_headers #12867

git-hulk commented Apr 16, 2024 •

edited by StarlightIbuki

Loading

CLAassistant commented Apr 16, 2024 •

edited

Loading

CLAassistant commented Apr 16, 2024

StarlightIbuki left a comment

git-hulk commented Apr 18, 2024

oowl Apr 18, 2024 •

edited

Loading

oowl Apr 18, 2024

git-hulk Apr 18, 2024

StarlightIbuki Apr 19, 2024

StarlightIbuki commented Apr 19, 2024 •

edited

Loading

git-hulk commented Apr 19, 2024 •

edited

Loading

StarlightIbuki commented Apr 19, 2024

git-hulk commented Apr 19, 2024 •

edited

Loading

StarlightIbuki commented Apr 22, 2024

git-hulk commented Apr 22, 2024

StarlightIbuki commented Apr 22, 2024

StarlightIbuki commented Apr 22, 2024

git-hulk commented Apr 22, 2024

git-hulk commented May 31, 2024

chronolaw commented May 31, 2024 •

edited

Loading

fix(proxy-cache): set Header Accept-Encoding as default values of vary_headers #12867

Are you sure you want to change the base?

fix(proxy-cache): set Header Accept-Encoding as default values of vary_headers #12867

Conversation

git-hulk commented Apr 16, 2024 • edited by StarlightIbuki Loading

Summary

Checklist

Issue reference

CLAassistant commented Apr 16, 2024 • edited Loading

CLAassistant commented Apr 16, 2024

StarlightIbuki left a comment

Choose a reason for hiding this comment

git-hulk commented Apr 18, 2024

oowl Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

oowl Apr 18, 2024

Choose a reason for hiding this comment

git-hulk Apr 18, 2024

Choose a reason for hiding this comment

StarlightIbuki Apr 19, 2024

Choose a reason for hiding this comment

StarlightIbuki commented Apr 19, 2024 • edited Loading

git-hulk commented Apr 19, 2024 • edited Loading

StarlightIbuki commented Apr 19, 2024

git-hulk commented Apr 19, 2024 • edited Loading

StarlightIbuki commented Apr 22, 2024

git-hulk commented Apr 22, 2024

StarlightIbuki commented Apr 22, 2024

StarlightIbuki commented Apr 22, 2024

git-hulk commented Apr 22, 2024

git-hulk commented May 31, 2024

chronolaw commented May 31, 2024 • edited Loading

fix(proxy-cache): set Header `Accept-Encoding` as default values of vary_headers #12867

fix(proxy-cache): set Header `Accept-Encoding` as default values of vary_headers #12867

git-hulk commented Apr 16, 2024 •

edited by StarlightIbuki

Loading

CLAassistant commented Apr 16, 2024 •

edited

Loading

oowl Apr 18, 2024 •

edited

Loading

StarlightIbuki commented Apr 19, 2024 •

edited

Loading

git-hulk commented Apr 19, 2024 •

edited

Loading

git-hulk commented Apr 19, 2024 •

edited

Loading

chronolaw commented May 31, 2024 •

edited

Loading