feat: put retryablehttp.Client behind feature flag

What
---
- Update the `client.HttpClient` fields to have `http.Client` and
  `retryablehttp.Client`, one of them will be `nil` depending on the
  feature flag toggle.
- Create new method `newRetryableRequest` which will create a
  `retryablehttp.Request` and use that if the
  `FF_GITLAB_SHELL_RETRYABLE_HTTP` feature flag is turned on.
- Add checks for `FF_GITLAB_SHELL_RETRYABLE_HTTP` everywhere we use the
  http client to use the `retryablehttp.Client` or the default
  `http.Client`
- New job `tests-integration-retryableHttp` to run the integraiton tests
  with the new retryablehttp client. We didn't update go tests because
  some assertions are different and will break table driven tests.

Why
---
As discussed in
https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/703#note_1229645097
we want to put the client behind a feature flag, not just the retry
logic. This does bring extra risk for accessing a `nil` field but there
should be checks everytime we access `RetryableHTTP` and `HTTPClient`.

Reference: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979


Signed-off-by: Steve Azzopardi <sazzopardi@gitlab.com>

.gitlab-ci.yml

@@ -64,6 +64,13 @@ 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

@@ -315,11 +315,13 @@ func TestRetryableHTTPFeatureToggle(t *testing.T) {
 
 		httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, nil)
 		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 unreachable")
+		require.EqualError(t, err, "Internal API error (500)")
 		require.Equal(t, 1, reqAttempts)
 	})
 
@@ -334,6 +336,8 @@ func TestRetryableHTTPFeatureToggle(t *testing.T) {
 
 		httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, nil)
 		require.NoError(t, err)
+		require.Nil(t, httpClient.HTTPClient)
+		require.NotNil(t, httpClient.RetryableHTTP)
 		client, err := NewGitlabNetClient("", "", "", httpClient)
 		require.NoError(t, err)
 

client/gitlabnet.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"os"
 	"strings"
 	"time"
 
@@ -54,7 +55,6 @@ func NewGitlabNetClient(
 	secret string,
 	httpClient *HttpClient,
 ) (*GitlabNetClient, error) {
-
 	if httpClient == nil {
 		return nil, fmt.Errorf("Unsupported protocol")
 	}
@@ -89,7 +89,26 @@ 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{}) (*retryablehttp.Request, error) {
+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) {
 	var jsonReader io.Reader
 	if data != nil {
 		jsonData, err := json.Marshal(data)
@@ -137,9 +156,15 @@ 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{
@@ -153,18 +178,31 @@ 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()
-	response, err := c.httpClient.Do(request)
+
+	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)
+	}
 	fields := log.Fields{
 		"method":      method,
 		"url":         request.URL.String(),
@@ -172,8 +210,8 @@ func (c *GitlabNetClient) DoRequest(ctx context.Context, method, path string, da
 	}
 	logger := log.WithContextFields(ctx, fields)
 
-	if err != nil {
-		logger.WithError(err).Error("Internal API unreachable")
+	if respErr != nil {
+		logger.WithError(respErr).Error("Internal API unreachable")
 		return nil, &ApiError{"Internal API unreachable"}
 	}
 

client/httpclient.go

@@ -29,8 +29,9 @@ const (
 var ErrCafileNotFound = errors.New("cafile not found")
 
 type HttpClient struct {
-	*retryablehttp.Client
-	Host string
+	HTTPClient    *http.Client
+	RetryableHTTP *retryablehttp.Client
+	Host          string
 }
 
 type httpClientCfg struct {
@@ -100,17 +101,27 @@ func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath stri
 		return nil, errors.New("unknown GitLab URL prefix")
 	}
 
-	c := retryablehttp.NewClient()
-	c.RetryMax = 0                                          // Retry logic is disabled by default
-	if os.Getenv("FF_GITLAB_SHELL_RETRYABLE_HTTP") == "1" { // Feature toggle for enabling retries on GitLab client.
+	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 = 2
 		c.RetryWaitMax = 15 * time.Second
+		c.Logger = nil
+		c.HTTPClient.Transport = correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(transport))
+		c.HTTPClient.Timeout = readTimeout(readTimeoutSeconds)
+
+		client = &HttpClient{RetryableHTTP: c, Host: host}
 	}
-	c.Logger = nil
-	c.HTTPClient.Transport = correlation.NewInstrumentedRoundTripper(tracing.NewRoundTripper(transport))
-	c.HTTPClient.Timeout = readTimeout(readTimeoutSeconds)
 
-	client := &HttpClient{Client: c, Host: host}
+	if client.HTTPClient == nil && client.RetryableHTTP == nil {
+		panic("client/httpclient.go did not set http client")
+	}
 
 	return client, nil
 }

client/httpclient_test.go

@@ -21,7 +21,13 @@ func TestReadTimeout(t *testing.T) {
 	require.NoError(t, err)
 
 	require.NotNil(t, client)
-	require.Equal(t, time.Duration(expectedSeconds)*time.Second, client.HTTPClient.Timeout)
+	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)
+	}
 }
 
 const (
@@ -117,7 +123,6 @@ func TestRequestWithUserAgent(t *testing.T) {
 	client.SetUserAgent(gitalyUserAgent)
 	_, err = client.Get(context.Background(), "/override_user_agent")
 	require.NoError(t, err)
-
 }
 
 func setup(t *testing.T, username, password string, requests []testserver.TestRequestHandler) *GitlabNetClient {

internal/command/discover/discover_test.go

@@ -16,31 +16,33 @@ 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!",
+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")
 				}
-				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)
@@ -110,7 +112,7 @@ func TestFailingExecute(t *testing.T) {
 		{
 			desc:          "When the API fails",
 			arguments:     &commandargs.Shell{GitlabUsername: "broken"},
-			expectedError: "Failed to get username: Internal API unreachable",
+			expectedError: "Failed to get username: Internal API error (500)",
 		},
 	}
 

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 unreachable")
+	require.EqualError(t, err, "Internal API available: FAILED - Internal API error (500)")
 }

internal/command/personalaccesstoken/personalaccesstoken_test.go

@@ -17,7 +17,9 @@ 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{
@@ -145,7 +147,7 @@ func TestExecute(t *testing.T) {
 				GitlabKeyId: "broken",
 				SshArgs:     []string{cmdname, "newtoken", "read_api,read_repository"},
 			},
-			expectedError: "Internal API unreachable",
+			expectedError: "Internal API error (500)",
 		},
 		{
 			desc: "Without KeyID or User",

internal/command/twofactorrecover/twofactorrecover_test.go

@@ -18,7 +18,9 @@ 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{
@@ -97,7 +99,7 @@ func TestExecute(t *testing.T) {
 			desc:           "With API fails",
 			arguments:      &commandargs.Shell{GitlabKeyId: "broken"},
 			answer:         "yes\n",
-			expectedOutput: question + errorHeader + "Internal API unreachable\n",
+			expectedOutput: question + errorHeader + "Internal API error (500)\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 unreachable\n",
+			expectedOutput: errorHeader + "Internal API error (500)\n",
 		},
 		{
 			desc:           "With missing arguments",

internal/config/config.go

@@ -134,8 +134,14 @@ func (c *Config) HttpClient() (*client.HttpClient, error) {
 			return
 		}
 
-		tr := client.HTTPClient.Transport
-		client.HTTPClient.Transport = metrics.NewRoundTripper(tr)
+		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)
+		}
 
 		c.httpClient = client
 	})

internal/config/config_test.go

@@ -28,36 +28,48 @@ func TestConfigApplyGlobalState(t *testing.T) {
 }
 
 func TestCustomPrometheusMetrics(t *testing.T) {
-	url := testserver.StartHttpServer(t, []testserver.TestRequestHandler{})
+	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{})
 
-	config := &Config{GitlabUrl: url}
-	client, err := config.HttpClient()
-	require.NoError(t, err)
+			config := &Config{GitlabUrl: url}
+			client, err := config.HttpClient()
+			require.NoError(t, err)
 
-	_, err = client.Get(url)
-	require.NoError(t, err)
+			if client.HTTPClient != nil {
+				_, err = client.HTTPClient.Get(url)
+				require.NoError(t, err)
+			}
 
-	ms, err := prometheus.DefaultGatherer.Gather()
-	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)
+			}
 
-	var actualNames []string
-	for _, m := range ms[0:9] {
-		actualNames = append(actualNames, m.GetName())
-	}
+			ms, err := prometheus.DefaultGatherer.Gather()
+			require.NoError(t, err)
 
-	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",
+			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)
+		})
 	}
-
-	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 unreachable",
+			expectedOutput: "Internal API error (500)",
 		},
 	}
 

spec/gitlab_shell_discover_spec.rb

@@ -124,7 +124,12 @@ 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"])
 
-      expect(stderr).to match(/Failed to get username: Internal API unreachable/)
+      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(status).not_to be_success
     end
   end