listener: Add support for listener socket options. #2519
Conversation
Allow listener filters to set socket options to be set on the listening socket. Instead of replicating more of the Network::ConnectionSocket functionality in the Network::ListenSocket, Network::ListenSocket is renamed as Network::Socket and used as the base class of Network::ConnectionSocket. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
| remove(uds_path.c_str()); | ||
| local_address_.reset(new Address::PipeInstance(uds_path)); |
There was a problem hiding this comment.
In the old code this was done after remove and now it's done before. Does that matter? Or does remove only need to be done before bind potentially?
There was a problem hiding this comment.
PipeInstance constructor does not issue any syscalls, so IMO remove() just has to be done before bind().
| public: | ||
| ~ListenSocketImpl() { close(); } | ||
| class SocketImpl : public virtual Socket { | ||
| protected: |
There was a problem hiding this comment.
nit: protected goes below public.
| ASSERT(!socket_); | ||
| socket_ = socket; | ||
| // Server config validation sets nullptr sockets. | ||
| if (socket_ && listen_socket_options_) { | ||
| bool ok = listen_socket_options_->setOptions(*socket_); |
There was a problem hiding this comment.
Should this throw or fail in some way? This will also break NVLOG builds, so at minimum needs an UNREFERENCED_PARAMATER(ok);
There was a problem hiding this comment.
The callers of setSocket throw, so I'll add a throw here.
test/mocks/server/mocks.cc
Outdated
| @@ -137,6 +137,10 @@ MockTransportSocketFactoryContext::MockTransportSocketFactoryContext() {} | |||
|
|
|||
| MockTransportSocketFactoryContext::~MockTransportSocketFactoryContext() {} | |||
|
|
|||
| MockListenerFactoryContext::MockListenerFactoryContext() {} | |||
|
|
|||
There was a problem hiding this comment.
nit: del newline, same at 137
Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
| ENVOY_LOG(debug, "{}: Setting socket options {}", name_, ok ? "succeeded" : "failed"); | ||
| const std::string message = | ||
| fmt::format("{}: Setting socket options {}", name_, ok ? "succeeded" : "failed"); | ||
| if (!ok) { |
There was a problem hiding this comment.
Do you have test coverage for both of these cases?
There was a problem hiding this comment.
Maybe not, I'll make sure I do. Sorry to make you ask for coverage again...
Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
Allow listener filters to set socket options to be set on the
listening socket.
To be able to set the options at the right time in the listener life cycle,
a setListenSocketOptions() call is added to listener factory context.
Instead of replicating more of the Network::ConnectionSocket
functionality in the Network::ListenSocket, Network::ListenSocket is
renamed as Network::Socket and used as the base class of
Network::ConnectionSocket.
Signed-off-by: Jarno Rajahalme jarno@covalent.io
Risk Level: Low