Use dynamicMetadata to construct metadataMatchCriteria() #3254

rgs1 · 2018-04-30T03:42:26Z

This enables filter writers to construct metadataMatchCriteria()
from a RequestInfo (e.g.: header value). This is useful given
that currently routes need to be statically matched when
using metadata, e.g.:

---
match:
  prefix: /
route:
  cluster: cluster-name
  metadata_match:
    filter_metadata:
      envoy.lb:
        version: '1.0'

For setups in which RDS isn't used and routes cannot be constructed
dynamically, a filter can be used to match which endpoints should be
used.

So a filter could do something like:

  auto header_value = std::string(header_entry->value().getStringView());
  auto keyval = MessageUtil::keyValueStruct("version", header_value);
  callbacks_->requestInfo().setDynamicMetadata("envoy.lb", keyval);

to determine the version for this request.

Signed-off-by: Raul Gutierrez Segales [email protected]

zuercher · 2018-04-30T16:29:47Z

source/common/router/router.h

@@ -163,6 +163,10 @@ class Filter : Logger::Loggable<Logger::Id::router>,
  }
  const Router::MetadataMatchCriteria* metadataMatchCriteria() const override {
    if (route_entry_) {
+      const auto impl = dynamic_cast<RequestInfo::RequestInfoImpl*>(&callbacks_->requestInfo());


Instead of depending on the RequestInfoImpl here (and Router::MetadataMatchCriteriaImpl in the RequestInfoImpl), is there a reason you can't invoke callbacks_->requestInfo().dynamicMetadata() here and then construct the merged MetadataMatchCriteriaImpl in this function?

because metadataMatchCriteria() needs to create a new Router::MetadataMatchCriteria object, which I think should be attached to RequestInfo. We could create it here, and then attach it to RequestInfo... Any better ideas?

Btw, I know it's confusing that I added this to the Impl classes, I was just lazy and didn't want to change the interfaces for the base classes. I will do that, if you think this approach is generally sane.

I'm still not sure we should leak the match criteria into the RequestInfo interface. A new Router::Filter instance is created for each new stream, so I think it would be safe to track the Router::MetadataMatchCriteria in the filter.

ah, i wasn't aware of that. let me change it then.

hmm, it'll mean more interface breakage tho given that Router::Filter inherits from Upstream::LoadBalancerContext:

const Router::MetadataMatchCriteria* metadataMatchCriteria() const override {}

which now must mutate the object... Let me see how it'll look.

ok, it's not that many places...

zuercher · 2018-04-30T16:31:48Z

source/common/request_info/request_info_impl.h

+  metadataMatchCriteria(const Router::MetadataMatchCriteria* metadata_match) {
+    // The request's metadata, if present, takes precedence over the route's.
+    const auto& metadata = dynamicMetadata().filter_metadata();
+    const auto filter_it = metadata.find("envoy.lb");


See my comment about placement of this code, but you should use the ENVOY_LB constant from common/config/well_known_names.h.

yep, will do. Thanks for the quick review!

This enables filter writers to construct metadataMatchCriteria() from a RequestInfo (e.g.: header value). This is useful given that currently routes need to be statically matched when using metadata, e.g.: ``` --- match: prefix: / route: cluster: cluster-name metadata_match: filter_metadata: envoy.lb: version: '1.0' ``` For setups in which RDS isn't used and routes cannot be constructed dynamically, a filter can be used to match which endpoints should be used. So a filter could do something like: ``` auto header_value = std::string(header_entry->value().getStringView()); auto keyval = MessageUtil::keyValueStruct("version", header_value); callbacks_->requestInfo().setDynamicMetadata("envoy.lb", keyval); ``` to determine the version for this request. Signed-off-by: Raul Gutierrez Segales <[email protected]>

rgs1 · 2018-04-30T19:58:32Z

@zuercher updated to keep everything within Filter.

It required relaxing the definition of LoadBalancerContext.metadataMatchCriteria(). I also moved mergeMatchCriteria from MetadataMatchCriteriaImpl to its base class, to avoid the dynamic_cast bussiness.

I accidentally rebased... sorry about that. I'll add some tests.

Signed-off-by: Raul Gutierrez Segales <[email protected]>

zuercher

Looks pretty good to me. I think there's one missing bit of code, though.

zuercher · 2018-05-01T16:33:11Z

source/common/router/router.h

+      const auto filter_it = request_metadata.find(Envoy::Config::MetadataFilters::get().ENVOY_LB);
+      if (filter_it != request_metadata.end()) {
+        metadata_match_ =
+            route_entry_->metadataMatchCriteria()->mergeMatchCriteria(filter_it->second);


I think this needs to handle the case where route_entry_->metadataMatchCriteria() is nullptr and add a test case for that path.

zuercher · 2018-05-01T16:36:25Z

test/common/router/router_test.cc


+  EXPECT_CALL(callbacks_.request_info_, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata));


I think you should modify the mock in test/mocks/request_info to return this by default.

Also: * add a test for the above * add default return value for MockRequestInfo.dynamicMetadata Signed-off-by: Raul Gutierrez Segales <[email protected]>

Signed-off-by: Raul Gutierrez Segales <[email protected]>

rgs1 · 2018-05-01T21:52:57Z

@zuercher thanks for the review! i added two more commits to address your latest comments.

htuch

Looks good, thanks, a couple of nits.

htuch · 2018-05-03T02:08:35Z

include/envoy/router/router.h

+   * Creates a new MetadataMatchCriteria, merging existing
+   * metadata criteria this criteria. The result criteria is the
+   * combination of both sets of criteria, with those from the
+   * ProtobufWkt::Struct taking precedence.


Nit: which ProtobufWkt::Struct? Do you want to use @param and @return here?

htuch · 2018-05-03T02:08:47Z

include/envoy/router/router.h

+
+  /**
+   * Creates a new MetadataMatchCriteria, merging existing
+   * metadata criteria this criteria. The result criteria is the


Nit: "with this criteria"

@param

Also, annotate with @param and @return. Signed-off-by: Raul Gutierrez Segales <[email protected]>

rgs1 · 2018-05-03T03:37:18Z

@htuch addressed -- thanks!

mattklein123

LGTM, small Q/comment.

mattklein123 · 2018-05-03T07:55:20Z

source/common/router/router.h

@@ -161,8 +163,20 @@ class Filter : Logger::Loggable<Logger::Id::router>,
    }
    return {};
  }
-  const Router::MetadataMatchCriteria* metadataMatchCriteria() const override {
+  const Router::MetadataMatchCriteria* metadataMatchCriteria() override {


In the retry case you may end up calling this method multiple times. Is it worth checking to see if metadata_match_ is not nullptr and returning if not? Is there any chance that doing the merge multiple times could have strange effects? If not to both of these, I would still add a comment about how this can happen multiple times for a single request.

I think checking for nullptr and returning early is worth it. Multiple calls to merge should yield the same result, but we can avoid the computation and creating new objects.

@mattklein123 hold on -- can the route_entry_ for a request change (e.g.: someone called clearRouteCache() and then metadataMatchCriteria() gets called again)? In that case, caching the result for metadataMatchCriteria() would be wrong... we'd want to recompute.

cc: @zuercher @htuch

I think it's ok, as long as metadataMatchCriteria() isn't invoked until LoadBalancer::chooseHost, which happens during Router::Filter::getConnPool in Filter::decodeHeaders and again in Filter::doRetry. The route_ and route_entry_ get set in decodeHeaders before load balancing, and I believe that once that happens they do not change.

clearRouteCache operates on the Http::ConnectionManagerImpl::ActiveStreamFilterBase which ends up being the source of the route_ and therefore route_entry_ via its implementation of StreamFilterCallbacks::route().

This seems right, can we add an ASSERT to this effect (i.e. if we get called twice, the route entry is the same)?

Signed-off-by: Raul Gutierrez Segales <[email protected]>

rgs1 · 2018-05-03T19:00:35Z

@htuch @mattklein123 addressed — thanks!

htuch · 2018-05-03T22:38:11Z

source/common/router/router.h

+      // Have we been called before? If so, there's no need to recompute because
+      // by the time this method is called for the first time, route_entry_ should
+      // not change anymore.
+      if (metadata_match_ != nullptr) {


Can you add a test for this as well? Thanks.

@htuch done — plus two bonus tests:

using > 1 key/value pair in dynamicMetadata()

mixing a key that overrides the route entry with one that doesn't

thanks for the review!

This tests 3 things: * using > 1 key/value in dynamicMetadata() * mixing a key/value pair that overrides the route entry's config with another pair that doesn't override the route entry's config * caching metadataMatchCriteria()'s result, when it has been computed from the request's dynamic metadata Signed-off-by: Raul Gutierrez Segales <[email protected]>

mattklein123

Very nice, thank you!

htuch

Thanks!

This was referenced Apr 30, 2018

extract subset load balancer metadata matches from request #2436

Closed

Allow filters to set metadata matches from request #3255

Closed

rgs1 force-pushed the merge-request-info-dynamic-metadata-for-lb branch from 39783b5 to 475617f Compare April 30, 2018 03:56

mattklein123 assigned zuercher Apr 30, 2018

zuercher requested changes Apr 30, 2018

View reviewed changes

rgs1 force-pushed the merge-request-info-dynamic-metadata-for-lb branch from 475617f to bde0c9c Compare April 30, 2018 19:54

Fix formatting issues

1747131

Signed-off-by: Raul Gutierrez Segales <[email protected]>

rgs1 force-pushed the merge-request-info-dynamic-metadata-for-lb branch from 4fb6b8c to 1747131 Compare April 30, 2018 20:09

Raul Gutierrez Segales added 2 commits April 30, 2018 13:48

Fix more tests

be4fe35

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Add a test for overriding metadata matcha via dynamic metadata

f1f12e0

Signed-off-by: Raul Gutierrez Segales <[email protected]>

zuercher requested changes May 1, 2018

View reviewed changes

Raul Gutierrez Segales added 2 commits May 1, 2018 13:52

Handle route_entry_->metadataMatchCriteria() == nullptr

b261a23

Also: * add a test for the above * add default return value for MockRequestInfo.dynamicMetadata Signed-off-by: Raul Gutierrez Segales <[email protected]>

Fix formatting

663c922

Signed-off-by: Raul Gutierrez Segales <[email protected]>

zuercher previously approved these changes May 1, 2018

View reviewed changes

htuch suggested changes May 3, 2018

View reviewed changes

Improve method docs.

b24da82

Also, annotate with @param and @return. Signed-off-by: Raul Gutierrez Segales <[email protected]>

rgs1 dismissed zuercher’s stale review via b24da82 May 3, 2018 03:37

mattklein123 reviewed May 3, 2018

View reviewed changes

Return the cached result of metadataMatchCriteria()

d29a4a5

Signed-off-by: Raul Gutierrez Segales <[email protected]>

htuch self-assigned this May 3, 2018

htuch reviewed May 3, 2018

View reviewed changes

mattklein123 approved these changes May 4, 2018

View reviewed changes

htuch approved these changes May 4, 2018

View reviewed changes

htuch merged commit 2d25067 into envoyproxy:master May 4, 2018

jannes mentioned this pull request Jul 15, 2021

dynamic routing with subset selectors k8sgateway/k8sgateway#4959

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dynamicMetadata to construct metadataMatchCriteria() #3254

Use dynamicMetadata to construct metadataMatchCriteria() #3254

rgs1 commented Apr 30, 2018

zuercher Apr 30, 2018

rgs1 Apr 30, 2018

zuercher Apr 30, 2018

rgs1 Apr 30, 2018

rgs1 Apr 30, 2018

rgs1 Apr 30, 2018

zuercher Apr 30, 2018

rgs1 Apr 30, 2018

rgs1 commented Apr 30, 2018

zuercher left a comment

zuercher May 1, 2018

zuercher May 1, 2018

rgs1 commented May 1, 2018

htuch left a comment

htuch May 3, 2018

htuch May 3, 2018

rgs1 commented May 3, 2018

mattklein123 left a comment

mattklein123 May 3, 2018

rgs1 May 3, 2018

rgs1 May 3, 2018

zuercher May 3, 2018

htuch May 3, 2018

rgs1 commented May 3, 2018

htuch May 3, 2018

rgs1 May 4, 2018

mattklein123 left a comment

htuch left a comment


		EXPECT_CALL(callbacks_.request_info_, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata));

Use dynamicMetadata to construct metadataMatchCriteria() #3254

Use dynamicMetadata to construct metadataMatchCriteria() #3254

Conversation

rgs1 commented Apr 30, 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

rgs1 commented Apr 30, 2018

zuercher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgs1 commented May 1, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgs1 commented May 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgs1 commented May 3, 2018

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

htuch left a comment

Choose a reason for hiding this comment