Envoy doesn't clear TE header

[alyssawilk]

Per the RFC https://tools.ietf.org/html/rfc2616#section-13.5.1 this should be hop by hop.

I'll throw together a quick PR to clear it in mutateRequestHeaders - just filing a tracking issue for internal accounting.

[PiotrSikora]

RFC2616 is obsoleted.

From RFC7230, sec. 4.3:

The "TE" header field in a request indicates what transfer codings,
besides chunked, the client is willing to accept in response, and
whether or not the client is willing to accept trailer fields in a
chunked transfer coding.

and:

Since the TE header field only applies to the immediate connection, a
sender of TE MUST also send a "TE" connection option within the
Connection header field (Section 6.1) in order to prevent the TE
field from being forwarded by intermediaries that do not support its
semantics.

In theory, TE header should be removed only if it's present in the Connection header (along with all other headers being listed there), since there is no other rule saying that TE should be removed.

On the other hand, Envoy, when acting as a client, accepts trailers, so it should emit it's own TE: trailers header when making requests, therefore forwarding TE header from clients willing to accept trailers is fine, IMHO.

Furthermore, removing TE: trailers from forwarded requests will result in upstreams not sending trailers in responses... and that might be true even for some HTTP/2 upstreams.

[alyssawilk]

Envoy does HTTP/1.1 trailers? I think unsupported 1.1 trailers was our (original) motivation for stripping TE as it was one of the few actual uses.

[PiotrSikora]

Doesn't it? Sorry, I actually don't know, but I assumed it does, since they are part of chunked encoding, and Envoy as a whole is aware of trailers.

Regardless, as far as I can tell, you change removes TE: trailers not only from HTTP/1.1 requests, but also from HTTP/2.

[alyssawilk]

Envoy sure supports trailers for H2, but if you attempt to send trailers with an H1 codec it doesn't actually encode them and if we get them in we explicitly drop them on the floor** so I think stripping TE: trailers for HTTP/1.1 makes sense.

By the H2 spec the only legal value of TE is "trailers", so we could theoretically pass that through coming from H2 and going to H2 (basically strip incoming and outgoing in the HTTP/1.1 codec).
I'm not convinced the complexity is worth the value add of passing through something I think is non-informational for H2, but I'm willing to do the extra work if you think it's worthwhile

*https://github.com/envoyproxy/envoy/blob/master/source/common/http/http1/codec_impl.cc#L156
**https://github.com/envoyproxy/envoy/blob/master/source/common/http/http1/codec_impl.cc#L390

[PiotrSikora]

My memory is a bit fuzzy at this point, but I'm pretty sure that gRPC requires TE: trailers in requests (or at least, it did at some point in the past).

I don't see this being explicitly called out in the spec, but grpc/PROTOCOL-HTTP2.md says:

TE → "te" "trailers" # Used to detect incompatible proxies

and official clients are definitely sending TE: trailers in requests.

Also, from https://nghttp2.org/blog/2015/03/24/proxying-grpc-with-nghttpx/:

gRPC does not work without “TE” header field.

So we pretty much need to forward TE: trailers in HTTP/2, otherwise we're going to break gRPC.

[alyssawilk]

OK, so the most correct thing to do would be to clear "trailers" from TE for HTTP/1.1 and leave the rest intact? Given prior experiences clearing individual fields from hop by hop headers causing avoidable perf issues I think I'm going to steer away, and either file this as a proxy difference, implement the aggressive in-house clearing, or suggest we upgrade the in-house sanitization to something more modern.

[mmahimtura]

Is my issue related?
#7738