Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
2fb53f1
listener_impl: Refactor for listener filters.
jrajahalme Aug 26, 2017
11766c4
listener: Add filter framework.
jrajahalme Aug 25, 2017
ee60ccf
Merge branch 'master' into listener-filters
jrajahalme Jan 11, 2018
6874568
Style fixes.
jrajahalme Jan 11, 2018
bc51bf8
test: Add missing 'override' keyword.
jrajahalme Jan 11, 2018
0a8fd6f
test: Remove unused 'this' lambda capture.
jrajahalme Jan 11, 2018
da0c39b
test: Fix format.
jrajahalme Jan 11, 2018
044a089
filter/original_dst: Increase log level.
jrajahalme Jan 12, 2018
d97257a
listener filters: Implement proto initializers.
jrajahalme Jan 12, 2018
efdf2f1
Merge branch 'master' into listener-filters
jrajahalme Jan 13, 2018
31ea8a0
Merge branch 'master' into listener-filters
jrajahalme Jan 13, 2018
c6f4fb2
test: Add Listener filter mocks.
jrajahalme Jan 13, 2018
a7845db
listener: Use ListenerFilter type, drop v1 support.
jrajahalme Jan 13, 2018
f9c5650
include: Fix comments.
jrajahalme Jan 13, 2018
d42ce02
dispatcher: Rename createConnection() as createServerConnection().
jrajahalme Jan 13, 2018
aa81333
Listener: Rename AcceptSocket class as AcceptedSocket
jrajahalme Jan 13, 2018
add8512
Review fixes.
jrajahalme Jan 13, 2018
c32fcaa
listener: Use unique_ptr for listener filters.
jrajahalme Jan 13, 2018
818858a
network: Use "Network" in function names for network filters.
jrajahalme Jan 13, 2018
228ac19
listener_manager_impl_test: Add OriginalDstFilter test to increase co…
jrajahalme Jan 13, 2018
0703ecc
connection_handler_impl: Fix doc comments.
jrajahalme Jan 13, 2018
84854b2
test: Fix format.
jrajahalme Jan 15, 2018
ad6f413
Merge branch 'master' into listener-filters
jrajahalme Jan 16, 2018
d4b2140
listen_socket_impl: Check if local address really changed.
jrajahalme Jan 15, 2018
597d05a
connection_handler: Check for local address reset after all listener …
jrajahalme Jan 15, 2018
1ad7886
listen_socket: Remove explicit close().
jrajahalme Jan 15, 2018
8bd512f
proxy_protocol: Do not reset addresses when "UNKNOWN"
jrajahalme Jan 15, 2018
dca40d8
listen_socket: Remove clearReset() member from AcceptedSocket
jrajahalme Jan 15, 2018
ab098f3
Merge branch 'master' into listener-filters
jrajahalme Jan 17, 2018
d0ad3d5
filter: Move listener filters to 'listener' subdirectory.
jrajahalme Jan 17, 2018
7f77eb0
filter: Move listener filters to 'Filter::Listener' namespace
jrajahalme Jan 17, 2018
4d72b93
connection_impl: Take ConnectionSocket as an argument.
jrajahalme Jan 17, 2018
07629cb
filter: Avoid using bare pointer.
jrajahalme Jan 17, 2018
de46c3b
Fix comments, add TODO.
jrajahalme Jan 17, 2018
f047852
Merge branch 'master' into listener-filters
jrajahalme Jan 17, 2018
13cba72
listen_socket: Add comment on ClientSocket.
jrajahalme Jan 17, 2018
7d82bbe
config_schemas: Revert remaining v1 changes for listener filters.
jrajahalme Jan 17, 2018
61da63f
Review fixes.
jrajahalme Jan 18, 2018
3503579
listener_manager: Translate legacy flags to listener filters.
jrajahalme Jan 18, 2018
b7e16d3
proxy_protocol: Reduce logging level.
jrajahalme Jan 19, 2018
e75f50d
connection_handler: Do not assert for listener filter completion.
jrajahalme Jan 20, 2018
9a2c62a
proxy_protocol: Release file event when done.
jrajahalme Jan 20, 2018
94751c0
proxy_protocol_test: Make tests more robust.
jrajahalme Jan 20, 2018
1021213
Merge branch 'master' into listener-filters
jrajahalme Jan 23, 2018
a390886
listener: Deprecate 'use_proxy_protocol' boolean from the v2 LDS API
jrajahalme Jan 23, 2018
ae07e0f
Review fixes.
jrajahalme Jan 23, 2018
393e2bc
Merge branch 'master' into listener-filters
jrajahalme Jan 23, 2018
f5c19a9
Last nits.
jrajahalme Jan 23, 2018
6b3e81d
Remame handOffRestoredDestinations() as handOffRestoredDestinationCon…
jrajahalme Jan 23, 2018
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
4 changes: 2 additions & 2 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ REPOSITORY_LOCATIONS = dict(
urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"],
),
envoy_api = dict(
commit = "90698c63882c4ed02414afd6b5dd080a0437b397",
remote = "https://github.com/envoyproxy/data-plane-api",
commit = "071ddd950fb391f5778799396b449254d228f764",
remote = "https://github.com/jrajahalme/envoy-api",
),
grpc_httpjson_transcoding = dict(
commit = "e4f58aa07b9002befa493a0a82e10f2e98b51fc6",
Expand Down
5 changes: 4 additions & 1 deletion configs/original-dst-cluster/proxy_config.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
{
"listeners": [{
"address": "tcp://0.0.0.0:10000",
"use_original_dst": true,
"listener_filters": [{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we please make this a v2 only feature? For v1 I would recommend just internally converting "use_original_dst" into a filter internally. Otherwise this is a breaking config change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Happy to make this v2 only, simplifies it a little as well. It was not, however, a breaking config change, I had used this to test v1 access to the listener filters.

"name": "original_dst",
"config": {}
}],
"filters": [{
"name": "http_connection_manager",
"config": {
Expand Down
32 changes: 13 additions & 19 deletions include/envoy/event/dispatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ class Dispatcher {
*/
virtual void clearDeferredDeleteList() PURE;

/**
* Create a server connection.
* @param accept_socket supplies a socket with an open file descriptor and connection metadata
* to use for the connection. Takes ownership of the accept_socket.
* @param ssl_ctx supplies the SSL context to use, if not nullptr.
* @return Network::ConnectionPtr a client connection that is owned by the caller.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"a server connection"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

*/
virtual Network::ConnectionPtr createConnection(Network::AcceptSocketPtr&& accept_socket,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would probably call this createServerConnection to match createClientConnection

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is no ServerConnection class, ClientConnection inherits from Connection, so from that viewpoint createConnection() is correct. Hosts also have a createConnection method, so createServerConnection makes grepping the source easier.

Ssl::Context* ssl_ctx) PURE;

/**
* Create a client connection.
* @param address supplies the address to connect to.
Expand Down Expand Up @@ -85,25 +95,9 @@ class Dispatcher {
* @param listener_options listener configuration options.
* @return Network::ListenerPtr a new listener that is owned by the caller.
*/
virtual Network::ListenerPtr
createListener(Network::ConnectionHandler& conn_handler, Network::ListenSocket& socket,
Network::ListenerCallbacks& cb, Stats::Scope& scope,
const Network::ListenerOptions& listener_options) PURE;

/**
* Create a listener on a specific port.
* @param conn_handler supplies the handler for connections received by the listener
* @param ssl_ctx supplies the SSL context to use.
* @param socket supplies the socket to listen on.
* @param cb supplies the callbacks to invoke for listener events.
* @param scope supplies the Stats::Scope to use.
* @param listener_options listener configuration options.
* @return Network::ListenerPtr a new listener that is owned by the caller.
*/
virtual Network::ListenerPtr
createSslListener(Network::ConnectionHandler& conn_handler, Ssl::ServerContext& ssl_ctx,
Network::ListenSocket& socket, Network::ListenerCallbacks& cb,
Stats::Scope& scope, const Network::ListenerOptions& listener_options) PURE;
virtual Network::ListenerPtr createListener(Network::ListenSocket& socket,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please fix doc comments above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, sorry for the sloppiness...

Network::ListenerCallbacks& cb,
bool bind_to_port) PURE;

/**
* Allocate a timer. @see Event::Timer for docs on how to use the timer.
Expand Down
1 change: 1 addition & 0 deletions include/envoy/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ envoy_cc_library(
envoy_cc_library(
name = "listener_interface",
hdrs = ["listener.h"],
deps = ["//include/envoy/network:listen_socket_interface"],
)

envoy_cc_library(
Expand Down
28 changes: 7 additions & 21 deletions include/envoy/network/connection_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
#include "envoy/ssl/context.h"

namespace Envoy {

namespace Server {
class Listener;
}
// XXX: Move to Server namespace?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah, from an abstraction level in general we try to make server code depend on common code, but not the other way around. Why was this necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Listener config is defined in the Server namespace (Server::Listener), and the ConnectionHandler's addListener() now takes it as an argument. I did not want to include server headers from here, so I used a bare class declaration (in the correct namespace) to circumvent the need for the include.

Could also see if the Listener config class could be moved to the common code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, let's just move Server::Listener (that interface only) into the Network namespace. That interface doesn't depend on anything else in Server. Please do that in a dedicated PR since it will be just code movement. We can review/merge that quickly and then rebase this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Network namespace already has a Listener (the abstract base class, so something needs to be renamed. How about renaming the Server::Listener as Network::ListenerConfig, as it is just that, "A configuration for an individual listener", as the comment says?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do this and the filter directory split tomorrow, hopefully. In the meanwhile I updated this PR so that you may continue the review as you please :-)

namespace Network {

/**
Expand All @@ -26,28 +31,9 @@ class ConnectionHandler {

/**
* Adds listener to the handler.
* @param factory supplies the configuration factory for new connections.
* @param socket supplies the already bound socket to listen on.
* @param scope supplies the stats scope to use for listener specific stats.
* @param listener_tag supplies an opaque tag that can be used to stop or remove the listener.
* @param listener_options listener configuration options.
*/
virtual void addListener(Network::FilterChainFactory& factory, Network::ListenSocket& socket,
Stats::Scope& scope, uint64_t listener_tag,
const Network::ListenerOptions& listener_options) PURE;

/**
* Adds listener to the handler.
* @param factory supplies the configuration factory for new connections.
* @param socket supplies the already bound socket to listen on.
* @param scope supplies the stats scope to use for listener specific stats.
* @param listener_tag supplies an opaque tag that can be used to stop or remove the listener.
* @param listener_options listener configuration options.
* @param config listener configuration options.
*/
virtual void addSslListener(Network::FilterChainFactory& factory, Ssl::ServerContext& ssl_ctx,
Network::ListenSocket& socket, Stats::Scope& scope,
uint64_t listener_tag,
const Network::ListenerOptions& listener_options) PURE;
virtual void addListener(Server::Listener& config) PURE;

/**
* Find a listener based on the provided listener address value.
Expand Down
70 changes: 70 additions & 0 deletions include/envoy/network/filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ namespace Envoy {
namespace Network {

class Connection;
class AcceptSocket;

/**
* Status codes returned by filters that can cause future filters to not get iterated to.
Expand Down Expand Up @@ -147,6 +148,67 @@ class FilterManager {
virtual bool initializeReadFilters() PURE;
};

/**
* Callbacks used by individual listener filter instances to communicate with the listener filter
* manager.
*/
class ListenerFilterCallbacks {
public:
virtual ~ListenerFilterCallbacks() {}

/**
* @return the AcceptSocket that owns this manager and the managed filters.
*/
virtual AcceptSocket& socket() PURE;

/**
* @return the Dispatcher for this manager.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"for this manager" doesn't really apply in filter context. I would reword.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This better: "@return the Dispatcher for issuing events."?

*/
virtual Event::Dispatcher& dispatcher() PURE;

/**
* If an filter stopped filter iteration by returning FilterStatus::StopIteration,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"a filter"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

* the filter should call continueFilterChain(true) when complete to continue the filter chain,
* or continueFilterChain(false) if the filter execution failed and the connection musrt be

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo "musrt"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

* closed.
* @param success boolean telling whether the filter execution was successful or not.
*/
virtual void continueFilterChain(bool success) PURE;
};

/**
* Listener Filter
*/
class ListenerFilter {
public:
virtual ~ListenerFilter() {}

/**
* Called when a new connection is accepted, but before a Connection is created.
* Filter chain iteration can be stopped if needed.
* @param cb the callbacks the filter instance can use to communicate with the filter chain.
* @return status used by the filter manager to manage further filter iteration.
*/
virtual FilterStatus onAccept(ListenerFilterCallbacks& cb) PURE;
};

typedef std::shared_ptr<ListenerFilter> ListenerFilterSharedPtr;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this can probably be unique_ptr. The reason the other filters are shared_ptr is because of read/write/both semantics. That doesn't apply here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will try this out.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was complicated a bit due to google mock not working with lvalue references. Worked around it by passing a raw pointer to addAcceptFilter() method.


/**
* Interface for filter callbacks and adding listener filters to a manager.
*/
class ListenerFilterManager {
public:
virtual ~ListenerFilterManager() {}

/**
* Add a filter to the listener. Filters are invoked in FIFO order (the filter added
* first is called first).
* @param filter supplies the filter being added.
*/
virtual void addAcceptFilter(ListenerFilterSharedPtr filter) PURE;
};

/**
* Creates a chain of network filters for a new connection.
*/
Expand All @@ -161,6 +223,14 @@ class FilterChainFactory {
* false, e.g. filter chain is empty.
*/
virtual bool createFilterChain(Connection& connection) PURE;

/**
* Called to create the listener filter chain.
* @param listener supplies the listener to create the chain on.
* @return true if filter chain was created successfully. Otherwise
* false.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: reflow comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not exactly sure what you mean here, but I moved the last word to the end of the previous line.

*/
virtual bool createFilterChain(ListenerFilterManager& listener) PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

createListnerFilterChain for this and createNetworkFilterChain for the one above, this shouldn't be an overloaded.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK.

};

} // namespace Network
Expand Down
60 changes: 60 additions & 0 deletions include/envoy/network/listen_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,65 @@ class ListenSocket {
typedef std::unique_ptr<ListenSocket> ListenSocketPtr;
typedef std::shared_ptr<ListenSocket> ListenSocketSharedPtr;

/**
* An abstract accept socket.
*/
class AcceptSocket {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I would call this AcceptedSocket. That's a little more natural for me. Please update related comments also.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

public:
virtual ~AcceptSocket() {}

/**
* @return the address that the socket was received at, or an original destination address if
* applicable.
*/
virtual Address::InstanceConstSharedPtr localAddress() const PURE;

/**
* Reset the original destination address of the socket to a different address than the one

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: remove "original".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, doing the same for resetRemoteAddress(), too.

* the socket was accepted at.
*/
virtual void resetLocalAddress(Address::InstanceConstSharedPtr& local_address) PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

const (Address::InstanceConstSharedPtr&

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


/**
* @return true if the local address has been reset.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I still really don't know what "reset" means in this context (as a casual reader). Can you add an example? WDYT about my previous suggestion of localAddressChanged()?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I must have missed that suggestion. The real semantics is that the original destination cluster needs to know if the local address was "restored to it's original value, as it was before redirection". The significance of this is that missing that distinction will cause the upstream connection be opened to the same local address the Listener was listening at leading to recursion of open connections until Envoy reaches the max number of open connections and dies. This also explains the difference between the resetLocalAddress() and setLocalAddress(). The latter is used for client connections, where the local address is fully known only after the socket is connected.

So, initially the local address is the one the listener received the new connection at, but it can be restored to the original destination address by the original dst listener filter.

Maybe it would be clearer to add a separate flag to the address change method to mark as the address as 'restored'? This way the semantics would be more explicit and likely easier to understand. I'll try this out.

*/
virtual bool localAddressReset() const PURE;

/**
* @return the address that the socket was received from, or an original source address if
* applicable.
*/
virtual Address::InstanceConstSharedPtr remoteAddress() const PURE;

/**
* Set the original source address of the socket
*/
virtual void resetRemoteAddress(Address::InstanceConstSharedPtr& remote_address) PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

const Address::InstanceConstSharedPtr&

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ditto.


/**
* @return fd the accept socket's file descriptor.
*/
virtual int fd() const PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Per @ggreenway, can we hide the fd from the filters and potentially just expose peek operations for now? I feel like fd can be an implementation detail and passed around as needed into the real connection when it is created. In general, we should heavily limit what filters can do on a non-const accepted socket. Can we try to limit this interface as much as possible, at least from the filter perspective? This might require a base interface just for use by filters.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note that PROXY protocol consumes bytes from the wire, so peek operation alone wouldn't work (but it's also needed).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, good point. We can have a read, etc. method also if needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I haven't looked at feasibility of this yet, but maybe it would be possible to factor out the buffering piece from the Connection object and have the listener filters use that as well?

I wanted to keep the changes in this PR minimal and towards that goal did not want to re-engineer the proxy protocol implementation any more than necessary. It could still be argued that implementing full data visibility for the listener filters should be an additional PR to follow rather than being merged in to this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Socket option access is needed by the original dst filter, so that should be added in as well. On the other hand I don't know what the transport socket PR will bring. Maybe keep these listener filters and the interface as they are now and reconsider after the transport socket work is in?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree that leaving fd() in this PR should be fine (unless @mattklein123 and/or @ggreenway have strong feelings against it), just please add TODO and hide the socket-level access in another PR (perhaps even after all other changes are merged?).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's fine to leave fd() in this PR, but per @PiotrSikora please add a TODO. We should also remove the fd transfer/move function from the public interface. That should be an internal implementation detail. Then in a followup we can remove fd() and replace with the few accessor functions we need.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a TODO.


/**
* Transfer ownership of the file descriptor to the caller, so that the underlying socket will
* not be closed on delete.
*/
virtual int takeFd() PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Per my other comment let's remove this from the exposed interface. I think we should be able to hide this as an implementation detail during listener processing (one option is to just store the AcceptedSocket and actually pass it, vs. the fd` to connections). If this is not easily possible can we add a TODO to think about cleaning this up? At the very least I don't want to pass this to filters, so we might need to have a split interface for internal/listener use and for filter use.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, did this, but it was a lot of work and I had to bring close() back in to the exposed interface. Had to split up AcceptedSocket into multiple classes as it felt wrong to use that for ClientConnection. Let me know if you'd rather revert the last commit and do it as a separate PR after this one.


/**
* Clear 'reset' state so that the socket can be used again with a new listener.
*/
virtual void clearReset() PURE;

/**
* Close the underlying socket.
*/
virtual void close() PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need an explicit close() method? Can this be handled in destruction? Even in the case of deferred deletion it shouldn't matter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, I'll remove the explicit close().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Had to reintroduce this in order to remove takeFd(), which was more offensive.

};

typedef std::unique_ptr<AcceptSocket> AcceptSocketPtr;
typedef std::shared_ptr<AcceptSocket> AcceptSocketSharedPtr;

} // namespace Network
} // namespace Envoy
35 changes: 7 additions & 28 deletions include/envoy/network/listener.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,45 +6,24 @@

#include "envoy/common/exception.h"
#include "envoy/network/connection.h"
#include "envoy/network/listen_socket.h"

namespace Envoy {
namespace Network {

/**
* Listener configurations options.
*/
struct ListenerOptions {
// Specifies if the listener should actually bind to the port. A listener that doesn't bind can
// only receive connections redirected from other listeners that set use_origin_dst_ to true.
bool bind_to_port_;
// Whether to use the PROXY Protocol V1
// (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt)
bool use_proxy_proto_;
// If a connection was redirected to this port using iptables, allow the listener to hand it off
// to the listener associated to the original port.
bool use_original_dst_;
// Soft limit on size of the listener's new connection read and write buffers.
uint32_t per_connection_buffer_limit_bytes_;

/**
* Factory for ListenerOptions with bind_to_port_ set.
* @return ListenerOptions object initialized with bind_to_port_ set.
*/
static ListenerOptions listenerOptionsWithBindToPort() {
return {.bind_to_port_ = true,
.use_proxy_proto_ = false,
.use_original_dst_ = false,
.per_connection_buffer_limit_bytes_ = 0};
}
};

/**
* Callbacks invoked by a listener.
*/
class ListenerCallbacks {
public:
virtual ~ListenerCallbacks() {}

/**
* Called when a new connection is accepted.
* @param socket supplies the socket that is moved into the callee.
*/
virtual void onAccept(AcceptSocketPtr&& socket) PURE;

/**
* Called when a new connection is accepted.
* @param new_connection supplies the new connection that is moved into the callee.
Expand Down
56 changes: 56 additions & 0 deletions include/envoy/server/filter_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,62 @@ class FactoryContext {
virtual Stats::Scope& listenerScope() PURE;
};

/**
* This function is used to wrap the creation of a listener filter chain for new sockets as they are
* created. Filter factories create the lambda at configuration initialization time, and then they
* are used at runtime.
* @param filter_manager supplies the filter manager for the listener to install filters to.
* Typically the function will install a single filter, but it's technically possibly to install
* more than one if desired.
*/
typedef std::function<void(Network::ListenerFilterManager& filter_manager)> ListenerFilterFactoryCb;

/**
* Implemented by each listener filter and registered via Registry::registerFactory()
* or the convenience class RegisterFactory.
*/
class NamedListenerFilterConfigFactory {
public:
virtual ~NamedListenerFilterConfigFactory() {}

/**
* Create a particular listener filter factory implementation. If the implementation is unable to
* produce a factory with the provided parameters, it should throw an EnvoyException in the case
* of general error or a Json::Exception if the json configuration is erroneous. The returned
* callback should always be initialized.
* @param config supplies the general json configuration for the filter
* @param context supplies the filter's context.
* @return ListenerFilterFactoryCb the factory creation function.
*/
virtual ListenerFilterFactoryCb createFilterFactory(const Json::Object& config,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we go v2 only, no necessary of this method

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, removed.

FactoryContext& context) PURE;

/**
* v2 variant of createFilterFactory(..), where filter configs are specified as proto. This may be
* optionally implemented today, but will in the future become compulsory once v1 is deprecated.
*/
virtual ListenerFilterFactoryCb createFilterFactoryFromProto(const Protobuf::Message& config,
FactoryContext& context) {
UNREFERENCED_PARAMETER(config);
UNREFERENCED_PARAMETER(context);
NOT_IMPLEMENTED;
}

/**
* @return ProtobufTypes::MessagePtr create empty config proto message for v2. The filter
* config, which arrives in an opaque google.protobuf.Struct message, will be converted to
* JSON and then parsed into this empty proto. Optional today, will be compulsory when v1
* is deprecated.
*/
virtual ProtobufTypes::MessagePtr createEmptyConfigProto() { return nullptr; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be a PURE method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, fixed!


/**
* @return std::string the identifying name for a particular implementation of a listener filter
* produced by the factory.
*/
virtual std::string name() PURE;
};

/**
* This function is used to wrap the creation of a network filter chain for new connections as
* they come in. Filter factories create the lambda at configuration initialization time, and then
Expand Down
Loading