Merge branch 'main' of gitlab.com:gitlab-community/gitlab-shell into 793-discover-lint

Author: Archish27

client/transport.go

@@ -1,3 +1,4 @@
+// Package client provides an HTTP client with enhanced logging, tracing, and correlation handling.
 package client
 
 import (
@@ -13,6 +14,7 @@ type transport struct {
 	next http.RoundTripper
 }
 
+// RoundTrip executes a single HTTP transaction, adding logging and tracing capabilities.
 func (rt *transport) RoundTrip(request *http.Request) (*http.Response, error) {
 	ctx := request.Context()
 
@@ -55,10 +57,12 @@ func (rt *transport) RoundTrip(request *http.Request) (*http.Response, error) {
 	return response, nil
 }
 
+// DefaultTransport returns a clone of the default HTTP transport.
 func DefaultTransport() http.RoundTripper {
 	return http.DefaultTransport.(*http.Transport).Clone()
 }
 
+// NewTransport creates a new transport with logging, tracing, and correlation handling.
 func NewTransport(next http.RoundTripper) http.RoundTripper {
 	t := &transport{next: next}
 	return correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(t))

cmd/gitlab-sshd/main.go

@@ -1,3 +1,4 @@
+// Package main implements the GitLab SSH daemon.
 package main
 
 import (
@@ -27,8 +28,8 @@ var (
 )
 
 func overrideConfigFromEnvironment(cfg *config.Config) {
-	if gitlabUrl := os.Getenv("GITLAB_URL"); gitlabUrl != "" {
-		cfg.GitlabUrl = gitlabUrl
+	if gitlabURL := os.Getenv("GITLAB_URL"); gitlabURL != "" {
+		cfg.GitlabUrl = gitlabURL
 	}
 	if gitlabTracing := os.Getenv("GITLAB_TRACING"); gitlabTracing != "" {
 		cfg.GitlabTracing = gitlabTracing
@@ -67,8 +68,11 @@ func main() {
 	cfg.ApplyGlobalState()
 
 	logCloser := logger.ConfigureStandalone(cfg)
-	defer logCloser.Close()
-
+	defer func() {
+		if err := logCloser.Close(); err != nil {
+			log.WithError(err).Fatal("Error closing logCloser")
+		}
+	}()
 	ctx, finished := command.Setup("gitlab-sshd", cfg)
 	defer finished()
 
@@ -81,15 +85,7 @@ func main() {
 
 	// Startup monitoring endpoint.
 	if cfg.Server.WebListen != "" {
-		go func() {
-			err := monitoring.Start(
-				monitoring.WithListenerAddress(cfg.Server.WebListen),
-				monitoring.WithBuildInformation(Version, BuildTime),
-				monitoring.WithServeMux(server.MonitoringServeMux()),
-			)
-
-			log.WithError(err).Fatal("monitoring service raised an error")
-		}()
+		startupMonitoringEndpoint(cfg, server)
 	}
 
 	ctx, cancel := context.WithCancel(ctx)
@@ -98,21 +94,39 @@ func main() {
 	done := make(chan os.Signal, 1)
 	signal.Notify(done, syscall.SIGINT, syscall.SIGTERM)
 
+	gracefulShutdown(ctx, done, cfg, server, cancel)
+
+	if err := server.ListenAndServe(ctx); err != nil {
+		log.WithError(err).Fatal("GitLab built-in sshd failed to listen for new connections")
+	}
+}
+
+func gracefulShutdown(ctx context.Context, done chan os.Signal, cfg *config.Config, server *sshd.Server, cancel context.CancelFunc) {
 	go func() {
 		sig := <-done
 		signal.Reset(syscall.SIGINT, syscall.SIGTERM)
 
 		gracePeriod := time.Duration(cfg.Server.GracePeriod)
 		log.WithContextFields(ctx, log.Fields{"shutdown_timeout_s": gracePeriod.Seconds(), "signal": sig.String()}).Info("Shutdown initiated")
 
-		server.Shutdown()
+		if err := server.Shutdown(); err != nil {
+			log.WithError(err).Fatal("Error shutting down the server")
+		}
 
 		<-time.After(gracePeriod)
 
 		cancel()
 	}()
+}
 
-	if err := server.ListenAndServe(ctx); err != nil {
-		log.WithError(err).Fatal("GitLab built-in sshd failed to listen for new connections")
-	}
+func startupMonitoringEndpoint(cfg *config.Config, server *sshd.Server) {
+	go func() {
+		err := monitoring.Start(
+			monitoring.WithListenerAddress(cfg.Server.WebListen),
+			monitoring.WithBuildInformation(Version, BuildTime),
+			monitoring.WithServeMux(server.MonitoringServeMux()),
+		)
+
+		log.WithError(err).Fatal("monitoring service raised an error")
+	}()
 }

go.mod

@@ -5,7 +5,7 @@ go 1.22
 toolchain go1.22.6
 
 require (
-	github.com/charmbracelet/git-lfs-transfer v0.1.1-0.20240809134258-2cab0ea18f7e
+	github.com/charmbracelet/git-lfs-transfer v0.1.1-0.20240909190640-edbf58104250
 	github.com/git-lfs/pktline v0.0.0-20230103162542-ca444d533ef1
 	github.com/golang-jwt/jwt/v5 v5.2.1
 	github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
@@ -18,11 +18,11 @@ require (
 	github.com/prometheus/client_golang v1.20.3
 	github.com/sirupsen/logrus v1.9.3
 	github.com/stretchr/testify v1.9.0
-	gitlab.com/gitlab-org/gitaly/v16 v16.11.8
+	gitlab.com/gitlab-org/gitaly/v16 v16.11.9
 	gitlab.com/gitlab-org/labkit v1.21.0
 	golang.org/x/crypto v0.27.0
 	golang.org/x/sync v0.8.0
-	google.golang.org/grpc v1.66.0
+	google.golang.org/grpc v1.66.1
 	google.golang.org/protobuf v1.34.2
 	gopkg.in/yaml.v3 v3.0.1
 )

go.sum

@@ -82,8 +82,8 @@ github.com/census-instrumentation/opencensus-proto v0.4.1 h1:iKLQ0xPNFxR/2hzXZMr
 github.com/census-instrumentation/opencensus-proto v0.4.1/go.mod h1:4T9NM4+4Vw91VeyqjLS6ao50K5bOcLKN6Q42XnYaRYw=
 github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=
 github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
-github.com/charmbracelet/git-lfs-transfer v0.1.1-0.20240809134258-2cab0ea18f7e h1:0ykk0ltl/PYMiDz1WfHDTc3WVg/nPY19O9SJq6y/ZW8=
-github.com/charmbracelet/git-lfs-transfer v0.1.1-0.20240809134258-2cab0ea18f7e/go.mod h1:84e2N3Hojky9EZivj6QyAQ7bjkZJ+pwFwqzi/1c/4iU=
+github.com/charmbracelet/git-lfs-transfer v0.1.1-0.20240909190640-edbf58104250 h1:7q/muqKUnoQgReeoJtS2qY13HS4qmXigl5opxTMcrUg=
+github.com/charmbracelet/git-lfs-transfer v0.1.1-0.20240909190640-edbf58104250/go.mod h1:eEYu9YGtNB3EhSYX+vb2BSAfxUuHMhs3mvYM1mj7ZgY=
 github.com/chzyer/logex v1.1.10/go.mod h1:+Ywpsq7O8HXn0nuIou7OrIPyXbp3wmkHB+jjWRnGsAI=
 github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e/go.mod h1:nSuG5e5PlCu98SY8svDHJxuZscDgtXS6KTTbou5AhLI=
 github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU=
@@ -383,8 +383,8 @@ github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9dec
 github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k=
 github.com/yusufpapurcu/wmi v1.2.2 h1:KBNDSne4vP5mbSWnJbO+51IMOXJB67QiYCSBrubbPRg=
 github.com/yusufpapurcu/wmi v1.2.2/go.mod h1:SBZ9tNy3G9/m5Oi98Zks0QjeHVDvuK0qfxQmPyzfmi0=
-gitlab.com/gitlab-org/gitaly/v16 v16.11.8 h1:bL9F90+rXTlQcsSuZJivn+CIwKGXXc787IJi4g3XQEU=
-gitlab.com/gitlab-org/gitaly/v16 v16.11.8/go.mod h1:lJizRUtXRd1SBHjNbbbL9OsGN4TiugvfRBd8bIsdWI0=
+gitlab.com/gitlab-org/gitaly/v16 v16.11.9 h1:UKuF9m7A6v4vKMWRjWQ4hATWoUd7xZstBZEtBFMdUi4=
+gitlab.com/gitlab-org/gitaly/v16 v16.11.9/go.mod h1:lJizRUtXRd1SBHjNbbbL9OsGN4TiugvfRBd8bIsdWI0=
 gitlab.com/gitlab-org/labkit v1.21.0 h1:hLmdBDtXjD1yOmZ+uJOac3a5Tlo83QaezwhES4IYik4=
 gitlab.com/gitlab-org/labkit v1.21.0/go.mod h1:zeATDAaSBelPcPLbTTq8J3ZJEHyPTLVBM1q3nva+/W4=
 go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU=
@@ -788,8 +788,8 @@ google.golang.org/grpc v1.37.1/go.mod h1:NREThFqKR1f3iQ6oBuvc5LadQuXVGo9rkm5ZGrQ
 google.golang.org/grpc v1.38.0/go.mod h1:NREThFqKR1f3iQ6oBuvc5LadQuXVGo9rkm5ZGrQdJfM=
 google.golang.org/grpc v1.39.0/go.mod h1:PImNr+rS9TWYb2O4/emRugxiyHZ5JyHW5F+RPnDzfrE=
 google.golang.org/grpc v1.39.1/go.mod h1:PImNr+rS9TWYb2O4/emRugxiyHZ5JyHW5F+RPnDzfrE=
-google.golang.org/grpc v1.66.0 h1:DibZuoBznOxbDQxRINckZcUvnCEvrW9pcWIE2yF9r1c=
-google.golang.org/grpc v1.66.0/go.mod h1:s3/l6xSSCURdVfAnL+TqCNMyTDAGN6+lZeVxnZR128Y=
+google.golang.org/grpc v1.66.1 h1:hO5qAXR19+/Z44hmvIM4dQFMSYX9XcWsByfoxutBpAM=
+google.golang.org/grpc v1.66.1/go.mod h1:s3/l6xSSCURdVfAnL+TqCNMyTDAGN6+lZeVxnZR128Y=
 google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0/go.mod h1:6Kw0yEErY5E/yWrBtf03jp27GLLJujG4z/JK95pnjjw=
 google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8=
 google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0=

internal/command/shared/disallowedcommand/disallowedcommand.go

@@ -1,7 +1,9 @@
+// Package disallowedcommand provides an error for handling disallowed commands.
 package disallowedcommand
 
 import "errors"
 
 var (
-	Error = errors.New("Disallowed command")
+	// Error is returned when a disallowed command is encountered.
+	Error = errors.New("Disallowed command") //nolint:stylecheck // Used to display the error message to the user.
 )

internal/command/uploadarchive/gitalycall_test.go

@@ -21,15 +21,15 @@ func TestUploadArchive(t *testing.T) {
 	for _, network := range []string{"unix", "tcp", "dns"} {
 		t.Run(fmt.Sprintf("via %s network", network), func(t *testing.T) {
 			gitalyAddress, testServer := testserver.StartGitalyServer(t, network)
-			t.Log(fmt.Sprintf("Server address: %s", gitalyAddress))
+			t.Logf("Server address: %s", gitalyAddress)
 
 			requests := requesthandlers.BuildAllowedWithGitalyHandlers(t, gitalyAddress)
 			url := testserver.StartHTTPServer(t, requests)
 
 			output := &bytes.Buffer{}
 			input := &bytes.Buffer{}
 
-			userId := "1"
+			userID := "1"
 			repo := "group/repo"
 
 			env := sshenv.Env{
@@ -39,7 +39,7 @@ func TestUploadArchive(t *testing.T) {
 			}
 
 			args := &commandargs.Shell{
-				GitlabKeyId: userId,
+				GitlabKeyId: userID,
 				CommandType: commandargs.UploadArchive,
 				SshArgs:     []string{"git-upload-archive", repo},
 				Env:         env,

internal/gitlabnet/lfsauthenticate/client_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-shell/v14/client/testserver"
@@ -26,10 +27,10 @@ func setup(t *testing.T) []testserver.TestRequestHandler {
 			Handler: func(w http.ResponseWriter, r *http.Request) {
 				b, err := io.ReadAll(r.Body)
 				defer r.Body.Close()
-				require.NoError(t, err)
+				assert.NoError(t, err)
 
 				var request *Request
-				require.NoError(t, json.Unmarshal(b, &request))
+				assert.NoError(t, json.Unmarshal(b, &request))
 
 				switch request.KeyID {
 				case keyID:
@@ -39,7 +40,7 @@ func setup(t *testing.T) []testserver.TestRequestHandler {
 						"repository_http_path": "https://gitlab.com/repo/path",
 						"expires_in":           1800,
 					}
-					require.NoError(t, json.NewEncoder(w).Encode(body))
+					assert.NoError(t, json.NewEncoder(w).Encode(body))
 				case "forbidden":
 					w.WriteHeader(http.StatusForbidden)
 				case "broken":
@@ -88,7 +89,7 @@ func TestFailedRequests(t *testing.T) {
 			_, err = client.Authenticate(context.Background(), operation, repo, "")
 			require.Error(t, err)
 
-			require.Equal(t, tc.expectedOutput, err.Error())
+			assert.Equal(t, tc.expectedOutput, err.Error())
 		})
 	}
 }
@@ -128,7 +129,7 @@ func TestSuccessfulRequests(t *testing.T) {
 				ExpiresIn: 1800,
 			}
 
-			require.Equal(t, expectedResponse, response)
+			assert.Equal(t, expectedResponse, response)
 		})
 	}
 }

internal/gitlabnet/lfstransfer/client.go

@@ -1,3 +1,4 @@
+// Package lfstransfer provides functionality for handling LFS (Large File Storage) transfers.
 package lfstransfer
 
 import (
@@ -17,6 +18,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet"
 )
 
+// Client holds configuration, arguments, and authentication details for the client.
 type Client struct {
 	config *config.Config
 	args   *commandargs.Shell
@@ -25,13 +27,15 @@ type Client struct {
 	header string
 }
 
+// BatchAction represents an action for a batch operation with metadata.
 type BatchAction struct {
 	Href      string            `json:"href"`
 	Header    map[string]string `json:"header,omitempty"`
 	ExpiresAt time.Time         `json:"expires_at,omitempty"`
 	ExpiresIn int               `json:"expires_in,omitempty"`
 }
 
+// BatchObject represents an object in a batch operation with its metadata and actions.
 type BatchObject struct {
 	Oid           string                  `json:"oid,omitempty"`
 	Size          int64                   `json:"size"`
@@ -50,6 +54,7 @@ type batchRequest struct {
 	HashAlgorithm string         `json:"hash_algo,omitempty"`
 }
 
+// BatchResponse contains batch operation results and the hash algorithm used.
 type BatchResponse struct {
 	Objects       []*BatchObject `json:"objects"`
 	HashAlgorithm string         `json:"hash_algo,omitempty"`
@@ -79,30 +84,36 @@ type listLocksVerifyRequest struct {
 	Ref    *batchRef `json:"ref,omitempty"`
 }
 
+// LockOwner represents the owner of a lock.
 type LockOwner struct {
 	Name string `json:"name"`
 }
 
+// Lock represents a lock with its ID, path, timestamp, and owner details.
 type Lock struct {
 	ID       string     `json:"id"`
 	Path     string     `json:"path"`
 	LockedAt time.Time  `json:"locked_at"`
 	Owner    *LockOwner `json:"owner"`
 }
 
+// ListLocksResponse contains a list of locks and a cursor for pagination.
 type ListLocksResponse struct {
 	Locks      []*Lock `json:"locks,omitempty"`
 	NextCursor string  `json:"next_cursor,omitempty"`
 }
 
+// ListLocksVerifyResponse provides lists of locks for "ours" and "theirs" with a cursor for pagination.
 type ListLocksVerifyResponse struct {
 	Ours       []*Lock `json:"ours,omitempty"`
 	Theirs     []*Lock `json:"theirs,omitempty"`
 	NextCursor string  `json:"next_cursor,omitempty"`
 }
 
+// ClientHeader specifies the content type for Git LFS JSON requests.
 var ClientHeader = "application/vnd.git-lfs+json"
 
+// NewClient creates a new Client instance using the provided configuration and credentials.
 func NewClient(config *config.Config, args *commandargs.Shell, href string, auth string) (*Client, error) {
 	return &Client{config: config, args: args, href: href, auth: auth, header: ClientHeader}, nil
 }
@@ -121,6 +132,7 @@ func newHTTPClient() *retryablehttp.Client {
 	return client
 }
 
+// Batch performs a batch operation on objects and returns the result.
 func (c *Client) Batch(operation string, reqObjects []*BatchObject, ref string, reqHashAlgo string) (*BatchResponse, error) {
 	// FIXME: This causes tests to fail
 	// if ref == "" {
@@ -211,6 +223,7 @@ func (c *Client) PutObject(_, href string, headers map[string]string, r io.Reade
 	return nil
 }
 
+// Lock acquires a lock for the specified path with an optional reference name.
 func (c *Client) Lock(path, refname string) (*Lock, error) {
 	var ref *batchRef
 	if refname != "" {
@@ -268,6 +281,7 @@ func (c *Client) Lock(path, refname string) (*Lock, error) {
 	}
 }
 
+// Unlock releases the lock with the given id, optionally forcing the unlock.
 func (c *Client) Unlock(id string, force bool, refname string) (*Lock, error) {
 	var ref *batchRef
 	if refname != "" {
@@ -318,6 +332,7 @@ func (c *Client) Unlock(id string, force bool, refname string) (*Lock, error) {
 	}
 }
 
+// ListLocksVerify retrieves locks for the given path and id, with optional pagination.
 func (c *Client) ListLocksVerify(path, id, cursor string, limit int, ref string) (*ListLocksVerifyResponse, error) {
 	url, err := url.Parse(c.href)
 	if err != nil {

internal/sshd/connection.go

@@ -168,7 +168,15 @@ 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)
+			status, payload, err := sconn.SendRequest(KeepAliveMsg, true, nil)
+			if err != nil {
+				ctxlog.Errorf("Error occurred while sending request :%v", err)
+				return
+			}
+
+			if status {
+				ctxlog.Debugf("connection: sendKeepAliveMsg: payload: %v", string(payload))
+			}
 		}
 	}
 }

internal/sshd/connection_test.go

@@ -78,7 +78,7 @@ func (f *fakeConn) SendRequest(name string, _ bool, _ []byte) (bool, []byte, err
 
 	f.sentRequestName = name
 
-	return true, nil, nil
+	return true, []byte("I am a response"), nil
 }
 
 func setup(newChannel *fakeNewChannel) (*connection, chan ssh.NewChannel) {