Skip to content

lds: Add listener_filter_chain. #400

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.

Sign up for GitHub

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

Merged
mattklein123 merged 16 commits into from
Jan 23, 2018
Merged

lds: Add listener_filter_chain. #400

Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
02b4a5e
lds: Add listener_filter_chain.
jrajahalme Aug 28, 2017
74a163a
lds: Fix spacing.
jrajahalme Jan 6, 2018
113052e
lds: Rename 'listener_filter_chain' to 'listener_filters'
jrajahalme Jan 10, 2018
071ddd9
Merge branch 'master' into listener-filter-chain
jrajahalme Jan 10, 2018
6f24f4d
lds: link to filter_chains in comment.
jrajahalme Jan 11, 2018
0dd802c
lds: Rephrase the comment on 'listener_filters'
jrajahalme Jan 11, 2018
2ce8e72
lds: Define ListenerFilter message type.
jrajahalme Jan 13, 2018
138d110
Merge branch 'master' into listener-filter-chain
jrajahalme Jan 13, 2018
94773bf
Merge branch 'master' into listener-filter-chain
jrajahalme Jan 16, 2018
fd02c81
docs: Add basic listener filter documentation.
jrajahalme Jan 16, 2018
5f41ccc
Merge branch 'master' into listener-filter-chain
jrajahalme Jan 16, 2018
11d9ab4
docs: Add listener_filters/listener_filters to toctree.
jrajahalme Jan 16, 2018
362031f
docs: Add arch_overview/listener_filters.rst to the toctree.
jrajahalme Jan 16, 2018
a7e6fdf
docs: Remove proxy protocol listener filter from the docs.
jrajahalme Jan 19, 2018
d6f8842
docs: Review fixes.
jrajahalme Jan 22, 2018
e2a201a
lds: Deprecate 'use_original_dst' flag.
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
7 changes: 7 additions & 0 deletions api/lds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,13 @@ message Listener {

// 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

@wora wora Jan 11, 2018

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.

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.

@jrajahalme jrajahalme Jan 11, 2018

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.

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.

// the listener. These filters are run before any in the 'filter_chains', and these filters have

@htuch htuch Jan 11, 2018

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.

Optional: you could refer to the filter_chains field via:

:ref:`filter_chains <envoy_api_field_Listener.filter_chains>`

// 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;

@wora wora Jan 6, 2018

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.

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"?

@htuch htuch Jan 7, 2018

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 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.

@PiotrSikora PiotrSikora Jan 10, 2018 •
edited
Loading

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.

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

@htuch htuch Jan 10, 2018

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 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.

@PiotrSikora PiotrSikora Jan 10, 2018

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.

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.

@htuch htuch Jan 10, 2018

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.

OK, yes, listener_filters makes more sense.

@jrajahalme jrajahalme Jan 10, 2018

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.

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

}

message Filter {
Expand Down