http filters: less filter factory boilerplate by mattklein123 · Pull Request #3109 · envoyproxy/envoy

mattklein123 · 2018-04-17T20:23:23Z

I want a quick feedback as to whether this direction is worth it or not. It makes the registration boilerplate marginally better. I'm not really sure what else can be easily done.

@jmarantz WDYT? Continue?

This is in reference to #2911

htuch

I like this direction, +1.

htuch · 2018-04-17T21:59:55Z

source/extensions/filters/http/gzip/config.cc

                                                const std::string&,
                                                Server::Configuration::FactoryContext&) {
-  return createFilter(
+  GzipFilterConfigSharedPtr config = std::make_shared<GzipFilterConfig>(


Can FactoryBase provide a method that hides the downcast? What I would do is have the GzipFilterFactory implement createFilterFactoryFromTypedProto, and pass in the already downcast variant from FactoryBase::createfilterFactoryFromProto.

yes I can do something like that, but the validation all has to be done here of course. I will see what I can do.

See https://github.com/envoyproxy/envoy/blob/master/include/envoy/grpc/async_client.h#L162 for an example of this pattern that I did for gRPC.

mattklein123 · 2018-04-17T23:30:11Z

@htuch @jmarantz PTAL. If this looks good I will do the other filters.

jmarantz

Thanks for doing this. It looks great; you just need to fix your comments I think.

My suggestion is not to expand this to other filters yet; there may be further ways to simplify. But IMO there's no need to get it perfect first as this a solid improvement.

jmarantz · 2018-04-18T01:57:44Z

source/extensions/filters/http/gzip/config.cc

-  return createFilter(
-      MessageUtil::downcastAndValidate<const envoy::config::filter::http::gzip::v2::Gzip&>(
-          proto_config));
+  GzipFilterConfigSharedPtr config = std::make_shared<GzipFilterConfig>(validate(proto_config));


this is a huge improvement and I think we should just move forward with this. I also wonder here what in this file is specific to gzip, and couldn't be supplied by the framework as a template default of some sort. But let's go with this and we can keep iterating after merge if we think there's more ways to factor out the boilerplate.

My preference is to not partially do this, at least within a filter family. I would rather just do all of the HTTP filters so we don't leave things an an inconsistent state. Can we brainstorm additional ways to reduce boilerplate now?

The only thing that comes to mind is to have the base class which implements the functions, but then accepts lambdas in the constructors. It's possible this would git rid of some more code? Not sure how much of an improvement that would be though.

htuch · 2018-04-18T14:06:12Z

source/extensions/filters/http/gzip/config.cc

-}
-
 Server::Configuration::HttpFilterFactoryCb
 GzipFilterFactory::createFilterFactoryFromProto(const Protobuf::Message& proto_config,


I would still argue for the typed vs. untyped variants and having createFilterFactoryFromProto defined in the superclass, with the typed variant here. This is cleaner, it saves basically a line of code and only exposes a typed interface to the subclass.

Sorry I don't think I fully understood what you were asking for. I think I get it now. Will update.

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2018-04-18T19:16:32Z

@htuch @jmarantz PTAL

htuch

Approach LGTM, will take another pass when out of WIP status.

htuch · 2018-04-18T20:10:06Z

source/extensions/filters/http/common/factory_base.h

+  FactoryBase(const std::string& name) : name_(name) {}
+
+private:
+  virtual Server::Configuration::HttpFilterFactoryCb


Should this be protected rather than private?

technically it can be private as it's only called by this class. Derived classes can override private functions. With that said, it doesn't matter to me one way or the other.

mattklein123 · 2018-04-18T20:37:52Z

I'm going to hold on this till next week when the various filter config stuff has landed as I think I will be able to clean up some of that also.

jmarantz · 2018-05-06T21:17:11Z

@mattklein123 if you want to pick this up again I can take another look. Once you merge master I want to patch in and play with the code so I can get a better feel.

mattklein123 · 2018-05-07T00:37:43Z

@jmarantz was planning on getting back to this, but also very happy to hand it over. I will merge master tomorrow so you can take a look.

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2018-05-07T18:24:12Z

@jmarantz I merged master and also updated the factory base class to take into account the new per-route filter config which is implemented for buffer filter. Let me know what you think and whether you want to fully take this PR over or just play with it. I would like to land the HTTP stuff this week if possible (trying to clear through my backlog after the conference).

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2018-05-11T00:14:20Z

api/envoy/config/filter/http/health_check/v2/health_check.proto

  // Specifies the incoming HTTP endpoint that should be considered the
  // health check endpoint. For example */healthcheck*.
-  string endpoint = 2 [(validate.rules).string.min_bytes = 1, deprecated = true];
+  string endpoint = 2 [deprecated = true];


This was exposed with improved testing via my changes.

mattklein123 · 2018-05-11T00:15:20Z

@htuch @jmarantz PTAL I updated the rest of the HTTP filters. Assuming this looks good, would like to merge and we can always improve more in the future. I will do something similar for the network filters next.

htuch

This is rad, some minor comments.

htuch · 2018-05-11T14:58:37Z

source/extensions/filters/http/buffer/config.cc

 namespace BufferFilter {

-Http::FilterFactoryCb BufferFilterConfigFactory::createFilter(
+Http::FilterFactoryCb BufferFilterFactory::createTypedFilterFactoryFromProto(


Nit: for consistency with gRPC, maybe this should have Typed as a suffix, same with per-route.

htuch · 2018-05-11T14:59:44Z

source/extensions/filters/http/common/factory_base.h

+  }
+
+  ProtobufTypes::MessagePtr createEmptyConfigProto() override {
+    return ProtobufTypes::MessagePtr{new ConfigProto()};


Nit: std::make_unique?

htuch · 2018-05-11T15:01:07Z

test/extensions/filters/http/squash/config_test.cc

@@ -1,3 +1,5 @@
+#include "envoy/config/filter/http/squash/v2/squash.pb.validate.h"


How come we started to need this?

Because now the factory has built-in validation which wasn't happening before in all cases and the include is only in the .cc file.

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2018-05-11T18:18:25Z

@htuch updated

htuch

Wh00t.

danielhochman assigned jmarantz Apr 17, 2018

htuch suggested changes Apr 17, 2018

View reviewed changes

jmarantz reviewed Apr 18, 2018

View reviewed changes

htuch self-assigned this Apr 18, 2018

htuch reviewed Apr 18, 2018

View reviewed changes

repo reorg: streamline HTTP filter factories

61d41f5

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 force-pushed the registration_fixes branch from d7dd9c3 to 61d41f5 Compare April 18, 2018 19:15

htuch reviewed Apr 18, 2018

View reviewed changes

lizan mentioned this pull request Apr 19, 2018

Add perFilerConfigObject to store pre-processed per-route-config #3128

Merged

mattklein123 added 2 commits May 7, 2018 13:34

Merge branch 'master' into registration_fixes

1e380ee

Signed-off-by: Matt Klein <mklein@lyft.com>

fixes

7d1e1dd

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 added 5 commits May 10, 2018 14:27

Merge branch 'master' into registration_fixes

1fde27b

Signed-off-by: Matt Klein <mklein@lyft.com>

more

fb805db

Signed-off-by: Matt Klein <mklein@lyft.com>

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

4c6639c

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

b5d706f

Signed-off-by: Matt Klein <mklein@lyft.com>

Merge branch 'master' into registration_fixes

8330775

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 force-pushed the registration_fixes branch from de16ba4 to 8330775 Compare May 11, 2018 00:13

mattklein123 commented May 11, 2018

View reviewed changes

mattklein123 changed the title ~~[RFC] Less filter boilerplate~~ http filters: less filter factory boilerplate May 11, 2018

htuch suggested changes May 11, 2018

View reviewed changes

mattklein123 added 2 commits May 11, 2018 14:08

Merge branch 'master' into registration_fixes

a7851b9

Signed-off-by: Matt Klein <mklein@lyft.com>

comment

bf40d5a

Signed-off-by: Matt Klein <mklein@lyft.com>

htuch approved these changes May 11, 2018

View reviewed changes

mattklein123 merged commit 01fef5b into master May 11, 2018

mattklein123 deleted the registration_fixes branch May 11, 2018 20:22

		@@ -1,3 +1,5 @@
		#include "envoy/config/filter/http/squash/v2/squash.pb.validate.h"

Conversation

mattklein123 commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mattklein123 commented Apr 17, 2018

Uh oh!

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

mattklein123 commented Apr 18, 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!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Apr 18, 2018

Uh oh!

jmarantz commented May 6, 2018

Uh oh!

mattklein123 commented May 7, 2018

Uh oh!

mattklein123 commented May 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented May 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!

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 May 11, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattklein123 commented Apr 17, 2018 •

edited

Loading