-
Notifications
You must be signed in to change notification settings - Fork 5.3k
dispatcher: add the client connection factory to support connecting to internal listener #18105
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
Closed
Closed
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
c00cc79
wip
lambdai e037f8e
stash client conn fac
lambdai 9ede26a
make it build
lambdai 14ae9dc
format
lambdai 3758571
adding int conn factory test
lambdai ac32344
basic internal connection factory test
lambdai d8907d1
Merge branch 'main' into privintfactory
lambdai 3eb8f16
add default client connection factory
lambdai b94abcb
adding getFactoryByAddressType
lambdai 85e3a73
Revert "adding getFactoryByAddressType"
lambdai 2adc021
client connection factory follows UntypedFactory
lambdai c22906c
Merge branch 'main' into privintfactory
lambdai bcb375b
fix after merge
lambdai 83c193d
fix gcc switch enum
lambdai a932479
clang-tidy and better name
lambdai a42efa6
renaming
lambdai 4caa42b
tidy
lambdai File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/config/typed_config.h" | ||
| #include "envoy/network/address.h" | ||
| #include "envoy/network/connection.h" | ||
| #include "envoy/network/listen_socket.h" | ||
| #include "envoy/network/transport_socket.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Network { | ||
|
|
||
| class ClientConnectionFactory : public Config::UntypedFactory { | ||
| public: | ||
| ClientConnectionFactory() = default; | ||
| ~ClientConnectionFactory() override = default; | ||
|
|
||
| // Config::UntypedFactory | ||
| std::string category() const override { return "network.connection"; } | ||
|
|
||
| virtual Network::ClientConnectionPtr | ||
| createClientConnection(Event::Dispatcher& dispatcher, | ||
| Network::Address::InstanceConstSharedPtr address, | ||
| Network::Address::InstanceConstSharedPtr source_address, | ||
| Network::TransportSocketPtr&& transport_socket, | ||
| const Network::ConnectionSocket::OptionsSharedPtr& options) PURE; | ||
| }; | ||
|
|
||
| } // namespace Network | ||
| } // namespace Envoy |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
24 changes: 24 additions & 0 deletions
24
source/common/network/default_client_connection_factory.cc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| #include "source/common/network/default_client_connection_factory.h" | ||
|
|
||
| #include "envoy/registry/registry.h" | ||
|
|
||
| #include "source/common/network/address_impl.h" | ||
| #include "source/common/network/connection_impl.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
||
| namespace Network { | ||
|
|
||
| Network::ClientConnectionPtr DefaultClientConnectionFactory::createClientConnection( | ||
| Event::Dispatcher& dispatcher, Network::Address::InstanceConstSharedPtr address, | ||
| Network::Address::InstanceConstSharedPtr source_address, | ||
| Network::TransportSocketPtr&& transport_socket, | ||
| const Network::ConnectionSocket::OptionsSharedPtr& options) { | ||
| ASSERT(address->ip() || address->pipe()); | ||
| return std::make_unique<Network::ClientConnectionImpl>(dispatcher, address, source_address, | ||
| std::move(transport_socket), options); | ||
| } | ||
| REGISTER_FACTORY(DefaultClientConnectionFactory, Network::ClientConnectionFactory); | ||
|
|
||
| } // namespace Network | ||
| } // namespace Envoy |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/common/pure.h" | ||
| #include "envoy/network/client_connection_factory.h" | ||
| #include "envoy/network/connection.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
||
| namespace Network { | ||
|
|
||
| class DefaultClientConnectionFactory : public Network::ClientConnectionFactory { | ||
| public: | ||
| ~DefaultClientConnectionFactory() override = default; | ||
| std::string name() const override { return "default"; } | ||
| Network::ClientConnectionPtr | ||
| createClientConnection(Event::Dispatcher& dispatcher, | ||
| Network::Address::InstanceConstSharedPtr address, | ||
| Network::Address::InstanceConstSharedPtr source_address, | ||
| Network::TransportSocketPtr&& transport_socket, | ||
| const Network::ConnectionSocket::OptionsSharedPtr& options) override; | ||
| }; | ||
|
|
||
| } // namespace Network | ||
| } // namespace Envoy |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
56 changes: 56 additions & 0 deletions
56
source/extensions/io_socket/user_space/client_connection_factory.cc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| #include "source/extensions/io_socket/user_space/client_connection_factory.h" | ||
|
|
||
| #include "envoy/registry/registry.h" | ||
|
|
||
| #include "source/common/network/address_impl.h" | ||
| #include "source/common/network/connection_impl.h" | ||
| #include "source/common/network/listen_socket_impl.h" | ||
| #include "source/extensions/io_socket/user_space/io_handle_impl.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
||
| namespace Extensions { | ||
| namespace IoSocket { | ||
| namespace UserSpace { | ||
|
|
||
| Network::ClientConnectionPtr InternalClientConnectionFactory::createClientConnection( | ||
| Event::Dispatcher& dispatcher, Network::Address::InstanceConstSharedPtr address, | ||
| Network::Address::InstanceConstSharedPtr source_address, | ||
| Network::TransportSocketPtr&& transport_socket, | ||
| const Network::ConnectionSocket::OptionsSharedPtr& options) { | ||
|
|
||
| auto [io_handle_client, io_handle_server] = | ||
| Extensions::IoSocket::UserSpace::IoHandleFactory::createIoHandlePair(); | ||
|
|
||
| auto client_conn = std::make_unique<Network::ClientConnectionImpl>( | ||
| dispatcher, | ||
| std::make_unique<Network::ConnectionSocketImpl>(std::move(io_handle_client), source_address, | ||
| address), | ||
| source_address, std::move(transport_socket), options); | ||
|
|
||
| // It's either in the main thread or the worker is not yet started. | ||
| auto internal_listener_manager = dispatcher.getInternalListenerManager(); | ||
| if (!internal_listener_manager.has_value()) { | ||
| io_handle_server->close(); | ||
| return client_conn; | ||
| } | ||
|
|
||
| // The request internal listener may not exist. | ||
| auto internal_listener = internal_listener_manager.value().get().findByAddress(address); | ||
| if (!internal_listener.has_value()) { | ||
| io_handle_server->close(); | ||
| return client_conn; | ||
| } | ||
|
|
||
| auto accepted_socket = std::make_unique<Network::AcceptedSocketImpl>(std::move(io_handle_server), | ||
| address, source_address); | ||
| internal_listener->onAccept(std::move(accepted_socket)); | ||
| return client_conn; | ||
| } | ||
|
|
||
| REGISTER_FACTORY(InternalClientConnectionFactory, Network::ClientConnectionFactory); | ||
|
|
||
| } // namespace UserSpace | ||
| } // namespace IoSocket | ||
| } // namespace Extensions | ||
| } // namespace Envoy |
34 changes: 34 additions & 0 deletions
34
source/extensions/io_socket/user_space/client_connection_factory.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| #pragma once | ||
|
|
||
| #include <memory> | ||
| #include <string> | ||
|
|
||
| #include "envoy/common/pure.h" | ||
| #include "envoy/network/client_connection_factory.h" | ||
| #include "envoy/network/connection.h" | ||
|
|
||
| #include "source/common/common/logger.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
||
| namespace Extensions { | ||
| namespace IoSocket { | ||
| namespace UserSpace { | ||
|
|
||
| class InternalClientConnectionFactory : public Network::ClientConnectionFactory, | ||
| Logger::Loggable<Logger::Id::connection> { | ||
| public: | ||
| ~InternalClientConnectionFactory() override = default; | ||
| std::string name() const override { return "EnvoyInternal"; } | ||
| Network::ClientConnectionPtr | ||
| createClientConnection(Event::Dispatcher& dispatcher, | ||
| Network::Address::InstanceConstSharedPtr address, | ||
| Network::Address::InstanceConstSharedPtr source_address, | ||
| Network::TransportSocketPtr&& transport_socket, | ||
| const Network::ConnectionSocket::OptionsSharedPtr& options) override; | ||
| }; | ||
|
|
||
| } // namespace UserSpace | ||
| } // namespace IoSocket | ||
| } // namespace Extensions | ||
| } // namespace Envoy |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could we avoid use of the dispatcher interface as a registration mechanism for factories like this one?
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.
It seems there was slight agreement on this so I stopped investing on bootstrap extension.
My opinion is that adding this interface to dispatcher is more straightforward, well, at the cost of adding getter/setter
CC @yanavlasov
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.
I think we need a client connection factory extension point that connection pools and other things can use to create connections. An eventual goal may be to completely remove the client connection creation methods from the dispatcher. But this is something that @envoyproxy/senior-maintainers may want to chime in on.
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.
I see two arguments here,
the first is to store the internal listener registry, here I choose dispatcher over bootstrap extension(where we can add a threadlocal registery), because the life time of dispatcher and listener manager is cleaner. The dispatcher is guaranteed existing when an internal listener is ready to add and use.
I think we can add the complexity in booting the server, and offer a hook for internal listener registry if we choose to store the internal listeners there. I feel we can do that after.
the second is to remove the thin dispatcher::createClientConnection. Again we can do that along with this PR but I prefer to do it later when signed off.
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.
The current approach of adding the internal listener registry to the dispatcher does not generalize to the next extension that needs to hook into the code that creates client connections. I recommend introducing a factory mechanism for client connections and transition to it, eventually deprecating the current Dispatcher::createClientConnection
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.
I agree with @antoniovicente that the extension behavior has leaked into core interfaces. II think it would be cleaner if the mechanism for finding the listener was completely encapsulated in the factory. For example when internal listener is created it is registering itself in the factory's thread local storage, where it can later be looked up for establishing client connections. In this way you can avoid needing to change the dispatcher interface and ConnectionHandlerImpl