Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add efficient path-based prefix matching #18148

Closed
howardjohn opened this issue Sep 16, 2021 · 12 comments · Fixed by #20145
Closed

Add efficient path-based prefix matching #18148

howardjohn opened this issue Sep 16, 2021 · 12 comments · Fixed by #20145
Labels

Comments

@howardjohn
Copy link
Contributor

Title: Add efficient path-based prefix matching

Description:
The Kubernetes Ingress spec, and soon the gateway-api spec (kubernetes-sigs/gateway-api#866) implement a different form of prefix path matching than Envoy. From the spec:

Prefix: Matches based on a URL path prefix split by /. Matching is case sensitive and done on a path element by element basis. A path element refers to the list of labels in the path split by the / separator. A request is a match for path p if every p is an element-wise prefix of p of the request path.

Essentially, if we want to match prefix=/foo in this manner we have two options:

The first option has a runtime cost in route matching. The second has a cost in Envoy+Control plane during XDS processing.

Given how common this is/will be in Envoy users, it would be great if there was a more efficient way to control this. For example, setting a bool var like case_sensitive to control the behavior or defining some new type of path_specifier.

cc @jpeach @youngnick @lambdai

@howardjohn howardjohn added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Sep 16, 2021
@jpeach
Copy link
Contributor

jpeach commented Sep 16, 2021

Xref kubernetes-sigs/gateway-api#869

@youngnick
Copy link

As @howardjohn said above, we've chosen to go with the "replace with regex" option for now in Contour, but strong +1 that this would be very nice for us.

@alyssawilk alyssawilk removed the triage Issue requires triage label Sep 20, 2021
@alyssawilk
Copy link
Contributor

cc @htuch @envoyproxy/api-shepherds for thoughts

@mattklein123
Copy link
Member

We already do support control over route case sensitivity in matching.

What is the actual feature request here? I'm a little confused. Why doesn't prefix=/foo work for your example?

@howardjohn
Copy link
Contributor Author

The problem is prefix=/foo is a string prefix. The spec has "element-wise"/label prefix. So in the spec, /foo would match /foo/bar but NOT /foobar. In envoy, it would match both.

This is semantically equivalent to existing Envoy matching capabilities: prefix=/foo/ OR exact=/foo, or converting to a regex (https://github.com/projectcontour/contour/blob/2b3376449bedfea7b8cea5fbade99fb64009c0f6/internal/envoy/v3/route.go#L78). Both of these incur a performance penality

@mattklein123
Copy link
Member

OK, so you want a "path splitting prefix mode?" That seems simple enough.

@mattklein123 mattklein123 added area/http help wanted Needs help! and removed enhancement Feature requests. Not bugs or questions. labels Sep 20, 2021
@htuch
Copy link
Member

htuch commented Sep 21, 2021

There's some overlap with #7763, which also provides matching with path fragments.

@tpetkov-VMW
Copy link
Contributor

I can try to do this, if it isn't obsoleted by the issue above.

@kyessenov
Copy link
Contributor

It would be nice to use unified matchers here. I imagine we'd need some sublinear implementation to deal with categories of templates (like in API gateway cases).

@tpetkov-VMW
Copy link
Contributor

It would be nice to use unified matchers here. I imagine we'd need some sublinear implementation to deal with categories of templates (like in API gateway cases).

@kyessenov, you mean having a new match pattern here?
api/envoy/type/matcher/v3/string.proto
As far as I could tell, this would need a change in the xds project as well:
https://github.com/cncf/xds/blob/main/xds/type/matcher/v3/string.proto
What is the process of submitting code in the xds project?
Just raise an issue + PR there and then bump in api/bazel/repository_locations.bzl ?

@kyessenov
Copy link
Contributor

I was thinking something like https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/common/matcher/v3/matcher.proto#config-common-matcher-v3-matcher-matchertree but with a custom matcher for templated URLs. There is already a prefix matcher but I think it's the simple prefix.

@tpetkov-VMW
Copy link
Contributor

I was thinking something like https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/common/matcher/v3/matcher.proto#config-common-matcher-v3-matcher-matchertree but with a custom matcher for templated URLs. There is already a prefix matcher but I think it's the simple prefix.

Sounds like straight up implementing #7763
I've a much simpler prototype that just does the path-based prefix matching in the simplest way possible, albeit with some duplicated code for now.
I'll open a PR, so people can check out the code, and if it's not the correct thing to do, It should at least be easier to discuss.

htuch pushed a commit that referenced this issue Apr 1, 2022
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>
ravenblackx pushed a commit to ravenblackx/envoy that referenced this issue Jun 8, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants