search

kubernetes-sigs / gateway-api

Repository for the next iteration of composite service (e.g. Ingress) and load balancing APIs.
https://gateway-api.sigs.k8s.io
Apache License 2.0
1.84k stars 480 forks source link

Allow Redirects to Drop Trailing Slash #1954

Open robscott opened 1 year ago

robscott commented 1 year ago

What would you like to be added: A way to configure a redirect to drop a trailing slash.

Why this is needed: Today our redirect filter makes it seem like this should maybe be possible, for example this gets kinda close:

kind: HTTPRoute
metadata:
  name: http-filter-redirect
spec:
  hostnames:
  - example.com
  rules:
  - matches:
    - path:
        type: Exact
        value: /example/
    filters:
    - type: RequestRedirect
      requestRedirect:
        statusCode: 302
        path:
          type: ReplaceFullPath
          replacePrefixMatch: /example

Unfortunately an implementation of this config might result in a redirect loop depending on what we decide in https://github.com/kubernetes-sigs/gateway-api/issues/1953. Even if it did work, it's still not as easy as it should be to do that. At a minimum, we should clarify whether this works. We should also consider providing a better more scalable way to do this even if this way may technically work.

tonygilkerson commented 1 year ago

with this would I also be able to do the following?

...
  - matches:
    - path:
        type: PathPrefix
        value: /api/
    filters:
    - type: URLRewrite
      urlRewrite:
        path:
          type: ReplacePrefixMatch
          replacePrefixMatch: /
    backendRefs:
    - name: ace-hub-docs
      port: 80
...

So that a request for /api/foo and /api/bar become /foo and /bar? When I try this today I get //foo and //bar

robscott commented 1 year ago

@tonygilkerson As long as it's broadly implementable, I think that needs to be in scope for whatever is proposed here.

shaneutt commented 1 year ago

/assign @robscott

JackPott commented 1 year ago

I have observed the same behavior as @tonygilkerson, which feels like a bug to me. If one sets replacePrefixMatch: "" the rule disables complete, but with replacePrefixMatch: / its rewriting /foo/bar into //bar, which my upstream rejects as a bad request.

Workaround is have one rule for /foo -> / and another for /foo/bar -> /bar but this has obvious drawbacks.

This is using Istio gateway as the implementation, unclear if this is their interpretation of the API?

roquie commented 11 months ago

1.0.0 is already being released, but this bug has not been fixed. This is a very critical problem, why is no one paying attention to it?

janeklb commented 11 months ago

This is particularly confusing since the docs for HTTPPathModifier (https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.HTTPPathModifier) have a table of examples which are inconsistent with actual behaviour.

shaneutt commented 8 months ago

Where are we at with this one?

Demonsthere commented 7 months ago

Bumping thread as we experience the same issue:

  - filters:
    - requestRedirect:
        path:
          replacePrefixMatch: /
          type: ReplacePrefixMatch
        port: 8080
        scheme: https
        statusCode: 301
      type: RequestRedirect
    matches:
    - path:
        type: PathPrefix
        value: /api/foo/public/

In the given example we want to simply delete the whole Prefix, replace it with /. However what we see happening is a double slash:

GET https://REDACTED/api/foo/public/bar
      accept: application/json
      accept-encoding: gzip,deflate,br
    ← 301 Moved Permanently
      transfer-encoding: chunked
      connection: close
      location: https://REDACTED:8080//bar

We have tried fixing that with a clienttrafficpolicy:

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
  name: proxy
spec:
  path:
    disableMergeSlashes: false

But that didn't remove the double slash as expected

zhaohuabing commented 7 months ago

I also ran into the same issue. To solve this, should we also make trailing splash significant for prefix match? @robscott

k8s-triage-robot commented 4 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

janeklb commented 3 months ago

/remove-lifecycle stale

shaneutt commented 3 months ago

Hi @janeklb!

I see you've bumped the lifecycle on this, out of curiosity is this an issue you might have some time to work on personally?

janeklb commented 3 months ago

Hi @janeklb!

I see you've bumped the lifecycle on this, out of curiosity is this an issue you might have some time to work on personally?

Hard to say just now, but I'd happily taken temporary ownership of this issue (maybe for a month?) to see if I can get it done within that timeframe. My go experience is very limited though, so i'll need some time to ramp up a bit.

I've looked through https://gateway-api.sigs.k8s.io/contributing/devguide/

Would you be able to point me at any files that you'd expect to be involved in solving this?

robscott commented 3 months ago

Hey @janeklb, thanks for following up on this one! I think this is more of a question of "how" we could even accomplish this. As a reminder, the root of the problem is how we've defined prefix matching:

https://github.com/kubernetes-sigs/gateway-api/blob/1e7e64f495f55ece9784910dae2e5e95e03d7bac/apis/v1/httproute_types.go#L381-L391

To solve this, we'd need a solution that allowed most common dataplanes (Envoy, NGINX, HAProxy, etc) to be able to both implement that matching logic and also honor a redirect to drop a trailing slash. The risk is that a naive implementation would result in a redirect loop:

  1. Request to /foo/ is matched by /foo PathPrefix
  2. We redirect to /foo
  3. Request to /foo is matched by /foo PathPrefix
  4. We redirect to /foo
  5. Repeat 3-4 indefinitely

This discussion ends up being closely tied to https://github.com/kubernetes-sigs/gateway-api/issues/1371.

I don't think we can change how existing path matching logic works in a backwards compatible way, so that seems to leave us with the following options:

  1. Introduce a new prefix path matching type that considers a trailing slash meaningful (this is an API change so would need to roll out over several releases following the GEP process
  2. Find a safe and broadly implementable way to prevent redirect loops with some form of spec clarification (this clarification may be limited to how we match trailing slashes)

I haven't had the time to think through this enough to figure out if 2 is viable, but if so, it seems like it would be the shortest path to a solution. If not, I feel fairly confident that 1 would be welcome and fairly straightforward, just would take a fair amount of time to become available.

janeklb commented 3 months ago

Thanks @robscott! Seems like this requires significant background knowledge, and given that it's more open ended than I expected, I don't feel as eager to approach it let alone commit to delivering a solution.

That said, I'll try to learn more about how all this works... but I wouldn't be surprised if the triage bot marks this as stale before I provide an update.

k8s-triage-robot commented 3 weeks ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale