kong
kong copied to clipboard
Kong
chore(session) do not read body by default and only on certain HTTP methods
Summary
On comment: https://github.com/Kong/kong/issues/7418#issuecomment-1411729346 @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
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?
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).
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.
If you are looking for a non-breaking compromise, you could always try to check the
Content-Lengthheader 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.
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.
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.
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.
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'.
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.
@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?
@bungle this seems to not made any progress. Do you still think this should be targeted to 3.5?
