search

fabiolb / fabio

Consul Load-Balancing made simple
https://fabiolb.net
MIT License
7.25k stars 619 forks source link

Simple HTTP path prefix replacement #767

Closed JamesJJ closed 3 years ago

JamesJJ commented 4 years ago

This is a potential solution for #691

A new option prepend is introduced. For example, prepend=/baz allows an incoming request with path /bar to be forwarded upstream with path /baz/bar.

Combining prepend=/baz with the existing strip=/foo option, allows an incoming request path /foo/bar to be forwarded upstream with path /baz/bar.

This change should not affect any existing functionality, and the docs are updated to match.

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

lizongshen commented 4 years ago

This function is very useful. When can I expect to use it？

lizongshen commented 3 years ago

I added prepend=baz tag to consult services，but it doesn't work?

ketzacoatl commented 3 years ago

@nathanejohnson, can you elaborate on the thinking label? What is needed to make a decision on this?

bac-w commented 3 years ago

@nathanejohnson the functionality is very useful, is it possible to see it in the next release?

aaronhurt commented 3 years ago

There are a few conflicts in this branch due to the fix for #824 that was recently pushed. The prepend should probably also happen before replacement of the pseudo variable.

JamesJJ commented 3 years ago

@leprechau Thank you for checking on this one. I've resolved the conflicts.

I'm not totally clear about 「The prepend should probably also happen before replacement of the pseudo variable」 - I'd be grateful if you could explain further if this needs other adjustments.

aaronhurt commented 3 years ago

@JamesJJ Could you take a look at #824 and see if the changes there do the same thing as the prepend option? I think these changes allow the same behavior.

The test cases there, I think, do the same as the proposed prepend option or am I missing something?

JamesJJ commented 3 years ago

@leprechau It's been a while since I played with this so I may be wrong, however my interest was in the http proxy function (not http redirect). It looks to me that between https://github.com/fabiolb/fabio/blob/master/proxy/http_proxy.go#L133 where the target path is set to the incoming request path, and https://github.com/fabiolb/fabio/blob/master/proxy/http_proxy.go#L198 where the request is proxied out, only StripPath updates the target path.

So if I read it correctly, t.URL.Path is not utilised when proxying - so we need a new prepend option, or update to involve t.URL.Path.

I think I opted for a new option as there is no risk of a breaking change if anyone is currently relying on the target URL path being ignored when proxying.

aaronhurt commented 3 years ago

@JamesJJ Thank you, that makes sense. You are correct the changes I was mentioning only affect redirects whereas this options is implemented in both the redirect and straight proxy portions of the code. Sorry for the confusion.

Thank you again, LGTM!