TUN-3607: Set up single-file logger with zerolog

cmd/cloudflared/tunnel/subcommand_context_test.go

@@ -271,7 +271,7 @@ func (d *deleteMockTunnelStore) CleanupConnections(tunnelID uuid.UUID) error {
 func Test_subcommandContext_Delete(t *testing.T) {
 	type fields struct {
 		c                 *cli.Context
-		logger            logger.Service
+		log               *zerolog.Logger
 		isUIEnabled       bool
 		fs                fileSystem
 		tunnelstoreClient *deleteMockTunnelStore
@@ -283,8 +283,7 @@ func Test_subcommandContext_Delete(t *testing.T) {
 	newCertPath := "new_cert.json"
 	tunnelID1 := uuid.MustParse("df5ed608-b8b4-4109-89f3-9f2cf199df64")
 	tunnelID2 := uuid.MustParse("af5ed608-b8b4-4109-89f3-9f2cf199df64")
-	logger, err := logger.New()
+	log := zerolog.Nop()
-	require.NoError(t, err)
 
 	var tests = []struct {
 		name    string
@@ -296,7 +295,7 @@ func Test_subcommandContext_Delete(t *testing.T) {
 		{
 			name: "clean up continues if credentials are not found",
 			fields: fields{
-				logger: logger,
+				log: &log,
 				fs: mockFileSystem{
 					rf: func(filePath string) ([]byte, error) {
 						return nil, errors.New("file not found")
@@ -307,7 +306,7 @@ func Test_subcommandContext_Delete(t *testing.T) {
 					flagSet := flag.NewFlagSet("test0", flag.PanicOnError)
 					flagSet.String(CredFileFlag, newCertPath, "")
 					c := cli.NewContext(cli.NewApp(), flagSet, nil)
-					err = c.Set(CredFileFlag, newCertPath)
+					_ = c.Set(CredFileFlag, newCertPath)
 					return c
 				}(),
 				tunnelstoreClient: newDeleteMockTunnelStore(
@@ -331,7 +330,7 @@ func Test_subcommandContext_Delete(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			sc := &subcommandContext{
 				c:                 tt.fields.c,
-				logger:            tt.fields.logger,
+				log:               tt.fields.log,
 				isUIEnabled:       tt.fields.isUIEnabled,
 				fs:                tt.fields.fs,
 				tunnelstoreClient: tt.fields.tunnelstoreClient,

logger/configuration.go

@@ -1,5 +1,7 @@
 package logger
 
+import "path/filepath"
+
 var defaultConfig = createDefaultConfig()
 
 // Logging configuration
@@ -16,11 +18,16 @@ type ConsoleConfig struct {
 }
 
 type FileConfig struct {
-	Filepath string
+	Dirname  string
+	Filename string
+}
+
+func (fc *FileConfig) Fullpath() string {
+	return filepath.Join(fc.Dirname, fc.Filename)
 }
 
 type RollingConfig struct {
-	Directory string
+	Dirname  string
 	Filename string
 
 	maxSize    int // megabytes
@@ -34,18 +41,19 @@ func createDefaultConfig() Config {
 	const RollingMaxSize = 1    // Mb
 	const RollingMaxBackups = 5 // files
 	const RollingMaxAge = 0     // Keep forever
-	const rollingLogFilename = "cloudflared.log"
+	const defaultLogFilename = "cloudflared.log"
 
 	return Config{
 		ConsoleConfig: &ConsoleConfig{
 			noColor: false,
 		},
 		FileConfig: &FileConfig{
-			Filepath: "",
+			Dirname:  "",
+			Filename: defaultLogFilename,
 		},
 		RollingConfig: &RollingConfig{
-			Directory:  "",
+			Dirname:    "",
-			Filename:   rollingLogFilename,
+			Filename:   defaultLogFilename,
 			maxSize:    RollingMaxSize,
 			maxBackups: RollingMaxBackups,
 			maxAge:     RollingMaxAge,
@@ -57,7 +65,7 @@ func createDefaultConfig() Config {
 func CreateConfig(
 	minLevel string,
 	disableTerminal bool,
-	rollingLogPath, rollingLogFilename, nonRollingLogFilePath string,
+	rollingLogPath, nonRollingLogFilePath string,
 ) *Config {
 	var console *ConsoleConfig
 	if !disableTerminal {
@@ -65,13 +73,11 @@ func CreateConfig(
 	}
 
 	var file *FileConfig
-	if nonRollingLogFilePath != "" {
-		file = createFileConfig(nonRollingLogFilePath)
-	}
-
 	var rolling *RollingConfig
 	if rollingLogPath != "" {
-		rolling = createRollingConfig(rollingLogPath, rollingLogFilename)
+		rolling = createRollingConfig(rollingLogPath)
+	} else if nonRollingLogFilePath != "" {
+		file = createFileConfig(nonRollingLogFilePath)
 	}
 
 	if minLevel == "" {
@@ -93,24 +99,27 @@ func createConsoleConfig() *ConsoleConfig {
 	}
 }
 
-func createFileConfig(filepath string) *FileConfig {
+func createFileConfig(fullpath string) *FileConfig {
-	if filepath == "" {
+	if fullpath == "" {
-		filepath = defaultConfig.FileConfig.Filepath
+		return defaultConfig.FileConfig
 	}
 
+	dirname, filename := filepath.Split(fullpath)
+
 	return &FileConfig{
-		Filepath: filepath,
+		Dirname:  dirname,
+		Filename: filename,
 	}
 }
 
-func createRollingConfig(directory, filename string) *RollingConfig {
+func createRollingConfig(directory string) *RollingConfig {
 	if directory == "" {
-		directory = defaultConfig.RollingConfig.Directory
+		directory = defaultConfig.RollingConfig.Dirname
 	}
 
 	return &RollingConfig{
-		Directory:  directory,
+		Dirname:    directory,
-		Filename:   filename,
+		Filename:   defaultConfig.RollingConfig.Filename,
 		maxSize:    defaultConfig.RollingConfig.maxSize,
 		maxBackups: defaultConfig.RollingConfig.maxBackups,
 		maxAge:     defaultConfig.RollingConfig.maxAge,

logger/create.go

@@ -1,6 +1,7 @@
 package logger
 
 import (
+	"fmt"
 	"io"
 	"os"
 	"path"
@@ -22,6 +23,9 @@ const (
 
 	LogSSHDirectoryFlag = "log-directory"
 	LogSSHLevelFlag     = "log-level"
+
+	dirPermMode  = 0744 // rwxr--r--
+	filePermMode = 0644 // rw-r--r--
 )
 
 func fallbackLogger(err error) *zerolog.Logger {
@@ -38,6 +42,15 @@ func newZerolog(loggerConfig *Config) *zerolog.Logger {
 		writers = append(writers, createConsoleLogger(*loggerConfig.ConsoleConfig))
 	}
 
+	if loggerConfig.FileConfig != nil {
+		fileLogger, err := createFileLogger(*loggerConfig.FileConfig)
+		if err != nil {
+			return fallbackLogger(err)
+		}
+
+		writers = append(writers, fileLogger)
+	}
+
 	if loggerConfig.RollingConfig != nil {
 		rollingLogger, err := createRollingLogger(*loggerConfig.RollingConfig)
 		if err != nil {
@@ -84,11 +97,14 @@ func createFromContext(
 		logLevel,
 		disableTerminal,
 		logDirectory,
-		defaultConfig.RollingConfig.Filename,
 		logFile,
 	)
 
-	return newZerolog(loggerConfig)
+	log := newZerolog(loggerConfig)
+	if incompatibleFlagsSet := logFile != "" && logDirectory != ""; incompatibleFlagsSet {
+		log.Error().Msgf("Your config includes values for both %s and %s, but they are incompatible. %s takes precedence.", LogFileFlag, logDirectoryFlagName, LogFileFlag)
+	}
+	return log
 }
 
 func Create(loggerConfig *Config) *zerolog.Logger {
@@ -106,13 +122,53 @@ func createConsoleLogger(config ConsoleConfig) io.Writer {
 	}
 }
 
+func createFileLogger(config FileConfig) (io.Writer, error) {
+	var logFile io.Writer
+	fullpath := config.Fullpath()
+
+	// Try to open the existing file
+	logFile, err := os.OpenFile(fullpath, os.O_APPEND|os.O_WRONLY, filePermMode)
+	if err != nil {
+		// If the existing file wasn't found, or couldn't be opened, just ignore
+		// it and recreate a new one.
+		logFile, err = createLogFile(config)
+		// If creating a new logfile fails, then we have no choice but to error out.
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	fileLogger := zerolog.New(logFile).With().Logger()
+
+	return fileLogger, nil
+}
+
+func createLogFile(config FileConfig) (io.Writer, error) {
+	if config.Dirname != "" {
+		err := os.MkdirAll(config.Dirname, dirPermMode)
+
+		if err != nil {
+			return nil, fmt.Errorf("unable to create directories for new logfile: %s", err)
+		}
+	}
+
+	mode := os.FileMode(filePermMode)
+
+	logFile, err := os.OpenFile(config.Filename, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode)
+	if err != nil {
+		return nil, fmt.Errorf("unable to create a new logfile: %s", err)
+	}
+
+	return logFile, nil
+}
+
 func createRollingLogger(config RollingConfig) (io.Writer, error) {
-	if err := os.MkdirAll(config.Directory, 0744); err != nil {
+	if err := os.MkdirAll(config.Dirname, dirPermMode); err != nil {
 		return nil, err
 	}
 
 	return &lumberjack.Logger{
-		Filename:   path.Join(config.Directory, config.Filename),
+		Filename:   path.Join(config.Dirname, config.Filename),
 		MaxBackups: config.maxBackups,
 		MaxSize:    config.maxSize,
 		MaxAge:     config.maxAge,