Envoy core changes for reverse connections #37368

basundhara-c · 2024-11-26T11:57:43Z

Commit Message: This commit collates the envoy core changes for reverse connections, described in this github issue. A detailed description of reverse connections concepts and workflows is provided in both the github issue and in the examples section.

Additional Description: This PR involves several working components, that are added as part of the following extensions:

"envoy.filters.http.reverse_conn" HTTP filter: reverse connection: code changes for reverse conn http filter #37822
"envoy.bootstrap.reverse_connection" bootstrap extension: reverse connection: reverse connection bootstrap extension #37819
"envoy.filters.listener.reverse_connection" listener filter: reverse connection: code changes for reverse connection listener filter #37821
"envoy.reverse_connection.reverse_connection_listener_config" listener that initiates reverse connections: reverse connection: code changes for a listener that initiates reverse connections #37820
"envoy.reverse_connection.conn_pool": A custom conn pool that enables requests to be sent over reverse connections: reverse_connection: custom conn pool that sends requests through reverse connections #37931
"envoy.clusters.reverse_connection": A custom cluster type for downstream services that are behind reverse connections: reverse_conn: envoy contrib changes for reverse_connection custom cluster #37932

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Basundhara Chakrabarty [email protected]
Co-authored-by: Arun Vasudevan [email protected]
Co-authored-by: Tejas Sangol [email protected]
Co-authored-by: Aditya Jaltade [email protected]

repokitteh-read-only · 2024-11-26T11:57:49Z

Hi @basundhara-c, 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: #37368 was opened by basundhara-c.

see: more, trace.

Commit Message: This commit collates the envoy core changes for reverse connections. Additional Description: Risk Level: Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] Signed-off-by: Basundhara Chakrabarty <[email protected]> Co-authored-by: Arun Vasudevan <[email protected]> Co-authored-by: Tejas Sangol <[email protected]> Co-authored-by: Aditya Jaltade <[email protected]>

alyssawilk

I'm really glad you got this working! Tossed in a few high level comments but I'm going to assign a first pass reviewer for the rest

alyssawilk · 2024-12-02T15:34:10Z

envoy/event/dispatcher.h

+  /**
+   * @return the cluster manager pointer.
+   */
+  virtual Upstream::ClusterManager* getClusterManager() PURE;


I don't think cluster manager APIs belong in the dispatcher - I think it's worth finding a more clean way to get the cluster manager you need.

@basundhara-c, could you pass the singleton ClusterManager() from server when creating the RCThreadLocalRegistry in your extension, so that the RCmanager has the thread_local_cluster() api, and we don't need to use this worker_.dispatcher().set/getClusterManager()?

alyssawilk · 2024-12-02T15:36:17Z

envoy/event/dispatcher.h

+   * Provides filters access to connection handler to save outgoing connections as
+   * incoming connections for reverse tunnels
+   */
+  virtual void setConnectionHandler(Network::ConnectionHandler* connection_handler) PURE;


I don't see what this function has to do with the dispatcher - did you stash this here as a convenient way to get the handler local to your thread? I think you want to look into the thread local state (tls) getters and setters.

alyssawilk · 2024-12-02T15:37:14Z

envoy/network/connection_handler.h

@@ -19,6 +21,26 @@
 namespace Envoy {
 namespace Network {

+// The thread local registry.
+class LocalRevConnRegistry {


I suspect you should be able to do this PR with minimal APIs in core Envoy, but instead in your extension directory

@alyssawilk The main set of changes is to support the creation of the ReverseConnectionManager and ReverseConnectionHandler thread-locally as implemented using a thread-local registry. The ReverseConnectionManager and ReverseConnectionHandler need to come up right after the workers are created because they are involved in accepting reverse connections from other initiating envoy instances even if the local envoy is not initiating any reverse connections, as long as the local envoy has the reverse connections bootstrap extension enabled.

So, currently, we check if the reverse connection bootstrap extension is enabled by checking if the singleton is present, and if so, right after workers are created, we post to each worker's dispatcher a functor to create the Thread Local Registries (creating the ReverseConnectionManager and ReverseConnectionHandler). This thread local registry is then parked with the Connection Handler and is accessed later wherever we need access to the ReverseConnectionManager and ReverseConnectionHandler like here,to initiate reverse conns, here in the reverse conn filter, which accepts reverse conns and in multiple other places where only the thread local dispatcher is otherwise available to us. Therefore, I have defined abstract classes for LocalRevConnRegistry, RevConnRegistry, etc so that these two entities can be stored in the form of a ThreadLocalRegistry and accessed later. We briefly touched upon this in this comment and on slack. In summary:

We have created two thread local entities which get initialized right after the workers are created. They need to be parked somewhere so that they can be accessed later in code paths where only the thread local dispatcher is available. We have parked them with the thread local connection handler and therefore added the abstract classes under Envoy::Network.

If the above is not the best approach, what would you suggest?

This thread local registry is then parked with the Connection Handler and is accessed later wherever we need access to the ReverseConnectionManager and ReverseConnectionHandler like here,to initiate reverse conns, here in the reverse conn filter, which accepts reverse conns and in multiple other places where only the thread local dispatcher is otherwise available to us

For filters we can get the slot from serverFactoryContext from Server::Configuration::FactoryContext& context during the filter chain creation, could you try to see if this can avoid the setConnectionHandler api changes?

@botengyao I have implemented this in the last set of commits.

botengyao

Thanks @basundhara-c, very interesting feature!
Here is a first pass to kick off the process.

botengyao · 2024-12-04T19:48:07Z

envoy/event/dispatcher.h

+  /**
+   * @return the cluster manager pointer.
+   */
+  virtual Upstream::ClusterManager* getClusterManager() PURE;


@basundhara-c, could you pass the singleton ClusterManager() from server when creating the RCThreadLocalRegistry in your extension, so that the RCmanager has the thread_local_cluster() api, and we don't need to use this worker_.dispatcher().set/getClusterManager()?

botengyao · 2024-12-04T20:03:28Z

envoy/network/connection_handler.h

@@ -19,6 +21,26 @@
 namespace Envoy {
 namespace Network {

+// The thread local registry.
+class LocalRevConnRegistry {


This thread local registry is then parked with the Connection Handler and is accessed later wherever we need access to the ReverseConnectionManager and ReverseConnectionHandler like here,to initiate reverse conns, here in the reverse conn filter, which accepts reverse conns and in multiple other places where only the thread local dispatcher is otherwise available to us

For filters we can get the slot from serverFactoryContext from Server::Configuration::FactoryContext& context during the filter chain creation, could you try to see if this can avoid the setConnectionHandler api changes?

botengyao · 2024-12-04T20:31:33Z

source/common/listener_manager/connection_handler_impl.cc

+                 "Thread local rverse conn registry should not be null.");
+}
+
+void ConnectionHandlerImpl::saveUpstreamConnection(Network::ConnectionSocketPtr&& upstream_socket,


We want to store the upstream socket to a dedicated REVESR_CLUSTER cluster so that it can be reused when public Envoy wants to establish connections to the on-perm one. And we also wan to save upstream connection here is just for PING and other handshake processes.

The request flow is like:

on-prem Envoy -> public Envoy

Then the public Envoy stores the downstream socket to the REVESR_CLUSTER,

and also initialize the listener to do RPING

And then the service behind the public Envoy can send request -> normal listener -> filster-chain / router -> the stored socket in REVERSE_CLUSTER.

Am I understanding correctly?

@botengyao yes, most of the above is correct, the request flow is as follows:

Onprem -> Cloud envoy

Onprem initiates reverse connections; it creates http connections to cloud envoy and sends a reverse connection initiation request through it.

This request is intercepted by the reverse conn http filter and cloud envoy stores the sockets with the ReverseConnectionHandler. This ReverseConnectionHandler periodically sends RPINGs over all such cached sockets.

On cloud envoy, a REVERSE_CONNECTION cluster type is defined and is used for all requests that need to be sent over a reverse connection. When a request arrives and this REVERSE_CONNECTION cluster is picked for the route, we interface with the ReverseConnectionHandler described above, get the cached socket and send the request over it.

botengyao · 2024-12-04T20:35:47Z

source/common/network/connection_impl.cc

@@ -263,6 +278,10 @@ void ConnectionImpl::setDetectedCloseType(DetectedCloseType close_type) {
 }

 void ConnectionImpl::closeSocket(ConnectionEvent close_type) {
+  if (connection_reused_ || !ConnectionImpl::ioHandle().isOpen()) {


When connection_reused_ is always true, how do we close the socket?

@botengyao apologies for the late reply, we don't, in this case, since reverse connections are a very small number of long lived connections. However, we might be able to mark sockets dead in the Listener filter which will be passed these sockets (#37821), will test it out and update it there.

We could need a red_button to clean up the resources even connection_reused_ is true.

emm, the changes in the connection socket with the customized connection reuse check seem hacky. It could be cleaner to create a dedicated close sequence only for the reused connection? Besides, the close can also be initialized by the peer, what will happen in this case? The socket will not close but the remote is closed, will we enter a dead loop when we find we cannot read/write data?

@botengyao I am trying to find a cleaner way of running the close sequence.

basundhara-c · 2024-12-11T01:21:08Z

@botengyao thanks a lot for the suggestion on obtaining the slot from the context! I am trying that out along with an attempt to move the code in extensions to contrib as much as possible and will be sharing the changes shortly!

Signed-off-by: Basundhara Chakrabarty <[email protected]>

…er to the RCManager 2. Deleting unwanted APIs and ReverseConnectionManager and Handler header files Signed-off-by: Basundhara Chakrabarty <[email protected]>

Signed-off-by: Basundhara Chakrabarty <[email protected]>

basundhara-c · 2025-01-06T23:55:10Z

@botengyao we have added a bunch of changes according to your suggestion, namely:

Removed the getters and setters for connection handler and cluster manager
The RCManager and RCHandler are now self contained within contrib/ and not parked with the Dispatcher. The filters and other entities obtain them through TLS getters. This has reduced the envoy core code.
Have added changes required to remove references to extensions code as the reverse connections code is now in contrib/
Will await further suggestions on this PR, thank you in advance!

alyssawilk · 2025-01-07T14:34:34Z

@botengyao can you take a look today?
I'm going to remove myself for now just to get it off PR notifier but please add me back once you're done with your pass.

botengyao

Thanks for working on this! Here is another pass, and some changes need at least unit tests to improve the coverage.

/wait

envoy/network/listener.h

envoy/network/connection_handler.h

source/common/listener_manager/connection_handler_impl.h

source/common/json/json_loader.cc

source/common/listener_manager/connection_handler_impl.cc

botengyao · 2025-01-07T22:10:00Z

source/common/listener_manager/listener_impl.cc

+                    config.name()));
+  }
+  // Reverse connection listener should not bind to port.
+  bind_to_port_ = false;


It would be better to add the invalid config check here, and we can reject it if the config contains bind_to_port.

@botengyao The bind_to_port_ field is set to true by default in shouldBindToPort(config). The idea here is to set bind_to_port_ to false for any listener with a reverse connection config. Are you suggesting that we make it mandatory for bind_to_port to be explicitly set to false in the listener config for reverse connection listener?

I see, I don't see there is reverse_connection_listener_config in the v3::Listener proto, is it missing? A dedicated sub-config for reverse connection is needed and if some conditions are needed we can update and validate the config.

The reverse_connection_listener_config was indeed missing, my apologies! Added it in listener.proto

This is an indicator that the test is missing, could you add a test to cover the config, please?

…eck for reverse conn cluster Signed-off-by: Basundhara Chakrabarty <[email protected]>

Signed-off-by: Basundhara Chakrabarty <[email protected]>

…ptr and some minor nits Signed-off-by: Basundhara Chakrabarty <[email protected]>

Signed-off-by: Basundhara Chakrabarty <[email protected]>

botengyao

Much better! Thanks for the contribution and here is another pass.

/wait

botengyao · 2025-01-09T21:41:59Z

source/common/listener_manager/listener_impl.cc

+                    config.name()));
+  }
+  // Reverse connection listener should not bind to port.
+  bind_to_port_ = false;


I see, I don't see there is reverse_connection_listener_config in the v3::Listener proto, is it missing? A dedicated sub-config for reverse connection is needed and if some conditions are needed we can update and validate the config.

source/common/upstream/cluster_manager_impl.cc

source/common/upstream/upstream_impl.h

botengyao · 2025-01-09T22:15:53Z

source/common/network/connection_impl.cc

@@ -263,6 +278,10 @@ void ConnectionImpl::setDetectedCloseType(DetectedCloseType close_type) {
 }

 void ConnectionImpl::closeSocket(ConnectionEvent close_type) {
+  if (connection_reused_ || !ConnectionImpl::ioHandle().isOpen()) {


emm, the changes in the connection socket with the customized connection reuse check seem hacky. It could be cleaner to create a dedicated close sequence only for the reused connection? Besides, the close can also be initialized by the peer, what will happen in this case? The socket will not close but the remote is closed, will we enter a dead loop when we find we cannot read/write data?

agrawroh · 2025-01-10T16:40:12Z

source/server/server.cc

@@ -765,6 +766,7 @@ absl::Status InstanceBase::initializeOrThrow(Network::Address::InstanceConstShar
  ASSERT(config_.clusterManager());
  xds_manager_ =
      std::make_unique<Config::XdsManagerImpl>(*config_.clusterManager(), validation_context_);
+  listener_manager_->setClusterManagerForWorkers(config_.clusterManager());


Looks like there is no setClusterManagerForWorkers() anymore.

Thanks for pointing this out! Fixed some issues caused due to missed code including removal of this and addition of the reverse_connection_listener_config field in listener.proto
@botengyao

…n in server Signed-off-by: Basundhara Chakrabarty <[email protected]>

…verse connections Signed-off-by: Basundhara Chakrabarty <[email protected]>

repokitteh-read-only · 2025-01-13T06:13:34Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #37368 was synchronize by basundhara-c.

see: more, trace.

Signed-off-by: Basundhara Chakrabarty <[email protected]>

botengyao

Hi @phlax, do you have insights why the ci doesn't work for this PR?
It is showing Run # Check if the merge commit SHA is not null Merge commit information not found for pull request 37368.

botengyao

Thanks for the dedicated effort! Looks good in high level except the runtime guard and connection close sequence.

/wait

botengyao · 2025-01-15T01:09:13Z

source/common/listener_manager/listener_impl.cc

+                    config.name()));
+  }
+  // Reverse connection listener should not bind to port.
+  bind_to_port_ = false;


This is an indicator that the test is missing, could you add a test to cover the config, please?

botengyao · 2025-01-15T01:12:21Z

source/common/http/filter_manager.cc

@@ -1000,10 +1000,12 @@ void DownstreamFilterManager::sendLocalReply(
      // route refreshment in the response filter chain.
      cb->route(nullptr);
    }
-
+
+    bool reverse_conn_force_local_reply = 


We perfer latching the runtime value to avoid the cost to get it every time sending a local reply.

botengyao · 2025-01-15T01:35:41Z

source/common/http/filter_manager.cc

    // We only prepare a local reply to execute later if we're actively
    // invoking filters to avoid re-entrant in filters.
-    if (state_.filter_call_state_ & FilterCallState::IsDecodingMask) {
+    if (!reverse_conn_force_local_reply && state_.filter_call_state_ & FilterCallState::IsDecodingMask) {


Actually a runtime guard has a 6 month deletion policy, and it is usually used to guard the data plane behavior change. I think here you want a parameter to tell if this is going to be a local reply on a reverse connection. Could you point me where you detect this use case, and I assume it is in your HTTP filter extension.

phlax · 2025-01-15T10:02:59Z

@botengyao

This branch has conflicts that must be resolved

jmarantz · 2025-01-16T14:08:43Z

/wait (Boteng has unresolved comments)

basundhara-c force-pushed the reverse_conn_envoy_core branch from 00a138f to e8d32a5 Compare November 26, 2024 20:57

basundhara-c force-pushed the reverse_conn_envoy_core branch from e8d32a5 to 457e1e9 Compare November 26, 2024 21:04

basundhara-c mentioned this pull request Nov 26, 2024

Envoy extensions for reverse connections #37367

Open

RyanTheOptimist assigned alyssawilk Nov 27, 2024

alyssawilk reviewed Dec 2, 2024

View reviewed changes

alyssawilk assigned botengyao Dec 2, 2024

alyssawilk added the waiting label Dec 3, 2024

botengyao reviewed Dec 4, 2024

View reviewed changes

alyssawilk added waiting and removed waiting labels Dec 5, 2024

basundhara-c added 4 commits December 25, 2024 06:59

Pass cluster manager while creating tls registry

6ce719e

Signed-off-by: Basundhara Chakrabarty <[email protected]>

Envoy core changes to use contrib/ extensions

3d33358

Signed-off-by: Basundhara Chakrabarty <[email protected]>

1. Remove the getDispatcher API and instead pass the connection handl…

3331617

…er to the RCManager 2. Deleting unwanted APIs and ReverseConnectionManager and Handler header files Signed-off-by: Basundhara Chakrabarty <[email protected]>

Formatting files

4104495

Signed-off-by: Basundhara Chakrabarty <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 25, 2024

Nit: Remove more extraneous code

edd3b17

Signed-off-by: Basundhara Chakrabarty <[email protected]>

alyssawilk removed their assignment Jan 7, 2025

botengyao reviewed Jan 7, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 7, 2025

This was referenced Jan 8, 2025

reverse_connection: custom conn pool that sends requests through reverse connections #37931

Open

reverse_conn: envoy contrib changes for reverse_connection custom cluster #37932

Open

basundhara-c added 2 commits January 8, 2025 23:52

reverse_conn: resolve PR comments, add logic in cluster manager to ch…

553fada

…eck for reverse conn cluster Signed-off-by: Basundhara Chakrabarty <[email protected]>

reverse_conn: add mock methods, add tests for json utils

5401791

Signed-off-by: Basundhara Chakrabarty <[email protected]>

repokitteh-read-only bot removed the waiting label Jan 8, 2025

basundhara-c added 2 commits January 9, 2025 05:57

nit: Change listener_map_by_tag_ to use shared_ptr instead of unique_…

7c270c2

…ptr and some minor nits Signed-off-by: Basundhara Chakrabarty <[email protected]>

Nit: Add mocks for Host functions

05ee7d6

Signed-off-by: Basundhara Chakrabarty <[email protected]>

botengyao reviewed Jan 9, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 9, 2025

agrawroh reviewed Jan 10, 2025

View reviewed changes

basundhara-c added 2 commits January 13, 2025 06:11

Nit: Add listener proto field and minor nit to remove deleted functio…

841dfea

…n in server Signed-off-by: Basundhara Chakrabarty <[email protected]>

Minor nits to reconcile missed code, and added a runtime guard for re…

ba4e0bf

…verse connections Signed-off-by: Basundhara Chakrabarty <[email protected]>

repokitteh-read-only bot added api and removed waiting labels Jan 13, 2025

repokitteh-read-only bot assigned mattklein123 Jan 13, 2025

Nit: Correct/Add mocks for HostDescription

ae3e00b

Signed-off-by: Basundhara Chakrabarty <[email protected]>

botengyao reviewed Jan 14, 2025

View reviewed changes

botengyao reviewed Jan 15, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 15, 2025

Envoy core changes for reverse connections #37368

Are you sure you want to change the base?

Envoy core changes for reverse connections #37368

Conversation

basundhara-c commented Nov 26, 2024 • edited Loading

repokitteh-read-only bot commented Nov 26, 2024

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

botengyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

botengyao Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basundhara-c Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

botengyao Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

botengyao Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basundhara-c commented Dec 11, 2024

basundhara-c commented Jan 6, 2025

alyssawilk commented Jan 7, 2025

botengyao left a comment

Choose a reason for hiding this comment

botengyao Jan 7, 2025 • edited Loading

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

botengyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

botengyao Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Jan 13, 2025

botengyao left a comment

Choose a reason for hiding this comment

botengyao 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

phlax commented Jan 15, 2025

jmarantz commented Jan 16, 2025

basundhara-c commented Nov 26, 2024 •

edited

Loading

botengyao Dec 4, 2024 •

edited

Loading

basundhara-c Jan 6, 2025 •

edited

Loading

botengyao Jan 7, 2025 •

edited

Loading

botengyao Jan 9, 2025 •

edited

Loading

botengyao Jan 7, 2025 •

edited

Loading

botengyao Jan 9, 2025 •

edited

Loading