From b52e222741c084bf4d22a6dd3a7a733542f63054 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Fri, 31 Jul 2026 12:26:28 +0200 Subject: [PATCH] feat: add gitlab/github provider API retries Allow administrators to opt in to retrying temporary GitHub and GitLab API failures. This prevents short rate-limit windows and provider outages from immediately dropping webhook work that could succeed a few moments later. Keep the feature disabled by default so existing installations retain their current behavior. With the setting off, GitHub performs no retries and GitLab keeps the retry behavior of the upstream GitLab client. When enabled, administrators can control how many times an operation is attempted and how long PAC may wait for the provider to recover, and both providers read those settings the same way. Spread retries over time rather than sending every request again at once. Use the provider rate-limit headers only on an actual rate-limit response and give other temporary failures a short bounded delay. Stop retrying when the provider asks PAC to wait longer than the configured limit, avoiding long-running webhook work and reducing the risk of releasing a large backlog in one wave. Apply the same behavior consistently to GitHub and GitLab operations, including GitHub App setup and temporary provider clients. Avoid repeating requests when doing so could accidentally create duplicate changes on the provider. Document the new configuration and cover enabled, disabled, exhausted, and successful retry scenarios with automated tests. A full end-to-end test is not included because safely forcing rate limits on shared live provider accounts is disruptive and unreliable. Co-Authored-By: Claude Jira: https://issues.redhat.com/browse/SRVKP-12884 Signed-off-by: Chmouel Boudjnah --- config/302-pac-configmap.yaml | 21 ++ docs/content/docs/api/configmap.md | 66 ++++++ go.mod | 2 +- pkg/adapter/incoming.go | 4 + pkg/params/settings/config.go | 13 ++ pkg/params/settings/config_test.go | 4 + pkg/provider/github/app/token.go | 2 +- pkg/provider/github/github.go | 28 ++- pkg/provider/github/parse_payload.go | 11 +- pkg/provider/github/retry_test.go | 56 +++++ pkg/provider/gitlab/gitlab.go | 131 +++++++++++- pkg/provider/gitlab/retry_test.go | 235 +++++++++++++++++++++ pkg/provider/gitlab/task.go | 2 +- pkg/provider/retryhttp/retryhttp.go | 185 +++++++++++++++++ pkg/provider/retryhttp/retryhttp_test.go | 250 +++++++++++++++++++++++ 15 files changed, 1000 insertions(+), 10 deletions(-) create mode 100644 pkg/provider/github/retry_test.go create mode 100644 pkg/provider/gitlab/retry_test.go create mode 100644 pkg/provider/retryhttp/retryhttp.go create mode 100644 pkg/provider/retryhttp/retryhttp_test.go 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) +}