tcp tunneling: optionally move response header to downstream info #23118

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

@@ -166,6 +166,7 @@ class HttpUpstream : public GenericUpstream, protected Http::StreamCallbacks {
    // 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

@@ -68,6 +68,16 @@ message TcpProxy {
    option (udpa.annotations.versioning).previous_message_type =
        "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

@@ -99,6 +99,10 @@ message TcpProxy {
    // Neither ``:-prefixed`` pseudo-headers nor the Host: header can be overridden.
    repeated config.core.v3.HeaderValueOption headers_to_add = 3
        [(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

@@ -223,6 +223,11 @@ new_features:
 - area: redis
  change: |
    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 zuercher, snowp, alyssawilk, ggreenway, lizan and wbpcode 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tcp tunneling: optionally move response header to downstream info #23118

tcp tunneling: optionally move response header to downstream info #23118

kyessenov commented Sep 15, 2022 •

edited

repokitteh-read-only bot commented Sep 15, 2022

alyssawilk left a comment

alyssawilk Sep 15, 2022

kyessenov Sep 15, 2022

alyssawilk Sep 15, 2022

kyessenov Sep 15, 2022

alyssawilk Sep 15, 2022

kyessenov Sep 15, 2022

alyssawilk Sep 15, 2022

kyessenov Sep 15, 2022

alyssawilk left a comment

alyssawilk Sep 19, 2022

kyessenov Sep 19, 2022

markdroth Sep 21, 2022

kyessenov Sep 21, 2022

alyssawilk left a comment

markdroth commented Sep 21, 2022

alyssawilk left a comment

alyssawilk Sep 21, 2022

kyessenov Sep 21, 2022

alyssawilk Sep 21, 2022

kyessenov Sep 21, 2022

markdroth commented Sep 21, 2022

alyssawilk left a comment

ggreenway left a comment

ggreenway Sep 22, 2022

kyessenov Sep 22, 2022

tcp tunneling: optionally move response header to downstream info #23118

tcp tunneling: optionally move response header to downstream info #23118

Conversation

kyessenov commented Sep 15, 2022 • edited

repokitteh-read-only bot commented Sep 15, 2022

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

markdroth commented Sep 21, 2022

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markdroth commented Sep 21, 2022

alyssawilk left a comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Sep 15, 2022 •

edited