diff options
author | Steve Azzopardi <sazzopardi@gitlab.com> | 2023-01-06 19:35:21 +0100 |
---|---|---|
committer | Ash McKenzie <amckenzie@gitlab.com> | 2023-01-12 02:56:43 +0000 |
commit | 16a5c84310f6bb1790f16b6b2e4df90af493b07c (patch) | |
tree | 5429ecdcc2996b3cf13beddea23671358fca54d3 | |
parent | a093c9d3cfc1ee18368ebbf828dc61c15b74540c (diff) | |
download | gitlab-shell-16a5c84310f6bb1790f16b6b2e4df90af493b07c.tar.gz |
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>
-rw-r--r-- | .gitlab-ci.yml | 7 | ||||
-rw-r--r-- | client/client_test.go | 6 | ||||
-rw-r--r-- | client/gitlabnet.go | 48 | ||||
-rw-r--r-- | client/httpclient.go | 29 | ||||
-rw-r--r-- | client/httpclient_test.go | 9 | ||||
-rw-r--r-- | internal/command/discover/discover_test.go | 50 | ||||
-rw-r--r-- | internal/command/healthcheck/healthcheck_test.go | 2 | ||||
-rw-r--r-- | internal/command/personalaccesstoken/personalaccesstoken_test.go | 6 | ||||
-rw-r--r-- | internal/command/twofactorrecover/twofactorrecover_test.go | 6 | ||||
-rw-r--r-- | internal/command/twofactorverify/twofactorverify_test.go | 2 | ||||
-rw-r--r-- | internal/config/config.go | 10 | ||||
-rw-r--r-- | internal/config/config_test.go | 60 | ||||
-rw-r--r-- | internal/gitlabnet/lfsauthenticate/client_test.go | 2 | ||||
-rw-r--r-- | spec/gitlab_shell_discover_spec.rb | 7 |
14 files changed, 169 insertions, 75 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index f06353e..2b43481 100644 --- a/.gitlab-ci.yml +++ b/.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: diff --git a/client/client_test.go b/client/client_test.go index 776a0aa..9bea4fd 100644 --- a/client/client_test.go +++ b/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) diff --git a/client/gitlabnet.go b/client/gitlabnet.go index 02bef8c..38adf2a 100644 --- a/client/gitlabnet.go +++ b/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"} } diff --git a/client/httpclient.go b/client/httpclient.go index 47486a0..fe6e2e5 100644 --- a/client/httpclient.go +++ b/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 } diff --git a/client/httpclient_test.go b/client/httpclient_test.go index a5de47c..ee2c6fd 100644 --- a/client/httpclient_test.go +++ b/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 { diff --git a/internal/command/discover/discover_test.go b/internal/command/discover/discover_test.go index df9ca47..ccbfa7e 100644 --- a/internal/command/discover/discover_test.go +++ b/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)", }, } diff --git a/internal/command/healthcheck/healthcheck_test.go b/internal/command/healthcheck/healthcheck_test.go index 12a8444..f642226 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 unreachable") + require.EqualError(t, err, "Internal API available: FAILED - Internal API error (500)") } diff --git a/internal/command/personalaccesstoken/personalaccesstoken_test.go b/internal/command/personalaccesstoken/personalaccesstoken_test.go index 492f745..daa5834 100644 --- a/internal/command/personalaccesstoken/personalaccesstoken_test.go +++ b/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", diff --git a/internal/command/twofactorrecover/twofactorrecover_test.go b/internal/command/twofactorrecover/twofactorrecover_test.go index 7e20a06..81b4dff 100644 --- a/internal/command/twofactorrecover/twofactorrecover_test.go +++ b/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", diff --git a/internal/command/twofactorverify/twofactorverify_test.go b/internal/command/twofactorverify/twofactorverify_test.go index 213c025..640d3ed 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 unreachable\n", + expectedOutput: errorHeader + "Internal API error (500)\n", }, { desc: "With missing arguments", diff --git a/internal/config/config.go b/internal/config/config.go index 7f473c5..b52f6f7 100644 --- a/internal/config/config.go +++ b/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 }) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index c5fc219..4d0531b 100644 --- a/internal/config/config_test.go +++ b/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) { diff --git a/internal/gitlabnet/lfsauthenticate/client_test.go b/internal/gitlabnet/lfsauthenticate/client_test.go index 01b3f84..f62d8e1 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 unreachable", + expectedOutput: "Internal API error (500)", }, } diff --git a/spec/gitlab_shell_discover_spec.rb b/spec/gitlab_shell_discover_spec.rb index 07a9be1..fe18b53 100644 --- a/spec/gitlab_shell_discover_spec.rb +++ b/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 |