Merge branch 'feat/remove-unretryable-http' into 'main'

feat: make retryable http default client

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



Merged-by: Ash McKenzie <amckenzie@gitlab.com>
Approved-by: Oscar Tovar <otovar@gitlab.com>
Approved-by: Ash McKenzie <amckenzie@gitlab.com>
Co-authored-by: Steve Azzopardi <sazzopardi@gitlab.com>

.gitlab-ci.yml

@@ -64,13 +64,6 @@ tests:
     - make coverage
   coverage: '/\d+.\d+%/'
 
-tests-integration-retryableHttp:
-  extends: .test
-  variables:
-    FF_GITLAB_SHELL_RETRYABLE_HTTP: '1'
-  script:
-    - make test_ruby
-
 race:
   extends: .test
   script:

client/client_test.go

@@ -7,7 +7,6 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
-	"os"
 	"path"
 	"strings"
 	"testing"
@@ -306,46 +305,21 @@ func buildRequests(t *testing.T, relativeURLRoot string) []testserver.TestReques
 	return requests
 }
 
-func TestRetryableHTTPFeatureToggle(t *testing.T) {
-	t.Run("retryable http off", func(t *testing.T) {
-		os.Setenv("FF_GITLAB_SHELL_RETRYABLE_HTTP", "0")
-		reqAttempts := 0
-		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			reqAttempts++
-			w.WriteHeader(500)
-		}))
-		defer srv.Close()
-
-		httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, defaultHttpOpts)
-		require.NoError(t, err)
-		require.NotNil(t, httpClient.HTTPClient)
-		require.Nil(t, httpClient.RetryableHTTP)
-		client, err := NewGitlabNetClient("", "", "", httpClient)
-		require.NoError(t, err)
-
-		_, err = client.Get(context.Background(), "/")
-		require.EqualError(t, err, "Internal API error (500)")
-		require.Equal(t, 1, reqAttempts)
-	})
-
-	t.Run("retryable http on", func(t *testing.T) {
-		os.Setenv("FF_GITLAB_SHELL_RETRYABLE_HTTP", "1")
-		reqAttempts := 0
-		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			reqAttempts++
-			w.WriteHeader(500)
-		}))
-		defer srv.Close()
-
-		httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, defaultHttpOpts)
-		require.NoError(t, err)
-		require.Nil(t, httpClient.HTTPClient)
-		require.NotNil(t, httpClient.RetryableHTTP)
-		client, err := NewGitlabNetClient("", "", "", httpClient)
-		require.NoError(t, err)
-
-		_, err = client.Get(context.Background(), "/")
-		require.EqualError(t, err, "Internal API unreachable")
-		require.Equal(t, 3, reqAttempts)
-	})
+func TestRetryOnFailure(t *testing.T) {
+	reqAttempts := 0
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		reqAttempts++
+		w.WriteHeader(500)
+	}))
+	defer srv.Close()
+
+	httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, defaultHttpOpts)
+	require.NoError(t, err)
+	require.NotNil(t, httpClient.RetryableHTTP)
+	client, err := NewGitlabNetClient("", "", "", httpClient)
+	require.NoError(t, err)
+
+	_, err = client.Get(context.Background(), "/")
+	require.EqualError(t, err, "Internal API unreachable")
+	require.Equal(t, 3, reqAttempts)
 }

client/gitlabnet.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"os"
 	"strings"
 	"time"
 
@@ -89,26 +88,7 @@ func appendPath(host string, path string) string {
 	return strings.TrimSuffix(host, "/") + "/" + strings.TrimPrefix(path, "/")
 }
 
-func newRequest(ctx context.Context, method, host, path string, data interface{}) (*http.Request, error) {
-	var jsonReader io.Reader
-	if data != nil {
-		jsonData, err := json.Marshal(data)
-		if err != nil {
-			return nil, err
-		}
-
-		jsonReader = bytes.NewReader(jsonData)
-	}
-
-	request, err := http.NewRequestWithContext(ctx, method, appendPath(host, path), jsonReader)
-	if err != nil {
-		return nil, err
-	}
-
-	return request, nil
-}
-
-func newRetryableRequest(ctx context.Context, method, host, path string, data interface{}) (*retryablehttp.Request, error) {
+func newRequest(ctx context.Context, method, host, path string, data interface{}) (*retryablehttp.Request, error) {
 	var jsonReader io.Reader
 	if data != nil {
 		jsonData, err := json.Marshal(data)
@@ -139,7 +119,6 @@ func parseError(resp *http.Response) error {
 	} else {
 		return &ApiError{parsedResponse.Message}
 	}
-
 }
 
 func (c *GitlabNetClient) Get(ctx context.Context, path string) (*http.Response, error) {
@@ -156,15 +135,9 @@ func (c *GitlabNetClient) DoRequest(ctx context.Context, method, path string, da
 		return nil, err
 	}
 
-	retryableRequest, err := newRetryableRequest(ctx, method, c.httpClient.Host, path, data)
-	if err != nil {
-		return nil, err
-	}
-
 	user, password := c.user, c.password
 	if user != "" && password != "" {
 		request.SetBasicAuth(user, password)
-		retryableRequest.SetBasicAuth(user, password)
 	}
 
 	claims := jwt.RegisteredClaims{
@@ -178,31 +151,19 @@ func (c *GitlabNetClient) DoRequest(ctx context.Context, method, path string, da
 		return nil, err
 	}
 	request.Header.Set(apiSecretHeaderName, tokenString)
-	retryableRequest.Header.Set(apiSecretHeaderName, tokenString)
 
 	originalRemoteIP, ok := ctx.Value(OriginalRemoteIPContextKey{}).(string)
 	if ok {
 		request.Header.Add("X-Forwarded-For", originalRemoteIP)
-		retryableRequest.Header.Add("X-Forwarded-For", originalRemoteIP)
 	}
 
 	request.Header.Add("Content-Type", "application/json")
-	retryableRequest.Header.Add("Content-Type", "application/json")
 	request.Header.Add("User-Agent", c.userAgent)
-	retryableRequest.Header.Add("User-Agent", c.userAgent)
 	request.Close = true
-	retryableRequest.Close = true
 
 	start := time.Now()
 
-	var response *http.Response
-	var respErr error
-	if c.httpClient.HTTPClient != nil {
-		response, respErr = c.httpClient.HTTPClient.Do(request)
-	}
-	if os.Getenv("FF_GITLAB_SHELL_RETRYABLE_HTTP") == "1" && c.httpClient.RetryableHTTP != nil {
-		response, respErr = c.httpClient.RetryableHTTP.Do(retryableRequest)
-	}
+	response, err := c.httpClient.RetryableHTTP.Do(request)
 	fields := log.Fields{
 		"method":      method,
 		"url":         request.URL.String(),
@@ -210,8 +171,8 @@ func (c *GitlabNetClient) DoRequest(ctx context.Context, method, path string, da
 	}
 	logger := log.WithContextFields(ctx, fields)
 
-	if respErr != nil {
-		logger.WithError(respErr).Error("Internal API unreachable")
+	if err != nil {
+		logger.WithError(err).Error("Internal API unreachable")
 		return nil, &ApiError{"Internal API unreachable"}
 	}
 

client/httpclient.go

@@ -32,7 +32,6 @@ const (
 var ErrCafileNotFound = errors.New("cafile not found")
 
 type HttpClient struct {
-	HTTPClient    *http.Client
 	RetryableHTTP *retryablehttp.Client
 	Host          string
 }
@@ -117,28 +116,15 @@ func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath stri
 		return nil, errors.New("unknown GitLab URL prefix")
 	}
 
-	c := &http.Client{
-		Transport: correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(transport)),
-		Timeout:   readTimeout(readTimeoutSeconds),
-	}
-
-	client := &HttpClient{HTTPClient: c, Host: host}
-
-	if os.Getenv("FF_GITLAB_SHELL_RETRYABLE_HTTP") == "1" {
-		c := retryablehttp.NewClient()
-		c.RetryMax = hcc.retryMax
-		c.RetryWaitMax = hcc.retryWaitMax
-		c.RetryWaitMin = hcc.retryWaitMin
-		c.Logger = nil
-		c.HTTPClient.Transport = correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(transport))
-		c.HTTPClient.Timeout = readTimeout(readTimeoutSeconds)
+	c := retryablehttp.NewClient()
+	c.RetryMax = hcc.retryMax
+	c.RetryWaitMax = hcc.retryWaitMax
+	c.RetryWaitMin = hcc.retryWaitMin
+	c.Logger = nil
+	c.HTTPClient.Transport = correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(transport))
+	c.HTTPClient.Timeout = readTimeout(readTimeoutSeconds)
 
-		client = &HttpClient{RetryableHTTP: c, Host: host}
-	}
-
-	if client.HTTPClient == nil && client.RetryableHTTP == nil {
-		panic("client/httpclient.go did not set http client")
-	}
+	client := &HttpClient{RetryableHTTP: c, Host: host}
 
 	return client, nil
 }

client/httpclient_test.go

@@ -21,13 +21,7 @@ func TestReadTimeout(t *testing.T) {
 	require.NoError(t, err)
 
 	require.NotNil(t, client)
-	if client.HTTPClient != nil {
-		require.Equal(t, time.Duration(expectedSeconds)*time.Second, client.HTTPClient.Timeout)
-	}
-
-	if client.RetryableHTTP != nil {
-		require.Equal(t, time.Duration(expectedSeconds)*time.Second, client.RetryableHTTP.HTTPClient.Timeout)
-	}
+	require.Equal(t, time.Duration(expectedSeconds)*time.Second, client.RetryableHTTP.HTTPClient.Timeout)
 }
 
 const (

client/testserver/testserver.go

@@ -60,7 +60,6 @@ func StartHttpServer(t *testing.T, handlers []TestRequestHandler) string {
 }
 
 func StartRetryHttpServer(t *testing.T, handlers []TestRequestHandler) string {
-	os.Setenv("FF_GITLAB_SHELL_RETRYABLE_HTTP", "1")
 	attempts := map[string]int{}
 
 	retryMiddileware := func(next func(w http.ResponseWriter, r *http.Request)) http.Handler {

internal/command/discover/discover_test.go

@@ -16,33 +16,31 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/config"
 )
 
-var (
-	requests = []testserver.TestRequestHandler{
-		{
-			Path: "/api/v4/internal/discover",
-			Handler: func(w http.ResponseWriter, r *http.Request) {
-				if r.URL.Query().Get("key_id") == "1" || r.URL.Query().Get("username") == "alex-doe" {
-					body := map[string]interface{}{
-						"id":       2,
-						"username": "alex-doe",
-						"name":     "Alex Doe",
-					}
-					json.NewEncoder(w).Encode(body)
-				} else if r.URL.Query().Get("username") == "broken_message" {
-					body := map[string]string{
-						"message": "Forbidden!",
-					}
-					w.WriteHeader(http.StatusForbidden)
-					json.NewEncoder(w).Encode(body)
-				} else if r.URL.Query().Get("username") == "broken" {
-					w.WriteHeader(http.StatusInternalServerError)
-				} else {
-					fmt.Fprint(w, "null")
+var requests = []testserver.TestRequestHandler{
+	{
+		Path: "/api/v4/internal/discover",
+		Handler: func(w http.ResponseWriter, r *http.Request) {
+			if r.URL.Query().Get("key_id") == "1" || r.URL.Query().Get("username") == "alex-doe" {
+				body := map[string]interface{}{
+					"id":       2,
+					"username": "alex-doe",
+					"name":     "Alex Doe",
+				}
+				json.NewEncoder(w).Encode(body)
+			} else if r.URL.Query().Get("username") == "broken_message" {
+				body := map[string]string{
+					"message": "Forbidden!",
 				}
-			},
+				w.WriteHeader(http.StatusForbidden)
+				json.NewEncoder(w).Encode(body)
+			} else if r.URL.Query().Get("username") == "broken" {
+				w.WriteHeader(http.StatusInternalServerError)
+			} else {
+				fmt.Fprint(w, "null")
+			}
 		},
-	}
-)
+	},
+}
 
 func TestExecute(t *testing.T) {
 	url := testserver.StartSocketHttpServer(t, requests)
@@ -112,7 +110,7 @@ func TestFailingExecute(t *testing.T) {
 		{
 			desc:          "When the API fails",
 			arguments:     &commandargs.Shell{GitlabUsername: "broken"},
-			expectedError: "Failed to get username: Internal API error (500)",
+			expectedError: "Failed to get username: Internal API unreachable",
 		},
 	}
 

internal/command/healthcheck/healthcheck_test.go

@@ -84,5 +84,5 @@ func TestFailingAPIExecute(t *testing.T) {
 
 	err := cmd.Execute(context.Background())
 	require.Empty(t, buffer.String())
-	require.EqualError(t, err, "Internal API available: FAILED - Internal API error (500)")
+	require.EqualError(t, err, "Internal API available: FAILED - Internal API unreachable")
 }

internal/command/personalaccesstoken/personalaccesstoken_test.go

@@ -17,9 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/personalaccesstoken"
 )
 
-var (
-	requests []testserver.TestRequestHandler
-)
+var requests []testserver.TestRequestHandler
 
 func setup(t *testing.T) {
 	requests = []testserver.TestRequestHandler{
@@ -147,7 +145,7 @@ func TestExecute(t *testing.T) {
 				GitlabKeyId: "broken",
 				SshArgs:     []string{cmdname, "newtoken", "read_api,read_repository"},
 			},
-			expectedError: "Internal API error (500)",
+			expectedError: "Internal API unreachable",
 		},
 		{
 			desc: "Without KeyID or User",

internal/command/twofactorrecover/twofactorrecover_test.go

@@ -18,9 +18,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/twofactorrecover"
 )
 
-var (
-	requests []testserver.TestRequestHandler
-)
+var requests []testserver.TestRequestHandler
 
 func setup(t *testing.T) {
 	requests = []testserver.TestRequestHandler{
@@ -99,7 +97,7 @@ func TestExecute(t *testing.T) {
 			desc:           "With API fails",
 			arguments:      &commandargs.Shell{GitlabKeyId: "broken"},
 			answer:         "yes\n",
-			expectedOutput: question + errorHeader + "Internal API error (500)\n",
+			expectedOutput: question + errorHeader + "Internal API unreachable\n",
 		},
 		{
 			desc:           "With missing arguments",

internal/command/twofactorverify/twofactorverify_test.go

@@ -136,7 +136,7 @@ func TestExecute(t *testing.T) {
 		{
 			desc:           "With API fails",
 			arguments:      &commandargs.Shell{GitlabKeyId: "broken"},
-			expectedOutput: errorHeader + "Internal API error (500)\n",
+			expectedOutput: errorHeader + "Internal API unreachable\n",
 		},
 		{
 			desc:           "With missing arguments",

internal/config/config.go

@@ -142,14 +142,8 @@ func (c *Config) HttpClient() (*client.HttpClient, error) {
 			return
 		}
 
-		if client.HTTPClient != nil {
-			tr := client.HTTPClient.Transport
-			client.HTTPClient.Transport = metrics.NewRoundTripper(tr)
-		}
-		if os.Getenv("FF_GITLAB_SHELL_RETRYABLE_HTTP") == "1" && client.RetryableHTTP != nil {
-			tr := client.RetryableHTTP.HTTPClient.Transport
-			client.RetryableHTTP.HTTPClient.Transport = metrics.NewRoundTripper(tr)
-		}
+		tr := client.RetryableHTTP.HTTPClient.Transport
+		client.RetryableHTTP.HTTPClient.Transport = metrics.NewRoundTripper(tr)
 
 		c.httpClient = client
 	})

internal/config/config_test.go

@@ -28,48 +28,38 @@ func TestConfigApplyGlobalState(t *testing.T) {
 }
 
 func TestCustomPrometheusMetrics(t *testing.T) {
-	for _, ffValue := range []string{"0", "1"} {
-		t.Run("FF_GITLAB_SHELL_RETRYABLE_HTTP="+ffValue, func(t *testing.T) {
-			os.Setenv("FF_GITLAB_SHELL_RETRYABLE_HTTP", ffValue)
-			url := testserver.StartHttpServer(t, []testserver.TestRequestHandler{})
+	url := testserver.StartHttpServer(t, []testserver.TestRequestHandler{})
 
-			config := &Config{GitlabUrl: url}
-			client, err := config.HttpClient()
-			require.NoError(t, err)
+	config := &Config{GitlabUrl: url}
+	client, err := config.HttpClient()
+	require.NoError(t, err)
 
-			if client.HTTPClient != nil {
-				_, err = client.HTTPClient.Get(url)
-				require.NoError(t, err)
-			}
+	if client.RetryableHTTP != nil {
+		_, err = client.RetryableHTTP.Get(url)
+		require.NoError(t, err)
+	}
 
-			if os.Getenv("FF_GITLAB_SHELL_RETRYABLE_HTTP") == "1" && client.RetryableHTTP != nil {
-				_, err = client.RetryableHTTP.Get(url)
-				require.NoError(t, err)
-			}
+	ms, err := prometheus.DefaultGatherer.Gather()
+	require.NoError(t, err)
 
-			ms, err := prometheus.DefaultGatherer.Gather()
-			require.NoError(t, err)
+	var actualNames []string
+	for _, m := range ms[0:9] {
+		actualNames = append(actualNames, m.GetName())
+	}
 
-			var actualNames []string
-			for _, m := range ms[0:9] {
-				actualNames = append(actualNames, m.GetName())
-			}
-
-			expectedMetricNames := []string{
-				"gitlab_shell_http_in_flight_requests",
-				"gitlab_shell_http_request_duration_seconds",
-				"gitlab_shell_http_requests_total",
-				"gitlab_shell_sshd_concurrent_limited_sessions_total",
-				"gitlab_shell_sshd_in_flight_connections",
-				"gitlab_shell_sshd_session_duration_seconds",
-				"gitlab_shell_sshd_session_established_duration_seconds",
-				"gitlab_sli:shell_sshd_sessions:errors_total",
-				"gitlab_sli:shell_sshd_sessions:total",
-			}
-
-			require.Equal(t, expectedMetricNames, actualNames)
-		})
+	expectedMetricNames := []string{
+		"gitlab_shell_http_in_flight_requests",
+		"gitlab_shell_http_request_duration_seconds",
+		"gitlab_shell_http_requests_total",
+		"gitlab_shell_sshd_concurrent_limited_sessions_total",
+		"gitlab_shell_sshd_in_flight_connections",
+		"gitlab_shell_sshd_session_duration_seconds",
+		"gitlab_shell_sshd_session_established_duration_seconds",
+		"gitlab_sli:shell_sshd_sessions:errors_total",
+		"gitlab_sli:shell_sshd_sessions:total",
 	}
+
+	require.Equal(t, expectedMetricNames, actualNames)
 }
 
 func TestNewFromDir(t *testing.T) {

internal/gitlabnet/lfsauthenticate/client_test.go

@@ -74,7 +74,7 @@ func TestFailedRequests(t *testing.T) {
 		{
 			desc:           "With API fails",
 			args:           &commandargs.Shell{GitlabKeyId: "broken", CommandType: commandargs.LfsAuthenticate, SshArgs: []string{"git-lfs-authenticate", repo, "download"}},
-			expectedOutput: "Internal API error (500)",
+			expectedOutput: "Internal API unreachable",
 		},
 	}
 

spec/gitlab_shell_discover_spec.rb

@@ -124,12 +124,7 @@ describe 'bin/gitlab-shell' do
     it 'returns an error message when the API call fails without a message' do
       _, stderr, status = run!(["-c/usr/share/webapps/gitlab-shell/bin/gitlab-shell", "username-broken"])
 
-      stderr_output = if ENV['FF_GITLAB_SHELL_RETRYABLE_HTTP'] == '1'
-                        /Failed to get username: Internal API unreachable/
-                      else
-                        /Failed to get username: Internal API error \(500\)/
-                      end
-      expect(stderr).to match(stderr_output)
+      expect(stderr).to match(/Failed to get username: Internal API unreachable/)
       expect(status).not_to be_success
     end
   end