filters: install network filters on upstream connections #173 #6173

alanconway · 2019-03-05T15:45:27Z

Allow network filters to be installed via cluster configuration,
uses the same configuration syntax as listener filters.

Added cluster filter config: api/envoy/api/v2/cluster/filter.proto

Tests to verify filters are aplied test/common/upstream/cluster_manager_impl_test.cc

Implementation in source/common/upstream:

Create filter factory and apply to new upstream connnections.
Implement Server::Configuration::FactoryContext.

NEEDS MINOR WORK: Some FactoryContext functions throw NOT_IMPLEMENTED, because
some context objects are not obviously available in the upstream code.
Needs attention from someone better versed in the architecture, see TODO(aconway)

Does not prevent use of the feature, all the contexts available from
TransportSocketFactoryContext are available which covers many use cases.

This feature is used by project: https://github.com/alanconway/envoy-amqp.

Signed-off-by: Alan Conway aconway@redhat.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Allow network filters to be installed via cluster configuration, uses the same configuration syntax as listener filters. Added the following: - Cluster filter config: api/envoy/api/v2/cluster/filter.proto - Test to verify filters are applied: test/common/upstream/cluster_manager_impl_test.cc - Create filter factory and apply to new upstream connnections - Implement Server::Configuration::FactoryContext using contexts from TransportSocketFactoryContext NEEDS MINOR WORK: Some FactoryContext functions throw NOT_IMPLEMENTED, because some context objects are not obviously available to the upstream code. Needs attention from someone better versed in the architecture, see TODO(aconway) This does not prevent use of the feature, most contexts are available. This feature is used by project: https://github.com/alanconway/envoy-amqp. Signed-off-by: Alan Conway <aconway@redhat.com>

mattklein123

Wow I'm actually surprised at how simple this change is. Very cool. It makes me think it might not be too bad to do an HTTP version also if we factored some of the filter handling code out of the HCM. I do think this change would be worthy of an integration test. Would you be willing to do one? Also I think we should have some additional docs on this (release notes, arch docs, etc.). Thank you!

/wait

mattklein123 · 2019-03-07T18:25:11Z

api/envoy/api/v2/cluster/filter.proto

+  // Filter specific configuration which depends on the filter being
+  // instantiated. See the supported filters for further documentation.
+  oneof config_type {
+    google.protobuf.Struct config = 2 [deprecated = true];


I think for this new feature we can have only typed_config

OK, but I need more guidance. This pattern is used by most every .proto file under api/v2. Is there an example I can follow?

Take a look at #6118 which is adding a new extension and includes only typed config.

alanconway · 2019-03-07T19:00:58Z

On Thu, Mar 7, 2019 at 1:27 PM Matt Klein ***@***.***> wrote: ***@***.**** requested changes on this pull request. Wow I'm actually surprised at how simple this change is. Very cool. It makes me think it might not be too bad to do an HTTP version also if we factored some of the filter handling code out of the HCM. I do think this change would be worthy of an integration test. Would you be willing to do one?

Yes but it won't be for at least a month, I've got other stuff to do this week and then going on vacation. If anyone else has time that would be great.

Also I think we should have some additional docs on this (release notes, arch docs, etc.). Thank you!

+1 on docs - upstream filters behave exactly the same as listener network filters, but that should be documented in a more obvious place. I need to figure out how the doc system works. Will also not happen till post vacation. My AMQP bridge https://github.com/alanconway/envoy-amqp uses the upstream filters. Would you consider taking it on as a built-in envoy filter? It is fairly complete but needs to be moved into the envoy repo, given some integration tests, and the docs need to be integrated into envoy's system. If you're agreeable I'll do that when I get back.

…

/wait ------------------------------ In api/envoy/api/v2/cluster/filter.proto <#6173 (comment)>: > +import "google/protobuf/struct.proto"; + +import "validate/validate.proto"; +import "gogoproto/gogo.proto"; + +option (gogoproto.equal_all) = true; + +// [#protodoc-title: Upstream filters] +message Filter { + // The name of the upstream filter to instantiate. The name must match a supported filter. + string name = 1 [(validate.rules).string.min_bytes = 1]; + + // Filter specific configuration which depends on the filter being + // instantiated. See the supported filters for further documentation. + oneof config_type { + google.protobuf.Struct config = 2 [deprecated = true]; I think for this new feature we can have only typed_config — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6173 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHa6XlxjtOxQkxHIKeOYN8je0tSlirThks5vUVoIgaJpZM4be52j> .

mattklein123 · 2019-03-08T01:04:45Z

@alanconway I will mark this so the stalebot doesn't close it and we can pick it up when you are back.

rshriram · 2019-03-10T15:07:00Z

source/common/upstream/upstream_impl.cc

+          factory.createFilterFactory(*filter_config->getObject("value", true), *factory_context_);
+    } else {
+      auto message = Config::Utility::translateToFactoryConfig(proto_config, factory);
+      callback = factory.createFilterFactoryFromProto(*message, *factory_context_);


you don't need this. deprecated_v1 was only for listener filters (mostly tcp) that needed old functionality. In your case, its all new..

rshriram · 2019-03-10T15:07:56Z

source/common/upstream/upstream_impl.cc

+  // Create upstream filter factories
+  auto filters = config.filters();
+  for (ssize_t i = 0; i < filters.size(); i++) {
+    const auto& proto_config = filters[i];


do you want to run a sanity check of disallowed filters? Like tcp proxy/redis?

rshriram · 2019-03-10T15:11:08Z

test/common/upstream/cluster_manager_impl_test.cc

+  EXPECT_CALL(factory_.tls_.dispatcher_, createClientConnection_(_, _, _, _))
+      .WillOnce(Return(connection));
+  auto conn_data = cluster_manager_->tcpConnForCluster("cluster_1", nullptr, nullptr);
+  EXPECT_EQ(connection, conn_data.connection_.get());


could you add a test here to check if filter gets invoked when data passes through?

rshriram · 2019-03-10T15:13:18Z

source/common/upstream/upstream_impl.h

+
+  class FactoryContextImpl;
+  std::unique_ptr<Server::Configuration::FactoryContext> factory_context_;
+  std::vector<Network::FilterFactoryCb> filter_factories_;


how does filter get called onData?

rshriram

what is the goal of these filters? is it to perform endpoint specific mutation on the data before sending it out?

alanconway · 2019-03-12T18:28:51Z

On Sun, Mar 10, 2019 at 11:31 AM Shriram Rajagopalan < ***@***.***> wrote: ***@***.**** commented on this pull request. what is the goal of these filters? is it to perform endpoint specific mutation on the data before sending it out?

Yep - it's used by https://github.com/alanconway/envoy-amqp to turn HTTP into AMQP (there's a listener filter on the other side turning AMQP into HTTP) Thanks for your suggestions, I'll get to them when I return from vacation in 2 weeks.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6173 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHa6XqU0Qtc25eJODwkLOK3QCknHfcmSks5vVSVYgaJpZM4be52j> .

gargnupur · 2019-06-26T18:26:12Z

@alanconway : why was this closed and not pursued further?

alanconway · 2019-07-04T14:28:46Z

The branch was deleted by mistake. Unfortunately I've changed roles in my organization so I don't have much time to work on this. It is code complete, but needs to be rebased on the latest envoy.

kyessenov · 2019-07-08T18:09:38Z

@alanconway If you're OK with it, we can take over and re-base this PR as long as we're all OK with the API changes.

mattklein123 · 2019-07-09T14:02:22Z

Closing as this is now covered by #7503

alanconway mentioned this pull request Mar 5, 2019

filters: allow filter installation for upstream connections #173

Closed

mattklein123 self-assigned this Mar 5, 2019

mattklein123 requested changes Mar 7, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 7, 2019

mattklein123 added the no stalebot Disables stalebot from closing an issue label Mar 8, 2019

rshriram reviewed Mar 10, 2019

View reviewed changes

alanconway closed this Apr 24, 2019

alanconway deleted the upstream-filter branch April 24, 2019 21:13

alanconway restored the upstream-filter branch July 4, 2019 14:25

alanconway reopened this Jul 4, 2019

kyessenov mentioned this pull request Jul 9, 2019

filter: add network filters to the upstreams #7503

Merged

mattklein123 closed this Jul 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filters: install network filters on upstream connections #173 #6173

filters: install network filters on upstream connections #173 #6173

alanconway commented Mar 5, 2019

mattklein123 left a comment

mattklein123 Mar 7, 2019

alanconway Mar 7, 2019

mattklein123 Mar 8, 2019

alanconway commented Mar 7, 2019 via email

mattklein123 commented Mar 8, 2019

rshriram Mar 10, 2019

rshriram Mar 10, 2019

rshriram Mar 10, 2019

rshriram Mar 10, 2019

rshriram left a comment

alanconway commented Mar 12, 2019 via email

gargnupur commented Jun 26, 2019

alanconway commented Jul 4, 2019

kyessenov commented Jul 8, 2019

mattklein123 commented Jul 9, 2019

filters: install network filters on upstream connections #173 #6173

filters: install network filters on upstream connections #173 #6173

Conversation

alanconway commented Mar 5, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Mar 7, 2019

Choose a reason for hiding this comment

alanconway Mar 7, 2019

Choose a reason for hiding this comment

mattklein123 Mar 8, 2019

Choose a reason for hiding this comment

alanconway commented Mar 7, 2019 via email

mattklein123 commented Mar 8, 2019

rshriram Mar 10, 2019

Choose a reason for hiding this comment

rshriram Mar 10, 2019

Choose a reason for hiding this comment

rshriram Mar 10, 2019

Choose a reason for hiding this comment

rshriram Mar 10, 2019

Choose a reason for hiding this comment

rshriram left a comment

Choose a reason for hiding this comment

alanconway commented Mar 12, 2019 via email

gargnupur commented Jun 26, 2019

alanconway commented Jul 4, 2019

kyessenov commented Jul 8, 2019

mattklein123 commented Jul 9, 2019