router: integrate matching API #17633

snowp · 2021-08-09T14:52:46Z

Adds support for using the matching API in the route table. This wires up using the generic match as part of a virtual host, making
it possible to define a match tree that results in Route actions that reuses the same routing actions currently in use by the router.

Risk Level: Medium, refactors route config but should be a net new feature
Testing: UTs (needs integration testing)
Docs Changes: Updates to the http_routing and matching_api docs
Release Notes: n/a (will add)
Platform Specific Features: n/a

Prior to this PR the MatchTreeFactory requried a ServerFactoryContext in order to pass this to the action factories. This is not available in all contexts, and there are other possible use cases where we might need to pass additional information to the match tree factory (e.g. the router might need to pass the parent vhost configuration). This PR introduces a paramterized action context, allowing each usage of the MatchTree to define its own data that should be presented to the action factories. This also has the nice benefit of partitioning the action factories per context: each unique ActionFactoryContext defines a new factory type, ensuring that actions defined for a HTTP filter context won't be conflated with actions defined for another context. Signed-off-by: Snow Pettersen <snowp@lyft.com>

Signed-off-by: Snow Pettersen <snowp@lyft.com>

…matcher Signed-off-by: Snow Pettersen <snowp@lyft.com>

Signed-off-by: Snow Pettersen <snowp@lyft.com>

…matcher Signed-off-by: Snow Pettersen <snowp@lyft.com>

Signed-off-by: Snow Pettersen <snowp@lyft.com>

mattklein123 · 2021-09-01T21:37:12Z

The main open question that remains imo is around whether we should runtime guard this like we do for ExtensionWithMatcher, or if we're at a point where we feel like we can enable the matching API without an explicit opt in (in which case we should probably remove the runtime flag requirement from ExtensionWithMatcher).

Per offline convo, my mild preference is to mark use of this field as experimental and exempt from CVE handling, and force opt-in via a runtime variable. This can be used for initial testing. Once we feel it's gotten a bit of use we can remove the gate.

Open to suggestions (probably from @mattklein123) about what kind of test coverage we want here. The majority of the routing logic is still the old code, so the single test I have that demonstrates that we can resolve the route covers all the new code paths, but perhaps we want to verify other flows.

I think as long as we have coverage of the new code it's probably fine, but just for completeness I would try to have some integration tests of the sub-linear matching flows if possible?

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp · 2021-09-03T14:24:41Z

This should be ready for code review now

mattklein123

Thanks very cool. Can you add more tests for some of the configuration error cases so it's easier to understand the error flows?

/wait

mattklein123 · 2021-09-07T16:47:43Z

source/common/router/config_impl.cc

-    }
+  if (virtual_host.has_matcher() &&
+      !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.experimental_matching_api")) {
+    throw EnvoyException("Experimental matching API is not enabled");


Can you make this error message more expressive such as which runtime key to set, etc.?

mattklein123 · 2021-09-07T16:50:10Z

source/common/router/config_impl.cc

+  } else {
+    for (const auto& route : virtual_host.routes()) {
+      if (!route.has_match()) {
+        throw EnvoyException("RouteValidationError.Match");


What does this error mean? Can you make it more descriptive and also maybe add more comments here?

I actually don't think we need this any more after we moved to evaluating the resolved Route, let me just remove this

mattklein123 · 2021-09-07T16:50:49Z

source/common/router/config_impl.cc

+    auto match = Matcher::evaluateMatch<Http::HttpMatchingData>(*matcher_, data);
+
+    if (match.result_) {
+      ASSERT(dynamic_cast<RouteMatchAction*>(match.result_.get()));


Can you add a comment on where this is validated?

mattklein123 · 2021-09-07T16:50:55Z

source/common/router/config_impl.cc

+    // TODO(snowp): Add logger
+    // ENVOY_LOG(debug, "failed to match incoming request: {}", match.result_);


Just do this now?

mattklein123 · 2021-09-07T16:52:47Z

docs/root/intro/arch_overview/http/http_routing.rst

+the match criteria of the resolved Route. Path matching done during the match tree traversal does not contribute
+to path rewrites.
+
+The only inputs supported are request headers (via `envoy.type.matcher.v3.HttpRequestHeaderMatchInput`). See


Where is this validated and what error message does the user get if they configure something else accidentally?

Same question about the action config which I guess will only currently work for route proto lookup?

Seems like this was not validated, now we are with the following error message:

requirement violation while creating route match tree: INVALID_ARGUMENT: Route table can only match on request headers, saw type.googleapis.com/envoy.type.matcher.v3.HttpResponseHeaderMatchInput

For action configs the only registered factory for ActionFactory<RouteActionContext> is RouteMatchActionFactory, so all others would result in the typical "no factory found"

Signed-off-by: Snow Pettersen <snowp@lyft.com>

mattklein123

Thanks excited to see this land. One small comment.

/wait

mattklein123 · 2021-10-12T15:34:52Z

api/envoy/config/route/v3/route_components.proto

+  // Note that this API is a work in progress and must be explicitly enabled by setting the runtime
+  // "envoy.reloadable_features.experimental_matching_api" to "true".


IMO we don't need this anymore given the WIP annotation and the warning about this?

Sure yeah I'll happily remove this. Might also be okay to remove this flag requirement from the filter integration as well? Can do that in a follow up, would love to get this PR in

Signed-off-by: Snow Pettersen <snowp@lyft.com>

mattklein123

Thanks! Awesome work! Fine to do the other removal in a follow up.

snowp added 30 commits June 17, 2021 13:02

fix build

44e162b

Signed-off-by: Snow Pettersen <snowp@lyft.com>

fix more tests

7c35634

Signed-off-by: Snow Pettersen <snowp@lyft.com>

router: poc of integration match api

0586cb1

Signed-off-by: Snow Pettersen <snowp@lyft.com>

api: move generic matcher proto to its own package

50fe3e8

Signed-off-by: Snow Pettersen <snowp@lyft.com>

deprecation note

81aa2bc

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Merge remote-tracking branch 'envoy/main' into move-generic-matcher

7ff4d44

Signed-off-by: Snow Pettersen <snowp@lyft.com>

add new file, make factory generic, add test

7acbde0

Signed-off-by: Snow Pettersen <snowp@lyft.com>

fix docs

98100c7

Signed-off-by: Snow Pettersen <snowp@lyft.com>

add alpha exception

fd00924

Signed-off-by: Snow Pettersen <snowp@lyft.com>

fix examples

28bf418

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Merge branch 'main' of github.com:envoyproxy/envoy into move-generic-…

f4a6292

…matcher Signed-off-by: Snow Pettersen <snowp@lyft.com>

use udpa version

a4b752b

Signed-off-by: Snow Pettersen <snowp@lyft.com>

fix examples

41f07e4

Signed-off-by: Snow Pettersen <snowp@lyft.com>

update versioning

ba371e8

Signed-off-by: Snow Pettersen <snowp@lyft.com>

format

0fb80ba

Signed-off-by: Snow Pettersen <snowp@lyft.com>

update to use xds namespaced protos

9d24e1f

Signed-off-by: Snow Pettersen <snowp@lyft.com>

fix docs

f6d9af7

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Merge branch 'main' of github.com:envoyproxy/envoy into move-generic-…

da0454b

…matcher Signed-off-by: Snow Pettersen <snowp@lyft.com>

use xds latest main

02dc3f0

Signed-off-by: Snow Pettersen <snowp@lyft.com>

use xds main

853a1bd

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Merge remote-tracking branch 'envoy/main' into move-generic-matcher

54079e5

Signed-off-by: Snow Pettersen <snowp@lyft.com>

fix release date

7863b17

Signed-off-by: Snow Pettersen <snowp@lyft.com>

fix more stringmatcherimpl ctors

4ebb008

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Merge remote-tracking branch 'envoy/main' into move-generic-matcher

09c782e

Signed-off-by: Snow Pettersen <snowp@lyft.com>

more fixes

199a8b1

Signed-off-by: Snow Pettersen <snowp@lyft.com>

more build fixes

73e1848

Signed-off-by: Snow Pettersen <snowp@lyft.com>

cleanups, fix composite example

4c9f5e6

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Merge branch 'move-generic-matcher' into router-matcher-poc

aae8d46

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Merge remote-tracking branch 'envoy/main' into router-matcher-poc

2381d38

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp added 2 commits September 2, 2021 16:01

integration test, runtime flag

2c2be12

Signed-off-by: Snow Pettersen <snowp@lyft.com>

remove trailing whitespace

082376d

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp marked this pull request as ready for review September 2, 2021 16:55

mattklein123 requested changes Sep 7, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 7, 2021

snowp added 4 commits September 22, 2021 18:52

wip

c3149c4

Signed-off-by: Snow Pettersen <snowp@lyft.com>

validate data input

82a5fdd

Signed-off-by: Snow Pettersen <snowp@lyft.com>

format

269954c

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Merge remote-tracking branch 'envoy/main' into router-matcher-poc

8083421

Signed-off-by: Snow Pettersen <snowp@lyft.com>

repokitteh-read-only bot removed the waiting label Sep 29, 2021

use wip instead of alpha

418a270

Signed-off-by: Snow Pettersen <snowp@lyft.com>

markdroth mentioned this pull request Oct 1, 2021

thrift: adding xDS RDS support for thrift proxy. #17631

Closed

Merge remote-tracking branch 'envoy/main' into router-matcher-poc

9d72842

Signed-off-by: Snow Pettersen <snowp@lyft.com>

mattklein123 added the waiting label Oct 5, 2021

Merge remote-tracking branch 'origin/main' into router-matcher-poc

694d945

Signed-off-by: Snow Pettersen <snowp@lyft.com>

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

snowp added 3 commits October 9, 2021 03:35

improve coverage

50080bd

Signed-off-by: Snow Pettersen <snowp@lyft.com>

format

a95c67c

Signed-off-by: Snow Pettersen <snowp@lyft.com>

improve coverage

1d16eba

Signed-off-by: Snow Pettersen <snowp@lyft.com>

mattklein123 requested changes Oct 12, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 12, 2021

remove runtime flag

71c4c5a

Signed-off-by: Snow Pettersen <snowp@lyft.com>

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

mattklein123 approved these changes Oct 12, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Oct 12, 2021

snowp merged commit 94d0013 into envoyproxy:main Oct 13, 2021

zhxie mentioned this pull request Oct 27, 2021

udp: add router for UDP proxy #18791

Merged

snowp mentioned this pull request Mar 10, 2022

router: add new path-splitting prefix matcher #20145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

router: integrate matching API #17633

router: integrate matching API #17633

snowp commented Aug 9, 2021

mattklein123 commented Sep 1, 2021

snowp commented Sep 3, 2021

mattklein123 left a comment

mattklein123 Sep 7, 2021

mattklein123 Sep 7, 2021

snowp Sep 10, 2021

mattklein123 Sep 7, 2021

mattklein123 Sep 7, 2021

mattklein123 Sep 7, 2021

snowp Sep 29, 2021

mattklein123 left a comment

mattklein123 Oct 12, 2021

snowp Oct 12, 2021

mattklein123 left a comment

		// TODO(snowp): Add logger
		// ENVOY_LOG(debug, "failed to match incoming request: {}", match.result_);

		// Note that this API is a work in progress and must be explicitly enabled by setting the runtime
		// "envoy.reloadable_features.experimental_matching_api" to "true".

router: integrate matching API #17633

router: integrate matching API #17633

Conversation

snowp commented Aug 9, 2021

mattklein123 commented Sep 1, 2021

snowp commented Sep 3, 2021

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

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

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