From e1d2949b18e8c6ae66ce0e545ab700ceaa5983fc Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 31 Mar 2017 11:20:44 -0400 Subject: [PATCH 1/6] Implement a MountableRouter The httprouter used in Chronograf did not support prefixing every route with some basepath. This caused problems for those using the --basepath parameter in combination with a load balancer that did not strip the basepath prefix from requests that it forwarded onto Chronograf. To support this, MountableRouter prefixes all routes at definition time with the supplied prefix. --- chronograf.go | 13 ++ server/mountable_router.go | 58 ++++++++ server/mountable_router_test.go | 240 ++++++++++++++++++++++++++++++++ 3 files changed, 311 insertions(+) create mode 100644 server/mountable_router.go create mode 100644 server/mountable_router_test.go diff --git a/chronograf.go b/chronograf.go index 83a246703b..33ab74f204 100644 --- a/chronograf.go +++ b/chronograf.go @@ -42,6 +42,19 @@ type Logger interface { Writer() *io.PipeWriter } +// Router is an abstracted Router based on the API provided by the +// julienschmidt/httprouter package. +type Router interface { + http.Handler + GET(string, http.HandlerFunc) + PATCH(string, http.HandlerFunc) + POST(string, http.HandlerFunc) + DELETE(string, http.HandlerFunc) + PUT(string, http.HandlerFunc) + + Handler(string, string, http.Handler) +} + // Assets returns a handler to serve the website. type Assets interface { Handler() http.Handler diff --git a/server/mountable_router.go b/server/mountable_router.go new file mode 100644 index 0000000000..387c0016b5 --- /dev/null +++ b/server/mountable_router.go @@ -0,0 +1,58 @@ +package server + +import ( + "net/http" + + "github.com/influxdata/chronograf" +) + +var _ chronograf.Router = &MountableRouter{} + +// MountableRouter is an implementation of a chronograf.Router which supports +// prefixing each route of a Delegated chronograf.Router with a prefix. +type MountableRouter struct { + Prefix string + Delegate chronograf.Router +} + +// DELETE defines a route responding to a DELETE request that will be prefixed +// with the configured route prefix +func (mr *MountableRouter) DELETE(path string, handler http.HandlerFunc) { + mr.Delegate.DELETE(mr.Prefix+path, handler) +} + +// GET defines a route responding to a GET request that will be prefixed +// with the configured route prefix +func (mr *MountableRouter) GET(path string, handler http.HandlerFunc) { + mr.Delegate.GET(mr.Prefix+path, handler) +} + +// POST defines a route responding to a POST request that will be prefixed +// with the configured route prefix +func (mr *MountableRouter) POST(path string, handler http.HandlerFunc) { + mr.Delegate.POST(mr.Prefix+path, handler) +} + +// PUT defines a route responding to a PUT request that will be prefixed +// with the configured route prefix +func (mr *MountableRouter) PUT(path string, handler http.HandlerFunc) { + mr.Delegate.PUT(mr.Prefix+path, handler) +} + +// PATCH defines a route responding to a PATCH request that will be prefixed +// with the configured route prefix +func (mr *MountableRouter) PATCH(path string, handler http.HandlerFunc) { + mr.Delegate.PATCH(mr.Prefix+path, handler) +} + +// Handler defines a prefixed route responding to a request type specified in +// the method parameter +func (mr *MountableRouter) Handler(method string, path string, handler http.Handler) { + mr.Delegate.Handler(method, mr.Prefix+path, handler) +} + +// ServeHTTP is an implementation of http.Handler which delegates to the +// configured Delegate's implementation of http.Handler +func (mr *MountableRouter) ServeHTTP(rw http.ResponseWriter, r *http.Request) { + mr.Delegate.ServeHTTP(rw, r) +} diff --git a/server/mountable_router_test.go b/server/mountable_router_test.go new file mode 100644 index 0000000000..6c8b2bb392 --- /dev/null +++ b/server/mountable_router_test.go @@ -0,0 +1,240 @@ +package server_test + +import ( + "encoding/json" + "fmt" + "io" + "io/ioutil" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/bouk/httprouter" + "github.com/influxdata/chronograf/server" +) + +func Test_MountableRouter_MountsRoutesUnderPrefix(t *testing.T) { + t.Parallel() + + mr := &server.MountableRouter{ + Prefix: "/chronograf", + Delegate: httprouter.New(), + } + + expected := "Hello?! McFly?! Anybody in there?!" + mr.GET("/biff", http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + fmt.Fprintf(rw, expected) + })) + + ts := httptest.NewServer(mr) + defer ts.Close() + + resp, err := http.Get(ts.URL + "/chronograf/biff") + if err != nil { + t.Fatal("Unexpected error fetching from mounted router: err:", err) + } + + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + t.Fatal("Unexpected error decoding response body: err:", err) + } + + if resp.StatusCode != http.StatusOK { + t.Fatal("Expected 200 but received", resp.StatusCode) + } + + if string(body) != expected { + t.Fatalf("Unexpected response body: Want: \"%s\". Got: \"%s\"", expected, string(body)) + } +} + +func Test_MountableRouter_PrefixesPosts(t *testing.T) { + t.Parallel() + + mr := &server.MountableRouter{ + Prefix: "/chronograf", + Delegate: httprouter.New(), + } + + expected := "Great Scott!" + actual := make([]byte, len(expected)) + mr.POST("/doc", http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + defer r.Body.Close() + if _, err := io.ReadFull(r.Body, actual); err != nil { + rw.WriteHeader(http.StatusInternalServerError) + } else { + rw.WriteHeader(http.StatusOK) + } + })) + + ts := httptest.NewServer(mr) + defer ts.Close() + + resp, err := http.Post(ts.URL+"/chronograf/doc", "text/plain", strings.NewReader(expected)) + if err != nil { + t.Fatal("Unexpected error posting to mounted router: err:", err) + } + + if resp.StatusCode != http.StatusOK { + t.Fatal("Expected 200 but received", resp.StatusCode) + } + + if string(actual) != expected { + t.Fatalf("Unexpected request body: Want: \"%s\". Got: \"%s\"", expected, string(actual)) + } +} + +func Test_MountableRouter_PrefixesPuts(t *testing.T) { + t.Parallel() + + mr := &server.MountableRouter{ + Prefix: "/chronograf", + Delegate: httprouter.New(), + } + + expected := "Great Scott!" + actual := make([]byte, len(expected)) + mr.PUT("/doc", http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + defer r.Body.Close() + if _, err := io.ReadFull(r.Body, actual); err != nil { + rw.WriteHeader(http.StatusInternalServerError) + } else { + rw.WriteHeader(http.StatusOK) + } + })) + + ts := httptest.NewServer(mr) + defer ts.Close() + + req := httptest.NewRequest(http.MethodPut, ts.URL+"/chronograf/doc", strings.NewReader(expected)) + req.Header.Set("Content-Type", "text/plain; charset=utf-8") + req.Header.Set("Content-Length", fmt.Sprintf("%d", len(expected))) + req.RequestURI = "" + + client := http.Client{} + resp, err := client.Do(req) + if err != nil { + t.Fatal("Unexpected error posting to mounted router: err:", err) + } + + if resp.StatusCode != http.StatusOK { + t.Fatal("Expected 200 but received", resp.StatusCode) + } + + if string(actual) != expected { + t.Fatalf("Unexpected request body: Want: \"%s\". Got: \"%s\"", expected, string(actual)) + } +} + +func Test_MountableRouter_PrefixesDeletes(t *testing.T) { + t.Parallel() + + mr := &server.MountableRouter{ + Prefix: "/chronograf", + Delegate: httprouter.New(), + } + + mr.DELETE("/proto1985", http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusNoContent) + })) + + ts := httptest.NewServer(mr) + defer ts.Close() + + req := httptest.NewRequest(http.MethodDelete, ts.URL+"/chronograf/proto1985", nil) + req.RequestURI = "" + + client := http.Client{} + resp, err := client.Do(req) + if err != nil { + t.Fatal("Unexpected error sending request to mounted router: err:", err) + } + + if resp.StatusCode != http.StatusNoContent { + t.Fatal("Expected 204 but received", resp.StatusCode) + } +} + +func Test_MountableRouter_PrefixesPatches(t *testing.T) { + t.Parallel() + + type Character struct { + Name string + Items []string + } + + mr := &server.MountableRouter{ + Prefix: "/chronograf", + Delegate: httprouter.New(), + } + + biff := Character{"biff", []string{"sports almanac"}} + mr.PATCH("/1955", http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + defer r.Body.Close() + c := Character{} + err := json.NewDecoder(r.Body).Decode(&c) + if err != nil { + rw.WriteHeader(http.StatusBadRequest) + } else { + biff.Items = c.Items + rw.WriteHeader(http.StatusOK) + } + })) + + ts := httptest.NewServer(mr) + defer ts.Close() + + r, w := io.Pipe() + go func() { + _ = json.NewEncoder(w).Encode(Character{"biff", []string{}}) + w.Close() + }() + + req := httptest.NewRequest(http.MethodPatch, ts.URL+"/chronograf/1955", r) + req.RequestURI = "" + + client := http.Client{} + resp, err := client.Do(req) + if err != nil { + t.Fatal("Unexpected error sending request to mounted router: err:", err) + } + + if resp.StatusCode != http.StatusOK { + t.Fatal("Expected 200 but received", resp.StatusCode) + } + + if len(biff.Items) != 0 { + t.Fatal("Failed to alter history, biff still has the sports almanac") + } +} + +func Test_MountableRouter_PrefixesHandler(t *testing.T) { + t.Parallel() + + mr := &server.MountableRouter{ + Prefix: "/chronograf", + Delegate: httprouter.New(), + } + + mr.Handler(http.MethodGet, "/recklessAmountOfPower", http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusOK) + rw.Write([]byte("1.21 Gigawatts!")) + })) + + ts := httptest.NewServer(mr) + defer ts.Close() + + req := httptest.NewRequest(http.MethodGet, ts.URL+"/chronograf/recklessAmountOfPower", nil) + req.RequestURI = "" + + client := http.Client{} + resp, err := client.Do(req) + if err != nil { + t.Fatal("Unexpected error sending request to mounted router: err:", err) + } + + if resp.StatusCode != http.StatusOK { + t.Fatal("Expected 200 but received", resp.StatusCode) + } +} From 77ede663478c13656317a22a66909d8bb0a5fe0e Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 31 Mar 2017 16:14:46 -0400 Subject: [PATCH 2/6] Use MountableRouter when Basepath is set This breaks compatibility with the old behavior of --basepath, so this requires that proxies be configured to not modify routes forwarded to backends. The old behavior will be supported in a subsequent commit. --- server/mux.go | 22 +++++++++++++++++++--- server/server.go | 3 +++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/server/mux.go b/server/mux.go index 49c69cdc76..8f5bee049a 100644 --- a/server/mux.go +++ b/server/mux.go @@ -31,7 +31,7 @@ type MuxOpts struct { // NewMux attaches all the route handlers; handler returned servers chronograf. func NewMux(opts MuxOpts, service Service) http.Handler { - router := httprouter.New() + hr := httprouter.New() /* React Application */ assets := Assets(AssetsOpts{ @@ -42,13 +42,29 @@ func NewMux(opts MuxOpts, service Service) http.Handler { // Prefix any URLs found in the React assets with any configured basepath prefixedAssets := NewDefaultURLPrefixer(basepath, assets, opts.Logger) + // Set route prefix for all routes if basepath is present + var router chronograf.Router + if opts.Basepath != "" { + router = &MountableRouter{ + Prefix: opts.Basepath, + Delegate: hr, + } + } else { + router = hr + } + // Compress the assets with gzip if an accepted encoding compressed := gziphandler.GzipHandler(prefixedAssets) // The react application handles all the routing if the server does not // know about the route. This means that we never have unknown // routes on the server. - router.NotFound = compressed + hr.NotFound = http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + if opts.Basepath != "" { + r.URL.Path = r.URL.Path[len(opts.Basepath):] + } + compressed.ServeHTTP(rw, r) + }) /* Documentation */ router.GET("/swagger.json", Spec()) @@ -178,7 +194,7 @@ func NewMux(opts MuxOpts, service Service) http.Handler { // AuthAPI adds the OAuth routes if auth is enabled. // TODO: this function is not great. Would be good if providers added their routes. -func AuthAPI(opts MuxOpts, router *httprouter.Router) (http.Handler, AuthRoutes) { +func AuthAPI(opts MuxOpts, router chronograf.Router) (http.Handler, AuthRoutes) { auth := oauth2.NewJWT(opts.TokenSecret) routes := AuthRoutes{} for _, pf := range opts.ProviderFuncs { diff --git a/server/server.go b/server/server.go index 4efb881234..a7a942655b 100644 --- a/server/server.go +++ b/server/server.go @@ -14,7 +14,9 @@ import ( "github.com/influxdata/chronograf" "github.com/influxdata/chronograf/bolt" + "github.com/influxdata/chronograf/canned" "github.com/influxdata/chronograf/influx" + "github.com/influxdata/chronograf/layouts" clog "github.com/influxdata/chronograf/log" "github.com/influxdata/chronograf/oauth2" "github.com/influxdata/chronograf/uuid" @@ -217,6 +219,7 @@ func (s *Server) Serve(ctx context.Context) error { Logger: logger, UseAuth: s.useAuth(), ProviderFuncs: providerFuncs, + Basepath: basepath, }, service) // Add chronograf's version header to all requests From df1946900250e6147dbe1acb613382ac0a9e8f79 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Mon, 3 Apr 2017 16:41:05 -0400 Subject: [PATCH 3/6] Add parameter to control mounting behavior Some load balancers will strip prefixes on their way to the chronograf backend, others won't. The "--prefix-routes" parameter forces all requests to the backend to have the prefix specified in "--basepath". Omitting it will only cause routes to be rewritten in rendered templates and assumes that the load balancer will remove the prefix. Use with Caddy ============== An easy way to test this out is using the free Caddy http server at http://caddyserver.com. This Caddyfile will work with the options `--basepath /chronograf --prefix-routes` set: ``` localhost:2020 { proxy /chronograf localhost:8888 log stdout } ``` This Caddyfile will work with only the option `--basepath /chronograf` set: ``` localhost:2020 { proxy /chronograf localhost:8888 { except /chronograf } log stdout } ``` --- server/mux.go | 17 ++++++++++------- server/server.go | 4 ++-- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/server/mux.go b/server/mux.go index 8f5bee049a..844c1b76bc 100644 --- a/server/mux.go +++ b/server/mux.go @@ -20,11 +20,12 @@ const ( // MuxOpts are the options for the router. Mostly related to auth. type MuxOpts struct { - Logger chronograf.Logger - Develop bool // Develop loads assets from filesystem instead of bindata - Basepath string // URL path prefix under which all chronograf routes will be mounted - UseAuth bool // UseAuth turns on Github OAuth and JWT - TokenSecret string + Logger chronograf.Logger + Develop bool // Develop loads assets from filesystem instead of bindata + Basepath string // URL path prefix under which all chronograf routes will be mounted + PrefixRoutes bool // Mounts all backend routes under route specified by the Basepath + UseAuth bool // UseAuth turns on Github OAuth and JWT + TokenSecret string ProviderFuncs []func(func(oauth2.Provider, oauth2.Mux)) } @@ -44,7 +45,7 @@ func NewMux(opts MuxOpts, service Service) http.Handler { // Set route prefix for all routes if basepath is present var router chronograf.Router - if opts.Basepath != "" { + if opts.Basepath != "" && opts.PrefixRoutes { router = &MountableRouter{ Prefix: opts.Basepath, Delegate: hr, @@ -60,7 +61,9 @@ func NewMux(opts MuxOpts, service Service) http.Handler { // know about the route. This means that we never have unknown // routes on the server. hr.NotFound = http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - if opts.Basepath != "" { + // The assets handler is always unaware of basepaths, so it needs to always + // be removed before sending requests to it + if opts.Basepath != "" && opts.PrefixRoutes { r.URL.Path = r.URL.Path[len(opts.Basepath):] } compressed.ServeHTTP(rw, r) diff --git a/server/server.go b/server/server.go index a7a942655b..bb77d6006c 100644 --- a/server/server.go +++ b/server/server.go @@ -14,9 +14,7 @@ import ( "github.com/influxdata/chronograf" "github.com/influxdata/chronograf/bolt" - "github.com/influxdata/chronograf/canned" "github.com/influxdata/chronograf/influx" - "github.com/influxdata/chronograf/layouts" clog "github.com/influxdata/chronograf/log" "github.com/influxdata/chronograf/oauth2" "github.com/influxdata/chronograf/uuid" @@ -71,6 +69,7 @@ type Server struct { ReportingDisabled bool `short:"r" long:"reporting-disabled" description:"Disable reporting of usage stats (os,arch,version,cluster_id,uptime) once every 24hr" env:"REPORTING_DISABLED"` LogLevel string `short:"l" long:"log-level" value-name:"choice" choice:"debug" choice:"info" choice:"error" default:"info" description:"Set the logging level" env:"LOG_LEVEL"` Basepath string `short:"p" long:"basepath" description:"A URL path prefix under which all chronograf routes will be mounted" env:"BASE_PATH"` + PrefixRoutes bool `long:"prefix-routes" description:"Force chronograf server to require that all requests to it are prefixed with the value set in --basepath" env:"PREFIX_ROUTES"` ShowVersion bool `short:"v" long:"version" description:"Show Chronograf version info"` BuildInfo BuildInfo Listener net.Listener @@ -220,6 +219,7 @@ func (s *Server) Serve(ctx context.Context) error { UseAuth: s.useAuth(), ProviderFuncs: providerFuncs, Basepath: basepath, + PrefixRoutes: s.PrefixRoutes, }, service) // Add chronograf's version header to all requests From 1376f522b6650711e728bed8331bc3e086d7178d Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 4 Apr 2017 10:59:16 -0400 Subject: [PATCH 4/6] Update CHANGELOG with --prefix-routes changes --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fcec9c37dd..9c9fdf16d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ 1. [#1120](https://github.com/influxdata/chronograf/pull/1120): Allow users to update user passwords. 1. [#1129](https://github.com/influxdata/chronograf/pull/1129): Allow InfluxDB and Kapacitor configuration via ENV vars or CLI options 1. [#1130](https://github.com/influxdata/chronograf/pull/1130): Add loading spinner to Alert History page. + 1. [#1168](https://github.com/influxdata/chronograf/issue/1168): Expand support for --basepath on some load balancers ### UI Improvements 1. [#1101](https://github.com/influxdata/chronograf/pull/1101): Compress InfluxQL responses with gzip From d04483f779fdfd3717f87a2643fc7d3ffc60a5e0 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 4 Apr 2017 14:03:46 -0400 Subject: [PATCH 5/6] Favor http.StripPrefix over home-rolled version http.StripPrefix is a standard library handler which is designed to do exactly what the inline http.HandlerFunc did (with almost the same implementation). --- server/mux.go | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/server/mux.go b/server/mux.go index 844c1b76bc..95d1741cb7 100644 --- a/server/mux.go +++ b/server/mux.go @@ -43,6 +43,9 @@ func NewMux(opts MuxOpts, service Service) http.Handler { // Prefix any URLs found in the React assets with any configured basepath prefixedAssets := NewDefaultURLPrefixer(basepath, assets, opts.Logger) + // Compress the assets with gzip if an accepted encoding + compressed := gziphandler.GzipHandler(prefixedAssets) + // Set route prefix for all routes if basepath is present var router chronograf.Router if opts.Basepath != "" && opts.PrefixRoutes { @@ -50,25 +53,16 @@ func NewMux(opts MuxOpts, service Service) http.Handler { Prefix: opts.Basepath, Delegate: hr, } + // The react application handles all the routing if the server does not + // know about the route. This means that we never have unknown routes on + // the server. The assets handler is always unaware of basepaths, so the + // basepath needs to always be removed before sending requests to it + hr.NotFound = http.StripPrefix(opts.Basepath, compressed) } else { router = hr + hr.NotFound = compressed } - // Compress the assets with gzip if an accepted encoding - compressed := gziphandler.GzipHandler(prefixedAssets) - - // The react application handles all the routing if the server does not - // know about the route. This means that we never have unknown - // routes on the server. - hr.NotFound = http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - // The assets handler is always unaware of basepaths, so it needs to always - // be removed before sending requests to it - if opts.Basepath != "" && opts.PrefixRoutes { - r.URL.Path = r.URL.Path[len(opts.Basepath):] - } - compressed.ServeHTTP(rw, r) - }) - /* Documentation */ router.GET("/swagger.json", Spec()) router.GET("/docs", Redoc("/swagger.json")) From 8a51adbced43aed0e0e677b16732a3a644e6cb29 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 4 Apr 2017 15:59:09 -0400 Subject: [PATCH 6/6] Remove unnecessary conditional tests Re-mounting should only happen if the --prefix-routes option is set. If this happens, the result will be a no-op as intended since the --basepath will be "". MountableRouter and http.StripPrefix are both no-ops with prefix set to "" --- server/mux.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/server/mux.go b/server/mux.go index 95d1741cb7..a32345e103 100644 --- a/server/mux.go +++ b/server/mux.go @@ -46,21 +46,23 @@ func NewMux(opts MuxOpts, service Service) http.Handler { // Compress the assets with gzip if an accepted encoding compressed := gziphandler.GzipHandler(prefixedAssets) + // The react application handles all the routing if the server does not + // know about the route. This means that we never have unknown routes on + // the server. + hr.NotFound = compressed + + var router chronograf.Router = hr + // Set route prefix for all routes if basepath is present - var router chronograf.Router - if opts.Basepath != "" && opts.PrefixRoutes { + if opts.PrefixRoutes { router = &MountableRouter{ Prefix: opts.Basepath, Delegate: hr, } - // The react application handles all the routing if the server does not - // know about the route. This means that we never have unknown routes on - // the server. The assets handler is always unaware of basepaths, so the + + //The assets handler is always unaware of basepaths, so the // basepath needs to always be removed before sending requests to it - hr.NotFound = http.StripPrefix(opts.Basepath, compressed) - } else { - router = hr - hr.NotFound = compressed + hr.NotFound = http.StripPrefix(opts.Basepath, hr.NotFound) } /* Documentation */