From ce7d3db1be57b04975922a3988c02df8f993293f Mon Sep 17 00:00:00 2001 From: "Julian V. Modesto" Date: Wed, 8 Feb 2017 11:23:33 -0500 Subject: [PATCH] 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}", }}, }}},