Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

http1 encode trailers in chunk encoding #8667

Merged
merged 69 commits into from
Dec 17, 2019
Merged

http1 encode trailers in chunk encoding #8667

merged 69 commits into from
Dec 17, 2019

Conversation

Chuongv
Copy link
Contributor

@Chuongv Chuongv commented Oct 18, 2019 •
edited
Loading

Add HTTP1 encoding and decoding of trailers

Encoding/Decoding of HTTP1 trailers is set behind the flag enable_trailers

  • Add unit tests
  • Add integration tests H1/H2 <--> Envoy <--> H2/H1

Changes made:

  • Enable parsing of H1 trailers
  • Enable decoding of H1 trailers
  • Configuration flag for encoding/decoding trailers
  • fake_upstream in integration tests is set up enabled with trailers
  • downstream connection in integration tests is setup enabled with trailers

Signed-off-by: Chuong Vu chuongv@google.com

Description: Pass trailers through from http2 upstream to http1 downstream
Risk Level: Med
Testing: Unit test and integration test with every combination of H1 and H2
Docs Changes: N/A
Release Notes:
Fixes #7149

@mattklein123 mattklein123 self-assigned this Oct 20, 2019
@mattklein123
Copy link
Member

Thanks for adding this. This will need both unit tests for the codec as well as integration tests. Thank you!

/wait

@Chuongv
http1 encode trailers in chunk encoding
08ae97c 
Allows encoding the trailers in chunk encoding

Signed-off-by: Chuong Vu <chuongv@google.com>
@Chuongv Chuongv force-pushed the http1-trailers-chunk-encoding branch from 20e428a to 08ae97c Compare October 21, 2019 23:06
@Chuongv
Copy link
Contributor Author

Chuongv commented Oct 23, 2019

@htuch your thoughts on this would be great!

Also I tested this locally and was able to get the trailers back. However I'm having a hard time simulating what I did in an integration test. So I would love some feedback on how I can properly incorporate the proper test.

@htuch
Copy link
Member

htuch commented Oct 23, 2019

@Chuongv looks like you are on the right track..

@Chuongv
Add integration test for trailers http1<--->Envoy<--->http2
5de8533 
Signed-off-by: Chuong Vu <chuongv@google.com>
@Chuongv
Use tcp client for integration test for trailers
9c550cb 
Signed-off-by: Chuong Vu <chuongv@google.com>
@Chuongv Chuongv changed the title [WIP] http1 encode trailers in chunk encoding http1 encode trailers in chunk encoding Oct 25, 2019
@Chuongv Chuongv changed the title http1 encode trailers in chunk encoding [WIP]http1 encode trailers in chunk encoding Oct 25, 2019
@Chuongv
Update tests for trailers http1
f9c9f48 
Signed-off-by: Chuong Vu <chuongv@google.com>
@Chuongv
Undo changes in codec_impl
e002b4b 
Signed-off-by: Chuong Vu <chuongv@google.com>
@Chuongv
Implementation of trailers support for http1
10a9089 
Signed-off-by: Chuong Vu <chuongv@google.com>
@Chuongv
Trailer code clean up
de29acb 
Cleaning up some of the code that was unnecessary

Signed-off-by: Chuong Vu <chuongv@google.com>
@Chuongv
Remove unnecessary check for trailers
96913f9 
onHeadersComplete is never called after trailers are done

Signed-off-by: Chuong Vu <chuongv@google.com>
@Chuongv
Keep the trailer processing on MessageComplete
77ec2ac 
Signed-off-by: Chuong Vu <chuongv@google.com>
@Chuongv
Remove trailer logic in onHeaderComplete
8b03163 
Signed-off-by: Chuong Vu <chuongv@google.com>
alyssawilk
alyssawilk reviewed Dec 11, 2019
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Two last nits from me and I've pinged Lizan to take a look at the bridge filter.

api/envoy/api/v2/core/protocol.proto Outdated Show resolved Hide resolved
source/common/http/http1/codec_impl.h Outdated Show resolved Hide resolved
snowp
snowp previously requested changes Dec 11, 2019
View reviewed changes
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good! Left a few comments

api/envoy/api/v2/core/protocol.proto Outdated Show resolved Hide resolved
api/envoy/api/v2/core/protocol.proto Outdated Show resolved Hide resolved
source/common/http/http1/codec_impl.cc Show resolved Hide resolved
source/common/http/http1/codec_impl.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc Show resolved Hide resolved
test/common/http/http1/codec_impl_test.cc Outdated Show resolved Hide resolved
@Chuongv
Address PR comments
0c717f8 
Signed-off-by: Chuong Vu <chuongv@google.com>
@mattklein123
Copy link
Member

Needs a master merge. Thank you!

/wait

mattklein123
mattklein123 requested changes Dec 13, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/ a few small nits. Thanks for powering through this. @alyssawilk @snowp any further comments?

/wait

api/envoy/api/v2/core/protocol.proto Outdated Show resolved Hide resolved
api/envoy/api/v2/core/protocol.proto Outdated Show resolved Hide resolved
include/envoy/http/codec.h Outdated Show resolved Hide resolved
source/common/http/http1/codec_impl.cc Outdated Show resolved Hide resolved
alyssawilk
alyssawilk previously approved these changes Dec 16, 2019
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo Matt's latest round of comments.
I was good other than wanting a second pair of eyes on the reverse bridge filter, and Snow did a pass on that.

@Chuongv
Move trailers/headers to constant strings. Use Consistent Http1 naming
d1c66e8 
Signed-off-by: Chuong Vu <chuongv@google.com>
mattklein123
mattklein123 approved these changes Dec 16, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit cbf565f into envoyproxy:master Dec 17, 2019
@Chuongv Chuongv deleted the http1-trailers-chunk-encoding branch December 17, 2019 01:09
spenceral added a commit to spenceral/envoy that referenced this pull request Dec 20, 2019
@spenceral
Merge branch 'master' into filter-config-dump
ade8dbe 
* master: (167 commits)
  stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, using the new MemBlock wrapper for memcpy (envoyproxy#8779)
  Make %UPSTREAM_LOCAL_ADDRESS% access-log format work for HTTP requests. (envoyproxy#9362)
  tools: API boosting support for using decls and enum constants. (envoyproxy#9418)
  Fix incorrect cluster InitializePhase type (envoyproxy#9379)
  build: fix merge race between envoyproxy#9241 and envoyproxy#9413. (envoyproxy#9427)
  fuzz: fix incorrect evaluator test (envoyproxy#9402)
  server: fix bogus startup log message (envoyproxy#9404)
  tools: Add protoxform tests (envoyproxy#9241)
  api: options after import (envoyproxy#9413)
  misc: use std::move instead of constructing a copy (envoyproxy#9415)
  tools: API boosting support for rewriting elaborated types. (envoyproxy#9375)
  docs: fix invalid transport_socket value (envoyproxy#9403)
  fix typo in docs (envoyproxy#9394)
  srds: remove to-de-removed scopes first and then apply additions to avoid scope key conflict. (envoyproxy#9366)
  api: generate whole directory and sync (envoyproxy#9382)
  bazel: Add load statements for proto_library (envoyproxy#9367)
  Fix typo (envoyproxy#9388)
  Correct test of OptionsImpl argc type (Was: Correct type for std::array size() result) (envoyproxy#9290)
  http1 encode trailers in chunk encoding (envoyproxy#8667)
  Add mode to PipeInstance (envoyproxy#8423)
  ...
prakhag1 pushed a commit to prakhag1/envoy that referenced this pull request Jan 3, 2020
@Chuongv @prakhag1
http1 encode trailers in chunk encoding (envoyproxy#8667)
e0a951e 
Signed-off-by: Chuong Vu <chuongv@google.com>
Signed-off-by: Prakhar <prakhar_au@yahoo.com>
@lizan lizan mentioned this pull request Mar 14, 2020
Closed
@yabberyabber yabberyabber mentioned this pull request Dec 16, 2022
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asraa asraa asraa left review comments

@wlhee wlhee wlhee left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

@zuercher zuercher Awaiting requested review from zuercher

@htuch htuch Awaiting requested review from htuch

@lizan lizan Awaiting requested review from lizan

@PiotrSikora PiotrSikora Awaiting requested review from PiotrSikora

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@snowp snowp Awaiting requested review from snowp

Assignees

@mattklein123 mattklein123

@alyssawilk alyssawilk

Labels
no stalebot Disables stalebot from closing an issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTP/1.1, chunked encoding and trailers
9 participants
@Chuongv @mattklein123 @htuch @snowp @alyssawilk @lizan @wlhee @PiotrSikora @asraa