-
Notifications
You must be signed in to change notification settings - Fork 5.5k
http1 encode trailers in chunk encoding #8667
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.
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
Changes from 45 commits
08ae97c
5de8533
9c550cb
f0d53cd
f9c9f48
e002b4b
10a9089
de29acb
96913f9
77ec2ac
8b03163
6955a1a
92421a8
e7cb695
592e315
9e2a48a
cd56fd8
e9c27e4
c70e7b0
557e027
614ff9b
4205198
b63375d
92d4efc
dae9233
07a0e79
e124271
962afd2
fce0ddb
9f990a5
de672b8
b41c492
ee426ea
1296eff
ce552b8
1b87726
fd5c7de
4ed8b97
2f18c1c
dfbd03e
76a0a6d
3041acb
e59101d
c784ce4
f28af8f
86fed8a
cee19aa
32e0c7a
4388cf4
8b13bdf
4aadf64
14ac61b
41135aa
678222d
dd6844a
b90f535
fbdf83d
9e8be74
c179840
915fb06
ce5cb82
b406f71
5447d69
be14d7e
0c717f8
a347b6f
fb321d8
d1c66e8
15b6715
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -229,6 +229,12 @@ struct Http1Settings { | |
| bool accept_http_10_{false}; | ||
| // Set a default host if no Host: header is present for HTTP/1.0 requests.` | ||
| std::string default_host_for_http_10_; | ||
| // Encode trailers in Http. By default the HTTP/1 codec drops proxied trailers. | ||
| // Note that this only happens when Envoy is chunk encoding which occurs when: | ||
| // The request is Http1.1 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding this! Can you either link this in the API comments or reiterate the limitations there, as more people read API docs than code docs. I'd indent these 3 // the request is HTTP/1.1 I think the content length one is a biggie. We could optionally even call out that for folks who want consistent trailers it may be necessary to have a filter remove content length in encode/decodeHeaders
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Isn't trailers generally used only when chunk encoding though? If you already know the content length, wouldn't you just stick the information into the header? The only edge case I can imagine is that all your responses are a fixed content-length and you still need to pass in trailers based on some arbitrary data.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many servers will send content length with known length response bodies. Let's say you have a set-up like the following: We could default strip content-length if enable-trailers is true, and there's a trailer header, but HTTP/2 doesn't require a trailer header. We could always strip content length. Or we could have a warning here. I don't feel very strongly about the options, but leaving the behavior confusing where sometimes trailers are sent and sometimes not (based on the presence of the content length header) without documentation seems like something we should fix. Does the situation I describe above make sense to you? @mattklein123 in case he has a preference
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great call out. I would probably just go with a big warning for now (in the rendered docs) about the feature limitations. Then if someone complains we can revisit (See other docs for use of the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough, I'll update the docs. 😄 |
||
| // Is neither a HEAD only request or a HTTP Upgrade | ||
| // Not a HEAD request | ||
| bool enable_trailers_{false}; | ||
|
|
||
| enum class HeaderKeyFormat { | ||
| // By default no formatting is performed, presenting all headers in lowercase (as Envoy | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.