lds: Add listener_filter_chain. by jrajahalme · Pull Request #400 · envoyproxy/data-plane-api

jrajahalme · 2018-01-06T01:11:25Z

Add listener_filter_chain to Listener, which is a list of individual
listener filters that make up the filter chain for sockets accepted
with the listener. These filters are run before any in the
'filter_chains', and these filters have the opportunity to manipulate
and augment the connection metadata that is used in connection filter
chain matching. Order matters as the filters are processed
sequentially right after a socket has been accepted by the listener,
and before a connection is created.

As an example the functionality controlled by Listener options
'use_original_dst' and 'use_proxy_proto' can be implemented as
listener filters instead of being hard-wired into Envoy's listener
logic. This makes also extensions (like proxy protocol v2) easier to
implement and deploy.

This implements the API part of the Envoy issue envoyproxy/envoy#1276

Signed-off-by: Jarno Rajahalme jarno@covalent.io

Add listener_filter_chain to Listener, which is a list of individual listener filters that make up the filter chain for sockets accepted with the listener. These filters are run before any in the 'filter_chains', and these filters have the opportunity to manipulate and augment the connection metadata that is used in connection filter chain matching. Order matters as the filters are processed sequentially right after a socket has been accepted by the listener, and before a connection is created. As an example the functionality controlled by Listener options 'use_original_dst' and 'use_proxy_proto' can be implemented as listener filters instead of being hard-wired into Envoy's listener logic. This makes also extensions (like proxy protocol v2) easier to implement and deploy. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-01-06T01:14:54Z

I have the corresponding Envoy side code done and tested on Envoy master as of 11-30-2017, will rebase and post after this is in.

Remove over-enthusiastic spaces, re-fill comment. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 · 2018-01-06T01:31:07Z

@jrajahalme thanks for this. I think in this case it would be worth it to post the PR for the code change at the same time so that we can review this with some context. I would just rebase your code change and the post the PR pointing to your fork data-plane-api repo, then we can discuss. Thank you!

jrajahalme · 2018-01-06T16:49:11Z

On 5 Jan 2018, at 17.31, Matt Klein ***@***.***> wrote: @jrajahalme <https://github.com/jrajahalme> thanks for this. I think in this case it would be worth it to post the PR for the code change at the same time so that we can review this with some context. I would just rebase your code change and the post the PR pointing to your fork data-plane-api repo, then we can discuss. Thank you!

OK, will post early next week :-) Jarno

wora · 2018-01-06T20:13:55Z

api/lds.proto

+  // the opportunity to manipulate and augment the connection metadata that is used in connection
+  // filter chain matching. Order matters as the filters are processed sequentially right after a
+  // socket has been accepted by the listener, and before a connection is created.
+  repeated Filter listener_filter_chain = 9;


There is a plural name -chains in this file. Please use plural name here as well. What is the difference between _filters vs _filter_chain? Can you please explain the semantics of "chain"?

I think linking to the header file defining the filter interface would be helpful, since this "connection filter" is a new concept. We could also document it somewhere like https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/network_filters and link.

This should be named listener_filters, since it's effectively a common part of filters from all filter chains.

I think semantically, we have the concept of filter chains already for L4 and L7. A chain is just a pipeline of filters, specified in sequential order as a list. So, I think arguably this is another filter chain here.

Currently, there is: Listener::filter_chains (list of message FilterChain) and Listener::filter_chains::filters (list of message Filter), which indeed is a chain of filters, but it's not named like that, because the name is already taken.

This PR adds Listener::listener_filter_chain (list of message Filter), which complements list in Listener::filter_chains::filters and not Listener::filter_chains , so I'm not sure how naming it listener_filter_chain can be a good idea.

OK, yes, listener_filters makes more sense.

Renamed. Will work on the docs once I get the Envoy implementation rebased.

Suggested-by: Piotr Sikora <piotrsikora@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-01-11T01:22:02Z

Posted the corresponding Envoy PR: envoyproxy/envoy#2346.

htuch

LGTM

htuch · 2018-01-11T03:00:40Z

api/lds.proto

  DrainType drain_type = 8;
+
+  // A list of individual listener filters that make up the filter chain for sockets accepted with
+  // the listener. These filters are run before any in the 'filter_chains', and these filters have


Optional: you could refer to the filter_chains field via:

:ref:`filter_chains <envoy_api_field_Listener.filter_chains>`

mattklein123 · 2018-01-11T03:18:41Z

note to maintainers: Please hold on merging this until we look at the associated PR a bit. Thanks.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

wora · 2018-01-11T05:16:49Z

api/lds.proto

  // The type of draining to perform at a listener-wide level.
  DrainType drain_type = 8;
+
+  // A list of individual listener filters that make up the filter chain for sockets accepted with


This sentence is a bit hard to read. Obviously this is a list of filters. What I want to know when should use use them and what action they would perform? How are they different from other filters.

If I guess correctly, this is a list of filters attached to socket listener. They do something before the listener accepts a connection.

Your guess would be almost correct. The rest of the comment states that these filters are run right after the listener accepts a connection (in the sence of the accept() system call).

I've reworded to the comment to not state the obvious and come to the point in the first sentence. Please have a look.

Requested-by: Hong Zhang <wora2000@gmail.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

ggreenway · 2018-01-12T17:21:47Z

api/lds.proto

+  // :ref:`filter_chains <envoy_api_field_Listener.filter_chains>`. Order matters as the filters
+  // are processed sequentially right after a socket has been accepted by the listener, and before
+  // a connection is created.
+  repeated Filter listener_filters = 9;


Since this is a completely different type of filter from network filters, should it use a different type to describe it? Right now they have the same set of data, but possibly in the future they could be different. Prior art: HttpFilter is a different type.

+1. I would do a ListenerFilter type just in case for future enhancement.

Done. Assuming we don't need the deprecated_v1 stuff, ListenerFilter message now only has the name and config members.

Use a distinct type for ListenerFilters. This allows future extensions independent of the network Filter message. ListenerFilter message type is v2 only, so it does not need v1 compatibility fields. Requested-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123

Generally looks great, few small comments.

mattklein123 · 2018-01-13T17:22:18Z

docs/root/configuration/listener_filters/original_dst_filter.rst

@@ -0,0 +1,10 @@
+.. _config_network_filters_echo:


I think this file needs some real text. :)

mattklein123 · 2018-01-13T17:23:23Z

docs/root/configuration/listener_filters/proxy_protocol_filter.rst

@@ -0,0 +1,13 @@
+.. _config_listener_filters_proxy_protocol:


love the new docs. Can we also have a brief mention of this functionality in https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/listeners?

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-01-16T17:36:41Z

@ggreenway Addressed your comments, maybe look again?

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Proxy protocol needs to be run after filter chain has been selected. Remove it from the list of built in filters. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123

LGTM, few comments.

mattklein123 · 2018-01-22T20:51:49Z

api/lds.proto

+  // filter. The built-in filters are:
+  //
+  // [#comment:TODO(mattklein123): Auto generate the following list]
+  // * :ref:`envoy.listener.original_dst <config_listener_filters_original_dst>`


Should we deprecate the existing original_dst field? I think we should. This means:

Add deprecated annotation to proto field.

Adding doc note about deprecation and pointing people to here.

Updating DEPRECATED.md in the main repo.

One issue about this I have been thinking about is that doing a search for a forward-to listener is kind of expensive when there are many listeners, and likely not needed when v2 filter chain matching is implemented. I'd like to deprecate this search together with this flag, if possible. If not possible, I'd like to keep this flag and perform the search for another listener only if this flag is set. I.e., if original destination feature is configured as a listener filter, the search for another listener would not be done. Thoughts?

I'd like to deprecate this search together with this flag, if possible.

Agreed. I guess if you don't want to deprecate until the full filter chain matching is implemented, that's fine, but I would put a TODO/comment somewhere. Or we can just mark up the deprecation now in this PR.

IMO marking for deprecation now is fine, anyone depending on this can still use v1 API regardless when filter chain matching is implemented. I'll mark this as TODO on the Envoy-side PR.

https://github.com/envoyproxy/data-plane-api/blob/master/api/bootstrap.proto#L222

No, I mean annotation per previous comment, then in the doc, then in DEPRECATED.md

Do you think this would work as the doc annotation:

This field is deprecated. Use :ref:`an original_dst <config_listener_filters_original_dst>` :ref:`listener filter <envoy_api_field_Listener.filter_filters>` instead. Note that hand off to another listener is *NOT* performed without this flag. :ref:`FilterChainMatch <envoy_api_msg_FilterChainMatch>` may be used to select a filter chain based on the restored destination address instead.

I would make it more clear that in the current code, handoff is not performed without this flag, and in the future when full filter chain matching is implemented it will be completely replaced with filter chain matching. Basically, it is partially deprecated. We can fully deprecate it when we do the full filter chain matching PR. Does that make sense?

Your comment above for additional clarity makes perfect sense, see the commit I'll post in a moment.

mattklein123 · 2018-01-22T20:53:13Z

docs/root/intro/arch_overview/listener_filters.rst

+================
+
+As discussed in the :ref:`listener <arch_overview_listeners>` section, listener filters may be
+used to manipulate connection metadata. In fact, Envoy implements :ref:`*use_original_dst* and


nit: I would probably just delete the "in fact" sentence, I don't think the v1 details are super important and will become much less over time.

mattklein123 · 2018-01-22T20:53:26Z

docs/root/intro/arch_overview/listener_filters.rst

+As discussed in the :ref:`listener <arch_overview_listeners>` section, listener filters may be
+used to manipulate connection metadata. In fact, Envoy implements :ref:`*use_original_dst* and
+*bind_to_port* listener options <config_listeners_v1>` internally by passing the newly accepted
+connection through listener filters implementing these functions. Main purpose of listener filters


"The main purpose..."

mattklein123 · 2018-01-22T20:53:49Z

docs/root/intro/arch_overview/listener_filters.rst

+*bind_to_port* listener options <config_listeners_v1>` internally by passing the newly accepted
+connection through listener filters implementing these functions. Main purpose of listener filters
+is to make adding further system integration functions easier by not requiring changes to Envoy
+core functionlity, and also makes interaction between multiple such features more explicit.


typo "functionlity" (please run change through spell check)

Done, did not find any more spelling mistakes.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Same functionality can be attained by other v2 API features, namely listener filters and filter chain match. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123

Sweet, thanks. Please merge this into your proxy PR so that we can do the final review there.

comments fixed

lds: Fix spacing.

74a163a

Remove over-enthusiastic spaces, re-fill comment. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

wora reviewed Jan 6, 2018

View reviewed changes

jrajahalme added 2 commits January 10, 2018 13:59

lds: Rename 'listener_filter_chain' to 'listener_filters'

113052e

Suggested-by: Piotr Sikora <piotrsikora@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Merge branch 'master' into listener-filter-chain

071ddd9

htuch previously approved these changes Jan 11, 2018

View reviewed changes

lds: link to filter_chains in comment.

6f24f4d

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme dismissed htuch’s stale review via 6f24f4d January 11, 2018 03:25

wora reviewed Jan 11, 2018

View reviewed changes

lds: Rephrase the comment on 'listener_filters'

0dd802c

Requested-by: Hong Zhang <wora2000@gmail.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

wora approved these changes Jan 11, 2018

View reviewed changes

ggreenway previously requested changes Jan 12, 2018

View reviewed changes

jrajahalme added 2 commits January 12, 2018 18:56

Merge branch 'master' into listener-filter-chain

138d110

mattklein123 reviewed Jan 13, 2018

View reviewed changes

jrajahalme added 3 commits January 15, 2018 16:20

Merge branch 'master' into listener-filter-chain

94773bf

docs: Add basic listener filter documentation.

fd02c81

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Merge branch 'master' into listener-filter-chain

5f41ccc

jrajahalme added 3 commits January 16, 2018 09:42

docs: Add listener_filters/listener_filters to toctree.

11d9ab4

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

docs: Add arch_overview/listener_filters.rst to the toctree.

362031f

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

docs: Remove proxy protocol listener filter from the docs.

a7e6fdf

Proxy protocol needs to be run after filter chain has been selected. Remove it from the list of built in filters. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 reviewed Jan 22, 2018

View reviewed changes

jrajahalme added 2 commits January 22, 2018 14:21

docs: Review fixes.

d6f8842

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

lds: Deprecate 'use_original_dst' flag.

e2a201a

Same functionality can be attained by other v2 API features, namely listener filters and filter chain match. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme force-pushed the listener-filter-chain branch 3 times, most recently from 9d787ee to e2a201a Compare January 23, 2018 01:35

mattklein123 approved these changes Jan 23, 2018

View reviewed changes

mattklein123 merged commit fd1a8c4 into envoyproxy:master Jan 23, 2018

Conversation

jrajahalme commented Jan 6, 2018

Uh oh!

jrajahalme commented Jan 6, 2018

Uh oh!

mattklein123 commented Jan 6, 2018

Uh oh!

jrajahalme commented Jan 6, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Jan 11, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Jan 16, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora Jan 10, 2018 •

edited

Loading