envoy
envoy copied to clipboard
envoyproxy
Envoy core changes for reverse connections
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: https://github.com/envoyproxy/envoy/pull/37822
- "envoy.bootstrap.reverse_connection" bootstrap extension: https://github.com/envoyproxy/envoy/pull/37819
- "envoy.filters.listener.reverse_connection" listener filter: https://github.com/envoyproxy/envoy/pull/37821
- "envoy.reverse_connection.reverse_connection_listener_config" listener that initiates reverse connections: https://github.com/envoyproxy/envoy/pull/37820
- "envoy.reverse_connection.conn_pool": A custom conn pool that enables requests to be sent over reverse connections: https://github.com/envoyproxy/envoy/pull/37931
- "envoy.clusters.reverse_connection": A custom cluster type for downstream services that are behind reverse connections: https://github.com/envoyproxy/envoy/pull/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]
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.
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/37368 was opened by basundhara-c.
![repokitteh-read-only[bot] avatar](https://avatars.githubusercontent.com/in/16729?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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!
@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!
@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.
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/).
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/37368 was synchronize by basundhara-c.
@botengyao I wanted to discuss the connection close sequence. The main issue is on the initiator side where we pass the socket to the listener which will henceforth own it and respond to RPINGs on the socket. Therefore, after the function returns, the connection object goes out of scope and the destructor is called, which in turn calls close(). However, the socket has been moved, and socket_ is null. We could remove the following check from close() to make it cleaner:
void ConnectionImpl::close(ConnectionCloseType type) { if (connection_reused_ || !ioHandle().isOpen()) { ENVOY_CONN_LOG_EVENT( debug, "connection_closing", "Not closing conn, conn is to be reused or IO handle is closed: connection_reused_:{}", *this, connection_reused_); // return; } ...}
However, we would need to check for a null socket here and therefore the close() call would return from this point:
void ConnectionImpl::close(ConnectionCloseType type) { if (socket_== nullptr || !socket_->isOpen()) { ENVOY_CONN_LOG_EVENT(debug, "connection_closing", "Not closing conn, socket object is null or socket is not open", *this); return; } ... }
Thereafter, in the listener filter owning the socket, we could handle the socket closure in the onClose() function. I look forward to your suggestions on this!
@botengyao I added the following in the last commit:
- Added a flag "reverse_conn_force_local_reply_" to send a local reply for reverse connections instead of the runtime guard
- Changed the connection close sequence to what I described here
- Added unit tests for the buildReverseConnectionListener() function
However, due to a change in my VM, I missed out on the DCO sign off, and also wanted to add some nits I missed (a mock for setReverseConnForceLocalReply and the runtime guard code removal). Is it okay if I rebase to amend the last commit and add the DCO signoff?
@basundhara-c, yes, could amend to add DCO to your last commit, and update the branch.
@basundhara-c, yes, could amend to add DCO to your last commit, and update the branch.
Done!
Coverage for this Pull Request will be rendered here:
https://storage.googleapis.com/envoy-pr/37368/coverage/index.html
The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.
:cat:
Caused by: a https://github.com/envoyproxy/envoy/pull/37368#issuecomment-2722568346 was created by @basundhara-c.
Coverage for this Pull Request will be rendered here:
https://storage.googleapis.com/envoy-pr/37368/coverage/index.html
The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.
:cat:
Caused by: a https://github.com/envoyproxy/envoy/pull/37368#issuecomment-2770118376 was created by @basundhara-c.
Coverage for this Pull Request will be rendered here:
https://storage.googleapis.com/envoy-pr/37368/coverage/index.html
The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.
:cat:
Caused by: a https://github.com/envoyproxy/envoy/pull/37368#issuecomment-2770967367 was created by @basundhara-c.
Coverage for this Pull Request will be rendered here:
https://storage.googleapis.com/envoy-pr/37368/coverage/index.html
The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.
:cat:
Caused by: a https://github.com/envoyproxy/envoy/pull/37368#issuecomment-2795456708 was created by @basundhara-c.