feat(proxy): Allow to remove the proxy cache headers

[utix]

Update of #7829

Summary

Adding Miss/Hit, Age and cache key to answer shouldn't be the default behavior.
Add option to remove the proxy cache headers from the response.
If the kong debug is set keep the headers

Full changelog

The following headers:

• X-Cache-Status
• X-Cache-Key
• Age

can be now be removed from the answer via the response_headers option,
for example to remove all:

        config = {
          [...]
          response_headers = {
            age = false,
            ["X-Cache-Status"] = false,
            ["X-Cache-Key"]  = false
          }

Default behavior to have all the header set is kept for retro compatibility reason.

Refactor test to remove duplicated lines and use get, post and delete helpers instead of send

[hanshuebner]

@hbagdi Please give this your final review.

[hbagdi]

Is this potentially a breaking change in terms of the behavior of the proxy?

[utix]

@hbagdi I have described the impact.

Remove proxy cache headers from response if the kong debug is not set.

Nothing has changed on the caching part, just the headers added in the response.
imho such header shouldn't be added by default.

If you think it is a breaking change I can add a configuration param in addition of debug mode to control if headers are added

[hanshuebner]

Remove proxy cache headers from response if the kong debug is not set.

Nothing has changed on the caching part, just the headers added in the response. imho such header shouldn't be added by default.

The reason why we've not just merged this change is that even though your opinion matches ours, we don't know how many customers rely on the incorrect behavior of Kong. We're still in the decision process.

If you think it is a breaking change I can add a configuration param in addition of debug mode to control if headers are added

Adding a specific configuration parameter to disable incorrect behavior of previous versions of Kong should be the last resort, but we may want to go that route and make the correct change, like you proposed, in the next major release. Please bear with us for a couple of more days.

[hbagdi]

@hanshuebner How is this not a breaking change?

[hanshuebner]

@hbagdi All the arguments have been made. From your question, I take it that you don't agree that we can merge this. Please let us know how to proceed.

[kikito]

@hbagdi The gist of the problem here is the interpretation of what the headers X-Cache-Status, X-Cache-Key, Age are.

• If we interpret that they are "headers that are only useful when debugging a problem" then generating them all the time was a bug. Anyone relying on this for doing this was thus relying on buggy behavior. So with this interpretation, merging this is not a breaking change (because breaking buggy behaviour is not a breaking change).
• If we interpret that "the way things were working before is acceptable" - that sending those headers all the time is correct, then this is a change in functionality and a breaking change. The soonest we would be able to merge this would be with a proper deprecation on Kong 4.0. We could start emitting deprecation messages in the logs in the meantime.

[utix]

Not sure that not being able to not sent the header was acceptable.
If you think this is a breaking change, I can add a flag to add them or not, set to true by default.
And when the flag is set to false, the debug mode will add them at demand.

[hbagdi]

The API was not clear and the RFCs do not make it obvious that these headers are for debugging.
Doing a breaking change without giving our users a way out (being able to get back the old behavior) for such an API should be avoided.

I recommend adding a flag as @utix suggests to avoid this breaking change, especially since the plugin has been around for a long time.

[hbagdi]

Another alternate solution is to go for an "allowlist" based approach which gives user the control over which headers should be emitted by Kong when this plugin is run. That essentially gives user a toggle over each header and allows the user to emit the cache status header (generally considered useful) and hide the cache key header (generally hidden) and other combinations.

[utix]

I was planning to add a flag for each header, to put it on the answer. set to true by default to keep the behavior
And force all on debug mode

[hbagdi]

Is a single configuration field that is an array of string better? Or worse?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[utix]

I have added a response_headers field into the configuration with 3 fields:

• age
• X-Cache-Status
• X-Cache-Key

Allowing to set or unset the presence of the header into the response.

As discussed, to keep previous behavior, all fields are present by default.

[hanshuebner]

Almost there! Now there is only the missing CHANGELOG entry and the docs update to be done. Also, the pull request title and description need some work to reflect what the change is about. @utix can you do that?

[hbagdi]

This needs a compat change as well i.e. addition to removed_fields.lua.

[hbagdi]

Closing because of lack of activity. We would like to accept this contribution if anyone wishes to pick this back up.

[utix]

Not sure what is missing, just rewords the Merge request?

…

On Mon, Jun 26, 2023, 20:00 Harry ***@***.***> wrote: Closed #10445 <#10445>. — Reply to this email directly, view it on GitHub <#10445 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB345OTE5PTWI3TH64KCCTXNHE2FANCNFSM6AAAAAAVSQJISI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[hbagdi]

That and #10445 (comment). Please re-open if you want to take this to the finish line.

[utix]

I have reword the PR, changelog commit is done on my fork, I don't have permission to reopen the PR.
Not sure what is a compat change, here as asked the previous configuration will still trigger the same behavior.

[hanshuebner]

@utix The removed_fields.lua update is required to make older dataplanes compatible with newer control planes. When preparing a configuration to be sent to a data plane, the control plane removes configuration fields that are present in the current configuration but that were not supported in the version of the target data plane. The removed_fields.lua file lists, for each version, what fields were added. Please include the new configuration fields in the section for 3.3->3.4.

[utix]

@hanshuebner is it good now ? If something else is needed can you highlight it ?

[hanshuebner]

@utix Finally merged, thank you for the contribution and your efforts getting it over the line!

[utix]

Thanks for your support to make it happen

[outsinre]

Will double-check when CE2EE merge.