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

Implement redirect capability #395

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions admin/api/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

old syntax

ar.Dst = fmt.Sprintf("%d|%s", tg.RedirectCode, tg.URL)
}
routes = append(routes, ar)
}
}
Expand Down
39 changes: 39 additions & 0 deletions proxy/http_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,45 @@ func TestProxyHost(t *testing.T) {
})
}

func TestRedirect(t *testing.T) {
routes := "route add mock /foo http://a.com/abc opts \"redirect=301\"\n"
routes += "route add mock /bar http://b.com/ opts \"redirect=302\"\n"
tbl, _ := route.NewTable(routes)

proxy := httptest.NewServer(&HTTPProxy{
Transport: http.DefaultTransport,
Lookup: func(r *http.Request) *route.Target {
return tbl.Lookup(r, "", route.Picker["rr"], route.Matcher["prefix"])
},
})
defer proxy.Close()

tests := []struct {
req string
wantCode int
wantLoc string
}{
{req: "/foo", wantCode: 301, wantLoc: "http://a.com/abc"},
{req: "/bar", wantCode: 302, wantLoc: "http://b.com"},
}

http.DefaultClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
// do not follow redirects
return http.ErrUseLastResponse
}

for _, tt := range tests {
resp, _ := mustGet(proxy.URL + tt.req)
if resp.StatusCode != tt.wantCode {
t.Errorf("got status code %d, want %d", resp.StatusCode, tt.wantCode)
}
gotLoc, _ := resp.Location()
if gotLoc.String() != tt.wantLoc {
t.Errorf("got location %s, want %s", gotLoc, tt.wantLoc)
}
}
}

func TestProxyLogOutput(t *testing.T) {
// build a format string from all log fields and one header field
fields := []string{"header.X-Foo:$header.X-Foo"}
Expand Down
9 changes: 9 additions & 0 deletions proxy/http_proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,15 @@ func (p *HTTPProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
r.Header.Set(p.Config.RequestID, id())
}

if t.RedirectCode != 0 {
http.Redirect(w, r, targetURL.String(), t.RedirectCode)
if t.Timer != nil {
t.Timer.Update(0)
}
metrics.DefaultRegistry.GetTimer(key(t.RedirectCode)).Update(0)
return
}

upgrade, accept := r.Header.Get("Upgrade"), r.Header.Get("Accept")

tr := p.Transport
Expand Down
6 changes: 6 additions & 0 deletions registry/consul/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ func TestParseTag(t *testing.T) {
route: "xx/Yy",
ok: true,
},
{
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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ok: true,
},
}

for i, tt := range tests {
Expand Down
2 changes: 2 additions & 0 deletions registry/consul/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, "://"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dst = o
case o == "proto=tcp":
dst = "tcp://" + addr
case o == "proto=https":
Expand Down
11 changes: 9 additions & 2 deletions route/route.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"net/url"
"reflect"
"sort"
"strconv"
"strings"

"github.com/fabiolb/fabio/metrics"
Expand Down Expand Up @@ -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"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

r.Targets = append(r.Targets, t)
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still using the old syntax

s += fmt.Sprintf(" %d|%s", t.RedirectCode, t.URL)
} else {
s += fmt.Sprintf(" %s", t.URL)
}
if addWeight {
s += fmt.Sprintf(" weight %2.4f", t.Weight)
} else if t.FixedWeight > 0 {
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gofmt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

screenshot
screenshot2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this makes sense. My bad.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. You live and learn :)

if dynamic < 0 {
dynamic = 0
}
Expand Down
9 changes: 5 additions & 4 deletions route/table.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, good call.

host = strings.ToLower(host)
if req.TLS == nil && strings.HasSuffix(host, ":80") {
return host[:len(host)-len(":80")]
}
Expand All @@ -287,9 +287,10 @@ func normalizeHost(req *http.Request) string {
// matchingHosts returns all keys (host name patterns) from the
// routing table which match the normalized request hostname.
func (t Table) matchingHosts(req *http.Request) (hosts []string) {
host := normalizeHost(req)
host := normalizeHost(req.Host, req)
for pattern := range t {
if glob.Glob(pattern, host) {
normpat := normalizeHost(pattern, req)
if glob.Glob(normpat, host) {
hosts = append(hosts, pattern)
}
}
Expand Down
9 changes: 8 additions & 1 deletion route/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

old syntax here as well

targetURLs[i] = fmt.Sprintf("%d|%s", tg.RedirectCode, targetURLs[i])
}
}

// count how often the 'url' from 'route add svc <path> <url>'
Expand Down Expand Up @@ -477,7 +480,7 @@ func TestNormalizeHost(t *testing.T) {
}

for i, tt := range tests {
if got, want := normalizeHost(tt.req), tt.host; got != want {
if got, want := normalizeHost(tt.req.Host, tt.req), tt.host; got != want {
t.Errorf("%d: got %v want %v", i, got, want)
}
}
Expand All @@ -495,6 +498,7 @@ func TestTableLookup(t *testing.T) {
route add svc z.abc.com/foo/ http://foo.com:3100
route add svc *.abc.com/ http://foo.com:4000
route add svc *.abc.com/foo/ http://foo.com:5000
route add svc xyz.com:80/ https://xyz.com
`

tbl, err := NewTable(s)
Expand Down Expand Up @@ -539,6 +543,9 @@ func TestTableLookup(t *testing.T) {

// exact match has precedence over glob match
{&http.Request{Host: "z.abc.com", URL: mustParse("/foo/")}, "http://foo.com:3100"},

// explicit port on route
{&http.Request{Host: "xyz.com", URL: mustParse("/")}, "https://xyz.com"},
}

for i, tt := range tests {
Expand Down
3 changes: 3 additions & 0 deletions route/target.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

RedirectCode int

// FixedWeight is the weight assigned to this target.
// If the value is 0 the targets weight is dynamic.
FixedWeight float64
Expand Down