original_dst listener filter: implement internal listener support #29655

kyessenov · 2023-09-15T22:26:47Z

Commit Message: Adds support for recovery of the local and the remote addresses in the internal connections using the original_dst listener filter. This supports two use cases:

cluster endpoint tunneling: an endpoint host metadata is passed through to the internal listener to set the IP destination, example:

name: internal_outbound
load_assignment:
  cluster_name: internal_outbound
  endpoints:
  - lb_endpoints:
    - endpoint:
        address:
          envoy_internal_address:
            server_listener_name: internal_outbound
      metadata:
        filter_metadata:
          envoy.filters.listener.original_dst:
            local: 127.0.0.1:8080 # Actual network destination
transport_socket:
  name: envoy.transport_sockets.internal_upstream
  typed_config:
    "@type": type.googleapis.com/envoy.extensions.transport_sockets.internal_upstream.v3.InternalUpstreamTransport
    passthrough_metadata:
    - name: envoy.filters.listener.original_dst
      kind: { host: {}}
    transport_socket:
      name: envoy.transport_sockets.raw_buffer
      typed_config:
        "@type": type.googleapis.com/envoy.extensions.transport_sockets.raw_buffer.v3.RawBuffer

de-tunneling, a CONNECT stream saves the host as the destination address, which is then passed through to the internal listener upstream. This requires a custom filter that sets a filter state object and marks it as shared with the upstream connection. Example implementation: https://github.com/istio/proxy/blob/master/source/extensions/filters/http/connect_authority/filter.cc

Additional Description:
Risk Level: low, new feature
Testing: added
Docs Changes: yes
Release Notes: yes
Issue: #29652
issue: #29681

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

kyessenov · 2023-09-15T22:33:57Z

/assign-from @envoyproxy/envoy-maintainers

repokitteh-read-only · 2023-09-15T22:34:02Z

@envoyproxy/envoy-maintainers assignee is @snowp

🐱

Caused by: a #29655 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2023-09-18T18:06:39Z

/retest

kyessenov · 2023-09-18T18:06:54Z

/assign-from @envoyproxy/envoy-maintainers

repokitteh-read-only · 2023-09-18T18:06:59Z

@envoyproxy/envoy-maintainers assignee is @snowp

🐱

Caused by: a #29655 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2023-09-18T18:08:38Z

/assign-from @envoyproxy/envoy-maintainers

repokitteh-read-only · 2023-09-18T18:08:42Z

@envoyproxy/envoy-maintainers assignee is @snowp

🐱

Caused by: a #29655 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2023-09-18T18:09:14Z

/assign-from @envoyproxy/envoy-maintainers

repokitteh-read-only · 2023-09-18T18:09:19Z

@envoyproxy/envoy-maintainers assignee is @htuch

🐱

Caused by: a #29655 (comment) was created by @kyessenov.

see: more, trace.

htuch · 2023-09-18T19:09:41Z

docs/root/configuration/listeners/listener_filters/original_dst_filter.rst

+
+Dynamic metadata for the destination address is expected to be placed into the
+key `envoy.filters.listener.original_dst` under the field `local` and should
+contain a string with an IP and a port address.


Looking at the existing use of original_dst and this form, it seems the contract with the rest of Envoy is different. This new use case is about populating filter state, rather than original_dst cluster. Feels a bit overloaded, should there be some plans to separate source of information in some consistent metadata from consumption?

Are you thinking of a "common" metadata for the destination? This change is about consuming a filter state and metadata, so the typical chain is the following:

Some listener/cluster that produces filter state and/or metadata to an internal address.

Internal listener with original_dst listener filter.

Any cluster, common to have original_dst cluster here.

I was thinking that original_dst listener filter updates the addresses, we no longer need to modify original_dst cluster. But it already has a filter state defined for the same purpose, so we could reuse the same object. This approach might interfere with normal original_dst cluster behavior because filter state takes priority and there is no way to delete a filter state object.

Are you saying we're using the same filter state as the existing original_dst cluster? If so, I suggest refactoring the docs to make this explicit.

No, we're using different filter states. ORIGINAL_DST cluster uses envoy.network.transport_socket.original_dst_address. original_dst listener filter uses envoy.filters.listener.original_dst.local_ip. I thought you suggested to share them? I think it's safer not to share.

I did a bit of refactoring to make it clear that ORIGINAL_DST cluster and filter share the object but have different keys.

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

htuch · 2023-09-20T03:58:06Z

test/extensions/filters/listener/original_dst/original_dst_test.cc

+  EXPECT_TRUE(socket_.connectionInfoProvider().localAddressRestored());
+  EXPECT_EQ(local->asString(), socket_.connectionInfoProvider().localAddress()->asString());
+  EXPECT_EQ(remote->asString(), socket_.connectionInfoProvider().remoteAddress()->asString());
+}


Does an integration test make sense?

I don't think it's strictly necessary - the override doesn't affect the data path of original_dst, only where the data comes from. But if you feel like we need it, I can add it. It would need 4 extensions (original_dst filter and cluster, internal listener and upstream).

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

htuch

LGTM, feel free to merge if you think this is done.

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

kyessenov · 2023-09-21T05:26:39Z

This should be ready now, @htuch PTAL again.

implement

d0d22b0

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

kyessenov requested a review from lizan as a code owner September 15, 2023 22:26

minor edits

67ae3b3

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

repokitteh-read-only bot assigned snowp Sep 15, 2023

envoyproxy deleted a comment from repokitteh-read-only bot Sep 18, 2023

kyessenov unassigned snowp Sep 18, 2023

repokitteh-read-only bot assigned htuch Sep 18, 2023

htuch reviewed Sep 18, 2023

View reviewed changes

kyessenov added 2 commits September 18, 2023 19:26

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

8be5b6d

fix bug

db63e17

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

htuch reviewed Sep 20, 2023

View reviewed changes

kyessenov added 3 commits September 20, 2023 23:00

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

068c6a6

revieww

284cd58

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

fix

10fc92a

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

htuch previously approved these changes Sep 21, 2023

View reviewed changes

add coverage

2d810ba

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

kyessenov dismissed htuch’s stale review via 2d810ba September 21, 2023 05:15

typo

be0efa5

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

htuch approved these changes Sep 21, 2023

View reviewed changes

htuch merged commit 287a42f into envoyproxy:main Sep 21, 2023
116 checks passed

marc-barry mentioned this pull request May 30, 2024

Dynamic host determination for plaintext HTTP encapsulation in CONNECT #34435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

original_dst listener filter: implement internal listener support #29655

original_dst listener filter: implement internal listener support #29655

kyessenov commented Sep 15, 2023 •

edited

Loading

kyessenov commented Sep 15, 2023

repokitteh-read-only bot commented Sep 15, 2023

kyessenov commented Sep 18, 2023

kyessenov commented Sep 18, 2023

repokitteh-read-only bot commented Sep 18, 2023

kyessenov commented Sep 18, 2023

repokitteh-read-only bot commented Sep 18, 2023

kyessenov commented Sep 18, 2023

repokitteh-read-only bot commented Sep 18, 2023

htuch Sep 18, 2023

kyessenov Sep 18, 2023

htuch Sep 19, 2023

kyessenov Sep 19, 2023

kyessenov Sep 20, 2023

htuch Sep 20, 2023

kyessenov Sep 20, 2023

htuch left a comment

kyessenov commented Sep 21, 2023

original_dst listener filter: implement internal listener support #29655

original_dst listener filter: implement internal listener support #29655

Conversation

kyessenov commented Sep 15, 2023 • edited Loading

kyessenov commented Sep 15, 2023

repokitteh-read-only bot commented Sep 15, 2023

kyessenov commented Sep 18, 2023

kyessenov commented Sep 18, 2023

repokitteh-read-only bot commented Sep 18, 2023

kyessenov commented Sep 18, 2023

repokitteh-read-only bot commented Sep 18, 2023

kyessenov commented Sep 18, 2023

repokitteh-read-only bot commented Sep 18, 2023

htuch Sep 18, 2023

Choose a reason for hiding this comment

kyessenov Sep 18, 2023

Choose a reason for hiding this comment

htuch Sep 19, 2023

Choose a reason for hiding this comment

kyessenov Sep 19, 2023

Choose a reason for hiding this comment

kyessenov Sep 20, 2023

Choose a reason for hiding this comment

htuch Sep 20, 2023

Choose a reason for hiding this comment

kyessenov Sep 20, 2023

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

kyessenov commented Sep 21, 2023

kyessenov commented Sep 15, 2023 •

edited

Loading