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

Strip prefix in the forwarded request #44

Closed
jeanblanchard opened this issue Feb 3, 2016 · 29 comments
Closed

Strip prefix in the forwarded request #44

jeanblanchard opened this issue Feb 3, 2016 · 29 comments
Milestone

Comments

@jeanblanchard
Copy link
Contributor

Scenario:

  • I have fabio set up to listen to http://fabioserver/
  • I have a route configured for a service, with prefix /myservice

When making a request to http://fabioserver/myservice/index.html, the service currently receives the forwarded request from fabio to /myservice/index.html
I would like to have the option to strip the prefix in fabio, so that the service would receive a request to /index.html

I currently have a homemade go proxy that does that, using http.StripPrefix. I may have the time to prepare a PR for this at some point, but no promises...

@magiconair
Copy link
Contributor

This is a duplicate of issue #2 for which I've decided not to support this. See the reasoning in the closing comment.

@jeanblanchard
Copy link
Contributor Author

OK I get the reasoning.

I would argue that this introduces a requirement that services need to know the route they serve.
You can't change the "mount" path for a service, say from /myservice to /legacy/myservice without changing the service itself (which may not be an option).

@magiconair
Copy link
Contributor

I would counter that argument that it is better to add another handler to the service than splitting up your configuration across services and load balancer. I know you can do and lots of other load balancers allow you to do it but in our own setup we have had enough additional complexity with this that this actually led to the development of fabio in the first place.

@jeanblanchard
Copy link
Contributor Author

... And I'm back...

It turns out I really need this after all. I tried to make all services have their prefix configurable, but this introduces a bit of complexity everywhere in the service, which I really want to avoid.
I think you'll agree that the services should not depend on the host where they are deployed. I believe the same is true for the path.
Stripping the prefix would allow services to work the same, regardless of the path where they are deployed.

Note that I do not suggest rewriting the urls in the responses (or maybe just Location headers), but instead encourage services to use relative urls.

Anyway, would you accept a pull request to add this option, or should I maintain my own fork ?

@magiconair
Copy link
Contributor

I agree on the host but not the path but that doesn't really matter. Please start this discussion with a suggestion on how this should work from the configuration perspective so that we can decide what the best approach is. No promises since I'd like to see how you want to solve that first and also want to think a bit about edge cases.

@magiconair magiconair reopened this Feb 19, 2016
@jeanblanchard
Copy link
Contributor Author

On the configuration side, I'm thinking a global configuration flag in fabio.properties (proxy.prefix.strip?).

It's probably not necessary to allow each service/route to override the behavior.

After a first look at the code, the global config case seems to be pretty straightforward. I'm not sure yet how to handle the websocket case, I'll look into it.

@magiconair
Copy link
Contributor

Why should this work only for a single, global prefix? Do you want to strip some of the path or all of it, for some of the services or for all of them, the same prefix for all services?

http://foo.com/service/user/index.html -> http://usersvc/index.html
or
http://foo.com/service/user/index.html -> http://usersvc/user/index.html

Can you explain a bit more what makes it so difficult to change the apps? Are these services you don't control or can't change or are they just too many?

@jeanblanchard
Copy link
Contributor Author

I don't think it should work for a single global prefix. I think there should be a global option to strip the configured prefix for all services.

Basically to access an app behind a fabio proxy, you need to add a prefix to identify the app. I propose to remove the prefix from the requests so that the app works as if there were no proxy.

http://fabioserver/serviceprefix/user/index.html -> http://serviceserver/user/index.html

The idea for my case is that the services are third-party developed plugins, that should work the same regardless of where they are deployed.

@magiconair
Copy link
Contributor

Does http://serviceserver/user/index.html need to know that /serviceprefix was stripped from the original request? (i.e. do we need to inject a header?)

I could extend the route add command with a strip <prefix> option which would allow to control this per route/target. The only question is how to encode that in the service registration.

In issue #42 I've outlined a more flexible notation for the tags, i.e. p-host/path?key=val&key=val... which would provide the mechanism to add a strip=/serviceprefix option.

@SathishN
Copy link

+1 for stripping service prefix.

In my case, I have multiple version of a web api, I'll have version prefix in the url.

I would like the option of striping the prefix at fabio.

e.g:

http://api.client:9999/v1/clients/1234 (tag: urlprefix-api.client/v1) -> http://server1:8030/clients/1235
http://api.client:9999/v2/clients/1234 (tag: urlprefix-api.client/v2) -> http://server1:8041/clients/1235

I like the strip prefix to be per route/registration(at fabio) and ideally be managed as a consul tag(like urlprefix).

@magiconair
Copy link
Contributor

@SathishN Actually, that is the kind of use case that made me decide not to support stripping in the first place. If you have two different API endpoints then you shouldn't name them both /client since they are different. The full names for these endpoints are /v1/client and /v2/client. Your integration tests should be different since they need to talk to the compatible endpoint and by moving the version routing in the load balancer - any load balancer - you make your application routing dependent on that configuration.

One motivation for fabio was to gently nudge people in the direction of not doing that since your code most likely doesn't care whether it has a handler for /client or /v1/client. However, the difference is that if you do that then you can just publish that route, i.e. your service says "I accept /v1/client" and fabio can pick that up and send you that traffic without any extra magic. It is also more explicit if you want to test the service by itself.

@SathishN
Copy link

@magiconair client is a resource, v1 and v2 are versions of the client resource, they are served under host name is api.client. Integration tests are contained with in the code base/repository, they don't need to know on api versions.

Your right on web api's should know it routes, yes it'll and when web api starts up, it will publish it's route to the consul. But I don't want to inject the version info in the source code, it's a config value. I have few ways to handle this routing within web api itself. If fabio handles it, it moves the complexity out of my web api.

@msabramo
Copy link

msabramo commented Apr 4, 2016

This is interesting. I see @magiconair's point about how services should advertise the exact endpoint they respond to.

In my case though, I'm part of an infrastructure team and we work with dozens of existing services that are owned by about a dozen teams. Changing the services is not really an option.

But maybe fabio is not designed for this use case of integrating a bunch of legacy services.

@magiconair
Copy link
Contributor

@msabramo have you tried? The main message is that the app should work the same with or without a load balancer in front of it.

@msabramo
Copy link

msabramo commented Apr 4, 2016

@magiconair Nope, I haven't tried. I just happened upon fabio yesterday while searching around for the latest on using consul-template to configure nginx or HAProxy. I've been thinking through what the possible roadblocks could be.

Thanks for sharing fabio!

@aaronhurt
Copy link
Member

Expanding a comment I made in #69 that linked to this issue. I would not like to see a global option as that would not work with our current deployment. We definitely need the ability to strip the route prefix from some applications but not others.

This requirement is absolutely due to the points raised by @msabramo about legacy and vendor applications were we do not have control over the observed prefix for the application without placing additional hacks before the application (htaccess/haproxy/etc..). I find it much more convenient and sane to keep this logic in as few places as possible. Spreading out individual hacks across several applications is much harder to maintain than keeping all of that close to the actual entry point (the load balancer).

We also run the majority of our applications out of docker containers including frontend javascript applications that don't have any real prefix knowledge and simply reference all resource relative to the request. These applications benefit greatly from the ability to change routing prefixes without modifying the underlying code.

@magiconair
Copy link
Contributor

I get it. I'll add it but need the refactor of #111 to be able to express this.

@magiconair magiconair mentioned this issue Jul 7, 2016
magiconair added a commit that referenced this issue Dec 21, 2016
This patch adds support for a 'strip=/path' option which will remove
/path from the front of the outgoing request path. For 'strip=/foo'
a request to '/foo/bar' will be matched on '/foo/bar' but forwarded
as '/bar'.

Fixes: #44, #124, #164
@magiconair
Copy link
Contributor

magiconair commented Dec 21, 2016

The change in the branch allows to specify a strip=/foo option to be added to strip /foo from outgoing requests. The incoming request is still matched on /foo/bar. The syntax is

# consul tags
urlprefix-/foo/bar strip=/foo

# config lang
route add svc /foo/bar http://1.2.3.4:5555 opts "strip=/foo"

@jovandeginste
Copy link
Contributor

Is there a way to pass this option via the Consul tags?

@magiconair
Copy link
Contributor

@jovandeginste yes, e.g. urlprefix-/foo/bar strip=/foo The example I gave is wrong since the opts "key=val key=val ..." syntax is used for the route add ... statement. I'll fix it.

@jovandeginste
Copy link
Contributor

Just tried this, and it works! Although spaces in the tags give some very strange side-effects :-)

@jovandeginste
Copy link
Contributor

For completeness sake, the stripped part is not added as a Request header?

@magiconair
Copy link
Contributor

What kind of side effects do you mean? The original request URI is not added as a request header. If you want that you could just not strip the prefix?

I'm also wondering whether the strip=/path is the best way to express this. If you have an opinion on this then please let me know.

@jovandeginste
Copy link
Contributor

The side effects are completely unrelated to fabio. We use service name and tags to create some automatic entries in consul's kv store like /services/$service_name/$service_tag/$container_id/, for each tag of the container. This becomes weird when the tags now start containing / and spaces by design ;-)

About the stripping, I'm still not sure if I want to use it. It's nice to have, but dangerous in practice. It works for specific cases. Why the header would be useful? Because the web app could refer to it's root by the header (/$header/) for assets and thus work irrespective of that root.

The actual use case I have is on demand spawning of docker containers for our dev teams, running their git branches of different services for internal testing. Since there will be numerous parallel branches, they should be easily discernible. My two main options are branchname.servicename.commondomain/ and commondomain/servicename/branchname/. Not sure if I need to explain pro's and cons of each option... I sadly don't have a well funded opinion about the way you implemented it.

@jovandeginste
Copy link
Contributor

I thought about it a bit more and I now think the tag system is not ideal. The better way would be to use consuls kv store for all the parameters including the urlprefix. Not sure if this still fits inside the scope of this project then...?

@magiconair
Copy link
Contributor

@jovandeginste consul kv is not an option since there are not transactions between registering a service and modifying the kv store. What if the one works and the other doesn't? When a service dies entries would have to be cleaned up from the kv store by a separate process. That's a design decision I've made early on. I agree the tag prefixes are a workaround since there is no room to have meta-data in the service registration.

@jovandeginste
Copy link
Contributor

@magiconair maybe fabio does not need to care about populating and cleaning the kv store (just like it doesn't care about adding the service tags); this can be an entirely separate process indeed, like the registrator we use. Fabio would only be a consumer of that information.

In this story, fabio reads the (relevant part of the) consul kv store to find out what services it is supposed to serve, and in that kv store it will find what consul service it should forward incoming requests to. Fabio will then monitor those backend services like it does now (health checks). If there is configuration in the kv store but no (healthy) backend, it could respond with the proper http code (502?)

Then again, maybe this is entirely out of scope for this project.

@magiconair
Copy link
Contributor

Our services don't use the registrator and are self-contained. That means they register and de-register themself and therefore something else would need to cleanup afterwards. KV and service registrations will get out of sync especially in large scale installations. It is only a matter of time.

magiconair added a commit that referenced this issue Jan 17, 2017
This patch adds support for a 'strip=/path' option which will remove
/path from the front of the outgoing request path. For 'strip=/foo'
a request to '/foo/bar' will be matched on '/foo/bar' but forwarded
as '/bar'.

Fixes: #44, #124, #164
magiconair added a commit that referenced this issue Jan 17, 2017
This patch adds support for a 'strip=/path' option which will remove
/path from the front of the outgoing request path. For 'strip=/foo'
a request to '/foo/bar' will be matched on '/foo/bar' but forwarded
as '/bar'.

Fixes: #44, #124, #164
@magiconair
Copy link
Contributor

Merged to master

@magiconair magiconair added this to the 1.3.7 milestone Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants