router: vhost/route/w.cluster local filter configuration #3045

rodaine · 2018-04-10T19:17:40Z

This patch adds support for resolving virtual host-, route-, and weighted cluster-local configuration for filters. See https://github.com/envoyproxy/data-plane-api/issues/540 for discussion. Configs are provided via RDS as opaque structs (similar to the existing LDS-provided configs), resolved to concrete message types only once, and exposed by the VirtualHost, Route and RouteEntry types. These RDS-scoped configs can be a different pb message type than the base configuration, but default to being identical. Filters can optionally access these configs to change their behavior accordingly.

I will be following up to this PR with making the buffer filter route-aware. @rshriram will also be working on the fault and lua filters to support this.

Risk Level: Low-Medium
Testing: unit
API/Docs Changes: envoyproxy/data-plane-api#605
Release Notes: (will update in this PR)

rshriram · 2018-04-10T20:24:53Z

cc @lizan @qiwzhang

lizan · 2018-04-10T22:19:31Z

include/envoy/server/filter_config.h

+   *         empty proto. By default, this method returns the same value as createEmptyConfigProto,
+   *         and can be optionally overridden in implementations.
+   */
+  virtual ProtobufTypes::MessagePtr createEmptyRDSConfigProto() { return createEmptyConfigProto(); }


s/RDS/Route/ ?
The route config can come from static config as well.

lizan · 2018-04-10T22:23:29Z

bazel/repository_locations.bzl

    envoy_api = dict(
-        commit = "2dcc435e8ae1d35f8c3a4fa9f132778482fb1a78",
+        commit = "c15104fce713856203fa4ad948d752e9a1f657c1",


revert this file other than this line? those seems unrelated.

the vscode bazel plugin seems a bit overzealous in its linting...

lizan · 2018-04-10T22:28:22Z

source/common/config/utility.h

@@ -219,6 +219,20 @@ class Utility {
    return config;
  }

+  template <class ProtoMessage, class Factory>


I don't think you need the first template parameter, it is always Struct.

was being consistent with the other translate function but will update!

lizan · 2018-04-10T22:33:33Z

source/common/router/config_impl.cc

+    auto& factory = Envoy::Config::Utility::getAndCheckFactory<
+        Server::Configuration::NamedHttpFilterConfigFactory>(name);
+
+    configs_[name] = Envoy::Config::Utility::translateToFactoryRDSConfig(struct_config, factory);


Need to perform validation when PerFilterConfigs are loaded too.

I don't think we can do this unfortunately. What type would we validate? (This is related to me asking very nicely for Validate() to be a method on the base proto message which is virtual and empty and can be overriden...

I meant Validate above, but on second thought, we need a method in Factory to pre-process the route-config at loading time, not in the data path. Currently in this PR there is no way to do pre-processing of the filter config in routes. (for configs in listeners, it is done in createFilterFactoryFromProto)

Pre-processing is being treated as an additional feature. That will need more thinking. We won't be doing that as part of this PR.

we'll likely need to delegate this to the factory(factory) where we already ask it for the pb message. will think about this some more!

mattklein123

Generally looks good, thank you. Some comments to get started.

mattklein123 · 2018-04-10T22:55:20Z

include/envoy/router/router.h

+
+  /**
+   * @return const Protobuf::Message* the per-filter config for the given
+   * filter name configured for this virtual host. If none is present, the


super nit: "the nullptr" is not typically said. Just "nullptr." Same below.

thought it sounded funny. Thanks, the @mattklein123!

mattklein123 · 2018-04-10T22:56:06Z

include/envoy/server/filter_config.h

+   *         empty proto. By default, this method returns the same value as createEmptyConfigProto,
+   *         and can be optionally overridden in implementations.
+   */
+  virtual ProtobufTypes::MessagePtr createEmptyRDSConfigProto() { return createEmptyConfigProto(); }


mattklein123 · 2018-04-10T22:56:45Z

source/common/config/utility.h

+                                                               Factory& factory) {
+    ProtobufTypes::MessagePtr config = factory.createEmptyRDSConfigProto();
+
+    if (config == nullptr) {


I would just RELEASE_ASSERT() this like I recently did for the other instances of this.

I was being consistent with the translate method above. Shall I update both?

Yes please. I don't think it's really needed to throw an exception here.

mattklein123 · 2018-04-10T22:57:56Z

source/common/router/BUILD

@@ -22,6 +22,7 @@ envoy_cc_library(
        "//include/envoy/http:header_map_interface",
        "//include/envoy/router:router_interface",
        "//include/envoy/runtime:runtime_interface",
+        "//include/envoy/server:filter_config_interface",


In general we try to not have common depend on server, though this isn't really enforced explicitly. I would add a TODO here and in code to figure out a better way to break this cycle.

sg. i imagine this could be done with bazel visibility rules?

I need to think about it. Will look at it as part of repo reorg work.

mattklein123 · 2018-04-10T22:59:48Z

source/common/router/config_impl.cc

+    auto& factory = Envoy::Config::Utility::getAndCheckFactory<
+        Server::Configuration::NamedHttpFilterConfigFactory>(name);
+
+    configs_[name] = Envoy::Config::Utility::translateToFactoryRDSConfig(struct_config, factory);


I don't think we can do this unfortunately. What type would we validate? (This is related to me asking very nicely for Validate() to be a method on the base proto message which is virtual and empty and can be overriden...

mattklein123 · 2018-04-10T23:01:04Z

test/common/router/config_impl_test.cc

+  proto_cfgs[factory.name()] = buffer_cfg;
+  PerFilterConfigs configs{proto_cfgs};
+
+  ASSERT_EQ(nullptr, configs.get("unknown.filter"))


s/ASSERT/EXPECT in all places for the most part.

mattklein123 · 2018-04-10T23:02:51Z

test/common/router/config_impl_test.cc

@@ -13,10 +16,13 @@
 #include "common/network/address_impl.h"
 #include "common/router/config_impl.h"

+#include "server/config/http/buffer.h"


This has been moved and will need a master merge.

mattklein123 · 2018-04-10T23:03:53Z

test/common/router/config_impl_test.cc

+}
+
+TEST(PerFilterConfigsTest, RouteLocalConfig) {
+  Server::Configuration::BufferFilterConfig factory;


Instead of using buffer filter, can you register a test filter for this test, so that you can correctly verify that you can override to be a different type for the filter data? If you need to you can have a proto just for testing.

is there an existing one I can use for this or should I create one? Should I create the test proto in envoy or data plane?

Just make one local to the test.

Signed-off-by: Chris Roche <croche@lyft.com>

qiwzhang · 2018-04-12T04:05:44Z

include/envoy/router/router.h

+   * filter name configured for this virtual host. If none is present,
+   * nullptr is returned.
+   */
+  virtual const Protobuf::Message* perFilterConfig(const std::string& name) const PURE;


This suggestion is trying to address this issue: #3008
We can do it in a separate PR. But it is so simple, you can also put in in this PR too.

In these VHost/Route/Cluster, add another function

// If perFitlerConfig exists for this filter, get its config hash value to detect if config is updated.
// return 0 if no config for the filter.
size_t perFitlerConfigHash(const std::string& name) const PURE;

I don't quite understand how this will help you. In the filter are you going to check this on a per-route basis? What you really want is another callback which allows a filter to update global structures on a route update.

This is to help filters need to pre-process per_route_config. per_route_config can be dynamically changed anytime. But you don't want to pre-process the config for each request. This hash will help to detect config change, and skip pre_processing step:

This is how it can be used, For each request in decodeHeader()

auto hash = route()->perFilterConfigHash("my_filter_name"); if (hash == 0) // handle not per_route_config case obj = global_obj_map.find(hash); if (obj == null) { obj = pre_process_config( route()->perFilterConfig("my_filter_name") ); global_obj_map[hash] = obj; }

This will also achieve: different routes with same config can share the same pre_processed object.

This is not how I would recommend implementing this. (The above is not thread safe.). I have a pretty good idea of how to efficiently implement this and give you the functionality you need. Let's do this in a follow up.

In istio mixer-client filter, the "global_map" actually is per_thread_global_map. so this approach works for us.
the filter_factory callback approach is better, the global_map can be used by all worker threads. But it is not easy for Istio to use it since its "object_map" object is way deep inside a per_thread object.
My request is: can we support both: 1) calculate the hash of per_route_config at config update time and filter can access it. 2) implements factory callback to support a config change update.

qiwzhang · 2018-04-12T04:06:44Z

source/common/router/config_impl.cc

+    auto& factory = Envoy::Config::Utility::getAndCheckFactory<
+        Server::Configuration::NamedHttpFilterConfigFactory>(name);
+
+    configs_[name] = Envoy::Config::Utility::translateToFactoryRouteConfig(struct_config, factory);


create another map

config_hash_ to store hash of config

The hash can be calculated by
https://github.com/envoyproxy/envoy/blob/master/source/common/protobuf/utility.h#L127

Same comment as above. I don't think this is very helpful and I would rather sort out filter update callbacks as a separate feature?

Signed-off-by: Chris Roche <croche@lyft.com>

rodaine · 2018-04-13T21:50:59Z

Ok, changes made. PTAL @mattklein123

mattklein123

LGTM, few small comments. Thank you and nice work!

mattklein123 · 2018-04-14T20:47:43Z

test/common/router/config_impl_test.cc

+  Protobuf::Map<std::string, ProtobufWkt::Struct> proto_cfgs;
+  proto_cfgs["unknown.filter"] = some_cfg;
+
+  EXPECT_THROW(PerFilterConfigs{proto_cfgs}, EnvoyException)


I would use EXPECT_THROW_WITH_MESSAGE here for better checking. Same elsewhere if applicable.

mattklein123 · 2018-04-14T20:50:17Z

test/common/router/config_impl_test.cc

+       << source;
+  }
+
+  std::unique_ptr<Registry::InjectFactory<Server::Configuration::NamedHttpFilterConfigFactory>>


nit: I think this can just be stack allocated with the test? Do you need a unique_ptr?

It's a bit self-referential, so I was having issues creating it in the constructor, but probably can do it in the setup. Let me give that a shot.

Ended up just splitting the filter into a nested class of the fixture

mattklein123 · 2018-04-14T20:50:51Z

test/common/router/config_impl_test.cc

+  }) << "returned message should be castable to known type";
+}
+
+TEST_F(PerFilterConfigsTest, UnknownFilter) {


nit: This would be a better test if you had a full route config that got parsed that then resulted in the exception.

mattklein123 · 2018-04-14T20:51:32Z

source/common/router/config_impl.cc

+const Protobuf::Message* PerFilterConfigs::get(const std::string& name) const {
+  auto cfg = configs_.find(name);
+
+  if (cfg == configs_.end()) {


nit: I would just use the ternary operator here to collapse the rest of these lines into one.

zuercher · 2018-04-16T16:41:03Z

lgtm, modulo pending comments.

Signed-off-by: Chris Roche <croche@lyft.com>

rodaine · 2018-04-16T19:19:02Z

@mattklein123 nits knitted!

mattklein123

Very nice, thanks.

lizan reviewed Apr 10, 2018

View reviewed changes

mattklein123 reviewed Apr 10, 2018

View reviewed changes

mattklein123 mentioned this pull request Apr 10, 2018

health check: add Redis custom health checker #2993

Merged

Chris Roche added 10 commits April 11, 2018 15:11

Router: Route/VHost/W.Cluster local filter configuration

636e605

Signed-off-by: Chris Roche <croche@lyft.com>

format

b2d0263

Signed-off-by: Chris Roche <croche@lyft.com>

missed async client types

134734b

Signed-off-by: Chris Roche <croche@lyft.com>

fix repo locations

0cc834f

Signed-off-by: Chris Roche <croche@lyft.com>

s/RDS/Route/

35f37f7

Signed-off-by: Chris Roche <croche@lyft.com>

remove unneccessary template var

4820c48

Signed-off-by: Chris Roche <croche@lyft.com>

the nullptr likes to be called just nullptr

d76ad4d

Signed-off-by: Chris Roche <croche@lyft.com>

checkpoint

6309d41

Signed-off-by: Chris Roche <croche@lyft.com>

s/ASSERT/EXPECT/

9897138

Signed-off-by: Chris Roche <croche@lyft.com>

dependency shuffle

118c728

Signed-off-by: Chris Roche <croche@lyft.com>

rodaine force-pushed the route-local-config branch from b13c20c to 118c728 Compare April 12, 2018 01:37

qiwzhang reviewed Apr 12, 2018

View reviewed changes

dnoe assigned zuercher and alyssawilk Apr 12, 2018

format + convert test to use fixtures with dummy filter

711f59f

Signed-off-by: Chris Roche <croche@lyft.com>

Merge branch 'master' into route-local-config

472f7ae

mattklein123 reviewed Apr 14, 2018

View reviewed changes

rshriram mentioned this pull request Apr 16, 2018

Route-local fault filter config #3084

Merged

Chris Roche and others added 2 commits April 16, 2018 11:34

comment nits

c2f915b

Signed-off-by: Chris Roche <croche@lyft.com>

Merge branch 'master' into route-local-config

cbd19d4

mattklein123 approved these changes Apr 16, 2018

View reviewed changes

mattklein123 merged commit 9d6b432 into envoyproxy:master Apr 16, 2018

rodaine deleted the route-local-config branch April 16, 2018 20:19

router: vhost/route/w.cluster local filter configuration #3045

router: vhost/route/w.cluster local filter configuration #3045

Conversation

rodaine commented Apr 10, 2018

rshriram commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodaine Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiwzhang Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

qiwzhang Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodaine commented Apr 13, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodaine Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

rodaine Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuercher commented Apr 16, 2018

rodaine commented Apr 16, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

rodaine Apr 12, 2018 •

edited

Loading

qiwzhang Apr 12, 2018 •

edited

Loading

qiwzhang Apr 12, 2018 •

edited

Loading

rodaine Apr 16, 2018 •

edited

Loading

rodaine Apr 16, 2018 •

edited

Loading