Allow {host} in proxy upstream #998

meyskens · 2016-08-04T15:18:49Z

This PR allow to use {host} in the proxy upstream to you can proxy the request to a specific server/path based on what hostname was requested. (discussed in #990 )

Note: I have barely any experience in Go, and the implementation I used seems very hacky to me. Any suggestions are welcome

CLAassistant · 2016-08-04T15:18:50Z

All committers have signed the CLA.

mholt · 2016-08-04T15:33:54Z

Cool, thanks for your work @meyskens! Will take a look at it later today.

In the meantime, run go fmt on your code which will automatically fix indentation and other formatting issues.

theblazehen · 2016-08-05T18:25:06Z

I've been running this by reverse proxying to nginx behind caddy, would be great to have it implemented directly

mholt · 2016-08-05T23:47:04Z

caddyhttp/proxy/reverseproxy.go

@@ -126,6 +127,40 @@ func NewSingleHostReverseProxy(target *url.URL, without string) *ReverseProxy {
 	return rp
 }

+func NewDynamicHostReverseProxy(templateUrl string, without string) *ReverseProxy {
+	templateUrl = strings.Replace(strings.ToLower(templateUrl), "{host}", "HOST", -1)


This doesn't generalize well, for example, if you want {hostonly}...

True. Maybe checking for {} would be better

mholt · 2016-08-05T23:47:53Z

Nice first attempt! This is not an easy problem to solve elegantly. I have been looking at this for about an hour trying to find a good solution independently as well. The director function is set up when the server is started, not at request-time. We do want to do as much processing as we can before actual requests start coming in in order to reduce latency. But that's what makes it a little hacky, because replacements require a *http.Request.

Will have some suggestions soon.

mholt · 2016-08-06T01:09:31Z

I think the most proper fix will involve making a httpserver.NewReplacer and parsing the upstream URL on each request, since the Director function needs it at request-time. This will introduce some overhead, but I haven't checked how much. Is that something we're willing to accept?

meyskens · 2016-08-06T09:36:33Z

Seems like a good idea. Overhead is unavoidable in this situation imo

meyskens · 2016-08-06T09:59:51Z

I think i know a way to do this. I'll try this out and update this PR

meyskens · 2016-08-06T11:48:50Z

This should parse the URL in the director itself on every request, this adds a little overhead on every request but I don't think an url parse will kill your server 😛. It should also allow to use parameters everywhere in the URL (so you can also set the path different if you like). Support for Unix sockets is not included as it seems to me as a security risk.

mholt · 2016-08-06T20:57:23Z

caddyhttp/proxy/reverseproxy.go

+		targetQuery := target.RawQuery
+
+		req.URL.Scheme = target.Scheme
+		req.URL.Host = replacer.Replace(strings.Replace(target.Host, "HOST", "{host}", -1))


How about doing the replacement before parsing the URL? Then you won't have to hack around the { } like this.

oops, it is already done but forgot to remove the old one

templateUrl = replacer.Replace(templateUrl) target, _ := url.Parse(templateUrl)

Oh yeah 😄 ... is it weird that I read PRs from the bottom-up? 😛

I do that with magazines so no. (disclaimer: I'm weird)

mholt · 2016-08-06T21:15:58Z

caddyhttp/proxy/reverseproxy.go

+	rp := &ReverseProxy{Director: director, FlushInterval: 250 * time.Millisecond} // flushing good for streaming & server-sent events
+
+	return rp
+}


This is the same thing as NewSingleHostReverseProxy, isn't it, but with a different director and without support for unix sockets?

Any chance we could reuse that code? i.e. call NewSingleHostReverseProxy somehow (maybe its signature has to change slightly?) and then just change the director?

Thanks for working through this. :) Not an easy change! But it's looking better already.

Once we get this looking good, it will need some tests...

Calling NewSingleHostReverseProxy seem hard as the URL has to change inside the director. The director is also where 90% of the code changes are. What I was thinking of is putting the property setters of the req inside a separate function.

That's a possibility too. I'll let you experiment with what is most elegant/clean!

oh yay 😛 If suddenly caddy has a bug that it burns down houses: don't blame me 😛

meyskens · 2016-08-06T21:47:35Z

Moved almost all logic of the director in a function that sends back the req. Since there is unix sockets in there and you probably don't want to let /var/run/{host}.socket become /var/run/mysql-without-a-password.socket I added a check that they always go to SingleHostProxy.

meyskens · 2016-08-06T22:04:57Z

Added the first part for the tests, now only finding out how to use it.

arikfr · 2016-08-07T17:52:05Z

caddyhttp/proxy/proxy_test.go

@@ -710,13 +710,18 @@ func basicAuthTestcase(t *testing.T, upstreamUser, clientUser *url.Userinfo) {
 }

 func newFakeUpstream(name string, insecure bool) *fakeUpstream {
-	uri, _ := url.Parse(name)
+	if strings.Contains(name,"{") && strings.Contains(name,"{") {


I think the second strings.Contains call was meant to be with a } instead of {.

meyskens · 2016-08-07T22:00:24Z

From testing on my server I noticed the health system isn't useful as are the fail counts. I've disabled them for dynamic hosts. Imo we need a property that tells if a backend is dynamic is the string.Contains gets overused. I also added some more explaining errors (maybe improve the logging, but that is for another PR).

On tests: anybody a hint what URL it tries to fetch 😛

meyskens · 2016-08-07T22:02:10Z

Also a configurable tryDuration may be handy as not everybody can wait a minute for a 502.

meyskens · 2016-08-07T22:11:15Z

Once I got this tested and wrote the tests it should be ready for review + merge. (may look into a tryDuration property in the caddyfile)

mholt · 2016-08-10T19:53:57Z

caddyhttp/proxy/upstream.go

@@ -138,6 +138,9 @@ func (u *staticUpstream) NewHost(host string) (*UpstreamHost, error) {
 				if uh.Unhealthy {
 					return true
 				}
+				if uh.Dynamic {
+					return false
+				}


Backends with hostnames that depend on the request are always up?

yeah, since caddy can't see the difference between 2 different backends with this unfortunately.

Yikes. That is a problem... will think on this...

mholt · 2016-08-12T06:09:57Z

I will be able to get back to this in a little while... this is a bigger change than I thought it would be!

mholt · 2016-08-22T22:38:10Z

I am kind of wondering if this change would merit a re-design of the entire proxy middleware...

meyskens · 2016-08-23T10:23:30Z

Depends on how you want it to work imo. The most efficient for dynamic will indeed be a redesign of the middleware.

mholt · 2016-08-26T22:41:32Z

@meyskens I think without another one or two people proficient with writing proxy code in Go to review this (as I don't trust such a significant change to myself), I just don't feel comfortable with this change. It's disruptive because of the design of the existing proxy code.

I believe a better way to approach this might be to design the proxy middleware with this feature in mind, rather than kind of hacking it into place. I really appreciate your time and efforts on this issue, it made a lot of progress. But I think this is the end of it for now, until proxy is redesigned and built by contributors. Just leaving this open will not be doing you or I a favor, so I'm going to close it.

Thank you!! Hope you will still contribute in the future. 😄

This PR enables the use of placeholders in an upstream's Dial address. A Dial address must represent precisely one socket after replacements. See also #998 and #1639.

mholt · 2019-10-11T20:28:54Z

This feature has been implemented into Caddy 2, as of 1e31be8.

It was possible in large part due to the experience gained from this pull request, so thank you for that!

Maartje Eyskens added 5 commits August 3, 2016 00:43

Replace placeholders on outgoing proxy request

91dac00

Add NewDynamicHostReverseProxy

23bf143

Remove old code

6364525

Push missing code in upstream.go

75437c4

Fix duplicate code

1e864b2

mholt added the under review 🧐 Review is pending before merging label Aug 4, 2016

Maartje Eyskens added 2 commits August 4, 2016 17:35

Run go fmt

5fe1de7

Merge branch 'master' into master

9d55199

mholt reviewed Aug 5, 2016
View reviewed changes

Maartje Eyskens added 4 commits August 6, 2016 13:43

use dynamic for all {}

47f2563

Parse URL in director

0bcddc6

Merge branch 'master' of github.com:meyskens/caddy

03a9e32

format code

bdc4378

mholt reviewed Aug 6, 2016
View reviewed changes

Maartje Eyskens added 2 commits August 6, 2016 23:01

Removed unneeded replacer

41d1312

run goimports

767a5b5

mholt reviewed Aug 6, 2016
View reviewed changes

Maartje Eyskens added 2 commits August 6, 2016 23:40

Move most director logic to buildHttpRequest

7deb0f1

Never use NewDynamicHostReverseProxy for unix sockets

0c550d3

add dynamicHost to newFakeUpstream

24f2a0a

Fix unix path check

7d69761

arikfr reviewed Aug 7, 2016
View reviewed changes

Maartje Eyskens added 6 commits August 7, 2016 23:23

Correct contains call

1c4da9f

Add more explainin errors

7b36619

Fix string check

fcd717b

Add more clear error messages

73aec36

Remove test code

006d532

Always report up on dynamic

a87c561

Maartje Eyskens added 2 commits August 8, 2016 00:05

Add Dynamic option to UpstreamHost

019a3b5

Use dynamic property

5e84f1b

mholt reviewed Aug 10, 2016
View reviewed changes

mholt added help wanted 🆘 Extra attention is needed deferred ⏰ We'll come back to this later and removed under review 🧐 Review is pending before merging labels Aug 26, 2016

mholt closed this Aug 26, 2016

mholt mentioned this pull request May 1, 2017

proxy: Rewrite the proxy middleware #1639

Closed

mholt removed deferred ⏰ We'll come back to this later help wanted 🆘 Extra attention is needed labels Sep 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow {host} in proxy upstream #998

Allow {host} in proxy upstream #998

meyskens commented Aug 4, 2016

CLAassistant commented Aug 4, 2016 •

edited

mholt commented Aug 4, 2016

theblazehen commented Aug 5, 2016

mholt Aug 5, 2016

meyskens Aug 6, 2016

mholt commented Aug 5, 2016

mholt commented Aug 6, 2016

meyskens commented Aug 6, 2016

meyskens commented Aug 6, 2016

meyskens commented Aug 6, 2016

mholt Aug 6, 2016 •

edited

meyskens Aug 6, 2016

mholt Aug 6, 2016

meyskens Aug 6, 2016

mholt Aug 6, 2016

meyskens Aug 6, 2016

mholt Aug 6, 2016

meyskens Aug 6, 2016

meyskens commented Aug 6, 2016

meyskens commented Aug 6, 2016

arikfr Aug 7, 2016

meyskens commented Aug 7, 2016

meyskens commented Aug 7, 2016

meyskens commented Aug 7, 2016

mholt Aug 10, 2016

meyskens Aug 10, 2016

mholt Aug 10, 2016

mholt commented Aug 12, 2016

mholt commented Aug 22, 2016

meyskens commented Aug 23, 2016

mholt commented Aug 26, 2016 •

edited

mholt commented Oct 11, 2019

Allow {host} in proxy upstream #998

Allow {host} in proxy upstream #998

Conversation

meyskens commented Aug 4, 2016

CLAassistant commented Aug 4, 2016 • edited

mholt commented Aug 4, 2016

theblazehen commented Aug 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mholt commented Aug 5, 2016

mholt commented Aug 6, 2016

meyskens commented Aug 6, 2016

meyskens commented Aug 6, 2016

meyskens commented Aug 6, 2016

mholt Aug 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meyskens commented Aug 6, 2016

meyskens commented Aug 6, 2016

Choose a reason for hiding this comment

meyskens commented Aug 7, 2016

meyskens commented Aug 7, 2016

meyskens commented Aug 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mholt commented Aug 12, 2016

mholt commented Aug 22, 2016

meyskens commented Aug 23, 2016

mholt commented Aug 26, 2016 • edited

mholt commented Oct 11, 2019

CLAassistant commented Aug 4, 2016 •

edited

mholt Aug 6, 2016 •

edited

mholt commented Aug 26, 2016 •

edited