diff --git a/log/logger.go b/log/logger.go index 9381ca4a65..065cd6e064 100644 --- a/log/logger.go +++ b/log/logger.go @@ -58,20 +58,44 @@ type Logger struct { var pid = os.Getpid() -// NewLogger creates a new Logger. -func NewLogger(name string, level int, target int, logDir string) *Logger { - var logger Logger +// NewLoggerE creates a new Logger and surfaces any errors encountered during +// the process. The returned logger is guaranteed to be safe to use when a +// non-nil error is returned, but may have undesired behavior. Callers should +// treat the logger as nil under error conditions unless necessary for +// backwards compatibility reasons. +func NewLoggerE(name string, level, target int, logDir string) (*Logger, error) { + logger := &Logger{ + l: log.New(io.Discard, logPrefix, log.LstdFlags), + name: name, + level: level, + directory: logDir, + maxFileSize: maxLogFileSize, + maxFileCount: maxLogFileCount, + mutex: &sync.Mutex{}, + } - logger.l = log.New(nil, logPrefix, log.LstdFlags) - logger.name = name - logger.level = level - logger.directory = logDir - logger.SetTarget(target) - logger.maxFileSize = maxLogFileSize - logger.maxFileCount = maxLogFileCount - logger.mutex = &sync.Mutex{} + err := logger.SetTarget(target) + if err != nil { + // we *do* want to return the logger here for backwards compatibility + return logger, fmt.Errorf("setting log target: %w", err) + } + return logger, nil +} + +// NewLogger creates a new Logger. +// +// Deprecated: use NewLoggerE instead +func NewLogger(name string, level, target int, logDir string) *Logger { + logger, err := NewLoggerE(name, level, target, logDir) + if err != nil { + // ideally this would be returned to the caller, but this API is depended + // on by unknown parties. Given at this point we have an unusable (but + // safe) logger, we log to stderr with the standard library in hopes that + // an operator will see the error and be able to take corrective action + log.Println("error initializing logger: err:", err) + } - return &logger + return logger } // SetName sets the log name. diff --git a/log/logger_test.go b/log/logger_test.go index 70c0636006..e341b5604a 100644 --- a/log/logger_test.go +++ b/log/logger_test.go @@ -6,6 +6,7 @@ package log import ( "fmt" "os" + "path" "strings" "testing" ) @@ -14,6 +15,33 @@ const ( logName = "test" ) +func TestNewLoggerError(t *testing.T) { + // we expect an error from NewLoggerE in the event that we provide an + // unwriteable directory + + // this test needs a guaranteed empty directory, so we create a temporary one + // and ensure that it gets destroyed afterward. + targetDir, err := os.MkdirTemp("", "acn") + if err != nil { + t.Fatal("unable to create temporary directory: err:", err) + } + + t.Cleanup(func() { + // This removal could produce an error, but since it's a temporary + // directory anyway, this is a best-effort cleanup + os.Remove(targetDir) + }) + + // if we just use the targetDir, NewLoggerE will create the file and it will + // work. We need a non-existent directory *within* the tempdir + fullPath := path.Join(targetDir, "definitelyDoesNotExist") + + _, err = NewLoggerE(logName, LevelInfo, TargetLogfile, fullPath) + if err == nil { + t.Error("expected an error but did not receive one") + } +} + // Tests that the log file rotates when size limit is reached. func TestLogFileRotatesWhenSizeLimitIsReached(t *testing.T) { logDirectory := "" // This sets the current location for logs