As we reevaluate how to best support and maintain Staging Ref in the future, we encourage development teams using this environment to highlight their use cases in the following issue: https://gitlab.com/gitlab-com/gl-infra/software-delivery/framework/software-delivery-framework-issue-tracker/-/issues/36.

Skip to content
Snippets Groups Projects
Unverified Commit cffe8fc0 authored by Igor Drozdov's avatar Igor Drozdov Committed by GitLab
Browse files

Merge branch '787-githttp-lint' into 'main' 

Lint fixes for githttp package

Closes #787

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



Merged-by: default avatarIgor Drozdov <idrozdov@gitlab.com>
Approved-by: default avatarIgor Drozdov <idrozdov@gitlab.com>
Reviewed-by: default avatarIgor Drozdov <idrozdov@gitlab.com>
Reviewed-by: default avatarAsh McKenzie <amckenzie@gitlab.com>
Co-authored-by: default avatarAsh McKenzie <amckenzie@gitlab.com>
Co-authored-by: default avatarArchish <archishthakkar@gmail.com>
parents b4ed8b5c b2000699
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
internal/command/githttp/pull.go
+ 27
− 29
View file @ cffe8fc0
// Package githttp provides functionality to handle Git operations over HTTP(S) and SSH,
// including executing Git commands like git-upload-pack and converting responses to the
// expected format for SSH protocols. It integrates with GitLab's internal components
// for secure access verification and data transfer.
package githttp
import (
"bytes"
"context"
"fmt"
"io"
"gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/commandargs"
@@ -17,8 +19,9 @@ import (
const pullService = "git-upload-pack"
var uploadPackHttpPrefix = []byte("001e# service=git-upload-pack\n0000")
var uploadPackHTTPPrefix = []byte("001e# service=git-upload-pack\n0000")
// PullCommand handles the execution of a Git pull operation over HTTP(S) or SSH
type PullCommand struct {
Config *config.Config
ReadWriter *readwriter.ReadWriter
@@ -36,6 +39,12 @@ type PullCommand struct {
// 5. Perform /git-upload-pack request and send this data
// 6. Return the output to the user
// ForInfoRefs returns the necessary Pull specifics for client.InfoRefs()
func (c *PullCommand) ForInfoRefs() (*readwriter.ReadWriter, string, []byte) {
return c.ReadWriter, pullService, uploadPackHTTPPrefix
}
// Execute runs the pull command by determining the appropriate method (HTTP/SSH)
func (c *PullCommand) Execute(ctx context.Context) error {
data := c.Response.Payload.Data
client := &git.Client{URL: data.PrimaryRepo, Headers: data.RequestHeaders}
@@ -48,39 +57,19 @@ func (c *PullCommand) Execute(ctx context.Context) error {
return c.requestSSHUploadPack(ctx, client)
}
if err := c.requestInfoRefs(ctx, client); err != nil {
if err := requestInfoRefs(ctx, client, c); err != nil {
return err
}
return c.requestUploadPack(ctx, client, data.GeoProxyFetchDirectToPrimaryWithOptions)
}
func (c *PullCommand) requestInfoRefs(ctx context.Context, client *git.Client) error {
response, err := client.InfoRefs(ctx, pullService)
if err != nil {
return err
}
defer response.Body.Close()
// Read the first bytes that contain 001e# service=git-upload-pack\n0000 string
// to convert HTTP(S) Git response to the one expected by SSH
p := make([]byte, len(uploadPackHttpPrefix))
_, err = response.Body.Read(p)
if err != nil || !bytes.Equal(p, uploadPackHttpPrefix) {
return fmt.Errorf("Unexpected git-upload-pack response")
}
_, err = io.Copy(c.ReadWriter.Out, response.Body)
return err
}
func (c *PullCommand) requestSSHUploadPack(ctx context.Context, client *git.Client) error {
response, err := client.SSHUploadPack(ctx, io.NopCloser(c.ReadWriter.In))
if err != nil {
return err
}
defer response.Body.Close()
defer response.Body.Close() //nolint:errcheck
_, err = io.Copy(c.ReadWriter.Out, response.Body)
@@ -95,7 +84,7 @@ func (c *PullCommand) requestUploadPack(ctx context.Context, client *git.Client,
if err != nil {
return err
}
defer response.Body.Close()
defer response.Body.Close() //nolint:errcheck
_, err = io.Copy(c.ReadWriter.Out, response.Body)
@@ -108,18 +97,27 @@ func (c *PullCommand) readFromStdin(pw *io.PipeWriter, geoProxyFetchDirectToPrim
for scanner.Scan() {
line := scanner.Bytes()
pw.Write(line)
_, err := pw.Write(line)
if err != nil {
log.WithError(err).Error("failed to write line")
}
if pktline.IsDone(line) {
break
}
if pktline.IsFlush(line) && geoProxyFetchDirectToPrimaryWithOptions {
pw.Write(pktline.PktDone())
_, err := pw.Write(pktline.PktDone())
if err != nil {
log.WithError(err).Error("failed to write packet done line")
}
break
}
}
pw.Close()
err := pw.Close()
if err != nil {
log.WithError(err).Error("failed to close writer")
}
}
internal/command/githttp/pull_test.go
+ 10
− 9
View file @ cffe8fc0
@@ -8,6 +8,7 @@ import (
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/gitlab-shell/v14/client/testserver"
@@ -104,7 +105,7 @@ func TestPullExecuteWithFailedInfoRefs(t *testing.T) {
desc: "unexpected response",
statusCode: http.StatusOK,
responseContent: "unexpected response",
expectedErr: "Unexpected git-upload-pack response",
expectedErr: "unexpected git-upload-pack response",
},
}
@@ -114,7 +115,7 @@ func TestPullExecuteWithFailedInfoRefs(t *testing.T) {
{
Path: "/info/refs",
Handler: func(w http.ResponseWriter, r *http.Request) {
require.Equal(t, "git-upload-pack", r.URL.Query().Get("service"))
assert.Equal(t, "git-upload-pack", r.URL.Query().Get("service"))
w.WriteHeader(tc.statusCode)
w.Write([]byte(tc.responseContent))
@@ -167,7 +168,7 @@ func setupPull(t *testing.T, uploadPackStatusCode int) string {
{
Path: "/info/refs",
Handler: func(w http.ResponseWriter, r *http.Request) {
require.Equal(t, "git-upload-pack", r.URL.Query().Get("service"))
assert.Equal(t, "git-upload-pack", r.URL.Query().Get("service"))
w.Write([]byte(infoRefs))
},
@@ -176,10 +177,10 @@ func setupPull(t *testing.T, uploadPackStatusCode int) string {
Path: "/git-upload-pack",
Handler: func(w http.ResponseWriter, r *http.Request) {
body, err := io.ReadAll(r.Body)
require.NoError(t, err)
assert.NoError(t, err)
defer r.Body.Close()
require.True(t, strings.HasSuffix(string(body), "0009done\n"))
assert.True(t, strings.HasSuffix(string(body), "0009done\n"))
w.WriteHeader(uploadPackStatusCode)
},
@@ -195,12 +196,12 @@ func setupSSHPull(t *testing.T, uploadPackStatusCode int) string {
Path: "/ssh-upload-pack",
Handler: func(w http.ResponseWriter, r *http.Request) {
body, err := io.ReadAll(r.Body)
require.NoError(t, err)
assert.NoError(t, err)
defer r.Body.Close()
require.True(t, strings.HasSuffix(string(body), "0009done\n"))
require.Equal(t, "version=2", r.Header.Get("Git-Protocol"))
require.Equal(t, "token", r.Header.Get("Authorization"))
assert.True(t, strings.HasSuffix(string(body), "0009done\n"))
assert.Equal(t, "version=2", r.Header.Get("Git-Protocol"))
assert.Equal(t, "token", r.Header.Get("Authorization"))
w.Write([]byte("upload-pack-response"))
w.WriteHeader(uploadPackStatusCode)
internal/command/githttp/push.go
+ 26
− 30
View file @ cffe8fc0
package githttp
import (
"bytes"
"context"
"fmt"
"io"
"gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/commandargs"
@@ -15,10 +13,12 @@ import (
"gitlab.com/gitlab-org/labkit/log"
)
const service = "git-receive-pack"
const pushService = "git-receive-pack"
var receivePackHttpPrefix = []byte("001f# service=git-receive-pack\n0000")
var receivePackHTTPPrefix = []byte("001f# service=git-receive-pack\n0000")
// PushCommand handles the execution of a Git push operation,
// including configuration, input/output handling, and access verification.
type PushCommand struct {
Config *config.Config
ReadWriter *readwriter.ReadWriter
@@ -35,6 +35,13 @@ type PushCommand struct {
// 4. Read the send-pack data provided by user via SSH (stdinReader)
// 5. Perform /git-receive-pack request and send this data
// 6. Return the output to the user
// ForInfoRefs returns the necessary Push specifics for client.InfoRefs()
func (c *PushCommand) ForInfoRefs() (*readwriter.ReadWriter, string, []byte) {
return c.ReadWriter, pushService, receivePackHTTPPrefix
}
// Execute runs the push command by determining the appropriate method (HTTP/SSH)
func (c *PushCommand) Execute(ctx context.Context) error {
data := c.Response.Payload.Data
client := &git.Client{URL: data.PrimaryRepo, Headers: data.RequestHeaders}
@@ -47,39 +54,19 @@ func (c *PushCommand) Execute(ctx context.Context) error {
return c.requestSSHReceivePack(ctx, client)
}
if err := c.requestInfoRefs(ctx, client); err != nil {
if err := requestInfoRefs(ctx, client, c); err != nil {
return err
}
return c.requestReceivePack(ctx, client)
}
func (c *PushCommand) requestInfoRefs(ctx context.Context, client *git.Client) error {
response, err := client.InfoRefs(ctx, service)
if err != nil {
return err
}
defer response.Body.Close()
// Read the first bytes that contain 001f# service=git-receive-pack\n0000 string
// to convert HTTP(S) Git response to the one expected by SSH
p := make([]byte, len(receivePackHttpPrefix))
_, err = response.Body.Read(p)
if err != nil || !bytes.Equal(p, receivePackHttpPrefix) {
return fmt.Errorf("Unexpected git-receive-pack response")
}
_, err = io.Copy(c.ReadWriter.Out, response.Body)
return err
}
func (c *PushCommand) requestSSHReceivePack(ctx context.Context, client *git.Client) error {
response, err := client.SSHReceivePack(ctx, io.NopCloser(c.ReadWriter.In))
if err != nil {
return err
}
defer response.Body.Close()
defer response.Body.Close() //nolint:errcheck
_, err = io.Copy(c.ReadWriter.Out, response.Body)
@@ -94,7 +81,7 @@ func (c *PushCommand) requestReceivePack(ctx context.Context, client *git.Client
if err != nil {
return err
}
defer response.Body.Close()
defer response.Body.Close() //nolint:errcheck
_, err = io.Copy(c.ReadWriter.Out, response.Body)
@@ -107,7 +94,10 @@ func (c *PushCommand) readFromStdin(pw *io.PipeWriter) {
scanner := pktline.NewScanner(c.ReadWriter.In)
for scanner.Scan() {
line := scanner.Bytes()
pw.Write(line)
_, err := pw.Write(line)
if err != nil {
log.WithError(err).Error("failed to write line")
}
if pktline.IsFlush(line) {
break
@@ -119,8 +109,14 @@ func (c *PushCommand) readFromStdin(pw *io.PipeWriter) {
}
if needsPackData {
io.Copy(pw, c.ReadWriter.In)
_, err := io.Copy(pw, c.ReadWriter.In)
if err != nil {
log.WithError(err).Error("failed to copy")
}
}
pw.Close()
err := pw.Close()
if err != nil {
log.WithError(err).Error("failed to close writer")
}
}
internal/command/githttp/push_test.go
+ 14
− 13
View file @ cffe8fc0
@@ -8,6 +8,7 @@ import (
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/gitlab-shell/v14/client/testserver"
@@ -40,7 +41,7 @@ func TestExecute(t *testing.T) {
}
require.NoError(t, cmd.Execute(context.Background()))
require.Equal(t, infoRefsWithoutPrefix, output.String())
assert.Equal(t, infoRefsWithoutPrefix, output.String())
}
func TestExecuteWithFailedInfoRefs(t *testing.T) {
@@ -63,7 +64,7 @@ func TestExecuteWithFailedInfoRefs(t *testing.T) {
desc: "unexpected response",
statusCode: http.StatusOK,
responseContent: "unexpected response",
expectedErr: "Unexpected git-receive-pack response",
expectedErr: "unexpected git-receive-pack response",
},
}
@@ -73,7 +74,7 @@ func TestExecuteWithFailedInfoRefs(t *testing.T) {
{
Path: "/info/refs",
Handler: func(w http.ResponseWriter, r *http.Request) {
require.Equal(t, "git-receive-pack", r.URL.Query().Get("service"))
assert.Equal(t, "git-receive-pack", r.URL.Query().Get("service"))
w.WriteHeader(tc.statusCode)
w.Write([]byte(tc.responseContent))
@@ -94,7 +95,7 @@ func TestExecuteWithFailedInfoRefs(t *testing.T) {
err := cmd.Execute(context.Background())
require.Error(t, err)
require.Equal(t, tc.expectedErr, err.Error())
assert.Equal(t, tc.expectedErr, err.Error())
})
}
}
@@ -115,7 +116,7 @@ func TestExecuteWithFailedReceivePack(t *testing.T) {
err := cmd.Execute(context.Background())
require.Error(t, err)
require.Equal(t, "Remote repository is unavailable", err.Error())
assert.Equal(t, "Remote repository is unavailable", err.Error())
}
func TestPushExecuteWithSSHReceivePack(t *testing.T) {
@@ -144,7 +145,7 @@ func TestPushExecuteWithSSHReceivePack(t *testing.T) {
}
require.NoError(t, cmd.Execute(context.Background()))
require.Equal(t, "receive-pack-response", output.String())
assert.Equal(t, "receive-pack-response", output.String())
}
func setup(t *testing.T, receivePackStatusCode int) (string, io.Reader) {
@@ -164,7 +165,7 @@ func setup(t *testing.T, receivePackStatusCode int) (string, io.Reader) {
{
Path: "/info/refs",
Handler: func(w http.ResponseWriter, r *http.Request) {
require.Equal(t, "git-receive-pack", r.URL.Query().Get("service"))
assert.Equal(t, "git-receive-pack", r.URL.Query().Get("service"))
w.Write([]byte(infoRefs))
},
@@ -173,10 +174,10 @@ func setup(t *testing.T, receivePackStatusCode int) (string, io.Reader) {
Path: "/git-receive-pack",
Handler: func(w http.ResponseWriter, r *http.Request) {
body, err := io.ReadAll(r.Body)
require.NoError(t, err)
assert.NoError(t, err)
defer r.Body.Close()
require.Equal(t, receivePackPrefix+flush+receivePackData, string(body))
assert.Equal(t, receivePackPrefix+flush+receivePackData, string(body))
w.WriteHeader(receivePackStatusCode)
},
},
@@ -191,12 +192,12 @@ func setupSSHPush(t *testing.T, uploadPackStatusCode int) string {
Path: "/ssh-receive-pack",
Handler: func(w http.ResponseWriter, r *http.Request) {
body, err := io.ReadAll(r.Body)
require.NoError(t, err)
assert.NoError(t, err)
defer r.Body.Close()
require.True(t, strings.HasSuffix(string(body), "0009done\n"))
require.Equal(t, "version=2", r.Header.Get("Git-Protocol"))
require.Equal(t, "token", r.Header.Get("Authorization"))
assert.True(t, strings.HasSuffix(string(body), "0009done\n"))
assert.Equal(t, "version=2", r.Header.Get("Git-Protocol"))
assert.Equal(t, "token", r.Header.Get("Authorization"))
w.WriteHeader(uploadPackStatusCode)
w.Write([]byte("receive-pack-response"))
internal/command/githttp/util.go 0 → 100644
+ 41
− 0
View file @ cffe8fc0
package githttp
import (
"bytes"
"context"
"fmt"
"io"
"gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/readwriter"
"gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/git"
)
type gitHTTPCommand interface {
ForInfoRefs() (*readwriter.ReadWriter, string, []byte)
}
// requestInfoRefs performs an HTTP request to the /info/refs endpoint for the specified Git service,
// verifies the response prefix, and writes the result to the output stream.
func requestInfoRefs(ctx context.Context, client *git.Client, command gitHTTPCommand) error {
readWriter, serviceName, httpPrefix := command.ForInfoRefs()
response, err := client.InfoRefs(ctx, serviceName)
if err != nil {
return err
}
defer response.Body.Close() //nolint:errcheck
// Read the first bytes that contain for
// push - 001f# service=git-receive-pack\n0000 string
// pull - 001e# service=git-upload-pack\n0000 string
// to convert HTTP(S) Git response to the one expected by SSH
p := make([]byte, len(httpPrefix))
_, err = response.Body.Read(p)
if err != nil || !bytes.Equal(p, httpPrefix) {
return fmt.Errorf("unexpected %s response", serviceName)
}
_, err = io.Copy(readWriter.Out, response.Body)
return err
}
support/lint_last_known_acceptable.txt
+ 0
− 40
View file @ cffe8fc0
@@ -131,46 +131,6 @@ internal/command/discover/discover.go:23:15: ST1005: error strings should not be
internal/command/discover/discover.go:29:14: Error return value of `fmt.Fprintf` is not checked (errcheck)
internal/command/discover/discover.go:32:14: Error return value of `fmt.Fprintf` is not checked (errcheck)
internal/command/discover/discover.go:35:20: context-keys-type: should not use basic type string as key in context.WithValue (revive)
internal/command/githttp/pull.go:1:1: package-comments: should have a package comment (revive)
internal/command/githttp/pull.go:20:5: var-naming: var uploadPackHttpPrefix should be uploadPackHTTPPrefix (revive)
internal/command/githttp/pull.go:22:6: exported: exported type PullCommand should have comment or be unexported (revive)
internal/command/githttp/pull.go:39:1: exported: exported method PullCommand.Execute should have comment or be unexported (revive)
internal/command/githttp/pull.go:58: 58-76 lines are duplicate of `internal/command/githttp/push.go:57-75` (dupl)
internal/command/githttp/pull.go:63:27: Error return value of `response.Body.Close` is not checked (errcheck)
internal/command/githttp/pull.go:70:10: ST1005: error strings should not be capitalized (stylecheck)
internal/command/githttp/pull.go:83:27: Error return value of `response.Body.Close` is not checked (errcheck)
internal/command/githttp/pull.go:98:27: Error return value of `response.Body.Close` is not checked (errcheck)
internal/command/githttp/pull.go:111:11: Error return value of `pw.Write` is not checked (errcheck)
internal/command/githttp/pull.go:118:12: Error return value of `pw.Write` is not checked (errcheck)
internal/command/githttp/pull.go:124:10: Error return value of `pw.Close` is not checked (errcheck)
internal/command/githttp/pull_test.go:117:7: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/pull_test.go:170:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/pull_test.go:179:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/pull_test.go:182:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/pull_test.go:198:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/pull_test.go:201:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/pull_test.go:202:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/pull_test.go:203:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/push.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck)
internal/command/githttp/push.go:20:5: var-naming: var receivePackHttpPrefix should be receivePackHTTPPrefix (revive)
internal/command/githttp/push.go:22:6: exported: exported type PushCommand should have comment or be unexported (revive)
internal/command/githttp/push.go:29:1: exported: comment on exported method PushCommand.Execute should be of the form "Execute ..." (revive)
internal/command/githttp/push.go:57: 57-75 lines are duplicate of `internal/command/githttp/pull.go:58-76` (dupl)
internal/command/githttp/push.go:62:27: Error return value of `response.Body.Close` is not checked (errcheck)
internal/command/githttp/push.go:69:10: ST1005: error strings should not be capitalized (stylecheck)
internal/command/githttp/push.go:82:27: Error return value of `response.Body.Close` is not checked (errcheck)
internal/command/githttp/push.go:97:27: Error return value of `response.Body.Close` is not checked (errcheck)
internal/command/githttp/push.go:110:11: Error return value of `pw.Write` is not checked (errcheck)
internal/command/githttp/push.go:122:10: Error return value of `io.Copy` is not checked (errcheck)
internal/command/githttp/push.go:125:10: Error return value of `pw.Close` is not checked (errcheck)
internal/command/githttp/push_test.go:76:7: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/push_test.go:167:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/push_test.go:176:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/push_test.go:179:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/push_test.go:194:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/push_test.go:197:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/push_test.go:198:5: go-require: do not use require in http handlers (testifylint)
internal/command/githttp/push_test.go:199:5: go-require: do not use require in http handlers (testifylint)
internal/command/healthcheck/healthcheck.go:1:1: package-comments: should have a package comment (revive)
internal/command/healthcheck/healthcheck.go:17:6: exported: exported type Command should have comment or be unexported (revive)
internal/command/healthcheck/healthcheck.go:22:1: exported: exported method Command.Execute should have comment or be unexported (revive)
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment

As we reevaluate how to best support and maintain Staging Ref in the future, we encourage development teams using this environment to highlight their use cases in the following issue: https://gitlab.com/gitlab-com/gl-infra/software-delivery/framework/software-delivery-framework-issue-tracker/-/issues/36.