diff --git a/config/302-pac-configmap.yaml b/config/302-pac-configmap.yaml index 8c965b692e..db4a243d2f 100644 --- a/config/302-pac-configmap.yaml +++ b/config/302-pac-configmap.yaml @@ -160,6 +160,27 @@ data: # Default: false require-ok-to-test-sha: "false" + # Enable retrying Git provider API requests when hitting rate limits (429, + # or 403 with rate limit headers on GitHub) or transient server errors. + # Retries use exponential backoff with jitter and honor the Retry-After and + # X-RateLimit-Reset headers, giving up when the reset is further away than + # api-retry-max-wait-seconds. + # When false, the GitLab client keeps the retry behaviour of the upstream + # GitLab Go client and the GitHub client does not retry. + # Default: false + enable-api-retry: "false" + + # Maximum number of attempts (initial request included) when enable-api-retry + # is true. Values lower than 1 fall back to the default. + # Default: 4 + api-retry-max-attempts: "4" + + # Maximum time in seconds to wait between retries when enable-api-retry is + # true. If the provider rate limit reset is further away than this, the + # request fails instead of blocking. + # Default: 120 + api-retry-max-wait-seconds: "120" + # When enabled, this option prevents duplicate pipeline runs when a commit appears in # both a push event and a pull request. If a push event comes from a commit that is # part of an open pull request, the push event will be skipped as it would create diff --git a/docs/content/docs/api/configmap.md b/docs/content/docs/api/configmap.md index 7d4971b40f..5ebb206bfa 100644 --- a/docs/content/docs/api/configmap.md +++ b/docs/content/docs/api/configmap.md @@ -339,6 +339,72 @@ skip-push-event-for-pr-commits: "true" {{< /param >}} +### API Retry + +{{< tech_preview "Provider API Retry for GitHub and GitLab" >}} + +{{< param name="enable-api-retry" type="boolean" default="false" id="param-enable-api-retry" >}} +Enables retrying GitHub and GitLab API requests when Pipelines-as-Code +encounters a temporary provider failure. This includes rate limits, temporary +server errors, and selected network failures. + +Retries use backoff with jitter so multiple requests do not all retry at the +same time. When the provider supplies a retry or reset time, Pipelines-as-Code +uses that information as long as it does not exceed +`api-retry-max-wait-seconds`. + +The setting is disabled by default. Enabling it affects API operations made +while processing an event, including temporary clients and GitHub App setup. + +When disabled, the GitLab client keeps the retry behaviour built into the +upstream GitLab Go client, and the GitHub client performs no retries. + +Pipelines-as-Code only repeats an operation when it can do so safely. It does +not repeat provider changes after an uncertain network or server failure when +doing so could create duplicate comments, statuses, or other mutations. + +```yaml +enable-api-retry: "false" +``` + +{{< /param >}} + +{{< param name="api-retry-max-attempts" type="integer" default="4" id="param-api-retry-max-attempts" >}} +Sets the maximum number of attempts when `enable-api-retry` is `true`. The +initial request counts as the first attempt. For example, a value of `4` +allows the initial request followed by up to three retries. + +```yaml +api-retry-max-attempts: "4" +``` + +{{< /param >}} + +{{< param name="api-retry-max-wait-seconds" type="integer" default="120" id="param-api-retry-max-wait-seconds" >}} +Sets the maximum time in seconds that Pipelines-as-Code waits between +attempts. If a provider asks the client to wait longer than this value, +Pipelines-as-Code stops retrying rather than holding the event for a long +cooling period. + +```yaml +api-retry-max-wait-seconds: "120" +``` + +{{< /param >}} + +#### Failure reporting and limitations + +If all attempts fail, Pipelines-as-Code reports the original provider error +through its normal logs, events, and provider status handling. Reporting a +failure status to GitHub or GitLab is best effort because a fully exhausted or +unavailable provider API might also reject the status update. + +These settings are intended for short, temporary provider failures. They do +not provide persistent queueing for long rate-limit windows and do not control +how quickly a large backlog of PipelineRuns is admitted to the cluster. +Long-duration queueing and workload admission should be handled separately at +the pipeline or platform level. + ## Complete Example ```yaml diff --git a/go.mod b/go.mod index 33f7295f62..3df3138717 100644 --- a/go.mod +++ b/go.mod @@ -17,6 +17,7 @@ require ( github.com/google/go-github/v85 v85.0.0 github.com/hako/durafmt v0.0.0-20210608085754-5c1018a4e16b github.com/jenkins-x/go-scm v1.15.32 + github.com/hashicorp/go-retryablehttp v0.7.8 github.com/jonboulle/clockwork v0.5.0 github.com/juju/ansiterm v1.0.0 github.com/ktrysmt/go-bitbucket v0.10.0 @@ -127,7 +128,6 @@ require ( github.com/google/uuid v1.6.0 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.29.0 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect - github.com/hashicorp/go-retryablehttp v0.7.8 // indirect github.com/hashicorp/go-version v1.9.0 // indirect github.com/hashicorp/golang-lru v1.0.2 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect diff --git a/pkg/adapter/incoming.go b/pkg/adapter/incoming.go index d3280fd249..441ac7e06e 100644 --- a/pkg/adapter/incoming.go +++ b/pkg/adapter/incoming.go @@ -176,6 +176,10 @@ func (l *listener) detectIncoming(ctx context.Context, event *info.Event, req *h if repo.Spec.GitProvider == nil || repo.Spec.GitProvider.Type == "" { gh := github.New() gh.Run = l.run + if l.run.Info.Pac != nil { + pacInfo := l.run.Info.GetPacOpts() + gh.SetPacInfo(&pacInfo) + } ns := info.GetNS(ctx) ip := app.NewInstallation(req, l.run, repo, gh, ns) enterpriseURL, token, installationID, err := ip.GetAndUpdateInstallationID(ctx) diff --git a/pkg/params/settings/config.go b/pkg/params/settings/config.go index a0bac8d564..56813114da 100644 --- a/pkg/params/settings/config.go +++ b/pkg/params/settings/config.go @@ -25,6 +25,13 @@ const ( CustomConsoleNamespaceURLKey = "custom-console-url-namespace" SecretGhAppTokenRepoScopedKey = "secret-github-app-token-scoped" //nolint: gosec + + // DefaultAPIRetryMaxAttempts is the fallback number of attempts (initial + // request included) used when api-retry-max-attempts is unset or invalid. + DefaultAPIRetryMaxAttempts = 4 + // DefaultAPIRetryMaxWaitSeconds is the fallback cap on the wait between + // attempts used when api-retry-max-wait-seconds is unset or invalid. + DefaultAPIRetryMaxWaitSeconds = 120 ) var ( @@ -78,6 +85,12 @@ type Settings struct { RememberOKToTest bool `json:"remember-ok-to-test"` RequireOkToTestSHA bool `json:"require-ok-to-test-sha"` + // Retry Git provider API requests on rate limits and transient errors. + // Disabled by default. + EnableAPIRetry bool `default:"false" json:"enable-api-retry"` + APIRetryMaxAttempts int `default:"4" json:"api-retry-max-attempts"` + APIRetryMaxWaitSeconds int `default:"120" json:"api-retry-max-wait-seconds"` + // Tracing label names. Defaults in config/302-pac-configmap.yaml. TracingLabelAction string `json:"tracing-label-action"` TracingLabelApplication string `json:"tracing-label-application"` diff --git a/pkg/params/settings/config_test.go b/pkg/params/settings/config_test.go index e8e4cdaa09..3f9db9338e 100644 --- a/pkg/params/settings/config_test.go +++ b/pkg/params/settings/config_test.go @@ -48,6 +48,8 @@ func TestSyncConfig(t *testing.T) { CustomConsolePRTaskLog: "", CustomConsoleNamespaceURL: "", RememberOKToTest: false, + APIRetryMaxAttempts: 4, + APIRetryMaxWaitSeconds: 120, }, }, { @@ -109,6 +111,8 @@ func TestSyncConfig(t *testing.T) { CustomConsoleNamespaceURL: "https://custom-console-namespace", RememberOKToTest: false, RequireOkToTestSHA: true, + APIRetryMaxAttempts: 4, + APIRetryMaxWaitSeconds: 120, }, }, { diff --git a/pkg/provider/github/app/token.go b/pkg/provider/github/app/token.go index 8d47adbcbf..06786c3533 100644 --- a/pkg/provider/github/app/token.go +++ b/pkg/provider/github/app/token.go @@ -74,7 +74,7 @@ func (ip *Install) GetAndUpdateInstallationID(ctx context.Context) (string, stri } } - client, _, _, err := github.MakeClient(ctx, apiURL, jwtToken) + client, _, _, err := ip.ghClient.MakeClient(ctx, apiURL, jwtToken) if err != nil { return "", "", 0, err } diff --git a/pkg/provider/github/github.go b/pkg/provider/github/github.go index ac135588f8..14ba3fdceb 100644 --- a/pkg/provider/github/github.go +++ b/pkg/provider/github/github.go @@ -26,6 +26,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" + "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/retryhttp" "go.uber.org/zap" "golang.org/x/oauth2" "k8s.io/client-go/kubernetes" @@ -120,6 +121,19 @@ func (v *Provider) SetPacInfo(pacInfo *info.PacOpts) { v.pacInfo = pacInfo } +// retryOptions returns retry transport options built from the pac settings, +// or nil when API retries are disabled (the default). +func (v *Provider) retryOptions() *retryhttp.Options { + if v.pacInfo == nil || !v.pacInfo.EnableAPIRetry { + return nil + } + return &retryhttp.Options{ + MaxAttempts: v.pacInfo.APIRetryMaxAttempts, + MaxWait: time.Duration(v.pacInfo.APIRetryMaxWaitSeconds) * time.Second, + Logger: v.Logger, + } +} + // detectGHERawURL Detect if we have a raw URL in GHE. func detectGHERawURL(event *info.Event, taskHost string) bool { gheURL, err := url.Parse(event.GHEURL) @@ -235,13 +249,16 @@ func (v *Provider) GetConfig() *info.ProviderConfig { } } -func MakeClient(ctx context.Context, apiURL, token string) (*github.Client, string, *string, error) { +func MakeClient(ctx context.Context, apiURL, token string, retryOpts ...*retryhttp.Options) (*github.Client, string, *string, error) { var client *github.Client ts := oauth2.StaticTokenSource( &oauth2.Token{AccessToken: token}, ) tc := oauth2.NewClient(ctx, ts) + if len(retryOpts) > 0 && retryOpts[0] != nil { + tc.Transport = retryhttp.Wrap(tc.Transport, *retryOpts[0]) + } if apiURL != "" { if !strings.HasPrefix(apiURL, "https") && !strings.HasPrefix(apiURL, "http") { apiURL = "https://" + apiURL @@ -265,6 +282,11 @@ func MakeClient(ctx context.Context, apiURL, token string) (*github.Client, stri return client, providerName, github.Ptr(apiURL), nil } +// MakeClient creates a GitHub API client using the provider retry settings. +func (v *Provider) MakeClient(ctx context.Context, apiURL, token string) (*github.Client, string, *string, error) { + return MakeClient(ctx, apiURL, token, v.retryOptions()) +} + func parseTS(headerTS string) (time.Time, error) { ts := time.Time{} // Normal UTC: 2023-01-31 23:00:00 UTC @@ -326,7 +348,7 @@ func (v *Provider) checkWebhookSecretValidity(ctx context.Context, cw clockwork. } func (v *Provider) SetClient(ctx context.Context, run *params.Run, event *info.Event, repo *v1alpha1.Repository, eventsEmitter *events.EventEmitter) error { - client, providerName, apiURL, err := MakeClient(ctx, event.Provider.URL, event.Provider.Token) + client, providerName, apiURL, err := v.MakeClient(ctx, event.Provider.URL, event.Provider.Token) if err != nil { return err } @@ -1244,7 +1266,7 @@ func (v *Provider) fetchAppSlug(ctx context.Context, apiURL string) (string, err return "", err } - client, _, _, err := MakeClient(ctx, apiURL, tokenString) + client, _, _, err := v.MakeClient(ctx, apiURL, tokenString) if err != nil { return "", err } diff --git a/pkg/provider/github/parse_payload.go b/pkg/provider/github/parse_payload.go index 15744bd589..9652059aca 100644 --- a/pkg/provider/github/parse_payload.go +++ b/pkg/provider/github/parse_payload.go @@ -25,6 +25,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" + "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/retryhttp" "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" "github.com/openshift-pipelines/pipelines-as-code/pkg/sort" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -90,15 +91,21 @@ func (v *Provider) GetAppToken(ctx context.Context, kube kubernetes.Interface, g gheURL = strings.TrimSuffix(reqTokenURL, "/api/v3") } + // wrap the installation transport with the retry transport when enabled + var apiTransport http.RoundTripper = itr + if retryOpts := v.retryOptions(); retryOpts != nil { + apiTransport = retryhttp.Wrap(itr, *retryOpts) + } + if gheURL != "" { if !strings.HasPrefix(gheURL, "https://") && !strings.HasPrefix(gheURL, "http://") { gheURL = "https://" + gheURL } uploadURL := gheURL + "/api/uploads" - v.ghClient, _ = github.NewClient(&http.Client{Transport: itr}).WithEnterpriseURLs(gheURL, uploadURL) + v.ghClient, _ = github.NewClient(&http.Client{Transport: apiTransport}).WithEnterpriseURLs(gheURL, uploadURL) itr.BaseURL = strings.TrimSuffix(v.Client().BaseURL.String(), "/") } else { - v.ghClient = github.NewClient(&http.Client{Transport: itr}) + v.ghClient = github.NewClient(&http.Client{Transport: apiTransport}) } // Get a token ASAP because we need it for setting private repos diff --git a/pkg/provider/github/retry_test.go b/pkg/provider/github/retry_test.go new file mode 100644 index 0000000000..3e34a39824 --- /dev/null +++ b/pkg/provider/github/retry_test.go @@ -0,0 +1,56 @@ +package github + +import ( + "testing" + "time" + + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" + "gotest.tools/v3/assert" +) + +func TestRetryOptions(t *testing.T) { + tests := []struct { + name string + pacInfo *info.PacOpts + wantNil bool + wantMaxAttempts int + wantMaxWait time.Duration + }{ + { + name: "nil pacinfo", + pacInfo: nil, + wantNil: true, + }, + { + name: "disabled by default", + pacInfo: &info.PacOpts{Settings: settings.DefaultSettings()}, + wantNil: true, + }, + { + name: "enabled with settings", + pacInfo: &info.PacOpts{ + Settings: settings.Settings{ + EnableAPIRetry: true, + APIRetryMaxAttempts: 7, + APIRetryMaxWaitSeconds: 42, + }, + }, + wantMaxAttempts: 7, + wantMaxWait: 42 * time.Second, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := &Provider{pacInfo: tt.pacInfo} + opts := v.retryOptions() + if tt.wantNil { + assert.Assert(t, opts == nil) + return + } + assert.Assert(t, opts != nil) + assert.Equal(t, tt.wantMaxAttempts, opts.MaxAttempts) + assert.Equal(t, tt.wantMaxWait, opts.MaxWait) + }) + } +} diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index 09d5b8fdcc..aba97441eb 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -13,7 +13,9 @@ import ( "strconv" "strings" "sync" + "time" + retryablehttp "github.com/hashicorp/go-retryablehttp" "github.com/openshift-pipelines/pipelines-as-code/pkg/action" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" @@ -22,6 +24,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" providerMetrics "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/providermetrics" @@ -77,6 +80,8 @@ type Provider struct { pipelineIDMu sync.Mutex } +type retryMethodContextKey struct{} + var defaultGitlabListOptions = gitlab.ListOptions{ PerPage: 100, } @@ -101,6 +106,128 @@ func (v *Provider) SetPacInfo(pacInfo *info.PacOpts) { v.pacInfo = pacInfo } +// clientOptions returns the options used to create the gitlab client. +func (v *Provider) clientOptions(apiURL string) []gitlab.ClientOptionFunc { + opts := make([]gitlab.ClientOptionFunc, 0, 6) + opts = append(opts, gitlab.WithBaseURL(apiURL)) + // When the retry setting is off, keep the go-gitlab client defaults, which + // already retry, so that existing behaviour is preserved. + if v.pacInfo == nil || !v.pacInfo.EnableAPIRetry { + return opts + } + + maxAttempts := v.pacInfo.APIRetryMaxAttempts + if maxAttempts <= 0 { + maxAttempts = settings.DefaultAPIRetryMaxAttempts + } + maxWait := time.Duration(v.pacInfo.APIRetryMaxWaitSeconds) * time.Second + if maxWait <= 0 { + maxWait = settings.DefaultAPIRetryMaxWaitSeconds * time.Second + } + return append( + opts, + gitlab.WithCustomRetryMax(maxAttempts-1), + gitlab.WithCustomRetryWaitMinMax(time.Second, maxWait), + gitlab.WithCustomRetry(gitlabRetryPolicy(maxWait)), + gitlab.WithCustomBackoff(gitlabRetryBackoff(maxWait)), + gitlab.WithRequestOptions(gitlabRetryRequestOption), + ) +} + +// gitlabRetryRequestOption carries the request method in the request context so +// that gitlabRetryPolicy can tell idempotent requests apart when a network +// failure leaves no response to inspect. +// +// The value is dropped when a call passes gitlab.WithContext, because the +// upstream client only copies its own internal context keys. In that case the +// method is unknown and the request is treated as non-idempotent, so retries +// are skipped rather than risking a duplicated mutation. +func gitlabRetryRequestOption(req *retryablehttp.Request) error { + ctx := context.WithValue(req.Context(), retryMethodContextKey{}, req.Method) + *req = *req.WithContext(ctx) + return nil +} + +func gitlabRetryPolicy(maxWait time.Duration) retryablehttp.CheckRetry { + return func(ctx context.Context, resp *http.Response, err error) (bool, error) { + if ctx.Err() != nil { + return false, ctx.Err() + } + if resp != nil && resp.StatusCode == http.StatusTooManyRequests { + if wait, ok := gitlabRateLimitWait(resp); ok && wait > maxWait { + return false, nil + } + return true, nil + } + // Do not retry mutations after network or server errors because the + // server may already have applied the request. + method, _ := ctx.Value(retryMethodContextKey{}).(string) + if resp != nil && resp.Request != nil { + method = resp.Request.Method + } + if !isIdempotentMethod(method) { + return false, nil + } + return retryablehttp.DefaultRetryPolicy(ctx, resp, err) + } +} + +func isIdempotentMethod(method string) bool { + switch method { + case http.MethodGet, http.MethodHead, http.MethodOptions: + return true + default: + return false + } +} + +func gitlabRetryBackoff(maxWait time.Duration) retryablehttp.Backoff { + return func(minWait, _ time.Duration, attempt int, resp *http.Response) time.Duration { + // Only trust the rate limit headers on an actual rate limit response, + // GitLab sets RateLimit-Reset on ordinary responses too. + if resp != nil && resp.StatusCode == http.StatusTooManyRequests { + if wait, ok := gitlabRateLimitWait(resp); ok { + return addGitLabJitter(wait, minWait, maxWait) + } + } + // Transient failures use a short linear backoff with jitter, maxWait + // only bounds the upper end instead of being drawn from. + wait := retryablehttp.LinearJitterBackoff(minWait, 2*minWait, attempt, resp) + return min(wait, maxWait) + } +} + +func gitlabRateLimitWait(resp *http.Response) (time.Duration, bool) { + if resp == nil { + return 0, false + } + if value := resp.Header.Get("Retry-After"); value != "" { + if seconds, err := strconv.Atoi(value); err == nil { + return time.Duration(seconds) * time.Second, true + } + if retryAt, err := http.ParseTime(value); err == nil { + return max(time.Until(retryAt), 0), true + } + } + if value := resp.Header.Get("RateLimit-Reset"); value != "" { + if epoch, err := strconv.ParseInt(value, 10, 64); err == nil { + return max(time.Until(time.Unix(epoch, 0)), 0), true + } + } + return 0, false +} + +func addGitLabJitter(wait, maxJitter, maxWait time.Duration) time.Duration { + if wait >= maxWait { + return maxWait + } + remaining := maxWait - wait + if maxJitter > remaining { + maxJitter = remaining + } + return wait + retryablehttp.LinearJitterBackoff(0, maxJitter, 0, nil) +} + func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, updateMarker string) error { if v.gitlabClient == nil { return fmt.Errorf("no gitlab client has been initialized") @@ -256,7 +383,7 @@ func (v *Provider) setClient(ctx context.Context, run *params.Run, runevent *inf v.apiURL = apiURL if v.gitlabClient == nil { - v.gitlabClient, err = gitlab.NewClient(runevent.Provider.Token, gitlab.WithBaseURL(apiURL)) + v.gitlabClient, err = gitlab.NewClient(runevent.Provider.Token, v.clientOptions(apiURL)...) if err != nil { return err } @@ -292,7 +419,7 @@ func (v *Provider) setClient(ctx context.Context, run *params.Run, runevent *inf v.Logger.Warnf("gitlab token auto-rotation check failed: %v", rotateErr) } } else if newToken != "" { - v.gitlabClient, err = gitlab.NewClient(newToken, gitlab.WithBaseURL(apiURL)) + v.gitlabClient, err = gitlab.NewClient(newToken, v.clientOptions(apiURL)...) if err != nil { return fmt.Errorf("failed to create client with rotated token: %w", err) } diff --git a/pkg/provider/gitlab/retry_test.go b/pkg/provider/gitlab/retry_test.go new file mode 100644 index 0000000000..40cc693d88 --- /dev/null +++ b/pkg/provider/gitlab/retry_test.go @@ -0,0 +1,235 @@ +package gitlab + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strconv" + "sync/atomic" + "testing" + "time" + + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" + gitlabclient "gitlab.com/gitlab-org/api/client-go" + "gotest.tools/v3/assert" +) + +func TestClientOptions(t *testing.T) { + tests := []struct { + name string + pacInfo *info.PacOpts + wantOpts int + }{ + { + name: "nil pacinfo keeps client defaults", + pacInfo: nil, + wantOpts: 1, + }, + { + name: "disabled keeps client defaults", + pacInfo: &info.PacOpts{Settings: settings.DefaultSettings()}, + wantOpts: 1, + }, + { + name: "enabled adds retry options", + pacInfo: &info.PacOpts{ + Settings: settings.Settings{ + EnableAPIRetry: true, + APIRetryMaxAttempts: 7, + APIRetryMaxWaitSeconds: 42, + }, + }, + wantOpts: 6, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := &Provider{pacInfo: tt.pacInfo} + opts := v.clientOptions("https://gitlab.example.com") + assert.Equal(t, tt.wantOpts, len(opts)) + }) + } +} + +func TestClientRetryAttempts(t *testing.T) { + tests := []struct { + name string + enableRetry bool + maxAttempts int + wantCalls int64 + }{ + { + // go-gitlab retries by default, disabling the setting must not + // change that pre-existing behaviour. + name: "disabled keeps client default retries", + maxAttempts: 4, + wantCalls: 6, + }, + { + name: "enabled honors total attempt limit", + enableRetry: true, + maxAttempts: 4, + wantCalls: 4, + }, + { + name: "enabled with unset max attempts uses default", + enableRetry: true, + maxAttempts: 0, + wantCalls: settings.DefaultAPIRetryMaxAttempts, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var calls int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt64(&calls, 1) + w.Header().Set("Retry-After", "0") + w.Header().Set("RateLimit-Reset", strconv.FormatInt(time.Now().Add(-time.Hour).Unix(), 10)) + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + + v := &Provider{ + pacInfo: &info.PacOpts{ + Settings: settings.Settings{ + EnableAPIRetry: tt.enableRetry, + APIRetryMaxAttempts: tt.maxAttempts, + APIRetryMaxWaitSeconds: 1, + }, + }, + } + client, err := gitlabclient.NewClient("", v.clientOptions(server.URL)...) + assert.NilError(t, err) + + _, _, err = client.Users.ListUsers(nil) + assert.Assert(t, err != nil) + assert.Equal(t, tt.wantCalls, atomic.LoadInt64(&calls)) + }) + } +} + +func TestGitLabRetryWaitCap(t *testing.T) { + maxWait := 2 * time.Second + resp := &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: http.Header{"Retry-After": []string{"3600"}}, + } + + retry, err := gitlabRetryPolicy(maxWait)(t.Context(), resp, nil) + assert.NilError(t, err) + assert.Assert(t, !retry) + + wait := gitlabRetryBackoff(maxWait)(time.Second, maxWait, 0, resp) + assert.Assert(t, wait <= maxWait) +} + +func TestGitLabRetryBackoffTransient(t *testing.T) { + maxWait := 120 * time.Second + minWait := time.Second + tests := []struct { + name string + resp *http.Response + attempt int + wantMax time.Duration + }{ + { + // A transient failure must not be delayed by the rate limit + // window, only an actual 429 may use those headers. + name: "server error ignores rate limit headers", + resp: &http.Response{ + StatusCode: http.StatusInternalServerError, + Header: http.Header{ + "RateLimit-Reset": []string{strconv.FormatInt(time.Now().Add(time.Hour).Unix(), 10)}, + }, + }, + wantMax: 2 * minWait, + }, + { + name: "network failure without response stays short", + resp: nil, + wantMax: 2 * minWait, + }, + { + name: "later attempts grow but stay bounded", + resp: &http.Response{StatusCode: http.StatusBadGateway, Header: make(http.Header)}, + attempt: 2, + wantMax: 6 * minWait, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + wait := gitlabRetryBackoff(maxWait)(minWait, maxWait, tt.attempt, tt.resp) + assert.Assert(t, wait >= minWait, "wait %s below minimum", wait) + assert.Assert(t, wait <= tt.wantMax, "wait %s above expected %s", wait, tt.wantMax) + }) + } +} + +func TestGitLabRetryPolicyMethods(t *testing.T) { + tests := []struct { + name string + method string + status int + wantRetry bool + }{ + { + name: "retry server error for GET", + method: http.MethodGet, + status: http.StatusInternalServerError, + wantRetry: true, + }, + { + name: "do not retry server error for POST", + method: http.MethodPost, + status: http.StatusInternalServerError, + }, + { + name: "retry rate limit for POST", + method: http.MethodPost, + status: http.StatusTooManyRequests, + wantRetry: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resp := &http.Response{ + StatusCode: tt.status, + Header: make(http.Header), + Request: &http.Request{ + Method: tt.method, + }, + } + retry, err := gitlabRetryPolicy(time.Minute)(t.Context(), resp, nil) + assert.NilError(t, err) + assert.Equal(t, tt.wantRetry, retry) + }) + } +} + +func TestGitLabRetryPolicyNetworkErrors(t *testing.T) { + tests := []struct { + name string + method string + wantRetry bool + }{ + { + name: "retry network error for GET", + method: http.MethodGet, + wantRetry: true, + }, + { + name: "do not retry network error for POST", + method: http.MethodPost, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.WithValue(t.Context(), retryMethodContextKey{}, tt.method) + retry, err := gitlabRetryPolicy(time.Minute)(ctx, nil, errors.New("network failure")) + assert.NilError(t, err) + assert.Equal(t, tt.wantRetry, retry) + }) + } +} diff --git a/pkg/provider/gitlab/task.go b/pkg/provider/gitlab/task.go index d5690f2fb9..470ce8c856 100644 --- a/pkg/provider/gitlab/task.go +++ b/pkg/provider/gitlab/task.go @@ -84,7 +84,7 @@ func (v *Provider) GetTaskURI(_ context.Context, event *info.Event, uri string) if client == nil { baseURL := fmt.Sprintf("%s://%s", extracted.Scheme, extracted.Host) var clientErr error - client, clientErr = gl.NewClient(event.Provider.Token, gl.WithBaseURL(baseURL)) + client, clientErr = gl.NewClient(event.Provider.Token, v.clientOptions(baseURL)...) if clientErr != nil { return false, "", fmt.Errorf("failed to create gitlab client: %w", clientErr) } diff --git a/pkg/provider/retryhttp/retryhttp.go b/pkg/provider/retryhttp/retryhttp.go new file mode 100644 index 0000000000..242e00ac76 --- /dev/null +++ b/pkg/provider/retryhttp/retryhttp.go @@ -0,0 +1,185 @@ +// Package retryhttp provides a rate-limit-aware retrying http.RoundTripper +// for Git provider API clients. +package retryhttp + +import ( + "fmt" + "io" + "math/rand" + "net/http" + "strconv" + "time" + + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" + "go.uber.org/zap" +) + +const ( + defaultMaxAttempts = settings.DefaultAPIRetryMaxAttempts + defaultMaxWait = settings.DefaultAPIRetryMaxWaitSeconds * time.Second + initialBackoff = 1 * time.Second +) + +// Options configures the retry transport. +type Options struct { + // MaxAttempts is the maximum number of attempts (initial request included). + MaxAttempts int + // MaxWait caps the wait between attempts. If a rate limit reset is + // further away than MaxWait, the transport gives up instead of blocking. + MaxWait time.Duration + // Logger is optional, used to log retries. + Logger *zap.SugaredLogger +} + +type transport struct { + base http.RoundTripper + opts Options +} + +// Wrap returns a RoundTripper retrying rate-limited (429, or 403 with rate +// limit headers as used by GitHub) and transient (5xx on idempotent methods) +// failures with jittered exponential backoff. Retry-After and +// X-RateLimit-Reset headers are honored, capped at opts.MaxWait. +func Wrap(base http.RoundTripper, opts Options) http.RoundTripper { + if base == nil { + base = http.DefaultTransport + } + if opts.MaxAttempts <= 0 { + opts.MaxAttempts = defaultMaxAttempts + } + if opts.MaxWait <= 0 { + opts.MaxWait = defaultMaxWait + } + return &transport{base: base, opts: opts} +} + +func (t *transport) RoundTrip(req *http.Request) (*http.Response, error) { + canReplay := req.Body == nil || req.GetBody != nil + + var resp *http.Response + var err error + for attempt := 1; ; attempt++ { + if req.GetBody != nil && attempt > 1 { + if req.Body, err = req.GetBody(); err != nil { + return nil, err + } + } + + resp, err = t.base.RoundTrip(req) + if !canReplay || attempt >= t.opts.MaxAttempts || !t.shouldRetry(req, resp, err) { + return resp, err + } + + wait, ok := t.backoff(attempt, resp) + if !ok { + // reset is too far away to wait for + return resp, err + } + if resp != nil && resp.Body != nil { + _, _ = io.Copy(io.Discard, resp.Body) + resp.Body.Close() + } + + if t.opts.Logger != nil { + status := "network error" + if resp != nil { + status = fmt.Sprintf("status %d", resp.StatusCode) + } + t.opts.Logger.Infof("retrying %s %s after %s (attempt %d/%d): %s", + req.Method, req.URL.Path, wait.Round(time.Millisecond), attempt, t.opts.MaxAttempts, status) + } + + timer := time.NewTimer(wait) + select { + case <-req.Context().Done(): + timer.Stop() + return nil, req.Context().Err() + case <-timer.C: + } + } +} + +func (t *transport) shouldRetry(req *http.Request, resp *http.Response, err error) bool { + idempotent := req.Method == http.MethodGet || req.Method == http.MethodHead || req.Method == http.MethodOptions + if err != nil || resp == nil { + // network level errors: only retry idempotent requests, the server + // may have processed the request without us seeing the response. + return idempotent + } + switch { + case resp.StatusCode == http.StatusTooManyRequests: + return true + case resp.StatusCode == http.StatusForbidden && isRateLimited(resp): + // GitHub primary/secondary rate limits use 403 with rate limit headers. + return true + case resp.StatusCode >= 500 && resp.StatusCode != http.StatusNotImplemented: + return idempotent + } + return false +} + +func isRateLimited(resp *http.Response) bool { + if resp.Header.Get("Retry-After") != "" { + return true + } + return resp.Header.Get("X-RateLimit-Remaining") == "0" +} + +// backoff returns how long to wait before the next attempt. The boolean is +// false when the rate limit reset is further away than MaxWait, meaning we +// should give up rather than block. +func (t *transport) backoff(attempt int, resp *http.Response) (time.Duration, bool) { + if resp != nil { + if s := resp.Header.Get("Retry-After"); s != "" { + if secs, err := strconv.Atoi(s); err == nil { + wait := time.Duration(secs) * time.Second + if wait > t.opts.MaxWait { + return 0, false + } + return t.addJitter(wait, time.Second), true + } + } + if s := resp.Header.Get("X-RateLimit-Reset"); s != "" && resp.Header.Get("X-RateLimit-Remaining") == "0" { + if epoch, err := strconv.ParseInt(s, 10, 64); err == nil { + wait := time.Until(time.Unix(epoch, 0)) + if wait > t.opts.MaxWait { + return 0, false + } + if wait < 0 { + wait = 0 + } + return t.addJitter(wait, time.Second), true + } + } + } + wait := initialBackoff + for retry := 1; retry < attempt && wait < t.opts.MaxWait; retry++ { + if wait > t.opts.MaxWait/2 { + wait = t.opts.MaxWait + break + } + wait *= 2 + } + if wait > t.opts.MaxWait { + wait = t.opts.MaxWait + } + return t.addJitter(wait, wait/2), true +} + +func (t *transport) addJitter(wait, maxJitter time.Duration) time.Duration { + remaining := t.opts.MaxWait - wait + if remaining <= 0 { + return t.opts.MaxWait + } + if maxJitter > remaining { + maxJitter = remaining + } + return wait + jitter(maxJitter) +} + +func jitter(maxJitter time.Duration) time.Duration { + if maxJitter <= 0 { + return 0 + } + return time.Duration(rand.Int63n(int64(maxJitter))) //nolint: gosec +} diff --git a/pkg/provider/retryhttp/retryhttp_test.go b/pkg/provider/retryhttp/retryhttp_test.go new file mode 100644 index 0000000000..f4b504ebfb --- /dev/null +++ b/pkg/provider/retryhttp/retryhttp_test.go @@ -0,0 +1,250 @@ +package retryhttp + +import ( + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "gotest.tools/v3/assert" +) + +func TestRoundTrip(t *testing.T) { + tests := []struct { + name string + method string + body string + maxAttempts int + maxWait time.Duration + handler func(calls int64, w http.ResponseWriter, r *http.Request) + wantStatus int + wantCalls int64 + wantErr bool + wantBodyOnLast string + unreplayable bool + }{ + { + name: "no retry on success", + method: http.MethodGet, + maxAttempts: 3, + handler: func(_ int64, w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }, + wantStatus: http.StatusOK, + wantCalls: 1, + }, + { + name: "retries 429 until success", + method: http.MethodGet, + maxAttempts: 3, + handler: func(calls int64, w http.ResponseWriter, _ *http.Request) { + if calls < 3 { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + return + } + w.WriteHeader(http.StatusOK) + }, + wantStatus: http.StatusOK, + wantCalls: 3, + }, + { + name: "gives up after max attempts", + method: http.MethodGet, + maxAttempts: 2, + handler: func(_ int64, w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + }, + wantStatus: http.StatusTooManyRequests, + wantCalls: 2, + }, + { + name: "no retry on 404", + method: http.MethodGet, + maxAttempts: 3, + handler: func(_ int64, w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }, + wantStatus: http.StatusNotFound, + wantCalls: 1, + }, + { + name: "no retry on plain 403 without rate limit headers", + method: http.MethodGet, + maxAttempts: 3, + handler: func(_ int64, w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + }, + wantStatus: http.StatusForbidden, + wantCalls: 1, + }, + { + name: "retries github 403 rate limit", + method: http.MethodGet, + maxAttempts: 3, + handler: func(calls int64, w http.ResponseWriter, _ *http.Request) { + if calls < 2 { + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Reset", fmt.Sprintf("%d", time.Now().Unix())) + w.WriteHeader(http.StatusForbidden) + return + } + w.WriteHeader(http.StatusOK) + }, + wantStatus: http.StatusOK, + wantCalls: 2, + }, + { + name: "gives up when reset is beyond max wait", + method: http.MethodGet, + maxAttempts: 3, + maxWait: 1 * time.Second, + handler: func(_ int64, w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "3600") + w.WriteHeader(http.StatusTooManyRequests) + }, + wantStatus: http.StatusTooManyRequests, + wantCalls: 1, + }, + { + name: "retries 500 on GET", + method: http.MethodGet, + maxAttempts: 3, + handler: func(calls int64, w http.ResponseWriter, _ *http.Request) { + if calls < 2 { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + }, + wantStatus: http.StatusOK, + wantCalls: 2, + }, + { + name: "no retry of 500 on POST", + method: http.MethodPost, + body: "hello", + maxAttempts: 3, + handler: func(_ int64, w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }, + wantStatus: http.StatusInternalServerError, + wantCalls: 1, + }, + { + name: "replays POST body on 429 retry", + method: http.MethodPost, + body: "hello", + maxAttempts: 3, + handler: func(calls int64, w http.ResponseWriter, _ *http.Request) { + if calls < 2 { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + return + } + w.WriteHeader(http.StatusOK) + }, + wantStatus: http.StatusOK, + wantCalls: 2, + wantBodyOnLast: "hello", + }, + { + name: "does not truncate or retry unreplayable body", + method: http.MethodPost, + body: strings.Repeat("x", 3*1024*1024), + maxAttempts: 3, + unreplayable: true, + handler: func(_ int64, w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + }, + wantStatus: http.StatusTooManyRequests, + wantCalls: 1, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var calls int64 + var lastBody atomic.Value + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt64(&calls, 1) + b, _ := io.ReadAll(r.Body) + lastBody.Store(string(b)) + tt.handler(n, w, r) + })) + defer srv.Close() + + client := &http.Client{Transport: Wrap(nil, Options{MaxAttempts: tt.maxAttempts, MaxWait: tt.maxWait})} + var reqBody io.Reader + if tt.body != "" { + reqBody = strings.NewReader(tt.body) + if tt.unreplayable { + reqBody = io.NopCloser(reqBody) + } + } + + req, err := http.NewRequestWithContext(t.Context(), tt.method, srv.URL, reqBody) + assert.NilError(t, err) + resp, err := client.Do(req) + if tt.wantErr { + assert.Assert(t, err != nil) + return + } + assert.NilError(t, err) + defer resp.Body.Close() + assert.Equal(t, tt.wantStatus, resp.StatusCode) + assert.Equal(t, tt.wantCalls, atomic.LoadInt64(&calls)) + if tt.unreplayable { + got, ok := lastBody.Load().(string) + assert.Assert(t, ok) + assert.Equal(t, tt.body, got) + } + if tt.wantBodyOnLast != "" { + got, ok := lastBody.Load().(string) + assert.Assert(t, ok) + assert.Equal(t, tt.wantBodyOnLast, got) + } + }) + } +} + +func TestGetBodyFailureDoesNotReturnClosedResponse(t *testing.T) { + var calls int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt64(&calls, 1) + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, server.URL, strings.NewReader("body")) + assert.NilError(t, err) + req.GetBody = func() (io.ReadCloser, error) { + return nil, errors.New("cannot rebuild body") + } + + client := &http.Client{Transport: Wrap(nil, Options{MaxAttempts: 2})} + resp, err := client.Do(req) + assert.ErrorContains(t, err, "cannot rebuild body") + if resp != nil { + resp.Body.Close() + } + assert.Assert(t, resp == nil) + assert.Equal(t, int64(1), atomic.LoadInt64(&calls)) +} + +func TestBackoffLargeAttemptDoesNotOverflow(t *testing.T) { + maxWait := 2 * time.Second + tr := &transport{opts: Options{MaxWait: maxWait}} + + wait, ok := tr.backoff(1_000_000, nil) + assert.Assert(t, ok) + assert.Assert(t, wait >= 0) + assert.Assert(t, wait <= maxWait) +}