-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Listener: Add listener filters. #2346
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
Changes from 46 commits
2fb53f1
11766c4
ee60ccf
6874568
bc51bf8
0a8fd6f
da0c39b
044a089
d97257a
efdf2f1
31ea8a0
c6f4fb2
a7845db
f9c5650
d42ce02
aa81333
add8512
c32fcaa
818858a
228ac19
0703ecc
84854b2
ad6f413
d4b2140
597d05a
1ad7886
8bd512f
dca40d8
ab098f3
d0ad3d5
7f77eb0
4d72b93
07629cb
de46c3b
f047852
13cba72
7d82bbe
61da63f
3503579
b7e16d3
e75f50d
9a2c62a
94751c0
1021213
a390886
ae07e0f
393e2bc
f5c19a9
6b3e81d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,5 +34,65 @@ class ListenSocket { | |
| typedef std::unique_ptr<ListenSocket> ListenSocketPtr; | ||
| typedef std::shared_ptr<ListenSocket> ListenSocketSharedPtr; | ||
|
|
||
| /** | ||
| * A socket passed to a connection. For server connections this represents the accepted socket, and | ||
| * for client connections this represents the socket being connected to a remote address. | ||
| * | ||
| * TODO(jrajahalme): Hide internals (e.g., fd) from listener filters by providing callbacks filters | ||
| * may need (set/getsockopt(), peek(), recv(), etc.) | ||
| */ | ||
| class ConnectionSocket { | ||
| public: | ||
| virtual ~ConnectionSocket() {} | ||
|
|
||
| /** | ||
| * @return the local address of the socket. | ||
| */ | ||
| virtual const Address::InstanceConstSharedPtr& localAddress() const PURE; | ||
|
|
||
| /** | ||
| * @return the remote address of the socket. | ||
| */ | ||
| virtual const Address::InstanceConstSharedPtr& remoteAddress() const PURE; | ||
|
|
||
| /** | ||
| * Set the local address of the socket. On accepted sockets the local address defaults to the | ||
| * one at which the connection was received at, which is the same as the listener's address, if | ||
| * the listener is bound to a specific address. | ||
| * | ||
| * @param local_address the new local address. | ||
| * @param restored a flag marking the local address as being restored to a value that is | ||
| * different from the one the socket was initially accepted at. This should only be set | ||
| * to 'true' when restoring the original destination address of a connection redirected | ||
| * by iptables REDIRECT. The caller is responsible for making sure the new address is | ||
| * actually different when passing restored as 'true'. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also give an example of when someone would call this function but set
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reverted the meaning of this boolean and renamed it as "hand_off_restored_destinations", should be clearer now, see the new commit. |
||
| */ | ||
| virtual void setLocalAddress(const Address::InstanceConstSharedPtr& local_address, | ||
| bool restored = false) PURE; | ||
|
|
||
| /** | ||
| * Set the remote address of the socket. | ||
| */ | ||
| virtual void setRemoteAddress(const Address::InstanceConstSharedPtr& remote_address) PURE; | ||
|
|
||
| /** | ||
| * @return true if the local address has been restored to a value that is different from the | ||
| * address the socket was initially accepted at. | ||
| */ | ||
| virtual bool localAddressRestored() const PURE; | ||
|
|
||
| /** | ||
| * @return fd the socket's file descriptor. | ||
| */ | ||
| virtual int fd() const PURE; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that leaving
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine to leave
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a TODO. |
||
|
|
||
| /** | ||
| * Close the underlying socket. | ||
| */ | ||
| virtual void close() PURE; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need an explicit
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I'll remove the explicit close().
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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<ConnectionSocket> ConnectionSocketPtr; | ||
|
|
||
| } // namespace Network | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,39 +6,12 @@ | |
|
|
||
| #include "envoy/common/exception.h" | ||
| #include "envoy/network/connection.h" | ||
| #include "envoy/network/listen_socket.h" | ||
| #include "envoy/ssl/context.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}; | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * A configuration for an individual listener. | ||
| */ | ||
|
|
@@ -63,12 +36,6 @@ class ListenerConfig { | |
| */ | ||
| virtual Ssl::ServerContext* defaultSslContext() PURE; | ||
|
|
||
| /** | ||
| * @return bool whether to use the PROXY Protocol V1 | ||
| * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) | ||
| */ | ||
| virtual bool useProxyProto() PURE; | ||
|
|
||
| /** | ||
| * @return bool specifies whether the listener should actually listen on the port. | ||
| * A listener that doesn't listen on a port can only receive connections | ||
|
|
@@ -77,10 +44,12 @@ class ListenerConfig { | |
| virtual bool bindToPort() PURE; | ||
|
|
||
| /** | ||
| * @return bool if a connection was redirected to this listener address using iptables, | ||
| * allow the listener to hand it off to the listener associated to the original address | ||
| * @return bool if a connection should be handed off to another Listener after the original | ||
| * destination address has been restored. 'true' when 'use_original_dst' flag in listener | ||
| * configuration is set, false otherwise. Note that this flag will be removed from the v2 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you explicitly use the word "deprecated" in here somewhere so that when we search for deprecated things this comes up?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| * API. | ||
| */ | ||
| virtual bool useOriginalDst() PURE; | ||
| virtual bool handOffRestoredDestinations() const PURE; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be not plural?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A listener accepts many connections, each of which has a destination address, and the listener will either hand all restored destinations off to another listener (if found) or not. To me this calls for a plural, but I'm happy to change it you don't buy this line of reasoning.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK makes sense. I guess I would probably name it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I'll change this, hold on for a while :-) |
||
|
|
||
| /** | ||
| * @return uint32_t providing a soft limit on size of the listener's new connection read and write | ||
|
|
@@ -96,7 +65,7 @@ class ListenerConfig { | |
| /** | ||
| * @return uint64_t the tag the listener should use for connection handler tracking. | ||
| */ | ||
| virtual uint64_t listenerTag() PURE; | ||
| virtual uint64_t listenerTag() const PURE; | ||
|
|
||
| /** | ||
| * @return const std::string& the listener's name. | ||
|
|
@@ -111,6 +80,16 @@ class ListenerCallbacks { | |
| public: | ||
| virtual ~ListenerCallbacks() {} | ||
|
|
||
| /** | ||
| * Called when a new connection is accepted. | ||
| * @param socket supplies the socket that is moved into the callee. | ||
| * @param redirected is true when the socket was first accepted by another listener | ||
| * and is redirected to a new listener. The recipient should not redirect | ||
| * the socket any further. | ||
| */ | ||
| virtual void onAccept(ConnectionSocketPtr&& socket, | ||
| bool hand_off_restored_destinations = true) PURE; | ||
|
|
||
| /** | ||
| * Called when a new connection is accepted. | ||
| * @param new_connection supplies the new connection that is moved into the callee. | ||
|
|
||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...