-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Full picture of in memory listener and connection #13361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
fb1db47
6fceaf2
df05bba
d7cac74
be6f09d
d305855
b6d9da5
c413df9
48e0640
e81a41e
72cd41d
a337bde
dc87455
3be784d
51914a9
f12c3d2
219e0da
f8816b0
bc57e78
4c2d693
cfca895
07c1ecb
171b1d9
c99aa54
b25f28d
73c5074
8439a67
d7cff5c
808ea05
50479ca
5fdec41
b55e03e
4a7970c
5ffedb4
7a972a5
81d4c2e
32eca3f
80a9d2c
a5572cb
254daa4
568e42b
953326b
cd30f84
95b5a54
abbffa5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package envoy.config.listener.v3; | ||
|
|
||
| import "udpa/annotations/status.proto"; | ||
| import "udpa/annotations/versioning.proto"; | ||
|
|
||
| option java_package = "io.envoyproxy.envoy.config.listener.v3"; | ||
| option java_outer_classname = "InternalListenerProto"; | ||
| option java_multiple_files = true; | ||
| option (udpa.annotations.file_status).package_version_status = ACTIVE; | ||
|
|
||
| // [#not-implemented-hide:] | ||
| // [#protodoc-title: internal listener] | ||
| // Describes a type of internal listener which expects to serve the cluster in | ||
| // the same envoy process. | ||
| message InternalListener { | ||
| option (udpa.annotations.versioning).previous_message_type = | ||
| "envoy.config.listener.v2.InternalListener"; | ||
| } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,12 @@ | |
| #include <chrono> | ||
| #include <cstdint> | ||
| #include <functional> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include "common/network/address_impl.h" | ||
| #include "common/network/raw_buffer_socket.h" | ||
| #include "envoy/api/api.h" | ||
| #include "envoy/network/listen_socket.h" | ||
| #include "envoy/network/listener.h" | ||
|
|
@@ -18,6 +21,7 @@ | |
| #include "common/event/signal_impl.h" | ||
| #include "common/event/timer_impl.h" | ||
| #include "common/filesystem/watcher_impl.h" | ||
| #include "common/network/buffered_io_socket_handle_impl.h" | ||
| #include "common/network/connection_impl.h" | ||
| #include "common/network/dns_impl.h" | ||
| #include "common/network/tcp_listener_impl.h" | ||
|
|
@@ -117,6 +121,71 @@ DispatcherImpl::createClientConnection(Network::Address::InstanceConstSharedPtr | |
| std::move(transport_socket), options); | ||
| } | ||
|
|
||
| namespace { | ||
| Network::Address::InstanceConstSharedPtr | ||
| nextClientAddress(const Network::Address::InstanceConstSharedPtr& server_address) { | ||
| uint64_t id = 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did you mean this to be static? If so fix and test?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yes. It should be static. The local address is well used, only need to guarantee such address is not nullptr. |
||
| return std::make_shared<Network::Address::EnvoyInternalInstance>(absl::StrCat(server_address->asStringView(), "_", ++id)); | ||
| } | ||
| } // namespace | ||
|
|
||
| Network::ClientConnectionPtr | ||
| DispatcherImpl::createInternalConnection(Network::Address::InstanceConstSharedPtr internal_address, | ||
| Network::Address::InstanceConstSharedPtr local_address) { | ||
| ASSERT(isThreadSafe()); | ||
| if (internal_address == nullptr) { | ||
| return nullptr; | ||
| } | ||
| if (local_address == nullptr) { | ||
| local_address = nextClientAddress(internal_address); | ||
| } | ||
| // Find the internal listener callback. The listener will setup the server connection. | ||
lambdai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| auto iter = internal_listeners_.find(internal_address->asString()); | ||
| for (const auto& [name, _] : internal_listeners_) { | ||
| ENVOY_LOG_MISC(debug, "lambdai: p listener {}", name); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm inclined to do my next pass once you've had a chance to clean these up, and do a sweep to make sure the code is commented.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR is adding 3000 lines. Do you think it is realistic to merge this PR without splitting? If not I prefer to keep the debug purpose log: I will backport the split PRs and see if the sub PRs breaks this overall PR: This PR has quite a few real world integration tests. These integration tests must pass but cannot be put in early sub PRs.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think given the complexity it'd be better to split out what we can. It could be that it's just 2-3? As said I think the buffer code would split out pretty easily so let's give that a try and see what's left. |
||
| } | ||
| if (iter == internal_listeners_.end()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm, I would have expected this in connect(). When we do normal IP style create and connect do we validate before connect? Actually I think if you move this work to CONNECT you can skip the entire closingClientConnectionImpl which would be nice clean up
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I want to split The underlying BTW, The model internal connection I am borrowing from "socketpair()" |
||
| ENVOY_LOG_MISC(debug, "lambdai: no valid listener registered for envoy internal address {}", | ||
| internal_address->asString()); | ||
| return std::make_unique<Network::ClosingClientConnectionImpl>(*this, internal_address, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also some of these are good to keep as debug logs, though not MISC_LOG
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely. Will assign reasonable logger and level in the split PRs |
||
| local_address); | ||
| } | ||
|
|
||
| auto client_io_handle_ = std::make_unique<Network::BufferedIoSocketHandleImpl>(); | ||
| auto server_io_handle_ = std::make_unique<Network::BufferedIoSocketHandleImpl>(); | ||
|
Comment on lines
+154
to
+155
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BufferedIoSocketHandleImpl as extension here? |
||
| client_io_handle_->setWritablePeer(server_io_handle_.get()); | ||
| server_io_handle_->setWritablePeer(client_io_handle_.get()); | ||
|
|
||
| Network::RawBufferSocketFactory client_transport_socket_factory; | ||
| // ConnectionSocket conn_socket | ||
| auto client_conn_socket = std::make_unique<Network::InternalConnectionSocketImpl>( | ||
| std::move(client_io_handle_), local_address, internal_address); | ||
| auto server_conn_socket = std::make_unique<Network::InternalConnectionSocketImpl>( | ||
| std::move(server_io_handle_), internal_address, local_address); | ||
| ENVOY_LOG_MISC(debug, "lambdai: internal address {}", internal_address->asString()); | ||
| ENVOY_LOG_MISC(debug, "lambdai: client address {}", local_address->asString()); | ||
|
|
||
| auto client_conn = std::make_unique<Network::ClientConnectionImpl>( | ||
| *this, internal_address, local_address, | ||
| client_transport_socket_factory.createTransportSocket(nullptr), nullptr, | ||
| std::move(client_conn_socket)); | ||
|
|
||
| (iter->second)(internal_address, std::move(server_conn_socket)); | ||
| return client_conn; | ||
| } | ||
|
|
||
| void DispatcherImpl::registerInternalListener( | ||
| const std::string& internal_listener_id, | ||
| DispatcherImpl::InternalConnectionCallback internal_conn_callback) { | ||
| if (internal_conn_callback == nullptr) { | ||
| ENVOY_LOG_MISC(debug, "lambdai: unregister pipe factory on address {}", internal_listener_id); | ||
| internal_listeners_.erase(internal_listener_id); | ||
| } else { | ||
| ENVOY_LOG_MISC(debug, "lambdai: register pipe factory on address {}", internal_listener_id); | ||
| internal_listeners_[internal_listener_id] = internal_conn_callback; | ||
| } | ||
| } | ||
|
|
||
| Network::DnsResolverSharedPtr DispatcherImpl::createDnsResolver( | ||
| const std::vector<Network::Address::InstanceConstSharedPtr>& resolvers, | ||
| const bool use_tcp_for_dns_lookups) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe explain what binding is in internal address context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Will add that listener side connection could obtain the bind local address for debug.
I also want to add here that I want to introduce a generic "socket options" param in the future to deliver more information including "original address" which is supposed to be "ip/port" address raising this internal address.