-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
network: add socket interface factory and config option #11630
Changes from 15 commits
388d1f9
c241e44
cc4d075
d77b0a9
fdd0e2f
1ca8153
6c34d86
0f300c8
3612cfa
af294af
43805c8
a02aa41
b622854
bd1a63d
f9cc098
97368af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
---|---|---|
@@ -0,0 +1,9 @@ | ||
# DO NOT EDIT. This file is generated by tools/proto_sync.py. | ||
|
||
load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package") | ||
|
||
licenses(["notice"]) # Apache 2 | ||
|
||
api_proto_package( | ||
deps = ["@com_github_cncf_udpa//udpa/annotations:pkg"], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
syntax = "proto3"; | ||
|
||
package envoy.extensions.network.socket_interface.v3; | ||
|
||
import "udpa/annotations/status.proto"; | ||
|
||
option java_package = "io.envoyproxy.envoy.extensions.network.socket_interface.v3"; | ||
option java_outer_classname = "DefaultSocketInterfaceProto"; | ||
option java_multiple_files = true; | ||
option (udpa.annotations.file_status).package_version_status = ACTIVE; | ||
|
||
// [#protodoc-title: Default Socket Interface configuration] | ||
|
||
// Configuration for default socket interface that relies on OS dependent syscall to create | ||
// sockets. | ||
message DefaultSocketInterface { | ||
} |
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 |
---|---|---|
@@ -0,0 +1,56 @@ | ||
#pragma once | ||
|
||
#include "envoy/config/typed_config.h" | ||
#include "envoy/network/socket.h" | ||
#include "envoy/registry/registry.h" | ||
#include "envoy/server/bootstrap_extension_config.h" | ||
|
||
#include "common/singleton/threadsafe_singleton.h" | ||
|
||
#include "absl/container/flat_hash_map.h" | ||
|
||
namespace Envoy { | ||
namespace Network { | ||
|
||
// Wrapper for SocketInterface instances returned by createBootstrapExtension() which must be | ||
// implemented by all factories that derive SocketInterfaceBase | ||
class SocketInterfaceExtension : public Server::BootstrapExtension { | ||
public: | ||
SocketInterfaceExtension(SocketInterface& sock_interface) : sock_interface_(sock_interface) {} | ||
SocketInterface& socketInterface() { return sock_interface_; } | ||
|
||
private: | ||
SocketInterface& sock_interface_; | ||
}; | ||
|
||
// Class to be derived by all SocketInterface implementations. | ||
// | ||
// It acts both as a SocketInterface and as a BootstrapExtensionFactory. The latter is used, on the | ||
// one hand, to configure and initialize the interface and, on the other, for SocketInterface lookup | ||
// by leveraging the FactoryRegistry. As required for all bootstrap extensions, all derived classes | ||
// should register via the REGISTER_FACTORY() macro as BootstrapExtensionFactory. | ||
// | ||
// SocketInterface instances can be retrieved using the factory name, i.e., string returned by | ||
// name() function implemented by all classes that derive SocketInterfaceBase, via | ||
// Network::socketInterface(). When instantiating addresses, address resolvers should | ||
// set the socket interface field to the name of the socket interface implementation that should | ||
// be used to create sockets for said addresses. | ||
class SocketInterfaceBase : public SocketInterface, | ||
public Server::Configuration::BootstrapExtensionFactory {}; | ||
|
||
/** | ||
* Lookup SocketInterface instance by name | ||
* @param name Name of the socket interface to be looked up | ||
* @return Pointer to @ref SocketInterface instance that registered using the name of nullptr | ||
*/ | ||
static inline const SocketInterface* socketInterface(std::string name) { | ||
auto factory = | ||
Registry::FactoryRegistry<Server::Configuration::BootstrapExtensionFactory>::getFactory(name); | ||
return dynamic_cast<SocketInterface*>(factory); | ||
} | ||
|
||
using SocketInterfaceSingleton = InjectableSingleton<SocketInterface>; | ||
using SocketInterfaceLoader = ScopedInjectableLoader<SocketInterface>; | ||
|
||
} // namespace Network | ||
} // namespace Envoy |
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.
Shall we do this in
server.cc
? i.e. if another socket interface is registered and specified as default, we don't need to instantiate default socket interface?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.
Definitely a good point but if tests are run without the server being properly initialized, the
SocketInterfaceSingleton
won't be properly initialized. Just ran a quick test and they do crash.Should we maybe statically initialize the singleton to the default
SocketInterface
in some common test file, liketest/test_common/environment.cc
?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.
Turns out integration:hotrestart_test, integration:run_envoy_test and test:router_tool_test still fail :-(
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.
Yeah I think that's good way to do test.
OTOH, if this causes a lot churn, we can do static registration here, then let's not do factory registration for default interface. Because even they're configured through
bootstrap_extensions
, it has nothing different than statically registered 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.
Because we do some network related things early on, before initializing the server, (e.g., call
validateIpv4Supported
), we'll have to initialize very early on theSocketInterfaceSingleton
to something.Do you mean to keep the global static registration in
socket_interface_impl.cc
(which is a bit of a misnomer at this point but that's for another time)? Or do you think we should move that toserver.cc
(and keep it as a global static)? I'm fine either way.As for the factory registration, here's the weird use case I came up with: people may want to configure a different default but may still want to configure addresses to use what we now call
SocketInterfaceImpl
, which is registered with nameenvoy.config.core.default_socket_interface
. So, we still end up needing the factory registration for name lookups, unless we decide we don't support that.Tying this to your first suggestion: we could maybe do a
SocketInterfaceSingleton::initialize(Network::socketInterface("envoy.config.core.default_socket_interface"))
very early on, e.g., somewhere inmain_common.cc
or inPlatformImpl()
and that would avoid the need for the static init and allocating a redundantSocketInterfaceImpl
unique ptr by reusing the factory registration.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.
Either
a. move initialization to
server.cc
and fix testb. keep global static registration without declaring factory
works for me.
In long term we might want a. but to keep PR smaller and easier to review we can do a. as a follow up.
I meant we should drop it until we can support it in a meaningful way? i.e. when address can specify socket interface.
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.
If it's fine with you, let's try to do a. as a follow up after we fix
Address
. If that's okay, I'll push a quick fix to the PR (SocketInterface
lookup needs some casting).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.
Not to forget about it, I pushed the fix now, but that's independent of the changes we've been discussing.
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.
sgtm
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.
Thanks! Let me know if anything else is needed (the update seems to have verified fine).