log: Return errors when logger configuration fails

When configuration of the logger fails then we print a log message via
the logrus package. This has the problem that this logger is not yet
properly configured and thus may or may not be the correct thing to do.

Refactor the code to instead return an error when configuration of the
logger fails. This lets callers handle the error appropriately by
themselves.

14 files changed, 81 insertions, 28 deletions

diff --git a/cmd/gitaly-backup/main.go b/cmd/gitaly-backup/main.go
index 287342dec..a37121a3e 100644
--- a/cmd/gitaly-backup/main.go
+++ b/cmd/gitaly-backup/main.go
@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"flag"
+	"fmt"
 	"io"
 	"os"
 
@@ -22,7 +23,11 @@ var subcommands = map[string]subcmd{
 }
 
 func main() {
-	logger := log.Configure(os.Stdout, "json", "")
+	logger, err := log.Configure(os.Stdout, "json", "")
+	if err != nil {
+		fmt.Printf("configuring logger failed: %v", err)
+		os.Exit(1)
+	}
 
 	flags := flag.NewFlagSet("gitaly-backup", flag.ExitOnError)
 	_ = flags.Parse(os.Args)
diff --git a/cmd/gitaly-blackbox/main.go b/cmd/gitaly-blackbox/main.go
index 5103cd99e..d9d604e0b 100644
--- a/cmd/gitaly-blackbox/main.go
+++ b/cmd/gitaly-blackbox/main.go
@@ -40,7 +40,11 @@ func main() {
 		os.Exit(1)
 	}
 
-	logger := log.Configure(os.Stdout, cfg.Logging.Format, cfg.Logging.Level)
+	logger, err := log.Configure(os.Stdout, cfg.Logging.Format, cfg.Logging.Level)
+	if err != nil {
+		fmt.Printf("configuring logger failed: %v", err)
+		os.Exit(1)
+	}
 
 	bb := blackbox.New(cfg)
 	prometheus.MustRegister(bb)
diff --git a/cmd/gitaly-git2go/main.go b/cmd/gitaly-git2go/main.go
index 17b4d046c..73e79c99d 100644
--- a/cmd/gitaly-git2go/main.go
+++ b/cmd/gitaly-git2go/main.go
@@ -68,10 +68,15 @@ func main() {
 		correlationID = correlation.SafeRandomID()
 	}
 
-	glog.Configure(os.Stderr, logFormat, logLevel)
-
 	ctx := correlation.ContextWithCorrelation(context.Background(), correlationID)
-	logger := glog.Default().WithFields(logrus.Fields{
+
+	logger, err := glog.Configure(os.Stderr, logFormat, logLevel)
+	if err != nil {
+		fmt.Printf("configuring logger failed: %v", err)
+		os.Exit(1)
+	}
+
+	logger = logger.WithFields(logrus.Fields{
 		"command.name":           git2go.BinaryName,
 		"correlation_id":         correlationID,
 		"enabled_feature_flags":  enabledFeatureFlags,
diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index 20dfba9ce..a19873cf0 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -76,7 +76,11 @@ func main() {
 	ctx, finished := labkittracing.ExtractFromEnv(ctx)
 	defer finished()
 
-	logger := configureLogger(ctx)
+	logger, err := configureLogger(ctx)
+	if err != nil {
+		fmt.Printf("configuring logger failed: %v", err)
+		os.Exit(1)
+	}
 
 	if err := run(ctx, os.Args); err != nil {
 		var hookError hookError
@@ -100,7 +104,7 @@ func main() {
 // configureLogger configures the logger used by gitaly-hooks. As both stdout and stderr might be interpreted by Git, we
 // need to log to a file instead. If the `log.GitalyLogDirEnvKey` environment variable is set, we thus log to a file
 // contained in the directory pointed to by it, otherwise we discard any log messages.
-func configureLogger(ctx context.Context) logrus.FieldLogger {
+func configureLogger(ctx context.Context) (logrus.FieldLogger, error) {
 	writer := io.Discard
 
 	if logDir := os.Getenv(log.GitalyLogDirEnvKey); logDir != "" {
@@ -113,7 +117,12 @@ func configureLogger(ctx context.Context) logrus.FieldLogger {
 		}
 	}
 
-	return log.Configure(writer, "text", "info").WithField(correlation.FieldName, correlation.ExtractFromContext(ctx))
+	logger, err := log.Configure(writer, "text", "info")
+	if err != nil {
+		return nil, err
+	}
+
+	return logger.WithField(correlation.FieldName, correlation.ExtractFromContext(ctx)), nil
 }
 
 // Both stderr and stdout of gitaly-hooks are streamed back to clients. stdout is processed by client
diff --git a/cmd/gitaly-ssh/main.go b/cmd/gitaly-ssh/main.go
index f5f0fad92..121186378 100644
--- a/cmd/gitaly-ssh/main.go
+++ b/cmd/gitaly-ssh/main.go
@@ -42,7 +42,11 @@ type gitalySSHCommand struct {
 // GITALY_USE_SIDECHANNEL=1 if desired
 // gitaly-ssh upload-pack <git-garbage-x2>
 func main() {
-	logger := log.Configure(os.Stderr, "", "info")
+	logger, err := log.Configure(os.Stderr, "", "info")
+	if err != nil {
+		fmt.Printf("configuring logger failed: %v", err)
+		os.Exit(1)
+	}
 
 	// < 4 since git throws on 2x garbage here
 	if n := len(os.Args); n < 4 {
diff --git a/cmd/gitaly-wrapper/main.go b/cmd/gitaly-wrapper/main.go
index dfc101dc1..b83af7bb9 100644
--- a/cmd/gitaly-wrapper/main.go
+++ b/cmd/gitaly-wrapper/main.go
@@ -29,7 +29,12 @@ func main() {
 		logFormat = "json"
 	}
 
-	logger := log.Configure(os.Stdout, logFormat, "").WithField("wrapper", os.Getpid())
+	logger, err := log.Configure(os.Stdout, logFormat, "")
+	if err != nil {
+		fmt.Printf("configuring logger failed: %v", err)
+		os.Exit(1)
+	}
+	logger = logger.WithField("wrapper", os.Getpid())
 
 	if len(os.Args) < 2 {
 		logger.Fatalf("usage: %s forking_binary [args]", os.Args[0])
@@ -102,7 +107,7 @@ func findProcess(pidFilePath string) (*os.Process, error) {
 	return nil, nil
 }
 
-func spawnProcess(logger *logrus.Entry, bin string, args []string) (*os.Process, error) {
+func spawnProcess(logger logrus.FieldLogger, bin string, args []string) (*os.Process, error) {
 	cmd := exec.Command(bin, args...)
 	cmd.Env = append(os.Environ(), fmt.Sprintf("%s=true", bootstrap.EnvUpgradesEnabled))
 
@@ -128,7 +133,7 @@ func isRuntimeSig(s os.Signal) bool {
 	return s == unix.SIGURG
 }
 
-func forwardSignals(gitaly *os.Process, log *logrus.Entry) {
+func forwardSignals(gitaly *os.Process, log logrus.FieldLogger) {
 	sigs := make(chan os.Signal, 1)
 	go func() {
 		for sig := range sigs {
diff --git a/internal/cli/gitaly/check.go b/internal/cli/gitaly/check.go
index c29df5a72..7e7153030 100644
--- a/internal/cli/gitaly/check.go
+++ b/internal/cli/gitaly/check.go
@@ -43,7 +43,10 @@ func checkAction(ctx *cli.Context) error {
 		return fmt.Errorf("loading configuration %q: %w", configPath, err)
 	}
 
-	logger := log.Configure(os.Stderr, "text", "error")
+	logger, err := log.Configure(os.Stderr, "text", "error")
+	if err != nil {
+		return cli.Exit(fmt.Errorf("configuring logger failed: %w", err), 1)
+	}
 
 	fmt.Fprint(ctx.App.Writer, "Checking GitLab API access: ")
 	info, err := checkAPI(cfg, logger)
diff --git a/internal/cli/gitaly/hooks.go b/internal/cli/gitaly/hooks.go
index db4675eed..e73ff0178 100644
--- a/internal/cli/gitaly/hooks.go
+++ b/internal/cli/gitaly/hooks.go
@@ -73,7 +73,9 @@ func setHooksAction(ctx *cli.Context) error {
 		return fmt.Errorf("load config: %w", err)
 	}
 
-	gitalylog.Configure(os.Stdout, cfg.Logging.Format, cfg.Logging.Level)
+	if _, err := gitalylog.Configure(os.Stdout, cfg.Logging.Format, cfg.Logging.Level); err != nil {
+		return fmt.Errorf("configuring loggers: %w", err)
+	}
 
 	storage := ctx.String(flagStorage)
 	if storage == "" {
diff --git a/internal/cli/gitaly/serve.go b/internal/cli/gitaly/serve.go
index 8c20a5a05..813014b71 100644
--- a/internal/cli/gitaly/serve.go
+++ b/internal/cli/gitaly/serve.go
@@ -120,7 +120,10 @@ func configure(configPath string) (config.Cfg, logrus.FieldLogger, error) {
 		"UpdateRemoteMirror",
 	)
 
-	logger := log.Configure(os.Stdout, cfg.Logging.Format, cfg.Logging.Level, urlSanitizer)
+	logger, err := log.Configure(os.Stdout, cfg.Logging.Format, cfg.Logging.Level, urlSanitizer)
+	if err != nil {
+		return config.Cfg{}, nil, fmt.Errorf("configuring logger failed: %w", err)
+	}
 
 	sentry.ConfigureSentry(logger, version.GetVersion(), sentry.Config(cfg.Logging.Sentry))
 	cfg.Prometheus.Configure(logger)
diff --git a/internal/cli/praefect/serve.go b/internal/cli/praefect/serve.go
index 352893085..21394b3a5 100644
--- a/internal/cli/praefect/serve.go
+++ b/internal/cli/praefect/serve.go
@@ -86,7 +86,10 @@ func run(appName string, logger *logrus.Entry, configPath string) error {
 		return err
 	}
 
-	conf.ConfigureLogger()
+	if _, err := conf.ConfigureLogger(); err != nil {
+		return fmt.Errorf("configuring logger: %w", err)
+	}
+
 	configure(logger, appName, conf)
 
 	starterConfigs, err := getStarterConfigs(conf)
diff --git a/internal/log/logger.go b/internal/log/logger.go
index ee7adb7cb..ef8604b3f 100644
--- a/internal/log/logger.go
+++ b/internal/log/logger.go
@@ -1,6 +1,7 @@
 package log
 
 import (
+	"fmt"
 	"io"
 	"os"
 
@@ -58,18 +59,24 @@ type Config struct {
 
 // Configure configures the default and gRPC loggers. The gRPC logger's log level will be mapped in order to decrease
 // its default verbosity. Returns the configured default logger that would also be returned by `Default()`.
-func Configure(out io.Writer, format string, level string, hooks ...logrus.Hook) logrus.FieldLogger {
-	configure(defaultLogger, out, format, level, hooks...)
+func Configure(out io.Writer, format string, level string, hooks ...logrus.Hook) (logrus.FieldLogger, error) {
+	if err := configure(defaultLogger, out, format, level, hooks...); err != nil {
+		return nil, err
+	}
 
 	// We replace the gRPC logger with a custom one because the default one is too chatty.
 	grpcLogger := logrus.New() //nolint:forbidigo
-	configure(grpcLogger, out, format, mapGRPCLogLevel(level), hooks...)
+
+	if err := configure(grpcLogger, out, format, mapGRPCLogLevel(level), hooks...); err != nil {
+		return nil, err
+	}
+
 	grpcmwlogrus.ReplaceGrpcLogger(grpcLogger.WithField("pid", os.Getpid()))
 
-	return Default()
+	return Default(), nil
 }
 
-func configure(logger *logrus.Logger, out io.Writer, format, level string, hooks ...logrus.Hook) {
+func configure(logger *logrus.Logger, out io.Writer, format, level string, hooks ...logrus.Hook) error {
 	var formatter logrus.Formatter
 	switch format {
 	case "json":
@@ -77,7 +84,7 @@ func configure(logger *logrus.Logger, out io.Writer, format, level string, hooks
 	case "", "text":
 		formatter = UTCTextFormatter()
 	default:
-		logrus.WithField("format", format).Fatal("invalid logger format")
+		return fmt.Errorf("invalid logger format %q", format)
 	}
 
 	logrusLevel, err := logrus.ParseLevel(level)
@@ -91,6 +98,8 @@ func configure(logger *logrus.Logger, out io.Writer, format, level string, hooks
 	for _, hook := range hooks {
 		logger.Hooks.Add(hook)
 	}
+
+	return nil
 }
 
 func mapGRPCLogLevel(level string) string {
diff --git a/internal/log/logger_test.go b/internal/log/logger_test.go
index d710eebe4..db2f0fe2e 100644
--- a/internal/log/logger_test.go
+++ b/internal/log/logger_test.go
@@ -101,7 +101,7 @@ func TestConfigure(t *testing.T) {
 			out.Reset()
 
 			logger := newLogger()
-			configure(logger, &out, tc.format, tc.level, tc.hooks...)
+			require.NoError(t, configure(logger, &out, tc.format, tc.level, tc.hooks...))
 
 			// We cannot directly compare the loggers with each other because they contain function
 			// pointers, so we have to check the relevant fields one by one.
diff --git a/internal/praefect/config/log.go b/internal/praefect/config/log.go
index 429034539..4bfe26408 100644
--- a/internal/praefect/config/log.go
+++ b/internal/praefect/config/log.go
@@ -9,8 +9,6 @@ import (
 
 // ConfigureLogger applies the settings from the configuration file to the
 // logger, setting the log level and format.
-func (c Config) ConfigureLogger() *logrus.Entry {
-	log.Configure(os.Stdout, c.Logging.Format, c.Logging.Level)
-
-	return log.Default()
+func (c Config) ConfigureLogger() (logrus.FieldLogger, error) {
+	return log.Configure(os.Stdout, c.Logging.Format, c.Logging.Level)
 }
diff --git a/internal/testhelper/configure.go b/internal/testhelper/configure.go
index 399e20bfc..537485239 100644
--- a/internal/testhelper/configure.go
+++ b/internal/testhelper/configure.go
@@ -81,7 +81,10 @@ func Run(m *testing.M, opts ...RunOption) {
 // configure sets up the global test configuration. On failure,
 // terminates the program.
 func configure() (_ func(), returnedErr error) {
-	logger := log.Configure(os.Stdout, "json", "panic")
+	logger, err := log.Configure(os.Stdout, "json", "panic")
+	if err != nil {
+		return nil, fmt.Errorf("configuring logger failed: %w", err)
+	}
 
 	for key, value := range map[string]string{
 		// We inject the following two variables, which instruct Git to search its