Implement redirect capability #395

ctlajoie · 2017-11-28T15:16:03Z

NOTE: The syntax described below was not used in the final implementation.

This is my initial attempt to implement the ability to perform redirections (see #87). I decided to indicate a redirect by prefixing the destination with the HTTP status code to be used for redirection, followed by a pipe, followed by the specific URL you want to redirect to. There is no built-in protection against creating redirect loops currently. Here is how you might use it:

route add svc example.com:80/ 302|https://example.com
route add svc example.com:443/ 1.2.3.4:5678

Note that these include explicit source ports. To use it on consul services, you have to use multiple service tags.

urlprefix-example.com:80/ 302|https://example.com
urlprefix-example.com:443/

You can create redirect routes to anywhere.

route add svc example.com/dothething/ 301|https://github.com/fabiolb/fabio

Looking for feedback.
(Signed CLA)

magiconair · 2017-11-28T15:31:55Z

Thx a lot. A number of users have been asking for this. I’ll have a look tonight.

aaronhurt · 2017-11-28T15:33:43Z

Yes, we were just discussing doing this last week but it looks like you beat us to it. Thank you, this is one of the few things we're still using HAProxy for on the frontend.

ctlajoie · 2017-11-28T15:54:41Z

Happy to help! I hope the syntax for indicating a redirect is acceptable. It's not even close to what was discussed in #87 but it enables you to redirect to any given URL, which I find valuable.

Also what's up with travis? The failure doesn't seem legit.

aaronhurt · 2017-11-28T15:57:16Z

Looks like golang tip and the Vault checks aren't getting along.

magiconair · 2017-11-28T17:46:40Z

lets disable tip for now then. I've got some cycles in the coming weeks to clean out the issue list. So lets get this done and merged.

magiconair · 2017-11-28T17:59:45Z

This is doing the right thing in the http_proxy.go but the config implementation is more complex than it needs to be IMO. It should be enough if you add a redirect=<code> option since the syntax parsing is mostly there. The notable exception is https://github.com/fabiolb/fabio/blob/master/registry/consul/service.go#L120-L131 which you have covered already and which could use some tests (cough). Then you don't need an addRedirectTarget method and can just add that one option to addTarget method.

I assume you need to make the changes for matching the ports in order to do the http -> https redirect. I'm wondering whether this wouldn't be more expressive

urlprefix-example.com/ https://example.com proto=http redirect=302
urlprefix-example.com/

I'd also like to see an integration test in https://github.com/fabiolb/fabio/blob/master/proxy/http_integration_test.go

magiconair · 2017-11-28T18:00:30Z

It may also make sense to get #385 fixed first. I have the patch but need to add the test.

ctlajoie · 2017-11-28T20:12:50Z

Ok I'll see what I can get done on this tonight. Understood regarding tests.. will add those too.

I assume you need to make the changes for matching the ports in order to do the http -> https redirect.

Correct. This makes it possible to define a route explicitly on http but not https (and vice versa), which was required for this feature.

My understanding is that the proto= option tells fabio what protocol to use for comms with upstream. I wouldn't recommend using it to define route-matching behavior.

magiconair · 2017-11-28T20:14:50Z

The proto= option means whatever we want it to mean. It can mean different things in different contexts if its meaning can be unambiguously derived from the context. Lets get this down to the minimal and tested change first and then we can polish it.

magiconair · 2017-11-28T21:02:10Z

I've just merged #388 so you want to rebase.

ctlajoie · 2017-11-29T00:48:12Z

I implemented your suggestions, minus the proto=http one.
In writing the integration test I discovered some issues that I'm not comfortable resolving without feedback. The integration test fails... have a look at it and I think you'll see the problem. It's copying the Path part of the request URL over to the target. In many cases this will be desired, but not always. I am considering adding another opt like absolute=true to indicate the target URL shouldn't be tampered with. Thoughts?

magiconair · 2017-11-29T06:31:20Z

Got the kids today. So this might be tomorrow.

magiconair

I left some comments but mostly realized that the the syntax I suggested earlier is not sufficient since you need to be able to specify a redirect target. I'm suggesting

redirect=302,http://www.bar.com/foo

but I'm open for suggestions.

magiconair · 2017-11-29T08:32:33Z

route/route.go

@@ -134,7 +136,12 @@ func contains(src, dst []string) bool {
 }

 func (r *Route) TargetConfig(t *Target, addWeight bool) string {
-	s := fmt.Sprintf("route add %s %s %s", t.Service, r.Host+r.Path, t.URL)
+	s := fmt.Sprintf("route add %s %s", t.Service, r.Host+r.Path)
+	if t.RedirectCode != 0 {


This is still using the old syntax

magiconair · 2017-11-29T08:32:46Z

route/route.go

@@ -215,7 +222,7 @@ func (r *Route) weighTargets() {
 	}

 	// compute the weight for the targets with dynamic weights
-	dynamic := (1 - sumFixed) / float64(len(r.Targets)-nFixed)
+	dynamic := float64(1-sumFixed) / float64(len(r.Targets)-nFixed)


I made this change because I thought it was a bug. VS Code was telling me that dynamic is an int. But then I ran some tests under the debugger and it is definitely not an int. Now I know I can't trust VS Code to determine var types correctly.

If you divide something by a float64 then the result can never be an int since Go doesn't auto-convert between numeric types. const values are the only exception.

Ok, this makes sense. My bad.

No worries. You live and learn :)

magiconair · 2017-11-29T08:33:31Z

route/table_test.go

@@ -416,6 +416,9 @@ func TestTableParse(t *testing.T) {
 				targetURLs := make([]string, len(r.wTargets))
 				for i, tg := range r.wTargets {
 					targetURLs[i] = tg.URL.Scheme + "://" + tg.URL.Host + tg.URL.Path
+					if tg.RedirectCode != 0 {


old syntax here as well

magiconair · 2017-11-29T08:38:17Z

route/table.go

@@ -273,8 +273,8 @@ func (t Table) route(host, path string) *Route {

 // normalizeHost returns the hostname from the request
 // and removes the default port if present.
-func normalizeHost(req *http.Request) string {
-	host := strings.ToLower(req.Host)
+func normalizeHost(host string, req *http.Request) string {


The reason I've passed in *http.Request was that I needed both host and the TLS state. If you pass in the host then you might as well pass in the TLS state, e.g.

func normalizeHost(host string, tls bool) string { host = strings.ToLower(host) if !tls && strings.HasSuffix(host, ":80") { return host[:len(host)-len(":80")] } if tls && strings.HasSuffix(host, ":443") { return host[:len(host)-len(":443")] } return host } ... normalizeHost(req.Host, req.TLS != nil)

Yup, good call.

magiconair · 2017-11-29T08:40:38Z

route/target.go

@@ -33,6 +33,9 @@ type Target struct {
 	// URL is the endpoint the service instance listens on
 	URL *url.URL

+	// RedirectCode is the HTTP status code used for redirects.


// RedirectCode is the HTTP status code used for redirects. // When set to a value > 0 the client is redirected to the target url.

magiconair · 2017-11-29T08:41:09Z

route/route.go

@@ -68,6 +69,7 @@ func (r *Route) addTarget(service string, targetURL *url.URL, fixedWeight float6
 		t.StripPath = opts["strip"]
 		t.TLSSkipVerify = opts["tlsskipverify"] == "true"
 		t.Host = opts["host"]
+		t.RedirectCode, _ = strconv.Atoi(opts["redirect"])


Can we log the error or catch this somewhere sooner and log it there?

magiconair · 2017-11-29T08:42:38Z

registry/consul/parse_test.go

+		{
+			tag:   "p-www.bar.com:80/foo https://www.bar.com/ redirect=302",
+			route: "www.bar.com:80/foo",
+			opts:  "https://www.bar.com/ redirect=302",


Ah, I forgot that we need to be able to specify the redirect target. How about this?

redirect=302,http://www.bar.com/

Sure I can do that syntax for the consul service tags.

magiconair · 2017-11-29T08:43:40Z

admin/api/routes.go

@@ -62,6 +62,9 @@ func (h *RoutesHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 					Rate1:   tg.Timer.Rate1(),
 					Pct99:   tg.Timer.Percentile(0.99),
 				}
+				if tg.RedirectCode != 0 {


magiconair · 2017-11-29T08:48:14Z

registry/consul/service.go

@@ -119,6 +119,8 @@ func serviceConfig(client *api.Client, name string, passing map[string]bool, tag
 				dst := "http://" + addr + "/"
 				for _, o := range strings.Fields(opts) {
 					switch {
+					case strings.Contains(o, "://"):


This would then also become more obvious by looking for redirect=xxx

ctlajoie · 2017-11-29T20:18:43Z

route/route.go

+		if opts["redirect"] != "" {
+			t.RedirectCode, err = strconv.Atoi(opts["redirect"])
+			if err != nil {
+				log.Printf("[ERROR] redirect status code should be numeric in 3xx range. Got: %s", opts["redirect"])


Ideally these kind of errors should prevent the route from being added, but that would take some refactoring.

That could be added the route parser but then it would need to treat opts not as an opaque k/v string. I can have a look at this if and when I write a better parser.

magiconair · 2017-11-29T20:36:18Z

You missed one spot:

route/table_test.go:480:48: cannot use tt.req (type *http.Request) as type bool in argument to normalizeHost

ctlajoie · 2017-11-29T20:36:55Z

I made the requested changes but we're not done yet unfortunately. As I mentioned in an earlier comment the integration test fails due to the way the targetURL is constructed in http_proxy.go. With
route add mock /foo http://a.com/abc opts "redirect=301", we get redirected to http://a.com/foo. So I was considering implementing some way to indicate you want the target URL to be absolute, in case this behavior is not desired. Can you comment on that?

magiconair · 2017-11-29T20:40:53Z

Yeah, I'm not sure that matters since the target URL should always be absolute. There is no path mangling in fabio since the routing table is usually generated and the http://host:port/ is really just host:port. Originally, I wanted to keep this open and used URLs for that but this may be misleading. So in fabio you can't forward /foo to /bar/foo. This isn't supported and so far not many have complained about that.

magiconair · 2017-11-29T20:46:14Z

I think the other thing that bugs me a bit but for which I don't have a good answer right now is the continued divergence between the syntax of the urlprefix- tags and the route commands.

urlprefix-/ redirect=302,http://www.google.com/

# translates to
route add svc / http://www.google.com/ opts "redirect=302"

I need to think about this a bit more.

magiconair · 2017-11-29T20:49:20Z

That isn't something that I'd want to address in this PR anyway but I'd like to keep in mind. One approach could be to write a template instead of a special syntax.

fabio <templated route add command>

e.g.

fabio route add $svc / $addr opts "redirect=302"

Then you'd have to learn only one syntax.

ctlajoie · 2017-11-29T20:50:32Z

I'm not so sure we always want it to be absolute. Suppose you have these routes

route add svc example.com:80 / https://example.com redirect=302
route add svc example.com:443 / 1.2.3.4:5678

If you hit http://example.com/some/deep/link you get redirected to https://example.com/some/deep/link. This seems very useful, but I can also see how it might not be desired, depending on the purpose of your redirect route.

magiconair · 2017-11-29T20:51:47Z

How do other load balancers address that?

ctlajoie · 2017-11-29T20:53:26Z

Don't know.. I'll do a bit of research.

magiconair · 2017-11-29T20:54:36Z

Thx. That'd be nice. It's getting a bit late here in NL. Going to hit the pillow since the kids get up early. We're getting there.

aaronhurt · 2017-11-29T21:06:59Z

I can comment on how it's done with HAProxy where we do our http to https redirects.

    redirect scheme https code 301 if { hdr(Host) -i my.ena.com } !{ ssl_fc }

We have that syntax in our main frontend that binds on both 80 and 443 and redirects any request coming in with a Host header of my.ena.com to the exact same URI but using the https scheme.

When I request http://my.ena.com/foo/bar/thing.html I will get a 301 redirect to https://my.ena.com/foo/bar/thing.html with the above configuration.

magiconair · 2017-11-29T21:12:20Z

We could add some magic with a terminating slash or not. redirect=301,http://foo.com would append the path from the original request and redirect=301,http://foo.com/bar would redirect to /bar. That wouldn't solve the path mangling if you want to redirect /foo to /foo/bar or /bar/foo. Another approach could be to add a pseudo-variable like $path at the end. This would give you full control over the redirect target.

request uri: http://bar.com/foo

redirect=301,http://foo.com$path -> http://foo.com/foo
redirect=301,http://foo.com/ -> http://foo.com/
redirect=301,http://foo.com/bar$path -> http://foo.com/bar/foo
redirect=301,http://foo.com/bar/$path -> http://foo.com/bar/foo # ignore double slashes

aaronhurt · 2017-11-29T21:14:38Z

The pseudo variable would be very flexible and is also less ambiguous than magic based on trailing vs non-trailing slash paths.

ctlajoie · 2017-11-29T21:15:52Z

I really like the pseudo-variable idea

ctlajoie · 2017-11-30T19:49:36Z

Pushed changes. Have a close look at http_proxy because I rearranged some code in there since I am no longer using the targetURL var as the redirect target.

magiconair · 2017-11-30T20:00:14Z

@ctlajoie I will. This looks decent so far but I'll let it bake for a couple more days since I want to think a bit more about edge cases. If me or you can't think of anything major then I'll merge this early next week (Mon or Tue). Sounds good?

ctlajoie · 2017-11-30T20:04:08Z

Sounds good. I'm actually using it right now in a semi-production environment (accessed by people working in my office) so it's getting some real-world testing.

magiconair · 2017-11-30T20:04:48Z

Even better.

ctlajoie · 2017-12-05T00:09:57Z

The only thing I can think of that this implementation does not cover would be complex URL rewriting based on regexps (like mod_rewrite). I think it's fair to say that's out of scope.

Do you want me to squash everything down to 1 commit before this gets merged? Are there any other changes you want me to make?

This patch adds support to redirect a request for a matching route to another URL. If the `redirect=<code>` option is set on a route fabio will send a redirect response to the dst address with the given code. The syntax for the `urlprefix-` tag is slightly different since the destination address is usually generated from the service registration stored in Consul. The `$path` pseudo-variable can be used to include the original request URI in the destination target. # redirect /foo to https://www.foo.com/ route add svc /foo https://www.foo.com/ opts "redirect=301" # redirect /foo to https://www.foo.com/ urlprefix-/foo redirect=301,https://www.foo.com/ # redirect /foo to https://www.foo.com/foo urlprefix-/foo redirect=301,https://www.foo.com$path Fixes #87

magiconair · 2017-12-05T08:56:55Z

I've merged this to master. I had to update the demo server to handle the comma in the redirect option but that was a good time to make this change anyway.

Thanks a lot for this. This has been a long requested feature.

ctlajoie · 2017-12-08T19:51:25Z

@magiconair You might want to know that there was/is a problem with current versions of nomad/consul where if you have :// in a service tag, nomad is unable to deregister that service. See hashicorp/nomad#3620

Should be fixed in the next nomad release.

magiconair · 2017-12-08T20:55:32Z

@ctlajoie Thanks. That's good to know.

magiconair added this to the 1.6 milestone Nov 28, 2017

Implement redirection capability

cf30c15

ctlajoie force-pushed the redirect branch from 838ba6b to cf30c15 Compare November 29, 2017 00:35

Ensure RedirectCode is somewhere in the 3xx range

2f2ce08

magiconair reviewed Nov 29, 2017

View reviewed changes

Implement suggestions. General cleanup.

1da6bff

ctlajoie commented Nov 29, 2017

View reviewed changes

typo

d1259f8

Fix TestNormalizeHost

1e83fa7

Implement $path pseudo-variable for redirect targets

1d78ef8

magiconair closed this in 44b8986 Dec 5, 2017

magiconair modified the milestones: 1.6, 1.5.4 Dec 9, 2017

Implement redirect capability #395

Implement redirect capability #395

Conversation

ctlajoie commented Nov 28, 2017 • edited Loading

magiconair commented Nov 28, 2017

aaronhurt commented Nov 28, 2017

ctlajoie commented Nov 28, 2017 • edited Loading

aaronhurt commented Nov 28, 2017

magiconair commented Nov 28, 2017

magiconair commented Nov 28, 2017

magiconair commented Nov 28, 2017

ctlajoie commented Nov 28, 2017

magiconair commented Nov 28, 2017

magiconair commented Nov 28, 2017

ctlajoie commented Nov 29, 2017 • edited Loading

magiconair commented Nov 29, 2017

magiconair left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair commented Nov 29, 2017

ctlajoie commented Nov 29, 2017

magiconair commented Nov 29, 2017

magiconair commented Nov 29, 2017

magiconair commented Nov 29, 2017

ctlajoie commented Nov 29, 2017

magiconair commented Nov 29, 2017

ctlajoie commented Nov 29, 2017

magiconair commented Nov 29, 2017

aaronhurt commented Nov 29, 2017 • edited Loading

magiconair commented Nov 29, 2017

aaronhurt commented Nov 29, 2017 • edited Loading

ctlajoie commented Nov 29, 2017

ctlajoie commented Nov 30, 2017

magiconair commented Nov 30, 2017

ctlajoie commented Nov 30, 2017

magiconair commented Nov 30, 2017

ctlajoie commented Dec 5, 2017

magiconair commented Dec 5, 2017

ctlajoie commented Dec 8, 2017

magiconair commented Dec 8, 2017

ctlajoie commented Nov 28, 2017 •

edited

Loading

ctlajoie commented Nov 28, 2017 •

edited

Loading

ctlajoie commented Nov 29, 2017 •

edited

Loading

aaronhurt commented Nov 29, 2017 •

edited

Loading

aaronhurt commented Nov 29, 2017 •

edited

Loading