Skip to content
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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Jul 17, 2020
Merged

network: add socket interface factory and config option #11630

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
388d1f9
network: add socket interface factory
florincoras Jun 18, 2020
c241e44
Merge branch 'master' into sock_factory
florincoras Jun 26, 2020
cc4d075
updates that try to reflect community conversations
florincoras Jun 27, 2020
d77b0a9
address comments
florincoras Jun 29, 2020
fdd0e2f
moving back to bootstrap extensions
florincoras Jun 30, 2020
1ca8153
Merge branch 'master' into sock_factory
florincoras Jul 1, 2020
6c34d86
use FactoryRegistry for SocketInterface lookup
florincoras Jul 1, 2020
0f300c8
fix protodoc-title
florincoras Jul 1, 2020
3612cfa
Merge branch 'master' into sock_factory
florincoras Jul 2, 2020
af294af
fix socket interface lookup
florincoras Jul 8, 2020
43805c8
move default_socket_interface.proto to extensions
florincoras Jul 10, 2020
a02aa41
Merge branch 'master' into sock_factory
florincoras Jul 12, 2020
b622854
address comments
florincoras Jul 14, 2020
bd1a63d
Merge branch 'master' into sock_factory
florincoras Jul 14, 2020
f9cc098
disable integration test on windows
florincoras Jul 15, 2020
97368af
add simple echo test
florincoras Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions api/envoy/config/core/v3/socket_interface.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
syntax = "proto3";

package envoy.config.core.v3;

import "udpa/annotations/status.proto";
import "udpa/annotations/versioning.proto";
import "validate/validate.proto";

option java_package = "io.envoyproxy.envoy.config.core.v3";
option java_outer_classname = "SocketInterfaceProto";
option java_multiple_files = true;
option (udpa.annotations.file_status).package_version_status = ACTIVE;

// [#protodoc-title: Socket Interface Configuration ]

// Used to select the socket interface implementation to use
message SocketInterfaceConfig {
enum SocketInterfaceType {
// Default, OS dependent, implementation
DEFAULT = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@florincoras florincoras Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be on the Address itself, similar to the resolver?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@florincoras florincoras Jun 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 SocketInterface implementation(s). Should be a typed config. I'm probably confused here, but the original goal of socket_interface was to use it as a typed_config under bootstrap_extensions and to keep on adding types to the enum, after DEFAULT, 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 after ListenerManager, 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 SocketInterface via Address::Instances. My previous comment was only concerned with this.

}

// Implementation to use
SocketInterfaceType type = 1 [(validate.rules).enum = {defined_only: true}];
}
28 changes: 28 additions & 0 deletions api/envoy/config/core/v4alpha/socket_interface.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 25 additions & 0 deletions generated_api_shadow/envoy/config/core/v3/socket_interface.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions generated_api_shadow/envoy/config/core/v4alpha/socket_interface.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions source/common/network/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,18 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "socket_interface_factory_lib",
srcs = ["socket_interface_factory.cc"],
hdrs = ["socket_interface_factory.h"],
deps = [
":socket_lib",
"//include/envoy/server:bootstrap_extension_config_interface",
"//source/extensions/network/vcl:vcl_interface_lib",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)

envoy_cc_library(
name = "listen_socket_lib",
srcs = ["listen_socket_impl.cc"],
Expand Down
34 changes: 34 additions & 0 deletions source/common/network/socket_interface_factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#include "common/network/socket_interface_factory.h"

#include "envoy/config/core/v3/socket_interface.pb.h"
#include "envoy/registry/registry.h"

#include "common/network/socket_interface_impl.h"

namespace Envoy {
namespace Network {

class SocketInterfaceExtension : public Server::BootstrapExtension {};

Server::BootstrapExtensionPtr
SocketInterfaceFactory::createBootstrapExtension(const Protobuf::Message& config,
Server::Configuration::ServerFactoryContext&) {
const auto* proto = dynamic_cast<const envoy::config::core::v3::SocketInterfaceConfig*>(&config);
switch (proto->type()) {
case envoy::config::core::v3::SocketInterfaceConfig::DEFAULT:
// no need to clear and re-initialize the SocketInterfaceSingleton
break;
default:
break;
}
return std::make_unique<SocketInterfaceExtension>();
}

ProtobufTypes::MessagePtr SocketInterfaceFactory::createEmptyConfigProto() {
return std::make_unique<envoy::config::core::v3::SocketInterfaceConfig>();
}

REGISTER_FACTORY(SocketInterfaceFactory, Server::Configuration::BootstrapExtensionFactory);

} // namespace Network
} // namespace Envoy
18 changes: 18 additions & 0 deletions source/common/network/socket_interface_factory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#pragma once

#include "envoy/server/bootstrap_extension_config.h"

namespace Envoy {
namespace Network {

class SocketInterfaceFactory : public Server::Configuration::BootstrapExtensionFactory {
public:
Server::BootstrapExtensionPtr
createBootstrapExtension(const Protobuf::Message& config,
Server::Configuration::ServerFactoryContext& context) override;
ProtobufTypes::MessagePtr createEmptyConfigProto() override;
std::string name() const override { return "envoy.bootstrap.socket_interface"; }
};

} // namespace Network
} // namespace Envoy