feat(cli/ssh): simplify log file flags

ammario · ammario · commit 4b3b2bdb02a6 · 2023-06-05T23:53:35.000Z
diff --git a/cli/ssh.go b/cli/ssh.go
@@ -10,7 +10,6 @@ import (
 	"net/url"
 	"os"
 	"os/exec"
-	"path"
 	"path/filepath"
 	"strings"
 	"time"
@@ -50,8 +49,7 @@ func (r *RootCmd) ssh() *clibase.Cmd {
 		identityAgent  string
 		wsPollInterval time.Duration
 		noWait         bool
-		logDir         string
-		logToFile      bool
+		logFilePath    string
 	)
 	client := new(codersdk.Client)
 	cmd := &clibase.Cmd{
@@ -74,21 +72,10 @@ func (r *RootCmd) ssh() *clibase.Cmd {
 					logger.Error(ctx, "command exit", slog.Error(retErr))
 				}
 			}()
-			if logToFile {
-				// we need a way to ensure different ssh invocations don't clobber
-				// each other's logs. Date-time strings will likely have collisions
-				// in unit tests and/or scripts unless we extend precision out to
-				// sub-millisecond, which seems unwieldy.  A simple 5-character random
-				// string will do it, since the operating system already tracks
-				// dates and times for file IO.
-				qual, err := cryptorand.String(5)
+			if logFilePath != "" {
+				logFile, err := os.OpenFile(logFilePath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600)
 				if err != nil {
-					return xerrors.Errorf("generate random qualifier: %w", err)
-				}
-				logPth := path.Join(logDir, fmt.Sprintf("coder-ssh-%s.log", qual))
-				logFile, err := os.Create(logPth)
-				if err != nil {
-					return xerrors.Errorf("error opening %s for logging: %w", logPth, err)
+					return xerrors.Errorf("error opening %s for logging: %w", logFilePath, err)
 				}
 				logger = slog.Make(sloghuman.Sink(logFile))
 				defer logFile.Close()
@@ -354,18 +341,11 @@ func (r *RootCmd) ssh() *clibase.Cmd {
 			Value:       clibase.BoolOf(&noWait),
 		},
 		{
-			Flag:        "log-dir",
-			Default:     os.TempDir(),
-			Description: "Specify the location for the log files.",
-			Env:         "CODER_SSH_LOG_DIR",
-			Value:       clibase.StringOf(&logDir),
-		},
-		{
-			Flag:          "log-to-file",
+			Flag:          "log-file",
+			Description:   "Specify the location of an SSH diagnostic log file.",
+			Env:           "CODER_SSH_LOG_FILE",
 			FlagShorthand: "l",
-			Env:           "CODER_SSH_LOG_TO_FILE",
-			Description:   "Enable diagnostic logging to file.",
-			Value:         clibase.BoolOf(&logToFile),
+			Value:         clibase.StringOf(&logFilePath),
 		},
 	}
 	return cmd
diff --git a/cli/ssh_test.go b/cli/ssh_test.go
@@ -411,10 +411,10 @@ func TestSSH(t *testing.T) {
 	t.Run("FileLogging", func(t *testing.T) {
 		t.Parallel()
 
-		dir := t.TempDir()
+		logFile := filepath.Join(t.TempDir(), "coder-ssh.log")
 
 		client, workspace, agentToken := setupWorkspaceForAgent(t, nil)
-		inv, root := clitest.New(t, "ssh", workspace.Name, "-l", "--log-dir", dir)
+		inv, root := clitest.New(t, "ssh", workspace.Name, "-l", logFile)
 		clitest.SetupConfig(t, client, root)
 		pty := ptytest.New(t).Attach(inv)
 
@@ -441,15 +441,10 @@ func TestSSH(t *testing.T) {
 		pty.WriteLine("exit")
 		<-cmdDone
 
-		entries, err := os.ReadDir(dir)
-		require.NoError(t, err)
-		for _, e := range entries {
-			t.Logf("logdir entry: %s", e.Name())
-			if strings.HasPrefix(e.Name(), "coder-ssh") {
-				return
-			}
+		_, err := os.Stat(logFile)
+		if err != nil {
+			t.Fatalf("failed to find ssh logfile: %v", err)
 		}
-		t.Fatal("failed to find ssh logfile")
 	})
 }
 
diff --git a/cli/testdata/coder_ssh_--help.golden b/cli/testdata/coder_ssh_--help.golden
@@ -18,11 +18,8 @@ Start a shell into a workspace
           Specifies which identity agent to use (overrides $SSH_AUTH_SOCK),
           forward agent must also be enabled.
 
-      --log-dir string, $CODER_SSH_LOG_DIR (default: /tmp)
-          Specify the location for the log files.
-
-  -l, --log-to-file bool, $CODER_SSH_LOG_TO_FILE
-          Enable diagnostic logging to file.
+  -l, --log-file string, $CODER_SSH_LOG_FILE
+          Specify the location of an SSH diagnostic log file.
 
       --no-wait bool, $CODER_SSH_NO_WAIT
           Specifies whether to wait for a workspace to become ready before
