- Move log rotation setup into separate function

- Improve error handling - Add --enable-log-rotation flag - Add default config
etcd-io · May 7, 2021 · 58ef1e3 · 58ef1e3
1 parent 552fbbd
commit 58ef1e3
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 55 deletions.
diff --git a/server/embed/config.go b/server/embed/config.go
@@ -63,11 +63,19 @@ const (
 	DefaultListenPeerURLs   = "http://localhost:2380"
 	DefaultListenClientURLs = "http://localhost:2379"
 
-	DefaultLogOutput       = "default"
-	JournalLogOutput       = "systemd/journal"
-	StdErrLogOutput        = "stderr"
-	StdOutLogOutput        = "stdout"
-	DefaultLogRotateConfig = ""
+	DefaultLogOutput = "default"
+	JournalLogOutput = "systemd/journal"
+	StdErrLogOutput  = "stderr"
+	StdOutLogOutput  = "stdout"
+
+	// DefaultLogRotationConfig is the default configuration used for log rotation.
+	// Log rotation is disabled by default.
+	// MaxSize    = 100 // MB
+	// MaxAge     = 0 // none
+	// MaxBackups = 0 // no limit
+	// LocalTime  = false // UTC by default
+	// Compress   = false
+	DefaultLogRotationConfig = `{"maxsize": 100, "maxage": 0, "maxbackups": 0, "localtime": false, "compress": false}`
 
 	// DefaultStrictReconfigCheck is the default value for "--strict-reconfig-check" flag.
 	// It's enabled by default.
@@ -322,8 +330,10 @@ type Config struct {
 	//  - file path to append server logs to.
 	// It can be multiple when "Logger" is zap.
 	LogOutputs []string `json:"log-outputs"`
-	// LogRotateConfigJSON is a passthrough allowing lumperjack log rotation config to be applied directly.
-	LogRotateConfigJSON string `json:"log-rotate-config-json"`
+	// EnableLogRotation enables log rotation of a single LogOutputs file target.
+	EnableLogRotation bool `json:"enable-log-rotation"`
+	// LogRotateConfigJSON is a passthrough allowing a log rotation JSON config to be passed directly.
+	LogRotationConfigJSON string `json:"log-rotation-config-json"`
 	// ZapLoggerBuilder is used to build the zap logger.
 	ZapLoggerBuilder func(*Config) error
 
@@ -440,13 +450,14 @@ func NewConfig() *Config {
 
 		PreVote: true,
 
-		loggerMu:            new(sync.RWMutex),
-		logger:              nil,
-		Logger:              "zap",
-		LogOutputs:          []string{DefaultLogOutput},
-		LogLevel:            logutil.DefaultLogLevel,
-		LogRotateConfigJSON: DefaultLogRotateConfig,
-		EnableGRPCGateway:   true,
+		loggerMu:              new(sync.RWMutex),
+		logger:                nil,
+		Logger:                "zap",
+		LogOutputs:            []string{DefaultLogOutput},
+		LogLevel:              logutil.DefaultLogLevel,
+		EnableLogRotation:     false,
+		LogRotationConfigJSON: DefaultLogRotationConfig,
+		EnableGRPCGateway:     true,
 
 		ExperimentalDowngradeCheckTime: DefaultDowngradeCheckTime,
 		ExperimentalMemoryMlock:        false,

diff --git a/server/embed/config_logging.go b/server/embed/config_logging.go
@@ -17,6 +17,7 @@ package embed
 import (
 	"crypto/tls"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/url"
@@ -45,6 +46,7 @@ func (cfg *Config) setupLogging() error {
 	switch cfg.Logger {
 	case "capnslog": // removed in v3.5
 		return fmt.Errorf("--logger=capnslog is removed in v3.5")
+
 	case "zap":
 		if len(cfg.LogOutputs) == 0 {
 			cfg.LogOutputs = []string{DefaultLogOutput}
@@ -56,35 +58,12 @@ func (cfg *Config) setupLogging() error {
 				}
 			}
 		}
-
-		var logRotateConfig *lumberjack.Logger
-		if cfg.LogRotateConfigJSON != "" {
-			var logOutputFilePaths int
-			for _, v := range cfg.LogOutputs {
-				switch v {
-				case DefaultLogOutput, StdErrLogOutput, StdOutLogOutput:
-					continue
-				default:
-					logOutputFilePaths++
-				}
-			}
-			if len(cfg.LogOutputs) == 1 && logOutputFilePaths == 0 {
-				return ErrLogRotationInvalidLogOutput
-			}
-			if logOutputFilePaths > 1 {
-				return ErrLogRotationInvalidLogOutput
+		if cfg.EnableLogRotation {
+			if err := setupLogRotation(cfg.LogOutputs, cfg.LogRotationConfigJSON); err != nil {
+				return err
 			}
-
-			if err := json.Unmarshal([]byte(cfg.LogRotateConfigJSON), &logRotateConfig); err != nil {
-				return fmt.Errorf("failed to deserialize lumberjack config: %v", cfg.LogRotateConfigJSON)
-			}
-			zap.RegisterSink("rotate", func(u *url.URL) (zap.Sink, error) {
-				logRotateConfig.Filename = u.Path
-				return rotationSink{
-					Logger: logRotateConfig,
-				}, nil
-			})
 		}
+
 		outputPaths, errOutputPaths := make([]string, 0), make([]string, 0)
 		isJournal := false
 		for _, v := range cfg.LogOutputs {
@@ -106,8 +85,8 @@ func (cfg *Config) setupLogging() error {
 
 			default:
 				var path string
-				if logRotateConfig != nil {
-					// append rotate scheme to logs managed by lumberjack
+				if cfg.EnableLogRotation {
+					// append rotate scheme to logs managed by lumberjack log rotation
 					path = fmt.Sprintf("rotate:%s", v)
 				} else {
 					path = v
@@ -249,9 +228,47 @@ func (cfg *Config) SetupGlobalLoggers() {
 	}
 }
 
+type logRotationConfig struct {
+	*lumberjack.Logger
+}
+
 // Sync implements zap.Sink
-func (rotationSink) Sync() error { return nil }
+func (logRotationConfig) Sync() error { return nil }
 
-type rotationSink struct {
-	*lumberjack.Logger
+// setupLogRotation initializes log rotation for a single file path target.
+func setupLogRotation(logOutputs []string, logRotateConfigJSON string) error {
+	var logRotationConfig logRotationConfig
+	outputFilePaths := 0
+	for _, v := range logOutputs {
+		switch v {
+		case DefaultLogOutput, StdErrLogOutput, StdOutLogOutput:
+			continue
+		default:
+			outputFilePaths++
+		}
+	}
+	// log rotation requires file target
+	if len(logOutputs) == 1 && outputFilePaths == 0 {
+		return ErrLogRotationInvalidLogOutput
+	}
+	// support max 1 file target for log rotation
+	if outputFilePaths > 1 {
+		return ErrLogRotationInvalidLogOutput
+	}
+
+	if err := json.Unmarshal([]byte(logRotateConfigJSON), &logRotationConfig); err != nil {
+		var unmarshalTypeError *json.UnmarshalTypeError
+		var syntaxError *json.SyntaxError
+		switch {
+		case errors.As(err, &syntaxError):
+			return fmt.Errorf("improperly formatted log rotation config: %w", err)
+		case errors.As(err, &unmarshalTypeError):
+			return fmt.Errorf("invalid log rotation config: %w", err)
+		}
+	}
+	zap.RegisterSink("rotate", func(u *url.URL) (zap.Sink, error) {
+		logRotationConfig.Filename = u.Path
+		return &logRotationConfig, nil
+	})
+	return nil
 }
diff --git a/server/embed/config_test.go b/server/embed/config_test.go
@@ -15,6 +15,7 @@
 package embed
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -290,7 +291,7 @@ func TestPeerURLsMapAndTokenFromSRV(t *testing.T) {
 	}
 }
 
-func TestLogRotate(t *testing.T) {
+func TestLogRotation(t *testing.T) {
 	tests := []struct {
 		name              string
 		logOutputs        []string
@@ -313,7 +314,7 @@ func TestLogRotate(t *testing.T) {
 		{
 			name:              "multiple file targets",
 			logOutputs:        []string{"/tmp/path1", "/tmp/path2"},
-			logRotationConfig: `{"maxsize": 1}`,
+			logRotationConfig: DefaultLogRotationConfig,
 			wantErr:           true,
 			wantErrMsg:        ErrLogRotationInvalidLogOutput,
 		},
@@ -323,29 +324,42 @@ func TestLogRotate(t *testing.T) {
 			wantErr:           true,
 			wantErrMsg:        ErrLogRotationInvalidLogOutput,
 		},
+		{
+			name:              "default log rotation config",
+			logOutputs:        []string{"/tmp/path"},
+			logRotationConfig: DefaultLogRotationConfig,
+		},
 		{
 			name:              "invalid logger config",
 			logOutputs:        []string{"/tmp/path"},
 			logRotationConfig: `{"maxsize": true}`,
 			wantErr:           true,
-			wantErrMsg:        fmt.Errorf("failed to deserialize lumberjack config: {\"maxsize\": true}"),
+			wantErrMsg:        errors.New("invalid log rotation config: json: cannot unmarshal bool into Go struct field logRotationConfig.maxsize of type int"),
+		},
+		{
+			name:              "improperly formatted logger config",
+			logOutputs:        []string{"/tmp/path"},
+			logRotationConfig: `{"maxsize": true`,
+			wantErr:           true,
+			wantErrMsg:        errors.New("improperly formatted log rotation config: unexpected end of JSON input"),
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			cfg := NewConfig()
 			cfg.Logger = "zap"
 			cfg.LogOutputs = tt.logOutputs
-			cfg.LogRotateConfigJSON = tt.logRotationConfig
+			cfg.EnableLogRotation = true
+			cfg.LogRotationConfigJSON = tt.logRotationConfig
 			err := cfg.Validate()
 			if err != nil && !tt.wantErr {
 				t.Errorf("test %q, unexpected error %v", tt.name, err)
 			}
 			if err != nil && tt.wantErr && tt.wantErrMsg.Error() != err.Error() {
-				t.Errorf("test %q, expected error: %v, got: %v", tt.name, tt.wantErrMsg, err)
+				t.Errorf("test %q, expected error: %+v, got: %+v", tt.name, tt.wantErrMsg, err)
 			}
 			if err == nil && tt.wantErr {
-				t.Errorf("test %q, expected error %v", tt.name, tt.wantErrMsg)
+				t.Errorf("test %q, expected error, got nil", tt.name)
 			}
 		})
 	}

diff --git a/server/etcdmain/config.go b/server/etcdmain/config.go
@@ -236,7 +236,8 @@ func newConfig() *config {
 	fs.StringVar(&cfg.ec.Logger, "logger", "zap", "Currently only supports 'zap' for structured logging.")
 	fs.Var(flags.NewUniqueStringsValue(embed.DefaultLogOutput), "log-outputs", "Specify 'stdout' or 'stderr' to skip journald logging even when running under systemd, or list of comma separated output targets.")
 	fs.StringVar(&cfg.ec.LogLevel, "log-level", logutil.DefaultLogLevel, "Configures log level. Only supports debug, info, warn, error, panic, or fatal. Default 'info'.")
-	fs.StringVar(&cfg.ec.LogRotateConfigJSON, "log-rotate-config-json", "", "Configures log rotation with a JSON lumberjack logger config. Supports a single file output target.")
+	fs.BoolVar(&cfg.ec.EnableLogRotation, "enable-log-rotation", false, "Enable log rotation of a single log-outputs file target.")
+	fs.StringVar(&cfg.ec.LogRotationConfigJSON, "log-rotation-config-json", embed.DefaultLogRotationConfig, "Configures log rotation if enabled with a JSON logger config. Default: MaxSize=100, MaxAge=0, MaxBackups=0, LocalTime=false, Compress=false")
 
 	// version
 	fs.BoolVar(&cfg.printVersion, "version", false, "Print the version and exit.")

diff --git a/server/etcdmain/help.go b/server/etcdmain/help.go
@@ -188,8 +188,10 @@ Logging:
     Specify 'stdout' or 'stderr' to skip journald logging even when running under systemd, or list of comma separated output targets.
   --log-level 'info'
     Configures log level. Only supports debug, info, warn, error, panic, or fatal.
-  --log-rotate-config-json 'disabled'
-    Configures log rotation with a JSON lumberjack logger config. Supports a single file output target.
+  --enable-log-rotation 'false'
+    Enable log rotation of a single log-outputs file target.
+  --log-rotation-config-json '{"maxsize": 100, "maxage": 0, "maxbackups": 0, "localtime": false, "compress": false}'
+    Configures log rotation if enabled with a JSON logger config.".
 
 v2 Proxy (to be deprecated in v4):
   --proxy 'off'