Merge pull request #806 from weaveworks/804-f-star-cking-slashes

Less hacky fix for /%2F bug

app/api_report_test.go

@@ -12,9 +12,8 @@ import (
 )
 
 func topologyServer() *httptest.Server {
-	router := mux.NewRouter()
-	app.RegisterTopologyRoutes(StaticReport{}, router)
-	return httptest.NewServer(router)
+	handler := app.TopologyHandler(StaticReport{}, mux.NewRouter(), nil)
+	return httptest.NewServer(handler)
 }
 
 func TestAPIReport(t *testing.T) {

app/api_topologies.go

@@ -259,9 +259,9 @@ func (r *registry) captureRenderer(rep Reporter, f reportRenderHandler) http.Han
 	}
 }
 
-func (r *registry) captureRendererWithoutFilters(rep Reporter, f reportRenderHandler) http.HandlerFunc {
+func (r *registry) captureRendererWithoutFilters(rep Reporter, topologyID string, f reportRenderHandler) http.HandlerFunc {
 	return func(w http.ResponseWriter, req *http.Request) {
-		topology, ok := r.get(mux.Vars(req)["topology"])
+		topology, ok := r.get(topologyID)
 		if !ok {
 			http.NotFound(w, req)
 			return

app/api_topologies_test.go

@@ -53,9 +53,9 @@ func TestAPITopology(t *testing.T) {
 func TestAPITopologyAddsKubernetes(t *testing.T) {
 	router := mux.NewRouter()
 	c := app.NewCollector(1 * time.Minute)
-	app.RegisterTopologyRoutes(c, router)
 	app.RegisterReportPostHandler(c, router)
-	ts := httptest.NewServer(router)
+	handler := app.TopologyHandler(c, router, nil)
+	ts := httptest.NewServer(handler)
 	defer ts.Close()
 
 	body := getRawJSON(t, ts, "/api/topology")

app/api_topology.go

@@ -4,7 +4,6 @@ import (
 	"net/http"
 	"time"
 
-	"github.com/gorilla/mux"
 	"github.com/gorilla/websocket"
 
 	"github.com/weaveworks/scope/render"
@@ -50,18 +49,18 @@ func handleWs(rep Reporter, renderer render.Renderer, w http.ResponseWriter, r *
 }
 
 // Individual nodes.
-func handleNode(rep Reporter, renderer render.Renderer, w http.ResponseWriter, r *http.Request) {
-	var (
-		vars     = mux.Vars(r)
-		nodeID   = vars["id"]
-		rpt      = rep.Report()
-		node, ok = renderer.Render(rep.Report())[nodeID]
-	)
-	if !ok {
-		http.NotFound(w, r)
-		return
+func handleNode(nodeID string) func(Reporter, render.Renderer, http.ResponseWriter, *http.Request) {
+	return func(rep Reporter, renderer render.Renderer, w http.ResponseWriter, r *http.Request) {
+		var (
+			rpt      = rep.Report()
+			node, ok = renderer.Render(rep.Report())[nodeID]
+		)
+		if !ok {
+			http.NotFound(w, r)
+			return
+		}
+		respondWith(w, http.StatusOK, APINode{Node: render.MakeDetailedNode(rpt, node)})
 	}
-	respondWith(w, http.StatusOK, APINode{Node: render.MakeDetailedNode(rpt, node)})
 }
 
 var upgrader = websocket.Upgrader{

app/api_topology_test.go

@@ -87,6 +87,20 @@ func TestAPITopologyApplications(t *testing.T) {
 		equals(t, false, node.Node.Pseudo)
 		// Let's not unit-test the specific content of the detail tables
 	}
+
+	{
+		body := getRawJSON(t, ts, "/api/topology/applications-by-name/"+
+			url.QueryEscape(fixture.Client1Name))
+		var node app.APINode
+		if err := json.Unmarshal(body, &node); err != nil {
+			t.Fatal(err)
+		}
+		equals(t, fixture.Client1Name, node.Node.ID)
+		equals(t, fixture.Client1Name, node.Node.LabelMajor)
+		equals(t, "2 processes", node.Node.LabelMinor)
+		equals(t, false, node.Node.Pseudo)
+		// Let's not unit-test the specific content of the detail tables
+	}
 }
 
 func TestAPITopologyHosts(t *testing.T) {

app/router.go

@@ -29,48 +29,86 @@ var (
 // mistakenly unescapes the Path before parsing it.  This breaks %2F (encoded
 // forward slashes) in the paths.
 func URLMatcher(pattern string) mux.MatcherFunc {
-	matchParts := strings.Split(pattern, "/")
-
 	return func(r *http.Request, rm *mux.RouteMatch) bool {
-		path := strings.SplitN(r.RequestURI, "?", 2)[0]
-		parts := strings.Split(path, "/")
-		if len(parts) != len(matchParts) {
-			return false
+		vars, match := matchURL(r, pattern)
+		if match {
+			rm.Vars = vars
 		}
+		return match
+	}
+}
 
-		rm.Vars = map[string]string{}
-		for i, part := range parts {
-			unescaped, err := url.QueryUnescape(part)
-			if err != nil {
-				return false
-			}
-			match := matchParts[i]
-			if strings.HasPrefix(match, "{") && strings.HasSuffix(match, "}") {
-				rm.Vars[strings.Trim(match, "{}")] = unescaped
-			} else if matchParts[i] != unescaped {
-				return false
-			}
+func matchURL(r *http.Request, pattern string) (map[string]string, bool) {
+	matchParts := strings.Split(pattern, "/")
+	path := strings.SplitN(r.RequestURI, "?", 2)[0]
+	parts := strings.Split(path, "/")
+	if len(parts) != len(matchParts) {
+		return nil, false
+	}
+
+	vars := map[string]string{}
+	for i, part := range parts {
+		unescaped, err := url.QueryUnescape(part)
+		if err != nil {
+			return nil, false
+		}
+		match := matchParts[i]
+		if strings.HasPrefix(match, "{") && strings.HasSuffix(match, "}") {
+			vars[strings.Trim(match, "{}")] = unescaped
+		} else if matchParts[i] != unescaped {
+			return nil, false
 		}
-		return true
 	}
+	return vars, true
 }
 
 func gzipHandler(h http.HandlerFunc) http.HandlerFunc {
 	return handlers.GZIPHandlerFunc(h, nil)
 }
 
-// RegisterTopologyRoutes registers the various topology routes with a http mux.
-func RegisterTopologyRoutes(c Reporter, router *mux.Router) {
-	get := router.Methods("GET").Subrouter()
+// TopologyHandler registers the various topology routes with a http mux.
+//
+// The returned http.Handler has to be passed directly to http.ListenAndServe,
+// and cannot be nested inside another gorrilla.mux.
+//
+// Routes which should be matched before the topology routes should be added
+// to a router and passed in preRoutes.  Routes to be matches after topology
+// routes should be added to a router and passed to postRoutes.
+func TopologyHandler(c Reporter, preRoutes *mux.Router, postRoutes http.Handler) http.Handler {
+	get := preRoutes.Methods("GET").Subrouter()
 	get.HandleFunc("/api", gzipHandler(apiHandler))
 	get.HandleFunc("/api/topology", gzipHandler(topologyRegistry.makeTopologyList(c)))
 	get.HandleFunc("/api/topology/{topology}",
 		gzipHandler(topologyRegistry.captureRenderer(c, handleTopology)))
 	get.HandleFunc("/api/topology/{topology}/ws",
 		topologyRegistry.captureRenderer(c, handleWs)) // NB not gzip!
-	get.MatcherFunc(URLMatcher("/api/topology/{topology}/{id}")).HandlerFunc(
-		gzipHandler(topologyRegistry.captureRendererWithoutFilters(c, handleNode)))
 	get.HandleFunc("/api/report", gzipHandler(makeRawReportHandler(c)))
+
+	if postRoutes != nil {
+		preRoutes.PathPrefix("/").Handler(postRoutes)
+	}
+
+	// We have to handle the node details path manually due to
+	// various bugs in gorilla.mux and Go URL parsing.  See #804.
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		vars, match := matchURL(r, "/api/topology/{topology}/{id}")
+		if !match {
+			preRoutes.ServeHTTP(w, r)
+			return
+		}
+
+		topologyID := vars["topology"]
+		nodeID := vars["id"]
+		if nodeID == "ws" {
+			preRoutes.ServeHTTP(w, r)
+			return
+		}
+
+		handler := gzipHandler(topologyRegistry.captureRendererWithoutFilters(
+			c, topologyID, handleNode(nodeID),
+		))
+		handler.ServeHTTP(w, r)
+	})
 }
 
 // RegisterReportPostHandler registers the handler for report submission

prog/app.go

@@ -18,14 +18,12 @@ import (
 )
 
 // Router creates the mux for all the various app components.
-func router(c app.Collector) *mux.Router {
+func router(c app.Collector) http.Handler {
 	router := mux.NewRouter()
-	app.RegisterTopologyRoutes(c, router)
 	app.RegisterReportPostHandler(c, router)
 	app.RegisterControlRoutes(router)
 	app.RegisterPipeRoutes(router)
-	router.Methods("GET").PathPrefix("/").Handler(http.FileServer(FS(false)))
-	return router
+	return app.TopologyHandler(c, router, http.FileServer(FS(false)))
 }
 
 // Main runs the app
@@ -48,10 +46,10 @@ func appMain() {
 	app.UniqueID = strconv.FormatInt(rand.Int63(), 16)
 	app.Version = version
 	log.Printf("app starting, version %s, ID %s", app.Version, app.UniqueID)
-	http.Handle("/", router(app.NewCollector(*window)))
+	handler := router(app.NewCollector(*window))
 	go func() {
 		log.Printf("listening on %s", *listen)
-		log.Print(http.ListenAndServe(*listen, nil))
+		log.Print(http.ListenAndServe(*listen, handler))
 	}()
 
 	common.SignalHandlerLoop()

render/expected/expected.go

@@ -102,7 +102,7 @@ var (
 		},
 		ServerProcessID: {
 			ID:         ServerProcessID,
-			LabelMajor: "apache",
+			LabelMajor: fixture.ServerName,
 			LabelMinor: fmt.Sprintf("%s (%s)", fixture.ServerHostID, fixture.ServerPID),
 			Rank:       fixture.ServerName,
 			Pseudo:     false,
@@ -137,11 +137,11 @@ var (
 	}).Prune()
 
 	RenderedProcessNames = (render.RenderableNodes{
-		"curl": {
-			ID:         "curl",
-			LabelMajor: "curl",
+		fixture.Client1Name: {
+			ID:         fixture.Client1Name,
+			LabelMajor: fixture.Client1Name,
 			LabelMinor: "2 processes",
-			Rank:       "curl",
+			Rank:       fixture.Client1Name,
 			Pseudo:     false,
 			Origins: report.MakeIDList(
 				fixture.Client54001NodeID,
@@ -150,17 +150,17 @@ var (
 				fixture.ClientProcess2NodeID,
 				fixture.ClientHostNodeID,
 			),
-			Node: report.MakeNode().WithAdjacent("apache"),
+			Node: report.MakeNode().WithAdjacent(fixture.ServerName),
 			EdgeMetadata: report.EdgeMetadata{
 				EgressPacketCount: newu64(30),
 				EgressByteCount:   newu64(300),
 			},
 		},
-		"apache": {
-			ID:         "apache",
-			LabelMajor: "apache",
+		fixture.ServerName: {
+			ID:         fixture.ServerName,
+			LabelMajor: fixture.ServerName,
 			LabelMinor: "1 process",
-			Rank:       "apache",
+			Rank:       fixture.ServerName,
 			Pseudo:     false,
 			Origins: report.MakeIDList(
 				fixture.Server80NodeID,
@@ -187,9 +187,9 @@ var (
 			Node:         report.MakeNode().WithAdjacent(render.TheInternetID),
 			EdgeMetadata: report.EdgeMetadata{},
 		},
-		unknownPseudoNode1ID: unknownPseudoNode1("apache"),
-		unknownPseudoNode2ID: unknownPseudoNode2("apache"),
-		render.TheInternetID: theInternetNode("apache"),
+		unknownPseudoNode1ID: unknownPseudoNode1(fixture.ServerName),
+		unknownPseudoNode2ID: unknownPseudoNode2(fixture.ServerName),
+		render.TheInternetID: theInternetNode(fixture.ServerName),
 	}).Prune()
 
 	RenderedContainers = (render.RenderableNodes{

test/fixture/report_fixture.go

@@ -47,8 +47,8 @@ var (
 	ServerPID       = "215"
 	NonContainerPID = "1234"
 
-	Client1Name      = "curl"
-	Client2Name      = "curl"
+	Client1Name      = "/usr/bin/curl"
+	Client2Name      = "/usr/bin/curl"
 	ServerName       = "apache"
 	NonContainerName = "bash"