From f5a018955645bedf087a8c54ff6023d68fdc5a99 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 23 Nov 2023 21:58:50 +0200 Subject: [PATCH 1/4] fix: improve exit codes for agent/agentssh and cli/ssh --- agent/agentssh/agentssh.go | 24 +++++++++++++++++++-- agent/agentssh/agentssh_test.go | 8 +++++-- cli/root.go | 38 ++++++++++++++++++++++++++++++--- cli/ssh.go | 7 +++++- 4 files changed, 69 insertions(+), 8 deletions(-) diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index 1829449a850be..48fe0462fa75e 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -237,8 +237,28 @@ func (s *Server) sessionHandler(session ssh.Session) { err := s.sessionStart(logger, session, extraEnv) var exitError *exec.ExitError if xerrors.As(err, &exitError) { - logger.Info(ctx, "ssh session returned", slog.Error(exitError)) - _ = session.Exit(exitError.ExitCode()) + code := exitError.ExitCode() + if code == -1 { + // If we return -1 here, it will be transmitted as an + // uint32(4294967295). This exit code is nonsense, so + // instead we return 255 (same as OpenSSH). This is + // also the same exit code that the shell returns for + // -1. + // + // For signals, we could consider sending 128+signal + // instead (however, OpenSSH doesn't seem to do this). + code = 255 + } + logger.Info(ctx, "ssh session returned", + slog.Error(exitError), + slog.F("process_exit_code", exitError.ExitCode()), + slog.F("exit_code", code), + ) + + // TODO(mafredri): For signal exit, there's also an "exit-signal" + // request (session.Exit sends "exit-status"), however, the + // implementation seems incomplete. + _ = session.Exit(code) return } if err != nil { diff --git a/agent/agentssh/agentssh_test.go b/agent/agentssh/agentssh_test.go index 48be4f5619630..deb5213bb1078 100644 --- a/agent/agentssh/agentssh_test.go +++ b/agent/agentssh/agentssh_test.go @@ -227,7 +227,9 @@ func TestNewServer_Signal(t *testing.T) { require.NoError(t, sc.Err()) err = sess.Wait() - require.Error(t, err) + exitErr := &ssh.ExitError{} + require.ErrorAs(t, err, &exitErr) + require.Equal(t, 255, exitErr.ExitStatus()) }) t.Run("PTY", func(t *testing.T) { t.Parallel() @@ -300,7 +302,9 @@ func TestNewServer_Signal(t *testing.T) { require.NoError(t, sc.Err()) err = sess.Wait() - require.Error(t, err) + exitErr := &ssh.ExitError{} + require.ErrorAs(t, err, &exitErr) + require.Equal(t, 255, exitErr.ExitStatus()) }) } diff --git a/cli/root.go b/cli/root.go index 8d645ea5f1d7a..31aa5fec47629 100644 --- a/cli/root.go +++ b/cli/root.go @@ -136,14 +136,22 @@ func (r *RootCmd) RunMain(subcommands []*clibase.Cmd) { } err = cmd.Invoke().WithOS().Run() if err != nil { + code := 1 + var exitErr *exitError + if errors.As(err, &exitErr) { + code = exitErr.code + err = exitErr.err + } if errors.Is(err, cliui.Canceled) { //nolint:revive - os.Exit(1) + os.Exit(code) } f := prettyErrorFormatter{w: os.Stderr, verbose: r.verbose} - f.format(err) + if err != nil { + f.format(err) + } //nolint:revive - os.Exit(1) + os.Exit(code) } } @@ -953,6 +961,30 @@ func DumpHandler(ctx context.Context) { } } +type exitError struct { + code int + err error +} + +var _ error = (*exitError)(nil) + +func (e *exitError) Error() string { + if e.err != nil { + return fmt.Sprintf("exit code %d: %v", e.code, e.err) + } + return fmt.Sprintf("exit code %d", e.code) +} + +func (e *exitError) Unwrap() error { + return e.err +} + +// ExitError returns an error that will cause the CLI to exit with the given +// exit code. If err is non-nil, it will be wrapped by the returned error. +func ExitError(code int, err error) error { + return &exitError{code: code, err: err} +} + // IiConnectionErr is a convenience function for checking if the source of an // error is due to a 'connection refused', 'no such host', etc. func isConnectionError(err error) bool { diff --git a/cli/ssh.go b/cli/ssh.go index 0c4b537949806..ec9e5d16f804c 100644 --- a/cli/ssh.go +++ b/cli/ssh.go @@ -379,11 +379,16 @@ func (r *RootCmd) ssh() *clibase.Cmd { err = sshSession.Wait() if err != nil { + if exitErr := &gossh.ExitError{}; errors.As(err, &exitErr) { + // Clear the error since it's not useful beyond + // reporting status. + return ExitError(exitErr.ExitStatus(), nil) + } // If the connection drops unexpectedly, we get an // ExitMissingError but no other error details, so try to at // least give the user a better message if errors.Is(err, &gossh.ExitMissingError{}) { - return xerrors.New("SSH connection ended unexpectedly") + return ExitError(255, xerrors.New("SSH connection ended unexpectedly")) } return xerrors.Errorf("session ended: %w", err) } From e07ab5af5cb915c324604f12fda199d3b28721fb Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 24 Nov 2023 11:06:18 +0000 Subject: [PATCH 2/4] fix apparently unsupported language syntax in linter... --- cli/ssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/ssh.go b/cli/ssh.go index ec9e5d16f804c..c409bf877ddfe 100644 --- a/cli/ssh.go +++ b/cli/ssh.go @@ -379,7 +379,7 @@ func (r *RootCmd) ssh() *clibase.Cmd { err = sshSession.Wait() if err != nil { - if exitErr := &gossh.ExitError{}; errors.As(err, &exitErr) { + if exitErr := (&gossh.ExitError{}); errors.As(err, &exitErr) { // Clear the error since it's not useful beyond // reporting status. return ExitError(exitErr.ExitStatus(), nil) From ce50540acf572c8706e6675cc53638f7b757bca4 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 24 Nov 2023 11:50:53 +0000 Subject: [PATCH 3/4] windows returns 1 on kill --- agent/agentssh/agentssh_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/agent/agentssh/agentssh_test.go b/agent/agentssh/agentssh_test.go index deb5213bb1078..49d07a11bd51e 100644 --- a/agent/agentssh/agentssh_test.go +++ b/agent/agentssh/agentssh_test.go @@ -229,7 +229,11 @@ func TestNewServer_Signal(t *testing.T) { err = sess.Wait() exitErr := &ssh.ExitError{} require.ErrorAs(t, err, &exitErr) - require.Equal(t, 255, exitErr.ExitStatus()) + wantCode := 255 + if runtime.GOOS == "windows" { + wantCode = 1 + } + require.Equal(t, wantCode, exitErr.ExitStatus()) }) t.Run("PTY", func(t *testing.T) { t.Parallel() @@ -304,7 +308,11 @@ func TestNewServer_Signal(t *testing.T) { err = sess.Wait() exitErr := &ssh.ExitError{} require.ErrorAs(t, err, &exitErr) - require.Equal(t, 255, exitErr.ExitStatus()) + wantCode := 255 + if runtime.GOOS == "windows" { + wantCode = 1 + } + require.Equal(t, wantCode, exitErr.ExitStatus()) }) } From 6fe5a42e64dbd817fa30adea7ce4c9596aefa08e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 24 Nov 2023 14:17:29 +0200 Subject: [PATCH 4/4] improve comment --- agent/agentssh/agentssh.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index 48fe0462fa75e..b6b916b834784 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -256,8 +256,9 @@ func (s *Server) sessionHandler(session ssh.Session) { ) // TODO(mafredri): For signal exit, there's also an "exit-signal" - // request (session.Exit sends "exit-status"), however, the - // implementation seems incomplete. + // request (session.Exit sends "exit-status"), however, since it's + // not implemented on the session interface and not used by + // OpenSSH, we'll leave it for now. _ = session.Exit(code) return }