-
Notifications
You must be signed in to change notification settings - Fork 5.4k
listener: add support for multiple filter chains. #3217
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 all commits
b24190c
e999693
104338a
50fceeb
f87d907
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 |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ | |
| #include <memory> | ||
|
|
||
| #include "envoy/buffer/buffer.h" | ||
| #include "envoy/network/listen_socket.h" | ||
| #include "envoy/network/transport_socket.h" | ||
| #include "envoy/upstream/host_description.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
@@ -232,6 +234,43 @@ class ListenerFilterManager { | |
| */ | ||
| typedef std::function<void(ListenerFilterManager& filter_manager)> ListenerFilterFactoryCb; | ||
|
|
||
| /** | ||
| * Interface representing a single filter chain. | ||
| */ | ||
| class FilterChain { | ||
| public: | ||
| virtual ~FilterChain() {} | ||
|
|
||
| /** | ||
|
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. You're copying two method from
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. To be honest, I don't see much value in exposing implementation details / member classes, since it makes it harder to make changes in the future. What would be benefit of returning
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. It is not exposing implementation details / member classes, the
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. Fair enough, changed. |
||
| * @return const TransportSocketFactory& a transport socket factory to be used by the new | ||
| * connection. | ||
| */ | ||
| virtual const TransportSocketFactory& transportSocketFactory() const PURE; | ||
|
|
||
| /** | ||
| * const std::vector<FilterFactoryCb>& a list of filters to be used by the new connection. | ||
| */ | ||
| virtual const std::vector<FilterFactoryCb>& networkFilterFactories() const PURE; | ||
| }; | ||
|
|
||
| typedef std::shared_ptr<FilterChain> FilterChainSharedPtr; | ||
|
|
||
| /** | ||
| * Interface for searching through configured filter chains. | ||
| */ | ||
| class FilterChainManager { | ||
| public: | ||
| virtual ~FilterChainManager() {} | ||
|
|
||
| /** | ||
| * Find filter chain that's matching metadata from the new connection. | ||
| * @param socket supplies connection metadata that's going to be used for the filter chain lookup. | ||
| * @return const FilterChain* filter chain to be used by the new connection, | ||
| * nullptr if no matching filter chain was found. | ||
| */ | ||
| virtual const FilterChain* findFilterChain(const ConnectionSocket& socket) const PURE; | ||
| }; | ||
|
|
||
| /** | ||
| * Creates a chain of network filters for a new connection. | ||
| */ | ||
|
|
@@ -242,10 +281,12 @@ class FilterChainFactory { | |
| /** | ||
| * Called to create the network filter chain. | ||
| * @param connection supplies the connection to create the chain on. | ||
| * @param filter_factories supplies a list of filter factories to create the chain from. | ||
| * @return true if filter chain was created successfully. Otherwise | ||
| * false, e.g. filter chain is empty. | ||
| */ | ||
| virtual bool createNetworkFilterChain(Connection& connection) PURE; | ||
| virtual bool createNetworkFilterChain(Connection& connection, | ||
| const std::vector<FilterFactoryCb>& filter_factories) PURE; | ||
|
|
||
| /** | ||
| * Called to create the listener filter chain. | ||
|
|
||
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.
What about when we have CIDR and SNI, what order do we resolve with? In general, it would be helpful to clarify where we want to go in the end state of the API for all the match criteria and precedence.
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.
I've been thinking about this a fair bit. I think the precedence should be configurable. I can think of a few ways that might be useful; I'm not sure yet what the right answer is.
One option is to let the user provide a list for the field precedence order. For instance, [SNI, source-ip, transport protocol]. Another possible matching type is the entire list of matches as an ordered list, with first-match-wins. I'm sure there's other options as well. I would be surprised if we can come up with a one-size-fits-all matching algorithm with this many potential criteria.
I think this can all be solved in a later PR.
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.
Sure, I can see this being a useful thing to add later. So, for now, as long as we have precedence for the fields that are actually implemented, and a TODO for the rest I'd be happy.
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.
As mentioned earlier (I think?), the destination IP would take precedence over SNI, over transport protocol. I didn't consider source IPs.
I could see having
filter_chain_match_precedence: ["server_names", "source_ranges", "transport_protocol"]as a way of controlling precedence, but it doesn't seem that this is something required by most of the use cases, so I'm going to let someone that has an use case for it work on this.As for the "end goal" docs - I'm under the impression that the API and docs document current state, and not future vision, so I don't think the "end goal" should be added here, especially since things change during implementation, and I wouldn't want to promise stuff that won't be eventually implemented.
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.
This isn't quite correct. The user facing API docs should reflect the current state. We do explicit hidden TODOs in various places to signal to developers and those interested in understanding future directions where we want to head, e.g.
envoy/api/envoy/config/accesslog/v2/als.proto
Line 13 in 6ae5612
Please do add appropriate TODOs in that style and preferably open an issue to track future work here. That's all that's being asked at this point.
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.
Added TODOs about few upcoming rules and the precedence we discussed here (I'll open separate issue to track it).