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

Attention: Patch coverage is 70.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 66.47%. Comparing base (dd034a0) to head (ac71a96).

Files	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.

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 <stephane.cottin@vixns.com>

Signed-off-by: Stéphane Cottin <stephane.cottin@vixns.com>

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 <stephane.cottin@vixns.com> (cherry picked from commit 11f56fd) Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* Run certgen when upgrading (#2934) run certgen when upgrading Signed-off-by: huabing zhao <zhaohuabing@gmail.com> (cherry picked from commit 62ecf15) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * Fix: nil secret in resourceversiontable (#2982) * fix nil secret in resourceversiontable Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * check secrets in the xds result Signed-off-by: huabing zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> (cherry picked from commit e880439) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <zhaohuabing@gmail.com> * refactor Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * add comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * remove refactor Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * remove refactor Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix gen Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> (cherry picked from commit f699edf) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: allow websockets in url rewrite (#3022) allow websockets in url rewrite Signed-off-by: Jesse Haka <haka.jesse@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com> (cherry picked from commit 3d51933) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <lemonlinger@gmail.com> * fix broken tests Signed-off-by: lemonlinger <lemonlinger@gmail.com> * fix health-check test case in xds translation Signed-off-by: lemonlinger <lemonlinger@gmail.com> * Simplify code and concise comments Signed-off-by: lemonlinger <lemonlinger@gmail.com> --------- Signed-off-by: lemonlinger <lemonlinger@gmail.com> (cherry picked from commit 8f450a9) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <karol.szwaj@gmail.com> * remove empty line Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * skip infra creation on empty listeners and log it Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> --------- Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> (cherry picked from commit 36d7141) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * Fix: double slashes in redirect URL (#2998) * fix: double trailing splashs in redirect URL Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * add e2e tests Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * add e2e tests Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * revert Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * remove comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * extract method Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * address comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> (cherry picked from commit ceb697f) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: Allow Policy to attach to multiple http listeners (#2967) * Fixing the clienttrafficpolicy validation. Signed-off-by: Lior Okman <lior.okman@sap.com> * Make SecurityPolicy validate correctly. Signed-off-by: Lior Okman <lior.okman@sap.com> * Reverted the SecurityPolicy validation - handled differently via another feature. Signed-off-by: Lior Okman <lior.okman@sap.com> * Updated the tests to reflect that this validation isn't required for SecurityPolicy Signed-off-by: Lior Okman <lior.okman@sap.com> * Added some comments to explain the validation being performed. Signed-off-by: Lior Okman <lior.okman@sap.com> * Updated the error message as requested in the review. Signed-off-by: Lior Okman <lior.okman@sap.com> --------- Signed-off-by: Lior Okman <lior.okman@sap.com> (cherry picked from commit f9409e4) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: set path prefix for http ext auth service (#3018) Signed-off-by: huabing zhao <zhaohuabing@gmail.com> (cherry picked from commit 2882b7c) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <stephane.cottin@vixns.com> (cherry picked from commit 11f56fd) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: infraIR duplicate port translation for merged gateways (#3061) * fix: duplicate port translation for merged gateways Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * refactor to map Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * rename map Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * add seperate testcase Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> --------- Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> (cherry picked from commit 29946b0) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * translator: set SpawnUpstreamSpan to true (#3102) * translator: set SpawnUpstreamSpan to true Signed-off-by: zirain <zirain2009@gmail.com> * update Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: zirain <zirain2009@gmail.com> (cherry picked from commit 635ebfc) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <zhaohuabing@gmail.com> * add e2e test for rate limit on multiple listeners Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * address comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Co-authored-by: Xunzhuo <bitliu@tencent.com> (cherry picked from commit a5bedbc) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * rerun make testdata Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Jesse Haka <haka.jesse@gmail.com> Signed-off-by: lemonlinger <lemonlinger@gmail.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> Signed-off-by: Lior Okman <lior.okman@sap.com> Signed-off-by: Stéphane Cottin <stephane.cottin@vixns.com> Signed-off-by: zirain <zirain2009@gmail.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: Jesse Haka <haka.jesse@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com> Co-authored-by: Meng <lemonlinger@gmail.com> Co-authored-by: Karol Szwaj <karol.szwaj@gmail.com> Co-authored-by: Lior Okman <lior.okman@sap.com> Co-authored-by: vixns <stephane.cottin@vixns.com> Co-authored-by: Xunzhuo <bitliu@tencent.com>

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 <stephane.cottin@vixns.com>

Fix for gen-check

3207b6a

Signed-off-by: Stéphane Cottin <stephane.cottin@vixns.com>

code style fixes.

2bd8691

Signed-off-by: Stéphane Cottin <stephane.cottin@vixns.com>

code style fixes.

ac71a96

Signed-off-by: Stéphane Cottin <stephane.cottin@vixns.com>

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