tcp tunneling: optionally move response header to downstream info by kyessenov · Pull Request #23118 · envoyproxy/envoy

kyessenov · 2022-09-15T04:34:02Z

Signed-off-by: Kuat Yessenov kuat@google.com

Commit Message: Add support to save response headers from CONNECT tunnels in tcp_proxy. Fixes #23116
Additional Description: The use case is saving "baggage" header which provides additional metadata about the upstream endpoint for telemetry purposes.
Risk Level: low (opt-in)
Testing: unit, integration
Docs Changes: none
Release Notes: yes

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only · 2022-09-15T04:34:13Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #23118 was opened by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>

alyssawilk

Thanks for picking this up!
Now that I think about it this could be useful for the http11 connect filter: @Augustyniak was just writing an EM integraion test which added to CONNECT response headers and there was no way to validate them elsewhere. If we end up sticking with per-header-name configuration I think it may be worth defining the message somewhere in the base codec protos rather than in the tcp proxy proto for eventual reuse.
/wait

alyssawilk · 2022-09-15T17:37:17Z

source/common/tcp_proxy/upstream.h

    // Http::ResponseDecoder
    void decode1xxHeaders(Http::ResponseHeaderMapPtr&&) override {}
    void decodeHeaders(Http::ResponseHeaderMapPtr&& headers, bool end_stream) override {
+      parent_.config_.copyResponseHeaders(*headers, parent_.downstream_info_.filterState());


Once we finish this function, don't we largely throw the headers away?
Would it make sense just to move the entire headers in the filter, with a well known name, rather than coping one bit of metadata at a time?

Yeah, I think we could do that with little performance impact. One annoying issue is that we don't have a generic way to traverse the filter state that is a map of string to string. That latter issue has been a long standing blocker for replacing dynamic metadata with filter state so maybe we should just fix that as well.

do we need a map of string to string? we could just create a filter state object "connect_headers" which we std::move the headers into?

Yes, that's what I want to do. But then, for example, access log printers can only print it as a whole. It's not a problem for internal filter consumption, just generic inspectors like access log, etc.

alyssawilk · 2022-09-15T17:38:14Z

api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto

+    // Defines the mapping from the header value to a filter state object key.
+    message HeaderToFilterState {
+      // Filter state object key, prefixed with ``envoy.tcp_proxy.tunnel_response_header.``.
+      string key = 1 [(validate.rules).string = {min_len: 1}];


If we do with putting in individual headers, is there value in being able to configure the key rather than just sticking header name at the end of the string?

I'll try a complete bag copy, and in that case we can use one key for all of them.

alyssawilk · 2022-09-15T17:38:48Z

api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto

        "envoy.config.filter.network.tcp_proxy.v2.TcpProxy.TunnelingConfig";

+    // Defines the mapping from the header value to a filter state object key.
+    message HeaderToFilterState {


probably want a release note for this one once we've settled on API

Signed-off-by: Kuat Yessenov <kuat@google.com>

alyssawilk

/wait

alyssawilk · 2022-09-19T13:10:59Z

api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto

+
+    // Emit response headers to the downstream info filter state for consumption
+    // by the network filters. The filter state key is ``envoy.tcp_proxy.tunnel_response_headers``.
+    bool emit_response_headers = 4;


I'd lean towards something akin to
save_connect_headers but I'll defer to api shephards on this one :-)

@markdroth For API review and deciding on naming.

How about calling this propagate_response_headers?

source/common/router/router.h

Signed-off-by: Kuat Yessenov <kuat@google.com>

alyssawilk

LGTM (though I still don't love emit - @markdroth ?)
Passing to Greg for !google pass

Signed-off-by: Kuat Yessenov <kuat@google.com>

markdroth · 2022-09-21T18:55:15Z

/lgtm api

alyssawilk

Yeah I like this better - thanks for your patience with naming
CI looks unhappy?
/wait

alyssawilk · 2022-09-21T20:59:55Z

api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto

        [(validate.rules).repeated = {max_items: 1000}];
+
+    // Save the response headers to the downstream info filter state for consumption
+    // by the network filters. The filter state key is ``envoy.tcp_proxy.tunnel_response_headers``.


maybe make the key propagate as well?

alyssawilk · 2022-09-21T21:00:11Z

changelogs/current.yaml

    added support for quit command to the redis proxy.
+- area: tcp_proxy
+  change: |
+    added support for emitting the response headers in :ref:`TunnelingConfig


emitting -> propagating ?

Signed-off-by: Kuat Yessenov <kuat@google.com>

markdroth · 2022-09-21T21:25:02Z

/lgtm api

alyssawilk

over to @ggreenway !

ggreenway

Please add a test validating that headers are not propagated when the new setting is disabled.

/wait

ggreenway · 2022-09-22T20:07:52Z

api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto

+
+    // Save the response headers to the downstream info filter state for consumption
+    // by the network filters. The filter state key is ``envoy.tcp_proxy.propagate_response_headers``.
+    bool propagate_response_headers = 4;


Why not do this unconditionally? Performance?

Yes, for long-living streams, we're adding extra memory per stream since the filter state lives till connection closes.

Signed-off-by: Kuat Yessenov <kuat@google.com>

tcp tunneling: copy response header to downstream info

39dd246

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested review from alyssawilk, ggreenway, lizan, snowp, wbpcode and zuercher as code owners September 15, 2022 04:34

repokitteh-read-only bot added the api label Sep 15, 2022

repokitteh-read-only bot assigned markdroth Sep 15, 2022

kyessenov added 2 commits September 15, 2022 09:03

CI

13ea62d

Signed-off-by: Kuat Yessenov <kuat@google.com>

mock

0d78ac4

Signed-off-by: Kuat Yessenov <kuat@google.com>

alyssawilk reviewed Sep 15, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 15, 2022

kyessenov added 2 commits September 15, 2022 10:45

Merge remote-tracking branch 'upstream/main' into copy_response_headers

50ac8a6

review

38b492a

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting label Sep 15, 2022

kyessenov added 3 commits September 15, 2022 12:31

stale

de40f8d

Signed-off-by: Kuat Yessenov <kuat@google.com>

ignore invalid response

9ed361b

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix

a5b05f3

Signed-off-by: Kuat Yessenov <kuat@google.com>

adisuissa assigned alyssawilk Sep 19, 2022

alyssawilk reviewed Sep 19, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 19, 2022

kyessenov added 2 commits September 19, 2022 09:38

Merge remote-tracking branch 'upstream/main' into copy_response_headers

ba86a92

review

f109cfd

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting label Sep 19, 2022

kyessenov changed the title ~~tcp tunneling: copy response header to downstream info~~ tcp tunneling: optionally move response header to downstream info Sep 19, 2022

alyssawilk previously approved these changes Sep 20, 2022

View reviewed changes

alyssawilk assigned ggreenway Sep 20, 2022

Merge remote-tracking branch 'upstream/main' into copy_response_headers

0cfe94c

emit->propagate

2cb76ea

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed alyssawilk’s stale review via 2cb76ea September 21, 2022 18:51

repokitteh-read-only bot removed the api label Sep 21, 2022

alyssawilk reviewed Sep 21, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 21, 2022

review

b557893

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot added api and removed waiting labels Sep 21, 2022

repokitteh-read-only bot removed the api label Sep 21, 2022

alyssawilk previously approved these changes Sep 22, 2022

View reviewed changes

ggreenway requested changes Sep 22, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 22, 2022

add negative test

826cb13

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed alyssawilk’s stale review via 826cb13 September 22, 2022 20:26

repokitteh-read-only bot removed the waiting label Sep 22, 2022

ggreenway approved these changes Sep 22, 2022

View reviewed changes

ggreenway merged commit caa81fa into envoyproxy:main Sep 26, 2022

IssaAbuKalbein mentioned this pull request Nov 8, 2023

UDP Tunneling: Optionally propagate response headers and trailers to downstream info #30597

Merged

IssaAbuKalbein mentioned this pull request Feb 29, 2024

Common TunnelResponseHeadersOrTrailers for UDP & TCP tunneling #32619

Merged

Conversation

kyessenov commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 15, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth commented Sep 21, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markdroth commented Sep 21, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kyessenov commented Sep 15, 2022 •

edited

Loading