Skip to content

listener: keep ListenerFactoryContext small #7528

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

Merged
mattklein123 merged 2 commits into from
Jul 18, 2019
Merged

listener: keep ListenerFactoryContext small #7528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b89beaa
listener: keep ListenerFactoryContext small
lambdai Jul 10, 2019
ad7b62d
remove addListenSocketOption in mocks
lambdai Jul 11, 2019
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
7 changes: 0 additions & 7 deletions include/envoy/server/filter_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,13 +173,6 @@ class FactoryContext {

class ListenerFactoryContext : public virtual FactoryContext {
public:
/**
* Store socket options to be set on the listen socket before listening.
*/
virtual void addListenSocketOption(const Network::Socket::OptionConstSharedPtr& option) PURE;

virtual void addListenSocketOptions(const Network::Socket::OptionsSharedPtr& options) PURE;

/**
* Give access to the listener configuration
*/
Expand Down
17 changes: 9 additions & 8 deletions source/server/listener_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,14 +305,6 @@ class ListenerImpl : public Network::ListenerConfig,
std::make_shared<std::vector<Network::Socket::OptionConstSharedPtr>>();
}
}
void addListenSocketOption(const Network::Socket::OptionConstSharedPtr& option) override {
ensureSocketOptions();
listen_socket_options_->emplace_back(std::move(option));
}
void addListenSocketOptions(const Network::Socket::OptionsSharedPtr& options) override {
ensureSocketOptions();
Network::Socket::appendOptions(listen_socket_options_, options);
}
const Network::ListenerConfig& listenerConfig() const override { return *this; }
ProtobufMessage::ValidationVisitor& messageValidationVisitor() override {
return parent_.server_.messageValidationVisitor();
Expand All @@ -336,6 +328,15 @@ class ListenerImpl : public Network::ListenerConfig,
SystemTime last_updated_;

private:
void addListenSocketOption(const Network::Socket::OptionConstSharedPtr& option) {
ensureSocketOptions();
listen_socket_options_->emplace_back(std::move(option));
}
void addListenSocketOptions(const Network::Socket::OptionsSharedPtr& options) {
ensureSocketOptions();
Network::Socket::appendOptions(listen_socket_options_, options);
}

ListenerManagerImpl& parent_;
Network::Address::InstanceConstSharedPtr address_;
FilterChainManagerImpl filter_chain_manager_;
Expand Down
8 changes: 0 additions & 8 deletions test/mocks/server/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,14 +516,6 @@ class MockListenerFactoryContext : public MockFactoryContext, public ListenerFac
MockListenerFactoryContext();
~MockListenerFactoryContext() override;

void addListenSocketOption(const Network::Socket::OptionConstSharedPtr& option) override {
addListenSocketOption_(option);
}
MOCK_METHOD1(addListenSocketOption_, void(const Network::Socket::OptionConstSharedPtr&));
void addListenSocketOptions(const Network::Socket::OptionsSharedPtr& options) override {
addListenSocketOptions_(options);
}
MOCK_METHOD1(addListenSocketOptions_, void(const Network::Socket::OptionsSharedPtr&));
const Network::ListenerConfig& listenerConfig() const override { return _listenerConfig_; }
MOCK_CONST_METHOD0(listenerConfig_, const Network::ListenerConfig&());

Expand Down
142 changes: 2 additions & 140 deletions test/server/listener_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2686,30 +2686,12 @@ class OriginalDstTestFilter : public Extensions::ListenerFilters::OriginalDst::O
};

TEST_F(ListenerManagerImplWithRealFiltersTest, OriginalDstTestFilter) {
// Static scope required for the io_handle to be in scope for the lambda below
Copy link
Member

@mattklein123 mattklein123 Jul 11, 2019

Choose a reason for hiding this comment

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

I'm a little concerned that we are losing some test coverage that matters here. Are you positive that this is not being used by existing listener filters? I'm not convinced. Can you see who added these tests and ask them?

Copy link
Contributor Author

@lambdai lambdai Jul 11, 2019

Choose a reason for hiding this comment

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

I remove the two functions from ListenerFactoryContext and the build is successful. That's the proof that no listener filter in the code tree is using it. Unfortunately I can never verify if private code is using ... and that is the linked issue for

// and for the final check at the end of this test.
static int fd;
fd = -1;
// temporary io_handle to test result of socket creation
Network::IoHandlePtr io_handle_tmp = std::make_unique<Network::IoSocketHandleImpl>(0);
EXPECT_CALL(*listener_factory_.socket_, ioHandle()).WillOnce(ReturnRef(*io_handle_tmp));

class OriginalDstTestConfigFactory : public Configuration::NamedListenerFilterConfigFactory {
public:
// NamedListenerFilterConfigFactory
Network::ListenerFilterFactoryCb
createFilterFactoryFromProto(const Protobuf::Message&,
Configuration::ListenerFactoryContext& context) override {
auto option = std::make_unique<Network::MockSocketOption>();
EXPECT_CALL(*option, setOption(_, envoy::api::v2::core::SocketOption::STATE_PREBIND))
.WillOnce(Return(true));
EXPECT_CALL(*option, setOption(_, envoy::api::v2::core::SocketOption::STATE_BOUND))
.WillOnce(Invoke(
[](Network::Socket& socket, envoy::api::v2::core::SocketOption::SocketState) -> bool {
fd = socket.ioHandle().fd();
return true;
}));
context.addListenSocketOption(std::move(option));
Copy link
Contributor Author

@lambdai lambdai Jul 11, 2019

Choose a reason for hiding this comment

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

@mattklein123 The tricky test is verifying if createFilterFactoryFromProto is called but delegating it to ListenerFactoryContext::addListenSocketOption. This might be reasonable in the past but is not appropriate at this moment.

BTW I think

envoy/source/server/listener_manager_impl.cc

Lines 208 to 217 in 1ca1d13

if (config.has_transparent()) {
addListenSocketOptions(Network::SocketOptionFactory::buildIpTransparentOptions());
}
if (config.has_freebind()) {
addListenSocketOptions(Network::SocketOptionFactory::buildIpFreebindOptions());
}
if (!config.socket_options().empty()) {
addListenSocketOptions(
Network::SocketOptionFactory::buildLiteralOptions(config.socket_options()));
}
is why OriginalDstListenerFilter(as well as other ListenerFilters) no longer need addListenSocketOption in ListenerFactoryContext

Copy link
Member

@mattklein123 mattklein123 Jul 11, 2019

Choose a reason for hiding this comment

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

I'm still concerned that people may have private listener filters that are using this and won't be able to now. Can you see who added the tests that you changed and ask them?

Configuration::ListenerFactoryContext&) override {
return [](Network::ListenerFilterManager& filter_manager) -> void {
filter_manager.addAcceptFilter(std::make_unique<OriginalDstTestFilter>());
};
Expand Down Expand Up @@ -2767,57 +2749,6 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, OriginalDstTestFilter) {
EXPECT_TRUE(filterChainFactory.createListenerFilterChain(manager));
EXPECT_TRUE(socket.localAddressRestored());
EXPECT_EQ("127.0.0.2:2345", socket.localAddress()->asString());
EXPECT_NE(fd, -1);
io_handle_tmp->close();
}

TEST_F(ListenerManagerImplWithRealFiltersTest, OriginalDstTestFilterOptionFail) {
class OriginalDstTestConfigFactory : public Configuration::NamedListenerFilterConfigFactory {
public:
// NamedListenerFilterConfigFactory
Network::ListenerFilterFactoryCb
createFilterFactoryFromProto(const Protobuf::Message&,
Configuration::ListenerFactoryContext& context) override {
auto option = std::make_unique<Network::MockSocketOption>();
EXPECT_CALL(*option, setOption(_, envoy::api::v2::core::SocketOption::STATE_PREBIND))
.WillOnce(Return(false));
context.addListenSocketOption(std::move(option));
return [](Network::ListenerFilterManager& filter_manager) -> void {
filter_manager.addAcceptFilter(std::make_unique<OriginalDstTestFilter>());
};
}

ProtobufTypes::MessagePtr createEmptyConfigProto() override {
return std::make_unique<Envoy::ProtobufWkt::Empty>();
}

std::string name() override { return "testfail.listener.original_dst"; }
};

/**
* Static registration for the original dst filter. @see RegisterFactory.
*/
static Registry::RegisterFactory<OriginalDstTestConfigFactory,
Configuration::NamedListenerFilterConfigFactory>
registered_;

const std::string yaml = TestEnvironment::substitute(R"EOF(
name: "socketOptionFailListener"
address:
socket_address: { address: 127.0.0.1, port_value: 1111 }
filter_chains: {}
listener_filters:
- name: "testfail.listener.original_dst"
config: {}
)EOF",
Network::Address::IpVersion::v4);

EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true));

EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", true),
EnvoyException,
"MockListenerComponentFactory: Setting socket options failed");
EXPECT_EQ(0U, manager_->listeners().size());
}

class OriginalDstTestFilterIPv6
Expand All @@ -2829,30 +2760,12 @@ class OriginalDstTestFilterIPv6
};

TEST_F(ListenerManagerImplWithRealFiltersTest, OriginalDstTestFilterIPv6) {
// Static scope required for the io_handle to be in scope for the lambda below
// and for the final check at the end of this test.
static int fd;
fd = -1;
// temporary io_handle to test result of socket creation
Network::IoHandlePtr io_handle_tmp = std::make_unique<Network::IoSocketHandleImpl>(0);
EXPECT_CALL(*listener_factory_.socket_, ioHandle()).WillOnce(ReturnRef(*io_handle_tmp));

class OriginalDstTestConfigFactory : public Configuration::NamedListenerFilterConfigFactory {
public:
// NamedListenerFilterConfigFactory
Network::ListenerFilterFactoryCb
createFilterFactoryFromProto(const Protobuf::Message&,
Configuration::ListenerFactoryContext& context) override {
auto option = std::make_unique<Network::MockSocketOption>();
EXPECT_CALL(*option, setOption(_, envoy::api::v2::core::SocketOption::STATE_PREBIND))
.WillOnce(Return(true));
EXPECT_CALL(*option, setOption(_, envoy::api::v2::core::SocketOption::STATE_BOUND))
.WillOnce(Invoke(
[](Network::Socket& socket, envoy::api::v2::core::SocketOption::SocketState) -> bool {
fd = socket.ioHandle().fd();
return true;
}));
context.addListenSocketOption(std::move(option));
Configuration::ListenerFactoryContext&) override {
return [](Network::ListenerFilterManager& filter_manager) -> void {
filter_manager.addAcceptFilter(std::make_unique<OriginalDstTestFilterIPv6>());
};
Expand Down Expand Up @@ -2910,57 +2823,6 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, OriginalDstTestFilterIPv6) {
EXPECT_TRUE(filterChainFactory.createListenerFilterChain(manager));
EXPECT_TRUE(socket.localAddressRestored());
EXPECT_EQ("[1::2]:2345", socket.localAddress()->asString());
EXPECT_NE(fd, -1);
io_handle_tmp->close();
}

TEST_F(ListenerManagerImplWithRealFiltersTest, OriginalDstTestFilterOptionFailIPv6) {
class OriginalDstTestConfigFactory : public Configuration::NamedListenerFilterConfigFactory {
public:
// NamedListenerFilterConfigFactory
Network::ListenerFilterFactoryCb
createFilterFactoryFromProto(const Protobuf::Message&,
Configuration::ListenerFactoryContext& context) override {
auto option = std::make_unique<Network::MockSocketOption>();
EXPECT_CALL(*option, setOption(_, envoy::api::v2::core::SocketOption::STATE_PREBIND))
.WillOnce(Return(false));
context.addListenSocketOption(std::move(option));
return [](Network::ListenerFilterManager& filter_manager) -> void {
filter_manager.addAcceptFilter(std::make_unique<OriginalDstTestFilterIPv6>());
};
}

ProtobufTypes::MessagePtr createEmptyConfigProto() override {
return std::make_unique<Envoy::ProtobufWkt::Empty>();
}

std::string name() override { return "testfail.listener.original_dstipv6"; }
};

/**
* Static registration for the original dst filter. @see RegisterFactory.
*/
static Registry::RegisterFactory<OriginalDstTestConfigFactory,
Configuration::NamedListenerFilterConfigFactory>
registered_;

const std::string yaml = TestEnvironment::substitute(R"EOF(
name: "socketOptionFailListener"
address:
socket_address: { address: ::0001, port_value: 1111 }
filter_chains: {}
listener_filters:
- name: "testfail.listener.original_dstipv6"
config: {}
)EOF",
Network::Address::IpVersion::v6);

EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true));

EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", true),
EnvoyException,
"MockListenerComponentFactory: Setting socket options failed");
EXPECT_EQ(0U, manager_->listeners().size());
}

// Validate that when neither transparent nor freebind is not set in the
Expand Down