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 3 commits
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
6 changes: 3 additions & 3 deletions source/common/network/socket_interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ namespace Network {
// 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_; }
SocketInterfaceExtension(SocketInterface& sock_interface) : sock_interface_(sock_interface) {}
SocketInterface& socketInterface() { return sock_interface_; }

private:
SocketInterface* sock_interface_;
SocketInterface& sock_interface_;
};

// Class to be derived by all SocketInterface implementations.
Expand Down
2 changes: 1 addition & 1 deletion source/common/network/socket_interface_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ bool SocketInterfaceImpl::ipFamilySupported(int domain) {
Server::BootstrapExtensionPtr
SocketInterfaceImpl::createBootstrapExtension(const Protobuf::Message&,
Server::Configuration::ServerFactoryContext&) {
return std::make_unique<SocketInterfaceExtension>(this);
return std::make_unique<SocketInterfaceExtension>(*this);
}

ProtobufTypes::MessagePtr SocketInterfaceImpl::createEmptyConfigProto() {
Expand Down
10 changes: 10 additions & 0 deletions test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,16 @@ envoy_cc_test(
],
)

envoy_cc_test(
name = "socket_interface_integration_test",
srcs = ["socket_interface_integration_test.cc"],
tags = ["fails_on_windows"],
deps = [
":http_integration_lib",
"//source/common/network:socket_interface_lib",
],
)

envoy_cc_test(
name = "stats_integration_test",
srcs = ["stats_integration_test.cc"],
Expand Down
43 changes: 43 additions & 0 deletions test/integration/socket_interface_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#include "common/network/socket_interface.h"

#include "test/integration/integration.h"
#include "test/test_common/environment.h"
#include "test/test_common/utility.h"

#include "gtest/gtest.h"

namespace Envoy {
namespace {

class SocketInterfaceIntegrationTest : public BaseIntegrationTest,
public testing::TestWithParam<Network::Address::IpVersion> {
public:
SocketInterfaceIntegrationTest() : BaseIntegrationTest(GetParam(), config()) {
use_lds_ = false;
};

static std::string config() {
// At least one empty filter chain needs to be specified.
return absl::StrCat(ConfigHelper::httpProxyConfig(), R"EOF(
bootstrap_extensions:
- name: envoy.extensions.network.socket_interface.default_socket_interface
typed_config:
"@type": type.googleapis.com/envoy.extensions.network.socket_interface.v3.DefaultSocketInterface
default_socket_interface: "envoy.extensions.network.socket_interface.default_socket_interface"
)EOF");
}
};

INSTANTIATE_TEST_SUITE_P(IpVersions, SocketInterfaceIntegrationTest,
testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
TestUtility::ipTestParamsToString);

TEST_P(SocketInterfaceIntegrationTest, Basic) {
BaseIntegrationTest::initialize();
const Network::SocketInterface* factory = Network::socketInterface(
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@florincoras florincoras Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect! Done!

"envoy.extensions.network.socket_interface.default_socket_interface");
ASSERT_TRUE(Network::SocketInterfaceSingleton::getExisting() == factory);
}

} // namespace
} // namespace Envoy