router: add new path-splitting prefix matcher by tpetkov-VMW · Pull Request #20145 · envoyproxy/envoy

tpetkov-VMW · 2022-02-28T21:45:21Z

Commit Message: Extend RouteMatch with a new field for subpath matching
Additional Description: The new field would allow more efficient generation of routes, replacing pairs of path+prefix routes into one path_separated_prefix route
Risk Level: Low
Testing: Unit test
Docs Changes: inline
Release Notes: Added
Fixes #18148

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

repokitteh-read-only · 2022-02-28T21:45:25Z

Hi @tpetkov-VMW, 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: #20145 was opened by tpetkov-VMW.

see: more, trace.

repokitteh-read-only · 2022-02-28T21:45:31Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20145 was opened by tpetkov-VMW.

see: more, trace.

lambdai

Thank you! Almost there

source/common/router/config_impl.cc

source/extensions/filters/http/jwt_authn/matcher.cc

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

tpetkov-VMW · 2022-03-01T17:11:00Z

Not sure about the naming, path_separated_prefix seems long.
Thinking about subpath as an alternative

lambdai

Thank you! The test cases are awesome!

htuch · 2022-03-03T04:18:44Z

I'm on the fence here on whether to do this vs the other alternatives in #18148. @kyessenov @snowp

kyessenov · 2022-03-03T05:43:58Z

Until we can use the matcher API to select routes, I don't see another way to support this without adding API fields. So I think the stumbling block is moving towards matchers in RDS, and I'm not up to date on that area.

htuch · 2022-03-10T01:56:43Z

@snowp can you help out here with matcher + RouteConfiguration?

snowp · 2022-03-10T14:47:40Z

It is possible today to use the new matching structure via RDS (#17633), so if that was the main perceived blocker perhaps this should go into the new matcher framework? Or are there other reasons to want to continue to add to the legacy route matcher?

tpetkov-VMW · 2022-03-14T09:36:46Z

It is possible today to use the new matching structure via RDS (#17633), so if that was the main perceived blocker perhaps this should go into the new matcher framework? Or are there other reasons to want to continue to add to the legacy route matcher?

Adding it to the legacy route matcher would give users a more easily consumable feature.
Frankly, the new matching structure is still confusing to me, and that's the main reason I chose the legacy routes.
I could try to open another PR for the RDS version and close this one, or perhaps they could coexist?

phlax · 2022-03-21T14:18:35Z

@tpetkov-VMW this is failing on version history formatting - but i guess you are waiting for feedback before proceeding/fixing

/wait-any

tpetkov-VMW · 2022-03-21T14:50:52Z

I'm not sure what to do, and whether this patch is desired at all.
I'll fix what's needed to pass the checks, so the PR is ready to go.

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

htuch · 2022-03-23T03:18:57Z

@tpetkov-VMW I've discussed this with @envoyproxy/api-shepherds and we're in-principle good with moving ahead with this PR. The consensus is that we will continue to support and evolve the existing matchers (and ideally integrate them as leaf functions into the generalized match trees).

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

htuch · 2022-03-23T03:21:28Z

/wai

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

adisuissa

Thanks, looks good!
Left a few nits.
/wait

source/common/router/config_impl.h

source/common/router/config_impl.cc

source/extensions/filters/http/jwt_authn/matcher.cc

source/common/router/config_impl.h

source/common/router/config_impl.cc

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

adisuissa

Thanks, LGTM!
/lgtm api
LGTM code changes.
Can you please add release-notes?

adisuissa · 2022-03-28T13:29:09Z

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2022-03-28T13:29:13Z

@envoyproxy/senior-maintainers assignee is @htuch

🐱

Caused by: a #20145 (comment) was created by @adisuissa.

see: more, trace.

tpetkov-VMW · 2022-03-28T13:56:42Z

Can you please add release-notes?

I've added them in /docs/root/version_history/current.rst but not sure what to write in the comment at the top
Is just "inline" okay?

adisuissa · 2022-03-28T14:04:40Z

Can you please add release-notes?

I've added them in /docs/root/version_history/current.rst but not sure what to write in the comment at the top Is just "inline" okay?

Yep. I typically write "Added", unless there's some special doc that was added.

BTW: please merge main.

soulxu · 2022-03-29T08:07:34Z

source/common/router/config_impl.cc

+    return nullptr;
+  }
+  absl::string_view path = Http::PathUtil::removeQueryAndFragment(headers.getPathValue());
+  if (path.size() >= prefix_.size() && path_matcher_->match(path) &&


The path_patcher_ will did the invoke Http::PathUtil::removeQueryAndFragment on the path again.

envoy/source/common/common/matchers.cc

Line 126 in b77651d

return matcher_.match(Http::PathUtil::removeQueryAndFragment(path));

Not sure whether worth to use envoy::type::matcher::v3::StringMatcher here directly. Leave this to other reviewers.

Yes, you are correct.
I think the alternative is to not invoke path_matcher_->match method, which could result in inconsistencies in the future if this method is updated.

It seemed like the most concise way to do it, without expanding the StringMatcher definition.

soulxu · 2022-03-29T08:08:05Z

LGTM

adisuissa

LGTM thanks!
Needs approval from senior-maintainer.

adisuissa · 2022-03-29T13:56:30Z

source/common/router/config_impl.cc

+    return nullptr;
+  }
+  absl::string_view path = Http::PathUtil::removeQueryAndFragment(headers.getPathValue());
+  if (path.size() >= prefix_.size() && path_matcher_->match(path) &&


Yes, you are correct.
I think the alternative is to not invoke path_matcher_->match method, which could result in inconsistencies in the future if this method is updated.

htuch · 2022-03-30T04:01:43Z

source/extensions/filters/http/jwt_authn/matcher.cc

+      return false;
+    }
+    absl::string_view path = Http::PathUtil::removeQueryAndFragment(headers.getPathValue());
+    if (path.size() >= prefix_.size() && path_matcher_->match(path) &&


I'm a bit unhappy we have duplicative logic here @qiwzhang. I've probably complained before and this is unrelated to this PR, so just an observation, but by having to copy+paste all our matching logic in two places, we are leaving open a significant opportunity for drift and check vs. use semantic differences between this and routing.

htuch

Looks good, my main concern is not specific to this PR but that we are having to constantly copy+paste our match logic across this and the JWT filter. This is related to the generic matcher discussion @envoyproxy/api-shepherds @snowp, since if we were using that, we would be able to just implement this once.

test/common/router/config_impl_test.cc

htuch · 2022-03-30T04:04:34Z

test/extensions/filters/http/jwt_authn/matcher_test.cc

+  TestUtility::loadFromYaml(config, rule);
+  MatcherConstPtr matcher = Matcher::create(rule);
+  auto headers = TestRequestHeaderMapImpl{{":path", "/api"}};
+  EXPECT_TRUE(matcher->matches(headers));


It would be good to use consistent test cases that are easy to correlate across the JWT and core router.

Do you mean "equalize the test inputs per test case"?
I'll start with that, and if more work needs to be done - just elaborate on it.

Not sure about the definition of equalize :) Anything you can do to make tests more consistent between JWT and core router. Since you're taking on #20578 I'm fine with punting to that if you prefer to do it there.

I made it so the jwt test cases are a subset of the router ones, and the test strings are pretty much the same.
They don't look exactly the same, but the correlation should be obvious.

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

htuch

LGTM, thanks!

The new field would allow more efficient generation of routes, replacing pairs of path+prefix routes into one path_separated_prefix route Risk Level: Low Testing: Unit test Docs Changes: inline Release Notes: Added Fixes envoyproxy#18148 Signed-off-by: Toma Petkov <tpetkov@vmware.com>

tpetkov-VMW added 3 commits February 24, 2022 11:33

header: Add path-splitting prefix matcher

04559b0

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

Add basic test and doc for route_components.proto

bb31a66

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

Fix paths with query parameter. Validate in route_components.proto

928235a

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

tpetkov-VMW requested a review from lizan as a code owner February 28, 2022 21:45

repokitteh-read-only bot added v2-freeze api labels Feb 28, 2022

repokitteh-read-only bot assigned htuch Feb 28, 2022

lambdai suggested changes Feb 28, 2022

View reviewed changes

source/common/router/config_impl.cc Outdated Show resolved Hide resolved

source/extensions/filters/http/jwt_authn/matcher.cc Outdated Show resolved Hide resolved

tpetkov-VMW and others added 2 commits March 1, 2022 15:30

Fix uris with fragments, add tests for jwt_authn matcher

6bc04ba

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

Merge branch 'main' into path_splitting_prefix_match

17c404f

lambdai previously approved these changes Mar 2, 2022

View reviewed changes

repokitteh-read-only bot added the waiting:any label Mar 21, 2022

repokitteh-read-only bot removed the waiting:any label Mar 21, 2022

tpetkov-VMW and others added 2 commits March 21, 2022 16:52

Merge branch 'envoyproxy:main' into path_splitting_prefix_match

62988f1

Fix version history formatting

e71d8d7

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

tpetkov-VMW dismissed lambdai’s stale review via e71d8d7 March 21, 2022 16:07

Fix proto formatting

4697277

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

htuch suggested changes Mar 23, 2022

View reviewed changes

api/envoy/config/route/v3/route_components.proto Outdated Show resolved Hide resolved

api/envoy/config/route/v3/route_components.proto Outdated Show resolved Hide resolved

Minor docs fix

6a78c3e

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

repokitteh-read-only bot added the api label Mar 25, 2022

adisuissa reviewed Mar 25, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 25, 2022

Simplify matching and more tests

0aa1804

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

repokitteh-read-only bot removed the waiting label Mar 25, 2022

Merge branch 'envoyproxy:main' into path_splitting_prefix_match

4958eb7

adisuissa reviewed Mar 28, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Mar 28, 2022

Merge branch 'main' into path_splitting_prefix_match

aeb6f0e

soulxu reviewed Mar 29, 2022

View reviewed changes

adisuissa previously approved these changes Mar 29, 2022

View reviewed changes

htuch reviewed Mar 30, 2022

View reviewed changes

htuch previously approved these changes Mar 30, 2022

View reviewed changes

htuch mentioned this pull request Mar 30, 2022

Refactor JWT matching logic #20578

Open

Match up router and jwt_authn test cases

dd8972a

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

tpetkov-VMW dismissed stale reviews from htuch and adisuissa via dd8972a March 31, 2022 16:08

Fix function name: CreateMatcher -> createMatcher

2202337

Signed-off-by: Toma Petkov <tpetkov@vmware.com>

htuch approved these changes Apr 1, 2022

View reviewed changes

htuch merged commit 8f9e112 into envoyproxy:main Apr 1, 2022

skriss mentioned this pull request Apr 1, 2022

make use of Envoy's path-based prefix matching projectcontour/contour#4442

Closed

dddddai mentioned this pull request Apr 4, 2022

Adopt efficient path based routing istio/istio#38233

Merged

11 tasks

tpetkov-VMW deleted the path_splitting_prefix_match branch April 5, 2022 13:21

Conversation

tpetkov-VMW commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Feb 28, 2022

Uh oh!

repokitteh-read-only bot commented Feb 28, 2022

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tpetkov-VMW commented Mar 1, 2022

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Mar 3, 2022

Uh oh!

kyessenov commented Mar 3, 2022

Uh oh!

htuch commented Mar 10, 2022

Uh oh!

snowp commented Mar 10, 2022

Uh oh!

tpetkov-VMW commented Mar 14, 2022

Uh oh!

phlax commented Mar 21, 2022

Uh oh!

tpetkov-VMW commented Mar 21, 2022

Uh oh!

htuch commented Mar 23, 2022

Uh oh!

Uh oh!

Uh oh!

htuch commented Mar 23, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa commented Mar 28, 2022

Uh oh!

repokitteh-read-only bot commented Mar 28, 2022

Uh oh!

tpetkov-VMW commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adisuissa commented Mar 28, 2022

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!

soulxu commented Mar 29, 2022

Uh oh!

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

htuch 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

tpetkov-VMW commented Feb 28, 2022 •

edited

Loading

tpetkov-VMW commented Mar 28, 2022 •

edited

Loading