varnish-cache icon indicating copy to clipboard operation
varnish-cache copied to clipboard

http: Skip req and beresp trailers

Open dridi opened this issue 1 year ago • 8 comments

This is the first step of the following plan:

  • process but drop incoming trailers to preserve HTTP framing
  • forward verbatim trailers for pass transactions
    • guarded by an experimental::pass_trailers flag
  • manipulate trailers in VCL

The first step, this pull request, should allow applications producing trailers to work with Varnish when the trailers are not strictly required.

The second step should allow protocols like gRPC to pass through Varnish, with the caveats that body filters could break the meaning of trailers (for example checksums), hence the experimental flag.

It will require internal changes, and from the look of it we may not be able to directly use the existing data structures and we may need to a dedicated OA_TRAILERS object attribute.

The last step will likely require changes to the VCL state machines in addition to new symbols.

Most of the work on trailers was done by @walid-git under my supervision.

This pull request includes work from other pull requests too:

  • 4 commits of mine taken from #3798, turning BS_CACHED into a request flag
  • 4 commits from @nigoroll taken from #3809 to better delimit HTTP/1 chunks
    • I disregarded @mbgrydeland's concern from #3811 that can be implemented later
  • 2 commits of mine generalizing pipelining in a workspace
  • the rest is trailer support from @walid-git
    • new BS_TRAILERS body status for coordination
    • new gettrls() callback for directors (missing docs update)
    • process incoming trailers in HTTP/1 code
    • move trailers outside of the HTTP/1 VFP code, they are not part of the body (as suggested in https://github.com/varnishcache/varnish-cache/pull/3809#issuecomment-1181577982)
    • process beresp trailers
    • process req trailers
    • process incoming trailers in h2 code

This is a lot of commits, but they should all have a reasonable size for reviewers. I already reviewed Walid's work, and already addressed my own review items while Walid is away. I added Walid or myself as a co-author when I made significant changes (at the scale of individual commits).

dridi avatar Jun 18 '24 10:06 dridi

All failing platforms appear to overflow the workspace used in the h2 test case, except the sanitizers job that fails on something else.

I will have a closer look later.

dridi avatar Jun 18 '24 12:06 dridi

As it turned out, the failing tests were exposing a race revolving around the request body status. Transitioning to BS_TAKEN would compete against the new transition to BS_TRAILERS (happening in different threads in the h2 case).

I took care of that and added a couple patches next to the ones picked from #3798 at the beginning of this patch series. I'm no longer able to stress test cases and cause failures and I believe the race is gone.

I have not yet studied the remaining CI failure, triggering a panic from the workspace emulator. I suspect either a logic error in the workspace pipelining (expanding from req keep-alive only to req and beresp trailers) or an error in how it translates in the workspace emulator.

I also started an effort to improve feature parity between HTTP/1 and h2 txreq in varnishtest but this is not ready.

dridi avatar Jun 24 '24 18:06 dridi

As I suspected, the emulated WS_Pipeline() was incorrect. There was a missing workspace release too.

dridi avatar Jun 25 '24 07:06 dridi

bugwash:

The following commits can be merged: https://github.com/varnishcache/varnish-cache/compare/23ec1fc3fb1198486350e1aedceb5b363d6e429c...98752585d7e086a685816a2934860c72ac0a92c6

The workspace refactoring commits can be submitted independently.

dridi avatar Jul 02 '24 12:07 dridi

I'm fine with you commiting those first six commits while we wait for @nigoroll

bsdphk avatar Jul 02 '24 12:07 bsdphk

As per bugwash:

  • I pushed ce3e446f4e...f0e9df843d to trunk
  • I submitted workspace changes independently (#4130)
    • they are still present at the beginning of this patch series
  • I squashed outstanding commits

dridi avatar Jul 03 '24 13:07 dridi

Force-pushed to rebase against trunk after merging #4130.

dridi avatar Jul 08 '24 17:07 dridi

~~I am slightly worried about the concern from https://github.com/varnishcache/varnish-cache/pull/3811#pullrequestreview-1074005690 being unresolved and pulled into VCP with this PR.~~ This was posted on the wrong PR. Please disregard.

mbgrydeland avatar Mar 06 '25 09:03 mbgrydeland