Merge branch 'ef-adopt-nicer-file-writing' into 'main'

Bumps the LabKit version to adopt the NewWithFile logging constructor

See merge request https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/1372

Merged-by: Vasilii Iakliushin <viakliushin@gitlab.com>
Approved-by: James Fargher <jfargher@gitlab.com>
Approved-by: Vasilii Iakliushin <viakliushin@gitlab.com>
Reviewed-by: James Fargher <jfargher@gitlab.com>
Reviewed-by: Vasilii Iakliushin <viakliushin@gitlab.com>
Co-authored-by: e_forbes <eforbes@gitlab.com>

Author: 2 people

cmd/gitlab-sshd/main.go

@@ -43,6 +43,7 @@ func overrideConfigFromEnvironment(cfg *config.Config) {
 	}
 }
 
+// nolint
 func main() {
 	ctx := context.Background()
 	command.CheckForVersionFlag(os.Args, Version, BuildTime)
@@ -58,7 +59,17 @@ func main() {
 			))
 		}
 	}
-	log := logger.ConfigureLogger(cfg)
+	log, logCloser, err := logger.ConfigureLogger(cfg)
+	if err != nil {
+		log.ErrorContext(ctx, "failed to log to file, reverting to stderr", slog.String(fields.ErrorMessage, err.Error()))
+	} else {
+		// nolint
+		defer func() {
+			if err = logCloser.Close(); err != nil {
+				log.ErrorContext(ctx, "failed to close log file", slog.String(fields.ErrorMessage, err.Error()))
+			}
+		}()
+	}
 	log.InfoContext(ctx, "gitlab-sshd starting up...")
 
 	overrideConfigFromEnvironment(cfg)

go.mod

@@ -19,7 +19,7 @@ require (
 	github.com/sirupsen/logrus v1.9.4
 	github.com/stretchr/testify v1.11.1
 	gitlab.com/gitlab-org/gitaly/v18 v18.9.0-rc4
-	gitlab.com/gitlab-org/labkit v1.36.0
+	gitlab.com/gitlab-org/labkit v1.37.0
 	golang.org/x/crypto v0.47.0
 	golang.org/x/sync v0.19.0
 	google.golang.org/grpc v1.77.0

go.sum

@@ -571,8 +571,8 @@ gitlab.com/gitlab-org/gitaly/v18 v18.9.0-rc4 h1:2PQlCQF5ioVdmcjY6Ut8zxwFY2TIgHWn
 gitlab.com/gitlab-org/gitaly/v18 v18.9.0-rc4/go.mod h1:ijDPC9GyV3UdmrZG4LiBQJyMjOuzLcbH5AuQ8GpSSIk=
 gitlab.com/gitlab-org/go/reopen v1.0.0 h1:6BujZ0lkkjGIejTUJdNO1w56mN1SI10qcVQyQlOPM+8=
 gitlab.com/gitlab-org/go/reopen v1.0.0/go.mod h1:D6OID8YJDzEVZNYW02R/Pkj0v8gYFSIhXFTArAsBQw8=
-gitlab.com/gitlab-org/labkit v1.36.0 h1:QA60DLXmtOyIRchrxaRwonrtbviWQogA623tR2JijE4=
-gitlab.com/gitlab-org/labkit v1.36.0/go.mod h1:JqQLdgjV/KKAZJ6gvNodaLStmWeTT9mxgJPIEi66VHI=
+gitlab.com/gitlab-org/labkit v1.37.0 h1:Nk7DIc5QB8QNQgrNhymDACMMSIwjjaHXL4CVSHmXvac=
+gitlab.com/gitlab-org/labkit v1.37.0/go.mod h1:JqQLdgjV/KKAZJ6gvNodaLStmWeTT9mxgJPIEi66VHI=
 go.etcd.io/raft/v3 v3.6.0 h1:5NtvbDVYpnfZWcIHgGRk9DyzkBIXOi8j+DDp1IcnUWQ=
 go.etcd.io/raft/v3 v3.6.0/go.mod h1:nLvLevg6+xrVtHUmVaTcTz603gQPHfh7kUAwV6YpfGo=
 go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU=

internal/logger/logger.go

@@ -18,22 +18,14 @@ import (
 // ConfigureLogger - gitlab-sshd's log output can be configured to text as per the documentation:
 // https://docs.gitlab.com/omnibus/settings/logs/#json-logging
 // This is currently controlled by the GITLAB_LOG_FORMAT environment variable.
-func ConfigureLogger(cfg *config.Config) *slog.Logger {
+func ConfigureLogger(cfg *config.Config) (*slog.Logger, io.Closer, error) {
 	logConfig := &v2log.Config{
 		LogLevel: parseLogLevel(cfg.LogLevel),
 	}
-	logFile, err := os.OpenFile(cfg.LogFile, os.O_APPEND|os.O_WRONLY, 0600)
-	if err != nil {
-		syslogError(err)
-		logConfig.Writer = os.Stderr
-	} else {
-		logConfig.Writer = logFile
-	}
-
 	if gitlabLogFormat := os.Getenv("GITLAB_LOG_FORMAT"); gitlabLogFormat == "text" {
 		logConfig.UseTextFormat = true
 	}
-	return v2log.NewWithConfig(logConfig)
+	return v2log.NewWithFile(cfg.LogFile, logConfig)
 }
 
 func parseLogLevel(level string) slog.Level {

internal/logger/logger_test.go

@@ -1,6 +1,7 @@
 package logger
 
 import (
+	"io"
 	"os"
 	"regexp"
 	"testing"
@@ -20,7 +21,7 @@ func TestConfigure(t *testing.T) {
 	}
 
 	closer := Configure(&config)
-	defer closer.Close()
+	defer MustClose(t, closer)
 
 	log.Info("this is a test")
 	log.WithFields(log.Fields{}).Debug("debug log message")
@@ -43,7 +44,7 @@ func TestConfigureWithDebugLogLevel(t *testing.T) {
 	}
 
 	closer := Configure(&config)
-	defer closer.Close()
+	defer MustClose(t, closer)
 
 	log.WithFields(log.Fields{}).Debug("debug log message")
 
@@ -61,7 +62,7 @@ func TestConfigureWithPermissionError(t *testing.T) {
 	}
 
 	closer := Configure(&config)
-	defer closer.Close()
+	defer MustClose(t, closer)
 
 	log.Info("this is a test")
 }
@@ -75,7 +76,7 @@ func TestLogInUTC(t *testing.T) {
 	}
 
 	closer := Configure(&config)
-	defer closer.Close()
+	defer MustClose(t, closer)
 
 	log.Info("this is a test")
 
@@ -107,7 +108,9 @@ func TestConfigureLabkitV2Log(t *testing.T) {
 		LogLevel:  "debug",
 	}
 
-	logger := ConfigureLogger(&config)
+	logger, closer, err := ConfigureLogger(&config)
+	require.NoError(t, err)
+	defer MustClose(t, closer)
 	logger.Info("this is a test")
 	logger.Debug("debug log message")
 
@@ -117,3 +120,11 @@ func TestConfigureLabkitV2Log(t *testing.T) {
 	require.Contains(t, dataStr, `"msg":"this is a test"`)
 	require.Contains(t, dataStr, `"msg":"debug log message"`)
 }
+
+// MustClose calls Close() on the Closer and fails the test in case it returns
+// an error. This function is useful when closing via `defer`, as a simple
+// `defer require.NoError(t, closer.Close())` would cause `closer.Close()` to
+// be executed early already.
+func MustClose(tb testing.TB, closer io.Closer) {
+	require.NoError(tb, closer.Close())
+}