chore(session) do not read body by default and only on certain HTTP methods

[bungle]

Summary

On comment: #7418 (comment) @PidgeyBE mentioned that session plugin reads bodies by on every HTTP request that is not an GET request.

Because it is quite common to use bodies to send large files, reading body makes features like route.request_buffering=off, not working. Thus, the default value for logout_post_arg in session plugin was removed. The bodies are only read when this is configured. This might change behaviour on scripts that create session plugin and which also think that logout by body argument works as before. On the other hand it is a way more common to read session than it is to log out session, thus it should be better value for future of us.

KAG-634

[hbagdi]

Aapo, is there a way to fix the performance problem without a breaking change?
Perhaps giving users a knob to disable reading bodies (and then calling that out in documentation)?

I'm all for fixing performance issues but I don't agree that this warrants a breaking change in Kong. Sessions library changes in 3.2 have enough breaking changes, we should not pile on that and instead try to limit them as much as possible.
What do you think?

[PidgeyBE]

Aapo, is there a way to fix the performance problem without a breaking change? Perhaps giving users a knob to disable reading bodies (and then calling that out in documentation)?

I'm all for fixing performance issues but I don't agree that this warrants a breaking change in Kong. Sessions library changes in 3.2 have enough breaking changes, we should not pile on that and instead try to limit them as much as possible. What do you think?

If you are looking for a non-breaking compromise, you could always try to check the Content-Length header and only get the body if it is below a certain threshold... In this way the performance issue is fixed for (large) file uploads and it won't break logout flows (which should be small requests in size).

[bungle]

I'm all for fixing performance issues but I don't agree that this warrants a breaking change in Kong.

As explained it is not only a performance fix, it also makes route.request_buffering=false to not work, so it can be viewed as a bug too. It does not break any existing configurations as they already have the default value stored in db. Just some scripts that rely on default value. Not a huge breakage.

[bungle]

If you are looking for a non-breaking compromise, you could always try to check the Content-Length header and only get the body if it is below a certain threshold... In this way the performance issue is fixed for (large) file uploads and it won't break logout flows (which should be small requests in size).

Just wondering whether it is too magical.

[bungle]

Aapo, is there a way to fix the performance problem without a breaking change? Perhaps giving users a knob to disable reading bodies (and then calling that out in documentation)?

The knob is there, aka set logout_post_arg actively to null. But that has turned out to be complicated with yamls/kic and stuff. If we add a knob it should be that bodies are not read by default to detect logout. In general the plugin should not do logout at all by default. You should configure separate plugin at /logout and enable it there. But that would be even more breaking.

[hbagdi]

It does not break any existing configurations as they already have the default value stored in db. Just some scripts that rely on default value. Not a huge breakage.

That's simplifying the breaking change problem a bit.
Whenever these defaults change, alternate control planes like Koko which work with DPs of multiple versions at the same time have a hard time adopting this change since there can be only one default (Kong schemas are not versioned whatsoever).

If the default value before this patch is captured in decK, KIC, etc, then the effectiveness of this "bug-fix" is questionable as well since fixing the bug actually requires more work than upgrading the gateway.

Are you more open to an alternate where we introduce a net new schema field that toggles the behavior of reading bodies or not?
This is still a breaking change in terms of the behavior of Kong but it is not a breaking change to the schema of Kong. We are sensitive to both kinds of breaking changes but the schema ones are more problematic currently. This further reduces the likelihood that a user will still run into this bug (the bug being reading bodies even with buffering disabled) after an upgrade.

[bungle]

Are you more open to an alternate where we introduce a net new schema field that toggles the behavior of reading bodies or not?

As said, there is already an option, just actively set logout_post_arg to null. The change in this PR was trying to make it default as having better defaults means better usability and that adding session plugin with default settings to Kong will not screw up the performance and other features. I am open adding feature read_bodies or something with a default value of false, but I don't think it makes sense to add one with default value of true.

[hbagdi]

Can we move this up into the "Breaking changes" section?

[hbagdi]

I'm adding the 3.4 milestones to this patch. It seems awfully close and is a worthwhile performance change. Recommend changing the commit type to 'perf'.

[hanshuebner]

I have tried to understand the pull request description and the CHANGELOG entry, but I found both very difficult to follow. It seems that there is consensus that the implementation makes sense, but maybe the verbiage can be improved. Happy to jump onto a Zoom to help with this.

[kikito]

I'm removing this from the milestone, we are too close to the GA date and have no people available to review/approve this in the meantime.

[hanshuebner]

@hbagdi We discussed whether we should really treat this as a breaking change in the PR queue review meeting earlier and these is some consensus that it is unlikely that someone relies on the current undocumented behavior. Thus, merging this PR should be fine, even if we document it as breaking change in the change log. What do you think?

[jschmid1]

@bungle this seems to not made any progress. Do you still think this should be targeted to 3.5?

[jschmid1]

To summarize this patch:

Currently we're reading the body for every request as logout_post_arg is set to a non-null default. This forces us to look into every request body by default to detect if a logout is requested.
Reading bodies can become expensive, especially when requests with large payloads are sent. To resolve this issue there are a couple of options.

1. Explicitly disable the logout functionality.

This affects a rather large group of people and doesn't really solve the issue at hand. When enabling this flag, you'd still have to set logout_post_arg to null to avoid body reads alltogether.

2. Disable body reads by default

This will also affect some people but the surface is smaller in comparison to (1). To get this functionality back, a user can re-enable this flag.

3. Change the default of logout_post_arg

This is technically speaking the most straightforward fix. Changing defaults will affect other products though and should be avoided if possible (@hbagdi agrees here)

This leaves us with (2) as it has the lowest impact and achieves what this patch tries to solve.

What I'm not happy with is the name of the config flag as it doesn't reflect the scope this parameter is used in. My proposal is

read_body_for_logout (move from bodies to body, define the scope - for logout)

resolved