From 11087ea49f3addb4df0f37d453c6d7fa9159bf4c Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Fri, 29 Apr 2016 02:00:45 -0700 Subject: [PATCH 1/7] traceapp: take a base URL parameter in New This change requires that users of traceapp pass a base URL parameter. This is needed in order to create absolute URLs to e.g. traces for permalinks. Prior to this change, we would get this base URL via `github.com/gorilla/mux.Route.URLTo` but this notably enforces an HTTP-only scheme ([source code here](https://github.com/gorilla/mux/blob/master/route.go#L473)) which prevents Appdash permalinks from working on HTTPS hosted systems. This is fixed with this change, by requiring an explicit base URL to be given as a parameter. --- traceapp/app.go | 27 +++++++++++++++++++++++---- traceapp/tmpl.go | 6 +----- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/traceapp/app.go b/traceapp/app.go index f5e04f97..607591f0 100644 --- a/traceapp/app.go +++ b/traceapp/app.go @@ -17,6 +17,7 @@ import ( "encoding/base64" "encoding/json" "errors" + "fmt" htmpl "html/template" "io/ioutil" "log" @@ -45,17 +46,35 @@ type App struct { tmplLock sync.Mutex tmpls map[string]*htmpl.Template - Log *log.Logger + Log *log.Logger + baseURL *url.URL } // New creates a new application handler. If r is nil, a new router is // created. -func New(r *Router) *App { +// +// The given base URL is the absolute base URL under which traceapp is being +// served, e.g., "https://appdash.mysite.com" or "https://mysite.com/appdash". +// The base URL must contain a scheme and host, or else an error will be +// returned. +func New(r *Router, base *url.URL) (*App, error) { if r == nil { r = NewRouter(nil) } - app := &App{Router: r, Log: log.New(os.Stderr, "appdash: ", log.LstdFlags)} + // Validate the base URL and use the root path if none was specified. + if base.Scheme == "" || base.Host == "" { + return nil, fmt.Errorf("appdash: base URL must contain both scheme and port, found %q", base.String()) + } + if base.Path == "" { + base.Path = "/" + } + + app := &App{ + Router: r, + Log: log.New(os.Stderr, "appdash: ", log.LstdFlags), + baseURL: base, + } r.r.Get(RootRoute).Handler(handlerFunc(app.serveRoot)) r.r.Get(TraceRoute).Handler(handlerFunc(app.serveTrace)) @@ -71,7 +90,7 @@ func New(r *Router) *App { // Static file serving. r.r.Get(StaticRoute).Handler(http.StripPrefix("/static/", http.FileServer(static.Data))) - return app + return app, nil } // ServeHTTP implements http.Handler. diff --git a/traceapp/tmpl.go b/traceapp/tmpl.go index c1c8885d..fcfc4f7d 100644 --- a/traceapp/tmpl.go +++ b/traceapp/tmpl.go @@ -61,14 +61,10 @@ func (a *App) renderTemplate(w http.ResponseWriter, r *http.Request, name string if data != nil { // Set TemplateCommon values. - baseURL, err := a.URLTo(RootRoute) - if err != nil { - return err - } reflect.ValueOf(data).Elem().FieldByName("TemplateCommon").Set(reflect.ValueOf(TemplateCommon{ CurrentRoute: mux.CurrentRoute(r).GetName(), CurrentURI: r.URL, - BaseURL: baseURL, + BaseURL: a.baseURL, })) } From 331a864de5df08b26cc53b2674fdbc54b6b27249 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Fri, 29 Apr 2016 02:00:56 -0700 Subject: [PATCH 2/7] cmd/appdash: upgrade to new traceapp.New API --- cmd/appdash/example_app.go | 5 ++++- cmd/appdash/serve_cmd.go | 11 ++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/cmd/appdash/example_app.go b/cmd/appdash/example_app.go index 0b71ec39..87cdc6bf 100644 --- a/cmd/appdash/example_app.go +++ b/cmd/appdash/example_app.go @@ -66,7 +66,10 @@ func (c *DemoCmd) Execute(args []string) error { log.Printf("Appdash web UI running at %s", appdashURL) // Start the web UI in a separate goroutine. - tapp := traceapp.New(nil) + tapp, err := traceapp.New(nil, appdashURL) + if err != nil { + log.Fatal(err) + } tapp.Store = store tapp.Queryer = store go func() { diff --git a/cmd/appdash/serve_cmd.go b/cmd/appdash/serve_cmd.go index 28fc953a..c314c5d8 100644 --- a/cmd/appdash/serve_cmd.go +++ b/cmd/appdash/serve_cmd.go @@ -9,6 +9,7 @@ import ( "log" "net" "net/http" + "net/url" "os" "time" @@ -32,6 +33,7 @@ func init() { // ServeCmd is the command for running Appdash in server mode, where a // collector server and the web UI are hosted. type ServeCmd struct { + URL string `long:"url" description:"URL which Appdash is being hosted at" default:"http://localhost:7700"` CollectorAddr string `long:"collector" description:"collector listen address" default:":7701"` HTTPAddr string `long:"http" description:"HTTP listen address" default:":7700"` SampleData bool `long:"sample-data" description:"add sample data"` @@ -94,7 +96,14 @@ func (c *ServeCmd) Execute(args []string) error { } } - app := traceapp.New(nil) + url, err := url.Parse(c.URL) + if err != nil { + log.Fatal(err) + } + app, err := traceapp.New(nil, url) + if err != nil { + log.Fatal(err) + } app.Store = Store app.Queryer = Queryer From a6db917d0a7fdaf235b10eb4061cdfc19bbed511 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Fri, 29 Apr 2016 02:01:15 -0700 Subject: [PATCH 3/7] example/cmd: upgrade to new traceapp.New API --- examples/cmd/webapp-influxdb/main.go | 10 +++++++++- examples/cmd/webapp-opentracing/main.go | 10 +++++++++- examples/cmd/webapp/main.go | 10 +++++++++- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/examples/cmd/webapp-influxdb/main.go b/examples/cmd/webapp-influxdb/main.go index a4545edc..a26a0b32 100644 --- a/examples/cmd/webapp-influxdb/main.go +++ b/examples/cmd/webapp-influxdb/main.go @@ -4,6 +4,7 @@ import ( "fmt" "log" "net/http" + "net/url" "time" "sourcegraph.com/sourcegraph/appdash" @@ -57,7 +58,14 @@ func main() { log.Fatal(err) } }() - tapp := traceapp.New(nil) + url, err := url.Parse("http://localhost:8700") + if err != nil { + log.Fatal(err) + } + tapp, err := traceapp.New(nil, url) + if err != nil { + log.Fatal(err) + } tapp.Store = store tapp.Queryer = store tapp.Aggregator = store diff --git a/examples/cmd/webapp-opentracing/main.go b/examples/cmd/webapp-opentracing/main.go index 5d77d43b..e07aa33f 100644 --- a/examples/cmd/webapp-opentracing/main.go +++ b/examples/cmd/webapp-opentracing/main.go @@ -9,6 +9,7 @@ import ( "fmt" "log" "net/http" + "net/url" "os" "strings" "time" @@ -42,7 +43,14 @@ func main() { // on HTTP port 8700 will bring us to the web UI, displaying information // about this specific web-server (another alternative would be to connect // to a centralized Appdash collection server). - tapp := traceapp.New(nil) + url, err := url.Parse("http://localhost:8700") + if err != nil { + log.Fatal(err) + } + tapp, err := traceapp.New(nil, url) + if err != nil { + log.Fatal(err) + } tapp.Store = store tapp.Queryer = memStore log.Println("Appdash web UI running on HTTP :8700") diff --git a/examples/cmd/webapp/main.go b/examples/cmd/webapp/main.go index 9e1b2317..218d1e0a 100644 --- a/examples/cmd/webapp/main.go +++ b/examples/cmd/webapp/main.go @@ -9,6 +9,7 @@ import ( "fmt" "log" "net/http" + "net/url" "time" "sourcegraph.com/sourcegraph/appdash" @@ -49,7 +50,14 @@ func main() { // on HTTP port 8700 will bring us to the web UI, displaying information // about this specific web-server (another alternative would be to connect // to a centralized Appdash collection server). - tapp := traceapp.New(nil) + url, err := url.Parse("http://localhost:8700") + if err != nil { + log.Fatal(err) + } + tapp, err := traceapp.New(nil, url) + if err != nil { + log.Fatal(err) + } tapp.Store = store tapp.Queryer = memStore log.Println("Appdash web UI running on HTTP :8700") From ddc65a319178eaf7e884b819fd2897ab3d4b2ad6 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Fri, 29 Apr 2016 02:19:12 -0700 Subject: [PATCH 4/7] CHANGELOG: mention traceapp.New change --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b06857e..0f0114f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # Changelog +- Apr 29, 2016 - **Breaking Change!** + - [#162](https://github.com/sourcegraph/appdash/pull/162) `traceapp.New` now requires a base URL parameter for compatability with HTTPS in trace permalinks. - Apr 26, 2016 - [#153](https://github.com/sourcegraph/appdash/pull/153) Added a Recorder.Logger field which, when non-nil, causes errors to be logged instead of checked explicitly via the Errors method. - [#154](https://github.com/sourcegraph/appdash/pull/154) Added trace permalinks which encode the trace within the URL. From 17c8403e7c2fd4d34984484364a215d103b006f0 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Mon, 9 May 2016 17:06:55 -0700 Subject: [PATCH 5/7] traceapp: s/port/host/g typo --- traceapp/app.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/traceapp/app.go b/traceapp/app.go index 607591f0..a6a5a3a1 100644 --- a/traceapp/app.go +++ b/traceapp/app.go @@ -64,7 +64,7 @@ func New(r *Router, base *url.URL) (*App, error) { // Validate the base URL and use the root path if none was specified. if base.Scheme == "" || base.Host == "" { - return nil, fmt.Errorf("appdash: base URL must contain both scheme and port, found %q", base.String()) + return nil, fmt.Errorf("appdash: base URL must contain both scheme and host, found %q", base.String()) } if base.Path == "" { base.Path = "/" From 48283ad56ecbabe46ae90bf4c2c997569190b1c2 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Mon, 9 May 2016 17:11:28 -0700 Subject: [PATCH 6/7] traceapp: properly copy the base URL parameter in New --- traceapp/app.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/traceapp/app.go b/traceapp/app.go index a6a5a3a1..320af1a2 100644 --- a/traceapp/app.go +++ b/traceapp/app.go @@ -63,6 +63,8 @@ func New(r *Router, base *url.URL) (*App, error) { } // Validate the base URL and use the root path if none was specified. + cpy := *base + base = &cpy if base.Scheme == "" || base.Host == "" { return nil, fmt.Errorf("appdash: base URL must contain both scheme and host, found %q", base.String()) } From cbf71cd120822c52b37b814f36280b6ae532e14c Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Mon, 9 May 2016 18:20:02 -0700 Subject: [PATCH 7/7] cmd/appdash: choose better `appdash serve --url` default value (based on --http addr). This causes `appdash serve` to always choose a better default for `--url` when not specified. Prior to this it would choose a hard-coded default which matched the `--http` default, but that default would not work when changing `--http`. --- cmd/appdash/serve_cmd.go | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/cmd/appdash/serve_cmd.go b/cmd/appdash/serve_cmd.go index c314c5d8..67f2a6a2 100644 --- a/cmd/appdash/serve_cmd.go +++ b/cmd/appdash/serve_cmd.go @@ -33,7 +33,7 @@ func init() { // ServeCmd is the command for running Appdash in server mode, where a // collector server and the web UI are hosted. type ServeCmd struct { - URL string `long:"url" description:"URL which Appdash is being hosted at" default:"http://localhost:7700"` + URL string `long:"url" description:"URL which Appdash is being hosted at (e.g. http://localhost:7700)"` CollectorAddr string `long:"collector" description:"collector listen address" default:":7701"` HTTPAddr string `long:"http" description:"HTTP listen address" default:":7700"` SampleData bool `long:"sample-data" description:"add sample data"` @@ -96,7 +96,7 @@ func (c *ServeCmd) Execute(args []string) error { } } - url, err := url.Parse(c.URL) + url, err := c.urlOrDefault() if err != nil { log.Fatal(err) } @@ -173,6 +173,29 @@ func (c *ServeCmd) Execute(args []string) error { return http.ListenAndServe(c.HTTPAddr, h) } +// urlOrDefault returns c.URL if non-empty, otherwise it returns c.HTTPAddr +// with localhost" as the default host (if not specified in c.HTTPAddr). +func (c *ServeCmd) urlOrDefault() (*url.URL, error) { + // Parse c.URL and return it if non-empty. + u, err := url.Parse(c.URL) + if err != nil { + return nil, err + } + if c.URL != "" { + return u, nil + } + + // Parse c.HTTPAddr and use a default host if not specified. + addr, err := url.Parse("http://" + c.HTTPAddr) + if err != nil { + return nil, err + } + if addr.Host == "" { + addr.Host = "localhost" + } + return addr, nil +} + func newBasicAuthHandler(user, passwd string, h http.Handler) http.Handler { want := "Basic " + base64.StdEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", user, passwd))) return &basicAuthHandler{h, []byte(want)}