http: clear hop by hop TE header #30535

alyssawilk · 2023-10-26T15:42:53Z

Changes Envoy to clear TE header by default, as it is a hop by hop header and should not be forwarded on.

Risk Level: medium (behavioral change)
Testing: new e2e test
Docs Changes: n/a
Release Notes: inline.
[Optional Runtime guard:] yes.
Fixes #30362

repokitteh-read-only · 2023-10-26T15:42:58Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #30535 was opened by alyssawilk.

see: more, trace.

repokitteh-read-only · 2023-10-26T15:43:00Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #30535 was opened by alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist

Looks great!

RyanTheOptimist · 2023-11-29T21:29:51Z

test/integration/header_integration_test.cc

@@ -1375,8 +1377,9 @@ TEST_P(HeaderIntegrationTest, TestTeHeaderPassthrough) {
      });
 }

-// Validates TE header is stripped if it contains an unsupported value
+// Validates TE header was stripped if it contains an unsupported value


nit: Your call, but I'd be inclined to use the present tense here as it's describing what the test is going to do, not what it has done. But totally your call.

RyanTheOptimist · 2023-11-29T21:30:24Z

test/integration/header_integration_test.cc

@@ -1341,8 +1341,10 @@ TEST_P(HeaderIntegrationTest, PathWithEscapedSlashesRedirected) {
                                   });
 }

-// Validates TE header is forwarded if it contains a supported value
+// Validates TE header was forwarded if it contains a supported value
+// It is now treated as the hop by hop header it is.


Should we remove this comment? I'm not sure that I understand why it's here?

RyanTheOptimist · 2023-11-29T21:30:58Z

changelogs/current.yaml

+- area: http
+  change: |
+    Sanitize the hop by hop TE header from downstream request headers. This change can be temporarily reverted
+    by setting ``envoy.reloadable_features.sanitize_te`` to false.


Does this flag sanitize the TE header, or does it remove it?

RyanTheOptimist · 2023-11-29T21:32:24Z

test/integration/protocol_integration_test.cc

+    default_request_headers_.setTE("gzip");
+  } else {
+    return;
+  }


nit: Early return:

if (downstreamProtocol() != Http::CodecType::HTTP1) { return; } default_request_headers_.setTE("gzip");

Optionally put this early return at the top of the test to make it clear that it's H1-specific.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

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

Since Envoy 1.29 I get Missing :te header when proxying to a Python-gRPC API because of this.

Removing the TE isn't the solution. We should replace it by trailers.

alyssawilk · 2024-02-07T19:10:54Z

So thankfully this is runtime guarded so you should be able to reinstate prior behavior with a config change.

I think if you want the envoy-upstream hop to support trailers you should just add request headers "te: trailers" to the outgoing request.

ejona86 · 2024-02-08T15:22:55Z

I think if you want the envoy-upstream hop to support trailers you should just add request headers "te: trailers" to the outgoing request.

If you are meaning unconditionally, that seems a bit simplistic. That ignores whether the originating client supports trailers. From RFC 7230 §4.1.2:

The presence of the keyword "trailers" indicates that the client is
willing to accept trailer fields in a chunked transfer coding, as
defined in Section 4.1.2, on behalf of itself and any downstream
clients.

"any downstream clients" being the important part. So you'd want to make it conditional based on the originating request.

RyanTheOptimist · 2024-02-08T22:14:40Z

I think if you want the envoy-upstream hop to support trailers you should just add request headers "te: trailers" to the outgoing request.

If you are meaning unconditionally, that seems a bit simplistic. That ignores whether the originating client supports trailers. From RFC 7230 §4.1.2:

The presence of the keyword "trailers" indicates that the client is
willing to accept trailer fields in a chunked transfer coding, as
defined in Section 4.1.2, on behalf of itself and any downstream
clients.

"any downstream clients" being the important part. So you'd want to make it conditional based on the originating request.

Oh, interesting. Yes, I think that's a good point. Envoy itself is able to process trailers and pass them along, but it's only "safe" to do that in the case where Envoy knows that the downstream client is able to process trailers. The client, of course, indicates this by sending "trailers" in te.

Does that sound right?

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>

) 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>

) 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>

alyssawilk force-pushed the te branch 2 times, most recently from 4ff13c1 to 5958a8e Compare November 2, 2023 12:46

alyssawilk force-pushed the te branch from 5958a8e to 2113d1c Compare November 29, 2023 16:49

http: clear hop by hop TE header

a063c67

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the te branch from 2113d1c to a063c67 Compare November 29, 2023 17:42

alyssawilk marked this pull request as ready for review November 29, 2023 19:37

alyssawilk assigned mattklein123 and RyanTheOptimist Nov 29, 2023

RyanTheOptimist reviewed Nov 29, 2023

View reviewed changes

mattklein123 previously approved these changes Nov 30, 2023

View reviewed changes

comments

0d6d08b

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed mattklein123’s stale review via 0d6d08b November 30, 2023 13:44

RyanTheOptimist approved these changes Nov 30, 2023

View reviewed changes

RyanTheOptimist merged commit d4ed8d4 into envoyproxy:main Nov 30, 2023
105 checks passed

quantumsheep mentioned this pull request Feb 7, 2024

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

Merged

quantumsheep mentioned this pull request Feb 8, 2024

[http] allow empty te header grpc/grpc#35841

Closed

alyssawilk deleted the te branch March 19, 2024 19:46

phlax mentioned this pull request Mar 20, 2024

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

Merged

jbohanon mentioned this pull request Apr 2, 2024

hardcode envoy's runtime flag for sanitize TE to false solo-io/gloo#9307

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: clear hop by hop TE header #30535

http: clear hop by hop TE header #30535

alyssawilk commented Oct 26, 2023 •

edited

repokitteh-read-only bot commented Oct 26, 2023

repokitteh-read-only bot commented Oct 26, 2023

RyanTheOptimist left a comment

RyanTheOptimist Nov 29, 2023

alyssawilk Nov 30, 2023

RyanTheOptimist Nov 29, 2023

alyssawilk Nov 30, 2023

RyanTheOptimist Nov 29, 2023

RyanTheOptimist Nov 29, 2023

quantumsheep commented Feb 7, 2024

alyssawilk commented Feb 7, 2024

ejona86 commented Feb 8, 2024

RyanTheOptimist commented Feb 8, 2024

http: clear hop by hop TE header #30535

http: clear hop by hop TE header #30535

Conversation

alyssawilk commented Oct 26, 2023 • edited

repokitteh-read-only bot commented Oct 26, 2023

repokitteh-read-only bot commented Oct 26, 2023

RyanTheOptimist left a comment

Choose a reason for hiding this comment

RyanTheOptimist Nov 29, 2023

Choose a reason for hiding this comment

alyssawilk Nov 30, 2023

Choose a reason for hiding this comment

RyanTheOptimist Nov 29, 2023

Choose a reason for hiding this comment

alyssawilk Nov 30, 2023

Choose a reason for hiding this comment

RyanTheOptimist Nov 29, 2023

Choose a reason for hiding this comment

RyanTheOptimist Nov 29, 2023

Choose a reason for hiding this comment

quantumsheep commented Feb 7, 2024

alyssawilk commented Feb 7, 2024

ejona86 commented Feb 8, 2024

RyanTheOptimist commented Feb 8, 2024

alyssawilk commented Oct 26, 2023 •

edited