Change route sorting order to Exact > RegularExpression > PathPrefix by vixns · Pull Request #2579 · envoyproxy/gateway

vixns · 2024-02-08T11:24:41Z

What type of PR is this?

fix

What this PR does / why we need it:

Like kubernetes-sigs/gateway-api#1770, I've encountered a case when converting the following ingress definition:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: sentry
  namespace: sentry
spec:
  tls:
    - hosts:
        - sentry.selfhosted.tld
  rules:
    - host: sentry.selfhosted.tld
      http:
        paths:
          - path: /api/store
            pathType: ImplementationSpecific
            backend:
              service:
                name: sentry-relay
                port:
                  number: 3000
          - path: /api/[1-9][0-9]*/(.*)
            pathType: ImplementationSpecific
            backend:
              service:
                name: sentry-relay
                port:
                  number: 3000
          - path: /
            pathType: ImplementationSpecific
            backend:
              service:
                name: sentry-web
                port:
                  number: 9000

to

apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: sentry
  namespace: sentry
spec:
  hostnames:
    - sentry.selfhosted.tld
  parentRefs:
    - group: gateway.networking.k8s.io
      kind: Gateway
      name: eg
      sectionName: https
  rules:
    - backendRefs:
        - group: ''
          kind: Service
          name: sentry-relay
          port: 3000
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /api/store
        - path:
            type: RegularExpression
            value: ^/api/[1-9][0-9]*/.*$
    - backendRefs:
        - group: ''
          kind: Service
          name: sentry-web
          port: 9000
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /

The RegularExpression is never matched because the PathPrefixes takes precedence

Based on Note: The precedence of RegularExpression path matches are implementation-specific. (kubernetes-sigs/gateway-api#1855), I propose in this PR to changes the route precedence order from Exact > PathPrefix > RegularExpression to Exact > RegularExpression > PathPrefix

codecov · 2024-02-10T04:49:10Z

Codecov Report

❌ Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.47%. Comparing base (dd034a0) to head (ac71a96).
⚠️ Report is 2762 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/gatewayapi/sort.go	70.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2579      +/-   ##
==========================================
+ Coverage   66.45%   66.47%   +0.02%     
==========================================
  Files         161      161              
  Lines       22650    22650              
==========================================
+ Hits        15051    15057       +6     
+ Misses       6723     6720       -3     
+ Partials      876      873       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

vixns · 2024-03-23T12:50:00Z

/retest

zirain · 2024-03-24T13:03:24Z

@arkodg I recall we discussed offline, should EG provide an API about matching order?

arkodg · 2024-03-24T15:18:51Z

met @vixns at Kubecon EU who brought this up in person :)
Lets gather some more info on why ingress-nginx and contour prioritize regex over prefix
@AliceProxy curious to know what Emissary does ?
@zirain do you know what Istio does ?

internal/gatewayapi/sort.go

arkodg · 2024-03-26T09:40:01Z

finally took a look at the config in the PR where PathPrefix

            type: PathPrefix
            value: /

is used as a fallback

This implies that if EG wants to promote or allow this usage of PathPrefix, we need to change the precendance ordering to Exact > RegularExpression > PathPrefix and ask users to build very strict and scoped regex matches

arkodg · 2024-03-26T09:41:03Z

hey @vixns added some suggestions, mainly around code style for this function

arkodg · 2024-03-26T09:47:02Z

should we also consider cherrypicking this into v1.0 ?
cc @envoyproxy/gateway-maintainers

zirain · 2024-03-26T14:56:01Z

met @vixns at Kubecon EU who brought this up in person :) Lets gather some more info on why ingress-nginx and contour prioritize regex over prefix @AliceProxy curious to know what Emissary does ? @zirain do you know what Istio does ?

I found istio may has an issue when converting this, let me double check first.

zirain · 2024-03-26T15:42:51Z

xref:

https://github.com/istio/istio/blob/454d155c00d51d2e11478f8e904266b3cbd9f81a/pilot/pkg/config/kube/gateway/conversion.go#L779

arkodg · 2024-04-04T12:26:54Z

@zirain imo RegularExpression > PathPrefix because users tend to use PathPrefix as a catchall

vixns · 2024-04-04T14:49:15Z

Hi @arkodg, it was nice to met in Paris.
I've updated the code style as asked.

Yes this precedence change is mainly for catchalls, which are a very common and imo should be easy to read and write.
To illustrate this PR's motivation, how would you write the catchall from the HTTProute in this PR's description without changing the precedence order ?

arkodg

LGTM thanks !

arkodg · 2024-04-04T15:02:44Z

@envoyproxy/gateway-maintainers should we back port this to v1.0 ?

vixns · 2024-04-04T15:26:53Z

/retest

kubernetes-sigs/gateway-api#1770 kubernetes-sigs/gateway-api#1855 Signed-off-by: Stéphane Cottin <[email protected]>

Signed-off-by: Stéphane Cottin <[email protected]>

zirain · 2024-04-05T12:40:31Z

@zirain imo RegularExpression > PathPrefix because users tend to use PathPrefix as a catchall

istio had a special handler about catchall.

arkodg · 2024-04-05T13:16:15Z

The client timeout e2e test seems to be failing

zirain · 2024-04-06T04:50:06Z

The client timeout e2e test seems to be failing

maybe we should disable it first, I can find out a way to make it stable now.

arkodg · 2024-04-06T05:43:13Z

The client timeout e2e test seems to be failing

maybe we should disable it first, I can find out a way to make it stable now.

+1, thanks @zirain !

…nvoyproxy#2579) * Change route sorting order to Exact > RegularExpression > PathPrefix kubernetes-sigs/gateway-api#1770 kubernetes-sigs/gateway-api#1855 Signed-off-by: Stéphane Cottin <[email protected]> (cherry picked from commit 11f56fd) Signed-off-by: Arko Dasgupta <[email protected]>

* 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: Sté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: Sté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]>

vixns requested a review from a team as a code owner February 8, 2024 11:24

vixns force-pushed the routeprecedence branch from 1873fbd to 1c6a77c Compare February 9, 2024 10:25

vixns force-pushed the routeprecedence branch 8 times, most recently from ea0eae2 to 8eda561 Compare February 23, 2024 09:06

vixns force-pushed the routeprecedence branch 2 times, most recently from c55b35a to 751fabd Compare February 27, 2024 07:33

vixns force-pushed the routeprecedence branch 4 times, most recently from 5a89705 to 669573f Compare March 10, 2024 07:06

vixns force-pushed the routeprecedence branch 2 times, most recently from 48872a2 to a473d17 Compare March 16, 2024 09:19

vixns force-pushed the routeprecedence branch from a473d17 to c883718 Compare March 23, 2024 12:49

arkodg reviewed Mar 26, 2024

View reviewed changes

internal/gatewayapi/sort.go Outdated Show resolved Hide resolved

arkodg reviewed Mar 26, 2024

View reviewed changes

internal/gatewayapi/sort.go Outdated Show resolved Hide resolved

vixns force-pushed the routeprecedence branch from c883718 to 71dc110 Compare April 4, 2024 14:30

vixns force-pushed the routeprecedence branch 2 times, most recently from b694eba to 2d4a4da Compare April 4, 2024 14:50

arkodg previously approved these changes Apr 4, 2024

View reviewed changes

arkodg requested review from a team April 4, 2024 15:01

vixns dismissed arkodg’s stale review via c3440b9 April 4, 2024 15:20

arkodg approved these changes Apr 4, 2024

View reviewed changes

arkodg added the cherrypick/release-v1.0 label Apr 5, 2024

arkodg requested review from a team April 5, 2024 10:52

vixns added 4 commits April 5, 2024 14:25

Change route sorting order to Exact > RegularExpression > PathPrefix

6255064

kubernetes-sigs/gateway-api#1770 kubernetes-sigs/gateway-api#1855 Signed-off-by: Stéphane Cottin <[email protected]>

Fix for gen-check

3207b6a

Signed-off-by: Stéphane Cottin <[email protected]>

code style fixes.

2bd8691

Signed-off-by: Stéphane Cottin <[email protected]>

code style fixes.

ac71a96

Signed-off-by: Stéphane Cottin <[email protected]>

vixns force-pushed the routeprecedence branch from c3440b9 to ac71a96 Compare April 5, 2024 12:25

zirain approved these changes Apr 5, 2024

View reviewed changes

arkodg merged commit 11f56fd into envoyproxy:main Apr 5, 2024

Conversation

vixns commented Feb 8, 2024

Uh oh!

codecov bot commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vixns commented Mar 23, 2024

Uh oh!

zirain commented Mar 24, 2024

Uh oh!

arkodg commented Mar 24, 2024

Uh oh!

Uh oh!

Uh oh!

arkodg commented Mar 26, 2024

Uh oh!

arkodg commented Mar 26, 2024

Uh oh!

arkodg commented Mar 26, 2024

Uh oh!

zirain commented Mar 26, 2024

Uh oh!

zirain commented Mar 26, 2024

Uh oh!

arkodg commented Apr 4, 2024

Uh oh!

vixns commented Apr 4, 2024

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

arkodg commented Apr 4, 2024

Uh oh!

vixns commented Apr 4, 2024

Uh oh!

zirain commented Apr 5, 2024

Uh oh!

arkodg commented Apr 5, 2024

Uh oh!

zirain commented Apr 6, 2024

Uh oh!

arkodg commented Apr 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 10, 2024 •

edited

Loading