Change route sorting order to Exact > RegularExpression > PathPrefix

[vixns]

What type of PR is this?

fix

What this PR does / why we need it:

Like https://github.com/kubernetes-sigs/gateway-api/issues/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. (https://github.com/kubernetes-sigs/gateway-api/pull/1855), I propose in this PR to changes the route precedence order from Exact > PathPrefix > RegularExpression to Exact > RegularExpression > PathPrefix

[codecov[bot]]

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 :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[vixns]

/retest

[zirain]

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

[arkodg]

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 ?

[arkodg]

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]

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

[arkodg]

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

[zirain]

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]

xref:

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

[arkodg]

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

[vixns]

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]

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

[vixns]

/retest

[zirain]

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

istio had a special handler about catchall.

[arkodg]

The client timeout e2e test seems to be failing

[zirain]

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]

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 !