Gateway API: support HTTPURLRewriteFilter (extended/experimental)

[skriss]

Add support for the Gateway API HTTPURLRewriteFilter. This is currently extended support/experimental, but seems worth adding.

[jkotalik]

Thanks for filing this.

If I wanted to have the ability to do Path Rewriting (or mainly, the ability to strip the matched prefix match before sending to the downstream), would you recommend trying out the HTTPProxy CRD type you have? That may be a decent middleground for me now, even though I really like the separation provided by the new gateways spec between Gateway and Routes.

[youngnick]

Yes, HTTPProxy is definitely able to do path rewrites.

Note that you can approximate the Gateway/Route separation by using a root HTTPProxy that simply lists the FQDN and then includes child HTTPProxies (which can't have an FQDN set). See https://projectcontour.io/docs/v1.19.1/config/inclusion-delegation/ for more info there. The Gateway/Route split is newer and better designed (we took what we learned from HTTPProxy and pushed it upstream), but HTTPProxy will get you most of the way there until we can catch up.

[jkotalik]

Is there a way to make the root HTTPProxy automatically pick up child HTTPProxies without modifying the root HTTPProxy with an include?

[skriss]

Is there a way to make the root HTTPProxy automatically pick up child HTTPProxies without modifying the root HTTPProxy with an include?

Not out of the box, no, you'd need to explicitly add it to the root HTTPProxy. I believe folks have written additional controllers to implement this kind of behavior but can't recall off the top of my head where they are.

[youngnick]

This property (having the root HTTPProxy need to explicitly include children) was actually designed to force an interaction - this was designed originally for multi-tenant clusters with a central infra team owning the root HTTPProxies, where the central infra team wanted to know where it was used.

The current setup in the Gateway API for Gateway/Route attachment is the opposite, the child (HTTPRoute) requests attachment to the parent (mostly Gateway), and the parent may have additional criteria than just "accept everything in the same namespace".

This is a long way of saying sorry, we made a mistake with this design that we can't fix without a version bump of HTTPProxy. I'm reluctant to do that, because Gateway API has a fixed version of the same idea, I would rather push engineering effort into bringing Gateway API into parity with HTTPProxy's functionality.

We discussed this issue previously in #2206.

[stevesloka]

we made a mistake with this design that we can't fix without a version bump of HTTPProxy.

So I wouldn't say this was a mistake, just that they way it implemented. The right team who should have ownership of that path is given full access to it. With GatewayAPI, there's no such way to make sure the resource who should own the path, gets it. GatewayAPI works on a first-come-first-serve resolution, so as long as you claim the resource first, you get it.

The same would happen if you had 2 resources and one is accepted and the other isn't (since the first got there, first). If you deleted the first, the second would get the resource. Then if you recreated the first, you'd be out of luck until the 2nd was deleted.

[mmalecki]

I started doing some work on this - primarily just trying to write a working, non-supported test like we do have for the HTTPRouteFilterRequestMirror, just to get a feel for the code-base.

[sunjayBhatia]

@mmalecki let us know if you would like to have this issue assigned, we would definitely take a PR

[mmalecki]

@sunjayBhatia yes, please. I poked around the source code, and think I should be able to get it done.

[mmalecki]

I was able to get part-way there, but ran into an issue that has me a bit stumped: Envoy appears to be replacing the entire path, as opposed to just the prefix. For example, with a prefix /a, set to replace to /b, requests to both /a and /a/c seem to land as /b (no /c). I've used PrefixRewrite, which does indicate that it could replace the entire path. Any advice here would be greatly appreciated.

[sunjayBhatia]

@mmalecki what was the path you are matching on for the route(s) you are testing? The Envoy config field we end up using will replace whatever is matched with the specified prefix. So if you have set up a generic prefix match for / then the whole path will be replaced. If you have a prefix match for /a and replace /b then you should get what you are expecting

[sunjayBhatia]

This seems to be how the Gateway API words it as well:

This type of modifier indicates that any prefix path matches will be replaced by the substitution value. For example, a path with a prefix match of “/foo” and a ReplacePrefixMatch substitution of “/bar” will have the “/foo” prefix replaced with “/bar” in matching requests.

From here

If we wanted to have a different path matching logic (e.g. a generic prefix path match like / can still replace a request with path /a/c with /b/c rather than /b) we may have to use a regex rewrite for that

[sunjayBhatia]

Here's also an example of path matching/rewriting in practice: https://github.com/projectcontour/contour/blob/main/test/e2e/httpproxy/path_rewrite_test.go

Could add an example for the generic match case as well

[sunjayBhatia]

@mmalecki contour v1.21 is coming up, just checking in if you are still working on this and need anything from us?

[mmalecki]

My apologies, @sunjayBhatia, I had to step away from this for a little while due to some other priorities.

I indeed use PrefixRewrite.

Oddly enough, I'm not using / as my prefix. Here's how my HTTPRoute is configured:

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: HTTPRoute
metadata:
  name: kuard
  namespace: projectcontour
spec:
  hostnames:
  - local.projectcontour.io
  parentRefs:
  - group: gateway.networking.k8s.io
    kind: Gateway
    name: contour
  rules:
  - backendRefs:
    - group: ""
      kind: Service
      name: kuard
      port: 80
      weight: 1
    filters:
    - type: URLRewrite
      urlRewrite:
        path:
          substitution: /substitution/
          type: ReplacePrefixMatch
    matches:
    - path:
        type: PathPrefix
        value: /prefix

I then curl:

$ curl -H "Host: local.projectcontour.io" "http://127.0.0.1/prefix/foo" -v
*   Trying 127.0.0.1:80...
* Connected to 127.0.0.1 (127.0.0.1) port 80 (#0)
> GET /prefix/foo HTTP/1.1
> Host: local.projectcontour.io
> User-Agent: curl/7.82.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 Not Found
< content-type: text/plain; charset=utf-8
< x-content-type-options: nosniff
< date: Fri, 15 Apr 2022 20:13:02 GMT
< content-length: 19
< x-envoy-upstream-service-time: 0
< server: envoy
< 
404 page not found
* Connection #0 to host 127.0.0.1 left intact

But kuard logs:

2022/04/15 13:40:15 Could not find certificates to serve TLS
2022/04/15 13:40:15 Serving on HTTP on :8080
2022/04/15 20:10:10 10.244.0.2:51968 GET /substitution/

when the URL should be /substitution/foo instead.

(My cluster is your typical "getting started with Contour" kind of a kind cluster. My branch is here.)

[skriss]

Quick thought, I wonder if the fact that internally we're generating a regex match for Gateway API because we need to do "path segement prefix matching" rather than "string prefix matching" is messing this up. Will look some more next week but that seems likely. Also ref. https://github.com/projectcontour/contour/issues/4442

[mmalecki]

Ah, this being related to a potential different method of matching that "eats up" the part after prefix was one of my suspicions. I'll also take a look if there's anything I can do here.

[mmalecki]

Hm, I think I may have a grip on how to approach this alongside #4442, since Envoy's path_separated_prefix appears to be a 1:1 compatible with ours. Can't make any promises, but I'll see if I can get anywhere during the weekend.

[mmalecki]

Whew, I was indeed able to get this going by using path_separated_prefix! Just smoothing out some rough edges (e.g. empty-string substitution) and PR'ing this.

[mmalecki]

I think since this depends on #4442 (I don't see a way to make this happen with RegExp-based matching), which is pending Envoy 1.22, we should change the milestone on this issue to 1.22.

One thing I'm at a loss about here is how to make PrefixRewrite on a Route to behave the way I want it to when we intend to rewrite the prefix to an empty string. My approach has been to set PrefixRewrite = "/" (since we can't have it empty - it disables the behavior). This, however, causes URLs with // to reach our backend. That's expected on the string level, but we instruct Envoy to merge our slashes by default. Are we seeing an Envoy issue, or am I missing a trick to make the rewrite truly empty?

[skriss]

Leaving this in 1.22 since @mmalecki has been working on it (currently waiting on #4477 to merge).

[sbaildon]

Asking for clarity; this the issue I should be tracking if I'm interested in watching for rewrite support in HTTPRoute?

[skriss]

Asking for clarity; this the issue I should be tracking if I'm interested in watching for rewrite support in HTTPRoute?

Yes, that's right. @mmalecki were you still interested in working on a PR for this?

[mmalecki]

Apologies for letting this slip through the cracks. I'm still interested. I've rebased my old urlrewrite branch off main and intend to test this tonight. Hopefully the slashes won't be back!

[skriss]

Apologies for letting this slip through the cracks. I'm still interested. I've rebased my old urlrewrite branch off main and intend to test this tonight. Hopefully the slashes won't be back!

No problem, thanks @mmalecki!

[skriss]

hey @mmalecki, just checking in again to see if you'd be able to proceed with a PR here. If not, perhaps we can use your work as a starting point and give you co-author credit.

[shrutiyer]

@skriss @mmalecki Any chance that this feature will be shipped with v1.24? Pretty useful feature and Gateway API feature will be enhanced by the inclusion of this

[skriss]

@skriss @mmalecki Any chance that this feature will be shipped with v1.24? Pretty useful feature and Gateway API feature will be enhanced by the inclusion of this

I would definitely like to include this; looks like @mmalecki has some work started here but if he's unavailable to complete it we should be able to move it forward and give him co-author credit on it.

[skriss]

cc @vmw-yingy @fangfpeng

[skriss]

I'm going to grab this issue and try to get it done for the upcoming 1.24 release.