http: keep `trailers` TE header instead of removing it #32255

quantumsheep · 2024-02-07T16:51:55Z

Official grpc library does not allow empty TE header. This causes an issue when proxying requests through Envoy which currently deletes the TE header since #30535.

I opened a related MR on the grpc library to allow this header to be empty as the HTTP/2 RFC allows it but it might cause issues with other libraries.

Risk Level: medium (behavioral change)
Testing: modified an existing test
Docs Changes: n/a
Release Notes: inline
Platform Specific Features:
Runtime guard: keep the previous one

repokitteh-read-only · 2024-02-07T16:52:01Z

Hi @quantumsheep, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #32255 was opened by quantumsheep.

see: more, trace.

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

alyssawilk · 2024-02-07T19:09:42Z

source/common/http/conn_manager_utility.cc

@@ -93,7 +93,7 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m
    request_headers.removeConnection();
    request_headers.removeUpgrade();
    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) {
-      request_headers.removeTE();
+      request_headers.setTE(Http::Headers::get().TEValues.Trailers);


I don't think this is correct either. TE may not have been "trailers" to begin with. Maybe check to see if there's a trailers token and if so replace any non'trailers string with trailers?

HTTP/2 and 3 only supports trailers TE headers in theory. I added a condition to do this only if the outgoing request is in gRPC, wdyt?

alyssawilk · 2024-02-07T19:11:57Z

Also beyond that, I don't think preserving trailers across hops is the correct thing by the spec. I think if you want trailers on the upstream hop you should configure Envoy to send them upstream.
Either way sorry the behavioral change was problematic for you all - I'm happy to work with you on a longer term solution
/wait

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

alyssawilk · 2024-02-08T14:27:47Z

source/common/http/conn_manager_utility.cc

    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_te")) {
-      request_headers.removeTE();
+      if (Grpc::Common::isGrpcRequestHeaders(request_headers)) {


I think TE is a hop by hop header, and if we want to send it upstream we should configure it in request_headers_to_add. For example if someone has a web gRPC filter and is downgrading from HTTP/2 gRPC to HTTP/1.1 they wouldn't necessarily support trailers on the upstream to envoy hop.
cc @RyanTheOptimist for thoughts
/wait

Yeah, I agree with @alyssawilk. It seems like request_headers_to_add would be a more standards complaint solution.

After catching up on #30535, I think I've changed my mind. I think that Envoy should send "TE: trailers" to the upstream when the downstream client sent it to Envoy. So instead of adding adding "trailers" when it's a gRPC request, I wonder if we should change the TE sanitization logic to preserve trailers, if present?

@alyssawilk we could remove the TE header when making an HTTP/1 request and keep it as trailers when doing an HTTP/2 or 3 request.

@RyanTheOptimist I like this idea, I updated the MR

It sounds like we're converging, which is great. it looks like the current patch set preserves trailers, if it was present. Since the TE header can have multiple values, do we need to check that trailers is one of the values in the list as opposed to just checking the string value of the field?

You're right! It's updated.

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

source/common/http/conn_manager_utility.cc

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

source/common/http/conn_manager_utility.cc

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

alyssawilk · 2024-02-12T15:50:51Z

I think it's fine given the documentation of the runtime guard change.

) Official grpc library does not allow empty TE header. This causes an issue when proxying requests through Envoy which currently deletes the TE header since envoyproxy#30535. I opened a related MR on the grpc library to allow this header to be empty as the HTTP/2 RFC allows it but it might cause issues with other libraries. Risk Level: medium (behavioral change) Testing: modified an existing test Docs Changes: n/a Release Notes: inline Platform Specific Features: Runtime guard: keep the previous one Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

rajatvig · 2024-03-08T00:44:18Z

Could this be backported to the 1.29 release please? Dropping the TE header caused a lot of grief on our as GRPC Services relying on it being always present stopped working.

oblazek · 2024-03-08T07:34:27Z

Could this be backported to the 1.29 release please? Dropping the TE header caused a lot of grief on our as GRPC Services relying on it being always present stopped working.

I heavily agree with that.. this has caused a lot of troubles for us as well and had to cherry-pick this into 1.29 (or rollback to 1.28.1)

RyanTheOptimist · 2024-03-08T16:30:03Z

That's certainly an option. But in the meantime you can just set envoy.reloadable_features.sanitize_te to false in order to get the previous behavior.

rajatvig · 2024-03-08T17:55:26Z

That's certainly an option. But in the meantime you can just set envoy.reloadable_features.sanitize_te to false in order to get the previous behavior.

Yes, that would work but we use Envoy with the xDS implementation of Contour in Kubernetes and that limits our ability to set arbitrary runtime features without putting in workarounds in the bootstrapping process.

) Official grpc library does not allow empty TE header. This causes an issue when proxying requests through Envoy which currently deletes the TE header since envoyproxy#30535. I opened a related MR on the grpc library to allow this header to be empty as the HTTP/2 RFC allows it but it might cause issues with other libraries. Risk Level: medium (behavioral change) Testing: modified an existing test Docs Changes: n/a Release Notes: inline Platform Specific Features: Runtime guard: keep the previous one Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

Removal of the header was added by default in Envoy v1.29.0. This change reverts back to prior behavior. This change can be reverted once envoyproxy/envoy#32255 is backported or present in a new release of Envoy. Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

) Official grpc library does not allow empty TE header. This causes an issue when proxying requests through Envoy which currently deletes the TE header since envoyproxy#30535. I opened a related MR on the grpc library to allow this header to be empty as the HTTP/2 RFC allows it but it might cause issues with other libraries. Risk Level: medium (behavioral change) Testing: modified an existing test Docs Changes: n/a Release Notes: inline Platform Specific Features: Runtime guard: keep the previous one Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com> Signed-off-by: Ryan Northey <ryan@synca.io>

) (#33018) Official grpc library does not allow empty TE header. This causes an issue when proxying requests through Envoy which currently deletes the TE header since #30535. I opened a related MR on the grpc library to allow this header to be empty as the HTTP/2 RFC allows it but it might cause issues with other libraries. Risk Level: medium (behavioral change) Testing: modified an existing test Docs Changes: n/a Release Notes: inline Platform Specific Features: Runtime guard: keep the previous one Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com> Signed-off-by: Ryan Northey <ryan@synca.io> Co-authored-by: Nathanael Demacon <nathanael.dmc@outlook.fr>

phlax · 2024-04-04T09:22:35Z

@quantumsheep @sunjayBhatia @jbohanon

this has been backported to the 1.29 branch and will imminently get a release

im not clear if this should be backported further but will remove the backport/review tag for now - lmk if it does need further backporting

quantumsheep · 2024-04-04T12:05:15Z

@phlax the issue was only present in 1.29, it's perfect!

Do you know when the release will arrive ? (today or in the coming days?)

phlax · 2024-04-04T12:06:07Z

optimistically today, realistically possibly tomorrow

http: set TE header to trailers instead of removing it

2611451

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

quantumsheep force-pushed the set-te-to-trailers branch from 4cc114a to 2611451 Compare February 7, 2024 16:56

alyssawilk reviewed Feb 7, 2024

View reviewed changes

alyssawilk self-assigned this Feb 7, 2024

repokitteh-read-only bot added waiting and removed waiting labels Feb 7, 2024

http: set TE header to trailers instead of removing it only in grpc

cfe8dd4

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

quantumsheep force-pushed the set-te-to-trailers branch from 2ff7fb0 to cfe8dd4 Compare February 8, 2024 11:23

alyssawilk reviewed Feb 8, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 8, 2024

allow "trailers"

afc5512

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

quantumsheep force-pushed the set-te-to-trailers branch from 13c24b2 to afc5512 Compare February 9, 2024 10:56

repokitteh-read-only bot removed the waiting label Feb 9, 2024

quantumsheep added 2 commits February 9, 2024 14:45

fix TE header get

6e96ea4

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

handle multiple TE values

5afbede

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

RyanTheOptimist reviewed Feb 9, 2024

View reviewed changes

quantumsheep added 9 commits February 9, 2024 18:48

add typing

7f34b49

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

move logic inside a dedicated function

c19ebc3

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

add unit tests

fb94a2e

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

simplify search

ae3dcbe

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

remove trailers weight from tests

1bc9bb6

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

update changelog

a62aa02

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

default reloadable value is true

0091898

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

add test for sanitize_te reloadable feature

e0c7156

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

fix reloadable feature guard

a330a6b

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

RyanTheOptimist reviewed Feb 9, 2024

View reviewed changes

source/common/http/conn_manager_utility.cc Outdated Show resolved Hide resolved

source/common/http/conn_manager_utility.cc Outdated Show resolved Hide resolved

quantumsheep added 2 commits February 9, 2024 22:10

simplify and add comments

da01b3a

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

naming

8eebaef

Signed-off-by: Nathanael DEMACON <ndemacon@scaleway.com>

RyanTheOptimist approved these changes Feb 12, 2024

View reviewed changes

RyanTheOptimist merged commit 44fc421 into envoyproxy:main Feb 12, 2024
54 checks passed

alyssawilk added the backport/review Request to backport to stable releases label Feb 26, 2024

fvdnabee mentioned this pull request Mar 8, 2024

Missing :te header when using envoy proxy with grpc-web filter triton-inference-server/server#6936

Closed

sunjayBhatia mentioned this pull request Mar 18, 2024

Disable Envoy removing TE request header projectcontour/contour#6288

Merged

sunjayBhatia mentioned this pull request Mar 18, 2024

release-1.28: Disable Envoy removing TE request header projectcontour/contour#6290

Merged

quantumsheep mentioned this pull request Mar 21, 2024

[bp/1.29] http: keep trailers TE header instead of removing it (#32255) #33018

Merged

frauniki mentioned this pull request Mar 26, 2024

Istio transforming grpc requests in a way not supported by client? istio/istio#49685

Closed

2 tasks

jbohanon mentioned this pull request Apr 2, 2024

hardcode envoy's runtime flag for sanitize TE to false k8sgateway/k8sgateway#9307

Merged

4 tasks

phlax removed the backport/review Request to backport to stable releases label Apr 4, 2024

This was referenced Jun 3, 2024

Upstream H2 connection reset when sending invalid TE header value #30362

Closed

Upstream gRPC connection reset when missing TE header value #34475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: keep `trailers` TE header instead of removing it #32255

http: keep `trailers` TE header instead of removing it #32255

quantumsheep commented Feb 7, 2024 •

edited

Loading

repokitteh-read-only bot commented Feb 7, 2024

alyssawilk Feb 7, 2024

quantumsheep Feb 8, 2024 •

edited

Loading

alyssawilk commented Feb 7, 2024

alyssawilk Feb 8, 2024

RyanTheOptimist Feb 8, 2024

RyanTheOptimist Feb 8, 2024

quantumsheep Feb 9, 2024 •

edited

Loading

RyanTheOptimist Feb 9, 2024

quantumsheep Feb 9, 2024

alyssawilk commented Feb 12, 2024

rajatvig commented Mar 8, 2024

oblazek commented Mar 8, 2024

RyanTheOptimist commented Mar 8, 2024

rajatvig commented Mar 8, 2024

phlax commented Apr 4, 2024

quantumsheep commented Apr 4, 2024

phlax commented Apr 4, 2024

http: keep trailers TE header instead of removing it #32255

http: keep trailers TE header instead of removing it #32255

Conversation

quantumsheep commented Feb 7, 2024 • edited Loading

repokitteh-read-only bot commented Feb 7, 2024

alyssawilk Feb 7, 2024

Choose a reason for hiding this comment

quantumsheep Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

alyssawilk commented Feb 7, 2024

alyssawilk Feb 8, 2024

Choose a reason for hiding this comment

RyanTheOptimist Feb 8, 2024

Choose a reason for hiding this comment

RyanTheOptimist Feb 8, 2024

Choose a reason for hiding this comment

quantumsheep Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

RyanTheOptimist Feb 9, 2024

Choose a reason for hiding this comment

quantumsheep Feb 9, 2024

Choose a reason for hiding this comment

alyssawilk commented Feb 12, 2024

rajatvig commented Mar 8, 2024

oblazek commented Mar 8, 2024

RyanTheOptimist commented Mar 8, 2024

rajatvig commented Mar 8, 2024

phlax commented Apr 4, 2024

quantumsheep commented Apr 4, 2024

phlax commented Apr 4, 2024

http: keep `trailers` TE header instead of removing it #32255

http: keep `trailers` TE header instead of removing it #32255

quantumsheep commented Feb 7, 2024 •

edited

Loading

quantumsheep Feb 8, 2024 •

edited

Loading

quantumsheep Feb 9, 2024 •

edited

Loading