From eeb23a24693568c5889bfafad6cee28064e47671 Mon Sep 17 00:00:00 2001 From: Tw Date: Sat, 21 Jan 2017 09:51:31 +0800 Subject: [PATCH 01/15] redirect: determine the FromScheme at runtime (#1297) Signed-off-by: Tw --- caddyhttp/redirect/redirect.go | 11 ++++++----- caddyhttp/redirect/redirect_test.go | 18 +++++++++--------- caddyhttp/redirect/setup.go | 9 +++++---- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/caddyhttp/redirect/redirect.go b/caddyhttp/redirect/redirect.go index a489e735..711313a8 100644 --- a/caddyhttp/redirect/redirect.go +++ b/caddyhttp/redirect/redirect.go @@ -34,15 +34,16 @@ func (rd Redirect) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error } func schemeMatches(rule Rule, req *http.Request) bool { - return (rule.FromScheme == "https" && req.TLS != nil) || - (rule.FromScheme != "https" && req.TLS == nil) + return (rule.FromScheme() == "https" && req.TLS != nil) || + (rule.FromScheme() != "https" && req.TLS == nil) } // Rule describes an HTTP redirect rule. type Rule struct { - FromScheme, FromPath, To string - Code int - Meta bool + FromScheme func() string + FromPath, To string + Code int + Meta bool httpserver.RequestMatcher } diff --git a/caddyhttp/redirect/redirect_test.go b/caddyhttp/redirect/redirect_test.go index 27998abe..bc817580 100644 --- a/caddyhttp/redirect/redirect_test.go +++ b/caddyhttp/redirect/redirect_test.go @@ -47,16 +47,16 @@ func TestRedirect(t *testing.T) { return 0, nil }), Rules: []Rule{ - {FromPath: "/from", To: "/to", Code: http.StatusMovedPermanently, RequestMatcher: httpserver.IfMatcher{}}, - {FromPath: "/a", To: "/b", Code: http.StatusTemporaryRedirect, RequestMatcher: httpserver.IfMatcher{}}, + {FromScheme: func() string { return "http" }, FromPath: "/from", To: "/to", Code: http.StatusMovedPermanently, RequestMatcher: httpserver.IfMatcher{}}, + {FromScheme: func() string { return "http" }, FromPath: "/a", To: "/b", Code: http.StatusTemporaryRedirect, RequestMatcher: httpserver.IfMatcher{}}, // These http and https schemes would never actually be mixed in the same // redirect rule with Caddy because http and https schemes have different listeners, // so they don't share a redirect rule. So although these tests prove something // impossible with Caddy, it's extra bulletproofing at very little cost. - {FromScheme: "http", FromPath: "/scheme", To: "https://localhost/scheme", Code: http.StatusMovedPermanently, RequestMatcher: httpserver.IfMatcher{}}, - {FromScheme: "https", FromPath: "/scheme2", To: "http://localhost/scheme2", Code: http.StatusMovedPermanently, RequestMatcher: httpserver.IfMatcher{}}, - {FromScheme: "", FromPath: "/scheme3", To: "https://localhost/scheme3", Code: http.StatusMovedPermanently, RequestMatcher: httpserver.IfMatcher{}}, + {FromScheme: func() string { return "http" }, FromPath: "/scheme", To: "https://localhost/scheme", Code: http.StatusMovedPermanently, RequestMatcher: httpserver.IfMatcher{}}, + {FromScheme: func() string { return "https" }, FromPath: "/scheme2", To: "http://localhost/scheme2", Code: http.StatusMovedPermanently, RequestMatcher: httpserver.IfMatcher{}}, + {FromScheme: func() string { return "" }, FromPath: "/scheme3", To: "https://localhost/scheme3", Code: http.StatusMovedPermanently, RequestMatcher: httpserver.IfMatcher{}}, }, } @@ -90,7 +90,7 @@ func TestRedirect(t *testing.T) { func TestParametersRedirect(t *testing.T) { re := Redirect{ Rules: []Rule{ - {FromPath: "/", Meta: false, To: "http://example.com{uri}", RequestMatcher: httpserver.IfMatcher{}}, + {FromScheme: func() string { return "http" }, FromPath: "/", Meta: false, To: "http://example.com{uri}", RequestMatcher: httpserver.IfMatcher{}}, }, } @@ -108,7 +108,7 @@ func TestParametersRedirect(t *testing.T) { re = Redirect{ Rules: []Rule{ - {FromPath: "/", Meta: false, To: "http://example.com/a{path}?b=c&{query}", RequestMatcher: httpserver.IfMatcher{}}, + {FromScheme: func() string { return "http" }, FromPath: "/", Meta: false, To: "http://example.com/a{path}?b=c&{query}", RequestMatcher: httpserver.IfMatcher{}}, }, } @@ -127,8 +127,8 @@ func TestParametersRedirect(t *testing.T) { func TestMetaRedirect(t *testing.T) { re := Redirect{ Rules: []Rule{ - {FromPath: "/whatever", Meta: true, To: "/something", RequestMatcher: httpserver.IfMatcher{}}, - {FromPath: "/", Meta: true, To: "https://example.com/", RequestMatcher: httpserver.IfMatcher{}}, + {FromScheme: func() string { return "http" }, FromPath: "/whatever", Meta: true, To: "/something", RequestMatcher: httpserver.IfMatcher{}}, + {FromScheme: func() string { return "http" }, FromPath: "/", Meta: true, To: "https://example.com/", RequestMatcher: httpserver.IfMatcher{}}, }, } diff --git a/caddyhttp/redirect/setup.go b/caddyhttp/redirect/setup.go index da797ab0..aa997431 100644 --- a/caddyhttp/redirect/setup.go +++ b/caddyhttp/redirect/setup.go @@ -34,10 +34,11 @@ func redirParse(c *caddy.Controller) ([]Rule, error) { cfg := httpserver.GetConfig(c) initRule := func(rule *Rule, defaultCode string, args []string) error { - if cfg.TLS.Enabled { - rule.FromScheme = "https" - } else { - rule.FromScheme = "http" + rule.FromScheme = func() string { + if cfg.TLS.Enabled { + return "https" + } + return "http" } var ( From 04da9c7374ea07dc19f9a42cd2696ed5c45cfffb Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Tue, 24 Jan 2017 19:16:54 -0700 Subject: [PATCH 02/15] Create only one log roller per file across whole process (fixes #1363) --- caddyhttp/httpserver/roller.go | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/caddyhttp/httpserver/roller.go b/caddyhttp/httpserver/roller.go index b8264636..0660de65 100644 --- a/caddyhttp/httpserver/roller.go +++ b/caddyhttp/httpserver/roller.go @@ -2,6 +2,7 @@ package httpserver import ( "io" + "path/filepath" "strconv" "github.com/mholt/caddy" @@ -19,14 +20,30 @@ type LogRoller struct { } // GetLogWriter returns an io.Writer that writes to a rolling logger. +// This should be called only from the main goroutine (like during +// server setup) because this method is not thread-safe; it is careful +// to create only one log writer per log file, even if the log file +// is shared by different sites or middlewares. This ensures that +// rolling is synchronized, since a process (or multiple processes) +// should not create more than one roller on the same file at the +// same time. See issue #1363. func (l LogRoller) GetLogWriter() io.Writer { - return &lumberjack.Logger{ - Filename: l.Filename, - MaxSize: l.MaxSize, - MaxAge: l.MaxAge, - MaxBackups: l.MaxBackups, - LocalTime: l.LocalTime, + absPath, err := filepath.Abs(l.Filename) + if err != nil { + absPath = l.Filename // oh well, hopefully they're consistent in how they specify the filename } + lj, has := lumberjacks[absPath] + if !has { + lj = &lumberjack.Logger{ + Filename: l.Filename, + MaxSize: l.MaxSize, + MaxAge: l.MaxAge, + MaxBackups: l.MaxBackups, + LocalTime: l.LocalTime, + } + lumberjacks[absPath] = lj + } + return lj } // ParseRoller parses roller contents out of c. @@ -62,3 +79,7 @@ func ParseRoller(c *caddy.Controller) (*LogRoller, error) { LocalTime: true, }, nil } + +// lumberjacks maps log filenames to the logger +// that is being used to keep them rolled/maintained. +var lumberjacks = make(map[string]*lumberjack.Logger) From 65cb966d38a9863b596aedd6b140494d3fcf6a45 Mon Sep 17 00:00:00 2001 From: Tw Date: Thu, 26 Jan 2017 14:54:25 +0800 Subject: [PATCH 03/15] httpserver: support QUIC reload fix issue #958 Signed-off-by: Tw --- caddyhttp/httpserver/server.go | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/caddyhttp/httpserver/server.go b/caddyhttp/httpserver/server.go index b75e46ca..f7e6efa2 100644 --- a/caddyhttp/httpserver/server.go +++ b/caddyhttp/httpserver/server.go @@ -145,8 +145,17 @@ func (s *Server) Listen() (net.Listener, error) { return ln.(*net.TCPListener), nil } -// ListenPacket is a noop to implement the Server interface. -func (s *Server) ListenPacket() (net.PacketConn, error) { return nil, nil } +// ListenPacket creates udp connection for QUIC if it is enabled, +func (s *Server) ListenPacket() (net.PacketConn, error) { + if QUIC { + udpAddr, err := net.ResolveUDPAddr("udp", s.Server.Addr) + if err != nil { + return nil, err + } + return net.ListenUDP("udp", udpAddr) + } + return nil, nil +} // Serve serves requests on ln. It blocks until ln is closed. func (s *Server) Serve(ln net.Listener) error { @@ -172,15 +181,6 @@ func (s *Server) Serve(ln net.Listener) error { s.tlsGovChan = caddytls.RotateSessionTicketKeys(s.Server.TLSConfig) } - if QUIC { - go func() { - err := s.quicServer.ListenAndServe() - if err != nil { - log.Printf("[ERROR] listening for QUIC connections: %v", err) - } - }() - } - err := s.Server.Serve(ln) if QUIC { s.quicServer.Close() @@ -188,8 +188,14 @@ func (s *Server) Serve(ln net.Listener) error { return err } -// ServePacket is a noop to implement the Server interface. -func (s *Server) ServePacket(pc net.PacketConn) error { return nil } +// ServePacket serves QUIC requests on pc until it is closed. +func (s *Server) ServePacket(pc net.PacketConn) error { + if QUIC { + err := s.quicServer.Serve(pc.(*net.UDPConn)) + return fmt.Errorf("serving QUIC connections: %v", err) + } + return nil +} // ServeHTTP is the entry point of all HTTP requests. func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { From 9e9298ee5d6a89143a54126b24f984c5cff1db86 Mon Sep 17 00:00:00 2001 From: Toby Allen Date: Sat, 4 Feb 2017 22:29:29 +0000 Subject: [PATCH 04/15] Added additional - to common log file format (#1399) --- caddyhttp/log/log.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/caddyhttp/log/log.go b/caddyhttp/log/log.go index b6ff6758..4470bccf 100644 --- a/caddyhttp/log/log.go +++ b/caddyhttp/log/log.go @@ -86,7 +86,7 @@ const ( // DefaultLogFilename is the default log filename. DefaultLogFilename = "access.log" // CommonLogFormat is the common log format. - CommonLogFormat = `{remote} ` + CommonLogEmptyValue + ` [{when}] "{method} {uri} {proto}" {status} {size}` + CommonLogFormat = `{remote} ` + CommonLogEmptyValue + " " + CommonLogEmptyValue + ` [{when}] "{method} {uri} {proto}" {status} {size}` // CommonLogEmptyValue is the common empty log value. CommonLogEmptyValue = "-" // CombinedLogFormat is the combined log format. From 74195732667f338f7a93ed737d6f0562a118ded4 Mon Sep 17 00:00:00 2001 From: James Raspass Date: Tue, 7 Feb 2017 23:55:36 +0000 Subject: [PATCH 05/15] Replace magic number 308 with http.StatusPermanentRedirect --- caddyhttp/redirect/setup.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/caddyhttp/redirect/setup.go b/caddyhttp/redirect/setup.go index da797ab0..cfdab278 100644 --- a/caddyhttp/redirect/setup.go +++ b/caddyhttp/redirect/setup.go @@ -165,5 +165,5 @@ var httpRedirs = map[string]int{ "304": http.StatusNotModified, "305": http.StatusUseProxy, "307": http.StatusTemporaryRedirect, - "308": 308, // Permanent Redirect (RFC 7238) + "308": http.StatusPermanentRedirect, // Permanent Redirect (RFC 7238) } From f32eed1912c3a7a2f60dd5d489123ae3491586b3 Mon Sep 17 00:00:00 2001 From: Mateusz Gajewski Date: Wed, 8 Feb 2017 16:02:09 +0100 Subject: [PATCH 06/15] Feature #1246 - Remote syslog (#1301) * Remote syslog * golint * Initialize mutex --- caddyhttp/errors/errors.go | 18 +-- caddyhttp/errors/errors_test.go | 9 +- caddyhttp/errors/setup.go | 69 ++------ caddyhttp/errors/setup_test.go | 86 +++++----- caddyhttp/httpserver/logger.go | 148 +++++++++++++++++ caddyhttp/httpserver/logger_test.go | 212 +++++++++++++++++++++++++ caddyhttp/internalsrv/internal_test.go | 3 +- caddyhttp/log/log.go | 13 +- caddyhttp/log/log_test.go | 14 +- caddyhttp/log/setup.go | 92 +++-------- caddyhttp/log/setup_test.go | 117 ++++++-------- caddyhttp/markdown/process_test.go | 3 +- 12 files changed, 496 insertions(+), 288 deletions(-) create mode 100644 caddyhttp/httpserver/logger.go create mode 100644 caddyhttp/httpserver/logger_test.go diff --git a/caddyhttp/errors/errors.go b/caddyhttp/errors/errors.go index e6060295..a3412ce5 100644 --- a/caddyhttp/errors/errors.go +++ b/caddyhttp/errors/errors.go @@ -4,12 +4,10 @@ package errors import ( "fmt" "io" - "log" "net/http" "os" "runtime" "strings" - "sync" "time" "github.com/mholt/caddy" @@ -28,12 +26,8 @@ type ErrorHandler struct { Next httpserver.Handler GenericErrorPage string // default error page filename ErrorPages map[int]string // map of status code to filename - LogFile string - Log *log.Logger - LogRoller *httpserver.LogRoller - Debug bool // if true, errors are written out to client rather than to a log - file *os.File // a log file to close when done - fileMu *sync.RWMutex // like with log middleware, os.File can't "safely" be closed in a different goroutine + Log *httpserver.Logger + Debug bool // if true, errors are written out to client rather than to a log } func (h ErrorHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { @@ -50,9 +44,7 @@ func (h ErrorHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, er fmt.Fprintln(w, errMsg) return 0, err // returning 0 signals that a response has been written } - h.fileMu.RLock() h.Log.Println(errMsg) - h.fileMu.RUnlock() } if status >= 400 { @@ -73,10 +65,8 @@ func (h ErrorHandler) errorPage(w http.ResponseWriter, r *http.Request, code int errorPage, err := os.Open(pagePath) if err != nil { // An additional error handling an error... - h.fileMu.RLock() h.Log.Printf("%s [NOTICE %d %s] could not load error page: %v", time.Now().Format(timeFormat), code, r.URL.String(), err) - h.fileMu.RUnlock() httpserver.DefaultErrorFunc(w, r, code) return } @@ -89,10 +79,8 @@ func (h ErrorHandler) errorPage(w http.ResponseWriter, r *http.Request, code int if err != nil { // Epic fail... sigh. - h.fileMu.RLock() h.Log.Printf("%s [NOTICE %d %s] could not respond with %s: %v", time.Now().Format(timeFormat), code, r.URL.String(), pagePath, err) - h.fileMu.RUnlock() httpserver.DefaultErrorFunc(w, r, code) } @@ -154,9 +142,7 @@ func (h ErrorHandler) recovery(w http.ResponseWriter, r *http.Request) { httpserver.WriteTextResponse(w, http.StatusInternalServerError, fmt.Sprintf("%s\n\n%s", panicMsg, stack)) } else { // Currently we don't use the function name, since file:line is more conventional - h.fileMu.RLock() h.Log.Printf(panicMsg) - h.fileMu.RUnlock() h.errorPage(w, r, http.StatusInternalServerError) } } diff --git a/caddyhttp/errors/errors_test.go b/caddyhttp/errors/errors_test.go index 2b160392..4833ecb9 100644 --- a/caddyhttp/errors/errors_test.go +++ b/caddyhttp/errors/errors_test.go @@ -4,14 +4,12 @@ import ( "bytes" "errors" "fmt" - "log" "net/http" "net/http/httptest" "os" "path/filepath" "strconv" "strings" - "sync" "testing" "github.com/mholt/caddy/caddyhttp/httpserver" @@ -33,8 +31,7 @@ func TestErrors(t *testing.T) { http.StatusNotFound: path, http.StatusForbidden: "not_exist_file", }, - Log: log.New(&buf, "", 0), - fileMu: new(sync.RWMutex), + Log: httpserver.NewTestLogger(&buf), } _, notExistErr := os.Open("not_exist_file") @@ -123,7 +120,6 @@ func TestVisibleErrorWithPanic(t *testing.T) { Next: httpserver.HandlerFunc(func(w http.ResponseWriter, r *http.Request) (int, error) { panic(panicMsg) }), - fileMu: new(sync.RWMutex), } req, err := http.NewRequest("GET", "/", nil) @@ -179,8 +175,7 @@ func TestGenericErrorPage(t *testing.T) { ErrorPages: map[int]string{ http.StatusNotFound: notFoundErrorPagePath, }, - Log: log.New(&buf, "", 0), - fileMu: new(sync.RWMutex), + Log: httpserver.NewTestLogger(&buf), } tests := []struct { diff --git a/caddyhttp/errors/setup.go b/caddyhttp/errors/setup.go index db4177c2..113a7e02 100644 --- a/caddyhttp/errors/setup.go +++ b/caddyhttp/errors/setup.go @@ -1,14 +1,11 @@ package errors import ( - "io" "log" "os" "path/filepath" "strconv" - "sync" - "github.com/hashicorp/go-syslog" "github.com/mholt/caddy" "github.com/mholt/caddy/caddyhttp/httpserver" ) @@ -16,61 +13,12 @@ import ( // setup configures a new errors middleware instance. func setup(c *caddy.Controller) error { handler, err := errorsParse(c) + if err != nil { return err } - // Open the log file for writing when the server starts - c.OnStartup(func() error { - var err error - var writer io.Writer - - switch handler.LogFile { - case "visible": - handler.Debug = true - case "stdout": - writer = os.Stdout - case "stderr": - writer = os.Stderr - case "syslog": - writer, err = gsyslog.NewLogger(gsyslog.LOG_ERR, "LOCAL0", "caddy") - if err != nil { - return err - } - default: - if handler.LogFile == "" { - writer = os.Stderr // default - break - } - - var file *os.File - file, err = os.OpenFile(handler.LogFile, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0644) - if err != nil { - return err - } - if handler.LogRoller != nil { - file.Close() - handler.LogRoller.Filename = handler.LogFile - writer = handler.LogRoller.GetLogWriter() - } else { - handler.file = file - writer = file - } - } - - handler.Log = log.New(writer, "", 0) - return nil - }) - - // When server stops, close any open log file - c.OnShutdown(func() error { - if handler.file != nil { - handler.fileMu.Lock() - handler.file.Close() - handler.fileMu.Unlock() - } - return nil - }) + handler.Log.Attach(c) httpserver.GetConfig(c).AddMiddleware(func(next httpserver.Handler) httpserver.Handler { handler.Next = next @@ -81,10 +29,14 @@ func setup(c *caddy.Controller) error { } func errorsParse(c *caddy.Controller) (*ErrorHandler, error) { + // Very important that we make a pointer because the startup // function that opens the log file must have access to the // same instance of the handler, not a copy. - handler := &ErrorHandler{ErrorPages: make(map[int]string), fileMu: new(sync.RWMutex)} + handler := &ErrorHandler{ + ErrorPages: make(map[int]string), + Log: &httpserver.Logger{}, + } cfg := httpserver.GetConfig(c) @@ -104,7 +56,7 @@ func errorsParse(c *caddy.Controller) (*ErrorHandler, error) { if where == "visible" { handler.Debug = true } else { - handler.LogFile = where + handler.Log.Output = where if c.NextArg() { if c.Val() == "{" { c.IncrNest() @@ -112,7 +64,7 @@ func errorsParse(c *caddy.Controller) (*ErrorHandler, error) { if err != nil { return hadBlock, err } - handler.LogRoller = logRoller + handler.Log.Roller = logRoller } } } @@ -121,6 +73,7 @@ func errorsParse(c *caddy.Controller) (*ErrorHandler, error) { if !filepath.IsAbs(where) { where = filepath.Join(cfg.Root, where) } + f, err := os.Open(where) if err != nil { log.Printf("[WARNING] Unable to open error page '%s': %v", where, err) @@ -166,7 +119,7 @@ func errorsParse(c *caddy.Controller) (*ErrorHandler, error) { if c.Val() == "visible" { handler.Debug = true } else { - handler.LogFile = c.Val() + handler.Log.Output = c.Val() } } } diff --git a/caddyhttp/errors/setup_test.go b/caddyhttp/errors/setup_test.go index f9e539c2..dd0896b8 100644 --- a/caddyhttp/errors/setup_test.go +++ b/caddyhttp/errors/setup_test.go @@ -3,7 +3,6 @@ package errors import ( "path/filepath" "reflect" - "sync" "testing" "github.com/mholt/caddy" @@ -27,12 +26,12 @@ func TestSetup(t *testing.T) { t.Fatalf("Expected handler to be type ErrorHandler, got: %#v", handler) } - if myHandler.LogFile != "" { - t.Errorf("Expected '%s' as the default LogFile", "") - } - if myHandler.LogRoller != nil { - t.Errorf("Expected LogRoller to be nil, got: %v", *myHandler.LogRoller) + expectedLogger := &httpserver.Logger{} + + if !reflect.DeepEqual(expectedLogger, myHandler.Log) { + t.Errorf("Expected '%v' as the default Log, got: '%v'", expectedLogger, myHandler.Log) } + if !httpserver.SameNext(myHandler.Next, httpserver.EmptyNext) { t.Error("'Next' field of handler was not set properly") } @@ -59,78 +58,71 @@ func TestErrorsParse(t *testing.T) { }{ {`errors`, false, ErrorHandler{ ErrorPages: map[int]string{}, - fileMu: new(sync.RWMutex), + Log: &httpserver.Logger{}, }}, {`errors errors.txt`, false, ErrorHandler{ ErrorPages: map[int]string{}, - LogFile: "errors.txt", - fileMu: new(sync.RWMutex), + Log: &httpserver.Logger{Output: "errors.txt"}, }}, {`errors visible`, false, ErrorHandler{ ErrorPages: map[int]string{}, Debug: true, - fileMu: new(sync.RWMutex), + Log: &httpserver.Logger{}, }}, {`errors { log visible }`, false, ErrorHandler{ ErrorPages: map[int]string{}, Debug: true, - fileMu: new(sync.RWMutex), + Log: &httpserver.Logger{}, }}, {`errors { log errors.txt - 404 404.html - 500 500.html -}`, false, ErrorHandler{ - LogFile: "errors.txt", + 404 404.html + 500 500.html + }`, false, ErrorHandler{ ErrorPages: map[int]string{ 404: "404.html", 500: "500.html", }, - fileMu: new(sync.RWMutex), + Log: &httpserver.Logger{Output: "errors.txt"}, }}, {`errors { log errors.txt { size 2 age 10 keep 3 } }`, false, ErrorHandler{ - LogFile: "errors.txt", - LogRoller: &httpserver.LogRoller{ + ErrorPages: map[int]string{}, + Log: &httpserver.Logger{Output: "errors.txt", Roller: &httpserver.LogRoller{ MaxSize: 2, MaxAge: 10, MaxBackups: 3, LocalTime: true, - }, - ErrorPages: map[int]string{}, - fileMu: new(sync.RWMutex), - }}, + }}}, + }, {`errors { log errors.txt { - size 3 - age 11 - keep 5 - } - 404 404.html - 503 503.html -}`, false, ErrorHandler{ - LogFile: "errors.txt", + size 3 + age 11 + keep 5 + } + 404 404.html + 503 503.html + }`, false, ErrorHandler{ ErrorPages: map[int]string{ 404: "404.html", 503: "503.html", }, - LogRoller: &httpserver.LogRoller{ + Log: &httpserver.Logger{Output: "errors.txt", Roller: &httpserver.LogRoller{ MaxSize: 3, MaxAge: 11, MaxBackups: 5, LocalTime: true, }, - fileMu: new(sync.RWMutex), - }}, + }}}, {`errors { log errors.txt - * generic_error.html - 404 404.html - 503 503.html -}`, false, ErrorHandler{ - LogFile: "errors.txt", + * generic_error.html + 404 404.html + 503 503.html + }`, false, ErrorHandler{ + Log: &httpserver.Logger{Output: "errors.txt"}, GenericErrorPage: "generic_error.html", ErrorPages: map[int]string{ 404: "404.html", 503: "503.html", }, - fileMu: new(sync.RWMutex), }}, // test absolute file path {`errors { @@ -140,18 +132,20 @@ func TestErrorsParse(t *testing.T) { ErrorPages: map[int]string{ 404: testAbs, }, - fileMu: new(sync.RWMutex), + Log: &httpserver.Logger{}, }}, // Next two test cases is the detection of duplicate status codes {`errors { - 503 503.html - 503 503.html -}`, true, ErrorHandler{ErrorPages: map[int]string{}, fileMu: new(sync.RWMutex)}}, + 503 503.html + 503 503.html + }`, true, ErrorHandler{ErrorPages: map[int]string{}, Log: &httpserver.Logger{}}}, + {`errors { - * generic_error.html - * generic_error.html -}`, true, ErrorHandler{ErrorPages: map[int]string{}, fileMu: new(sync.RWMutex)}}, + * generic_error.html + * generic_error.html + }`, true, ErrorHandler{ErrorPages: map[int]string{}, Log: &httpserver.Logger{}}}, } + for i, test := range tests { actualErrorsRule, err := errorsParse(caddy.NewTestController("http", test.inputErrorsRules)) diff --git a/caddyhttp/httpserver/logger.go b/caddyhttp/httpserver/logger.go new file mode 100644 index 00000000..f16042c0 --- /dev/null +++ b/caddyhttp/httpserver/logger.go @@ -0,0 +1,148 @@ +package httpserver + +import ( + "bytes" + "io" + "log" + "os" + "strings" + "sync" + + "github.com/hashicorp/go-syslog" + "github.com/mholt/caddy" +) + +var remoteSyslogPrefixes = map[string]string{ + "syslog+tcp://": "tcp", + "syslog+udp://": "udp", + "syslog://": "udp", +} + +// Logger is shared between errors and log plugins and supports both logging to +// a file (with an optional file roller), local and remote syslog servers. +type Logger struct { + Output string + *log.Logger + Roller *LogRoller + writer io.Writer + fileMu *sync.RWMutex +} + +// NewTestLogger creates logger suitable for testing purposes +func NewTestLogger(buffer *bytes.Buffer) *Logger { + return &Logger{ + Logger: log.New(buffer, "", 0), + fileMu: new(sync.RWMutex), + } +} + +// Println wraps underlying logger with mutex +func (l Logger) Println(args ...interface{}) { + l.fileMu.RLock() + l.Logger.Println(args...) + l.fileMu.RUnlock() +} + +// Printf wraps underlying logger with mutex +func (l Logger) Printf(format string, args ...interface{}) { + l.fileMu.RLock() + l.Logger.Printf(format, args...) + l.fileMu.RUnlock() +} + +// Attach binds logger Start and Close functions to +// controller's OnStartup and OnShutdown hooks. +func (l *Logger) Attach(controller *caddy.Controller) { + if controller != nil { + // Opens file or connect to local/remote syslog + controller.OnStartup(l.Start) + + // Closes file or disconnects from local/remote syslog + controller.OnShutdown(l.Close) + } +} + +type syslogAddress struct { + network string + address string +} + +func parseSyslogAddress(location string) *syslogAddress { + for prefix, network := range remoteSyslogPrefixes { + if strings.HasPrefix(location, prefix) { + return &syslogAddress{ + network: network, + address: strings.TrimPrefix(location, prefix), + } + } + } + + return nil +} + +// Start initializes logger opening files or local/remote syslog connections +func (l *Logger) Start() error { + // initialize mutex on start + l.fileMu = new(sync.RWMutex) + + var err error + +selectwriter: + switch l.Output { + case "", "stderr": + l.writer = os.Stderr + + case "stdout": + l.writer = os.Stdout + + case "syslog": + l.writer, err = gsyslog.NewLogger(gsyslog.LOG_ERR, "LOCAL0", "caddy") + if err != nil { + return err + } + default: + + if address := parseSyslogAddress(l.Output); address != nil { + l.writer, err = gsyslog.DialLogger(address.network, address.address, gsyslog.LOG_ERR, "LOCAL0", "caddy") + + if err != nil { + return err + } + + break selectwriter + } + + var file *os.File + + file, err = os.OpenFile(l.Output, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0644) + if err != nil { + return err + } + + if l.Roller != nil { + file.Close() + l.Roller.Filename = l.Output + l.writer = l.Roller.GetLogWriter() + } else { + l.writer = file + } + } + + l.Logger = log.New(l.writer, "", 0) + + return nil + +} + +// Close closes open log files or connections to syslog. +func (l *Logger) Close() error { + // Will close local/remote syslog connections too :) + if closer, ok := l.writer.(io.WriteCloser); ok { + l.fileMu.Lock() + err := closer.Close() + l.fileMu.Unlock() + return err + } + + return nil +} diff --git a/caddyhttp/httpserver/logger_test.go b/caddyhttp/httpserver/logger_test.go new file mode 100644 index 00000000..b444c3a7 --- /dev/null +++ b/caddyhttp/httpserver/logger_test.go @@ -0,0 +1,212 @@ +//+build linux darwin + +package httpserver + +import ( + "bytes" + "fmt" + "io/ioutil" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "gopkg.in/mcuadros/go-syslog.v2" + "gopkg.in/mcuadros/go-syslog.v2/format" +) + +func TestLoggingToStdout(t *testing.T) { + testCases := []struct { + Output string + ExpectedOutput string + }{ + { + Output: "stdout", + ExpectedOutput: "Hello world logged to stdout", + }, + } + + for i, testCase := range testCases { + output := captureStdout(func() { + logger := Logger{Output: testCase.Output, fileMu: new(sync.RWMutex)} + + if err := logger.Start(); err != nil { + t.Fatalf("Got unexpected error: %v", err) + } + + logger.Println(testCase.ExpectedOutput) + }) + + if !strings.Contains(output, testCase.ExpectedOutput) { + t.Fatalf("Test #%d: Expected output to contain: %s, got: %s", i, testCase.ExpectedOutput, output) + } + } +} + +func TestLoggingToStderr(t *testing.T) { + + testCases := []struct { + Output string + ExpectedOutput string + }{ + { + Output: "stderr", + ExpectedOutput: "Hello world logged to stderr", + }, + { + Output: "", + ExpectedOutput: "Hello world logged to stderr #2", + }, + } + + for i, testCase := range testCases { + output := captureStderr(func() { + logger := Logger{Output: testCase.Output, fileMu: new(sync.RWMutex)} + + if err := logger.Start(); err != nil { + t.Fatalf("Got unexpected error: %v", err) + } + + logger.Println(testCase.ExpectedOutput) + }) + + if !strings.Contains(output, testCase.ExpectedOutput) { + t.Fatalf("Test #%d: Expected output to contain: %s, got: %s", i, testCase.ExpectedOutput, output) + } + } +} + +func TestLoggingToFile(t *testing.T) { + file := filepath.Join(os.TempDir(), "access.log") + expectedOutput := "Hello world written to file" + + logger := Logger{Output: file} + + if err := logger.Start(); err != nil { + t.Fatalf("Got unexpected error during logger start: %v", err) + } + + logger.Print(expectedOutput) + + content, err := ioutil.ReadFile(file) + if err != nil { + t.Fatalf("Could not read log file content: %v", err) + } + + if !bytes.Contains(content, []byte(expectedOutput)) { + t.Fatalf("Expected log file to contain: %s, got: %s", expectedOutput, string(content)) + } + + os.Remove(file) +} + +func TestLoggingToSyslog(t *testing.T) { + + testCases := []struct { + Output string + ExpectedOutput string + }{ + { + Output: "syslog://127.0.0.1:5660", + ExpectedOutput: "Hello world! Test #1 over tcp", + }, + { + Output: "syslog+tcp://127.0.0.1:5661", + ExpectedOutput: "Hello world! Test #2 over tcp", + }, + { + Output: "syslog+udp://127.0.0.1:5662", + ExpectedOutput: "Hello world! Test #3 over udp", + }, + } + + for i, testCase := range testCases { + + ch := make(chan format.LogParts, 256) + server, err := bootServer(testCase.Output, ch) + defer server.Kill() + + if err != nil { + t.Errorf("Test #%d: expected no error during syslog server boot, got: %v", i, err) + } + + logger := Logger{Output: testCase.Output, fileMu: new(sync.RWMutex)} + + if err := logger.Start(); err != nil { + t.Errorf("Test #%d: expected no error during logger start, got: %v", i, err) + } + + defer logger.Close() + + logger.Print(testCase.ExpectedOutput) + + actual := <-ch + + if content, ok := actual["content"].(string); ok { + if !strings.Contains(content, testCase.ExpectedOutput) { + t.Errorf("Test #%d: expected server to capture content: %s, but got: %s", i, testCase.ExpectedOutput, content) + } + } else { + t.Errorf("Test #%d: expected server to capture content but got: %v", i, actual) + } + } +} + +func bootServer(location string, ch chan format.LogParts) (*syslog.Server, error) { + address := parseSyslogAddress(location) + + if address == nil { + return nil, fmt.Errorf("Could not parse syslog address: %s", location) + } + + server := syslog.NewServer() + server.SetFormat(syslog.Automatic) + + switch address.network { + case "tcp": + server.ListenTCP(address.address) + case "udp": + server.ListenUDP(address.address) + } + + server.SetHandler(syslog.NewChannelHandler(ch)) + + if err := server.Boot(); err != nil { + return nil, err + } + + return server, nil +} + +func captureStdout(f func()) string { + original := os.Stdout + r, w, _ := os.Pipe() + + os.Stdout = w + + f() + + w.Close() + + written, _ := ioutil.ReadAll(r) + os.Stdout = original + + return string(written) +} + +func captureStderr(f func()) string { + original := os.Stderr + r, w, _ := os.Pipe() + + os.Stderr = w + + f() + + w.Close() + + written, _ := ioutil.ReadAll(r) + os.Stderr = original + + return string(written) +} diff --git a/caddyhttp/internalsrv/internal_test.go b/caddyhttp/internalsrv/internal_test.go index b519460a..ea071044 100644 --- a/caddyhttp/internalsrv/internal_test.go +++ b/caddyhttp/internalsrv/internal_test.go @@ -6,8 +6,9 @@ import ( "net/http/httptest" "testing" - "github.com/mholt/caddy/caddyhttp/httpserver" "strconv" + + "github.com/mholt/caddy/caddyhttp/httpserver" ) const ( diff --git a/caddyhttp/log/log.go b/caddyhttp/log/log.go index 4470bccf..d0a0fd69 100644 --- a/caddyhttp/log/log.go +++ b/caddyhttp/log/log.go @@ -3,10 +3,7 @@ package log import ( "fmt" - "log" "net/http" - "os" - "sync" "github.com/mholt/caddy" "github.com/mholt/caddy/caddyhttp/httpserver" @@ -55,9 +52,7 @@ func (l Logger) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { // Write log entries for _, e := range rule.Entries { - e.fileMu.RLock() e.Log.Println(rep.Replace(e.Format)) - e.fileMu.RUnlock() } return status, err @@ -68,12 +63,8 @@ func (l Logger) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { // Entry represents a log entry under a path scope type Entry struct { - OutputFile string - Format string - Log *log.Logger - Roller *httpserver.LogRoller - file *os.File // if logging to a file that needs to be closed - fileMu *sync.RWMutex // files can't be safely read/written in one goroutine and closed in another (issue #1371) + Format string + Log *httpserver.Logger } // Rule configures the logging middleware. diff --git a/caddyhttp/log/log_test.go b/caddyhttp/log/log_test.go index 8dcf928b..2cf46afd 100644 --- a/caddyhttp/log/log_test.go +++ b/caddyhttp/log/log_test.go @@ -3,11 +3,9 @@ package log import ( "bytes" "io/ioutil" - "log" "net/http" "net/http/httptest" "strings" - "sync" "testing" "github.com/mholt/caddy/caddyhttp/httpserver" @@ -29,8 +27,7 @@ func TestLoggedStatus(t *testing.T) { PathScope: "/", Entries: []*Entry{{ Format: DefaultLogFormat + " {testval}", - Log: log.New(&f, "", 0), - fileMu: new(sync.RWMutex), + Log: httpserver.NewTestLogger(&f), }}, } @@ -73,8 +70,7 @@ func TestLogRequestBody(t *testing.T) { PathScope: "/", Entries: []*Entry{{ Format: "{request_body}", - Log: log.New(&got, "", 0), - fileMu: new(sync.RWMutex), + Log: httpserver.NewTestLogger(&got), }}, }}, Next: httpserver.HandlerFunc(func(w http.ResponseWriter, r *http.Request) (int, error) { @@ -133,13 +129,11 @@ func TestMultiEntries(t *testing.T) { Entries: []*Entry{ { Format: "foo {request_body}", - Log: log.New(&got1, "", 0), - fileMu: new(sync.RWMutex), + Log: httpserver.NewTestLogger(&got1), }, { Format: "{method} {request_body}", - Log: log.New(&got2, "", 0), - fileMu: new(sync.RWMutex), + Log: httpserver.NewTestLogger(&got2), }, }, }}, diff --git a/caddyhttp/log/setup.go b/caddyhttp/log/setup.go index 8dd586b6..3b909d1a 100644 --- a/caddyhttp/log/setup.go +++ b/caddyhttp/log/setup.go @@ -1,13 +1,6 @@ package log import ( - "io" - "log" - "os" - "path/filepath" - "sync" - - "github.com/hashicorp/go-syslog" "github.com/mholt/caddy" "github.com/mholt/caddy/caddyhttp/httpserver" ) @@ -19,61 +12,11 @@ func setup(c *caddy.Controller) error { return err } - // Open the log files for writing when the server starts - c.OnStartup(func() error { - for _, rule := range rules { - for _, entry := range rule.Entries { - var err error - var writer io.Writer - - if entry.OutputFile == "stdout" { - writer = os.Stdout - } else if entry.OutputFile == "stderr" { - writer = os.Stderr - } else if entry.OutputFile == "syslog" { - writer, err = gsyslog.NewLogger(gsyslog.LOG_INFO, "LOCAL0", "caddy") - if err != nil { - return err - } - } else { - err := os.MkdirAll(filepath.Dir(entry.OutputFile), 0744) - if err != nil { - return err - } - file, err := os.OpenFile(entry.OutputFile, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0644) - if err != nil { - return err - } - if entry.Roller != nil { - file.Close() - entry.Roller.Filename = entry.OutputFile - writer = entry.Roller.GetLogWriter() - } else { - entry.file = file - writer = file - } - } - - entry.Log = log.New(writer, "", 0) - } + for _, rule := range rules { + for _, entry := range rule.Entries { + entry.Log.Attach(c) } - - return nil - }) - - // When server stops, close any open log files - c.OnShutdown(func() error { - for _, rule := range rules { - for _, entry := range rule.Entries { - if entry.file != nil { - entry.fileMu.Lock() - entry.file.Close() - entry.fileMu.Unlock() - } - } - } - return nil - }) + } httpserver.GetConfig(c).AddMiddleware(func(next httpserver.Handler) httpserver.Handler { return Logger{Next: next, Rules: rules, ErrorFunc: httpserver.DefaultErrorFunc} @@ -111,18 +54,20 @@ func logParse(c *caddy.Controller) ([]*Rule, error) { if len(args) == 0 { // Nothing specified; use defaults rules = appendEntry(rules, "/", &Entry{ - OutputFile: DefaultLogFilename, - Format: DefaultLogFormat, - Roller: logRoller, - fileMu: new(sync.RWMutex), + Log: &httpserver.Logger{ + Output: DefaultLogFilename, + Roller: logRoller, + }, + Format: DefaultLogFormat, }) } else if len(args) == 1 { // Only an output file specified rules = appendEntry(rules, "/", &Entry{ - OutputFile: args[0], - Format: DefaultLogFormat, - Roller: logRoller, - fileMu: new(sync.RWMutex), + Log: &httpserver.Logger{ + Output: args[0], + Roller: logRoller, + }, + Format: DefaultLogFormat, }) } else { // Path scope, output file, and maybe a format specified @@ -141,10 +86,11 @@ func logParse(c *caddy.Controller) ([]*Rule, error) { } rules = appendEntry(rules, args[0], &Entry{ - OutputFile: args[1], - Format: format, - Roller: logRoller, - fileMu: new(sync.RWMutex), + Log: &httpserver.Logger{ + Output: args[1], + Roller: logRoller, + }, + Format: format, }) } } diff --git a/caddyhttp/log/setup_test.go b/caddyhttp/log/setup_test.go index 8d613907..fa82e69c 100644 --- a/caddyhttp/log/setup_test.go +++ b/caddyhttp/log/setup_test.go @@ -3,6 +3,8 @@ package log import ( "testing" + "reflect" + "github.com/mholt/caddy" "github.com/mholt/caddy/caddyhttp/httpserver" ) @@ -29,20 +31,19 @@ func TestSetup(t *testing.T) { if myHandler.Rules[0].PathScope != "/" { t.Errorf("Expected / as the default PathScope") } - if myHandler.Rules[0].Entries[0].OutputFile != DefaultLogFilename { - t.Errorf("Expected %s as the default OutputFile", DefaultLogFilename) + + expectedLogger := &httpserver.Logger{Output: DefaultLogFilename} + + if !reflect.DeepEqual(myHandler.Rules[0].Entries[0].Log, expectedLogger) { + t.Errorf("Expected %v as the default Log, got: %v", expectedLogger, myHandler.Rules[0].Entries[0].Log) } if myHandler.Rules[0].Entries[0].Format != DefaultLogFormat { t.Errorf("Expected %s as the default Log Format", DefaultLogFormat) } - if myHandler.Rules[0].Entries[0].Roller != nil { - t.Errorf("Expected Roller to be nil, got: %v", - *myHandler.Rules[0].Entries[0].Roller) - } + if !httpserver.SameNext(myHandler.Next, httpserver.EmptyNext) { t.Error("'Next' field of handler was not set properly") } - } func TestLogParse(t *testing.T) { @@ -54,95 +55,108 @@ func TestLogParse(t *testing.T) { {`log`, false, []Rule{{ PathScope: "/", Entries: []*Entry{{ - OutputFile: DefaultLogFilename, - Format: DefaultLogFormat, + Log: &httpserver.Logger{Output: DefaultLogFilename}, + Format: DefaultLogFormat, }}, }}}, {`log log.txt`, false, []Rule{{ PathScope: "/", Entries: []*Entry{{ - OutputFile: "log.txt", - Format: DefaultLogFormat, + Log: &httpserver.Logger{Output: "log.txt"}, + Format: DefaultLogFormat, + }}, + }}}, + {`log syslog://127.0.0.1:5000`, false, []Rule{{ + PathScope: "/", + Entries: []*Entry{{ + Log: &httpserver.Logger{Output: "syslog://127.0.0.1:5000"}, + Format: DefaultLogFormat, + }}, + }}}, + {`log syslog+tcp://127.0.0.1:5000`, false, []Rule{{ + PathScope: "/", + Entries: []*Entry{{ + Log: &httpserver.Logger{Output: "syslog+tcp://127.0.0.1:5000"}, + Format: DefaultLogFormat, }}, }}}, {`log /api log.txt`, false, []Rule{{ PathScope: "/api", Entries: []*Entry{{ - OutputFile: "log.txt", - Format: DefaultLogFormat, + Log: &httpserver.Logger{Output: "log.txt"}, + Format: DefaultLogFormat, }}, }}}, {`log /serve stdout`, false, []Rule{{ PathScope: "/serve", Entries: []*Entry{{ - OutputFile: "stdout", - Format: DefaultLogFormat, + Log: &httpserver.Logger{Output: "stdout"}, + Format: DefaultLogFormat, }}, }}}, {`log /myapi log.txt {common}`, false, []Rule{{ PathScope: "/myapi", Entries: []*Entry{{ - OutputFile: "log.txt", - Format: CommonLogFormat, + Log: &httpserver.Logger{Output: "log.txt"}, + Format: CommonLogFormat, }}, }}}, {`log /test accesslog.txt {combined}`, false, []Rule{{ PathScope: "/test", Entries: []*Entry{{ - OutputFile: "accesslog.txt", - Format: CombinedLogFormat, + Log: &httpserver.Logger{Output: "accesslog.txt"}, + Format: CombinedLogFormat, }}, }}}, {`log /api1 log.txt log /api2 accesslog.txt {combined}`, false, []Rule{{ PathScope: "/api1", Entries: []*Entry{{ - OutputFile: "log.txt", - Format: DefaultLogFormat, + Log: &httpserver.Logger{Output: "log.txt"}, + Format: DefaultLogFormat, }}, }, { PathScope: "/api2", Entries: []*Entry{{ - OutputFile: "accesslog.txt", - Format: CombinedLogFormat, + Log: &httpserver.Logger{Output: "accesslog.txt"}, + Format: CombinedLogFormat, }}, }}}, {`log /api3 stdout {host} log /api4 log.txt {when}`, false, []Rule{{ PathScope: "/api3", Entries: []*Entry{{ - OutputFile: "stdout", - Format: "{host}", + Log: &httpserver.Logger{Output: "stdout"}, + Format: "{host}", }}, }, { PathScope: "/api4", Entries: []*Entry{{ - OutputFile: "log.txt", - Format: "{when}", + Log: &httpserver.Logger{Output: "log.txt"}, + Format: "{when}", }}, }}}, {`log access.log { rotate { size 2 age 10 keep 3 } }`, false, []Rule{{ PathScope: "/", Entries: []*Entry{{ - OutputFile: "access.log", - Format: DefaultLogFormat, - Roller: &httpserver.LogRoller{ + Log: &httpserver.Logger{Output: "access.log", Roller: &httpserver.LogRoller{ MaxSize: 2, MaxAge: 10, MaxBackups: 3, LocalTime: true, - }, + }}, + Format: DefaultLogFormat, }}, }}}, {`log / stdout {host} log / log.txt {when}`, false, []Rule{{ PathScope: "/", Entries: []*Entry{{ - OutputFile: "stdout", - Format: "{host}", + Log: &httpserver.Logger{Output: "stdout"}, + Format: "{host}", }, { - OutputFile: "log.txt", - Format: "{when}", + Log: &httpserver.Logger{Output: "log.txt"}, + Format: "{when}", }}, }}}, } @@ -172,43 +186,16 @@ func TestLogParse(t *testing.T) { } for k, actualEntry := range actualLogRule.Entries { - if actualEntry.OutputFile != test.expectedLogRules[j].Entries[k].OutputFile { - t.Errorf("Test %d expected %dth LogRule OutputFile to be %s , but got %s", - i, j, test.expectedLogRules[j].Entries[k].OutputFile, actualEntry.OutputFile) + if !reflect.DeepEqual(actualEntry.Log, test.expectedLogRules[j].Entries[k].Log) { + t.Errorf("Test %d expected %dth LogRule Log to be %v , but got %v", + i, j, test.expectedLogRules[j].Entries[k].Log, actualEntry.Log) } if actualEntry.Format != test.expectedLogRules[j].Entries[k].Format { t.Errorf("Test %d expected %dth LogRule Format to be %s , but got %s", i, j, test.expectedLogRules[j].Entries[k].Format, actualEntry.Format) } - if actualEntry.Roller != nil && test.expectedLogRules[j].Entries[k].Roller == nil || actualEntry.Roller == nil && test.expectedLogRules[j].Entries[k].Roller != nil { - t.Fatalf("Test %d expected %dth LogRule Roller to be %v, but got %v", - i, j, test.expectedLogRules[j].Entries[k].Roller, actualEntry.Roller) - } - if actualEntry.Roller != nil && test.expectedLogRules[j].Entries[k].Roller != nil { - if actualEntry.Roller.Filename != test.expectedLogRules[j].Entries[k].Roller.Filename { - t.Fatalf("Test %d expected %dth LogRule Roller Filename to be %s, but got %s", - i, j, test.expectedLogRules[j].Entries[k].Roller.Filename, actualEntry.Roller.Filename) - } - if actualEntry.Roller.MaxAge != test.expectedLogRules[j].Entries[k].Roller.MaxAge { - t.Fatalf("Test %d expected %dth LogRule Roller MaxAge to be %d, but got %d", - i, j, test.expectedLogRules[j].Entries[k].Roller.MaxAge, actualEntry.Roller.MaxAge) - } - if actualEntry.Roller.MaxBackups != test.expectedLogRules[j].Entries[k].Roller.MaxBackups { - t.Fatalf("Test %d expected %dth LogRule Roller MaxBackups to be %d, but got %d", - i, j, test.expectedLogRules[j].Entries[k].Roller.MaxBackups, actualEntry.Roller.MaxBackups) - } - if actualEntry.Roller.MaxSize != test.expectedLogRules[j].Entries[k].Roller.MaxSize { - t.Fatalf("Test %d expected %dth LogRule Roller MaxSize to be %d, but got %d", - i, j, test.expectedLogRules[j].Entries[k].Roller.MaxSize, actualEntry.Roller.MaxSize) - } - if actualEntry.Roller.LocalTime != test.expectedLogRules[j].Entries[k].Roller.LocalTime { - t.Fatalf("Test %d expected %dth LogRule Roller LocalTime to be %t, but got %t", - i, j, test.expectedLogRules[j].Entries[k].Roller.LocalTime, actualEntry.Roller.LocalTime) - } - } } } } - } diff --git a/caddyhttp/markdown/process_test.go b/caddyhttp/markdown/process_test.go index bce1613d..fbafaf98 100644 --- a/caddyhttp/markdown/process_test.go +++ b/caddyhttp/markdown/process_test.go @@ -1,10 +1,11 @@ package markdown import ( - "github.com/mholt/caddy/caddyhttp/httpserver" "os" "strings" "testing" + + "github.com/mholt/caddy/caddyhttp/httpserver" ) func TestConfig_Markdown(t *testing.T) { From ce7d3db1be57b04975922a3988c02df8f993293f Mon Sep 17 00:00:00 2001 From: "Julian V. Modesto" Date: Wed, 8 Feb 2017 11:23:33 -0500 Subject: [PATCH 07/15] Roll all logs by default (#1379) * Use new subdirectives and flatten rolling config * Set default rotate config * Set default rolling config (hopefully) errwhere * Make private * Flatten errors directive and remove c.IncrNest() * Don't skip first error log roller subdirective we see * Remove hadBlock * Try lumberjack import * Unname import --- caddyfile/dispenser.go | 6 --- caddyhttp/errors/setup.go | 63 ++++++++++------------- caddyhttp/errors/setup_test.go | 88 +++++++++++++++++--------------- caddyhttp/httpserver/roller.go | 83 +++++++++++++++++------------- caddyhttp/log/setup.go | 31 ++++++------ caddyhttp/log/setup_test.go | 92 +++++++++++++++++++++++++--------- 6 files changed, 207 insertions(+), 156 deletions(-) diff --git a/caddyfile/dispenser.go b/caddyfile/dispenser.go index 91af9b12..edb7bfaf 100644 --- a/caddyfile/dispenser.go +++ b/caddyfile/dispenser.go @@ -120,12 +120,6 @@ func (d *Dispenser) NextBlock() bool { return true } -// IncrNest adds a level of nesting to the dispenser. -func (d *Dispenser) IncrNest() { - d.nesting++ - return -} - // Val gets the text of the current token. If there is no token // loaded, it returns empty string. func (d *Dispenser) Val() string { diff --git a/caddyhttp/errors/setup.go b/caddyhttp/errors/setup.go index 113a7e02..b6e2025f 100644 --- a/caddyhttp/errors/setup.go +++ b/caddyhttp/errors/setup.go @@ -40,33 +40,20 @@ func errorsParse(c *caddy.Controller) (*ErrorHandler, error) { cfg := httpserver.GetConfig(c) - optionalBlock := func() (bool, error) { - var hadBlock bool - + optionalBlock := func() error { for c.NextBlock() { - hadBlock = true what := c.Val() if !c.NextArg() { - return hadBlock, c.ArgErr() + return c.ArgErr() } where := c.Val() - if what == "log" { - if where == "visible" { - handler.Debug = true - } else { - handler.Log.Output = where - if c.NextArg() { - if c.Val() == "{" { - c.IncrNest() - logRoller, err := httpserver.ParseRoller(c) - if err != nil { - return hadBlock, err - } - handler.Log.Roller = logRoller - } - } + if httpserver.IsLogRollerSubdirective(what) { + var err error + err = httpserver.ParseRoller(handler.Log.Roller, what, where) + if err != nil { + return err } } else { // Error page; ensure it exists @@ -82,24 +69,24 @@ func errorsParse(c *caddy.Controller) (*ErrorHandler, error) { if what == "*" { if handler.GenericErrorPage != "" { - return hadBlock, c.Errf("Duplicate status code entry: %s", what) + return c.Errf("Duplicate status code entry: %s", what) } handler.GenericErrorPage = where } else { whatInt, err := strconv.Atoi(what) if err != nil { - return hadBlock, c.Err("Expecting a numeric status code or '*', got '" + what + "'") + return c.Err("Expecting a numeric status code or '*', got '" + what + "'") } if _, exists := handler.ErrorPages[whatInt]; exists { - return hadBlock, c.Errf("Duplicate status code entry: %s", what) + return c.Errf("Duplicate status code entry: %s", what) } handler.ErrorPages[whatInt] = where } } } - return hadBlock, nil + return nil } for c.Next() { @@ -107,21 +94,23 @@ func errorsParse(c *caddy.Controller) (*ErrorHandler, error) { if c.Val() == "}" { continue } - // Configuration may be in a block - hadBlock, err := optionalBlock() - if err != nil { - return handler, err + + args := c.RemainingArgs() + + if len(args) == 1 { + switch args[0] { + case "visible": + handler.Debug = true + default: + handler.Log.Output = args[0] + handler.Log.Roller = httpserver.DefaultLogRoller() + } } - // Otherwise, the only argument would be an error log file name or 'visible' - if !hadBlock { - if c.NextArg() { - if c.Val() == "visible" { - handler.Debug = true - } else { - handler.Log.Output = c.Val() - } - } + // Configuration may be in a block + err := optionalBlock() + if err != nil { + return handler, err } } diff --git a/caddyhttp/errors/setup_test.go b/caddyhttp/errors/setup_test.go index dd0896b8..2132a4f1 100644 --- a/caddyhttp/errors/setup_test.go +++ b/caddyhttp/errors/setup_test.go @@ -62,62 +62,70 @@ func TestErrorsParse(t *testing.T) { }}, {`errors errors.txt`, false, ErrorHandler{ ErrorPages: map[int]string{}, - Log: &httpserver.Logger{Output: "errors.txt"}, + Log: &httpserver.Logger{ + Output: "errors.txt", + Roller: httpserver.DefaultLogRoller(), + }, }}, {`errors visible`, false, ErrorHandler{ ErrorPages: map[int]string{}, Debug: true, Log: &httpserver.Logger{}, }}, - {`errors { log visible }`, false, ErrorHandler{ - ErrorPages: map[int]string{}, - Debug: true, - Log: &httpserver.Logger{}, - }}, - {`errors { log errors.txt - 404 404.html - 500 500.html - }`, false, ErrorHandler{ + {`errors errors.txt { + 404 404.html + 500 500.html +}`, false, ErrorHandler{ ErrorPages: map[int]string{ 404: "404.html", 500: "500.html", }, - Log: &httpserver.Logger{Output: "errors.txt"}, + Log: &httpserver.Logger{ + Output: "errors.txt", + Roller: httpserver.DefaultLogRoller(), + }, }}, - {`errors { log errors.txt { size 2 age 10 keep 3 } }`, false, ErrorHandler{ + {`errors errors.txt { rotate_size 2 rotate_age 10 rotate_keep 3 }`, false, ErrorHandler{ ErrorPages: map[int]string{}, - Log: &httpserver.Logger{Output: "errors.txt", Roller: &httpserver.LogRoller{ - MaxSize: 2, - MaxAge: 10, - MaxBackups: 3, - LocalTime: true, - }}}, - }, - {`errors { log errors.txt { - size 3 - age 11 - keep 5 - } - 404 404.html - 503 503.html - }`, false, ErrorHandler{ + Log: &httpserver.Logger{ + Output: "errors.txt", Roller: &httpserver.LogRoller{ + MaxSize: 2, + MaxAge: 10, + MaxBackups: 3, + LocalTime: true, + }, + }, + }}, + {`errors errors.txt { + rotate_size 3 + rotate_age 11 + rotate_keep 5 + 404 404.html + 503 503.html +}`, false, ErrorHandler{ ErrorPages: map[int]string{ 404: "404.html", 503: "503.html", }, - Log: &httpserver.Logger{Output: "errors.txt", Roller: &httpserver.LogRoller{ - MaxSize: 3, - MaxAge: 11, - MaxBackups: 5, - LocalTime: true, + Log: &httpserver.Logger{ + Output: "errors.txt", + Roller: &httpserver.LogRoller{ + MaxSize: 3, + MaxAge: 11, + MaxBackups: 5, + LocalTime: true, + }, + }, + }}, + {`errors errors.txt { + * generic_error.html + 404 404.html + 503 503.html +}`, false, ErrorHandler{ + Log: &httpserver.Logger{ + Output: "errors.txt", + Roller: httpserver.DefaultLogRoller(), }, - }}}, - {`errors { log errors.txt - * generic_error.html - 404 404.html - 503 503.html - }`, false, ErrorHandler{ - Log: &httpserver.Logger{Output: "errors.txt"}, GenericErrorPage: "generic_error.html", ErrorPages: map[int]string{ 404: "404.html", @@ -158,7 +166,7 @@ func TestErrorsParse(t *testing.T) { } if !reflect.DeepEqual(actualErrorsRule, &test.expectedErrorHandler) { t.Errorf("Test %d expect %v, but got %v", i, - actualErrorsRule, test.expectedErrorHandler) + test.expectedErrorHandler, actualErrorsRule) } } } diff --git a/caddyhttp/httpserver/roller.go b/caddyhttp/httpserver/roller.go index 0660de65..b4f648ec 100644 --- a/caddyhttp/httpserver/roller.go +++ b/caddyhttp/httpserver/roller.go @@ -5,8 +5,6 @@ import ( "path/filepath" "strconv" - "github.com/mholt/caddy" - "gopkg.in/natefinch/lumberjack.v2" ) @@ -46,40 +44,57 @@ func (l LogRoller) GetLogWriter() io.Writer { return lj } -// ParseRoller parses roller contents out of c. -func ParseRoller(c *caddy.Controller) (*LogRoller, error) { - var size, age, keep int - // This is kind of a hack to support nested blocks: - // As we are already in a block: either log or errors, - // c.nesting > 0 but, as soon as c meets a }, it thinks - // the block is over and return false for c.NextBlock. - for c.NextBlock() { - what := c.Val() - if !c.NextArg() { - return nil, c.ArgErr() - } - value := c.Val() - var err error - switch what { - case "size": - size, err = strconv.Atoi(value) - case "age": - age, err = strconv.Atoi(value) - case "keep": - keep, err = strconv.Atoi(value) - } - if err != nil { - return nil, err - } - } - return &LogRoller{ - MaxSize: size, - MaxAge: age, - MaxBackups: keep, - LocalTime: true, - }, nil +// IsLogRollerSubdirective is true if the subdirective is for the log roller. +func IsLogRollerSubdirective(subdir string) bool { + return subdir == directiveRotateSize || + subdir == directiveRotateAge || + subdir == directiveRotateKeep } +// ParseRoller parses roller contents out of c. +func ParseRoller(l *LogRoller, what string, where string) error { + if l == nil { + l = DefaultLogRoller() + } + var value int + var err error + value, err = strconv.Atoi(where) + if err != nil { + return err + } + switch what { + case directiveRotateSize: + l.MaxSize = value + case directiveRotateAge: + l.MaxAge = value + case directiveRotateKeep: + l.MaxBackups = value + } + return nil +} + +// DefaultLogRoller will roll logs by default. +func DefaultLogRoller() *LogRoller { + return &LogRoller{ + MaxSize: defaultRotateSize, + MaxAge: defaultRotateAge, + MaxBackups: defaultRotateKeep, + LocalTime: true, + } +} + +const ( + // defaultRotateSize is 100 MB. + defaultRotateSize = 100 + // defaultRotateAge is 14 days. + defaultRotateAge = 14 + // defaultRotateKeep is 10 files. + defaultRotateKeep = 10 + directiveRotateSize = "rotate_size" + directiveRotateAge = "rotate_age" + directiveRotateKeep = "rotate_keep" +) + // lumberjacks maps log filenames to the logger // that is being used to keep them rolled/maintained. var lumberjacks = make(map[string]*lumberjack.Logger) diff --git a/caddyhttp/log/setup.go b/caddyhttp/log/setup.go index 3b909d1a..2366aec0 100644 --- a/caddyhttp/log/setup.go +++ b/caddyhttp/log/setup.go @@ -32,25 +32,24 @@ func logParse(c *caddy.Controller) ([]*Rule, error) { args := c.RemainingArgs() var logRoller *httpserver.LogRoller - if c.NextBlock() { - if c.Val() == "rotate" { - if c.NextArg() { - if c.Val() == "{" { - var err error - logRoller, err = httpserver.ParseRoller(c) - if err != nil { - return nil, err - } - // This part doesn't allow having something after the rotate block - if c.Next() { - if c.Val() != "}" { - return nil, c.ArgErr() - } - } - } + logRoller = httpserver.DefaultLogRoller() + + for c.NextBlock() { + what := c.Val() + if !c.NextArg() { + return nil, c.ArgErr() + } + where := c.Val() + + if httpserver.IsLogRollerSubdirective(what) { + var err error + err = httpserver.ParseRoller(logRoller, what, where) + if err != nil { + return nil, err } } } + if len(args) == 0 { // Nothing specified; use defaults rules = appendEntry(rules, "/", &Entry{ diff --git a/caddyhttp/log/setup_test.go b/caddyhttp/log/setup_test.go index fa82e69c..f56749c5 100644 --- a/caddyhttp/log/setup_test.go +++ b/caddyhttp/log/setup_test.go @@ -32,7 +32,10 @@ func TestSetup(t *testing.T) { t.Errorf("Expected / as the default PathScope") } - expectedLogger := &httpserver.Logger{Output: DefaultLogFilename} + expectedLogger := &httpserver.Logger{ + Output: DefaultLogFilename, + Roller: httpserver.DefaultLogRoller(), + } if !reflect.DeepEqual(myHandler.Rules[0].Entries[0].Log, expectedLogger) { t.Errorf("Expected %v as the default Log, got: %v", expectedLogger, myHandler.Rules[0].Entries[0].Log) @@ -40,7 +43,6 @@ func TestSetup(t *testing.T) { if myHandler.Rules[0].Entries[0].Format != DefaultLogFormat { t.Errorf("Expected %s as the default Log Format", DefaultLogFormat) } - if !httpserver.SameNext(myHandler.Next, httpserver.EmptyNext) { t.Error("'Next' field of handler was not set properly") } @@ -55,56 +57,80 @@ func TestLogParse(t *testing.T) { {`log`, false, []Rule{{ PathScope: "/", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: DefaultLogFilename}, + Log: &httpserver.Logger{ + Output: DefaultLogFilename, + Roller: httpserver.DefaultLogRoller(), + }, Format: DefaultLogFormat, }}, }}}, {`log log.txt`, false, []Rule{{ PathScope: "/", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "log.txt"}, + Log: &httpserver.Logger{ + Output: "log.txt", + Roller: httpserver.DefaultLogRoller(), + }, Format: DefaultLogFormat, }}, }}}, {`log syslog://127.0.0.1:5000`, false, []Rule{{ PathScope: "/", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "syslog://127.0.0.1:5000"}, + Log: &httpserver.Logger{ + Output: "syslog://127.0.0.1:5000", + Roller: httpserver.DefaultLogRoller(), + }, Format: DefaultLogFormat, }}, }}}, {`log syslog+tcp://127.0.0.1:5000`, false, []Rule{{ PathScope: "/", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "syslog+tcp://127.0.0.1:5000"}, + Log: &httpserver.Logger{ + Output: "syslog+tcp://127.0.0.1:5000", + Roller: httpserver.DefaultLogRoller(), + }, Format: DefaultLogFormat, }}, }}}, {`log /api log.txt`, false, []Rule{{ PathScope: "/api", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "log.txt"}, + Log: &httpserver.Logger{ + Output: "log.txt", + Roller: httpserver.DefaultLogRoller(), + }, Format: DefaultLogFormat, }}, }}}, {`log /serve stdout`, false, []Rule{{ PathScope: "/serve", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "stdout"}, + Log: &httpserver.Logger{ + Output: "stdout", + Roller: httpserver.DefaultLogRoller(), + }, Format: DefaultLogFormat, }}, }}}, {`log /myapi log.txt {common}`, false, []Rule{{ PathScope: "/myapi", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "log.txt"}, + Log: &httpserver.Logger{ + Output: "log.txt", + Roller: httpserver.DefaultLogRoller(), + }, Format: CommonLogFormat, }}, }}}, {`log /test accesslog.txt {combined}`, false, []Rule{{ PathScope: "/test", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "accesslog.txt"}, + Log: &httpserver.Logger{ + Output: "accesslog.txt", + Roller: httpserver.DefaultLogRoller(), + }, Format: CombinedLogFormat, }}, }}}, @@ -112,13 +138,19 @@ func TestLogParse(t *testing.T) { log /api2 accesslog.txt {combined}`, false, []Rule{{ PathScope: "/api1", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "log.txt"}, + Log: &httpserver.Logger{ + Output: "log.txt", + Roller: httpserver.DefaultLogRoller(), + }, Format: DefaultLogFormat, }}, }, { PathScope: "/api2", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "accesslog.txt"}, + Log: &httpserver.Logger{ + Output: "accesslog.txt", + Roller: httpserver.DefaultLogRoller(), + }, Format: CombinedLogFormat, }}, }}}, @@ -126,25 +158,33 @@ func TestLogParse(t *testing.T) { log /api4 log.txt {when}`, false, []Rule{{ PathScope: "/api3", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "stdout"}, + Log: &httpserver.Logger{ + Output: "stdout", + Roller: httpserver.DefaultLogRoller(), + }, Format: "{host}", }}, }, { PathScope: "/api4", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "log.txt"}, + Log: &httpserver.Logger{ + Output: "log.txt", + Roller: httpserver.DefaultLogRoller(), + }, Format: "{when}", }}, }}}, - {`log access.log { rotate { size 2 age 10 keep 3 } }`, false, []Rule{{ + {`log access.log { rotate_size 2 rotate_age 10 rotate_keep 3 }`, false, []Rule{{ PathScope: "/", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "access.log", Roller: &httpserver.LogRoller{ - MaxSize: 2, - MaxAge: 10, - MaxBackups: 3, - LocalTime: true, - }}, + Log: &httpserver.Logger{ + Output: "access.log", + Roller: &httpserver.LogRoller{ + MaxSize: 2, + MaxAge: 10, + MaxBackups: 3, + LocalTime: true, + }}, Format: DefaultLogFormat, }}, }}}, @@ -152,10 +192,16 @@ func TestLogParse(t *testing.T) { log / log.txt {when}`, false, []Rule{{ PathScope: "/", Entries: []*Entry{{ - Log: &httpserver.Logger{Output: "stdout"}, + Log: &httpserver.Logger{ + Output: "stdout", + Roller: httpserver.DefaultLogRoller(), + }, Format: "{host}", }, { - Log: &httpserver.Logger{Output: "log.txt"}, + Log: &httpserver.Logger{ + Output: "log.txt", + Roller: httpserver.DefaultLogRoller(), + }, Format: "{when}", }}, }}}, From 18edf5864e75b77c04242d54c841adcd759f1852 Mon Sep 17 00:00:00 2001 From: Nathan Caza Date: Fri, 10 Feb 2017 21:02:00 -0600 Subject: [PATCH 08/15] add fix from golang/go --- caddyhttp/staticfiles/fileserver.go | 35 ++++++++++++++++++++++++ caddyhttp/staticfiles/fileserver_test.go | 5 ++++ 2 files changed, 40 insertions(+) diff --git a/caddyhttp/staticfiles/fileserver.go b/caddyhttp/staticfiles/fileserver.go index be4da60a..f6ca23ab 100644 --- a/caddyhttp/staticfiles/fileserver.go +++ b/caddyhttp/staticfiles/fileserver.go @@ -56,6 +56,9 @@ func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name stri f, err := fs.Root.Open(name) if err != nil { + // TODO: remove when http.Dir handles this + // Go issue #18984 + err = mapFSRootOpenErr(err) if os.IsNotExist(err) { return http.StatusNotFound, nil } else if os.IsPermission(err) { @@ -230,3 +233,35 @@ var staticEncodingPriority = []string{ "br", "gzip", } + +// mapFSRootOpenErr maps the provided non-nil error +// to a possibly better non-nil error. In particular, it turns OS-specific errors +// about opening files in non-directories into os.ErrNotExist. +// +// TODO: remove when http.Dir handles this +// Go issue #18984 +func mapFSRootOpenErr(originalErr error) error { + if os.IsNotExist(originalErr) || os.IsPermission(originalErr) { + return originalErr + } + + perr, ok := originalErr.(*os.PathError) + if !ok { + return originalErr + } + name := perr.Path + parts := strings.Split(name, string(filepath.Separator)) + for i := range parts { + if parts[i] == "" { + continue + } + fi, err := os.Stat(strings.Join(parts[:i+1], string(filepath.Separator))) + if err != nil { + return originalErr + } + if !fi.IsDir() { + return os.ErrNotExist + } + } + return originalErr +} diff --git a/caddyhttp/staticfiles/fileserver_test.go b/caddyhttp/staticfiles/fileserver_test.go index 346a1d15..6753ecac 100644 --- a/caddyhttp/staticfiles/fileserver_test.go +++ b/caddyhttp/staticfiles/fileserver_test.go @@ -178,6 +178,11 @@ func TestServeHTTP(t *testing.T) { expectedBodyContent: testFiles[filepath.Join("webroot", "sub", "brotli.html.br")], expectedEtag: `W/"1e240-e"`, }, + // Test 20 - treat existing file as a directory. + { + url: "https://foo/file1.html/other", + expectedStatus: http.StatusNotFound, + }, } for i, test := range tests { From b210101f4597c7843f2313d6308745aab80f1598 Mon Sep 17 00:00:00 2001 From: Kurt Date: Sat, 11 Feb 2017 09:38:25 -0500 Subject: [PATCH 09/15] Register cgi plugin --- caddyhttp/httpserver/plugin.go | 1 + 1 file changed, 1 insertion(+) diff --git a/caddyhttp/httpserver/plugin.go b/caddyhttp/httpserver/plugin.go index c1bd6a57..a8c1295d 100644 --- a/caddyhttp/httpserver/plugin.go +++ b/caddyhttp/httpserver/plugin.go @@ -454,6 +454,7 @@ var directives = []string{ "prometheus", // github.com/miekg/caddy-prometheus "proxy", "fastcgi", + "cgi", // github.com/jung-kurt/caddy-cgi "websocket", "filemanager", // github.com/hacdias/caddy-filemanager "markdown", From 5072d70f3810acf5e57f93a43a02b030a0616102 Mon Sep 17 00:00:00 2001 From: Toby Allen Date: Mon, 13 Feb 2017 21:22:19 +0000 Subject: [PATCH 10/15] Fix for #1388 dont attempt to hide Caddyfile if non existant --- caddyhttp/httpserver/plugin.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/caddyhttp/httpserver/plugin.go b/caddyhttp/httpserver/plugin.go index c1bd6a57..9401f222 100644 --- a/caddyhttp/httpserver/plugin.go +++ b/caddyhttp/httpserver/plugin.go @@ -55,6 +55,12 @@ func init() { func hideCaddyfile(cctx caddy.Context) error { ctx := cctx.(*httpContext) for _, cfg := range ctx.siteConfigs { + + // if no Caddyfile exists exit. + if cfg.originCaddyfile == "" { + return nil + } + absRoot, err := filepath.Abs(cfg.Root) if err != nil { return err From 1a7612071acdfe746a22abf6923358385c50537a Mon Sep 17 00:00:00 2001 From: Toby Allen Date: Mon, 13 Feb 2017 21:28:19 +0000 Subject: [PATCH 11/15] remove whitespace --- caddyhttp/httpserver/plugin.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/caddyhttp/httpserver/plugin.go b/caddyhttp/httpserver/plugin.go index 9401f222..9faf5ac3 100644 --- a/caddyhttp/httpserver/plugin.go +++ b/caddyhttp/httpserver/plugin.go @@ -55,12 +55,10 @@ func init() { func hideCaddyfile(cctx caddy.Context) error { ctx := cctx.(*httpContext) for _, cfg := range ctx.siteConfigs { - // if no Caddyfile exists exit. if cfg.originCaddyfile == "" { return nil } - absRoot, err := filepath.Abs(cfg.Root) if err != nil { return err From b650a26727c23310ebd728dc2142274f07ce3108 Mon Sep 17 00:00:00 2001 From: Rohan Pai Date: Tue, 14 Feb 2017 14:32:12 -0800 Subject: [PATCH 12/15] Added Sourcegraph badge to README --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 67287858..38292351 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,7 @@ [![community](https://img.shields.io/badge/community-forum-ff69b4.svg?style=flat-square)](https://forum.caddyserver.com) [![twitter](https://img.shields.io/badge/twitter-@caddyserver-55acee.svg?style=flat-square)](https://twitter.com/caddyserver) [![Documentation](https://img.shields.io/badge/godoc-reference-blue.svg?style=flat-square)](https://godoc.org/github.com/mholt/caddy) [![Linux Build Status](https://img.shields.io/travis/mholt/caddy.svg?style=flat-square&label=linux+build)](https://travis-ci.org/mholt/caddy) [![Windows Build Status](https://img.shields.io/appveyor/ci/mholt/caddy.svg?style=flat-square&label=windows+build)](https://ci.appveyor.com/project/mholt/caddy) [![Go Report Card](https://goreportcard.com/badge/github.com/mholt/caddy?style=flat-square)](https://goreportcard.com/report/mholt/caddy) +[![Sourcegraph Badge](https://sourcegraph.com/github.com/mholt/caddy/-/badge.svg)](https://sourcegraph.com/github.com/mholt/caddy?badge) Caddy is a general-purpose web server for Windows, Mac, Linux, BSD, and From 463c9d9dd250344fd20069449d3bab82cffb292a Mon Sep 17 00:00:00 2001 From: Augusto Roman Date: Wed, 15 Feb 2017 08:09:42 -0700 Subject: [PATCH 13/15] Fix data race for max connection limiting in proxy directive. (#1438) * Fix data race for max connection limiting in proxy directive. The Conns and Unhealthy fields are updated concurrently across all active requests. Because of this, they must use atomic operations for reads and writes. Prior to this change, Conns was incremented atomically, but read unsafely. Unhealthly was updated & read unsafely. The new test TestReverseProxyMaxConnLimit exposes this race when run with -race. Switching to atomic operations makes the race detector happy. * oops, remove leftover dead code. --- caddyhttp/proxy/policy_test.go | 14 +++---- caddyhttp/proxy/proxy.go | 11 +++-- caddyhttp/proxy/proxy_test.go | 69 ++++++++++++++++++++++++++++++++ caddyhttp/proxy/upstream.go | 17 +++++--- caddyhttp/proxy/upstream_test.go | 12 +++--- 5 files changed, 102 insertions(+), 21 deletions(-) diff --git a/caddyhttp/proxy/policy_test.go b/caddyhttp/proxy/policy_test.go index 2a8dfe61..9b277197 100644 --- a/caddyhttp/proxy/policy_test.go +++ b/caddyhttp/proxy/policy_test.go @@ -60,13 +60,13 @@ func TestRoundRobinPolicy(t *testing.T) { t.Error("Expected third round robin host to be first host in the pool.") } // mark host as down - pool[1].Unhealthy = true + pool[1].Unhealthy = 1 h = rrPolicy.Select(pool, request) if h != pool[2] { t.Error("Expected to skip down host.") } // mark host as up - pool[1].Unhealthy = false + pool[1].Unhealthy = 0 h = rrPolicy.Select(pool, request) if h == pool[2] { @@ -161,7 +161,7 @@ func TestIPHashPolicy(t *testing.T) { // we should get a healthy host if the original host is unhealthy and a // healthy host is available request.RemoteAddr = "172.0.0.1" - pool[1].Unhealthy = true + pool[1].Unhealthy = 1 h = ipHash.Select(pool, request) if h != pool[2] { t.Error("Expected ip hash policy host to be the third host.") @@ -172,10 +172,10 @@ func TestIPHashPolicy(t *testing.T) { if h != pool[2] { t.Error("Expected ip hash policy host to be the third host.") } - pool[1].Unhealthy = false + pool[1].Unhealthy = 0 request.RemoteAddr = "172.0.0.3" - pool[2].Unhealthy = true + pool[2].Unhealthy = 1 h = ipHash.Select(pool, request) if h != pool[0] { t.Error("Expected ip hash policy host to be the first host.") @@ -219,8 +219,8 @@ func TestIPHashPolicy(t *testing.T) { } // We should get nil when there are no healthy hosts - pool[0].Unhealthy = true - pool[1].Unhealthy = true + pool[0].Unhealthy = 1 + pool[1].Unhealthy = 1 h = ipHash.Select(pool, request) if h != nil { t.Error("Expected ip hash policy host to be nil.") diff --git a/caddyhttp/proxy/proxy.go b/caddyhttp/proxy/proxy.go index c0c2bb4b..c2d05b49 100644 --- a/caddyhttp/proxy/proxy.go +++ b/caddyhttp/proxy/proxy.go @@ -49,6 +49,8 @@ type UpstreamHostDownFunc func(*UpstreamHost) bool // UpstreamHost represents a single proxy upstream type UpstreamHost struct { + // This field is read & written to concurrently, so all access must use + // atomic operations. Conns int64 // must be first field to be 64-bit aligned on 32-bit systems MaxConns int64 Name string // hostname of this upstream host @@ -59,7 +61,10 @@ type UpstreamHost struct { WithoutPathPrefix string ReverseProxy *ReverseProxy Fails int32 - Unhealthy bool + // This is an int32 so that we can use atomic operations to do concurrent + // reads & writes to this value. The default value of 0 indicates that it + // is healthy and any non-zero value indicates unhealthy. + Unhealthy int32 } // Down checks whether the upstream host is down or not. @@ -68,14 +73,14 @@ type UpstreamHost struct { func (uh *UpstreamHost) Down() bool { if uh.CheckDown == nil { // Default settings - return uh.Unhealthy || uh.Fails > 0 + return atomic.LoadInt32(&uh.Unhealthy) != 0 || atomic.LoadInt32(&uh.Fails) > 0 } return uh.CheckDown(uh) } // Full checks whether the upstream host has reached its maximum connections func (uh *UpstreamHost) Full() bool { - return uh.MaxConns > 0 && uh.Conns >= uh.MaxConns + return uh.MaxConns > 0 && atomic.LoadInt64(&uh.Conns) >= uh.MaxConns } // Available checks whether the upstream host is available for proxying to diff --git a/caddyhttp/proxy/proxy_test.go b/caddyhttp/proxy/proxy_test.go index 90753ab3..380094e0 100644 --- a/caddyhttp/proxy/proxy_test.go +++ b/caddyhttp/proxy/proxy_test.go @@ -19,6 +19,7 @@ import ( "reflect" "runtime" "strings" + "sync" "sync/atomic" "testing" "time" @@ -143,6 +144,74 @@ func TestReverseProxyInsecureSkipVerify(t *testing.T) { } } +// This test will fail when using the race detector without atomic reads & +// writes of UpstreamHost.Conns and UpstreamHost.Unhealthy. +func TestReverseProxyMaxConnLimit(t *testing.T) { + log.SetOutput(ioutil.Discard) + defer log.SetOutput(os.Stderr) + + const MaxTestConns = 2 + connReceived := make(chan bool, MaxTestConns) + connContinue := make(chan bool) + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + connReceived <- true + <-connContinue + })) + defer backend.Close() + + su, err := NewStaticUpstreams(caddyfile.NewDispenser("Testfile", strings.NewReader(` + proxy / `+backend.URL+` { + max_conns `+fmt.Sprint(MaxTestConns)+` + } + `))) + if err != nil { + t.Fatal(err) + } + + // set up proxy + p := &Proxy{ + Next: httpserver.EmptyNext, // prevents panic in some cases when test fails + Upstreams: su, + } + + var jobs sync.WaitGroup + + for i := 0; i < MaxTestConns; i++ { + jobs.Add(1) + go func(i int) { + defer jobs.Done() + w := httptest.NewRecorder() + code, err := p.ServeHTTP(w, httptest.NewRequest("GET", "/", nil)) + if err != nil { + t.Errorf("Request %d failed: %v", i, err) + } else if code != 0 { + t.Errorf("Bad return code for request %d: %d", i, code) + } else if w.Code != 200 { + t.Errorf("Bad statuc code for request %d: %d", i, w.Code) + } + }(i) + } + // Wait for all the requests to hit the backend. + for i := 0; i < MaxTestConns; i++ { + <-connReceived + } + + // Now we should have MaxTestConns requests connected and sitting on the backend + // server. Verify that the next request is rejected. + w := httptest.NewRecorder() + code, err := p.ServeHTTP(w, httptest.NewRequest("GET", "/", nil)) + if code != http.StatusBadGateway { + t.Errorf("Expected request to be rejected, but got: %d [%v]\nStatus code: %d", + code, err, w.Code) + } + + // Now let all the requests complete and verify the status codes for those: + close(connContinue) + + // Wait for the initial requests to finish and check their results. + jobs.Wait() +} + func TestWebSocketReverseProxyNonHijackerPanic(t *testing.T) { // Capture the expected panic defer func() { diff --git a/caddyhttp/proxy/upstream.go b/caddyhttp/proxy/upstream.go index 5742eff0..0e831124 100644 --- a/caddyhttp/proxy/upstream.go +++ b/caddyhttp/proxy/upstream.go @@ -9,6 +9,7 @@ import ( "path" "strconv" "strings" + "sync/atomic" "time" "github.com/mholt/caddy/caddyfile" @@ -128,15 +129,15 @@ func (u *staticUpstream) NewHost(host string) (*UpstreamHost, error) { Conns: 0, Fails: 0, FailTimeout: u.FailTimeout, - Unhealthy: false, + Unhealthy: 0, UpstreamHeaders: u.upstreamHeaders, DownstreamHeaders: u.downstreamHeaders, CheckDown: func(u *staticUpstream) UpstreamHostDownFunc { return func(uh *UpstreamHost) bool { - if uh.Unhealthy { + if atomic.LoadInt32(&uh.Unhealthy) != 0 { return true } - if uh.Fails >= u.MaxFails { + if atomic.LoadInt32(&uh.Fails) >= u.MaxFails { return true } return false @@ -355,12 +356,18 @@ func parseBlock(c *caddyfile.Dispenser, u *staticUpstream) error { func (u *staticUpstream) healthCheck() { for _, host := range u.Hosts { hostURL := host.Name + u.HealthCheck.Path + var unhealthy bool if r, err := u.HealthCheck.Client.Get(hostURL); err == nil { io.Copy(ioutil.Discard, r.Body) r.Body.Close() - host.Unhealthy = r.StatusCode < 200 || r.StatusCode >= 400 + unhealthy = r.StatusCode < 200 || r.StatusCode >= 400 } else { - host.Unhealthy = true + unhealthy = true + } + if unhealthy { + atomic.StoreInt32(&host.Unhealthy, 1) + } else { + atomic.StoreInt32(&host.Unhealthy, 0) } } } diff --git a/caddyhttp/proxy/upstream_test.go b/caddyhttp/proxy/upstream_test.go index 2d7828eb..1163fffe 100644 --- a/caddyhttp/proxy/upstream_test.go +++ b/caddyhttp/proxy/upstream_test.go @@ -36,12 +36,12 @@ func TestNewHost(t *testing.T) { t.Error("Expected new host not to be down.") } // mark Unhealthy - uh.Unhealthy = true + uh.Unhealthy = 1 if !uh.CheckDown(uh) { t.Error("Expected unhealthy host to be down.") } // mark with Fails - uh.Unhealthy = false + uh.Unhealthy = 0 uh.Fails = 1 if !uh.CheckDown(uh) { t.Error("Expected failed host to be down.") @@ -74,13 +74,13 @@ func TestSelect(t *testing.T) { MaxFails: 1, } r, _ := http.NewRequest("GET", "/", nil) - upstream.Hosts[0].Unhealthy = true - upstream.Hosts[1].Unhealthy = true - upstream.Hosts[2].Unhealthy = true + upstream.Hosts[0].Unhealthy = 1 + upstream.Hosts[1].Unhealthy = 1 + upstream.Hosts[2].Unhealthy = 1 if h := upstream.Select(r); h != nil { t.Error("Expected select to return nil as all host are down") } - upstream.Hosts[2].Unhealthy = false + upstream.Hosts[2].Unhealthy = 0 if h := upstream.Select(r); h == nil { t.Error("Expected select to not return nil") } From dc3efc939c71d2acf422dafeee6562e4bbd92114 Mon Sep 17 00:00:00 2001 From: Augusto Roman Date: Wed, 15 Feb 2017 21:59:24 -0700 Subject: [PATCH 14/15] Add request placeholder support for querying request cookies. (#1392) * Add request placeholder support for querying request cookies. This adds the ability to query the request cookies for placeholders using the syntax "@cookiename". For example, this would allow rewriting based on a cookie: rewrite { if @version is 'dev' to /dev/index.html } * Switch cookie special char from @ to : * Switch special char for cookies from : to ~ --- caddyhttp/httpserver/replacer.go | 7 +++++++ caddyhttp/httpserver/replacer_test.go | 8 +++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/caddyhttp/httpserver/replacer.go b/caddyhttp/httpserver/replacer.go index a3dc258a..22e8aa8f 100644 --- a/caddyhttp/httpserver/replacer.go +++ b/caddyhttp/httpserver/replacer.go @@ -198,6 +198,13 @@ func (r *replacer) getSubstitution(key string) string { } } } + // next check for cookies + if key[1] == '~' { + name := key[2 : len(key)-1] + if cookie, err := r.request.Cookie(name); err == nil { + return cookie.Value + } + } // search default replacements in the end switch key { diff --git a/caddyhttp/httpserver/replacer_test.go b/caddyhttp/httpserver/replacer_test.go index e03aed81..52608c57 100644 --- a/caddyhttp/httpserver/replacer_test.go +++ b/caddyhttp/httpserver/replacer_test.go @@ -47,6 +47,7 @@ func TestReplace(t *testing.T) { repl := NewReplacer(request, recordRequest, "-") // add some headers after creating replacer request.Header.Set("CustomAdd", "caddy") + request.Header.Set("Cookie", "foo=bar; taste=delicious") hostname, err := os.Hostname() if err != nil { @@ -72,12 +73,17 @@ func TestReplace(t *testing.T) { {"{when_iso}", "2006-01-02T15:04:12Z"}, {"The Custom header is {>Custom}.", "The Custom header is foobarbaz."}, {"The CustomAdd header is {>CustomAdd}.", "The CustomAdd header is caddy."}, - {"The request is {request}.", "The request is POST / HTTP/1.1\\r\\nHost: localhost\\r\\nCustom: foobarbaz\\r\\nCustomadd: caddy\\r\\nShorterval: 1\\r\\n\\r\\n."}, + {"The request is {request}.", "The request is POST / HTTP/1.1\\r\\nHost: localhost\\r\\n" + + "Cookie: foo=bar; taste=delicious\\r\\nCustom: foobarbaz\\r\\nCustomadd: caddy\\r\\n" + + "Shorterval: 1\\r\\n\\r\\n."}, {"The cUsToM header is {>cUsToM}...", "The cUsToM header is foobarbaz..."}, {"The Non-Existent header is {>Non-Existent}.", "The Non-Existent header is -."}, {"Bad {host placeholder...", "Bad {host placeholder..."}, {"Bad {>Custom placeholder", "Bad {>Custom placeholder"}, {"Bad {>Custom placeholder {>ShorterVal}", "Bad -"}, + {"Bad {}", "Bad -"}, + {"Cookies are {~taste}", "Cookies are delicious"}, + {"Missing cookie is {~missing}", "Missing cookie is -"}, } for _, c := range testCases { From 55bded68c2ef23a85f643190f2444690a18f5d5d Mon Sep 17 00:00:00 2001 From: Alex Harrington Date: Thu, 16 Feb 2017 05:02:51 +0000 Subject: [PATCH 15/15] fixing panic when root is symlink (#1429) * fixing panic when root is symlink checking root path is a symlink before os.Stat which panics * fixing formatting * adding test to verify symlink root path check * fixing typo --- caddyhttp/root/root.go | 27 ++++++++++++++++---------- caddyhttp/root/root_test.go | 38 ++++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 11 deletions(-) diff --git a/caddyhttp/root/root.go b/caddyhttp/root/root.go index b3dded1c..f9f11bfa 100644 --- a/caddyhttp/root/root.go +++ b/caddyhttp/root/root.go @@ -28,16 +28,23 @@ func setupRoot(c *caddy.Controller) error { return c.ArgErr() } } - - // Check if root path exists - _, err := os.Stat(config.Root) - if err != nil { - if os.IsNotExist(err) { - // Allow this, because the folder might appear later. - // But make sure the user knows! - log.Printf("[WARNING] Root path does not exist: %s", config.Root) - } else { - return c.Errf("Unable to access root path '%s': %v", config.Root, err) + //first check that the path is not a symlink, os.Stat panics when this is true + info, _ := os.Lstat(config.Root) + if info != nil && info.Mode()&os.ModeSymlink == os.ModeSymlink { + //just print out info, delegate responsibility for symlink validity to + //underlying Go framework, no need to test / verify twice + log.Printf("[INFO] Root path is symlink: %s", config.Root) + } else { + // Check if root path exists + _, err := os.Stat(config.Root) + if err != nil { + if os.IsNotExist(err) { + // Allow this, because the folder might appear later. + // But make sure the user knows! + log.Printf("[WARNING] Root path does not exist: %s", config.Root) + } else { + return c.Errf("Unable to access root path '%s': %v", config.Root, err) + } } } diff --git a/caddyhttp/root/root_test.go b/caddyhttp/root/root_test.go index daeb9eea..e4ad8841 100644 --- a/caddyhttp/root/root_test.go +++ b/caddyhttp/root/root_test.go @@ -91,7 +91,7 @@ func TestRoot(t *testing.T) { } } -// getTempDirPath returnes the path to the system temp directory. If it does not exists - an error is returned. +// getTempDirPath returns the path to the system temp directory. If it does not exists - an error is returned. func getTempDirPath() (string, error) { tempDir := os.TempDir() _, err := os.Stat(tempDir) @@ -104,3 +104,39 @@ func getTempDirPath() (string, error) { func getInaccessiblePath(file string) string { return filepath.Join("C:", "file\x00name") // null byte in filename is not allowed on Windows AND unix } + +func TestSymlinkRoot(t *testing.T) { + origDir, err := ioutil.TempDir("", "root_test") + if err != nil { + t.Fatalf("BeforeTest: Failed to create temp dir for testing! Error was: %v", err) + } + defer func() { + os.Remove(origDir) + }() + + tempDir, err := getTempDirPath() + if err != nil { + t.Fatalf("BeforeTest: Failed to find an existing directory for testing! Error was: %v", err) + } + symlinkDir := filepath.Join(tempDir, "symlink") + + err = os.Symlink(origDir, symlinkDir) + if err != nil { + if strings.Contains(err.Error(), "A required privilege is not held by the client") { + t.Skip("BeforeTest: A required privilege is not held by the client and is required to create a symlink to run this test.") + } + t.Fatalf("BeforeTest: Cannot create symlink! Error was: %v", err) + } + defer func() { + os.Remove(symlinkDir) + }() + + input := fmt.Sprintf(`root %s`, symlinkDir) + c := caddy.NewTestController("http", input) + err = setupRoot(c) + _ = httpserver.GetConfig(c) + + if err != nil { + t.Errorf("Test Symlink Root: Expected no error but found one for input %s. Error was: %v", input, err) + } +}