load balancer: thread metadata matches through LoadBalancerContext#1844
load balancer: thread metadata matches through LoadBalancerContext#1844htuch merged 5 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
htuch
left a comment
There was a problem hiding this comment.
Looks good, just a few clarifications.
| } | ||
|
|
||
| std::vector<MetadataMatchCriterionConstSharedPtr> | ||
| MetadataMatchCriteriaImpl::extractMetadataMatchCriteria(const MetadataMatchCriteriaImpl* parent, |
There was a problem hiding this comment.
Can we document somewhere (maybe constructor) the significance of parent-child relationship of MetadataMatchCriteria? I think this bit is non-intuitive, the mechanics seem fine though.
There was a problem hiding this comment.
I think the commit message actually has this info, so probably just inline it into some comment as well.
There was a problem hiding this comment.
I did some of this, but let me refactor it a bit. I think it'll make more sense if the merging happens in an explicit call to a member function on the parent instead of using a special constructor.
| MetadataMatchCriteriaImpl::extractMetadataMatchCriteria(const MetadataMatchCriteriaImpl* parent, | ||
| const ProtobufWkt::Struct& matches) { | ||
| std::vector<MetadataMatchCriterionConstSharedPtr> v; | ||
| std::unordered_map<std::string, std::size_t> existing; |
There was a problem hiding this comment.
Size seems to be providing some notion of initial ordering in the loop that populates it below, and is then used to find the place in v that a parent entry would be, but a comment to explain would help.
| } | ||
| } | ||
|
|
||
| std::sort( |
There was a problem hiding this comment.
Explain why sorting, with link to MD design doc as appropriate.
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
| // single cluster, pointing back to the parent. | ||
| // single cluster, pointing back to the parent. Metadata criteria | ||
| // from the weighted cluster (if any) are merged with and override | ||
| // the criteria from the route. |
There was a problem hiding this comment.
Quick question: isn't route more specific than cluster, so would it not make sense to have the merge/override the other way around? I don't have any clue here, just asking.
There was a problem hiding this comment.
I think the argument can be made either way.
The way I arrived at this is a request is mapped to a Route, which further maps the request to a Cluster and that it makes sense for the metadata criteria to be built up in the same order.
That said, we don't plan on depending on this behavior (we may use both fields, but the sets of keys in each will be disjoint).
There was a problem hiding this comment.
OK, I have no strong opinion here, was just curious.
| EXPECT_EQ((*it)->value().value().string_value(), std::string("v1")); | ||
| it++; | ||
|
|
||
| EXPECT_EQ((*it)->name(), std::string("b")); |
There was a problem hiding this comment.
Do you really need std::string here? I would have guessed there would be implicit constructor invocation. This is actually a bit dangerous for the Google import, since our proto string type is not std::string, we rely in various places (such as what's being done here) for the implicit constructor to get us to our internal string type.
There was a problem hiding this comment.
Hm. I recall getting a compiler error that required this to fix it, but I can't reproduce it. I'll take them out.
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
…ion_requested_server_name_attribute Revert "Add connection requested server name attribute to TCP read filter"
**Description** For gemini-3-flash-preview, there are some additional reasoning efforts: https://docs.cloud.google.com/vertex-ai/generative-ai/docs/start/get-started-with-gemini-3#openai-example --------- Signed-off-by: yxia216 <yxia216@bloomberg.net> Co-authored-by: Ignasi Barrera <ignasi@tetrate.io> Co-authored-by: Dan Sun <dsun20@bloomberg.net>
3rd of 4 reviews split out from #1735.
Router::MetadataMatchCriteria is implemented. Instances of the implementation are created when a route's RouteAction has an "envoy.lb" section in metadata_match values. These are merged with additional "envoy.lb" metadata_match values from the WeightedCluster, if any. In the event of a collision, the WeightedCluster keys take precedence.
The LoadBalancerContext interface is modified to allow MetadataMatchCriteria to be accessed by load balancers. Router::Filter's implementation of the LoadBalancerContext is updated to return the selected route's criteria, if any.