thrift router: Support request subset lb by xiaoma2015 · Pull Request #18541 · envoyproxy/envoy

xiaoma2015 · 2021-10-09T08:26:55Z

Signed-off-by: Zhangdong Ma zhdma_xd@163.com

Commit Message:
Implement the function of subset lb for request metadata filter like http：

Line 349 in 96701cb

// The request's metadata, if present, takes precedence over the route's.

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

repokitteh-read-only · 2021-10-09T08:26:58Z

Hi @xiaoma2015, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #18541 was opened by xiaoma2015.

see: more, trace.

wbpcode · 2021-10-11T06:57:38Z

We need add some tests for this new feature.

wbpcode · 2021-10-11T06:57:56Z

/assign

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

wbpcode

Thanks for your contributions, and some minor comments are added.

test/extensions/filters/network/thrift_proxy/router_test.cc

wbpcode · 2021-10-12T06:27:10Z

test/extensions/filters/network/thrift_proxy/router_test.cc

+    if (route_entry_has_match) {
+      ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria())
+          .WillByDefault(Return(&route_entry_matches));
+    } else {
+      ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria())
+          .WillByDefault(Return(nullptr));
+    }


So, where is the verify of metadata match criteria?

test code coming soon

wbpcode · 2021-10-12T06:33:57Z

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

+    if (filter_it != request_metadata.end() && route_criteria != nullptr) {
+      metadata_match_criteria_ = route_criteria->mergeMatchCriteria(filter_it->second);
+    } else if (filter_it != request_metadata.end()) {
+      metadata_match_criteria_ =
+          std::make_unique<Envoy::Router::MetadataMatchCriteriaImpl>(filter_it->second);
+    } else {
+      return route_criteria;
+    }


May be following form can make this branches more clear:

if (filter_it != request_metadata.end()) { metadata_match_criteria_ = route_criteria != nullptr ? ... : ...; // or // if (route_criteria != nullptr) { .... } } else { return route_criteria; }

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

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

wbpcode

Thanks. Looks great. And add some comments about the line comments.

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

test/extensions/filters/network/thrift_proxy/router_test.cc

wbpcode · 2021-10-12T14:07:09Z

test/extensions/filters/network/thrift_proxy/router_test.cc

+  initializeRouter();
+  startRequest(MessageType::Call);
+
+  // Test case for router filter metadata is not empty


May be this comment is unnecessary?

wbpcode · 2021-10-12T14:07:19Z

test/extensions/filters/network/thrift_proxy/router_test.cc

+  initializeRouter();
+  startRequest(MessageType::Call);
+
+  // Test case for router filter metadata is empty


May be this comment is unnecessary?

wbpcode · 2021-10-12T14:09:48Z

test/extensions/filters/network/thrift_proxy/router_test.cc

+  verifyMetadataMatchCriteriaFromRoute(true);
+}
+
+// Test for request metadata is empty


May Test the case where both route metadata match criteria and dynamic metadata match criteria are empty. ?

wbpcode · 2021-10-12T14:21:09Z

test/extensions/filters/network/thrift_proxy/router_test.cc

+    EXPECT_EQ((*it)->value().value().string_value(), "devel");
+    it++;
+
+    // `version` should be what came from the request, overriding the route entry.


May

`version` should be what came from the request and override the route entry's.

?

rgs1

Thanks! Two comments.

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

rgs1 · 2021-10-12T17:44:02Z

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

+      return route_criteria;
+    }
+
+    return metadata_match_criteria_.get();


we can simplify this a bit:

if (filter_it == request_metadata.end()) { return route_criteria; } metadata_match_criteria_ = route_criteria != nullptr ? route_criteria->mergeMatchCriteria(filter_it->second) : std::make_unique<Envoy::Router::MetadataMatchCriteriaImpl>(filter_it->second); return metadata_match_criteria_.get();

using ifelse is better?

thx for review
using ifelse is better?

Yeah looks great -- I am just a fan of the ternary operator 😁.

rgs1 · 2021-10-12T20:05:00Z

Also you'll need to merge main once and possibly twice if #18586 lands while this is being reviewed.

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

xiaoma2015 · 2021-10-13T07:05:43Z

Thanks! Two comments.

thx for review
Is use if is better ?
`
if (filter_it == request_metadata.end()) {
return route_criteria;
}

if (route_criteria != nullptr) {
  metadata_match_criteria_ = route_criteria->mergeMatchCriteria(filter_it->second);
} else {
  metadata_match_criteria_ =
      std::make_unique<Envoy::Router::MetadataMatchCriteriaImpl>(filter_it->second);
}

return metadata_match_criteria_.get();

`

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

rgs1

Looks great - thank you! One last ask: can you add an entry to docs/root/version_history/current.rst noting this was added?

rgs1 · 2021-10-13T14:33:30Z

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

+      return route_criteria;
+    }
+
+    return metadata_match_criteria_.get();


Yeah looks great -- I am just a fan of the ternary operator 😁.

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

xiaoma2015 · 2021-10-14T03:09:49Z

Looks great - thank you! One last ask: can you add an entry to docs/root/version_history/current.rst noting this was added?

done.
like this: * thrift_proxy: support subset lb for request metadata filter

…hrift_metadata_filter Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

rgs1

Thanks! One last thing then it's good for me.

rgs1 · 2021-10-14T04:50:41Z

docs/root/version_history/current.rst

 * http: added support for :ref:`retriable health check status codes <envoy_v3_api_field_config.core.v3.HealthCheck.HttpHealthCheck.retriable_statuses>`.
 * thrift_proxy: add upstream response zone metrics in the form ``cluster.cluster_name.zone.local_zone.upstream_zone.thrift.upstream_resp_success``.
 * vcl_socket_interface: added VCL socket interface extension for fd.io VPP integration to :ref:`contrib images <install_contrib>`. This can be enabled via :ref:`VCL <envoy_v3_api_msg_extensions.vcl.v3alpha.VclSocketInterface>` configuration.
+* thrift_proxy: support subset lb for request metadata filter.


Nit - I think it should be just:

* thrift_proxy: support subset lb when using request or route metadata.

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

…voy into add_thrift_metadata_filter Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

rgs1 · 2021-10-14T17:02:30Z

@xiaoma2015 please merge main.

…hrift_metadata_filter Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

xiaoma2015 · 2021-10-15T02:18:36Z

@xiaoma2015 please merge main.
done

wbpcode

Thanks.

wbpcode · 2021-10-15T06:27:28Z

cc @rgs1 cc @zuercher

thrift router: Support request subset lb

9ec0944

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

xiaoma2015 requested a review from zuercher as a code owner October 9, 2021 08:26

repokitteh-read-only bot assigned wbpcode Oct 11, 2021

xiaoma2015 added 4 commits October 11, 2021 15:46

unit test

abb822d

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

format fix

58df464

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

format fix

f68bb91

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

unit test

2aa1ed4

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

wbpcode reviewed Oct 12, 2021

View reviewed changes

xiaoma2015 added 2 commits October 12, 2021 16:22

unit test

3ea7ccc

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

format fix

4342443

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

wbpcode reviewed Oct 12, 2021

View reviewed changes

davinci26 assigned rgs1 Oct 12, 2021

davinci26 added the waiting label Oct 12, 2021

rgs1 reviewed Oct 12, 2021

View reviewed changes

xiaoma2015 added 2 commits October 13, 2021 10:57

merge main branch

fe993ff

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

update some comments

e4bf7a3

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

repokitteh-read-only bot removed the waiting label Oct 13, 2021

format fix

98f5eab

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

rgs1 reviewed Oct 13, 2021

View reviewed changes

update docs

99a1ce9

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

xiaoma2015 added 2 commits October 14, 2021 11:13

Merge branch 'main' of https://github.com/envoyproxy/envoy into add_t…

43bbed8

…hrift_metadata_filter Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

Merge branch 'main' into add_thrift_metadata_filter

1f4e49e

rgs1 reviewed Oct 14, 2021

View reviewed changes

update docs

3ec0de9

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

xiaoma2015 added 2 commits October 14, 2021 15:17

Merge branch 'add_thrift_metadata_filter' of github.com:xiaoma2015/en…

dc26d39

…voy into add_thrift_metadata_filter Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

update docs

8b0af2d

Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

rgs1 previously approved these changes Oct 14, 2021

View reviewed changes

Merge branch 'main' of https://github.com/envoyproxy/envoy into add_t…

825a0f0

…hrift_metadata_filter Signed-off-by: Zhangdong Ma <zhdma_xd@163.com>

xiaoma2015 dismissed rgs1’s stale review via 825a0f0 October 15, 2021 02:17

wbpcode approved these changes Oct 15, 2021

View reviewed changes

rgs1 approved these changes Oct 15, 2021

View reviewed changes

mattklein123 approved these changes Oct 15, 2021

View reviewed changes

mattklein123 merged commit 11945b4 into envoyproxy:main Oct 15, 2021

xiaoma2015 deleted the add_thrift_metadata_filter branch October 18, 2021 02:49

Conversation

xiaoma2015 commented Oct 9, 2021

Uh oh!

repokitteh-read-only bot commented Oct 9, 2021

Uh oh!

wbpcode commented Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Oct 11, 2021

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaoma2015 Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

rgs1 commented Oct 12, 2021

Uh oh!

xiaoma2015 commented Oct 13, 2021

Uh oh!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaoma2015 commented Oct 14, 2021

Uh oh!

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

rgs1 commented Oct 14, 2021

Uh oh!

xiaoma2015 commented Oct 15, 2021

Uh oh!

wbpcode commented Oct 11, 2021 •

edited

Loading

xiaoma2015 Oct 12, 2021 •

edited

Loading