Merge branch 'id-use-http-client-for-geo' into 'main'

Use a separate HTTP Client for Geo requests

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



Merged-by: Patrick Bajao <ebajao@gitlab.com>
Approved-by: Michael Kozono <mkozono@gitlab.com>
Approved-by: Alejandro Rodríguez <alejandro@gitlab.com>
Approved-by: Patrick Bajao <ebajao@gitlab.com>
Reviewed-by: Igor Drozdov <idrozdov@gitlab.com>
Reviewed-by: Patrick Bajao <ebajao@gitlab.com>
Co-authored-by: Igor Drozdov <idrozdov@gitlab.com>

client/httpclient.go

@@ -119,7 +119,7 @@ func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath stri
 	c.RetryWaitMax = hcc.retryWaitMax
 	c.RetryWaitMin = hcc.retryWaitMin
 	c.Logger = nil
-	c.HTTPClient.Transport = newTransport(transport)
+	c.HTTPClient.Transport = NewTransport(transport)
 	c.HTTPClient.Timeout = readTimeout(readTimeoutSeconds)
 
 	client := &HttpClient{RetryableHTTP: c, Host: host}

client/transport.go

@@ -55,7 +55,11 @@ func (rt *transport) RoundTrip(request *http.Request) (*http.Response, error) {
 	return response, nil
 }
 
-func newTransport(next http.RoundTripper) http.RoundTripper {
+func DefaultTransport() http.RoundTripper {
+	return http.DefaultTransport.(*http.Transport).Clone()
+}
+
+func NewTransport(next http.RoundTripper) http.RoundTripper {
 	t := &transport{next: next}
 	return correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(t))
 }

internal/command/githttp/push.go

@@ -34,11 +34,7 @@ type PushCommand struct {
 // 6. Return the output to the user
 func (c *PushCommand) Execute(ctx context.Context) error {
 	data := c.Response.Payload.Data
-	client, err := git.NewClient(c.Config, data.PrimaryRepo, data.RequestHeaders)
-	if err != nil {
-		return err
-	}
-
+	client := &git.Client{Url: data.PrimaryRepo, Headers: data.RequestHeaders}
 	if err := c.requestInfoRefs(ctx, client); err != nil {
 		return err
 	}

internal/command/githttp/push_test.go

@@ -51,7 +51,7 @@ func TestExecuteWithFailedInfoRefs(t *testing.T) {
 		{
 			desc:        "request failed",
 			statusCode:  http.StatusForbidden,
-			expectedErr: "Internal API error (403)",
+			expectedErr: "Remote repository is unavailable",
 		}, {
 			desc:            "unexpected response",
 			statusCode:      http.StatusOK,
@@ -108,7 +108,7 @@ func TestExecuteWithFailedReceivePack(t *testing.T) {
 
 	err := cmd.Execute(context.Background())
 	require.Error(t, err)
-	require.Equal(t, "Internal API error (403)", err.Error())
+	require.Equal(t, "Remote repository is unavailable", err.Error())
 }
 
 func setup(t *testing.T, receivePackStatusCode int) (string, io.Reader) {

internal/gitlabnet/git/client.go

@@ -2,32 +2,25 @@ package git
 
 import (
 	"context"
-	"fmt"
 	"io"
 	"net/http"
 
 	"gitlab.com/gitlab-org/gitlab-shell/v14/client"
-	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/config"
-	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet"
 )
 
-type Client struct {
-	url     string
-	headers map[string]string
-	client  *client.GitlabNetClient
+var httpClient = &http.Client{
+	Transport: client.NewTransport(client.DefaultTransport()),
 }
 
-func NewClient(cfg *config.Config, url string, headers map[string]string) (*Client, error) {
-	client, err := gitlabnet.GetClient(cfg)
-	if err != nil {
-		return nil, fmt.Errorf("Error creating http client: %v", err)
-	}
+const repoUnavailableErrMsg = "Remote repository is unavailable"
 
-	return &Client{client: client, headers: headers, url: url}, nil
+type Client struct {
+	Url     string
+	Headers map[string]string
 }
 
 func (c *Client) InfoRefs(ctx context.Context, service string) (*http.Response, error) {
-	request, err := http.NewRequestWithContext(ctx, http.MethodGet, c.url+"/info/refs?service="+service, nil)
+	request, err := http.NewRequestWithContext(ctx, http.MethodGet, c.Url+"/info/refs?service="+service, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -36,7 +29,7 @@ func (c *Client) InfoRefs(ctx context.Context, service string) (*http.Response,
 }
 
 func (c *Client) ReceivePack(ctx context.Context, body io.Reader) (*http.Response, error) {
-	request, err := http.NewRequestWithContext(ctx, http.MethodPost, c.url+"/git-receive-pack", body)
+	request, err := http.NewRequestWithContext(ctx, http.MethodPost, c.Url+"/git-receive-pack", body)
 	if err != nil {
 		return nil, err
 	}
@@ -47,10 +40,14 @@ func (c *Client) ReceivePack(ctx context.Context, body io.Reader) (*http.Respons
 }
 
 func (c *Client) do(request *http.Request) (*http.Response, error) {
-
-	for k, v := range c.headers {
+	for k, v := range c.Headers {
 		request.Header.Add(k, v)
 	}
 
-	return c.client.Do(request)
+	response, err := httpClient.Do(request)
+	if err != nil || response.StatusCode >= 400 {
+		return nil, &client.ApiError{repoUnavailableErrMsg}
+	}
+
+	return response, nil
 }

internal/gitlabnet/git/client_test.go

@@ -8,8 +8,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	httpclient "gitlab.com/gitlab-org/gitlab-shell/v14/client"
 	"gitlab.com/gitlab-org/gitlab-shell/v14/client/testserver"
-	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/config"
 )
 
 var customHeaders = map[string]string{
@@ -50,6 +50,21 @@ func TestReceivePack(t *testing.T) {
 	require.Equal(t, "git-receive-pack: content", string(body))
 }
 
+func TestFailedHTTPRequest(t *testing.T) {
+	client := &Client{
+		Url:     testserver.StartHttpServer(t, []testserver.TestRequestHandler{}),
+		Headers: customHeaders,
+	}
+
+	response, err := client.InfoRefs(context.Background(), "git-receive-pack")
+	require.Nil(t, response)
+	require.Error(t, err)
+
+	var apiErr *httpclient.ApiError
+	require.ErrorAs(t, err, &apiErr)
+	require.EqualError(t, err, repoUnavailableErrMsg)
+}
+
 func setup(t *testing.T) *Client {
 	requests := []testserver.TestRequestHandler{
 		{
@@ -80,9 +95,10 @@ func setup(t *testing.T) *Client {
 		},
 	}
 
-	url := testserver.StartHttpServer(t, requests)
-	client, err := NewClient(&config.Config{GitlabUrl: url}, url, customHeaders)
-	require.NoError(t, err)
+	client := &Client{
+		Url:     testserver.StartHttpServer(t, requests),
+		Headers: customHeaders,
+	}
 
 	return client
 }