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.

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
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
1 change: 1 addition & 0 deletions api/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ proto_library(
"//envoy/extensions/internal_redirect/allow_listed_routes/v3:pkg",
"//envoy/extensions/internal_redirect/previous_routes/v3:pkg",
"//envoy/extensions/internal_redirect/safe_cross_scheme/v3:pkg",
"//envoy/extensions/network/socket_interface/v3:pkg",
"//envoy/extensions/retry/host/omit_host_metadata/v3:pkg",
"//envoy/extensions/retry/priority/previous_priorities/v3:pkg",
"//envoy/extensions/transport_sockets/alts/v3:pkg",
Expand Down
6 changes: 5 additions & 1 deletion api/envoy/config/bootstrap/v3/bootstrap.proto
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// <config_overview_bootstrap>` for more detail.

// Bootstrap :ref:`configuration overview <config_overview_bootstrap>`.
// [#next-free-field: 24]
// [#next-free-field: 25]
message Bootstrap {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.bootstrap.v2.Bootstrap";
Expand Down Expand Up @@ -222,6 +222,10 @@ message Bootstrap {
// other resolution fails.
// [#not-implemented-hide:]
core.v3.ConfigSource default_config_source = 23;

// Optional overriding of default socket interface. The value must be the name of one of the
// socket interface factories initialized through a bootstrap extension
string default_socket_interface = 24;
}

// Administration interface :ref:`operations documentation
Expand Down
6 changes: 5 additions & 1 deletion api/envoy/config/bootstrap/v4alpha/bootstrap.proto

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

9 changes: 9 additions & 0 deletions api/envoy/extensions/network/socket_interface/v3/BUILD
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 {
}
1 change: 1 addition & 0 deletions api/versioning/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ proto_library(
"//envoy/extensions/internal_redirect/allow_listed_routes/v3:pkg",
"//envoy/extensions/internal_redirect/previous_routes/v3:pkg",
"//envoy/extensions/internal_redirect/safe_cross_scheme/v3:pkg",
"//envoy/extensions/network/socket_interface/v3:pkg",
"//envoy/extensions/retry/host/omit_host_metadata/v3:pkg",
"//envoy/extensions/retry/priority/previous_priorities/v3:pkg",
"//envoy/extensions/transport_sockets/alts/v3:pkg",
Expand Down
1 change: 1 addition & 0 deletions docs/root/api-v3/common_messages/common_messages.rst
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ Common messages
../config/core/v3/substitution_format_string.proto
../extensions/common/ratelimit/v3/ratelimit.proto
../extensions/filters/common/fault/v3/fault.proto
../extensions/network/socket_interface/v3/default_socket_interface.proto

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.

1 change: 1 addition & 0 deletions include/envoy/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ envoy_cc_library(
deps = [
":address_interface",
":io_handle_interface",
"//include/envoy/config:typed_config_interface",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)
Expand Down
2 changes: 2 additions & 0 deletions include/envoy/network/socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -271,5 +271,7 @@ class SocketInterface {
virtual bool ipFamilySupported(int domain) PURE;
};

using SocketInterfacePtr = std::unique_ptr<SocketInterface>;

} // namespace Network
} // namespace Envoy
16 changes: 14 additions & 2 deletions source/common/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ envoy_cc_library(
hdrs = [
"address_impl.h",
"io_socket_handle_impl.h",
"socket_interface_impl.h",
],
deps = [
":io_socket_error_lib",
":socket_interface_lib",
"//include/envoy/buffer:buffer_interface",
"//include/envoy/network:address_interface",
"//include/envoy/network:io_handle_interface",
Expand Down Expand Up @@ -172,6 +172,17 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "socket_interface_lib",
hdrs = ["socket_interface.h"],
deps = [
"//include/envoy/config:typed_config_interface",
"//include/envoy/network:socket_interface",
"//include/envoy/registry",
"//include/envoy/server:bootstrap_extension_config_interface",
],
)

envoy_cc_library(
name = "socket_lib",
srcs = [
Expand All @@ -184,10 +195,11 @@ envoy_cc_library(
],
deps = [
":address_lib",
":socket_interface_lib",
"//include/envoy/network:socket_interface",
"//source/common/common:assert_lib",
"//source/common/common:utility_lib",
"//source/common/singleton:threadsafe_singleton",
"@envoy_api//envoy/extensions/network/socket_interface/v3:pkg_cc_proto",
],
)

Expand Down
2 changes: 1 addition & 1 deletion source/common/network/address_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include "common/common/assert.h"
#include "common/common/fmt.h"
#include "common/common/utility.h"
#include "common/network/socket_interface_impl.h"
#include "common/network/socket_interface.h"

namespace Envoy {
namespace Network {
Expand Down
56 changes: 56 additions & 0 deletions source/common/network/socket_interface.h
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
14 changes: 14 additions & 0 deletions source/common/network/socket_interface_impl.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "common/network/socket_interface_impl.h"

#include "envoy/common/exception.h"
#include "envoy/extensions/network/socket_interface/v3/default_socket_interface.pb.h"
#include "envoy/network/socket.h"

#include "common/api/os_sys_calls_impl.h"
Expand Down Expand Up @@ -81,6 +82,19 @@ bool SocketInterfaceImpl::ipFamilySupported(int domain) {
return SOCKET_VALID(result.rc_);
}

Server::BootstrapExtensionPtr
SocketInterfaceImpl::createBootstrapExtension(const Protobuf::Message&,
Server::Configuration::ServerFactoryContext&) {
return std::make_unique<SocketInterfaceExtension>(*this);
}

ProtobufTypes::MessagePtr SocketInterfaceImpl::createEmptyConfigProto() {
return std::make_unique<
envoy::extensions::network::socket_interface::v3::DefaultSocketInterface>();
}

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

static SocketInterfaceLoader* socket_interface_ =
Copy link
Member

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?

Copy link
Member Author

@florincoras florincoras Jul 8, 2020

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, like test/test_common/environment.cc?

Copy link
Member Author

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 :-(

Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe statically initialize the singleton to the default SocketInterface in some common test file, like test/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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we maybe statically initialize the singleton to the default SocketInterface in some common test file, like test/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.

Copy link
Member

@lizan lizan Jul 9, 2020

Choose a reason for hiding this comment

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

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.

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

I meant we should drop it until we can support it in a meaningful way? i.e. when address can specify socket interface.

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

Copy link
Member Author

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

new SocketInterfaceLoader(std::make_unique<SocketInterfaceImpl>());

Expand Down
17 changes: 13 additions & 4 deletions source/common/network/socket_interface_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,31 @@
#include "envoy/network/address.h"
#include "envoy/network/socket.h"

#include "common/singleton/threadsafe_singleton.h"
#include "common/network/socket_interface.h"

namespace Envoy {
namespace Network {

class SocketInterfaceImpl : public SocketInterface {
class SocketInterfaceImpl : public SocketInterfaceBase {
public:
// SocketInterface
IoHandlePtr socket(Socket::Type socket_type, Address::Type addr_type,
Address::IpVersion version) override;
IoHandlePtr socket(Socket::Type socket_type, const Address::InstanceConstSharedPtr addr) override;
IoHandlePtr socket(os_fd_t fd) override;
bool ipFamilySupported(int domain) override;

// Server::Configuration::BootstrapExtensionFactory
Server::BootstrapExtensionPtr
createBootstrapExtension(const Protobuf::Message& config,
Server::Configuration::ServerFactoryContext& context) override;
ProtobufTypes::MessagePtr createEmptyConfigProto() override;
std::string name() const override {
return "envoy.extensions.network.socket_interface.default_socket_interface";
};
};

using SocketInterfaceSingleton = InjectableSingleton<SocketInterface>;
using SocketInterfaceLoader = ScopedInjectableLoader<SocketInterface>;
DECLARE_FACTORY(SocketInterfaceImpl);

} // namespace Network
} // namespace Envoy
1 change: 1 addition & 0 deletions source/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ envoy_cc_library(
"//source/common/config:runtime_utility_lib",
"//source/common/config:utility_lib",
"//source/common/network:resolver_lib",
"//source/common/network:socket_interface_lib",
"//source/common/network:socket_option_factory_lib",
"//source/common/network:utility_lib",
"//source/common/protobuf:utility_lib",
Expand Down
Loading