Fixes #119. Update http handlers for api and zipper to use gorilla/mux

[unjuli]

This provides functionality to add implicit redirect for endpoints with
optional forward slash and avoid adding redundant endpoints

Update:

• Add middleware to trim trailing slash from the request path (& unit tests)
• Update app tests to include testing for routes
• Refactor app tests to run subtests in parallel
• Refactor app tests to use httptest package for creating test requests

[azhiltsov]

Since this is changing the http library we are using for a while I would like to understand a performance implications of this change.

[unjuli]

@azhiltsov @grzkv Any suggestions which tests to perform around this?

[unjuli]

As per this benchmark (https://github.com/julienschmidt/go-http-routing-benchmark), we can try using https://github.com/julienschmidt/httprouter which seems to have the best performance vs memory consumption. Though, it doesn't seem to be maintained that frequently.

[jwkohnen]

The introduction of gorilla mux changes the behaviour. Before a request to e.g. /block-headers and /block-headers/ behaved indistinguishable. With this change a request to /block-headers is redirected with HTTP code 301 to /block-headers/. That's an extra round trip for the client and has some side effects:

The re-direct is a HTTP 301 (Moved Permanently). Note that when this is set for routes with a non-idempotent method (e.g. POST, PUT), the subsequent re-directed request will be made as a GET by most clients. Use middleware or client settings to modify this behaviour as needed.

and

Special case: when a route sets a path prefix using the PathPrefix() method, strict slash is ignored for that route because the redirect behavior can't be determined from a prefix alone. However, any subrouters created from that route inherit the original StrictSlash setting.

I suppose all incoming requests are already idempotent and all clients out there do automatic redirect following and the mentioned special case doesn't apply. But is that confirmed and the changes behaviour desired?

[grzkv]

@azhiltsov Performance is not a concern here. Here's a benchmark (can't say it's very authoritative though). Even in the very worst case, we are looking at 1ms delay max. My guess is that delay will be in microseconds.

[grzkv]

Regarding the muxer to use: I'd go with gorilla/mux. It has the most established reputation.

[azhiltsov]

301 HTTP redirects couldn't be handled by all kind of used external clients. I recall grafana was unable to handle this properly if handled from the client side (js in a browser)
If instead of doing internal redirect we would issue an external one than we most probably would cut-off some consumers. What make it worse - we would not even notice this.

[grzkv]

I think, 301s is exactly what we are trying to fix with this PRs.

[grzkv]

I would be nice to have routing tests with this PR.

[grzkv]

• Please add routing tests
• 301s should not be returned, I believe this could be achieved with using regex in routes like this Optionally Match trailing slash in the URL and HTTP Method based matching gorilla/mux#30 (comment)

[Civil]

@grzkv main problem with 301s is what jwkohnen mentioned.

Grafana uses POST requests to send it's requests (as this allows arbitrary large requests).

If webserver replies with 301, the behavior of most of the HTTP Clients (including browsers) is to change the request to GET and drop the body[1].

So your request to /render with body target=foo will suddenly become just a /render with no parameters.

The correct behavior would be to return 308 instead[1], but then you might still encounter some HTTP Clients that won't understand 308. That's the reason why gorilla/mux doesn't do it by default[3] and that was original reason for having same handler function defined for both URL with and without / in the end.

[1] https://golang.org/pkg/net/http/#Client.Do - paragraph that starts with "If the server replies with a redirect"
[2] https://tools.ietf.org/html/rfc7538
[3] gorilla/mux#285

[azhiltsov]

I think, 301s is exactly what we are trying to fix with this PRs.

$ curl -v 127.0.0.1:8091/render/?format=json

GET /render/?format=json HTTP/1.1
< HTTP/1.1 200 OK

$ curl -v 127.0.0.1:8091/render?format=json

GET /render?format=json HTTP/1.1
< HTTP/1.1 200 OK

[grzkv]

@Civil Thanks for the additional info. Bottomline: 301 should be avoided. So, everything is OK. We are trying to avoid them. We should return 200, this is what we want to do.

@azhiltsov Yes, everything is correct. But this is only achieved by having duplicate routing paths, see

carbonapi/app/carbonapi/routes.go

Line 38 in 2934a79

r.HandleFunc("/render/", httputil.TimeHandler(app.validateRequest(http.HandlerFunc(app.renderHandler), "render"), app.bucketRequestTimes))

This can be achieved by a single route with mux.

[grzkv]

Hey all! Let's move this PR to some final conclustion.

[grzkv]

@avereha Can you please give some feedback?

[avereha]

Graphite-web seem to be doing the same trick with "/?", for example:

https://github.com/graphite-project/graphite-web/blob/master/webapp/graphite/render/urls.py

I would look there for other examples/URLs and make sure we don't diverge too much.

Another approach that I noticed was to add middleware that removes tailing shashes. Not sure if this will add too many cases that would be different from graphite-web. Or we could make small muxer that wraps original muxer and supports registering two paths.
I think all approaches would work.

No matter what way we choose, I would still add tests for the next time we want to replace the muxer.

[grzkv]

@azhiltsov There should be not 301 redirect responses anymore. Could you please re-evaluate your review?

[avereha]

The integration test is failing because go-carbon got moved to go-graphite. Will try to fix this is a separate PR because it is not related to this change.

Tested the latest version with production traffic and found there was no 3xx answer, which means we are handling tailing / OK.
Issues that I found:

• curl -L -v localhost:8091/ goes in infinite redirect loop: curl: (47) Maximum (50) redirects followed
• curl -L -v localhost:8091/qwfqewf answers with 404(better than before where we answered with 200). But the usage body is missing.

[grzkv]

@avereha @anjali-sharma Is this a WIP or ready for review?

[unjuli]

@grzkv It's a WIP. I'm looking into the issues reported by @avereha.

[unjuli]

@avereha Added fix for the edge case (thanks for reporting!) and test for it. Also refactored the tests a bit to handle test App instance setup for the package.

[avereha]

Tested again and everything works as expected.
Again, there was no 3xx answer.

[avereha]

Thank you @anjali-sharma !