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

Corner case in HTTPRouteRule match order when RegularExpression is used #1770

Closed
spacewander opened this issue Mar 6, 2023 · 5 comments · Fixed by #1855
Closed

Corner case in HTTPRouteRule match order when RegularExpression is used #1770

spacewander opened this issue Mar 6, 2023 · 5 comments · Fixed by #1855
Assignees
Labels
area/conformance priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-blocker MUST be completed to complete the milestone
Milestone

Comments

@spacewander
Copy link
Contributor

What would you like to be added:

We need to add test cases to ensure the RegularExpression match happens after the other matches.

Why this is needed:

According to the HTTPRouteRule definition:
https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io%2fv1beta1.HTTPRouteRule

Proxy or Load Balancer routing configuration generated from HTTPRoutes MUST prioritize matches based on the following criteria, continuing on ties. Across all rules specified on applicable Routes, precedence must be given to the match with the largest number of:

Characters in a matching path.
Header matches.
Query param matches.

The routes are sorted by the "Characters in a matching path." first.

However, when the RegularExpression in https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.PathMatchType is involved, the order is ambiguous as the regex can be very specific.

For example, a regex /a/(.*)+ has more characters in a matching path than exact match /a/b, while it is similar to the path prefix /a/. If we strictly sort the route by the path, /a/(.*)+ will be matched before /a/b.

An idea to sort this problem is to ensure the regex match happens after the other matches.

@robscott
Copy link
Member

robscott commented Mar 7, 2023

I agree this is an important issue, thanks for catching it! Before we update conformance tests, we should likely work on an update to the spec to clarify the correct order of operations here. It may be sufficient to replace:

* Characters in a matching path.

With:

* Characters in a matching "Exact" path match
* Characters in a matching "Prefix" path match
* Characters in a matching "RegularExpression" path match

@shaneutt
Copy link
Member

shaneutt commented Mar 7, 2023

/help

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Mar 7, 2023
@sunjayBhatia
Copy link
Member

in case we're looking for input on the sorting order/precedence of different match types, in Contour we've historically had:

  • exact match gets most precedence, sorted by length (descending)
  • regex is next, since it could be more specific than a prefix (again sorted by length)
  • prefix match (again sorted by length)

we don't compare length of match string between different match types which is a distinction to point out in the docs if we match this behavior

@pleshakov
Copy link
Contributor

in case we're looking for input on the sorting order/precedence of different match types,

In NGINX, we have the following order:

  • exact match gets most precedence
  • regex match, which is the first match among the regexes in the order of appearance in the configuration file; In NGINX Kubernetes Gateway, we sort regex matches by their order of appearance in the corresponding HTTPRoutes + by the timestamp and namespace/name of the HTTPRoutes.
  • most specific prefix match

https://nginx.org/en/docs/http/ngx_http_core_module.html#location

@Xunzhuo
Copy link
Member

Xunzhuo commented Mar 8, 2023

/assign
/remove-help

@k8s-ci-robot k8s-ci-robot removed the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Mar 8, 2023
@shaneutt shaneutt moved this from Todo to In Progress in Gateway API: The Road to GA Mar 8, 2023
@shaneutt shaneutt added release-blocker MUST be completed to complete the milestone priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Mar 14, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Gateway API: The Road to GA Mar 27, 2023
arkodg pushed a commit to envoyproxy/gateway that referenced this issue Apr 5, 2024
…2579)

* Change route sorting order to Exact > RegularExpression > PathPrefix

kubernetes-sigs/gateway-api#1770
kubernetes-sigs/gateway-api#1855

Signed-off-by: Stéphane Cottin <[email protected]>
arkodg pushed a commit to arkodg/gateway that referenced this issue Apr 8, 2024
…nvoyproxy#2579)

* Change route sorting order to Exact > RegularExpression > PathPrefix

kubernetes-sigs/gateway-api#1770
kubernetes-sigs/gateway-api#1855

Signed-off-by: Stéphane Cottin <[email protected]>
(cherry picked from commit 11f56fd)
Signed-off-by: Arko Dasgupta <[email protected]>
Xunzhuo added a commit to envoyproxy/gateway that referenced this issue Apr 8, 2024
* Run certgen when upgrading (#2934)

run certgen when upgrading

Signed-off-by: huabing zhao <[email protected]>
(cherry picked from commit 62ecf15)
Signed-off-by: Arko Dasgupta <[email protected]>

* Fix: nil secret in resourceversiontable (#2982)

* fix nil secret in resourceversiontable

Signed-off-by: huabing zhao <[email protected]>

* check secrets in the xds result

Signed-off-by: huabing zhao <[email protected]>

---------

Signed-off-by: huabing zhao <[email protected]>
(cherry picked from commit e880439)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: add missing http filters to the http filter chain (#2970)

* fix: add missing http filters to the http filter chain

Signed-off-by: huabing zhao <[email protected]>

* refactor

Signed-off-by: huabing zhao <[email protected]>

* fix lint

Signed-off-by: huabing zhao <[email protected]>

* add comments

Signed-off-by: huabing zhao <[email protected]>

* remove refactor

Signed-off-by: huabing zhao <[email protected]>

* remove refactor

Signed-off-by: huabing zhao <[email protected]>

* fix gen

Signed-off-by: huabing zhao <[email protected]>

* fix lint

Signed-off-by: Huabing Zhao <[email protected]>

---------

Signed-off-by: huabing zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
(cherry picked from commit f699edf)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: allow websockets in url rewrite (#3022)

allow websockets in url rewrite

Signed-off-by: Jesse Haka <[email protected]>
Co-authored-by: zirain <[email protected]>
(cherry picked from commit 3d51933)
Signed-off-by: Arko Dasgupta <[email protected]>

* Set host for http health checker explicitly to avoid using the cluster name as host header for http health checking request. (#3057)

* Set host for http health checker explictly to avoid using the cluster name as host header for http health checking request

Signed-off-by: lemonlinger <[email protected]>

* fix broken tests

Signed-off-by: lemonlinger <[email protected]>

* fix health-check test case in xds translation

Signed-off-by: lemonlinger <[email protected]>

* Simplify code and concise comments

Signed-off-by: lemonlinger <[email protected]>

---------

Signed-off-by: lemonlinger <[email protected]>
(cherry picked from commit 8f450a9)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: do not create infra resources when missing translated listeners (#3043)

* fix: do not create infra resources when missing translated listeners

Signed-off-by: Karol Szwaj <[email protected]>

* remove empty line

Signed-off-by: Karol Szwaj <[email protected]>

* skip infra creation on empty listeners and log it

Signed-off-by: Karol Szwaj <[email protected]>

---------

Signed-off-by: Karol Szwaj <[email protected]>
(cherry picked from commit 36d7141)
Signed-off-by: Arko Dasgupta <[email protected]>

* Fix: double slashes in redirect URL (#2998)

* fix: double trailing splashs in redirect URL

Signed-off-by: huabing zhao <[email protected]>

* add e2e tests

Signed-off-by: huabing zhao <[email protected]>

* fix lint

Signed-off-by: huabing zhao <[email protected]>

* fix test

Signed-off-by: huabing zhao <[email protected]>

* fix test

Signed-off-by: huabing zhao <[email protected]>

* fix test

Signed-off-by: huabing zhao <[email protected]>

* fix test

Signed-off-by: huabing zhao <[email protected]>

* add e2e tests

Signed-off-by: huabing zhao <[email protected]>

* fix test

Signed-off-by: huabing zhao <[email protected]>

* revert

Signed-off-by: huabing zhao <[email protected]>

* use regex rewrite to generate the redirect url

Signed-off-by: huabing zhao <[email protected]>

* use regex rewrite to generate the redirect url

Signed-off-by: huabing zhao <[email protected]>

* use regex rewrite to generate the redirect url

Signed-off-by: huabing zhao <[email protected]>

* remove comments

Signed-off-by: huabing zhao <[email protected]>

* extract method

Signed-off-by: huabing zhao <[email protected]>

* address comments

Signed-off-by: huabing zhao <[email protected]>

---------

Signed-off-by: huabing zhao <[email protected]>
(cherry picked from commit ceb697f)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: Allow Policy to attach to multiple http listeners  (#2967)

* Fixing the clienttrafficpolicy validation.

Signed-off-by: Lior Okman <[email protected]>

* Make SecurityPolicy validate correctly.

Signed-off-by: Lior Okman <[email protected]>

* Reverted the SecurityPolicy validation - handled differently via
another feature.

Signed-off-by: Lior Okman <[email protected]>

* Updated the tests to reflect that this validation isn't required for SecurityPolicy

Signed-off-by: Lior Okman <[email protected]>

* Added some comments to explain the validation being performed.

Signed-off-by: Lior Okman <[email protected]>

* Updated the error message as requested in the review.

Signed-off-by: Lior Okman <[email protected]>

---------

Signed-off-by: Lior Okman <[email protected]>
(cherry picked from commit f9409e4)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: set path prefix for http ext auth service (#3018)

Signed-off-by: huabing zhao <[email protected]>
(cherry picked from commit 2882b7c)
Signed-off-by: Arko Dasgupta <[email protected]>

* Change route sorting order to Exact > RegularExpression > PathPrefix (#2579)

* Change route sorting order to Exact > RegularExpression > PathPrefix

kubernetes-sigs/gateway-api#1770
kubernetes-sigs/gateway-api#1855

Signed-off-by: Stéphane Cottin <[email protected]>
(cherry picked from commit 11f56fd)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: infraIR duplicate port translation for merged gateways (#3061)

* fix: duplicate port translation for merged gateways

Signed-off-by: Karol Szwaj <[email protected]>

* refactor to map

Signed-off-by: Karol Szwaj <[email protected]>

* rename map

Signed-off-by: Karol Szwaj <[email protected]>

* add seperate testcase

Signed-off-by: Karol Szwaj <[email protected]>

---------

Signed-off-by: Karol Szwaj <[email protected]>
(cherry picked from commit 29946b0)
Signed-off-by: Arko Dasgupta <[email protected]>

* translator: set SpawnUpstreamSpan to true (#3102)

* translator: set SpawnUpstreamSpan to true

Signed-off-by: zirain <[email protected]>

* update

Signed-off-by: zirain <[email protected]>

---------

Signed-off-by: zirain <[email protected]>
(cherry picked from commit 635ebfc)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: rate limit doesn't work with two(and more) listeners (#3085)

* fix: rate limit doesn't work with two listeners

Signed-off-by: huabing zhao <[email protected]>

* add e2e test for rate limit on multiple listeners

Signed-off-by: huabing zhao <[email protected]>

* address comments

Signed-off-by: huabing zhao <[email protected]>

---------

Signed-off-by: huabing zhao <[email protected]>
Co-authored-by: Xunzhuo <[email protected]>
(cherry picked from commit a5bedbc)
Signed-off-by: Arko Dasgupta <[email protected]>

* rerun make testdata

Signed-off-by: Arko Dasgupta <[email protected]>

---------

Signed-off-by: huabing zhao <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Jesse Haka <[email protected]>
Signed-off-by: lemonlinger <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Lior Okman <[email protected]>
Signed-off-by: Stéphane Cottin <[email protected]>
Signed-off-by: zirain <[email protected]>
Co-authored-by: Huabing Zhao <[email protected]>
Co-authored-by: Jesse Haka <[email protected]>
Co-authored-by: zirain <[email protected]>
Co-authored-by: Meng <[email protected]>
Co-authored-by: Karol Szwaj <[email protected]>
Co-authored-by: Lior Okman <[email protected]>
Co-authored-by: vixns <[email protected]>
Co-authored-by: Xunzhuo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/conformance priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-blocker MUST be completed to complete the milestone
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants