Merge branch 'id-ignore-gitaly-unavailable-errors' into 'main'

Exclude Gitaly unavailable error from error rate

See merge request gitlab-org/gitlab-shell!641

internal/config/config_test.go

@@ -41,7 +41,7 @@ func TestCustomPrometheusMetrics(t *testing.T) {
 	require.NoError(t, err)
 
 	var actualNames []string
-	for _, m := range ms[0:10] {
+	for _, m := range ms[0:9] {
 		actualNames = append(actualNames, m.GetName())
 	}
 
@@ -49,7 +49,6 @@ func TestCustomPrometheusMetrics(t *testing.T) {
 		"gitlab_shell_http_in_flight_requests",
 		"gitlab_shell_http_request_duration_seconds",
 		"gitlab_shell_http_requests_total",
-		"gitlab_shell_sshd_canceled_sessions",
 		"gitlab_shell_sshd_concurrent_limited_sessions_total",
 		"gitlab_shell_sshd_in_flight_connections",
 		"gitlab_shell_sshd_session_duration_seconds",

internal/handler/exec.go

@@ -58,13 +58,11 @@ func (gc *GitalyCommand) RunGitalyCommand(ctx context.Context, handler GitalyHan
 	exitStatus, err := handler(childCtx, conn)
 
 	if err != nil {
-		if grpcstatus.Convert(err).Code() == grpccodes.Unavailable {
-			ctxlog.WithError(fmt.Errorf("RunGitalyCommand: %v", err)).Error("Gitaly is unavailable")
+		ctxlog.WithError(err).WithFields(log.Fields{"exit_status": exitStatus}).Error("Failed to execute Git command")
 
-			return fmt.Errorf("The git server, Gitaly, is not available at this time. Please contact your administrator.")
+		if grpcstatus.Code(err) == grpccodes.Unavailable {
+			return grpcstatus.Error(grpccodes.Unavailable, "The git server, Gitaly, is not available at this time. Please contact your administrator.")
 		}
-
-		ctxlog.WithError(err).WithFields(log.Fields{"exit_status": exitStatus}).Error("Failed to execute Git command")
 	}
 
 	return err

internal/handler/exec_test.go

@@ -84,10 +84,8 @@ func TestUnavailableGitalyErr(t *testing.T) {
 		},
 	)
 
-	expectedErr := grpcstatus.Error(grpccodes.Unavailable, "error")
-	err := cmd.RunGitalyCommand(context.Background(), makeHandler(t, expectedErr))
-
-	require.EqualError(t, err, "The git server, Gitaly, is not available at this time. Please contact your administrator.")
+	err := cmd.RunGitalyCommand(context.Background(), makeHandler(t, grpcstatus.Error(grpccodes.Unavailable, "error")))
+	require.Equal(t, err, grpcstatus.Error(grpccodes.Unavailable, "The git server, Gitaly, is not available at this time. Please contact your administrator."))
 }
 
 func TestRunGitalyCommandMetadata(t *testing.T) {

internal/metrics/metrics.go

@@ -77,15 +77,6 @@ var (
 		},
 	)
 
-	SshdCanceledSessions = promauto.NewCounter(
-		prometheus.CounterOpts{
-			Namespace: namespace,
-			Subsystem: sshdSubsystem,
-			Name:      sshdCanceledSessionsName,
-			Help:      "The number of canceled gitlab-sshd sessions.",
-		},
-	)
-
 	SliSshdSessionsTotal = promauto.NewCounter(
 		prometheus.CounterOpts{
 			Name: sliSshdSessionsTotalName,

internal/sshd/connection.go

@@ -89,14 +89,7 @@ func (c *connection) handle(ctx context.Context, chans <-chan ssh.NewChannel, ha
 			metrics.SliSshdSessionsTotal.Inc()
 			err := handler(ctx, channel, requests)
 			if err != nil {
-				if grpcstatus.Convert(err).Code() == grpccodes.Canceled {
-					metrics.SshdCanceledSessions.Inc()
-				} else {
-					var apiError *client.ApiError
-					if !errors.As(err, &apiError) {
-						metrics.SliSshdSessionsErrorsTotal.Inc()
-					}
-				}
+				c.trackError(err)
 			}
 
 			ctxlog.Info("connection: handle: done")
@@ -126,3 +119,17 @@ func (c *connection) sendKeepAliveMsg(ctx context.Context, ticker *time.Ticker)
 		}
 	}
 }
+
+func (c *connection) trackError(err error) {
+	var apiError *client.ApiError
+	if errors.As(err, &apiError) {
+		return
+	}
+
+	grpcCode := grpcstatus.Code(err)
+	if grpcCode == grpccodes.Canceled || grpcCode == grpccodes.Unavailable {
+		return
+	}
+
+	metrics.SliSshdSessionsErrorsTotal.Inc()
+}

internal/sshd/connection_test.go

@@ -200,7 +200,6 @@ func TestSessionsMetrics(t *testing.T) {
 	// https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#pkg-index
 	initialSessionsTotal := testutil.ToFloat64(metrics.SliSshdSessionsTotal)
 	initialSessionsErrorTotal := testutil.ToFloat64(metrics.SliSshdSessionsErrorsTotal)
-	initialCanceledSessions := testutil.ToFloat64(metrics.SshdCanceledSessions)
 
 	newChannel := &fakeNewChannel{channelType: "session"}
 
@@ -212,17 +211,15 @@ func TestSessionsMetrics(t *testing.T) {
 
 	require.InDelta(t, initialSessionsTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsTotal), 0.1)
 	require.InDelta(t, initialSessionsErrorTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsErrorsTotal), 0.1)
-	require.InDelta(t, initialCanceledSessions, testutil.ToFloat64(metrics.SshdCanceledSessions), 0.1)
 
 	conn, chans = setup(1, newChannel)
 	conn.handle(context.Background(), chans, func(context.Context, ssh.Channel, <-chan *ssh.Request) error {
 		close(chans)
-		return grpcstatus.Error(grpccodes.Canceled, "error")
+		return grpcstatus.Error(grpccodes.Canceled, "canceled")
 	})
 
 	require.InDelta(t, initialSessionsTotal+2, testutil.ToFloat64(metrics.SliSshdSessionsTotal), 0.1)
 	require.InDelta(t, initialSessionsErrorTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsErrorsTotal), 0.1)
-	require.InDelta(t, initialCanceledSessions+1, testutil.ToFloat64(metrics.SshdCanceledSessions), 0.1)
 
 	conn, chans = setup(1, newChannel)
 	conn.handle(context.Background(), chans, func(context.Context, ssh.Channel, <-chan *ssh.Request) error {
@@ -232,5 +229,13 @@ func TestSessionsMetrics(t *testing.T) {
 
 	require.InDelta(t, initialSessionsTotal+3, testutil.ToFloat64(metrics.SliSshdSessionsTotal), 0.1)
 	require.InDelta(t, initialSessionsErrorTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsErrorsTotal), 0.1)
-	require.InDelta(t, initialCanceledSessions+1, testutil.ToFloat64(metrics.SshdCanceledSessions), 0.1)
+
+	conn, chans = setup(1, newChannel)
+	conn.handle(context.Background(), chans, func(context.Context, ssh.Channel, <-chan *ssh.Request) error {
+		close(chans)
+		return grpcstatus.Error(grpccodes.Unavailable, "unavailable")
+	})
+
+	require.InDelta(t, initialSessionsTotal+4, testutil.ToFloat64(metrics.SliSshdSessionsTotal), 0.1)
+	require.InDelta(t, initialSessionsErrorTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsErrorsTotal), 0.1)
 }

internal/sshd/session.go

@@ -8,11 +8,14 @@ import (
 
 	"gitlab.com/gitlab-org/labkit/log"
 	"golang.org/x/crypto/ssh"
+	grpccodes "google.golang.org/grpc/codes"
+	grpcstatus "google.golang.org/grpc/status"
 
 	shellCmd "gitlab.com/gitlab-org/gitlab-shell/cmd/gitlab-shell/command"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/shared/disallowedcommand"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/config"
+	"gitlab.com/gitlab-org/gitlab-shell/internal/console"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/sshenv"
 )
 
@@ -158,10 +161,12 @@ func (s *session) handleShell(ctx context.Context, req *ssh.Request) (uint32, er
 
 	cmd, err := shellCmd.NewWithKey(s.gitlabKeyId, env, s.cfg, rw)
 	if err != nil {
-		if !errors.Is(err, disallowedcommand.Error) {
-			s.toStderr(ctx, "Failed to parse command: %v\n", err.Error())
+		if errors.Is(err, disallowedcommand.Error) {
+			s.toStderr(ctx, "ERROR: Unknown command: %v\n", s.execCmd)
+		} else {
+			s.toStderr(ctx, "ERROR: Failed to parse command: %v\n", err.Error())
 		}
-		s.toStderr(ctx, "Unknown command: %v\n", s.execCmd)
+
 		return 128, err
 	}
 
@@ -169,7 +174,11 @@ func (s *session) handleShell(ctx context.Context, req *ssh.Request) (uint32, er
 	ctxlog.WithFields(log.Fields{"env": env, "command": cmdName}).Info("session: handleShell: executing command")
 
 	if err := cmd.Execute(ctx); err != nil {
-		s.toStderr(ctx, "remote: ERROR: %v\n", err.Error())
+		grpcStatus := grpcstatus.Convert(err)
+		if grpcStatus.Code() != grpccodes.Internal {
+			s.toStderr(ctx, "ERROR: %v\n", grpcStatus.Message())
+		}
+
 		return 1, err
 	}
 
@@ -181,7 +190,7 @@ func (s *session) handleShell(ctx context.Context, req *ssh.Request) (uint32, er
 func (s *session) toStderr(ctx context.Context, format string, args ...interface{}) {
 	out := fmt.Sprintf(format, args...)
 	log.WithContextFields(ctx, log.Fields{"stderr": out}).Debug("session: toStderr: output")
-	fmt.Fprint(s.channel.Stderr(), out)
+	console.DisplayWarningMessage(out, s.channel.Stderr())
 }
 
 func (s *session) exit(ctx context.Context, status uint32) {

internal/sshd/session_test.go

@@ -13,6 +13,7 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-shell/client/testserver"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/config"
+	"gitlab.com/gitlab-org/gitlab-shell/internal/console"
 )
 
 type fakeChannel struct {
@@ -160,14 +161,14 @@ func TestHandleShell(t *testing.T) {
 		{
 			desc:              "fails to parse command",
 			cmd:               `\`,
-			errMsg:            "Failed to parse command: Invalid SSH command: invalid command line string\nUnknown command: \\\n",
+			errMsg:            "ERROR: Failed to parse command: Invalid SSH command: invalid command line string\n",
 			gitlabKeyId:       "root",
 			expectedErrString: "Invalid SSH command: invalid command line string",
 			expectedExitCode:  128,
 		}, {
 			desc:              "specified command is unknown",
 			cmd:               "unknown-command",
-			errMsg:            "Unknown command: unknown-command\n",
+			errMsg:            "ERROR: Unknown command: unknown-command\n",
 			gitlabKeyId:       "root",
 			expectedErrString: "Disallowed command",
 			expectedExitCode:  128,
@@ -175,7 +176,7 @@ func TestHandleShell(t *testing.T) {
 			desc:              "fails to parse command",
 			cmd:               "discover",
 			gitlabKeyId:       "",
-			errMsg:            "remote: ERROR: Failed to get username: who='' is invalid\n",
+			errMsg:            "ERROR: Failed to get username: who='' is invalid\n",
 			expectedErrString: "Failed to get username: who='' is invalid",
 			expectedExitCode:  1,
 		}, {
@@ -208,7 +209,14 @@ func TestHandleShell(t *testing.T) {
 			}
 
 			require.Equal(t, tc.expectedExitCode, exitCode)
-			require.Equal(t, tc.errMsg, out.String())
+
+			formattedErr := &bytes.Buffer{}
+			if tc.errMsg != "" {
+				console.DisplayWarningMessage(tc.errMsg, formattedErr)
+				require.Equal(t, formattedErr.String(), out.String())
+			} else {
+				require.Equal(t, tc.errMsg, out.String())
+			}
 		})
 	}
 }

internal/sshd/sshd.go

@@ -44,7 +44,7 @@ func logSSHInitError(ctxlog *logrus.Entry, err error) {
 
 	logger := ctxlog.WithError(err)
 
-	if strings.Contains(err.Error(), "no common algorithm for host key") {
+	if strings.Contains(err.Error(), "no common algorithm for host key") || err.Error() == "EOF" {
 		logger.Debug(msg)
 	} else {
 		logger.Warn(msg)