From 80f684e48eca2bf1ef2006d84f8c49bec7104344 Mon Sep 17 00:00:00 2001 From: Steve Azzopardi Date: Mon, 30 Jan 2023 09:21:17 +0100 Subject: feat: make retryable http default client What --- Make the retryableHTTP client introduced in https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/703 the default HTTP client. Why --- In https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979#note_1254964426 we've seen a 99% error reduction on `git` commands from `gitlab-shell` when the retryableHTTP client is used. This has been running in production for over 2 weeks in `us-east1-b` and 5 days fleet-wide so we should be confident that this client works as expected. Reference: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7979 Signed-off-by: Steve Azzopardi --- .gitlab-ci.yml | 7 --- client/client_test.go | 60 ++++++--------------- client/gitlabnet.go | 47 ++-------------- client/httpclient.go | 30 +++-------- client/httpclient_test.go | 8 +-- client/testserver/testserver.go | 1 - internal/command/discover/discover_test.go | 50 +++++++++-------- internal/command/healthcheck/healthcheck_test.go | 2 +- .../personalaccesstoken_test.go | 6 +-- .../twofactorrecover/twofactorrecover_test.go | 6 +-- .../twofactorverify/twofactorverify_test.go | 2 +- internal/config/config.go | 10 +--- internal/config/config_test.go | 62 +++++++++------------- internal/gitlabnet/lfsauthenticate/client_test.go | 2 +- spec/gitlab_shell_discover_spec.rb | 7 +-- 15 files changed, 90 insertions(+), 210 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 2b43481..f06353e 100644 --- a/.gitlab-ci.yml +++ b/.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: diff --git a/client/client_test.go b/client/client_test.go index e48bb67..bf27ef9 100644 --- a/client/client_test.go +++ b/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) } diff --git a/client/gitlabnet.go b/client/gitlabnet.go index 38adf2a..352196e 100644 --- a/client/gitlabnet.go +++ b/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"} } diff --git a/client/httpclient.go b/client/httpclient.go index 82b4b40..9b57add 100644 --- a/client/httpclient.go +++ b/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 } diff --git a/client/httpclient_test.go b/client/httpclient_test.go index ee2c6fd..b893853 100644 --- a/client/httpclient_test.go +++ b/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 ( diff --git a/client/testserver/testserver.go b/client/testserver/testserver.go index 105e7a0..7779f29 100644 --- a/client/testserver/testserver.go +++ b/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 { diff --git a/internal/command/discover/discover_test.go b/internal/command/discover/discover_test.go index ccbfa7e..df9ca47 100644 --- a/internal/command/discover/discover_test.go +++ b/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", }, } diff --git a/internal/command/healthcheck/healthcheck_test.go b/internal/command/healthcheck/healthcheck_test.go index f642226..12a8444 100644 --- a/internal/command/healthcheck/healthcheck_test.go +++ b/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") } diff --git a/internal/command/personalaccesstoken/personalaccesstoken_test.go b/internal/command/personalaccesstoken/personalaccesstoken_test.go index daa5834..492f745 100644 --- a/internal/command/personalaccesstoken/personalaccesstoken_test.go +++ b/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", diff --git a/internal/command/twofactorrecover/twofactorrecover_test.go b/internal/command/twofactorrecover/twofactorrecover_test.go index 81b4dff..7e20a06 100644 --- a/internal/command/twofactorrecover/twofactorrecover_test.go +++ b/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", diff --git a/internal/command/twofactorverify/twofactorverify_test.go b/internal/command/twofactorverify/twofactorverify_test.go index 640d3ed..213c025 100644 --- a/internal/command/twofactorverify/twofactorverify_test.go +++ b/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", diff --git a/internal/config/config.go b/internal/config/config.go index 207517b..cfee3d0 100644 --- a/internal/config/config.go +++ b/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 }) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 4d0531b..d371033 100644 --- a/internal/config/config_test.go +++ b/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) { diff --git a/internal/gitlabnet/lfsauthenticate/client_test.go b/internal/gitlabnet/lfsauthenticate/client_test.go index f62d8e1..01b3f84 100644 --- a/internal/gitlabnet/lfsauthenticate/client_test.go +++ b/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", }, } diff --git a/spec/gitlab_shell_discover_spec.rb b/spec/gitlab_shell_discover_spec.rb index fe18b53..07a9be1 100644 --- a/spec/gitlab_shell_discover_spec.rb +++ b/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 -- cgit v1.2.1