Merge branch 'fix-lint-sshd-connection' into 'main'

Fix `make lint` (golangci-lint) issues for `internal/sshd/connection.go` and `internal/sshd/connection_test.go`

Closes #715

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



Merged-by: Ash McKenzie <amckenzie@gitlab.com>
Approved-by: Ash McKenzie <amckenzie@gitlab.com>
Reviewed-by: Ash McKenzie <amckenzie@gitlab.com>
Co-authored-by: gaurav.marwal <gauravmarwal@gmail.com>

internal/sshd/connection.go

+// Package sshd provides functionality for SSH daemon connections.
 package sshd
 
 import (
@@ -22,10 +23,14 @@ import (
 )
 
 const (
-	KeepAliveMsg   = "keepalive@openssh.com"
+	// KeepAliveMsg is the message used for keeping SSH connections alive.
+	KeepAliveMsg = "keepalive@openssh.com"
+
+	// NotOurRefError represents the error message indicating that the git upload-pack is not our reference
 	NotOurRefError = `exit status 128, stderr: "fatal: git upload-pack: not our ref `
 )
 
+// EOFTimeout specifies the timeout duration for EOF (End of File) in SSH connections
 var EOFTimeout = 10 * time.Second
 
 type connection struct {
@@ -72,8 +77,8 @@ func (c *connection) handle(ctx context.Context, srvCfg *ssh.ServerConfig, handl
 
 func (c *connection) initServerConn(ctx context.Context, srvCfg *ssh.ServerConfig) (*ssh.ServerConn, <-chan ssh.NewChannel, error) {
 	if c.cfg.Server.LoginGraceTime > 0 {
-		c.nconn.SetDeadline(time.Now().Add(time.Duration(c.cfg.Server.LoginGraceTime)))
-		defer c.nconn.SetDeadline(time.Time{})
+		_ = c.nconn.SetDeadline(time.Now().Add(time.Duration(c.cfg.Server.LoginGraceTime)))
+		defer func() { _ = c.nconn.SetDeadline(time.Time{}) }()
 	}
 
 	sconn, chans, reqs, err := ssh.NewServerConn(c.nconn, srvCfg)
@@ -102,13 +107,13 @@ func (c *connection) handleRequests(ctx context.Context, sconn *ssh.ServerConn,
 
 		if newChannel.ChannelType() != "session" {
 			ctxlog.Info("connection: handleRequests: unknown channel type")
-			newChannel.Reject(ssh.UnknownChannelType, "unknown channel type")
+			_ = newChannel.Reject(ssh.UnknownChannelType, "unknown channel type")
 			continue
 		}
 
 		if !c.concurrentSessions.TryAcquire(1) {
 			ctxlog.Info("connection: handleRequests: too many concurrent sessions")
-			newChannel.Reject(ssh.ResourceShortage, "too many concurrent sessions")
+			_ = newChannel.Reject(ssh.ResourceShortage, "too many concurrent sessions")
 			metrics.SshdHitMaxSessions.Inc()
 			continue
 		}
@@ -150,7 +155,7 @@ func (c *connection) handleRequests(ctx context.Context, sconn *ssh.ServerConn,
 	// Related issue: https://gitlab.com/gitlab-org/gitlab-shell/-/issues/563
 	ctx, cancel := context.WithTimeout(ctx, EOFTimeout)
 	defer cancel()
-	c.concurrentSessions.Acquire(ctx, c.maxSessions)
+	_ = c.concurrentSessions.Acquire(ctx, c.maxSessions)
 }
 
 func (c *connection) sendKeepAliveMsg(ctx context.Context, sconn *ssh.ServerConn, ticker *time.Ticker) {
@@ -163,7 +168,7 @@ func (c *connection) sendKeepAliveMsg(ctx context.Context, sconn *ssh.ServerConn
 		case <-ticker.C:
 			ctxlog.Debug("connection: sendKeepAliveMsg: send keepalive message to a client")
 
-			sconn.SendRequest(KeepAliveMsg, true, nil)
+			_, _, _ = sconn.SendRequest(KeepAliveMsg, true, nil)
 		}
 	}
 }

internal/sshd/connection_test.go

@@ -72,7 +72,7 @@ func (f *fakeConn) SentRequestName() string {
 	return f.sentRequestName
 }
 
-func (f *fakeConn) SendRequest(name string, wantReply bool, payload []byte) (bool, []byte, error) {
+func (f *fakeConn) SendRequest(name string, _ bool, _ []byte) (bool, []byte, error) {
 	f.mu.Lock()
 	defer f.mu.Unlock()
 
@@ -81,7 +81,8 @@ func (f *fakeConn) SendRequest(name string, wantReply bool, payload []byte) (boo
 	return true, nil, nil
 }
 
-func setup(sessionsNum int64, newChannel *fakeNewChannel) (*connection, chan ssh.NewChannel) {
+func setup(newChannel *fakeNewChannel) (*connection, chan ssh.NewChannel) {
+	var sessionsNum int64 = 1
 	cfg := &config.Config{Server: config.ServerConfig{ConcurrentSessionsLimit: sessionsNum}}
 	conn := &connection{cfg: cfg, concurrentSessions: semaphore.NewWeighted(sessionsNum)}
 
@@ -93,18 +94,18 @@ func setup(sessionsNum int64, newChannel *fakeNewChannel) (*connection, chan ssh
 
 func TestPanicDuringSessionIsRecovered(t *testing.T) {
 	newChannel := &fakeNewChannel{channelType: "session"}
-	conn, chans := setup(1, newChannel)
+	conn, chans := setup(newChannel)
 
 	numSessions := 0
 	require.NotPanics(t, func() {
 		conn.handleRequests(context.Background(), nil, chans, func(context.Context, *ssh.ServerConn, ssh.Channel, <-chan *ssh.Request) error {
-			numSessions += 1
+			numSessions++
 			close(chans)
 			panic("This is a panic")
 		})
 	})
 
-	require.Equal(t, numSessions, 1)
+	require.Equal(t, 1, numSessions)
 }
 
 func TestUnknownChannelType(t *testing.T) {
@@ -112,7 +113,7 @@ func TestUnknownChannelType(t *testing.T) {
 	defer close(rejectCh)
 
 	newChannel := &fakeNewChannel{channelType: "unknown session", rejectCh: rejectCh}
-	conn, chans := setup(1, newChannel)
+	conn, chans := setup(newChannel)
 
 	go func() {
 		conn.handleRequests(context.Background(), nil, chans, nil)
@@ -129,7 +130,7 @@ func TestTooManySessions(t *testing.T) {
 	defer close(rejectCh)
 
 	newChannel := &fakeNewChannel{channelType: "session", rejectCh: rejectCh}
-	conn, chans := setup(1, newChannel)
+	conn, chans := setup(newChannel)
 
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -142,12 +143,12 @@ func TestTooManySessions(t *testing.T) {
 	}()
 
 	chans <- newChannel
-	require.Equal(t, <-rejectCh, rejectCall{reason: ssh.ResourceShortage, message: "too many concurrent sessions"})
+	require.Equal(t, rejectCall{reason: ssh.ResourceShortage, message: "too many concurrent sessions"}, <-rejectCh)
 }
 
 func TestAcceptSessionSucceeds(t *testing.T) {
 	newChannel := &fakeNewChannel{channelType: "session"}
-	conn, chans := setup(1, newChannel)
+	conn, chans := setup(newChannel)
 	ctx := context.Background()
 
 	channelHandled := false
@@ -166,7 +167,7 @@ func TestAcceptSessionFails(t *testing.T) {
 
 	acceptErr := errors.New("some failure")
 	newChannel := &fakeNewChannel{channelType: "session", acceptCh: acceptCh, acceptErr: acceptErr}
-	conn, chans := setup(1, newChannel)
+	conn, chans := setup(newChannel)
 	ctx := context.Background()
 
 	channelHandled := false
@@ -177,7 +178,7 @@ func TestAcceptSessionFails(t *testing.T) {
 		})
 	}()
 
-	require.Equal(t, <-acceptCh, struct{}{})
+	require.Equal(t, struct{}{}, <-acceptCh)
 
 	// Waits until the number of sessions is back to 0, since we can only have 1
 	conn.concurrentSessions.Acquire(context.Background(), 1)
@@ -193,7 +194,7 @@ func TestClientAliveInterval(t *testing.T) {
 	defer ticker.Stop()
 
 	conn := &connection{}
-	go conn.sendKeepAliveMsg(context.Background(), &ssh.ServerConn{f, nil}, ticker)
+	go conn.sendKeepAliveMsg(context.Background(), &ssh.ServerConn{Conn: f, Permissions: nil}, ticker)
 
 	require.Eventually(t, func() bool { return KeepAliveMsg == f.SentRequestName() }, time.Second, time.Millisecond)
 }
@@ -205,7 +206,7 @@ func TestSessionsMetrics(t *testing.T) {
 	initialSessionsErrorTotal := testutil.ToFloat64(metrics.SliSshdSessionsErrorsTotal)
 
 	newChannel := &fakeNewChannel{channelType: "session"}
-	conn, chans := setup(1, newChannel)
+	conn, chans := setup(newChannel)
 	ctx := context.Background()
 
 	conn.handleRequests(ctx, nil, chans, func(context.Context, *ssh.ServerConn, ssh.Channel, <-chan *ssh.Request) error {
@@ -213,8 +214,8 @@ func TestSessionsMetrics(t *testing.T) {
 		return errors.New("custom error")
 	})
 
-	eventuallyInDelta(t, initialSessionsTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsTotal), 0.1)
-	eventuallyInDelta(t, initialSessionsErrorTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsErrorsTotal), 0.1)
+	eventuallyInDelta(t, initialSessionsTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsTotal))
+	eventuallyInDelta(t, initialSessionsErrorTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsErrorsTotal))
 
 	for i, ignoredError := range []struct {
 		desc string
@@ -222,12 +223,12 @@ func TestSessionsMetrics(t *testing.T) {
 	}{
 		{"canceled requests", grpcstatus.Error(grpccodes.Canceled, "canceled")},
 		{"unavailable Gitaly", grpcstatus.Error(grpccodes.Unavailable, "unavailable")},
-		{"api error", &client.APIError{"api error"}},
+		{"api error", &client.APIError{Msg: "api error"}},
 		{"disallowed command", disallowedcommand.Error},
 		{"not our ref", grpcstatus.Error(grpccodes.Internal, `rpc error: code = Internal desc = cmd wait: exit status 128, stderr: "fatal: git upload-pack: not our ref 9106d18f6a1b8022f6517f479696f3e3ea5e68c1"`)},
 	} {
 		t.Run(ignoredError.desc, func(t *testing.T) {
-			conn, chans := setup(1, newChannel)
+			conn, chans := setup(newChannel)
 			ignored := ignoredError.err
 			ctx := context.Background()
 
@@ -236,13 +237,14 @@ func TestSessionsMetrics(t *testing.T) {
 				return ignored
 			})
 
-			eventuallyInDelta(t, initialSessionsTotal+2+float64(i), testutil.ToFloat64(metrics.SliSshdSessionsTotal), 0.1)
-			eventuallyInDelta(t, initialSessionsErrorTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsErrorsTotal), 0.1)
+			eventuallyInDelta(t, initialSessionsTotal+2+float64(i), testutil.ToFloat64(metrics.SliSshdSessionsTotal))
+			eventuallyInDelta(t, initialSessionsErrorTotal+1, testutil.ToFloat64(metrics.SliSshdSessionsErrorsTotal))
 		})
 	}
 }
 
-func eventuallyInDelta(t *testing.T, expected, actual, delta float64) {
+func eventuallyInDelta(t *testing.T, expected, actual float64) {
+	var delta = 0.1
 	require.Eventually(t, func() bool {
 		return ((expected - actual) < delta)
 	}, 1*time.Second, time.Millisecond)