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.

Sign up for GitHub

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

Jump to bottom

Fix stats hijack #1598

Merged
merged 3 commits into from
May 15, 2017
Merged

Fix stats hijack #1598

merged 3 commits into from
May 15, 2017

Conversation

emilevauge
Copy link
Member

@emilevauge emilevauge commented May 13, 2017 •
edited by ldez
Loading

Description

This PR fixes #1315 and adds a generic http recover.

Fixes #1315, #1591

@emilevauge emilevauge added this to the 1.3 milestone May 13, 2017
@emilevauge emilevauge changed the base branch from master to v1.3 May 13, 2017 17:05
@emilevauge emilevauge force-pushed the fix-stats-hijack branch 2 times, most recently from 2d82c69 to d7003fd Compare May 13, 2017 17:46
m3co-code
m3co-code reviewed May 13, 2017
View reviewed changes
Copy link
Contributor

@m3co-code m3co-code left a comment

Choose a reason for hiding this comment

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

Hi @emilevauge,

I was so free to have a look at the code and left few comments. Please let me know when this is not appreciated!

middlewares/recover.go Outdated
// RecoverHandler recovers from a panic in http handlers
func RecoverHandler(next http.Handler) http.Handler {
fn := func(w http.ResponseWriter, r *http.Request) {
defer func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could extract this anonymous fn and reuse it in the NegroniRecoverHandler.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

middlewares/stateful.go Outdated

import "net/http"

// Stateful interface groups all http interface that must be
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo HTTP interfaces ...

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

server/server.go Outdated
@@ -525,6 +525,12 @@ func (server *Server) prepareServer(entryPointName string, router *middlewares.H
return nil, err
}

middlewaresStr := ""
for _, handler := range negroni.Handlers() {
middlewaresStr += reflect.TypeOf(handler).String() + " "
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this here by intend or a left over?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this intentionally ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok :) To be honest I am not sure how useful I find this. With a simple configuration I currently get the following output:

Middlewares: negroni.HandlerFunc *middlewares.Logger *stats.Stats negroni.HandlerFunc

Additionally I have for example retries configured. It is not mentioned here, as this middleware is setup in the loadConfig() method and specific to a backend. Therefore we should maybe make this message more explicit or extend the logging also on other positions. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, you are right, as we don't have middlewares created in loadConfig, it is a bit useless. I will remove it.

@regner
Copy link
Contributor

regner commented May 14, 2017

Any chance of this helping with #1591?

@emilevauge
Copy link
Member Author

@regner Indeed, it will fix the error Unable to hijack the connection: *middlewares.responseRecorder :)

ldez
ldez suggested changes May 14, 2017
View reviewed changes
middlewares/recover.go Outdated
import (
"github.com/codegangsta/negroni"
"github.com/containous/traefik/log"
"net/http"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please group the non-stdlib imports below the stdlib ones 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Ouch, I finally configured Gogland correctly on imports ^^

middlewares/recover_test.go Outdated
"github.com/codegangsta/negroni"
"net/http"
"net/http/httptest"
"testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please group the non-stdlib imports below the stdlib ones.

@regner
Copy link
Contributor

regner commented May 14, 2017

Thanks @emilevauge. I wasn't entirely sure but if thats the case I eagerly look forward to this being merged.

timoreimann
timoreimann reviewed May 15, 2017
View reviewed changes
middlewares/recover_test.go Outdated
t.Fatal(err)
}
if resp.StatusCode != 500 {
t.Fatalf("Received non-%d response: %d\n", 500, resp.StatusCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't really need to replace the 500 error code with a variable, do we?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do ^^

middlewares/recover_test.go Outdated
}
}

func TestNegroniRecoverHandler(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the two tests could be fairly easily merged into a single table-driven test by letting the handler be created through a func() http.Handler or so?

Copy link
Member Author

Choose a reason for hiding this comment

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

@timoreimann to be honest, as we are in the process of removing negroni dep, I don't think that's what we want. TestNegroniRecoverHandler will be removed soon ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah good point. 👍

timoreimann
timoreimann approved these changes May 15, 2017
View reviewed changes
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

LGTM. 🎉

@emilevauge
Fix stats responseRecorder Hijacker
71cec15 
Signed-off-by: Emile Vauge <emile@vauge.com>
@emilevauge
Adds Panic Recover middleware
111251d 
Signed-off-by: Emile Vauge <emile@vauge.com>
@emilevauge
Add Recover tests
48a91d0
@emilevauge emilevauge merged commit e3ab4e4 into v1.3 May 15, 2017
@ldez ldez deleted the fix-stats-hijack branch May 15, 2017 23:10
@ldez ldez mentioned this pull request May 15, 2017
Closed
@ldez ldez mentioned this pull request May 15, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m3co-code m3co-code m3co-code left review comments

@ldez ldez ldez approved these changes

@timoreimann timoreimann timoreimann approved these changes

Assignees
No one assigned
Labels
area/middleware area/websocket kind/bug/fix a bug fix
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.
5 participants
@emilevauge @regner @timoreimann @m3co-code @ldez