network: add socket interface factory and config option#11630
network: add socket interface factory and config option#11630lizan merged 16 commits intoenvoyproxy:masterfrom
Conversation
Add boostrap config option that allows startup injection of a custom SocketInterface. Signed-off-by: Florin Coras <fcoras@cisco.com>
|
WIP meant to help with #11618 |
| message SocketInterfaceConfig { | ||
| enum SocketInterfaceType { | ||
| // Default, OS dependent, implementation | ||
| DEFAULT = 0; |
There was a problem hiding this comment.
How do you envision this to be used? I.e. what other implementations would be defined in the public Envoy API? How is this different from using the Envoy::Network::SocketInterfaceLoader to override default SocketInterface?
There was a problem hiding this comment.
IMO we want this to be separate from bootstrap extensions, as there is only one of these, right? Or per @yanavlasov do we potentially need to allow multiple that are potentially looked up by the resolver?
I think my general feeling for this is that we should be specifying this by typed config, either directly via a bootstrap extension or as a discrete thing that is also specified in bootstrap?
There was a problem hiding this comment.
Given the conversation on #11618, I think that just having the ability to inject a different implementation (bootstrap or not) may be enough. The thing that we're missing is a way to signal to the custom SocketInterface that certain Address::Instances should be treated differently.
As for the best option to change the SocketInterface, I'll let you be the judge of it! :-)
My rather naive (and probably wrong) idea was to add to extensions "socket interface shims" that would link to actual implementations, have them imported in socket_interface_factory.h and added as options in socket_interface.proto.
There was a problem hiding this comment.
Should this be on the Address itself, similar to the resolver?
There was a problem hiding this comment.
@htuch, that's an interesting idea. My original plan was to have a way of overriding the default SocketInterface implementation with a startup conf* because I assumed we wanted only one such interface to be active at a time.
Are you thinking about having multiple SocketInterfaces in a FactoryRegistry and then, based on Address attributes, retrieve and ask the factory to generate an IoHandle (or a Socket if we later decide to merge the two)? It sounds good to me.
*side note: just did some testing and realized that bootstrap_extensions are loaded after the ListenerManager is initialized. So, this can't work as is.
There was a problem hiding this comment.
Yes, similar to how we deal with the resolver. One thing to be cautious of is proto bloat, since we may have a lot of addresses in an Envoy config, so having to repeat the opaque config for each extension in each address might be expensive (I think the existing resolver interface suffers from this). So, I actually think I'm more in favor of the tags that @ggreenway suggested; I see this as being somewhat similar to the transport socket matchers that are now used by clusters.
There was a problem hiding this comment.
I agree that this should be a typed config, similar to other extensions configuration. Otherwise you'd have to leak extension specific config into the core API.
I do not have a strong sense whether this should involve addresses or not. I can see someone adding iWARP support in Envoy and it might not involve addresses.
There was a problem hiding this comment.
Would then a hash_map/vector of Address::Tag objects (to be defined) in Address::InstanceBase that can be updated via Address::Instance apis be enough? Or do you think we need something more generic? (cc @ggreenway , @yanavlasov)
This would limit configurability but it should be pretty light weight. It does have the advantage that the resolver can easily pass opaque information to the SocketInterface implementation.
There was a problem hiding this comment.
@yanavlasov agreed and apologies, I somehow managed to miss your comment.
To summarize, I guess we now have two issues that need to be solved:
- configuration of the
SocketInterfaceimplementation(s). Should be a typed config. I'm probably confused here, but the original goal ofsocket_interfacewas to use it as a typed_config under bootstrap_extensions and to keep on adding types to the enum, afterDEFAULT, as we add more implementations to extensions. Were you thinking about something else? Even so, this doesn't work as a bootstrap extension because it's initialized afterListenerManager, i.e., after the addresses are bound. I guess we'll have to move it directly under boostrap. - passing of extra context between resolvers and the
SocketInterfaceviaAddress::Instances. My previous comment was only concerned with this.
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for tackling. I think before we land this we need to really clearly understand the path forward for how socket extensions will be loaded, where they need to be available from (referenced by resolver, etc.). Is it worth it to have a short gdoc that we can iterate on before we go further on this? In this doc we can also discuss the future of IoHandle vs. Socket, etc. There is a huge amount of debt here that @florincoras is cleaning up which is awesome, but at this point I think we should agree on what remains. Thank you!
/wait
| message SocketInterfaceConfig { | ||
| enum SocketInterfaceType { | ||
| // Default, OS dependent, implementation | ||
| DEFAULT = 0; |
There was a problem hiding this comment.
IMO we want this to be separate from bootstrap extensions, as there is only one of these, right? Or per @yanavlasov do we potentially need to allow multiple that are potentially looked up by the resolver?
I think my general feeling for this is that we should be specifying this by typed config, either directly via a bootstrap extension or as a discrete thing that is also specified in bootstrap?
|
I agree that the spec would be desirable before committing to making changes, just so that everyone is one the same page about upcoming changes. @florincoras would you be able to write one up? |
|
@yanavlasov, sure! Will share once I have something ready. |
|
@yanavlasov here is a first draft of the document. It's not yet complete but I wanted to get it out ASAP to maybe discuss it during the community meeting |
Signed-off-by: Florin Coras <fcoras@cisco.com>
- support loading multiple SocketInterface implementations that are bootstrap configurable - added singleton to be used to lookup SocketInterface implementations by name (as provided through bootstrap config) Signed-off-by: Florin Coras <fcoras@cisco.com>
| using SocketInterfacesMap = absl::flat_hash_map<std::string, SocketInterfacePtr>; | ||
| using SocketInterfacesSingleton = InjectableSingleton<SocketInterfacesMap>; | ||
|
|
||
| static inline const SocketInterface* socketInterface(std::string name) { |
There was a problem hiding this comment.
This is to be used to find the right SocketInterface implementation when Address::Instance has a non-empty socket_interface field.
There was a problem hiding this comment.
Doesn't the registry basically provide this? (type_url -> factory) I'm probably missing why we need to redo this type of stuff.
There was a problem hiding this comment.
The way I set this up, and maybe that's not what we want, is: typed_config is handed off to SocketInterfaceFactory registered via REGISTER_FACTORY (eg declaration above at line 27 is registered here), and looked up via typed_config.name(), which then creates a SocketInterface implementation that we add to the hash table stored in the SocketInterfacesSingleton. So this new hash table is for implementations instances as opposed to factories.
If there's a nicer way this could be done with type_url, point me to an example and I'll try to follow it (still have a lot of things to learn :-() .
There was a problem hiding this comment.
I will defer to @lizan and @kyessenov who have more experience with the factory, but I'm wondering if we could change it slightly so the registered factory just spits out sockets and then we would just look up the factory by type_url?
| repeated envoy.extensions.transport_sockets.tls.v3.Secret secrets = 3; | ||
|
|
||
| // Allows loading of multiple socket interfaces and configuration of the default one | ||
| core.v3.SocketInterfacesConfig socket_interfaces = 4; |
There was a problem hiding this comment.
Is the thinking that these will one day be dynamically discovered by an xDS? If not, you don't need to put them in static_resources, they can just live top-level.
There was a problem hiding this comment.
Done! Actually this was just me not knowing what I was doing, so thanks for the comment!
Another thing that's confusing is the ci/circleci:docs error. Should I add the new *.proto files to some list or is there something wrong with them?
| // Used to select the socket interface implementation to use | ||
| message SocketInterfacesConfig { | ||
| // Addition socket interfaces to initialize | ||
| repeated TypedExtensionConfig custom_types = 1; |
There was a problem hiding this comment.
Can you document how the name or type URL of these extensions is significant and used during resolution?
There was a problem hiding this comment.
Done. Let me know if it's not clear.
| repeated TypedExtensionConfig custom_types = 1; | ||
|
|
||
| // Override default socket interface | ||
| TypedExtensionConfig custom_default_type = 2; |
There was a problem hiding this comment.
Should this just be a string? I.e. the extension config is above, this is just specifying some entry in it.
mattklein123
left a comment
There was a problem hiding this comment.
Generally likes like it is on the right track. Thank you!
/wait
| repeated envoy.extensions.transport_sockets.tls.v3.Secret secrets = 3; | ||
|
|
||
| // Allows loading of multiple socket interfaces and configuration of the default one | ||
| core.v3.SocketInterfacesConfig socket_interfaces = 4; |
| using SocketInterfacesMap = absl::flat_hash_map<std::string, SocketInterfacePtr>; | ||
| using SocketInterfacesSingleton = InjectableSingleton<SocketInterfacesMap>; | ||
|
|
||
| static inline const SocketInterface* socketInterface(std::string name) { |
There was a problem hiding this comment.
Doesn't the registry basically provide this? (type_url -> factory) I'm probably missing why we need to redo this type of stuff.
source/server/server.cc
Outdated
| const auto& custom_sock = sock_interfaces.custom_default_type(); | ||
| loadSocketInterface(custom_sock, validation_ctx); | ||
| auto sock = const_cast<Network::SocketInterface*>(Network::socketInterface(custom_sock.name())); | ||
| Network::SocketInterfaceSingleton::initialize(sock); |
There was a problem hiding this comment.
In a perfect world we would move away from the scoped singleton pattern. Where do we need to access the interfaces(s)? Can we hang this off of Api perhaps?
There was a problem hiding this comment.
I know what you mean! :-) I wanted to do that instead of SocketInterfaceSingleton as well but then realized that we need to pass an Api instance down to SocketImpl through several layers of Socket derived classes (that's why I asked if we need that many types of sockets at one point).
After merging Socket and IoHandle, if we stop relying on Socket constructors and instead directly allocate Sockets that we wrap in TcpListenSocket, AcceptedSocketImpl and the likes, then this might be even easier. That is, we should be able to do something like api->socketInterface(name)->socket() in those classes or those that allocate them.
Would you like to try to do that with this patch or should we postpone it until we're done with the current set of changes/issues?
There was a problem hiding this comment.
Let's definitely tackle this in later changes. SGTM!
- move socket_interfaces to bootstrap top-level - document how custom socket interface names can be used to lookup implementations - custom default type as string Signed-off-by: Florin Coras <fcoras@cisco.com>
| repeated core.v3.TypedExtensionConfig bootstrap_extensions = 21; | ||
|
|
||
| // Optional loading of custom socket interfaces and configuration of the default one | ||
| core.v3.SocketInterfacesConfig socket_interfaces = 22; |
There was a problem hiding this comment.
I don't think you really need the new field, you should be able to roll this PR entirely into a bootstrap_extensions above? i.e. you should be able to register SocketInterfaceFactory as BootstrapExtensionFactory and load socket interface there.
There was a problem hiding this comment.
There was a problem hiding this comment.
That was the first version of the PR but unfortunately the bootstrap extensions are loaded after the
ListenerManagerImplis created and initialized, so after the listen sockets are allocated. If you think it's fine to move the bootstrap extensions init code to here we should be able to use them.
Yeah that would be fine. Since there is nothing else using bootstrap extension (there will be in wasm but not upstreamed here) now, just go ahead and move it ahead.
When I added bootstrap extensions I realized there will be needs for different initialization phase, so we might want add that option later.
There was a problem hiding this comment.
Done! If you can, do check if I'm using bootstrap extensions the way you expect them to be used. Also, as per @mattklein123's comment, if you know of any way that we could simplify/avoid the building of a hash map in the SocketInterfacesSingleton, we could use it to simplify the current logic. That is, config handed to factory registered with REGISTER_FACTORY which creates and registers SocketInterface instances in the SocketInterfacesSingleton.
There was a problem hiding this comment.
@mattklein123 is the latest update closer to what you are thinking?
There was a problem hiding this comment.
@florincoras IIUC, what you want to:
- register multiple SocketInterfaceFactories with config (via bootstrap extensions)
- specify a default socket interface in bootstrap.proto
- override socket interface to use in addresses.
Is that right? If so you should be able to use the bootstrap extension name to lookup the factory instead of using a flat hash map?
There was a problem hiding this comment.
@lizan In this PR, only the first 2, the 3rd will involve changes to addresses, so it'll be a separate PR.
With the latest change, the SocketInterface implementations also derive BootstrapExtensionFactory so the lookup is done like this. And here is an example of the changes needed for a SocketInterface implementation.
There was a problem hiding this comment.
@lizan, if you get a chance to take a look at the latest commit, do let me know if the approach seems okay to you.
There was a problem hiding this comment.
Looks it is in right direction. Yeah I agree 3 shouldn't be in this PR, but just to understand the context.
Signed-off-by: Florin Coras <fcoras@cisco.com>
Signed-off-by: Florin Coras <fcoras@cisco.com>
Signed-off-by: Florin Coras <fcoras@cisco.com>
|
|
||
| REGISTER_FACTORY(SocketInterfaceImpl, Server::Configuration::BootstrapExtensionFactory); | ||
|
|
||
| static SocketInterfaceLoader* socket_interface_ = |
There was a problem hiding this comment.
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.
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, like test/test_common/environment.cc?
There was a problem hiding this comment.
Turns out integration:hotrestart_test, integration:run_envoy_test and test:router_tool_test still fail :-(
There was a problem hiding this comment.
Should we maybe statically initialize the singleton to the default
SocketInterfacein some common test file, liketest/test_common/environment.cc?
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.
Should we maybe statically initialize the singleton to the default
SocketInterfacein some common test file, liketest/test_common/environment.cc?Yeah I think that's good way to do test.
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 the SocketInterfaceSingleton to something.
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?
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 to server.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 name envoy.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 in main_common.cc or in PlatformImpl() and that would avoid the need for the static init and allocating a redundant SocketInterfaceImpl unique ptr by reusing the factory registration.
There was a problem hiding this comment.
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.
Either
a. move initialization to server.cc and fix test
b. 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.
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.
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.
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.
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.
Thanks! Let me know if anything else is needed (the update seems to have verified fine).
Signed-off-by: Florin Coras <fcoras@cisco.com>
dce30bb to
43805c8
Compare
Signed-off-by: Florin Coras <fcoras@cisco.com>
Signed-off-by: Florin Coras <fcoras@cisco.com>
|
/lgtm api |
|
Thanks @htuch! @mattklein123 @lizan @yanavlasov do let me know if you think this needs additional changes. Linux-x64 tsan is failing because of this. Do we need to wait for that to be fixed? |
|
The TSAN is a known false positive. @lizan @yanavlasov @mattklein123 API LGTM, will defer the rest to you folks. |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks in general this looks great. Can you please add a test for this functionality? One option would be a small integration test with a fake socket that just proxies through to the default socket. Thank you!
/wait
| // implemented by all factories that derive SocketInterfaceBase | ||
| class SocketInterfaceExtension : public Server::BootstrapExtension { | ||
| public: | ||
| SocketInterfaceExtension(SocketInterface* sock_interface) : sock_interface_(sock_interface) {} |
There was a problem hiding this comment.
nit: take/return/store references here as none of these can be null.
- use SocketInterface references in SocketInterfaceExtension - add basic integration test Signed-off-by: Florin Coras <fcoras@cisco.com>
Signed-off-by: Florin Coras <fcoras@cisco.com>
Added a basic integration test. Not sure if it entirely captures what you expected so do nitpick if missed the point. |
Signed-off-by: Florin Coras <fcoras@cisco.com>
mattklein123
left a comment
There was a problem hiding this comment.
LGTM with a small comment. Will defer to @lizan and @yanavlasov for further review. Thank you!
|
|
||
| TEST_P(SocketInterfaceIntegrationTest, Basic) { | ||
| BaseIntegrationTest::initialize(); | ||
| const Network::SocketInterface* factory = Network::socketInterface( |
There was a problem hiding this comment.
Wouldn't this be the case even if the registration didn't work? I think what I'm proposing is to define a shim extension in this integration test, and just internally have it return the default implementation, then run some very basic echo like integration test.
There was a problem hiding this comment.
At this point no. We have a static global initialization of the default SocketInterface done via SocketInterfaceLoader, a ScopedInjectableLoader. In this test, that is overridden by the config() above which initializes the default interface as a bootstrap extension and uses it to reinitialize the SocketInterfaceSingleton (done in the server init phase). We did discuss with @lizan the option to completely remove the static global initialization, but we'll have to update some tests to support that, as not all of them initialize the server. So that's now on the ever growing todo list :-) and maybe can be bundled together with the move of the SocketInterfaceSingleton to the Api (if that's eventually possible).
As for the test, this does start a listener through the bootstrap initialized SocketInterface. I guess we could add something like the Hello echo test from echo_integration_test.cc, or were you thinking about something more complex?
There was a problem hiding this comment.
OK fair enough. Yeah it would be good to get rid of the scoped singleton if possible but agreed that can be done later.
I guess we could add something like the Hello echo test from echo_integration_test.cc, or were you thinking about something more complex?
Yes exactly this would be fine.
Signed-off-by: Florin Coras <fcoras@cisco.com>
|
|
…1630) Add bootstrap config option that allows startup injection of a custom SocketInterface. Risk Level: Low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Florin Coras <fcoras@cisco.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
…1630) Add bootstrap config option that allows startup injection of a custom SocketInterface. Risk Level: Low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Florin Coras <fcoras@cisco.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>
Add bootstrap config option that allows startup injection of a custom
SocketInterface.
Signed-off-by: Florin Coras fcoras@cisco.com
Risk Level: Low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a