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

@@ -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 {

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,22 +94,24 @@ func errorsParse(c *caddy.Controller) (*ErrorHandler, error) {
 		if c.Val() == "}" {
 			continue
 		}
+
+		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()
+			}
+		}
+
 		// Configuration may be in a block
-		hadBlock, err := optionalBlock()
+		err := optionalBlock()
 		if err != nil {
 			return handler, err
 		}
-
-		// 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()
-				}
-			}
-		}
 	}
 
 	return handler, nil

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)
 		}
 	}
 }

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
 }
 
+// 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(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
-		}
+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:    size,
-		MaxAge:     age,
-		MaxBackups: keep,
+		MaxSize:    defaultRotateSize,
+		MaxAge:     defaultRotateAge,
+		MaxBackups: defaultRotateKeep,
 		LocalTime:  true,
-	}, nil
+	}
 }
 
+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)

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{

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}",
 			}},
 		}}},