rds: expose path match criterion via route entry#2531
rds: expose path match criterion via route entry#2531htuch merged 4 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Matt Rice <mattrice@google.com>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
IDK, this goes back to our earlier internal discussion on whether we should just persist the entire RouteConfiguration and make it available via const& to the access log.
In this case, it's not that useful to be given the entire RouteConfiguration, as you only want the matching bits, but the principle of just passing a reference to that part of the RouteConfiguration could still hold, rather than having to recover this as some C++ type.
@mattklein123 thoughts here? Seems it would be useful to persist xDS config for other reasons, such as #2421.
Sure, either way. |
Signed-off-by: Matt Rice <mattrice@google.com>
| } | ||
| bool includeVirtualHostRateLimits() const override { return include_vh_rate_limits_; } | ||
| const envoy::api::v2::core::Metadata& metadata() const override { return metadata_; } | ||
| const PathMatchCriterion& pathMatchCriterion() const override { return *this; } |
There was a problem hiding this comment.
I'm wondering if I've forgotten how to read C++ :) Can you explain why this is needed (or even how it passes type checking?).
There was a problem hiding this comment.
Yeah, so the inheritance structure is a little convoluted here. RouteEntryImplBase inherits from RouteEntry, Matchable, and now PathMatchCriterion (among others). It has three derived classes (PrefixRouteEntryImpl, PathRouteEntryImpl, and RegexRouteEntryImpl) that implement the Matchable and PathMatchCriterion abstract methods since they implement all of the match-type specific logic. So, for the pathMatchCriterion() method (part of the RouteEntry interface), it just returns itself as the PathMatchCriterion since it implements that interface. Does that help clarify things?
There was a problem hiding this comment.
Does it ever need to be directly instantiated (i.e. be concrete)? Or do we only use derived classes? If the latter, it seems we don't need to define it.
There was a problem hiding this comment.
The intent of the class was to group the idea of a route match type and a matcher since one is needed to interpret the other (like a virtualized POD struct) - as opposed to polluting the RouteEntry interface (which is already starting to feel a little big) with a routeMatchType() method and a routeMatchString() method or something. I'm happy to promote them to the RouteEntry interface if that seems like a net gain, though.
| ASSERT_NE(nullptr, route); | ||
| auto route_entry = route->routeEntry(); | ||
| ASSERT_NE(nullptr, route_entry); | ||
| auto& match_criterion = route_entry->pathMatchCriterion(); |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
LGTM, just some minor comments.
Signed-off-by: Matt Rice <mattrice@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
4 participants
Description:
Expose the path match string as well as the type (regex, exact match, or prefix) through
RouteEntryusing a new object calledPathMatchCriterionfor the purpose of using this information in access logs.Risk Level: Low
Testing: Unit tested getters.