Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support rewrote targets during path based routing #851

Open
JorTurFer opened this issue Dec 8, 2023 · 10 comments
Open

Support rewrote targets during path based routing #851

JorTurFer opened this issue Dec 8, 2023 · 10 comments

Comments

@JorTurFer
Copy link
Member

JorTurFer commented Dec 8, 2023

Proposal

Reverse proxies support rewriting the path before calling the next backend.

Imagine this ingress:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: demo-ingress
  annotations:
    nginx.ingress.kubernetes.io/rewrite-target: /$2
    kubernetes.io/ingress.class: nginx
spec:
  rules:
    - host: potato.com
      http:
        paths:
          - path: /store(/|$)(.*)
            pathType: Prefix
            backend:
              service:
                name: interceptor-svc
                port:
                  number: 8080

Requests to the interceptor won't contain the full path but the rewrote path, for example https://potato.com/store/checkout will be passed to the interceptor as https://potato.com/checkout.

The problem is that if we want to support path based routing in the interceptor, we need to maintain the pathPrefix. For example, this HTTPScaledObject doesn't work:

kind: HTTPScaledObject
apiVersion: http.keda.sh/v1alpha1
metadata:
  name: demo-http-sp
spec:
  hosts:
  - potato.com
  pathPrefixes:
  - /store

This won't work due to this removed prefix, and the same request (https://potato.com/store/checkout) returns 404 and generates an error like this the interceptor:

{"level":"error","ts":1702068375.455492,"logger":"LoggingMiddleware.RoutingMiddleware.StaticHandler","caller":"handler/static.go:36","msg":"Not Found","routingKey":"//potato.com/checkout/","namespacedNameError":"PANIC=value method k8s.io/apimachinery/pkg/types.NamespacedName.MarshalLog called using nil *NamespacedName pointer","stream":"<nil>","stacktrace":"github.com/kedacore/http-add-on/interceptor/handler.(*Static).ServeHTTP\n\tgithub.com/kedacore/http-add-on/interceptor/handler/static.go:36\ngithub.com/kedacore/http-add-on/interceptor/middleware.(*Routing).ServeHTTP\n\tgithub.com/kedacore/http-add-on/interceptor/middleware/routing.go:46\ngithub.com/kedacore/http-add-on/interceptor/middleware.(*Logging).ServeHTTP\n\tgithub.com/kedacore/http-add-on/interceptor/middleware/logging.go:42\nnet/http.serverHandler.ServeHTTP\n\tnet/http/server.go:2936\nnet/http.(*conn).serve\n\tnet/http/server.go:1995"}

TBH, I'm not sure about the best approach here. Maybe we can introduce a custom header (via snippet) in the ingress with the original path (IDK if this is possible), or even better (but more complicated and worse for migration to the add-on), supporting the target-rewrite at interceptor level.

This second option (supporting the rewrites within interceptor) allows users to maintain their current backends without any change as in that scenario, they would have to move the rewrite from ingress to interceptor and backends would continue without any change.

@JorTurFer
Copy link
Member Author

@tomkerkhove @t0rr3sp3dr0 , WDYT about this feature?

@miooochi
Copy link

miooochi commented Dec 17, 2023

I have the same concern. My case is slightly different as I am using Kong API Gateway. However, when it comes to path-based routing in the interceptor level, I ended up with the same technical issue. If possible, could we have a feature like “strip-path: true”?

@worldspawn
Copy link
Contributor

Ingress-nginx provides a configuration property called proxy-add-original-uri-header which adds X-Original-Uri to the backend request (request the interceptor will receive). This seems a nice, out-of-the-box way to get this done.

https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#proxy-add-original-uri-header

@JorTurFer
Copy link
Member Author

I like the idea of supporting X-Original-URI but I think that it's not an standard and it can be a problem in the future :/

@JorTurFer
Copy link
Member Author

@tomkerkhove @t0rr3sp3dr0 ?

@t0rr3sp3dr0
Copy link
Contributor

I think we need to define the scope of the add-on first. I'm not sure if we just want to provide a solid base for HTTP scaling or if we want to have feature-parity with generic ingresses.

I would prioritize other features before implementing this, given it's something that can be achieved with the current implementation by adding Nginx or other kind of proxy between the add-on and the service.

@JorTurFer
Copy link
Member Author

No no, I didn't mean to apply the rewrite in the add-on. As you said, this can be done by load balancer. My point is that if you have a load balancer on top which replaces the path during the process, in the add-on we won't know the correct route.
For example, imagine 3 services routed by path

  • myhost.com/serviceA
  • myhost.com/serviceB
  • myhost.com/serviceC

If the ingresses apply a redirect to / during the process, each backend will receive the request on /, which is correct. The problem is that the add-on will receive it also for /, and here the add-on doesn't work as HTTPScaledObjects are configured to route paths /serviceX but the requests have been rewritten by the load balancer to '/'.
Currently, we enforce the backends to manage the full path as otherwise the add-on won't work and I'd like to avoid that constraint (if it's possible).
How would you handle this with a proxy between the add-on and the backend? I see the problem in the interceptor itself and not behind it, but I can be missing some important point in the picture

@t0rr3sp3dr0
Copy link
Contributor

My suggestion was not to have something rewriting paths at the Ingress/LoadBalancer level. As you just explained, that wouldn’t work because we would loose access to the full paths in the add-on.

What I was suggesting was to make the add-on point to Nginx, HAProxy, or anything that is able to manipulate HTTP. In the HTTPSO spec, the service field would be the name of the service that points to the proxy. And the proxy would be the one responsible for rewriting paths and forwarding them to the service we are scaling.

I know this is an extra thing that developers would need to setup and manage, but at the same time our current scope doesn’t include rewriting paths: https://github.com/kedacore/http-add-on/blob/main/docs/scope.md. And I’m not saying we shouldn’t support this kind of feature, just that it would be wise to discuss it before taking this path.

Would we like to support other stuff that is currently achievable with a proxy? Here are some examples of HTTP manipulations we could potentially support: https://www.haproxy.com/documentation/haproxy-configuration-tutorials/http-rewrites/. If we don’t want support everything, what’s is our boundary and why?

@JorTurFer
Copy link
Member Author

I fully agree with your point.
Maybe we should talk about where we are, what do we need for going to GA, and the desired roadmap and new features.
Let's hold the issue as it is until we decide the other stuff.
I'm going to open an issue to talk about them

@JorTurFer JorTurFer mentioned this issue Apr 4, 2024
18 tasks
Copy link

stale bot commented Apr 9, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Apr 9, 2024
@JorTurFer JorTurFer removed the stale All issues that are marked as stale due to inactivity label Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To Triage
Development

No branches or pull requests

4 participants