TS-4042: Add feature to buffer request body before making downstream requests #351

bgaff · 2015-11-25T02:19:46Z

We need a way to examine the request body without making a downstream request, this feature has many use cases including:

Ability to buffer the body and ensure a full post is received before committing downstream resources.
Ability to choose an origin based on request body
Ability to do request content filtering such as a WAF might provide before the origin is involved.

Today you have two options to inspect a request body:

Transformations: the problem with transformations is that you only start receiving the request bytes after a sink has been established, which in this case is the downstream origin.
Create an intercept and use fetch apis to then send the downstream request: while this technically works it turns out to be a ton of code and is in general pretty problematic, we actually tried this approach for a while and had nothing but problems with it.

We feel it would be ideal if we could intercept the body without breaking the normal ATS state flow. There used to exist code (and it's still in the core just #ifdefed out) to drain the request body. I use that code as the basis for this request buffering code. We added APIs to both the C and C++ APIs so that this request buffering can be enabled from a plugin and the plugin can inspect the body as chunks arrive or when it's complete. We've included an example plugin that will error a transaction if a minimum rate of transfer is not maintained. We've been using a very similar method in the core for buffer request bodies for several months without issues so the code that is new (for us) is basically all the API stuff.

I'm confident that this feature will bring plenty of questions / feedback, so let's get that party started. @zwoop @SolidWallOfCode @jpeach @sudheerv : if you have time would you mind commenting / reviewing.

cc. @jacksontj @zizhong @canselcik

jacksontj · 2015-11-25T22:18:14Z

Glad to see this PR finally make its way out :) We've been running with the base for this PR for quite a while. In addition to simplifying the code-- it really cleans up metrics/debugging as we are no longer required to loop requests back through the state machine.

jpeach · 2015-11-26T00:32:30Z

I quickly scanned the diff. The API changes will still need to go through API review on dev@. Documentation for the new APIs should be done as an API man page in the main documentation rather than in headerdoc (perfectly OK to do that as a subsequent patch).

bgaff · 2015-11-29T16:02:07Z

@jpeach sure thing. Does anyone else have comments regarding the approach before we discuss the individual APIs?

sudheerv · 2015-11-30T22:13:13Z

+1 on the approach.

A specific comment on a quick scan of the code - it seems like the patch only handles the case with Content-Length header, what about Chunked-Encoding or H2/SPDY scenarios that don't present the Content-Length header?

bgaff · 2015-12-07T03:54:28Z

@sudheerv , the chunked encoding case is one that I don't think any browser actually does, have you ever seen a browser make a request w/ chunked encoding if so I'll have to look into that case.

bgaff · 2015-12-07T05:26:37Z

@sudheerv, I've asked @zizhong to help in addressing the transfer-encoding chunked case, we should have an update soon.

…requests

bgaff · 2015-12-14T03:35:38Z

@sudheerv / @jpeach this pull request has been updated w/ tests for the chunked encoding case that @sudheerv was concerned about. Please let me know if you have any other questions about this?

sudheerv · 2015-12-14T16:05:39Z

@bgaff - Thanks, I'd have to double check though that the change made automatically handles a H2/SPDY upload. There's no Content-Length, nor even a TE header in those cases (although, perhaps, ATS implementation may make it appear like CHUNKED_ENCODING from FetchSM to HttpSM layer?)

bgaff · 2015-12-14T16:07:28Z

That's correct since both use fetchsm if just works like a normal http 1.1 request, as that's what fetch sm is. I've verified this functionality.

sudheerv · 2015-12-14T16:08:01Z

Cool, thanks!

bgaff · 2016-01-05T01:12:59Z

Any other questions about this?

bryancall · 2016-02-23T17:56:27Z

Talking at the github meeting. We should have a maximum size that will be buffered in memory. This should go through API review. Please provide documentation on the APIs.

zwoop · 2016-03-22T15:55:19Z

ping on API review?

zwoop · 2016-06-27T20:22:28Z

@bgaff What do you want to do with this? It's been sitting here for quite a while.

bryancall · 2016-08-16T02:22:16Z

plugins/request_buffer/request_buffer.cc

+        consumed += data_len;
+        block = TSIOBufferBlockNext(block);
+      }
+      // play with the body


Why are you copying the body if you aren't doing anything with it?

I think this is just a demo plugin that shows we can get the body.

bryancall · 2016-08-16T03:38:08Z

Looking over the pull request I didn't see any limits on how much is going to be buffered on the server. It might be good to start the transfer to the origin once a configurable limite gets reached.

jacksontj · 2016-08-16T14:46:34Z

@bryancall it'd be nice if we could have some way to "re-enable" the bytes-- such that a plugin could either buffer everything or stream (since the "buffer everything" case is a possible use-case.

zizhong · 2017-01-24T00:58:11Z

@bgaff @bryancall @zwoop What are the remaining works to get this pull request merged? Is there anything I can help with?

As a summary in this thread,

add a limit for the buffered body;
API review.

What else?

bryancall · 2017-01-24T05:27:41Z

I haven't seen an update to this PR for awhile. I will close it in a week if it hasn't been updated.

bgaff · 2017-01-24T10:17:46Z

I can help, @zizhong what of the previous asks haven't been completed?

zizhong · 2017-01-24T19:34:33Z

Thanks, @bgaff . After reading the previous comments, I think @bryancall suggested there should be a limit for the request body. And have you guys done API review about this PR?

YTSATS-1085: url-encoding the Location header

zizhong · 2017-03-06T19:52:34Z

@bryancall @bgaff @zwoop @SolidWallOfCode @jpeach @sudheerv Local patches of request buffer we have in Linkedin ATS repo really hurt us a lot(merge issues, etc). I'll work on updating this PR and push it out.
Before that, just want to make sure that upstream still agree on this approach.

Implement log throttling

* change MemArena::make test to remove memory leak (apache#8352) (cherry picked from commit 2a6156f) * Fix leaks in ConfigManager::configName (apache#8269) This fixes an ASan reported leak of ConfigManager::configName. It used to be strdup'd but not freed in the destructor. This simply changes it to a std::string. ASan also reported a leak in AddConfigFilesHere which is fixed with an ats_free as well. (cherry picked from commit ee820c7) * Lua plugin memory leak on remap configuration reloads (apache#8764) This fix adds reference counting for the Lua plugin remap instance handles. The reference counting allows us to eliminate an existing memory leak of the instance handles. In addition, this means that the old Lua memory allocated by LuaJIT may also be freed via LuaJIT garbage collection. This fix also adds the '--ljgc' remap instance plugin parameter to the Lua plugin. This paramter enables on-demand LuaJIT garbage collection while the remap instances are created and deleted. This is useful when operating close to the LuaJIT memory limit, which is currently 2GB on Linux using LuaJIT v2.1.0-beta3 from 2017. Fixes apache#8728 (cherry picked from commit b6f83f1) * Fixes leak of SNI config filename on load (cherry picked from commit e99f33c) * Fixes leak of ssl_ocsp_response_path_only on reload (cherry picked from commit 18c5404) * SNIConfig (tunnel_route): Change the way we extract matched subgroups from the server name. (apache#8589) This now uses the provided offsets from pcre_exec to read each matched group, this avoids allocating memory for the subgroups. There was a memory leak here as well which is now eliminated. This also changes the ActionItem::Context vector of strings to a vector of views to keep each matched group. (cherry picked from commit 4f0c4f2) Conflicts: iocore/net/P_SSLSNI.h iocore/net/TLSSNISupport.cc * Fixes leak in SNIAction name globbing (apache#8827) pcre_compile allocated object is never pcre_free-ed (cherry picked from commit efaf441) Conflicts: iocore/net/P_SSLSNI.h Co-authored-by: Brian Olsen <bnolsen@gmail.com> Co-authored-by: Brian Neradt <brian.neradt@gmail.com> Co-authored-by: pbchou <pbchou@labs.att.com> Co-authored-by: Damian Meden <damian.meden@gmail.com>

bgaff and others added 4 commits December 6, 2015 22:18

TS-4042: Add feature to buffer request body before making downstream …

0654349

…requests

TS-4042: adding example c++ plugin

e5fc86e

TS-4042: Fix typo

ba89031

TS-4042: Fixing support for chunked encoding and adding tests

295ef61

zwoop assigned bryancall Jun 27, 2016

zwoop added Plugins CPP API labels Jun 27, 2016

zwoop added this to the 7.0.0 milestone Jun 27, 2016

bryancall reviewed Aug 16, 2016
View reviewed changes

zwoop modified the milestones: 7.1.0, 7.0.0 Sep 15, 2016

zwoop modified the milestones: 7.1.0, 7.2.0 Jan 8, 2017

zwoop modified the milestones: 7.2.0, 7.1.0 Jan 8, 2017

SolidWallOfCode pushed a commit to SolidWallOfCode/trafficserver that referenced this pull request Feb 1, 2017

Merge pull request apache#351 from persia/YTSATS-1085

d5d6acb

YTSATS-1085: url-encoding the Location header

zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017

bryancall closed this May 15, 2017

zwoop removed this from the 8.0.0 milestone May 25, 2017

bneradt pushed a commit to bneradt/trafficserver that referenced this pull request Nov 19, 2020

Merge pull request apache#351 from bneradt/edge_log_throttling

fe94cd6

Implement log throttling

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TS-4042: Add feature to buffer request body before making downstream requests #351

TS-4042: Add feature to buffer request body before making downstream requests #351

bgaff commented Nov 25, 2015

jacksontj commented Nov 25, 2015

jpeach commented Nov 26, 2015

bgaff commented Nov 29, 2015

sudheerv commented Nov 30, 2015

bgaff commented Dec 7, 2015

bgaff commented Dec 7, 2015

bgaff commented Dec 14, 2015

sudheerv commented Dec 14, 2015

bgaff commented Dec 14, 2015

sudheerv commented Dec 14, 2015

bgaff commented Jan 5, 2016

bryancall commented Feb 23, 2016

zwoop commented Mar 22, 2016

zwoop commented Jun 27, 2016

bryancall Aug 16, 2016

zizhong Jan 24, 2017

bryancall commented Aug 16, 2016

jacksontj commented Aug 16, 2016

zizhong commented Jan 24, 2017

bryancall commented Jan 24, 2017

bgaff commented Jan 24, 2017

zizhong commented Jan 24, 2017

zizhong commented Mar 6, 2017

TS-4042: Add feature to buffer request body before making downstream requests #351

TS-4042: Add feature to buffer request body before making downstream requests #351

Conversation

bgaff commented Nov 25, 2015

jacksontj commented Nov 25, 2015

jpeach commented Nov 26, 2015

bgaff commented Nov 29, 2015

sudheerv commented Nov 30, 2015

bgaff commented Dec 7, 2015

bgaff commented Dec 7, 2015

bgaff commented Dec 14, 2015

sudheerv commented Dec 14, 2015

bgaff commented Dec 14, 2015

sudheerv commented Dec 14, 2015

bgaff commented Jan 5, 2016

bryancall commented Feb 23, 2016

zwoop commented Mar 22, 2016

zwoop commented Jun 27, 2016

bryancall Aug 16, 2016

Choose a reason for hiding this comment

zizhong Jan 24, 2017

Choose a reason for hiding this comment

bryancall commented Aug 16, 2016

jacksontj commented Aug 16, 2016

zizhong commented Jan 24, 2017

bryancall commented Jan 24, 2017

bgaff commented Jan 24, 2017

zizhong commented Jan 24, 2017

zizhong commented Mar 6, 2017