Fix gitlab-shell panic when log file not writablesh-fix-log-permission-error

Previously when the gitlab-shell log was not writable, gitlab-shell
would attempt to fall back to the syslog to log an error. However,
if the syslog logger creation succeeded, gitlab-shell would panic
since `err` was `nil`.

Relates to https://gitlab.com/gitlab-org/gitlab-shell/-/issues/510

2 files changed, 43 insertions, 15 deletions

diff --git a/internal/logger/logger.go b/internal/logger/logger.go
index f836555..3608574 100644
--- a/internal/logger/logger.go
+++ b/internal/logger/logger.go
@@ -22,8 +22,10 @@ func Configure(cfg *config.Config) {
 	logFile, err := os.OpenFile(cfg.LogFile, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
 	if err != nil {
 		progName, _ := os.Executable()
-		syslogLogger, err := syslog.NewLogger(syslog.LOG_ERR|syslog.LOG_USER, 0)
-		syslogLogger.Print(progName + ": Unable to configure logging: " + err.Error())
+		syslogLogger, sysLogErr := syslog.NewLogger(syslog.LOG_ERR|syslog.LOG_USER, 0)
+		if sysLogErr != nil {
+			syslogLogger.Print(progName + ": Unable to configure logging: " + err.Error())
+		}
 
 		// Discard logs since a log file was specified but couldn't be opened
 		log.SetOutput(ioutil.Discard)
diff --git a/internal/logger/logger_test.go b/internal/logger/logger_test.go
index 8b28b37..c2268c1 100644
--- a/internal/logger/logger_test.go
+++ b/internal/logger/logger_test.go
@@ -12,21 +12,47 @@ import (
 )
 
 func TestConfigure(t *testing.T) {
-	tmpFile, err := ioutil.TempFile(os.TempDir(), "logtest-")
-	require.NoError(t, err)
-	defer tmpFile.Close()
-
-	config := config.Config{
-		LogFile:   tmpFile.Name(),
-		LogFormat: "json",
+	testCases := []struct {
+		denyWrite bool
+	}{
+		{
+			denyWrite: false,
+		},
+		{
+			denyWrite: true,
+		},
 	}
 
-	Configure(&config)
-	log.Info("this is a test")
+	for _, tc := range testCases {
+		tmpFile, err := ioutil.TempFile(os.TempDir(), "logtest-")
+		require.NoError(t, err)
+		defer tmpFile.Close()
+		defer os.Remove(tmpFile.Name())
+
+		config := config.Config{
+			LogFile:   tmpFile.Name(),
+			LogFormat: "json",
+		}
+
+		if tc.denyWrite {
+			err = os.Chmod(tmpFile.Name(), 0400)
+		}
 
-	tmpFile.Close()
+		Configure(&config)
+		log.Info("this is a test")
+		tmpFile.Close()
 
-	data, err := ioutil.ReadFile(tmpFile.Name())
-	require.NoError(t, err)
-	require.True(t, strings.Contains(string(data), `msg":"this is a test"`))
+		data, err := ioutil.ReadFile(tmpFile.Name())
+		require.NoError(t, err)
+
+		if tc.denyWrite {
+			// root users can overwrite any file, so we can't verify this in CI.
+			// chattr +i also doesn't work in Docker without special flags.
+			if os.Getenv("CI") == "" {
+				require.Empty(t, data)
+			}
+		} else {
+			require.True(t, strings.Contains(string(data), `msg":"this is a test"`))
+		}
+	}
 }