[thrift_proxy/router] Add service name matching to router implementation #4130

brirams · 2018-08-13T19:20:47Z

Description:
Currently, the thrift router only supports method matching as a way to route thrift requests. This builds on that by adding the ability to specify a service name that is used when matching. This change updates the RouteMatch proto definition to use a oneof field to indicate what type of matching should be done, as well as an invert flag that will allow for inverse matching rules.

Additionally:

ensure new RouteEntryImplBase implementations check that inversion and wildcard matching are not enabled at the same time, as this would result in no matches for a route
implement service matching as checking the prefix of the method name, as that's how it's implemented in thrift

Risk Level:
Low

Testing:

new and existing unit tests pass.
updated integration test use new matching rules and ensure that expected upstreams receive requests.

RouteMatch, as well as invert flag. - create new matching subclasses and construct based on proto definition Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

… for various cases/combinations Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

matching. - better proto documentation - more explicit integration test config Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

…NameRouteEntryImpl to ensure colon(':') is appended to service name, add some invert logic documentation Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

…trap and pick the appropriate destination for each test Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

zuercher

Couple small things, but otherwise looks good!

zuercher · 2018-08-14T18:04:47Z

source/extensions/filters/network/thrift_proxy/router/router_impl.cc

+    throw EnvoyException("Cannot have an empty service name with inversion enabled");
+  }
+
+  if (!service_name.empty() && !StringUtil::endsWith(service_name.c_str(), ":")) {


StringUtil::endsWith takes a const std::string&, so you can just pass service_name. (This is constructing a new temp std::string copy for the call.)

ah -- I think this is a vestige of when I using the class member as an arg to this. will update.

zuercher · 2018-08-14T18:05:43Z

source/extensions/filters/network/thrift_proxy/router/router_impl.h

  RouteConstSharedPtr matches(const MessageMetadata& metadata) const override;

 private:
  const std::string method_name_;
+  bool invert_;


nit: Make this const

zuercher · 2018-08-14T18:06:45Z

source/extensions/filters/network/thrift_proxy/router/router_impl.h

+
+private:
+  std::string service_name_;
+  bool invert_;


nit: both of these should be const. That said, it makes the constructor tricky so, I think I'm ok with leaving service_name_ non-const.

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brian-pane · 2018-08-14T20:56:53Z

api/envoy/config/filter/network/thrift_proxy/v2alpha1/route.proto

-  // If specified, the route must exactly match the request method name. As a special case, an
-  // empty string matches any request method name.
-  string method = 1;
+  oneof match_specifier {


This will allow Thrift requests to be routed based on either method name or service name. @fishcakez and @rgs1, do you think we'll need to be able to route based on the combination of both, e.g. service_name=X and method_name=Y?

FWIW, you can do this already with method_name=service:method

Cool, thanks. That solves my one concern.

zuercher

Looks good to me. Thanks for pitching in!

brirams · 2018-08-14T22:39:53Z

anytime!

zuercher · 2018-08-15T18:17:11Z

@brian-pane do you have any other concerns?

brian-pane

Thanks for the new feature!

brirams · 2018-08-15T19:02:29Z

🎉🎉🎉

…#1938) This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/envoyproxy/envoy: c1cc68d stats: refactoring MetricImpl without strings (envoyproxy#4190) 36809d8 fuzz: coverage profile generation instructions. (envoyproxy#4193) ba40cc9 upstream: rebuild cluster when health check config is changed (envoyproxy#4075) 05c0d52 build: use clang-6.0. (envoyproxy#4168) 01f403e thrift_proxy: introduce header transport (envoyproxy#4082) 564d256 tcp: allow connection pool callers to store protocol state (envoyproxy#4131) 3e1d643 configs: match latest API changes (envoyproxy#4185) bc6a10c Fix wrong mock function name. (envoyproxy#4187) e994c1c Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) 3d1325e Converting envoy configs to V2 (envoyproxy#2957) 8d0680f Add timestamp to HealthCheckEvent definition (envoyproxy#4119) 497efb9 server: handle non-EnvoyExceptions safely if thrown in constructor. (envoyproxy#4173) 6d6fafd config: strengthen validation for gRPC config sources. (envoyproxy#4171) 132302c fuzz: reduce log level when running under fuzz engine. (envoyproxy#4161) 7c04ac2 test: fix V6EmptyOptions in coverage with IPv6 enable (envoyproxy#4155) 1b2219b ci: remove deprecated bazel --batch option (envoyproxy#4166) 2db6a4c ci: update clang to version 6.0 in the Ubuntu build image. (envoyproxy#4157) 71152b7 ratelimit: Add ratelimit custom response headers (envoyproxy#4015) 3062874 ssl: make Ssl::Connection const everywhere (envoyproxy#4179) 706e262 Handle validation of non expiring tokens in jwt_authn filter (envoyproxy#4007) f06e958 fuzz: tag trivial fuzzers with no_fuzz. (envoyproxy#4156) 27fb1d3 thrift_proxy: add service name matching to router implementation (envoyproxy#4130) 8c189a5 Make over provisioning factor configurable (envoyproxy#4003) 6c08fb4 Making test less flaky (envoyproxy#4149) 592775b fuzz: bare bones HCM fuzzer. (envoyproxy#4118) For istio/istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

brirams requested a review from zuercher as a code owner August 13, 2018 19:20

zuercher assigned zuercher and brian-pane Aug 13, 2018

brirams added 7 commits August 14, 2018 10:51

- Add service_name and method_name matching as a union field to thrift

91555d1

RouteMatch, as well as invert flag. - create new matching subclasses and construct based on proto definition Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

throw exception when matching anything and inversion is on. add tests…

ddf3f44

… for various cases/combinations Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

- create Matchable interface and use it for method and service

9850f73

matching. - better proto documentation - more explicit integration test config Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

adhere to conditional convention, pre-process service name in Service…

b3fa46f

…NameRouteEntryImpl to ensure colon(':') is appended to service name, add some invert logic documentation Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

modify integration test to create define additional clusters in boots…

1a3dac2

…trap and pick the appropriate destination for each test Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

format everything

d137082

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

remove superfluous matchable interface

ec5a143

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brirams force-pushed the brirams/#5550/thrift_service_name_matching branch from 1264d0a to ec5a143 Compare August 14, 2018 17:54

zuercher reviewed Aug 14, 2018

View reviewed changes

use const where appropriate, fix failing integration test

c8a8a3a

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>

brian-pane reviewed Aug 14, 2018

View reviewed changes

zuercher approved these changes Aug 14, 2018

View reviewed changes

brian-pane approved these changes Aug 15, 2018

View reviewed changes

zuercher merged commit 27fb1d3 into envoyproxy:master Aug 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[thrift_proxy/router] Add service name matching to router implementation #4130

[thrift_proxy/router] Add service name matching to router implementation #4130

Uh oh!

brirams commented Aug 13, 2018 •

edited

Loading

Uh oh!

zuercher left a comment

Uh oh!

zuercher Aug 14, 2018

Uh oh!

brirams Aug 14, 2018

Uh oh!

zuercher Aug 14, 2018

Uh oh!

zuercher Aug 14, 2018

Uh oh!

brian-pane Aug 14, 2018

Uh oh!

zuercher Aug 14, 2018

Uh oh!

brian-pane Aug 15, 2018

Uh oh!

zuercher left a comment

Uh oh!

brirams commented Aug 14, 2018

Uh oh!

zuercher commented Aug 15, 2018

Uh oh!

brian-pane left a comment

Uh oh!

brirams commented Aug 15, 2018

Uh oh!

Uh oh!

[thrift_proxy/router] Add service name matching to router implementation #4130

[thrift_proxy/router] Add service name matching to router implementation #4130

Uh oh!

Conversation

brirams commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

brirams Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

zuercher Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

zuercher Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

brian-pane Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

zuercher Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

brian-pane Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

brirams commented Aug 14, 2018

Uh oh!

zuercher commented Aug 15, 2018

Uh oh!

brian-pane left a comment

Choose a reason for hiding this comment

Uh oh!

brirams commented Aug 15, 2018

Uh oh!

Uh oh!

brirams commented Aug 13, 2018 •

edited

Loading