From 6c1f2af53ac0920d6e27f843444efd5e676417bf Mon Sep 17 00:00:00 2001 From: Simon Jefford Date: Fri, 12 Jun 2015 17:21:19 +0100 Subject: [PATCH 1/2] log: ensure the correct status is always logged in the case of error (>=400) then no response may have been sent --- middleware/log/log.go | 6 +++++ middleware/log/log_test.go | 48 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 middleware/log/log_test.go diff --git a/middleware/log/log.go b/middleware/log/log.go index f4329d21..7c206458 100644 --- a/middleware/log/log.go +++ b/middleware/log/log.go @@ -2,6 +2,7 @@ package log import ( + "fmt" "log" "net/http" @@ -19,6 +20,11 @@ func (l Logger) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { if middleware.Path(r.URL.Path).Matches(rule.PathScope) { responseRecorder := middleware.NewResponseRecorder(w) status, err := l.Next.ServeHTTP(responseRecorder, r) + if status >= 400 { + responseRecorder.WriteHeader(status) + fmt.Fprintf(responseRecorder, "%d %s", status, http.StatusText(status)) + status = 0 + } rep := middleware.NewReplacer(r, responseRecorder) rule.Log.Println(rep.Replace(rule.Format)) return status, err diff --git a/middleware/log/log_test.go b/middleware/log/log_test.go new file mode 100644 index 00000000..40560e4c --- /dev/null +++ b/middleware/log/log_test.go @@ -0,0 +1,48 @@ +package log + +import ( + "bytes" + "log" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +type erroringMiddleware struct{} + +func (erroringMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { + return http.StatusNotFound, nil +} + +func TestLoggedStatus(t *testing.T) { + var f bytes.Buffer + var next erroringMiddleware + rule := Rule{ + PathScope: "/", + Format: DefaultLogFormat, + Log: log.New(&f, "", 0), + } + + logger := Logger{ + Rules: []Rule{rule}, + Next: next, + } + + r, err := http.NewRequest("GET", "/", nil) + if err != nil { + t.Fatal(err) + } + + rec := httptest.NewRecorder() + + status, err := logger.ServeHTTP(rec, r) + if status != 0 { + t.Error("Expected status to be 0 - was", status) + } + + logged := f.String() + if !strings.Contains(logged, "404 13") { + t.Error("Expected 404 to be logged. Logged string -", logged) + } +} From c811d416a7024cf71c854e8903e6efd483a398af Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Mon, 15 Jun 2015 10:17:09 -0600 Subject: [PATCH 2/2] log: Customizable default error function --- config/setup/log.go | 3 ++- middleware/log/log.go | 16 ++++++++++++---- server/server.go | 8 ++++++-- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/config/setup/log.go b/config/setup/log.go index 4b5e11c6..694f3625 100644 --- a/config/setup/log.go +++ b/config/setup/log.go @@ -6,6 +6,7 @@ import ( "github.com/mholt/caddy/middleware" caddylog "github.com/mholt/caddy/middleware/log" + "github.com/mholt/caddy/server" ) // Log sets up the logging middleware. @@ -39,7 +40,7 @@ func Log(c *Controller) (middleware.Middleware, error) { }) return func(next middleware.Handler) middleware.Handler { - return caddylog.Logger{Next: next, Rules: rules} + return caddylog.Logger{Next: next, Rules: rules, ErrorFunc: server.DefaultErrorFunc} }, nil } diff --git a/middleware/log/log.go b/middleware/log/log.go index 7c206458..d6b147b8 100644 --- a/middleware/log/log.go +++ b/middleware/log/log.go @@ -11,8 +11,9 @@ import ( // Logger is a basic request logging middleware. type Logger struct { - Next middleware.Handler - Rules []Rule + Next middleware.Handler + Rules []Rule + ErrorFunc func(http.ResponseWriter, *http.Request, int) // failover error handler } func (l Logger) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { @@ -21,8 +22,15 @@ func (l Logger) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { responseRecorder := middleware.NewResponseRecorder(w) status, err := l.Next.ServeHTTP(responseRecorder, r) if status >= 400 { - responseRecorder.WriteHeader(status) - fmt.Fprintf(responseRecorder, "%d %s", status, http.StatusText(status)) + // There was an error up the chain, but no response has been written yet. + // The error must be handled here so the log entry will record the response size. + if l.ErrorFunc != nil { + l.ErrorFunc(responseRecorder, r, status) + } else { + // Default failover error handler + responseRecorder.WriteHeader(status) + fmt.Fprintf(responseRecorder, "%d %s", status, http.StatusText(status)) + } status = 0 } rep := middleware.NewReplacer(r, responseRecorder) diff --git a/server/server.go b/server/server.go index a97f68f4..4a751de6 100644 --- a/server/server.go +++ b/server/server.go @@ -221,11 +221,15 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Fallback error response in case error handling wasn't chained in if status >= 400 { - w.WriteHeader(status) - fmt.Fprintf(w, "%d %s", status, http.StatusText(status)) + DefaultErrorFunc(w, r, status) } } else { w.WriteHeader(http.StatusNotFound) fmt.Fprintf(w, "No such host at %s", s.address) } } + +func DefaultErrorFunc(w http.ResponseWriter, r *http.Request, status int) { + w.WriteHeader(status) + fmt.Fprintf(w, "%d %s", status, http.StatusText(status)) +}