dubbo_proxy: Adds a routing matcher for the Dubbo protocol. #5571

gengleilei · 2019-01-11T08:33:16Z

Description: Adds a routing matcher for the Dubbo protocol.
Risk Level: low
Testing: unit test
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue] #3998
[Optional Deprecated:]

this is only part of the code for DubboProxy, the rest of the code hasn't been committed yet, complete implementation: https://github.com/gengleilei/envoy/tree/feature-dubbo-router-develop/source/extensions/filters/network/dubbo_proxy

zyfjeff · 2019-01-14T02:06:31Z

api/envoy/config/filter/network/dubbo_proxy/v2alpha1/dubbo_proxy.proto

+
+  // A list of individual Dubbo filters that make up the filter chain for requests made to the
+  // Dubbo proxy. Order matters as the filters are processed sequentially. For backwards
+  // compatibility, if no thrift_filters are specified, a default Dubbo router filter


thrift -> dubbo

zyfjeff · 2019-01-14T02:11:22Z

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

+      // * For range [-10,0), route will match for header value -1, but not for 0,
+      // "somestring", 10.9,
+      //   "-1somestring"
+      envoy.type.Int64Range range_match = 4;


If the type of the parameter is not an integer, is this range matching useful?

Sorry, The type definition is not needed. I delete it.

zyfjeff · 2019-01-14T02:13:38Z

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

+syntax = "proto3";
+
+package envoy.config.filter.network.dubbo_proxy.v2alpha1;
+option java_package = "io.envoyproxy.envoy.config.filter.network.thrift_proxy.v2alpha1";


zyfjeff · 2019-01-14T02:13:51Z

api/envoy/config/filter/network/dubbo_proxy/v2alpha1/dubbo_proxy.proto

-package envoy.extensions.filters.network.dubbo_proxy.v2alpha1;
-option java_package = "io.envoyproxy.envoy.extensions.filters.network.dubbo_proxy.v2alpha1";
+package envoy.config.filter.network.dubbo_proxy.v2alpha1;
+option java_package = "io.envoyproxy.envoy.config.filter.network.thrift_proxy.v2alpha1";


zyfjeff · 2019-01-14T02:46:02Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

+namespace Router {
+
+bool Utility::isContainWildcard(const std::string& input) {
+  return (input.find('*') != std::string::npos) || (input.find('?') != std::string::npos);


https://en.wikipedia.org/wiki/Glob_(programming)
Do you want to support the full wildcard? or you just want to support the * and ? ?, I suggest that it is the same as the virtual host, only * is supported.

Dubbo routing matches need to support wildcard * and ?.

zyfjeff · 2019-01-14T02:46:50Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

+    const envoy::config::filter::network::dubbo_proxy::v2alpha1::Route& route)
+    : cluster_name_(route.route().cluster()) {
+  for (const auto& header_map : route.match().headers()) {
+    config_headers_.push_back(header_map);


emplace_back ?

zyfjeff · 2019-01-14T02:55:15Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.h

+namespace DubboProxy {
+namespace Router {
+
+class Utility {


I prefer using an anonymous namespace

I have moved utility class to utiliti.h file.

zyfjeff · 2019-01-14T02:55:38Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.h

+                           public Logger::Loggable<Logger::Id::dubbo> {
+public:
+  RouteEntryImplBase(const envoy::config::filter::network::dubbo_proxy::v2alpha1::Route& route);
+  virtual ~RouteEntryImplBase() {}


zyfjeff · 2019-01-14T02:57:24Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

+
+ParameterRouteEntryImpl::ParameterData::ParameterData(const ParameterConfig& config) {
+  index_ = config.index();
+  type_ = config.type();


Where is it used?

type_ is useless. I deleted it.

zyfjeff · 2019-01-14T03:01:10Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

+RouteConstSharedPtr MultiRouteMatcher::route(const MessageMetadata& metadata,
+                                             uint64_t random_value) const {
+  for (const auto& route_matcher : route_matcher_list_) {
+    ENVOY_LOG(debug, "dubbo route matcher: route matching failed");


route matching failed?

Sorry，i delete it.

zyfjeff · 2019-01-14T03:04:36Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

+
+RouteConstSharedPtr RouteMatcher::route(const MessageMetadata& metadata,
+                                        uint64_t random_value) const {
+  if (interface_name_ == metadata.service_name() &&


Can interface_name and service_name be unified?

It has been uniformly named service name.

zyfjeff

LGTM, just some nits.

@lizan
I passed the first round of review

test/extensions/filters/network/dubbo_proxy/route_matcher_test.cc

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

gengleilei · 2019-01-15T15:06:07Z

/retest

repokitteh-read-only · 2019-01-15T15:06:16Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #5571 (comment) was created by @gengleilei.

see: more, trace.

api/envoy/config/filter/network/dubbo_proxy/v2alpha1/dubbo_proxy.proto

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

gengleilei · 2019-01-18T03:40:01Z

@lizan Is there any other code review opinion ?

lizan

Sorry for the delay.

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

source/extensions/filters/network/dubbo_proxy/utility.h

lizan

Great, sorry for the confusion

lizan · 2019-01-18T08:19:45Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

+  using envoy::config::filter::network::dubbo_proxy::v2alpha1::RouteMatch;
+
+  // Verify configuration rules.
+  MessageUtil::validate(config);


Ah sorry I misunderstood so ignore my previous comment, you don't need this...

lizan · 2019-01-18T08:20:40Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.h

+
+private:
+  const Matchers::StringMatcher method_name_;
+  absl::optional<std::shared_ptr<ParameterRouteEntryImpl>> parameter_route_;


do you need to differentiate nullopt vs nullptr? If no simple shared_ptr should be enough.

gengleilei · 2019-01-22T06:47:27Z

@lizan Is there any other code review opinion ?

lizan

LGTM

lizan · 2019-01-23T05:59:20Z

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

+
+package envoy.config.filter.network.dubbo_proxy.v2alpha1;
+option java_package = "io.envoyproxy.envoy.config.filter.network.dubbo_proxy.v2alpha1";
+option java_multiple_files = true;


can you merge master? I think this line is no longer needed (will fail master CI)

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

… utility class. Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

lizan

@gengleilei in the future, please do not rebase and force-push, instead you can merge master. force-push will make it harder to follow review history.

gengleilei · 2019-01-24T02:10:36Z

@lizan Ok, thank you for reminder.

…xy#5571) *Description*: Adds a routing matcher for the Dubbo protocol. envoyproxy#3998 *Risk Level*: low *Testing*: unit test *Docs Changes*: N/A *Release Notes*: N/A this is only part of the code for DubboProxy, the rest of the code hasn't been committed yet, complete implementation: https://github.com/gengleilei/envoy/tree/feature-dubbo-router-develop/source/extensions/filters/network/dubbo_proxy Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com> Signed-off-by: Dan Zhang <danzh@google.com>

…xy#5571) *Description*: Adds a routing matcher for the Dubbo protocol. envoyproxy#3998 *Risk Level*: low *Testing*: unit test *Docs Changes*: N/A *Release Notes*: N/A this is only part of the code for DubboProxy, the rest of the code hasn't been committed yet, complete implementation: https://github.com/gengleilei/envoy/tree/feature-dubbo-router-develop/source/extensions/filters/network/dubbo_proxy Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

…xy#5571) *Description*: Adds a routing matcher for the Dubbo protocol. envoyproxy#3998 *Risk Level*: low *Testing*: unit test *Docs Changes*: N/A *Release Notes*: N/A this is only part of the code for DubboProxy, the rest of the code hasn't been committed yet, complete implementation: https://github.com/gengleilei/envoy/tree/feature-dubbo-router-develop/source/extensions/filters/network/dubbo_proxy Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com> Signed-off-by: Fred Douglas <fredlas@google.com>

hzxuzhonghu · 2019-08-02T03:00:03Z

api/envoy/config/filter/network/dubbo_proxy/v2alpha1/dubbo_proxy.proto

+  // Dubbo proxy. Order matters as the filters are processed sequentially. For backwards
+  // compatibility, if no dubbo_filters are specified, a default Dubbo router filter
+  // (`envoy.filters.dubbo.router`) is used.
+  repeated DubboFilter dubbo_filters = 5;


@gengleilei I am new to dubbo, my question is how to setup individual Dubbo filters?

I can not find any other supported filters except envoy.filters.dubbo.router .

You can refer to the Router (router_impl.h), you need to implement DubboFilters: : DecoderFilter interface, yaml configuration is as follows:
stat_prefix: ingress
route_config:
name: local_route
dubbo_filters:
- name: envoy.filters.dubbo.mock_filter
config:
"@type": type.googleapis.com/google.protobuf.Struct
value:
name: test_service

IC， this is for advanced users, need to hack into the envoy code.

The Filter based extension mechanism is similar to HTTP. Dubbo does not define a unique Filter mechanism. of course, if you have good suggestions, we are also very welcome to build together.

I got this error, any idea?

[2019-08-06 02:35:19.872][27][error][dubbo] [external/envoy/source/extensions/filters/network/dubbo_proxy/conn_manager.cc:132] [C35] dubbo error: invalid dubbo mes sage magic number 20186

gengleilei requested a review from lizan as a code owner January 11, 2019 08:33

lizan requested a review from zyfjeff January 11, 2019 08:39

lizan self-assigned this Jan 11, 2019

zyfjeff reviewed Jan 14, 2019

View reviewed changes

zyfjeff previously approved these changes Jan 15, 2019

View reviewed changes

gengleilei dismissed zyfjeff’s stale review via 2cf4731 January 15, 2019 11:15

lizan reviewed Jan 16, 2019

View reviewed changes

lizan reviewed Jan 18, 2019

View reviewed changes

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc Show resolved Hide resolved

source/extensions/filters/network/dubbo_proxy/utility.h Outdated Show resolved Hide resolved

lizan reviewed Jan 18, 2019

View reviewed changes

lizan reviewed Jan 23, 2019

View reviewed changes

gengleilei requested review from htuch, mattklein123 and zuercher as code owners January 23, 2019 11:06

gengleilei force-pushed the feature-dubbo-router-ex branch from 1838398 to 33ae24e Compare January 23, 2019 11:09

gengleilei added 8 commits January 23, 2019 19:35

Adds a routing matcher for the Dubbo protocol.

1f0cffd

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Fixed spelling errors in word parameter.

af379bc

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Delete useless code and add utility.h file.

9ea2792

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Add unit test cases for wildcards(?).

496b88b

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Modify the route.proto and dubbo_proxy.proto definition.

0b6a7bc

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Change the method name type to StringMatcher.

62f8268

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Add validation for the RouteConfiguration rule and remove the useless…

f66d586

… utility class. Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Changed the parameter_router_ type tto shared_ptr.

bbe4d86

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

gengleilei force-pushed the feature-dubbo-router-ex branch from 33ae24e to bbe4d86 Compare January 23, 2019 11:43

Remove "java_multiple_files" declaration from the route.proto file

2ca11f1

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

lizan approved these changes Jan 23, 2019

View reviewed changes

lizan merged commit 57b1982 into envoyproxy:master Jan 23, 2019

gengleilei deleted the feature-dubbo-router-ex branch January 29, 2019 07:31

hzxuzhonghu reviewed Aug 2, 2019

View reviewed changes

dubbo_proxy: Adds a routing matcher for the Dubbo protocol. #5571

dubbo_proxy: Adds a routing matcher for the Dubbo protocol. #5571

Conversation

gengleilei commented Jan 11, 2019

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

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

zyfjeff left a comment

Choose a reason for hiding this comment

gengleilei commented Jan 15, 2019

repokitteh-read-only bot commented Jan 15, 2019

gengleilei commented Jan 18, 2019

lizan left a comment

Choose a reason for hiding this comment

lizan 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

gengleilei commented Jan 22, 2019

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

gengleilei commented Jan 24, 2019

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