config: refactor filter registration by mattklein123 · Pull Request #1083 · envoyproxy/envoy

mattklein123 · 2017-06-11T16:06:40Z

For LDS, we need to plumb a stats scope through to all of the filter
factory creation functions. This commit takes the opportunity to no
longer pass the full server and instead passes a new FactoryContext
interface which will allow us to more easily alter what is seen by
filters in the future. While making the same breaking change, I
took the opportunity to encode the type of filter into the registration
which removes a ton of boilerplate from various places. (We have
decided to remove the ability to explicitly configure filter type in the
v2 API).

Though this is a breaking change, since we
are still in the 1.4.0 release cycle I will not be deprecating
the new network/http filter registration functions that were
added during this cycle.

This commit also improves some related coverage and gets us up to
around 98.2%. I will do a follow up change with some other easy
coverage wins that I notice.

htuch · 2017-06-11T17:07:57Z

Seems reasonable at design level. To simplify review, could you split out the mechanical rename of scope/stats and any other trivials into a separate PR?

mattklein123 · 2017-06-11T18:02:34Z

Yes I will split it. Right now I'm just trying to clean up some stuff and see a coverage report.

For LDS, we need to plumb a stats scope through to all of the filter factory creation functions. This commit takes the opportunity to no longer pass the full server and instead passes a new FactoryContext interface which will allow us to more easily alter what is seen by filters in the future. Though this is a breaking change, since we are still in the 1.4.0 release cycle I will not be deprecating the new network/http filter registration functions that were added during this cycle.

mattklein123 · 2017-06-13T03:02:00Z

@lyft/network-team @htuch this is ready to go. I tried to reduce it down as much as possible. It's mostly just boilerplate changes. @htuch I will trade you the other reviews in the morning.

mattklein123 · 2017-06-13T03:11:43Z

Sorry it's ready to go modulo fixing the example config repo. Will do that in the morning. The rest is ready to review.

htuch

LGTM

htuch · 2017-06-13T14:03:42Z

include/envoy/server/BUILD

+
+envoy_cc_library(
+    name = "filter_config_interface",
+    hdrs = ["filter_config.h"],


This should have deps for all the included header interfaces.

htuch · 2017-06-13T14:14:00Z

include/envoy/server/filter_config.h

+  /**
+   * @return Upstream::ClusterManager& singleton for use by the entire server.
+   */
+  virtual Upstream::ClusterManager& clusterManager() PURE;


I assume all of these are needed for existing filter and there is no further way to minimize the exposed surface.

yes, minimized as much as possible.

htuch · 2017-06-13T14:17:40Z

include/envoy/server/filter_config.h

+/**
+ * This lambda 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 they
+ * are used at runtime. This maps JSON -> runtime configuration.


It's a bit unclear from this type signature that this is what is happening. All you get is a Network::FilterManager, whereas when I look at how this is used later, it's capturing JSON config in the lambda in some examples. I think maybe this should be made clearer.

htuch · 2017-06-13T14:17:54Z

include/envoy/server/filter_config.h

+enum class NetworkFilterType { Read, Write, Both };
+
+/**
+ * This lambda is used to wrap the creation of a network filter chain for new connections as they


Nit: Since this is std::function, it's not necessarily a lambda.

htuch · 2017-06-13T14:20:18Z

include/envoy/server/filter_config.h

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


Nit: @return

htuch · 2017-06-13T14:20:32Z

include/envoy/server/filter_config.h

+
+  /**
+   * Returns the identifying name for a particular implementation of a network filter produced by
+   * the factory.


Nit: @return

htuch · 2017-06-13T14:21:23Z

include/envoy/server/filter_config.h

+  /**
+   * Create a particular network 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


What if the factory lambda captures some JSON config and then finds an issue at runtime instantiation?

htuch · 2017-06-13T14:22:12Z

include/envoy/server/filter_config.h

+  * Create a particular http 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.


Same comments as for the network filters above.

htuch · 2017-06-13T14:29:33Z

source/server/config/network/http_connection_manager.cc

    // Now see if there is a factory that will accept the config.
    auto search_it = namedFilterConfigFactories().find(string_name);
-    if (search_it != namedFilterConfigFactories().end()) {
+    if (search_it != namedFilterConfigFactories().end() && search_it->second->type() == type) {


Are there tests covering a found filter with wrong type?

Probably not. Will add.

mattklein123 · 2017-06-13T16:37:31Z

Note that re: the comments about the function documentation, I just copied that, but will go ahead and clean up the text to make it more clear.

mattklein123 · 2017-06-13T17:16:27Z

@htuch updated per comments

RomanDzhabarov · 2017-06-13T20:15:45Z

include/envoy/server/filter_config.h

+   * callback should always be initialized.
+   * @param config supplies the general json configuration for the filter
+   * @param context supplies the filter's context.
+   * @return NetworkFilterFactoryCb fixfix


RomanDzhabarov · 2017-06-13T20:17:36Z

include/envoy/server/filter_config.h

+   * @param config supplies the general json configuration for the filter
+   * @param stat_prefix prefix for stat logging
+   * @param context supplies the filter's context.
+   * @return HttpFilterFactoryCb fixfix


here as well, fixfix

RomanDzhabarov

lgtm

* updates required for envoyproxy/envoy#1083 * update envoy

Cleanups from #1083. Rename some variables and repoint to filter example master. No logic changes.

Regression from #1083. The listener config needs two scopes. One for listener named stats, and one for global named stats that need to be reaped during LDS processing.

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This decouples Server struct from the tracer implementation. **Related Issues/PRs (if applicable)** Preparation for #1083 Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

**Description** This consolidates all the copy-pasted processors that existed per endpoint we support into one generic processor. This was made possible thanks to the series of refactoring that we landed in the past few weeks primarily for dynamic module work #90. Notably, now in order to add an endpoint, majority of the new code will be in translator (where also have shared generic interface) as well as the type definition. No longer it requires the huge copy paste of processors. **Related Issues/PRs (if applicable)** Resolves #1083 Blocker for #1429 #1584 #1592 #1594 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com> Co-authored-by: Ignasi Barrera <ignasi@tetrate.io>

mattklein123 force-pushed the lds_refactor branch 2 times, most recently from e37f4db to 01c5941 Compare June 13, 2017 00:05

mattklein123 force-pushed the lds_refactor branch from 01c5941 to d5508d9 Compare June 13, 2017 02:58

mattklein123 changed the title ~~config: refactor filter registration [WIP DO NOT REVIEW]~~ config: refactor filter registration Jun 13, 2017

htuch reviewed Jun 13, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into lds_refactor

1021faf

mattklein123 added a commit to envoyproxy/envoy-filter-example that referenced this pull request Jun 13, 2017

updates required for envoyproxy/envoy#1083

b85101b

mattklein123 added 4 commits June 13, 2017 09:43

update filter example SHA

4aeac64

fix

ae8895f

comments

a954523

fix

3f567e9

RomanDzhabarov reviewed Jun 13, 2017

View reviewed changes

fix

72549a7

RomanDzhabarov approved these changes Jun 13, 2017

View reviewed changes

RomanDzhabarov merged commit e9c56dc into master Jun 13, 2017

mattklein123 added a commit to envoyproxy/envoy-filter-example that referenced this pull request Jun 13, 2017

Update master (#7)

d4225fc

* updates required for envoyproxy/envoy#1083 * update envoy

mattklein123 added a commit that referenced this pull request Jun 13, 2017

cleanup: variable renames

00b639b

Cleanups from #1083. Rename some variables and repoint to filter example master. No logic changes.

mattklein123 mentioned this pull request Jun 13, 2017

cleanup: variable renames #1103

Merged

mattklein123 added a commit that referenced this pull request Jun 13, 2017

cleanup: variable renames (#1103)

f7e76b6

Cleanups from #1083. Rename some variables and repoint to filter example master. No logic changes.

mattklein123 added a commit that referenced this pull request Jun 14, 2017

stats: fix listener/filter scope prefixes

76b6fc8

Regression from #1083. The listener config needs two scopes. One for listener named stats, and one for global named stats that need to be reaped during LDS processing.

mattklein123 mentioned this pull request Jun 14, 2017

stats: fix listener/filter scope prefixes #1108

Merged

mattklein123 deleted the lds_refactor branch June 16, 2017 23:00

jpsim pushed a commit that referenced this pull request Nov 28, 2022

codeowners: add codeowners file (#1083)

2ec3180

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

codeowners: add codeowners file (#1083)

b6140df

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Conversation

mattklein123 commented Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Jun 11, 2017

Uh oh!

mattklein123 commented Jun 11, 2017

Uh oh!

mattklein123 commented Jun 13, 2017

Uh oh!

mattklein123 commented Jun 13, 2017

Uh oh!

htuch 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!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jun 13, 2017

Uh oh!

mattklein123 commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RomanDzhabarov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattklein123 commented Jun 11, 2017 •

edited

Loading